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(model): updateOnDuplicate handles composite keys now #11984

Merged
merged 2 commits into from Mar 16, 2020

Conversation

thaddeusmt
Copy link
Contributor

@thaddeusmt thaddeusmt commented Mar 8, 2020

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

I noticed that my original PR #11163 to support updateOnDuplicate for Postgres (and subsequent community updates since then) isn't working with composite unique keys. This fixes that, I think.

This bug appears to have been raised in issues #11569 and #11534, and maybe #11665?

I think PR #11611 is supposed to fix this? But it looks like it also adds additional functionality (upsertIndex and upsertKeys options on bulkCreate()), and has not been merged yet.

I added 2 tests as well: one for composite primaryKey bulkCreates, and one for composite unique bulkCreates. All Postgres tests appear to be passing.

@codecov
Copy link

@codecov codecov bot commented Mar 8, 2020

Codecov Report

Merging #11984 into master will increase coverage by 6.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11984      +/-   ##
==========================================
+ Coverage   90.11%   96.23%   +6.12%     
==========================================
  Files          93       95       +2     
  Lines        9106     9202      +96     
==========================================
+ Hits         8206     8856     +650     
+ Misses        900      346     -554     
Impacted Files Coverage Δ
lib/model.js 96.55% <100.00%> (+0.06%) ⬆️
lib/dialects/mssql/index.js 100.00% <0.00%> (ø)
lib/dialects/mssql/connection-manager.js 87.01% <0.00%> (ø)
lib/query-interface.js 92.19% <0.00%> (+0.48%) ⬆️
lib/sequelize.js 95.93% <0.00%> (+0.62%) ⬆️
lib/dialects/abstract/query-generator.js 97.12% <0.00%> (+1.26%) ⬆️
...dialects/abstract/query-generator/helpers/quote.js 100.00% <0.00%> (+6.66%) ⬆️
lib/dialects/mssql/data-types.js 100.00% <0.00%> (+69.87%) ⬆️
lib/dialects/mssql/resource-lock.js 100.00% <0.00%> (+77.77%) ⬆️
... and 3 more

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 b2a2134...dadaed1. Read the comment docs.

@cxcorp
Copy link

@cxcorp cxcorp commented Mar 23, 2020

Will this fix be released under v5?

@JPeer264
Copy link

@JPeer264 JPeer264 commented Mar 31, 2020

@cxcorp I am not sure if it will get fixed in v5 as well but there are very little breaking changes in v6 (yet) so the migration from v5 to v6 should be nearly seamless.

@AppElent
Copy link

@AppElent AppElent commented Apr 9, 2020

Is this fix released already? I wanted to bulkCreate something with a composite primary key, but it keeps failing me. I am using postgres. Have tried both v5 and v6 of sequelize.
bulkCreate(data) --> returns SequelizeUniqueConstraintError: Validation error
bulkCreate(data, updateOnDuplicate: ['column1', column2]) --> returns SequelizeDatabaseError: syntax error at or near ")"

If not (going to be soon) released. What is the workaround?

@JPeer264
Copy link

@JPeer264 JPeer264 commented Apr 9, 2020

@ericjansen1988 no, not yet it is just on master right now.

There are two possibilities. But those possibilities should just be done if you know what you are doing and which effects it could have for the future:

  1. You can install the version of the master, you could also specify the commit SHA (this is what we do currently):
$ npm i https://github.com/sequelize/sequelize

But I just recommend this if you are aware of the risks pulling from master directly (use a package-lock too just in case). This version is still in Beta, there might be other bugs appearing - which I don't think so, but the risk is still there. Remember, if you might update to stable v6 the official API could be changed.


  1. Create a fork from a stable v5 version and cherry-pick this commit into your forked codebase -> Then install your own fork, also via Github to not spam the npm registry with forks

This could be leading to merging conflicts, depending on the codebase.

@@ -2708,7 +2708,7 @@ class Model {
// Get primary keys for postgres to enable updateOnDuplicate
options.upsertKeys = _.chain(model.primaryKeys).values().map('field').value();
if (Object.keys(model.uniqueKeys).length > 0) {
options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length === 1).map(c => c.fields[0]).value();
options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length >= 1).map(c => c.fields).reduce(c => c[0]).value();
Copy link
Contributor

@SimonSchick SimonSchick Apr 24, 2020

Choose a reason for hiding this comment

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

I'm confused about the reduce here, would this not effectively end up returning undefined all the time?

cc @eseliger @sushantdhiman

Copy link
Member

@eseliger eseliger Apr 24, 2020

Choose a reason for hiding this comment

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

I think you are correct. The initializer for the reduce is undefined (not specified), and c is the previous argument, so it would effectively always return the undefined, leaving the actual values untouched.

Choose a reason for hiding this comment

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

Due to the .reduce updateOnDuplicate does not work for Composite unique keys or multiple unique keys. Can you recheck for the same.

@ridakk
Copy link

@ridakk ridakk commented May 7, 2020

when will this fix be released? any plan or a date?

@klausXR
Copy link

@klausXR klausXR commented Jun 9, 2020

Is there any workaround or a temporary patch for v5?
I think this is a pretty useful feature for a lot of people, I have to do this in multiple parts of my project, I've been keeping an eye on this issue, its fixed in v6, but thats in beta, I can't just deploy that to production.

@brianschardt
Copy link
Contributor

@brianschardt brianschardt commented Jun 12, 2020

This is a much needed feature when can we get this released?

@brianschardt
Copy link
Contributor

@brianschardt brianschardt commented Jun 12, 2020

Can we get this change in version 5?

@klausXR
Copy link

@klausXR klausXR commented Jun 13, 2020

The issue, as I understand it, is that only conflicts on the primary key, as of now, will trigger the update on duplicate part, so if you have a primary key and a unique constraint, only the primary key will be taken into account, because it only gets the first one, so having more than one is no good.
In my case, my tables have two columns which together form uniqueness, so I used composite primary keys to solve the problem - I ended up with a single primary key, but it was a combination of two columns. What this solves is the need for having PK + Unique constraint.

As a simple example, if you have the following table

players
name teamId

and for the sake of simplicity we assume that a team cannot have two players with the same name, instead of adding the unique constraint, you can add a composite primary key like so

ALTER TABLE "players" ADD CONSTRAINT "pk_name_goes_here" PRIMARY KEY ("name", "teamId")

This doesn't solve case-insensitivity - I used foreign keys, so it wasn't an issue for me, but you might want to take this into account if its an issue for you.

@Jplus2
Copy link

@Jplus2 Jplus2 commented Jun 19, 2020

Can we get this on V5 please

@brianschardt
Copy link
Contributor

@brianschardt brianschardt commented Jul 4, 2020

what can i do to get this in v5?

@brianschardt
Copy link
Contributor

@brianschardt brianschardt commented Jul 16, 2020

Just tried this on v6 and this will fail if the composite is a unique index. as the this.uniqueKeys does not pickup composit indexes. So it looks like there is still some more work to do on this front.

@aecorredor
Copy link
Contributor

@aecorredor aecorredor commented Jul 19, 2020

@brianschardt I can confirm it's still an issue for me as well for your same use case. Any other workarounds to achieve the same goal?

@brianschardt
Copy link
Contributor

@brianschardt brianschardt commented Jul 22, 2020

This PR #12516 I just created should fix this issue
Lets encourage the sequelize team to get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet