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 path collisions for multiple update operators #2713

Merged
merged 33 commits into from
Jun 2, 2023
Merged

Fix path collisions for multiple update operators #2713

merged 33 commits into from
Jun 2, 2023

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented May 25, 2023 •
edited
Loading

Description

Closes #2539.

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.

@noisersup noisersup added the not ready Issues that are not ready to be worked on; PRs that should skip CI label May 25, 2023
@noisersup noisersup self-assigned this May 25, 2023
@noisersup noisersup added code/bug Some user-visible feature works incorrectly and removed not ready Issues that are not ready to be worked on; PRs that should skip CI labels May 26, 2023
@noisersup noisersup marked this pull request as ready for review May 26, 2023 11:54
@noisersup noisersup requested a review from a team as a code owner May 26, 2023 11:54
@noisersup noisersup requested review from w84thesun, rumyantseva, a team and chilagrow May 26, 2023 11:54
@noisersup noisersup enabled auto-merge (squash) May 26, 2023 11:55
@codecov
Copy link

codecov bot commented May 26, 2023 •
edited
Loading

Codecov Report

Merging #2713 (eab6000) into main (ede2468) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2713      +/-   ##
==========================================
- Coverage   26.17%   26.15%   -0.02%     
==========================================
  Files         441      441              
  Lines       22638    22653      +15     
==========================================
  Hits         5926     5926              
- Misses      16102    16117      +15     
  Partials      610      610
Impacted Files Coverage Δ
internal/handlers/common/update.go 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

Flag Coverage Δ
integration 4.96% <0.00%> (-0.01%) ⬇️
mongodb 4.96% <0.00%> (-0.01%) ⬇️
unit 24.62% <0.00%> (-0.02%) ⬇️

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

chilagrow
chilagrow reviewed May 29, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

I don't quite understand the implementation, it seems to work though 🙈

internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Outdated Show resolved Hide resolved
@noisersup noisersup requested review from a team, chilagrow and rumyantseva May 29, 2023 12:01
chilagrow
chilagrow reviewed May 30, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Looks great, just minor comments

internal/handlers/common/update.go Outdated Show resolved Hide resolved
integration/findandmodify_compat_test.go Show resolved Hide resolved
integration/findandmodify_compat_test.go Outdated Show resolved Hide resolved
rumyantseva
rumyantseva reviewed May 30, 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.

A couple of comments about tests.

integration/findandmodify_compat_test.go Outdated Show resolved Hide resolved
integration/findandmodify_compat_test.go Outdated Show resolved Hide resolved
@noisersup noisersup requested review from rumyantseva May 31, 2023 13:57
rumyantseva
rumyantseva approved these changes May 31, 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.

LGTM

internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Outdated Show resolved Hide resolved
rumyantseva
rumyantseva previously approved these changes May 31, 2023
View reviewed changes
@noisersup @rumyantseva
Apply suggestions from code review
4e128f6 
Co-authored-by: Elena Grahovac <elena@grahovac.me>
rumyantseva
rumyantseva previously approved these changes May 31, 2023
View reviewed changes
w84thesun
w84thesun previously approved these changes May 31, 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 reviewed Jun 1, 2023
View reviewed changes
internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Show resolved Hide resolved
@noisersup noisersup dismissed stale reviews from rumyantseva and w84thesun via a268b97 June 1, 2023 10:18
@AlekSi AlekSi removed their request for review June 2, 2023 06:59
@noisersup noisersup merged commit 96858fd into FerretDB:main Jun 2, 2023
@noisersup noisersup deleted the fix-update-update-collisions-2539 branch June 2, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@w84thesun w84thesun w84thesun approved these changes

@AlekSi AlekSi AlekSi approved these changes

Assignees

@noisersup noisersup

Labels
code/bug Some user-visible feature works incorrectly
Projects
Current Engineering
Archived in project
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Handle path collisions for multiple update operators
5 participants
@noisersup @AlekSi @w84thesun @rumyantseva @chilagrow