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

Allow $set operator to update _id #3009

Merged
merged 10 commits into from Jul 10, 2023

Conversation

chilagrow
Copy link
Contributor

@chilagrow chilagrow commented Jul 7, 2023

Description

Closes #2803.

Scope changed, because of #3013 we decided to remove validation.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@chilagrow chilagrow changed the title add test remove code Update document with _id Jul 7, 2023
@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #3009 (f83bd59) into main (7d70c74) will decrease coverage by 25.07%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3009       +/-   ##
===========================================
- Coverage   76.25%   51.19%   -25.07%     
===========================================
  Files         385      385               
  Lines       21060    21042       -18     
===========================================
- Hits        16060    10773     -5287     
- Misses       4076     9533     +5457     
+ Partials      924      736      -188     
Impacted Files Coverage Δ
internal/handlers/common/update.go 72.83% <100.00%> (-16.78%) ⬇️

... and 123 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 41.99% <100.00%> (-30.77%) ⬇️
mongodb 5.34% <0.00%> (+<0.01%) ⬆️
pg ?
shard-1 36.05% <100.00%> (-19.24%) ⬇️
shard-2 ?
shard-3 36.43% <100.00%> (-20.24%) ⬇️
sqlite 41.90% <100.00%> (-4.05%) ⬇️
unit 24.27% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@chilagrow chilagrow added the code/bug Some user-visible feature works incorrectly label Jul 10, 2023
@chilagrow chilagrow changed the title Update document with _id Fix $set operator for updating _id Jul 10, 2023
@chilagrow chilagrow marked this pull request as ready for review July 10, 2023 07:29
@chilagrow chilagrow requested a review from a team as a code owner July 10, 2023 07:29
@chilagrow chilagrow enabled auto-merge (squash) July 10, 2023 07:30
Copy link
Member

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

I understand why you changed these functions the way you did, but I think we need to find a better design as the current approach is too confusing with having extra params that are needed only once and for a particular case. I don't have a clear idea of a better design, but I'm available for pair programming.

internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Show resolved Hide resolved
@chilagrow chilagrow changed the title Fix $set operator for updating _id Allow $set operator to update _id Jul 10, 2023
Copy link
Member

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

Based on the number of things that need to be done for refactoring, I think the current simple solution is the best choice to solve the exact blocker quickly!

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

:shipit:

@AlekSi AlekSi added this to the Next milestone Jul 10, 2023
@chilagrow chilagrow merged commit d50e794 into FerretDB:main Jul 10, 2023
23 of 27 checks passed
@ujibang
Copy link

ujibang commented Jul 31, 2023

should be reopened, incomplete fix see #2803 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/bug Some user-visible feature works incorrectly
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Error updating a document passing its same id with the $set operator
4 participants