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

Support for map type in query string #535

Merged
merged 2 commits into from
Jan 30, 2018

Conversation

adamstruck
Copy link
Contributor

@adamstruck adamstruck commented Jan 30, 2018

Adds support for the map type in query strings. Filter strings take the form: ?map_field[key]=value.

Only maps with primitive keys and values are supported.

Closes #316

Copy link
Collaborator

@achew22 achew22 left a comment

Choose a reason for hiding this comment

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

Looks like a great thing to add to grpc-gateway. A few small tweaks and then I'm happy to merge this 😄

runtime/query.go Outdated
@@ -84,6 +94,11 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []
case reflect.Struct:
m = f
continue
case reflect.Map:
if !isLast {
return fmt.Errorf("unexpected map field in %s", strings.Join(fieldPath, "."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate in this error message. As I understand it the conern here is that you could make the querystring ?foo[bar] which doesn't have a value. Could you add a remark about what was expected and why this is an error condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to handle the case where an intermediate field in fieldPath refers to a map.

i.e. obj.nested.map_field.foo where map_field is a map.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you enhance the error message to reflect that intent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure no problem.

"map_value12[key]": {"2.5"},
"map_value13[2.5]": {"value"},
"map_value14[key]": {"true"},
"map_value15[true]": {"value"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add some examples where you populate multiple map values at the same time?

@adamstruck
Copy link
Contributor Author

Thanks for the feedback! I think my latest commit addresses your comments.

@achew22
Copy link
Collaborator

achew22 commented Jan 30, 2018

Thanks for the new feature and thanks for contributing!

@achew22 achew22 merged commit 6658b3a into grpc-ecosystem:master Jan 30, 2018
@Stoakes Stoakes mentioned this pull request Apr 17, 2018
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants