Support oneof fields in query params #321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds tests, which fail, to read values for oneof fields from the query string. The assumption for the change right now is that the query string should only contain one use of a oneof group's fields -- e.g., in the test, only one of oneof_string_value or oneof_bool_value, or neither, may be specified. Passing both is an error. This also adds a wanterr field to the test spec for parameters, simply to detect that case.
This is only for testing. Has no effect on tests passing (the tag is unused in runtime), but fixed for the sake of correctness.
nilium
commented
Mar 3, 2017
runtime/query.go
Outdated
| @@ -43,13 +43,16 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values [] | |||
| var props *proto.Properties | |||
| m = m.Elem() | |||
| for i, fieldName := range fieldPath { | |||
| isLast := i == len(fieldPath)-1 | |||
| var isLast = i == len(fieldPath)-1 | |||
There was a problem hiding this comment.
Considering making a commit to revert this particular line. It got changed as part of hacking on this a bit (before fieldByProtoName returned an error). I could also amend the commit and submit a new pull request. Does it seem worthwhile to do either?
There was a problem hiding this comment.
Amended commit force-pushed. Didn't realize GitHub supported that now. (Has it always?)
This modifies fieldByProtoName to do two things: 1. It may now return an error. It only does this for the oneof case right now, as the not-found case only causes the field to be skipped. 2. It will first look for the field name in the message's OneofTypes map. If the field matches a OneOf type, it will check first that the field is nil -- if it isn't, the field is already set and it returns an error. If nil, it allocates a new oneof type for the field and returns the first field of the oneof type. The reason for rejecting multiple oneof fields is that the runtime iterates over url.Values and gets pseudo-random key order, meaning that one field is selected out of several. We could address this by sorting the url.Values and picking the last seen, in which case the code is far simpler but involves allocating and sorting an array of field names before walking them (only to get predictable field selection). This change amends the error result tested for in TestPopulateParameters to include the name of oneof_value. Related to issue grpc-ecosystem#82. Change-Id: I7a5ecc9ce397bd71156cd4ac8690bae782ecc55e
nilium
force-pushed
the
support-oneof-query-params
branch
from
3ab9905
to
e9752d4
Compare
|
@nilium this looks great, thank you for your contribution. |
|
I think being explicit over allowing random behavior is prefereable here. LGTM. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This modifies fieldByProtoName to support two things:
It may now return an error. It only does this for the oneof case
right now, as the not-found case only causes the field to be
skipped.
It will also look for the field name in the message's OneofTypes
map. If the field matches a OneOf type, it will check first that the
field is nil -- if it isn't, the field is already set and it returns
an error. If nil, it allocates a new oneof type for the field and
returns the first field of the oneof type.
The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler (in that there's no new error case) but involves allocating and
sorting an array of field names before walking them (only to get
predictable field selection).
This change amends the error result tested for in
TestPopulateParameters to include the name of oneof_value.
Related to issue #82.
Commit 24b4e8c adds tests that intentionally fail without oneof support
in place. Commit 3ab9905 includes changes to support oneof fields
(allowing the tests to pass).