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

protoc-gen-swagger: support all well-known wrapper types #695

Merged
merged 1 commit into from Jul 24, 2018

Conversation

Projects
None yet
4 participants
@jriecken
Contributor

jriecken commented Jul 7, 2018

There were a few well-known wrapper types missing from the wkSchemas map, in specific: UInt32Value, UInt64Value and BytesValue. This change handles them (and maps them to the same swagger types as the non-wrapped values)

This also fixes the mapping of Int64Value. The Int64Value handling maps the value to a swagger integer. The documentation for Int64Value indicates that it should be mapped to a JSON string (also the mapping for normal int64 in protoc-gen-swagger maps it to a string, so this was inconsistent.)

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jul 7, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot commented Jul 7, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no label Jul 7, 2018

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

I signed it!

Contributor

jriecken commented Jul 7, 2018

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jul 7, 2018

CLAs look good, thanks!

googlebot commented Jul 7, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 7, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 7, 2018

Collaborator

Hi @jriecken! Thanks for your contribution! The idea looks good, unfortunately, it looks like your changes caused an issue with the marshal_jsonpb tests:

marshal_jsonpb_test.go:274: strings.Contains("{\"singleNested\":{},\"uuid\":\"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7\",\"nested\":[{\"name\":\"foo\",\"amount\":12345}],\"uint64Value\":\"18446744073709551615\",\"repeatedStringValue\":[],\"oneofString\":\"bar\",\"mapValue\":{\"a\":\"ONE\",\"b\":\"ZERO\"},\"mappedStringValue\":{},\"mappedNestedValue\":{},\"timestampValue\":\"1970-01-01T00:00:00Z\",\"repeatedEnumValue\":[]}", "ONE") = true; want false

If I had to guess, I think you might need to add these new types to to the test as well. Other than that, looks good!

Collaborator

johanbrandhorst commented Jul 7, 2018

Hi @jriecken! Thanks for your contribution! The idea looks good, unfortunately, it looks like your changes caused an issue with the marshal_jsonpb tests:

marshal_jsonpb_test.go:274: strings.Contains("{\"singleNested\":{},\"uuid\":\"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7\",\"nested\":[{\"name\":\"foo\",\"amount\":12345}],\"uint64Value\":\"18446744073709551615\",\"repeatedStringValue\":[],\"oneofString\":\"bar\",\"mapValue\":{\"a\":\"ONE\",\"b\":\"ZERO\"},\"mappedStringValue\":{},\"mappedNestedValue\":{},\"timestampValue\":\"1970-01-01T00:00:00Z\",\"repeatedEnumValue\":[]}", "ONE") = true; want false

If I had to guess, I think you might need to add these new types to to the test as well. Other than that, looks good!

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

It looks like this was caused by an upstream bugfix in github.com/golang/protobuf/jsonpb that was made yesterday: golang/protobuf#645

The JSON generated by that marshal_jsonpb test before that fix looks like:

{"singleNested":{},"uuid":"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7","nested":[{"name":"foo","amount":12345}],"uint64Value":"18446744073709551615"
,"repeatedStringValue":[],"oneofString":"bar","mapValue":{"a":1,"b":0},"mappedStringValue":{},"mappedNestedValue":{},"timestampValue":"1970-01-01T00:00:00Z","repeatedEnumValue":[
]}

and after:

{"singleNested":{},"uuid":"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7","nested":[{"name":"foo","amount":12345}],"uint64Value":"18446744073709551615"
,"repeatedStringValue":[],"oneofString":"bar","mapValue":{"a":"ONE","b":"ZERO"},"mappedStringValue":{},"mappedNestedValue":{},"timestampValue":"1970-01-01T00:00:00Z","repeatedEnu
mValue":[]}

Note how the mapValue map's values changed from the numeric representation of the enum ("mapValue":{"a":1,"b":0}) to the string one ("mapValue":{"a":"ONE","b":"ZERO"}). This makes sense as the test is setting EnumsAsInts to 0 (i.e. output them as strings).

I'll update the test to match the fixed behavior from jsonpb

Contributor

jriecken commented Jul 7, 2018

It looks like this was caused by an upstream bugfix in github.com/golang/protobuf/jsonpb that was made yesterday: golang/protobuf#645

The JSON generated by that marshal_jsonpb test before that fix looks like:

{"singleNested":{},"uuid":"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7","nested":[{"name":"foo","amount":12345}],"uint64Value":"18446744073709551615"
,"repeatedStringValue":[],"oneofString":"bar","mapValue":{"a":1,"b":0},"mappedStringValue":{},"mappedNestedValue":{},"timestampValue":"1970-01-01T00:00:00Z","repeatedEnumValue":[
]}

and after:

{"singleNested":{},"uuid":"6EC2446F-7E89-4127-B3E6-5C05E6BECBA7","nested":[{"name":"foo","amount":12345}],"uint64Value":"18446744073709551615"
,"repeatedStringValue":[],"oneofString":"bar","mapValue":{"a":"ONE","b":"ZERO"},"mappedStringValue":{},"mappedNestedValue":{},"timestampValue":"1970-01-01T00:00:00Z","repeatedEnu
mValue":[]}

Note how the mapValue map's values changed from the numeric representation of the enum ("mapValue":{"a":1,"b":0}) to the string one ("mapValue":{"a":"ONE","b":"ZERO"}). This makes sense as the test is setting EnumsAsInts to 0 (i.e. output them as strings).

I'll update the test to match the fixed behavior from jsonpb

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

One of the browser tests was also assuming integer enum values - fixed it too

Contributor

jriecken commented Jul 7, 2018

One of the browser tests was also assuming integer enum values - fixed it too

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 7, 2018

Collaborator

Looks like we still need some regeneration of files.

Collaborator

johanbrandhorst commented Jul 7, 2018

Looks like we still need some regeneration of files.

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

One of the travis runs failed because protoc generated a file descriptor var that was slightly different (fileDescriptor_wrappers_5850bd48b0187754 vs fileDescriptor_wrappers_0cd12813574dca64) and the git diff failed. The other 5 non-bazel test runs did not have this same issue.

The Bazel version of the build also failed (it must somehow be pulling in a different version of jsonpb as it output the old numeric enum values?)

I feel like I'm very quickly going down a rabbit hole in grpc-gateway's build process...

Contributor

jriecken commented Jul 7, 2018

One of the travis runs failed because protoc generated a file descriptor var that was slightly different (fileDescriptor_wrappers_5850bd48b0187754 vs fileDescriptor_wrappers_0cd12813574dca64) and the git diff failed. The other 5 non-bazel test runs did not have this same issue.

The Bazel version of the build also failed (it must somehow be pulling in a different version of jsonpb as it output the old numeric enum values?)

I feel like I'm very quickly going down a rabbit hole in grpc-gateway's build process...

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 7, 2018

Collaborator

I think the first failure should be fixed with a regeneration of the protofiles (I guess it's sha-dependent?). I admit I'm mostly ignorant of the build process. @achew22 @tmc could you please help?

Collaborator

johanbrandhorst commented Jul 7, 2018

I think the first failure should be fixed with a regeneration of the protofiles (I guess it's sha-dependent?). I admit I'm mostly ignorant of the build process. @achew22 @tmc could you please help?

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

I was using the latest version of protoc instead of 3.1 - regenerating with 3.1 created the same var name.

Contributor

jriecken commented Jul 7, 2018

I was using the latest version of protoc instead of 3.1 - regenerating with 3.1 created the same var name.

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 7, 2018

Contributor

I did a little more digging and the Bazel Go rules pin the golang/protobuf dependency to this version: https://github.com/golang/protobuf/releases/tag/v1.1.0 (see https://github.com/bazelbuild/rules_go/blob/7e07b18263c08c8eca31207e1f5b28aa71539a72/go/private/repositories.bzl#L75)

The non-Bazel build uses go get (i..e pulling from master of golang/protobuf, which has the change made yesterday that caused the marshal_jsonpb test to break in the first place)

Contributor

jriecken commented Jul 7, 2018

I did a little more digging and the Bazel Go rules pin the golang/protobuf dependency to this version: https://github.com/golang/protobuf/releases/tag/v1.1.0 (see https://github.com/bazelbuild/rules_go/blob/7e07b18263c08c8eca31207e1f5b28aa71539a72/go/private/repositories.bzl#L75)

The non-Bazel build uses go get (i..e pulling from master of golang/protobuf, which has the change made yesterday that caused the marshal_jsonpb test to break in the first place)

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 7, 2018

Collaborator

So it seems we have a mismatch of versions between that pinned by Bazel and that fetched in the normal test runs. Presumably this means we cannot get a successful build unless we change one of the two. I'm not comfortable with making the call here but I would assume we'd be more keen to pin versions to releases, so assuming that, how do we pin the non-Bazel tests? This might well not be fixable until something like #689 is done.

Collaborator

johanbrandhorst commented Jul 7, 2018

So it seems we have a mismatch of versions between that pinned by Bazel and that fetched in the normal test runs. Presumably this means we cannot get a successful build unless we change one of the two. I'm not comfortable with making the call here but I would assume we'd be more keen to pin versions to releases, so assuming that, how do we pin the non-Bazel tests? This might well not be fixable until something like #689 is done.

@johanbrandhorst johanbrandhorst self-requested a review Jul 8, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 8, 2018

Collaborator

@jriecken It looks to me like the Bazel rules used are v0.10.3:

url = "https://github.com/bazelbuild/rules_go/releases/download/0.10.3/rules_go-0.10.3.tar.gz",
. I'm working on a PR which will add parity between Bazel pins and Gopkg.toml.

Collaborator

johanbrandhorst commented Jul 8, 2018

@jriecken It looks to me like the Bazel rules used are v0.10.3:

url = "https://github.com/bazelbuild/rules_go/releases/download/0.10.3/rules_go-0.10.3.tar.gz",
. I'm working on a PR which will add parity between Bazel pins and Gopkg.toml.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst
Collaborator

johanbrandhorst commented Jul 8, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 8, 2018

Collaborator

#696 should hopefully fix these dependency inconsistencies. Then this PR should be easy to merge.

Collaborator

johanbrandhorst commented Jul 8, 2018

#696 should hopefully fix these dependency inconsistencies. Then this PR should be easy to merge.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 23, 2018

Collaborator

@jriecken could you rebase on master please? The fix for #696 has been merged

Collaborator

johanbrandhorst commented Jul 23, 2018

@jriecken could you rebase on master please? The fix for #696 has been merged

protoc-gen-swagger: support all well-known wrapper types
There were a few well-known wrapper types missing from
the wkSchemas map. In specific UInt32Value, UInt64Value
and BytesValue. This change handles them (and maps them to
the same swagger types as the non-wrapped values)

This also fixes the mapping of Int64Value. The Int64Value handling
maps the value to a swagger integer. The documentation for
Int64Value indicates that it should be mapped to a JSON
string (also the mapping for normal int64 in protoc-gen-swagger
maps it to a string, so this was inconsistent.)
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 23, 2018

Codecov Report

Merging #695 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   56.54%   56.43%   -0.12%     
==========================================
  Files          30       30              
  Lines        3040     3046       +6     
==========================================
  Hits         1719     1719              
- Misses       1152     1158       +6     
  Partials      169      169
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.18% <0%> (-0.29%) ⬇️

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 d724c4f...bf62347. Read the comment docs.

codecov-io commented Jul 23, 2018

Codecov Report

Merging #695 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   56.54%   56.43%   -0.12%     
==========================================
  Files          30       30              
  Lines        3040     3046       +6     
==========================================
  Hits         1719     1719              
- Misses       1152     1158       +6     
  Partials      169      169
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.18% <0%> (-0.29%) ⬇️

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 d724c4f...bf62347. Read the comment docs.

@jriecken

This comment has been minimized.

Show comment
Hide comment
@jriecken

jriecken Jul 23, 2018

Contributor

Done

Contributor

jriecken commented Jul 23, 2018

Done

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jul 24, 2018

Collaborator

Great, this LGTM!

Collaborator

johanbrandhorst commented Jul 24, 2018

Great, this LGTM!

@johanbrandhorst johanbrandhorst merged commit 209d7ce into grpc-ecosystem:master Jul 24, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment