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.

Sign up for GitHub

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

Jump to bottom

fix(NODE-3765): make replacement for replaceOne operations without _id #3040

Merged
merged 1 commit into from
Dec 8, 2021
Merged

fix(NODE-3765): make replacement for replaceOne operations without _id #3040

merged 1 commit into from
Dec 8, 2021

Conversation

tusbar
Copy link
Contributor

@tusbar tusbar commented Nov 17, 2021 •
edited
Loading

Description

What is changing?

collection.replaceOne, collection.findOneAndReplace and the replaceOne bulk operation now take a WithoutId<TModel> type for the replacement document.

Is there new documentation needed for these changes?

I don’t think so. It should be self-documented from the tsdoc.

What is the motivation for this change?

  • The _id field is immutable, so it shouldn’t be required as it has to be the same value when passed.
  • This makes replaceOne more in line with insertOne.

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>
  • Changes are covered by tests

@tusbar tusbar force-pushed the replace-one-optional-id branch from 6d30520 to f4085ac Compare November 17, 2021 12:56
@dariakp dariakp added External Submission PR submitted from outside the team tracked-in-jira Ticket filed in MongoDB's Jira system labels Nov 17, 2021
@dariakp dariakp changed the title feat(NODE-3765): make _id optional in replacement for replaceOne operations fix(NODE-3765): make _id optional in replacement for replaceOne operations Dec 3, 2021
@dariakp dariakp requested a review from nbbeeken December 3, 2021 16:30
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Dec 3, 2021
nbbeeken
nbbeeken requested changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Thanks so much for your help here! I think we can take this in a bit more of a strict direction

test/types/community/collection/replaceX.test-d.ts Show resolved Hide resolved
src/collection.ts Outdated Show resolved Hide resolved
src/collection.ts Outdated Show resolved Hide resolved
@tusbar tusbar force-pushed the replace-one-optional-id branch from f4085ac to 3553fe4 Compare December 3, 2021 17:10
@tusbar tusbar changed the title fix(NODE-3765): make _id optional in replacement for replaceOne operations fix(NODE-3765): make replacement for replaceOne operations without _id Dec 3, 2021
nbbeeken
nbbeeken previously approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, (test failures are unrelated and already are fixed on main)

@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 Dec 7, 2021
@tusbar
feat(NODE-3765): make _id optional in replacement for replaceOne oper…
e865c09 
…ations

The _id field is immutable in a replaceOne operation, so it does not
make sense to require passing it. Also, this makes replaceOne consistent
with insertOne.
@tusbar tusbar force-pushed the replace-one-optional-id branch from 3553fe4 to e865c09 Compare December 7, 2021 18:46
dariakp
dariakp reviewed Dec 7, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM, but wanted to follow up on this orphaned comment

test/types/community/collection/replaceX.test-d.ts Show resolved Hide resolved
baileympearson
baileympearson approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

LGTM!

@dariakp dariakp merged commit e07e564 into mongodb:main Dec 8, 2021
@tusbar tusbar deleted the replace-one-optional-id branch December 9, 2021 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbbeeken nbbeeken nbbeeken left review comments

@dariakp dariakp dariakp approved these changes

@baileympearson baileympearson baileympearson approved these changes

@durran durran Awaiting requested review from durran

Assignees

@nbbeeken nbbeeken

Labels
External Submission PR submitted from outside the team Team Review Needs review from team tracked-in-jira Ticket filed in MongoDB's Jira system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tusbar @nbbeeken @baileympearson @dariakp