-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update neo4j-driver #2546
Update neo4j-driver #2546
Conversation
Fix API changes. Also close the session in resolver only and refactor userMiddleware to become a part of the resolver.
… into update_neo4j_driver
Fix API changes. Also close the session in resolver only and refactor userMiddleware to become a part of the resolver.
…an-Connection into update_neo4j_driver
… into update_neo4j_driver
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
return result | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authored by mattwr18
Dec 19, 2019
Outdated (history rewrite) - original diff
@@ -1,16 +0,0 @@
-import createOrUpdateLocations from '../nodes/locations'
-
-export default {
- Mutation: {
- SignupVerification: async (resolve, root, args, context, info) => {
- const result = await resolve(root, args, context, info)
- await createOrUpdateLocations(result.id, args.locationName, context.driver)
- return result
- },
- UpdateUser: async (resolve, root, args, context, info) => {
- const result = await resolve(root, args, context, info)
- await createOrUpdateLocations(args.id, args.locationName, context.driver)
- return result
- },
- },
-}
why are these changes in this PR
@roschaefer
code creep?
return user.toJson() | ||
} catch (e) { | ||
if (e.code === 'Neo.ClientError.Schema.ConstraintValidationFailed') | ||
throw new UserInputError('User with this slug already exists!') | ||
throw new UserInputError(e.message) | ||
} finally { | ||
session.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authored by mattwr18
Dec 19, 2019
Outdated (history rewrite) - original diff
@@ -51,11 +54,14 @@ export default {
emailAddress.relateTo(user, 'belongsTo'),
emailAddress.update({ verifiedAt: new Date().toISOString() }),
])
+ await createOrUpdateLocations(args.id, args.locationName, session)
return user.toJson()
} catch (e) {
if (e.code === 'Neo.ClientError.Schema.ConstraintValidationFailed')
throw new UserInputError('User with this slug already exists!')
throw new UserInputError(e.message)
+ } finally {
+ session.close()
maybe these make sense here, if so, please explain...
if not, please open an issue to refactor, then add them there where our future selves can find them more easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authored by roschaefer
Dec 19, 2019
The signature of createOrUpdateLocations
has changed. It is now expecting session and not driver. When I updated I got complaints that a session was already closed. So I re-use the same session and close it at the end.
}) | ||
} finally { | ||
session.close() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authored by mattwr18
Dec 19, 2019
Outdated (history rewrite) - original diff
@@ -110,44 +105,36 @@ const createOrUpdateLocations = async (userId, locationName, driver) => {
if (data.context) {
await asyncForEach(data.context, async ctx => {
await createLocation(session, ctx)
- try {
- await session.writeTransaction(transaction => {
- return transaction.run(
- `
+ await session.writeTransaction(transaction => {
+ return transaction.run(
+ `
MATCH (parent:Location {id: $parentId}), (child:Location {id: $childId})
MERGE (child)<-[:IS_IN]-(parent)
RETURN child.id, parent.id
`,
- {
- parentId: parent.id,
- childId: ctx.id,
- },
- )
- })
- parent = ctx
- } finally {
- session.close()
- }
+ {
+ parentId: parent.id,
+ childId: ctx.id,
+ },
+ )
+ })
+ parent = ctx
})
}
// delete all current locations from user and add new location
- try {
- await session.writeTransaction(transaction => {
- return transaction.run(
- `
+ await session.writeTransaction(transaction => {
+ return transaction.run(
+ `
MATCH (user:User {id: $userId})-[relationship:IS_IN]->(location:Location)
DETACH DELETE relationship
WITH user
- MATCH (location:Location {id: $locationId})
- MERGE (user)-[:IS_IN]->(location)
+ MATCH (location:Location {id: $locationId})
+ MERGE (user)-[:IS_IN]->(location)
RETURN location.id, user.id
`,
- { userId: userId, locationId: data.id },
- )
- })
- } finally {
- session.close()
- }
why are you no longer closing the session here in this function?
is it closed later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes it harder to review when there are unrelated changes @roschaefer
please split them 😸
Fix API changes. Also close the session in resolver only and refactor
userMiddleware to become a part of the resolver.
The fix for the test was to move
session.close
into the finally block at the end of the resolver. It was complaining about using a session inuserMiddleware.js
which was already closed.