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

improve(genswagger:template):added support for google.protobuf.Timestamp #209

Merged
merged 2 commits into from
Aug 24, 2016
Merged

improve(genswagger:template):added support for google.protobuf.Timestamp #209

merged 2 commits into from
Aug 24, 2016

Conversation

EranAvidor
Copy link
Contributor

google.protobuf.Timestamp is constructed from 2 primitives (int32, 64) which results in a wrong swagger file.

@yugui
Copy link
Member

yugui commented Aug 4, 2016

github is failing to check if Eran has signed a CLA.
I have asked grpc team about this issue.

@EranAvidor please sign the CLA if you have not yet

@zinuga
Copy link

zinuga commented Aug 4, 2016

@EranAvidor shows up as not signed CLA yet.

@EranAvidor
Copy link
Contributor Author

Hi @yugui @zinuga, I just signed the CLA.

@achew22
Copy link
Collaborator

achew22 commented Aug 5, 2016

Eran,

Thanks for your contribution to the project! Is there any chance I could get you to update one of the example .proto files to include the usage examples for swagger in examples/examplepb? Once you do that you should be able to run make clean examples. That will update the example swagger output in files like this.

Thanks again

@EranAvidor
Copy link
Contributor Author

sorry for the delay @achew22 , enjoy your weekend.

@achew22
Copy link
Collaborator

achew22 commented Aug 11, 2016

@wing328, To my novice eyes this looks like it generates a swagger template that will do time. However, the documentation on getting times working is a little sparse so this is based on my generating code. Would you be willing to take a look at the generated swagger output and say if this is the correct approach?

@wing328
Copy link

wing328 commented Aug 11, 2016

@achew22 is it correct to say that you want me to review this file to see if it's a valid Swagger/OpenAPI spec?

I did a search on the keyword "time" in that file but only found 2 references to "timeout"

@achew22
Copy link
Collaborator

achew22 commented Aug 13, 2016

@wing328 The lines in question are here. By my of the spec, specifically this section:

Common Name type format Comments
dateTime string date-time As defined by date-time - RFC3339

It looks like you can do one of two things.

i. put the format as the literal token "date-time"
ii. put the format as a string representing RFC3339

Which way is the correct way to interpret that? Now that I'm reading it more closely I suspect that it might be the prior.

@EranAvidor
Copy link
Contributor Author

Hi @achew22 and @wing328 ,
I came up with this documentation in order to achieve ease of use.
The options above will lead developers to search for usage examples (that can't be directly conclude from the specs or the literal token).
What do you think?

@wing328
Copy link

wing328 commented Aug 17, 2016

@achew22 if the datetime string conforms to RFC3339, then you can specify the format as date-time

This is the first time I saw people documenting a datetime string in the following format:

          "type": "string",
          "format": "0000-00-00T00:00:00.000000000Z"

and the format is allowed by the spec (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types)

Tools like swagger codegen only recognize date-time but not 0000-00-00T00:00:00.000000000Z.

if fd.GetTypeName() == ".google.protobuf.Timestamp" && pbdescriptor.FieldDescriptorProto_TYPE_MESSAGE == ft {
core = schemaCore{
Type: "string",
Format: "0000-00-00T00:00:00.000000000Z",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @wing328 for taking a look.

@EranAvidor Do you think you could update this to say "date-time" instead of "0000-00-00T00:00:00.0000000Z"?

@EranAvidor
Copy link
Contributor Author

Hi @achew22 , I just replaced the format as we talked.

@EranAvidor
Copy link
Contributor Author

rebased with master

@achew22
Copy link
Collaborator

achew22 commented Aug 19, 2016

This looks good to me. I generated clients in a couple of languages and the generated code has these new fields typed as Date objects.

Can you regenerate the examples to make the tests pass and then upload again?

Thank you!

@EranAvidor
Copy link
Contributor Author

@achew22 done.

@achew22 achew22 merged commit 331418a into grpc-ecosystem:master Aug 24, 2016
@achew22
Copy link
Collaborator

achew22 commented Aug 24, 2016

Looks good to me. Merged

@tamalsaha tamalsaha mentioned this pull request Mar 30, 2017
1 task
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…supported-types

improve(genswagger:template):added support for google.protobuf.Timestamp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants