-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(NODE-5171): allow upsertedId
to be null in UpdateResult
#3631
Conversation
src/operations/update.ts
Outdated
modifiedCount: number; | ||
/** The number of documents that were upserted */ | ||
upsertedCount: number; | ||
/** The identifier of the inserted document if an upsert took place */ | ||
upsertedId: ObjectId; | ||
upsertedId: ObjectId | null; |
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.
This doesn't account for setOnInsert or other ways of setting the _id on the new document
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.
What are you saying the correct type should be? Either way, we have no way of knowing what the actual id type is. Should we type it as unknown?
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.
Both the filter for the operation and the $set
and $setOnInsert
operators match against TSchema
, meaning that if an _id
is passed it at all, it would be constrained to InferIdType
, so it would make sense to do that here, too. It's true that this does not account for the _id
being missing entirely resulting in the ObjectId
being generated server-side, but this is a similar issue to what we have in, for example, insertOne
and insertMany
(if the user does not provide an _id
or a pkFactory
), with the only difference being that a pkFactory
can't be applied here as a backup. So if we follow our existing pattern, we would call the use of the upsert without specifying an _id
in the case of a custom _id
TSchema
"incorrect usage", and advise the users in our documentation because TypeScript can't effectively catch that. Our operating assumption is that users performing upserts do not want to introduce inconsistencies into their schema, if one exists. But we can discuss that further with the team if you believe there is a stronger argument to be made for unknown
.
5adb2c9
to
94b7ae4
Compare
94b7ae4
to
7f257d5
Compare
UpdateResult
upsertedId
to be null in UpdateResult
Description
What is changing?
UpdateResult
is now generic over a collection schema type.UpdateResult.upsertedId
has two changes: it can now be null, and the type is inferred from the collection schema.Collection.update*
methods now return anUpdateResult
that is generic over the collection's schema.Is there new documentation needed for these changes?
What is the motivation for this change?
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript