fix(bulkcreate): use correct PK column names in updateOnDuplicate

[PeledYuval]

Reopened accidentally closed PR: #11433

Pull Request check-list

• 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)? - no need
• Did you update the typescript typings accordingly (if applicable)? - not applicable
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This pull request fixes the following issue:
#11432

When performing bulkCreate with updateOnDuplicate using Postgres dialect, the generated query has bad column names. The query has the Javascript model's field names instead of the column names.

[PeledYuval]

I'm debating where to add tests for this issue, since it relates to model.js and Postgres dialect:

• test/unit/model/bulkCreate.test.js? (but this is dialect-agnostic and my fix is PG centric)
• test/unit/dialects/postgres/model.test.js? (which would be a new file)
• test/integration/model/bulkCreate.test.js? (dialect-agnostic)
• test/integration/dialects/postgres/model.test.js? (which would be a new file)

Please advise and I will add appropriate tests.

[papb]

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future! 😬

As for the test, I think you should do the third option. It makes sense to be dialect agnostic to me because updateOnDuplicate should work on all dialects it is supported. So although it works on MySQL and MariaDB already, it should work on every supported dialect (i.e. every dialect except MSSQL).

Therefore I think the best option is your third one (test/integration/model/bulkCreate.test.js). Thanks!!

[papb]

I have tagged it as SQLite as well because I reproduced the same problem on SQLite. I would guess your suggested fix already fixes SQLite as well, but would you be willing to double check? And fix both in the same PR? Thank you very much!!

[PeledYuval]

Sure, thanks for the quick response. I'm on it

[PeledYuval]

Hi @papb,

I've added a test for this fix.
Since this is my first contribution, the idea was to keep the current tests unchanged. Since the current tests didn't a have a Model fit for this test (one that has a primary key with a different field name and column name) I created a new Model.

I also grouped two relevant tests under a single new describe - one preexisting test which I left unchanged (other than nesting in a describe) and my added test.

I ran the tests with Postgres, Sqlite and Mysql and they pass.
Happily, the test fails when my fix is reverted :)

Is there anything you'd like me to add/change before this can be merged?

[PeledYuval]

I see the Travis build is failing but after a short investigation I don't see how it has anything to do with this PR.

I'm continuing to investigate, but on the chance that this is caused by a flaky test- could I trigger a rebuild? I couldn't find it in the Travis UI.

Thanks!

[codecov]

Codecov Report

Merging #11434 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #11434   +/-   ##
=======================================
  Coverage   92.49%   92.49%           
=======================================
  Files          91       91           
  Lines        8866     8866           
=======================================
  Hits         8201     8201           
  Misses        665      665

Impacted Files	Coverage Δ
lib/model.js	96.46% <100%> (ø)	⬆️

---

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 d5e4a10...9a2a920. Read the comment docs.

[PeledYuval]

Alright, I pushed an inconsequential commit to trigger a rebuild (I was a little impatient...) and it does seem like a flaky test, the build passes now.

[papb]

Excellent, great work!!

[sushantdhiman]

🎉 This PR is included in version 5.19.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀