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(postgres): update upsert regex to match the last RETURNING * #11538

Merged

Conversation

Americas
Copy link
Contributor

@Americas Americas commented Oct 11, 2019

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Fixes #11523

Postgres' upsert query generator replaces the first RETURNING * it finds on the insert and update query, so if the data being added has that string, it replaces it (so it replaces the wrong RETURNING *) changing the input data and messing with the the final query.

This PR updates the replace method to use a regex with a negative lookahead so it only chooses the last RETURNING * in the query.

@Americas Americas force-pushed the bugfix/fix-postgres-upsert-replace branch from 72f80a1 to 6967bbb Compare October 11, 2019 09:40
@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #11538 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11538      +/-   ##
==========================================
+ Coverage   96.27%   96.27%   +<.01%     
==========================================
  Files          94       94              
  Lines        9185     9187       +2     
==========================================
+ Hits         8843     8845       +2     
  Misses        342      342
Impacted Files Coverage Δ
lib/dialects/postgres/query-generator.js 94.38% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd59b87...1655b1f. Read the comment docs.

@Americas Americas force-pushed the bugfix/fix-postgres-upsert-replace branch from 6f75bc3 to f58b66e Compare October 11, 2019 10:28
Copy link
Member

@papb papb left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

@papb
Copy link
Member

papb commented Oct 11, 2019

I have edited

Fixes issue #11523.

to the following

Fixes #11523

so that GitHub detects the fix automatically:

image

@papb papb added dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). status: awaiting maintainer type: bug labels Oct 11, 2019
@papb
Copy link
Member

papb commented Oct 11, 2019

By the way, I have a question, was the SSCCE (papb/sequelize-sscce#9) useful to you? Or do you think you could have fixed the issue as easily without it?

@Americas
Copy link
Contributor Author

By the way, I have a question, was the SSCCE (papb/sequelize-sscce#9) useful to you? Or do you think you could have fixed the issue as easily without it?

I used it yes, just changed the log to an error if the result was different from the expected. It helps to have the problem easily available, otherwise there is this great setup cost to start analyzing the issue 😄

@papb
Copy link
Member

papb commented Oct 11, 2019

@Americas Nice! I'm happy that my idea of an SSCCE repo was useful then :)

@papb
Copy link
Member

papb commented Oct 11, 2019

I was thinking... Would you be willing to add an explicit test for the JSONB case as well?

@papb papb added status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action and removed status: awaiting maintainer labels Oct 11, 2019
Copy link
Contributor

@SimonSchick SimonSchick left a comment

Choose a reason for hiding this comment

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

Looks good.

insert.query = insert.query.replace('RETURNING *', `RETURNING ${primaryField} INTO primary_key`);
update.query = update.query.replace('RETURNING *', `RETURNING ${primaryField} INTO primary_key`);
if (options.returning) {
insert.query = insert.query.replace(/RETURNING \*(?![\s\S]*RETURNING \*)/, `RETURNING ${primaryField} INTO primary_key`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Regex should be moved to a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Americas Americas force-pushed the bugfix/fix-postgres-upsert-replace branch from f58b66e to 1655b1f Compare October 12, 2019 08:40
@Americas
Copy link
Contributor Author

I was thinking... Would you be willing to add an explicit test for the JSONB case as well?

Added JSON example from SSCCE and a normal example.

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Oct 12, 2019
@sushantdhiman sushantdhiman merged commit 2b9baa2 into sequelize:master Oct 18, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). released type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On postgres, unable to upsert text containing "RETURNING *"
4 participants