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

Configurable query parameters parser #1138

Merged
merged 6 commits into from
Feb 25, 2020

Conversation

wp0pw
Copy link
Contributor

@wp0pw wp0pw commented Feb 24, 2020

Implementing changes for #1128
Added new mux Option to configure query parameter parser to use with instance of mux.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This is very nearly there, but I think we have to make one major change; instead of changing the generated code (which will inevitably break our users), lets change the PopulateQueryParameters function to call runtime.customQueryParameterParser if it is non-nil, and have a new runtime.WithQueryParameterParser set this unexported field. It would mean the parser can only be set globally, not per-servemux. Is that still okay for you?

runtime/mux.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
@wp0pw
Copy link
Contributor Author

wp0pw commented Feb 25, 2020

Global parser is fine I think, if we don't want to change the generated code there is not much else we can do here - I'll try to make changes some time this week, thanks for quick feedback.

Makes names more consistent and improves docs

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>
@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@18b7146). Click here to learn what that means.
The diff coverage is 75.86%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1138   +/-   ##
=========================================
  Coverage          ?   53.79%           
=========================================
  Files             ?       42           
  Lines             ?     4207           
  Branches          ?        0           
=========================================
  Hits              ?     2263           
  Misses            ?     1696           
  Partials          ?      248
Impacted Files Coverage Δ
protoc-gen-grpc-gateway/gengateway/template.go 80.58% <ø> (ø)
protoc-gen-swagger/genswagger/types.go 0% <ø> (ø)
runtime/mux.go 54.13% <0%> (ø)
examples/server/a_bit_of_everything.go 0% <0%> (ø)
runtime/query.go 72.22% <100%> (ø)
runtime/errors.go 49.31% <100%> (ø)
protoc-gen-swagger/genswagger/template.go 56.72% <86.36%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18b7146...ab4627a. Read the comment docs.

@wp0pw
Copy link
Contributor Author

wp0pw commented Feb 25, 2020

Changed to use global parser. Added interface for parser implementations.
No changes to generated code.

runtime/mux.go Outdated Show resolved Hide resolved
runtime/query.go Outdated Show resolved Hide resolved
runtime/mux.go Outdated Show resolved Hide resolved
wp0pw and others added 2 commits February 25, 2020 14:32
Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@johanbrandhorst johanbrandhorst merged commit 000dde5 into grpc-ecosystem:master Feb 25, 2020
@wp0pw
Copy link
Contributor Author

wp0pw commented Feb 25, 2020

Pleasure!

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* grpc-ecosystem#1128 make it possible to configure query param parser implementation

* Apply suggestions from code review

Makes names more consistent and improves docs

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

* grpc-ecosystem#1128 make configured parser a global parser

* update docs for configuring query parser

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

* grpc-ecosystem#1128 unexport default query parameter parser and rename mux option

Co-authored-by: Johan Brandhorst <johan.brandhorst@gmail.com>

Fixes grpc-ecosystem#1128
pull bot pushed a commit to BuildingRobotics/grpc-gateway that referenced this pull request Apr 29, 2020
* grpc-ecosystem#1128 make it possible to configure query param parser implementation

* Apply suggestions from code review

Makes names more consistent and improves docs

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

* grpc-ecosystem#1128 make configured parser a global parser

* update docs for configuring query parser

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

* grpc-ecosystem#1128 unexport default query parameter parser and rename mux option

Co-authored-by: Johan Brandhorst <johan.brandhorst@gmail.com>

Fixes grpc-ecosystem#1128
@anilcse
Copy link

anilcse commented Aug 29, 2020

Is there any example implementation for this? I was looking at the docs, didn't find any info

@johanbrandhorst
Copy link
Collaborator

The default implementation in both master and v2 implements the interface. You'll just have to design your own with the same signature. See https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query.go#L37

@anilcse
Copy link

anilcse commented Aug 29, 2020

Looks like there's some issue with gogoproto support.

 cannot use &customQueryParser literal (type *customQueryParser) as type runtime.QueryParameterParser in assignment:
        *customQueryParser does not implement runtime.QueryParameterParser (wrong type for Parse method)
                have Parse("github.com/gogo/protobuf/proto".Message, url.Values, *utilities.DoubleArray) error
                want Parse(protoiface.MessageV1, url.Values, *utilities.DoubleArray) error

Any known workarounds for this?

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Aug 29, 2020

No, you can't use it with Gogo protobuf.

@anilcse
Copy link

anilcse commented Aug 29, 2020

Oh okay, thank you for the info @johanbrandhorst . We are trying to convert query parameters to grpc server specific types in grpc-gateway. Is there an easy way to achieve this? For the sake of UX, we want to keep human-readable parameters for clients and convert them to bytes in grpc-gateway's middleware/interceptor (?).

@johanbrandhorst
Copy link
Collaborator

Any parameters not captured in the body or the path automatically become query parameters, are you sure you need to do anything?

@anilcse
Copy link

anilcse commented Aug 29, 2020

No issues in reading query parameters. But I need to typecast the query parameters before forwarding them to grpc server. We have some prefix encoded strings for clients as that gives better UX. our gRPC server uses bytes (proto definitions are bytes). We want to support strings from client and typecast them to bytes in grpc gateway middleware

@achew22
Copy link
Collaborator

achew22 commented Aug 29, 2020

@anilcse you should be able to write anything you want in some kind of Go middleware. grpc-gateway doesn't have any provisions for doing this directly so you will have to write something custom up.

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.

6 participants