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

fix(NODE-5171): allow upsertedId to be null in UpdateResult #3631

Merged
merged 5 commits into from Apr 12, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Apr 10, 2023

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 an UpdateResult 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

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

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;
Copy link
Contributor

@dariakp dariakp Apr 10, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

src/operations/update.ts Outdated Show resolved Hide resolved
@dariakp dariakp marked this pull request as draft April 10, 2023 18:08
@baileympearson baileympearson marked this pull request as ready for review April 11, 2023 18:53
src/operations/update.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken self-assigned this Apr 11, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 11, 2023
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Apr 11, 2023
nbbeeken
nbbeeken previously approved these changes Apr 11, 2023
@nbbeeken nbbeeken changed the title fix(NODE-5171): allow upsertedid to be null in UpdateResult fix(NODE-5171): allow upsertedId to be null in UpdateResult Apr 12, 2023
@nbbeeken nbbeeken merged commit 4b5be21 into main Apr 12, 2023
17 of 21 checks passed
@nbbeeken nbbeeken deleted the NODE-5171-upsertedid-null branch April 12, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants