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 findAndModify for $exists query operator #2385

Merged
merged 18 commits into from
Apr 12, 2023
Merged

Fix findAndModify for $exists query operator #2385

merged 18 commits into from
Apr 12, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented Apr 7, 2023 •
edited
Loading

Description

Closes #2241.
Closes #1243.
Closes #1098.

In this PR, duplicated logic and codes were added to common package instead of duplicating them in pg and tigris handlers.

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), Assignee, Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@chilagrow chilagrow added the code/bug Some user-visible feature works incorrectly label Apr 7, 2023
@chilagrow chilagrow self-assigned this Apr 7, 2023
@codecov
Copy link

codecov bot commented Apr 7, 2023 •
edited
Loading

Codecov Report

Merging #2385 (d6d6454) into main (d220682) will increase coverage by 0.34%.
The diff coverage is 75.84%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2385      +/-   ##
==========================================
+ Coverage   63.99%   64.34%   +0.34%     
==========================================
  Files         391      392       +1     
  Lines       19144    19233      +89     
==========================================
+ Hits        12252    12376     +124     
+ Misses       5984     5955      -29     
+ Partials      908      902       -6
Impacted Files Coverage Δ
internal/handlers/common/upsertoperation_string.go 0.00% <0.00%> (ø)
internal/handlers/commonerrors/error.go 48.38% <ø> (ø)
internal/handlers/commonerrors/errorcode_string.go 80.00% <ø> (ø)
internal/handlers/tigris/msg_findandmodify.go 0.00% <0.00%> (ø)
internal/handlers/pg/msg_findandmodify.go 76.87% <77.50%> (+8.02%) ⬆️
internal/handlers/common/findandmodify.go 88.42% <100.00%> (+10.65%) ⬆️
internal/handlers/common/update.go 87.29% <100.00%> (+0.12%) ⬆️

... and 2 files with indirect coverage changes

Flag Coverage Δ
integration 57.34% <75.84%> (+0.38%) ⬆️
mongodb 4.73% <0.00%> (-0.03%) ⬇️
pg 57.26% <75.84%> (+0.38%) ⬆️
unit 25.53% <0.00%> (-0.13%) ⬇️

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

@chilagrow chilagrow changed the title findAndModify supports $exists operator Fix findAndModify for _id $exists operator Apr 7, 2023
@chilagrow chilagrow changed the title Fix findAndModify for _id $exists operator Fix findAndModify for $exists operator on _id Apr 7, 2023
@chilagrow chilagrow changed the title Fix findAndModify for $exists operator on _id Fix findAndModify for $exists query operator Apr 10, 2023
@chilagrow chilagrow requested review from a team, w84thesun, rumyantseva and noisersup April 10, 2023 08:26
@chilagrow chilagrow marked this pull request as ready for review April 10, 2023 08:26
@chilagrow chilagrow requested a review from a team as a code owner April 10, 2023 08:26
@chilagrow chilagrow requested a review from AlekSi April 10, 2023 08:26
@chilagrow chilagrow enabled auto-merge (squash) April 10, 2023 08:26
@rumyantseva rumyantseva mentioned this pull request Apr 11, 2023
Closed
rumyantseva
rumyantseva suggested changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva 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.

I like that we cover more cases, and I like the idea of refactoring findAndModify.
However, I find some structuring/naming a bit confusing.

Let's decide if we want to have a quick fix for this particular bug and a proper refactoring later, or if a proper refactoring is a part of this issue.

Also, as agreed, I created a separate issue to cover the rest of $exists cases for findAndModify - #2400.

integration/findandmodify_test.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
internal/handlers/common/findandmodify.go Outdated Show resolved Hide resolved
@chilagrow chilagrow requested a review from rumyantseva April 12, 2023 02:48
rumyantseva
rumyantseva approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@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.

As agreed, in the scope of this PR we fix a particular bug (actually, three bugs)!
I like the test cases and I like how we started moving the logic to common!

@AlekSi AlekSi requested review from a team April 12, 2023 13:13
@AlekSi AlekSi added this to the v1.1.0 milestone Apr 12, 2023
w84thesun
w84thesun approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

👍

@chilagrow chilagrow merged commit 15f35e0 into FerretDB:main Apr 12, 2023
fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 17, 2023
@chilagrow @fenogentov
Fix findAndModify for $exists query operator (FerretDB#2385)
63001bb
fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023
@chilagrow @fenogentov
Fix findAndModify for $exists query operator (FerretDB#2385)
ae4da41
fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023
@chilagrow @fenogentov
Fix findAndModify for $exists query operator (FerretDB#2385)
020e9b1
fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023
@chilagrow @fenogentov
Fix findAndModify for $exists query operator (FerretDB#2385)
5134f75
fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023
@chilagrow @fenogentov
Fix findAndModify for $exists query operator (FerretDB#2385)
c04a7bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rumyantseva rumyantseva rumyantseva approved these changes

@w84thesun w84thesun w84thesun approved these changes

@noisersup noisersup Awaiting requested review from noisersup noisersup was automatically assigned from FerretDB/core

@AlekSi AlekSi Awaiting requested review from AlekSi AlekSi is a code owner automatically assigned from FerretDB/leads

Assignees

@chilagrow chilagrow

Labels
code/bug Some user-visible feature works incorrectly
Projects
Current Engineering
Archived in project
Milestone
v1.1.0
4 participants
@chilagrow @w84thesun @rumyantseva @AlekSi