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

feat(model): allow passing timestamps option to Model.bulkSave(...) #12082

Merged
merged 15 commits into from Jul 16, 2022

Conversation

AbdelrahmanHafez
Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez commented Jul 8, 2022

fixes #12059
I also refactored bulkSave to use async/await instead of the ugly promise chain, even though I wrote it, I find it difficult to read.
This comes with the downside of shorter stack traces, hopefully, this is not an issue.

@vkarpov15
There is a test failing, bulkSave calls bulkWrite with timestamps false, so mongoose sends the writeOperations to MongoDB without updatedAt or createdAt, but the documents in memory still have updatedAt and createdAt that is inconsistent with the database.

This happens when we document.schema.s.hooks.execPre('save',...). Do you know if there is a way to make pre-save hook not update timestamps? or pass an option to it? pre-post hooks seem like a blackbox to me and for years I couldn't figure out how they work.

@AbdelrahmanHafez AbdelrahmanHafez requested review from vkarpov15 and Uzlopak and removed request for vkarpov15 July 8, 2022 17:23
@vkarpov15
Copy link
Collaborator

@AbdelrahmanHafez I think the issue is this line:

const timestampOption = get(this, '$__.saveOptions.timestamps');
if (timestampOption === false) {
return next();
}
. setupTimestamps() registers a pre('save') hook that sets timestamps unless this.$__.saveOptions.timestamps === false.

I think that bulkSave() needs to set doc.$__.saveOptions = options on every doc being saved.

@AbdelrahmanHafez
Copy link
Collaborator Author

Thanks, @vkarpov15, that fixes it. t👍

@AbdelrahmanHafez AbdelrahmanHafez added this to the 6.5 milestone Jul 15, 2022
@AbdelrahmanHafez AbdelrahmanHafez changed the base branch from master to 6.5 July 15, 2022 05:50
@AbdelrahmanHafez AbdelrahmanHafez added the new feature This change adds new functionality, like a new method or class label Jul 15, 2022
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Nice, thanks 👍

@vkarpov15 vkarpov15 merged commit 7187253 into 6.5 Jul 16, 2022
@vkarpov15 vkarpov15 deleted the gh-12059 branch July 16, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: add timestamps option to bulkSave
2 participants