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.

Sign up for GitHub

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

Jump to bottom

Copy query expression to guard against subsequent in-place edits to input expression #274

Merged
merged 3 commits into from
Feb 29, 2020
Merged

Copy query expression to guard against subsequent in-place edits to input expression #274

merged 3 commits into from
Feb 29, 2020

Conversation

mskeving
Copy link
Contributor

@mskeving mskeving commented Feb 28, 2020 •
edited
Loading

Use deep copy of expression in Query constructor to prevent unintentional edits.

@mskeving
Prevent using saved query if new expression doesn't match hash
bc23444 
If a query expression is altered, the hash remains the same, which
means new queries matching the hash will use a query expression that
has been altered, resulting in unexpected results.

Adds extra check to ensure expressions match, otherwise a new query
will be added.
ericyhwang
ericyhwang reviewed Feb 28, 2020
View reviewed changes
Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

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

I think it would be safer to instead do a deep-copy of the expression in the Query constructor. That way, re-fetching an existing query would use the original query parameters.

Also, Travis is failing due to some lint issues, npm eslint --ignore-path .gitignore --fix . should fix it up.

@mskeving
Copy link
Contributor Author

mskeving commented Feb 29, 2020 •
edited
Loading

@ericyhwang thanks. I went down that route originally, but thought the issue was coming earlier in the _getOrCreateQuery step of the query, preventing the query from being created in the first place. Turns out the way I had written my tests caused me to go down that path.

I updated tests for this scenario and tested locally and this does fix the issue 🎉

Still working on getting eslint to run to fix linting...

@ericyhwang
Copy link
Contributor

Sorry, the command should be npx eslint --ignore-path .gitignore --fix . - npx runs a NPM binary installed under node_modules/.bin/

ericyhwang
ericyhwang approved these changes Feb 29, 2020
View reviewed changes
Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for finding and fixing!

@ericyhwang ericyhwang merged commit fd5632a into derbyjs:master Feb 29, 2020
@ericyhwang ericyhwang changed the title Prevent using saved query if new expression doesn't match hash Copy query expression to guard against subsequent in-place edits to input expression Feb 29, 2020
@ericyhwang
Copy link
Contributor

Published to NPM as racer@0.9.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericyhwang ericyhwang ericyhwang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mskeving @ericyhwang