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

types(models): fix incorrect bulk write options #14513

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

emiljanitzek
Copy link
Contributor

Summary
The bulkWrite options are incorrectly typed. timestamp does not exists as an option on updateOne/updateMany (there was a test but it didn't work, I fixed this as well). And the type incorrectly showed a timestamp as options to bulkWrite, which doesn't work.

Also extended the mongodb BulkWriteOptions making it easier to use the MongooseBulkWriteOption without the need to also always extend mongodb.

Examples

See fixed test

@vkarpov15 vkarpov15 added this to the 8.3.2 milestone Apr 11, 2024
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.

Thanks 👍

@vkarpov15 vkarpov15 merged commit 78bbcb5 into Automattic:master Apr 11, 2024
3 checks passed
@sderrow
Copy link
Contributor

sderrow commented Apr 12, 2024

@emiljanitzek @vkarpov15 FYI the timestamps option at the bulkWrite level is actually valid, but it's only applied for insertOnes, as seen here.

Should I open a PR to revert that particular piece of this PR?

Longer-term, probably makes sense to be consistent and remove timestamps from the outer options level and move it to the insertOne level, so it mirrors the structure of updateOne and updateMany. But that would be a breaking change, so maybe for 9.0?

@emiljanitzek
Copy link
Contributor Author

@emiljanitzek @vkarpov15 FYI the timestamps option at the bulkWrite level is actually valid, but it's only applied for insertOnes, as seen here.

Should I open a PR to revert that particular piece of this PR?

Longer-term, probably makes sense to be consistent and remove timestamps from the outer options level and move it to the insertOne level, so it mirrors the structure of updateOne and updateMany. But that would be a breaking change, so maybe for 9.0?

Seems very dangerous to have a function wide option that only applies sometimes. It does make sense as an option but then it should be respected for updateOne/updateMany as well. Or rename the option to insertTimestamps to avoid confusion and unexpected behaviors.

@sderrow
Copy link
Contributor

sderrow commented Apr 12, 2024

I also wonder if the same option should be included for replaceOne, and implement similar behavior?

@vkarpov15
Copy link
Collaborator

@sderrow you're right, we should support the timestamps option for all bulk write elements. If you're willing to put in a separate PR for supporting that, that would be great. I'll open a separate issue, and we'll get to it in the next couple of weeks if you don't get to it first.

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.

3 participants