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

bulkCreate updateOnDuplicate createdAt field explicitly omitted #17347

Open
3 of 7 tasks
LJ1102 opened this issue May 29, 2024 · 3 comments
Open
3 of 7 tasks

bulkCreate updateOnDuplicate createdAt field explicitly omitted #17347

LJ1102 opened this issue May 29, 2024 · 3 comments
Labels
pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug

Comments

@LJ1102
Copy link
Contributor

LJ1102 commented May 29, 2024

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

When using bulkCreate to create records but wanting to update automatic keys (like the timestamps) on duplicates, an invalid query is generated.

Reproducible Example

await Foo.bulkCreate(
  [{ id: 'e8388762-1daa-11ef-b9f0-b7d47c40e7e3' }],
  { updateOnDuplicate: ['createdAt'] }
);

Here is the link to the SSCCE for this issue:
https://github.com/sequelize/sequelize-sscce/pull/277/files
https://github.com/LJ1102/sequelize-sscce/tree/main

What do you expect to happen?

Create row with given id or update createdAt timestamp of instance matching the id.

What is actually happening?

Generates a broken query:

INSERT INTO `Foos` (`id`,`createdAt`,`updatedAt`) VALUES ('e8388762-1daa-11ef-b9f0-b7d47c40e7e3','2024-05-29 11:23:02.787  00:00','2024-05-29 11:23:02.787  00:00') ON CONFLICT (`id`) DO UPDATE SET; 

Environment

  • Sequelize version: 6.x
  • Node.js version: any
  • Database & Version: any

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • Yes, I have the time but I cannot run the testing toolchain locally.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as " 1" will be removed.

@LJ1102 LJ1102 added pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug labels May 29, 2024
@LJ1102
Copy link
Contributor Author

LJ1102 commented Jun 6, 2024

Looking to get feedback / approval for this bug before I put time into it.

@LJ1102 LJ1102 changed the title bulkCreate updateOnDuplicate automatic fields not accounted for bulkCreate updateOnDuplicate createdAt field explicitly omitted Jun 13, 2024
@LJ1102
Copy link
Contributor Author

LJ1102 commented Jun 13, 2024

I did some digging and found that the createdAt timestamp is explicitly removed / omitted from the updateOnDuplicate array, see:

_.without(Object.keys(model.tableAttributes), createdAtAttr),

This change was introduced with commit 0ce88c4. Initially updateOnDuplicate had a boolean option to enable all fields, in that scenario it can be assumed that we don't want to implicitly update the createdAt timestamp, however this functionality is not supported anymore, as of 8b937dd updateOnDuplicate is required to be an array of field names, in which case we can assume that if the user provides thecreatedAt field they want it to be updated, like I do, hence we can remove the exclusion of createdAt.

LJ1102 pushed a commit to LJ1102/sequelize that referenced this issue Jun 13, 2024
LJ1102 pushed a commit to LJ1102/sequelize that referenced this issue Jun 13, 2024
LJ1102 pushed a commit to LJ1102/sequelize that referenced this issue Jun 13, 2024
@LJ1102
Copy link
Contributor Author

LJ1102 commented Jun 27, 2024

I came to realize that timestamps have a few more special cases, i.e. one can also not simply "touch" an instance via myInstance.update({ updatedAt: new Date() }) which I find rather unintuitive.

sequelize/src/model.js

Lines 3343 to 3347 in b9e71a7

} else if (_.isEmpty(valuesUse)
|| Object.keys(valuesUse).length === 1 && valuesUse[this._timestampAttributes.updatedAt]) {
// only updatedAt is being passed, then skip update
result = [0];
} else {

I think we should honor the users request to update timestamps when explicitly asked to do so rather than silently omitting the update. I'd be willing to implement the required changes if the maintainers concur with this sentiment.

As a workaround one can disable the timestamps and explicitly specify and configure / augment the fields in the model definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug
Projects
None yet
Development

No branches or pull requests

1 participant