-
Notifications
You must be signed in to change notification settings - Fork 409
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
Conversation
Codecov Report
@@ 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
... and 19 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this 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.
…/collection-param-tag
…ii/FerretDB into fix/collection-param-tag
There was a problem hiding this 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 🤗
…/collection-param-tag
…ii/FerretDB into fix/collection-param-tag
There was a problem hiding this 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 🤗
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment was relevant
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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
Description
Closes #2786.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Milestone (Next
), Labels, Project and project's Sprint fields.