Skip to content
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

Fix :auto for multistatement queries #1561

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

OskarDamkjaer
Copy link
Contributor

@OskarDamkjaer OskarDamkjaer commented Oct 13, 2021

Multistatements with :auto would get ignored since they were not in the list of allowed multistatement commands. Preview @ http://fix_auto_multlistatement.surge.sh

This query can be used to verify that the fix works (given at least two nodes in the db it creates two nodes with label Test). Make sure you've got multistatments enabled (setting in the sidebar)

:auto MATCH (n)
WITH n
LIMIT 1
CREATE (t:Test {zid: id(n)})
RETURN t;
:auto MATCH (n)
WITH n
SKIP 1
LIMIT 1
CREATE (t:Test {zid: id(n)})
RETURN t;

The customer who reported the issue also tested it in production for the issue they ran into and confirmed the solution.

@OskarDamkjaer OskarDamkjaer changed the base branch from cooldown to master October 15, 2021 12:01
@jharris4
Copy link
Collaborator

This appears to fix at least the basic use cases, so was there anything we want to do to test that it works for any more complicated cases?

@OskarDamkjaer
Copy link
Contributor Author

I've tried a few different queries I got from Greg, and the customer has supposedly tested all the cases they care about. This change doesn't change how we parse the input from the user, only changes what of the parsed commands are allowed to run, so there's no risk of introducing new syntax errors

@jharris4
Copy link
Collaborator

Thanks for the update. Should we add some e2e tests for auto with single & multi-statement though?

@OskarDamkjaer
Copy link
Contributor Author

Single statement :auto is tested in loadcsv.spec.ts, I'll add a test for multi-statement :auto as well

Copy link
Collaborator

@jharris4 jharris4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the multi-statement test, LGTM

@OskarDamkjaer OskarDamkjaer merged commit d215643 into neo4j:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants