feat: protoc-gen-swagger: Implement Response Header(s) object, Implem… #1730

elijah-roberts · 2020-10-08T05:20:54Z

protoc-gen-swagger: Implement Response Header(s) object, Implement Field Enum,Example&Format attributes #1729

References to other Issues or PRs

#1729

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

I have implemented the following:

openapi headers object proto
openapi header object proto, implementing the Description, Type, Format, Default, and Pattern fields
updated the response proto to use the headers/header objects
updated the jsonSchema proto to implement the Format, Example and Enum fields
implemented/updated template structs to match the above proto changes
implemented validation of Type field (must be one of Integer, Number, Boolean, String)
implemented validation of Default field(any.Any type that must match the Type field)
created tests to cover Header(s) logic, and validation functions
updated example proto with example usage for adding headers, and using added fields

Other comments

Per the contributing guidelines I run the 2 docker commands, and attached the updated files to my PR

elijah-roberts · 2020-10-08T05:51:19Z

In investigating the generate failure it is failing on the git diff --exit-code step.

if I run through the pipeline steps locally

    - run:
        command: make realclean
    - run:
        command: make examples
    - run:
        command: make testproto
    - run:
        command: go mod tidy
    - run:
        command: git diff --exit-code

the following files are modified:

  modified:   examples/internal/clients/abe/.swagger-codegen/VERSION
        modified:   examples/internal/clients/abe/api/swagger.yaml
        modified:   examples/internal/clients/abe/api_a_bit_of_everything_service.go
        modified:   examples/internal/clients/abe/api_camel_case_service_name.go
        modified:   examples/internal/clients/abe/api_echo_rpc.go
        modified:   examples/internal/clients/abe/client.go
        modified:   examples/internal/clients/abe/configuration.go
        modified:   examples/internal/clients/abe/model_a_bit_of_everything_nested.go
        modified:   examples/internal/clients/abe/model_examplepb_a_bit_of_everything.go
        modified:   examples/internal/clients/abe/model_examplepb_a_bit_of_everything_repeated.go
        modified:   examples/internal/clients/abe/model_examplepb_body.go
        modified:   examples/internal/clients/abe/model_examplepb_book.go
        modified:   examples/internal/clients/abe/model_examplepb_error_object.go
        modified:   examples/internal/clients/abe/model_examplepb_error_response.go
        deleted:    examples/internal/clients/abe/model_examplepb_numeric_enum.go
        modified:   examples/internal/clients/abe/model_examplepb_update_v2_request.go
        deleted:    examples/internal/clients/abe/model_message_path_enum_nested_path_enum.go
        deleted:    examples/internal/clients/abe/model_nested_deep_enum.go
        deleted:    examples/internal/clients/abe/model_pathenum_path_enum.go
        modified:   examples/internal/clients/abe/model_protobuf_any.go
        modified:   examples/internal/clients/abe/model_protobuf_field_mask.go
        modified:   examples/internal/clients/abe/model_runtime_error.go
        modified:   examples/internal/clients/abe/model_sub_string_message.go
        modified:   examples/internal/clients/abe/response.go
        modified:   examples/internal/clients/echo/.swagger-codegen/VERSION
        modified:   examples/internal/clients/echo/api/swagger.yaml
        modified:   examples/internal/clients/echo/api_echo_service.go
        modified:   examples/internal/clients/echo/client.go
        modified:   examples/internal/clients/echo/configuration.go
        modified:   examples/internal/clients/echo/model_examplepb_embedded.go
        modified:   examples/internal/clients/echo/model_examplepb_simple_message.go
        modified:   examples/internal/clients/echo/model_protobuf_any.go
        modified:   examples/internal/clients/echo/model_runtime_error.go
        modified:   examples/internal/clients/echo/response.go
        modified:   examples/internal/clients/responsebody/.swagger-codegen/VERSION
        modified:   examples/internal/clients/responsebody/api/swagger.yaml
        modified:   examples/internal/clients/responsebody/api_response_body_service.go
        modified:   examples/internal/clients/responsebody/client.go
        modified:   examples/internal/clients/responsebody/configuration.go
        modified:   examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseBodyOut.md
        modified:   examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseBodyOutResponse.md
        modified:   examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseStrings.md
        modified:   examples/internal/clients/responsebody/docs/ExamplepbResponseBodyOut.md
        modified:   examples/internal/clients/responsebody/docs/ExamplepbResponseBodyOutResponse.md
        modified:   examples/internal/clients/responsebody/docs/ProtobufAny.md
        modified:   examples/internal/clients/responsebody/docs/ResponseBodyServiceApi.md
        modified:   examples/internal/clients/responsebody/docs/RuntimeError.md
        modified:   examples/internal/clients/responsebody/docs/RuntimeStreamError.md
        modified:   examples/internal/clients/responsebody/docs/StreamResultOfExamplepbResponseBodyOut.md
        modified:   examples/internal/clients/responsebody/model_examplepb_repeated_response_body_out.go
        modified:   examples/internal/clients/responsebody/model_examplepb_repeated_response_body_out_response.go
        modified:   examples/internal/clients/responsebody/model_examplepb_repeated_response_strings.go
        modified:   examples/internal/clients/responsebody/model_examplepb_response_body_out.go
        modified:   examples/internal/clients/responsebody/model_examplepb_response_body_out_response.go
        modified:   examples/internal/clients/responsebody/model_protobuf_any.go
        deleted:    examples/internal/clients/responsebody/model_response_response_type.go
        modified:   examples/internal/clients/responsebody/model_runtime_error.go
        modified:   examples/internal/clients/responsebody/model_runtime_stream_error.go
        modified:   examples/internal/clients/responsebody/model_stream_result_of_examplepb_response_body_out.go
        modified:   examples/internal/clients/responsebody/response.go
        modified:   examples/internal/clients/unannotatedecho/.swagger-codegen/VERSION
        modified:   examples/internal/clients/unannotatedecho/api/swagger.yaml
        modified:   examples/internal/clients/unannotatedecho/api_unannotated_echo_service.go
        modified:   examples/internal/clients/unannotatedecho/client.go
        modified:   examples/internal/clients/unannotatedecho/configuration.go
        modified:   examples/internal/clients/unannotatedecho/model_examplepb_unannotated_simple_message.go
        modified:   examples/internal/clients/unannotatedecho/model_protobuf_any.go
        modified:   examples/internal/clients/unannotatedecho/model_runtime_error.go
        modified:   examples/internal/clients/unannotatedecho/response.go

Prior to my inital commit I ran the following docker commands per the instructions:

docker run -v $(pwd):/src/grpc-gateway --rm docker.pkg.github.com/grpc-ecosystem/grpc-gateway/build-env:1.15 \
    /bin/bash -c 'cd /src/grpc-gateway && \
        make realclean && \
        make examples && \
        make testproto'
docker run -itv $(pwd):/grpc-gateway -w /grpc-gateway --entrypoint /bin/bash --rm \
    l.gcr.io/google/bazel -c '\
        bazel run :gazelle -- update-repos -from_file=go.mod -to_macro=repositories.bzl%go_repositories && \
        bazel run :gazelle && \
        bazel run :buildifier'

And then attached the updated files...from the error output though, it appears as if these other examples changes need to be checked in?

johanbrandhorst

Hi Elijah, and thanks for your interest in the project! I've had a look over the changes and they mostly look good, a few style mistakes that I'm sure are just because you're unfamiliar with the Go best practices and tooling ecosystem.

There are a few more substantive comments but this looks great for a first stab at the issue.

examples/internal/clients/generateunboundmethods/.swagger-codegen/VERSION

examples/internal/proto/examplepb/a_bit_of_everything.proto

go.mod

protoc-gen-swagger/genswagger/template.go

protoc-gen-swagger/genswagger/types.go

protoc-gen-swagger/options/openapiv2.proto

elijah-roberts · 2020-10-08T21:53:55Z

@johanbrandhorst Thank you so much for the time you took to review my request!

I have completed all recommended changes (all were done locally so it may still show that you have open requests changes).

Please let me know if there is anything else that I can do 👍

johanbrandhorst · 2020-10-09T10:40:07Z

@johanbrandhorst Thank you so much for the time you took to review my request!

I have completed all recommended changes (all were done locally so it may still show that you have open requests changes).

Please let me know if there is anything else that I can do +1

Just a heads up Elijah, I've decided to freeze master and v2 while I complete the merge, so this PR will eventually have to be rebased on the new v1 branch. It shouldn't take much, since I'm making that branch from current master. I'll take another look at this after that has been finished. You can follow the progress in #1731. See #1223 (comment) for the general plan.

johanbrandhorst · 2020-10-10T15:02:36Z

I've created the v1 branch now so if you wouldn't mind rebasing this PR on top of v1 I'd really appreciate it. If you ping me when that's done I'll have another look.

johanbrandhorst · 2020-10-14T08:40:15Z

Hi @elijah-roberts, could you please rebase this on v1 and re-target this PR? Or if you want, please feel free to target master directly and skip v1.

elijah-roberts · 2020-10-14T14:54:24Z

@johanbrandhorst I will work on this today 👍

…ent Field Enum,Example&Format attributes grpc-ecosystem#1729

* a bunch of whitespace/format changes... have since added this to my IDE * remove any.Any objects, and convert them to string * move runtime regex strings to global variables * update examples * update tests

codecov-io · 2020-10-14T15:07:58Z

Codecov Report

Merging #1730 into v1 will increase coverage by 0.51%.
The diff coverage is 79.03%.

@@            Coverage Diff             @@
##               v1    #1730      +/-   ##
==========================================
+ Coverage   53.38%   53.89%   +0.51%     
==========================================
  Files          42       42              
  Lines        3902     3963      +61     
==========================================
+ Hits         2083     2136      +53     
- Misses       1562     1566       +4     
- Partials      257      261       +4

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/descriptor/services.go	`73.60% <0.00%> (ø)`
protoc-gen-swagger/genswagger/types.go	`15.00% <ø> (ø)`
protoc-gen-swagger/genswagger/template.go	`58.64% <80.32%> (+1.52%)`	⬆️

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 f6391fc...e615b55. Read the comment docs.

elijah-roberts · 2020-10-14T15:21:37Z

@johanbrandhorst I have rebased the current changes onto v1 and retargeted.

can you review my comments here #1729 (comment).

I should have the changes completed today or tomorrow and can either add them here, or create a new issue/pr.

johanbrandhorst · 2020-10-15T09:51:33Z

@johanbrandhorst I have rebased the current changes onto v1 and retargeted.

can you review my comments here #1729 (comment).

I should have the changes completed today or tomorrow and can either add them here, or create a new issue/pr.

As I said in that issue, lets do that in a separate PR. I will take another look at this PR now.

johanbrandhorst

Thanks for the updates, I left a few more comments.

examples/internal/proto/examplepb/a_bit_of_everything.proto

protoc-gen-grpc-gateway/descriptor/grpc_api_configuration_test.go

protoc-gen-grpc-gateway/descriptor/grpc_api_service.go

protoc-gen-swagger/genswagger/template.go

protoc-gen-swagger/genswagger/template_test.go

protoc-gen-swagger/genswagger/template.go

examples/internal/proto/examplepb/a_bit_of_everything.proto

protoc-gen-swagger/options/openapiv2.proto

* remove regex logic, and update functions to use simple booleans * cleanup example proto * enforce CanonicalMIMEHeaderKey for header name

protoc-gen-swagger/genswagger/template.go

protoc-gen-swagger/genswagger/template_test.go

simplifying string check Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

fix: remove string comparisons Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

elijah-roberts · 2020-10-22T02:56:12Z

@johanbrandhorst I appreciate your patience with me as I work through your suggestions!

I believe that I have resolved all of your comments, if it is possible for you to take another look that would be awesome!

johanbrandhorst

Thanks a lot for the work you've put into this Elijah 😄. Just a few last suggestions, we're almost there! It looks great.

protoc-gen-swagger/genswagger/template.go

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

* parse unsigned ints with strconv.ParseUint * update unqoute logic

elijah-roberts · 2020-10-22T12:50:35Z

@johanbrandhorst Thanks added those changes 👍

johanbrandhorst

Almost there 😂, just noticed a potential bug when using type: "integer" and format: "uint64" and value: math.MaxUint64.

protoc-gen-swagger/genswagger/template.go

elijah-roberts · 2020-10-27T23:48:55Z

@johanbrandhorst This has been updated to include your requested changes... as well as adding test cases to validate.

google-cla · 2020-10-28T10:13:59Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

johanbrandhorst · 2020-10-28T10:15:31Z

@googlebot I consent.

google-cla · 2020-10-28T10:15:47Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

johanbrandhorst

Didn't want to waste your time any longer so I went and cleaned up the last few things I noticed on the last pass. This is good to go! Thank you for your patience! I'll tag a minor with this new feature :). Would you be happy to cherry pick this PR against master as well? I think there'll be some conflicts and I could help sort them out, but it'd be great to have parity.

johanbrandhorst · 2020-10-28T10:18:38Z

Thanks for your contribution 😄!

johanbrandhorst · 2020-11-23T09:47:22Z

@elijah-roberts would you mind cherry-picking this against the master branch so we can add this to v2 as well?

elijah-roberts · 2020-11-23T18:34:56Z

Sure thing... I will put some time on my calender to do this, this week..

…

On Mon, Nov 23, 2020 at 2:47 AM Johan Brandhorst-Satzkorn < ***@***.***> wrote: @elijah-roberts <https://github.com/elijah-roberts> would you mind cherry-picking this against the master branch so we can add this to v2 as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1730 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEWT4BKU5LT5P27GHK246OLSRIVTRANCNFSM4SIIT7CA> .

-- Elijah Roberts Site Reliability Engineer Cloud Solutions Architect 509-828-3776 | elijah@elijahjamesroberts.com

elijah-roberts · 2020-11-25T00:07:54Z

@johanbrandhorst created #1841

google-cla bot added the cla: yes label Oct 8, 2020

johanbrandhorst requested changes Oct 8, 2020

View reviewed changes

johanbrandhorst mentioned this pull request Oct 8, 2020

protoc-gen-swagger: Implement Response Header(s) object, Implement Field Enum,Example&Format attributes #1729

Closed

elijah-roberts requested a review from johanbrandhorst October 8, 2020 21:50

elijah-roberts added 5 commits October 14, 2020 09:02

feat: protoc-gen-swagger: Implement Response Header(s) object, Implem…

2831fda

…ent Field Enum,Example&Format attributes grpc-ecosystem#1729

fix: resolve linting errors

2a0e115

fix: running per CI

cfa0455

fix: Resolve PR requests

dfa5c22

* a bunch of whitespace/format changes... have since added this to my IDE * remove any.Any objects, and convert them to string * move runtime regex strings to global variables * update examples * update tests

fix: Running go mode tidy to cleanup go.mod and go.sum

f4ed5a6

elijah-roberts force-pushed the feature/implement-response-headers branch from 8f1ae22 to f4ed5a6 Compare October 14, 2020 15:03

elijah-roberts changed the base branch from master to v1 October 14, 2020 15:04

johanbrandhorst requested changes Oct 15, 2020

View reviewed changes

fix: Resolving PR comments

e615b55

* remove regex logic, and update functions to use simple booleans * cleanup example proto * enforce CanonicalMIMEHeaderKey for header name

elijah-roberts requested a review from johanbrandhorst October 16, 2020 06:27

johanbrandhorst reviewed Oct 16, 2020

View reviewed changes

elijah-roberts and others added 4 commits October 16, 2020 07:09

Update protoc-gen-swagger/genswagger/template.go

c943e16

simplifying string check Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

Update protoc-gen-swagger/genswagger/template.go

7672f81

fix: remove string comparisons Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

fix: resolving MR requests

1cea661

fix: unqouting error

5ef99d0

elijah-roberts requested a review from johanbrandhorst October 22, 2020 02:55

johanbrandhorst requested changes Oct 22, 2020

View reviewed changes

elijah-roberts and others added 3 commits October 22, 2020 06:36

Update protoc-gen-swagger/genswagger/template.go

23275bf

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

Update protoc-gen-swagger/genswagger/template.go

89d01c9

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

fix: adding requested changes

9b22951

* parse unsigned ints with strconv.ParseUint * update unqoute logic

elijah-roberts requested a review from johanbrandhorst October 22, 2020 12:49

johanbrandhorst requested changes Oct 23, 2020

View reviewed changes

protoc-gen-swagger/genswagger/template.go Outdated Show resolved Hide resolved

Updating integer switch

968e362

elijah-roberts requested a review from johanbrandhorst October 27, 2020 23:49

Minor nits

bbb50e6

google-cla bot added cla: no and removed cla: yes labels Oct 28, 2020

Remove extra newlines

9e9c879

google-cla bot added cla: yes and removed cla: no labels Oct 28, 2020

johanbrandhorst approved these changes Oct 28, 2020

View reviewed changes

johanbrandhorst merged commit e0a026a into grpc-ecosystem:v1 Oct 28, 2020

elijah-roberts mentioned this pull request Nov 25, 2020

Feature/implement response headers v2 #1841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: protoc-gen-swagger: Implement Response Header(s) object, Implem… #1730

feat: protoc-gen-swagger: Implement Response Header(s) object, Implem… #1730

elijah-roberts commented Oct 8, 2020 •

edited

Loading

elijah-roberts commented Oct 8, 2020 •

edited

Loading

johanbrandhorst left a comment

elijah-roberts commented Oct 8, 2020

johanbrandhorst commented Oct 9, 2020

johanbrandhorst commented Oct 10, 2020

johanbrandhorst commented Oct 14, 2020

elijah-roberts commented Oct 14, 2020

codecov-io commented Oct 14, 2020 •

edited

Loading

elijah-roberts commented Oct 14, 2020

johanbrandhorst commented Oct 15, 2020

johanbrandhorst left a comment

elijah-roberts commented Oct 22, 2020

johanbrandhorst left a comment

elijah-roberts commented Oct 22, 2020

johanbrandhorst left a comment

elijah-roberts commented Oct 27, 2020

google-cla bot commented Oct 28, 2020

johanbrandhorst commented Oct 28, 2020

google-cla bot commented Oct 28, 2020

johanbrandhorst left a comment

johanbrandhorst commented Oct 28, 2020

johanbrandhorst commented Nov 23, 2020

elijah-roberts commented Nov 23, 2020 via email

elijah-roberts commented Nov 25, 2020

feat: protoc-gen-swagger: Implement Response Header(s) object, Implem… #1730

feat: protoc-gen-swagger: Implement Response Header(s) object, Implem… #1730

Conversation

elijah-roberts commented Oct 8, 2020 • edited Loading

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Other comments

elijah-roberts commented Oct 8, 2020 • edited Loading

johanbrandhorst left a comment

Choose a reason for hiding this comment

elijah-roberts commented Oct 8, 2020

johanbrandhorst commented Oct 9, 2020

johanbrandhorst commented Oct 10, 2020

johanbrandhorst commented Oct 14, 2020

elijah-roberts commented Oct 14, 2020

codecov-io commented Oct 14, 2020 • edited Loading

Codecov Report

elijah-roberts commented Oct 14, 2020

johanbrandhorst commented Oct 15, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

elijah-roberts commented Oct 22, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

elijah-roberts commented Oct 22, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

elijah-roberts commented Oct 27, 2020

google-cla bot commented Oct 28, 2020

johanbrandhorst commented Oct 28, 2020

google-cla bot commented Oct 28, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Oct 28, 2020

johanbrandhorst commented Nov 23, 2020

elijah-roberts commented Nov 23, 2020 via email

elijah-roberts commented Nov 25, 2020

elijah-roberts commented Oct 8, 2020 •

edited

Loading

elijah-roberts commented Oct 8, 2020 •

edited

Loading

codecov-io commented Oct 14, 2020 •

edited

Loading