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

allow protobuf well known types in params #400

Closed
tmc opened this issue May 28, 2017 · 9 comments
Closed

allow protobuf well known types in params #400

tmc opened this issue May 28, 2017 · 9 comments

Comments

@tmc
Copy link
Collaborator

tmc commented May 28, 2017

example: allow a google.protobuf.Duration in a get param.

johanbrandhorst added a commit to johanbrandhorst/grpc-gateway that referenced this issue Jul 3, 2018
Adds support for parsing the google.protobuf.Duration as
well as native *time.Time and *time.Duration types in
url query parameters.

Helps grpc-ecosystem#400.
tmc pushed a commit that referenced this issue Jul 5, 2018
Adds support for parsing the google.protobuf.Duration as
well as native *time.Time and *time.Duration types in
url query parameters.

Helps #400.
@AlekSi
Copy link
Contributor

AlekSi commented Aug 26, 2019

Right now, support for google.protobuf.Duration is very confusing.

  1. https://developers.google.com/protocol-buffers/docs/proto3#json says:

Generated output always contains 0, 3, 6, or 9 fractional digits, depending on required precision, followed by the suffix "s". Accepted are any fractional digits (also none) as long as they fit into nano-seconds precision and the suffix "s" is required.

  1. https://github.com/go-openapi/strfmt/blob/v0.19.2/duration.go contains support for various suffixes.

  2. But in practice, grpc-gateway always returns duration as seconds with s suffix and uses time.ParseDuration for parsing, so suffixes like d are not supported.

@johanbrandhorst
Copy link
Collaborator

Is this not implemented already? Also not sure I understand your comment @AlekSi, are you saying it should support the full range required by the openapi definition of a duration? It might be possible with some pre-json unmarshalling parsing.

@AlekSi
Copy link
Contributor

AlekSi commented Aug 26, 2019

All I'm saying is that current handling is inconsistent.

I see several options:

  1. Do nothing in code, just document that behavior. :)
  2. Extend parsing (only) with strfmt mapping, so suffixes like d are accepted.
  3. Use protobuf JSON mapping consistently: accept and return only seconds with s suffix.
  4. Use strfmt mapping consistently: accept and return other suffixes like d.

Options 3 and 4 are breaking changes, so probably some generator flag / protobuf option / runtime parameter would be required to use a new behavior.

Personally, I would prefer option 4 with a generator flag option 2. Option 4 will make parsing on the receiver side harder.

@johanbrandhorst
Copy link
Collaborator

What are we doing right now? I thought it was #3.

@AlekSi

This comment has been minimized.

@AlekSi
Copy link
Contributor

AlekSi commented Aug 26, 2019

That's how it works now:

"59s": "59s",
"60s": "60s",
"61s": "61s",
"61" : "", // no suffix => error
"2m" : "120s",
"1h" : "3600s",
"1d" : "", // d suffix => error

Suffixes m and h are accepted, but never returned.

@johanbrandhorst
Copy link
Collaborator

I see. So #2 but without d support?

@AlekSi
Copy link
Contributor

AlekSi commented Aug 26, 2019

Yes, right now it is #2 without d and w.

@johanbrandhorst
Copy link
Collaborator

@AlekSi would you mind opening a new issue with this background as a feature request? I will close this one since we do support it.

adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
…em#693)

Adds support for parsing the google.protobuf.Duration as
well as native *time.Time and *time.Duration types in
url query parameters.

Helps grpc-ecosystem#400.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants