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

Process collection name param using collection tag #3476

Merged
merged 36 commits into from
Oct 4, 2023

Conversation

adetunjii
Copy link
Contributor

@adetunjii adetunjii commented Oct 2, 2023

Description

Closes #2786.

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • 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), Milestone (Next), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

adetunjii and others added 24 commits May 1, 2023 20:57
@adetunjii adetunjii requested a review from a team as a code owner October 2, 2023 02:31
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #3476 (b0b8de5) into main (531f91c) will decrease coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3476      +/-   ##
==========================================
- Coverage   74.61%   74.07%   -0.55%     
==========================================
  Files         422      422              
  Lines       26393    26394       +1     
==========================================
- Hits        19694    19551     -143     
- Misses       5565     5686     +121     
- Partials     1134     1157      +23     
Files Coverage Δ
internal/handlers/common/count.go 100.00% <ø> (ø)
internal/handlers/common/delete_params.go 100.00% <ø> (ø)
internal/handlers/common/distinct.go 79.71% <ø> (ø)
internal/handlers/common/find.go 100.00% <ø> (ø)
internal/handlers/common/findandmodify.go 77.83% <ø> (ø)
internal/handlers/common/insert.go 100.00% <ø> (ø)
internal/handlers/common/update_params.go 100.00% <ø> (ø)
internal/handlers/commonparams/extract_params.go 82.60% <100.00%> (+0.06%) ⬆️

... and 19 files with indirect coverage changes

Flag Coverage Δ
filter-false ?
filter-true 69.06% <62.50%> (-0.65%) ⬇️
hana-1 ?
integration 69.06% <62.50%> (-0.70%) ⬇️
mongodb-1 4.63% <0.00%> (-0.01%) ⬇️
pg-1 40.54% <62.50%> (-0.56%) ⬇️
pg-2 43.21% <62.50%> (-0.47%) ⬇️
pg-3 42.85% <62.50%> (-0.65%) ⬇️
postgresql-1 ∅ <ø> (∅)
postgresql-2 ∅ <ø> (∅)
postgresql-3 ∅ <ø> (∅)
sort-false 69.06% <62.50%> (-0.28%) ⬇️
sort-true ?
sqlite-1 39.68% <62.50%> (-0.07%) ⬇️
sqlite-2 42.43% <62.50%> (-0.03%) ⬇️
sqlite-3 42.36% <62.50%> (-0.05%) ⬇️
unit 25.12% <100.00%> (+0.07%) ⬆️

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

@AlekSi AlekSi added the code/chore Code maintenance improvements label Oct 2, 2023
@AlekSi AlekSi requested review from a team, chilagrow and noisersup October 2, 2023 07:15
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.

Thank you for your contribution 🤗 I have just one comment about adding a test for new logic.

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.

Thanks for your quick response and updating godoc! I see that newly introduced logic has a case where collection tag is not set. Let's cover that test case as well 🤗

internal/handlers/commonparams/extract_params.go Outdated Show resolved Hide resolved
@chilagrow
Copy link
Member

By the way, when you are ready to request review again, you can press this button. It will notify reviewers 🤗
image

@adetunjii adetunjii requested a review from chilagrow October 3, 2023 16:01
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.

Great work! Thanks again for your contribution 🤗

@chilagrow chilagrow requested a review from a team October 4, 2023 02:29
@chilagrow chilagrow added this to the Next milestone Oct 4, 2023
@chilagrow chilagrow enabled auto-merge (squash) October 4, 2023 02:29
@chilagrow chilagrow merged commit 25ea2df into FerretDB:main Oct 4, 2023
27 of 31 checks passed
@@ -87,12 +84,6 @@ func ExtractParams(doc *types.Document, command string, value any, l *zap.Logger

lookup := key

// If the key is the same as the command name, then it is a collection name.
// Depending on the driver, the key may be camel case or lower case for a collection name.
Copy link
Member

Choose a reason for hiding this comment

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

That comment was relevant

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for capturing this. Fix made in #3494

@@ -44,7 +44,7 @@ const (
// FindAndModifyParams represent parameters for the findAndModify command.
type FindAndModifyParams struct {
DB string `ferretdb:"$db"`
Collection string `ferretdb:"collection"`
Collection string `ferretdb:"findAndModify,collection"`
Copy link
Member

Choose a reason for hiding this comment

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

Some drivers send this as findandmodify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Process collection name parameter in tags
3 participants