-
Notifications
You must be signed in to change notification settings - Fork 13
REP-5806 Parse document filter as extended JSON #111
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
base: main
Are you sure you want to change the base?
Conversation
803c1ba
to
ce20362
Compare
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, had some questions!
Previously we parsed the document filter as plain JSON, which made it impossible to filter on ObjectID or other BSON-specific types. This changeset fixes that by parsing the document filter as MongoDB’s extended JSON.
This avoids hard-to-understand phrases like this: > failed comparison on field counter between srcClient ... … which really sounds like there is a counter for fields rather than a field named “counter”.
cb297d9
to
597d268
Compare
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.
LGTM!
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.
LGTM % one question about a very trivial case
|
||
func (ejb extJSONBindingType) Bind(req *http.Request, obj any) error { | ||
if req == nil || req.Body == nil { | ||
return errors.New("invalid request") |
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 req.Body == nil
be an error or should it return an empty struct?
Previously we parsed the document filter as plain JSON, which made it impossible to filter on ObjectID or other BSON-specific types.
This changeset fixes that by parsing the document filter as MongoDB’s extended JSON.
I’m going to merge the commits as-is rather than squashing. Note that there are two unrelated UX improvements here, as well as a fix for a race condition in an unrelated test, in discrete commits.