-
Notifications
You must be signed in to change notification settings - Fork 385
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
Forbid regex types and arrays in document's _id
field
#1326
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1326 +/- ##
===========================================
+ Coverage 46.63% 67.37% +20.74%
===========================================
Files 279 279
Lines 13134 13141 +7
===========================================
+ Hits 6125 8854 +2729
+ Misses 6273 3365 -2908
- Partials 736 922 +186
Flags with carried forward coverage won't be shown. Click here to find out more. |
_id
field_id
field
Need a help with a linter. It screams about deprecation because of generating files, though we don't have any quick alternatives to the |
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.
I like the current implementation idea! Added a couple of comments (not super critical).
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.
Looks good to me overall.
I think, one tiny thing could be improved in insert compat tests (see my comment), but as this task blocks some other things, I consider this request for changes minor.
var targetFindRes, compatFindRes bson.D | ||
targetCur, err := targetCollection.Find(ctx, bson.D{{}}) | ||
require.NoError(t, err) | ||
targetCur.Decode(&targetFindRes) | ||
|
||
compatCur, err := compatCollection.Find(ctx, bson.D{{}}) | ||
require.NoError(t, err) | ||
compatCur.Decode(&targetFindRes) | ||
|
||
AssertEqualDocuments(t, compatFindRes, targetFindRes) |
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.
Should we find by _id
to compare inserted documents here? (The idea is to check that what we inserted is really stored equally).
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.
I think that checking whole collection content is much more full-bodied. What if the insert function misbehaves and creates additional entry in collection? Checking only for specific _id
would not cover this case
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.
Seems good. I just have questions about skipped tests.
Description
Closes #1235.
Readiness checklist
task all
, and it passed.task godocs
.@FerretDB/core
), Assignee, Labels, Project and project's Sprint fields.