-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: make bulkWrite can work with timestamps: false
#10050
fix: make bulkWrite can work with timestamps: false
#10050
Conversation
lib/helpers/model/castBulkWrite.js
Outdated
@@ -20,7 +20,7 @@ module.exports = function castBulkWrite(originalModel, op, options) { | |||
const model = decideModelByObject(originalModel, op['insertOne']['document']); | |||
|
|||
const doc = new model(op['insertOne']['document']); | |||
if (model.schema.options.timestamps != null) { | |||
if (doc.initializeTimestamps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be if (model.schema.options.timestamps)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered using it at first, but I tried to find where is also using doc.initializeTimestamps()
, and I found this:
Lines 3324 to 3326 in 9cf48f5
if (doc.initializeTimestamps) { | |
return doc.initializeTimestamps().toObject(internalToObjectOptions); | |
} |
So I think it's better to use the same condition, if you need to change it in the future, it will be easier to find where you need to edit.
But it just my opinion, if you think it's better to use if (model.schema.options.timestamps)
, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is meant to handle a separate case. I'll make the changes on my own and merge this 👍
lib/helpers/model/castBulkWrite.js
Outdated
@@ -147,7 +147,7 @@ module.exports = function castBulkWrite(originalModel, op, options) { | |||
|
|||
// set `skipId`, otherwise we get "_id field cannot be changed" | |||
const doc = new model(op['replaceOne']['replacement'], strict, true); | |||
if (model.schema.options.timestamps != null) { | |||
if (doc.initializeTimestamps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, shouldn't this be if (model.schema.options.timestamps)
?
Summary
bulkWrite use
if (model.schema.options.timestamps != null)
to check that it should runinitializeTimestamps()
or not, so it will got error when settimestamps: false
in schema.I saw
Model.$__insertMany
useif (doc.initializeTimestamps)
to check, so I make bulkWrite use the same condition to fix issue.Examples
#10048