fix(openapi): generate correct schema definition for google.protobuf.Any #2180

dan-j · 2021-06-09T15:29:11Z

References to other Issues or PRs

Fixes #2177, but still needs some tests.

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

See comments in #2177, but this essentially adds a new entry to the well-known types which is a ref to the type which will be generated by customRefs. We then do some edge-case handling at the end of renderMessageAsDefinition() when processing the Any type to make sure the field is @type and not typeUrl/type_url.

dan-j · 2021-06-09T15:33:26Z

protoc-gen-openapiv2/internal/genopenapi/template.go

+			if core.Ref != "" {
+				refs[fd.GetTypeName()] = struct{}{}
+			}
+
 			if fd.GetTypeName() == ".google.protobuf.Empty" {
 				props = &openapiSchemaObjectProperties{}
 			}


This can be tidied up now, and props for .google.protobuf.Empty be defined on the wktSchemas map. Can address this later (I need to log off for a few hours)

dan-j · 2021-06-09T15:33:59Z

protoc-gen-openapiv2/internal/genopenapi/types.go

@@ -157,7 +157,10 @@ type schemaCore struct {
 	Ref     string          `json:"$ref,omitempty"`
 	Example json.RawMessage `json:"example,omitempty"`

-	Items *openapiItemsObject `json:"items,omitempty"`
+	// Properties can be recursively defined
+	Properties *openapiSchemaObjectProperties `json:"properties,omitempty"`


Moved this from the openapiSchemaObject

I've just been familiarising myself with the changes I made before adding tests and I'm wondering whether or not this field should be moved back to openapiSchemaObject, or whether the change I've made is OK.

If we keep this change, we can also remove the workarounds on lines 1041 and 1108.

Thoughts?

I'm a little confused, could you elaborate on the reason for moving it? Does it make more sense for the openapiv2 output we're trying to get?

So it makes the code easier to handle the edge-cases for the .google.protobuf.Empty type, at the minute there are if statements dotted around on lines 1041 and 1108 which could be removed if this change is kept.

However, as a more general point, I don't really understand why schemaCore and openapiSchemaObject are different types. The only usage of schemaCore in a type is in openapiSchemaObject and openapiItemsObject. As far as the JSON Schema spec goes, "items" is another schema (I'm not certain if this holds true for OpenAPI - I know there are a few differences).

If we wanted to keep things really simple, I'd do away with schemaCore completely, move the properties into openapiSchemaObject and refactor usages as necessary.

dan-j · 2021-06-10T01:04:31Z

So I've just pushed another commit with a different approach which seems to do what it's supposed to. However now we have a problem that the generated go code from swagger-codegen doesn't actually match what it's meant to be. Will comment inline on what I mean

codecov-commenter · 2021-06-10T01:05:59Z

Codecov Report

Merging #2180 (761c422) into master (cc77dc6) will increase coverage by 0.12%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2180      +/-   ##
==========================================
+ Coverage   59.16%   59.28%   +0.12%     
==========================================
  Files          35       35              
  Lines        4484     4500      +16     
==========================================
+ Hits         2653     2668      +15     
  Misses       1544     1544              
- Partials      287      288       +1

Impacted Files	Coverage Δ
protoc-gen-openapiv2/internal/genopenapi/types.go	`17.39% <ø> (ø)`
...otoc-gen-openapiv2/internal/genopenapi/template.go	`63.43% <85.71%> (+0.33%)`	⬆️

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 cc77dc6...761c422. Read the comment docs.

dan-j · 2021-06-10T01:08:37Z

examples/internal/clients/generateunboundmethods/model_protobuf_any.go

@@ -10,6 +10,5 @@
 package generateunboundmethods

 type ProtobufAny struct {
-	TypeUrl string `json:"typeUrl,omitempty"`
-	Value string `json:"value,omitempty"`
+	Type_ string `json:"@type,omitempty"`


swagger-codegen now generates go code which I wouldn't expect to work. Really I'd expect this type to not be generated, and any usages of ProtobufAny actually be of type interface{}, and say it's up to the caller to provide a value which marshals to a JSON object which is valid under the rules set out by the protojson package.

I guess this would be a swagger-codegen problem, and one which most likely exists across most programming languages. Not sure how to approach this.

dan-j · 2021-06-10T15:24:56Z

FYI, this isn't working yet. Just tried to use my locally built version on my project and there's an issue where swagger-ui won't allow additional properties to the @type field. Will update the MR by tomorrow

dan-j · 2021-06-11T09:53:57Z

Ok, this seems to be working for now, but going to spend a couple more days finishing my tasks with work using my fork and if all goes well I'll add some tests and hopefully have this mergeable by the end of next week.

johanbrandhorst · 2021-06-11T11:47:43Z

Ok, this seems to be working for now, but going to spend a couple more days finishing my tasks with work using my fork and if all goes well I'll add some tests and hopefully have this mergeable by the end of next week.

Sounds great, thank you!

dan-j · 2021-07-21T11:29:05Z

Apologies I've not had chance to pick this up. I was doing this for work and my personal life got in the way.

I'm on holiday now but will be able to finish this hopefully next week.

johanbrandhorst · 2021-07-23T01:32:26Z

Apologies I've not had chance to pick this up. I was doing this for work and my personal life got in the way.

I'm on holiday now but will be able to finish this hopefully next week.

No pressure buddy, I'm here to review whenever you need it :).

…efinition for google.protobuf.Any

dan-j

I've just opened this back up, rebased on top of the latest master and got familiar with my changes before adding tests.

I've added a comment to types.go, I'd like a response from somebody with more experience in this project.

I'm aiming to add some tests tomorrow.

dan-j · 2021-08-16T16:30:41Z

protoc-gen-openapiv2/internal/genopenapi/types.go

@@ -157,7 +157,10 @@ type schemaCore struct {
 	Ref     string          `json:"$ref,omitempty"`
 	Example json.RawMessage `json:"example,omitempty"`

-	Items *openapiItemsObject `json:"items,omitempty"`
+	// Properties can be recursively defined
+	Properties *openapiSchemaObjectProperties `json:"properties,omitempty"`


I've just been familiarising myself with the changes I made before adding tests and I'm wondering whether or not this field should be moved back to openapiSchemaObject, or whether the change I've made is OK.

If we keep this change, we can also remove the workarounds on lines 1041 and 1108.

Thoughts?

johanbrandhorst

This looks good, but I'd like some more background on some of the changes before we merge.

johanbrandhorst · 2021-08-17T01:17:22Z

protoc-gen-openapiv2/internal/genopenapi/template.go

+	".google.protobuf.Empty": {
+		Properties: &openapiSchemaObjectProperties{},
+	},


What is the consequence of this change?

I think I did this to remove the random if statements on lines 1041 and 1108, but forgot to actually remove them. However still happy to discuss what to do around moving the Properties field.

johanbrandhorst · 2021-08-17T01:18:40Z

protoc-gen-openapiv2/internal/genopenapi/template.go

+			// if wfkSchema is a reference to another known type, add it to refs to be processed later
+			if core.Ref != "" {
+				refs[fd.GetTypeName()] = struct{}{}


I'm not sure I understand this change, could you explain?

I can't remember exactly, however I believe this was the previous behaviour:

rpcStatus (which references protobufAny) is hardcoded to always output in the definitions object, and rpcStatus.details is an array of "$ref": "#/definitions/protobufAny".

If you define a type in your API with a google.protobuf.Any field, the generated schema outputted the schema as an explicit schema under that properties object, and not as a $ref. This meant if your schema referenced Any multiple times, your schema would define the schema for Any in every usage, growing your schema.

Because our wktSchemas maps now includes .google.protobuf.Any as a known-type with the ref field populated, any references to Any in a custom type will be declared using said $ref. However we need to make sure that the type is still outputted to definitions, so we add it to the refs map so that it is added during the call to addCustomRefs.

johanbrandhorst · 2021-08-17T01:19:46Z

protoc-gen-openapiv2/internal/genopenapi/types.go

@@ -157,7 +157,10 @@ type schemaCore struct {
 	Ref     string          `json:"$ref,omitempty"`
 	Example json.RawMessage `json:"example,omitempty"`

-	Items *openapiItemsObject `json:"items,omitempty"`
+	// Properties can be recursively defined
+	Properties *openapiSchemaObjectProperties `json:"properties,omitempty"`


I'm a little confused, could you elaborate on the reason for moving it? Does it make more sense for the openapiv2 output we're trying to get?

dan-j · 2021-08-19T14:36:11Z

I've found a better way to fix this and added tests, going to close this and reopen a new PR to make reviewing easier

google-cla bot added the cla: yes label Jun 9, 2021

dan-j commented Jun 9, 2021

View reviewed changes

dan-j commented Jun 10, 2021

View reviewed changes

dan-j force-pushed the master branch from 71387e4 to 01078af Compare June 10, 2021 01:14

dan-j added 3 commits August 16, 2021 15:01

fix(openapi): generate correct schema definition for google.protobuf.Any

955a351

fix(openapi): attempt grpc-ecosystem#2 at generating correct schema d…

54531d3

…efinition for google.protobuf.Any

fix(openapiv2): add additionalProperties to protobufAny definition

761c422

dan-j force-pushed the master branch from 1cc4d50 to 761c422 Compare August 16, 2021 15:02

dan-j commented Aug 16, 2021

View reviewed changes

johanbrandhorst reviewed Aug 17, 2021

View reviewed changes

dan-j closed this Aug 19, 2021

dan-j mentioned this pull request Aug 19, 2021

fix: output protobufAny definition using correct JSON schema #2292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(openapi): generate correct schema definition for google.protobuf.Any #2180

fix(openapi): generate correct schema definition for google.protobuf.Any #2180

dan-j commented Jun 9, 2021 •

edited

Loading

dan-j Jun 9, 2021

dan-j Jun 9, 2021

dan-j Aug 16, 2021

johanbrandhorst Aug 17, 2021

dan-j Aug 19, 2021

dan-j commented Jun 10, 2021

codecov-commenter commented Jun 10, 2021 •

edited

Loading

dan-j Jun 10, 2021

dan-j commented Jun 10, 2021

dan-j commented Jun 11, 2021

johanbrandhorst commented Jun 11, 2021

dan-j commented Jul 21, 2021

johanbrandhorst commented Jul 23, 2021

dan-j left a comment

dan-j Aug 16, 2021

johanbrandhorst left a comment

johanbrandhorst Aug 17, 2021

dan-j Aug 19, 2021

johanbrandhorst Aug 17, 2021

dan-j Aug 19, 2021

johanbrandhorst Aug 17, 2021

dan-j commented Aug 19, 2021

fix(openapi): generate correct schema definition for google.protobuf.Any #2180

fix(openapi): generate correct schema definition for google.protobuf.Any #2180

Conversation

dan-j commented Jun 9, 2021 • edited Loading

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-j commented Jun 10, 2021

codecov-commenter commented Jun 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dan-j commented Jun 10, 2021

dan-j commented Jun 11, 2021

johanbrandhorst commented Jun 11, 2021

dan-j commented Jul 21, 2021

johanbrandhorst commented Jul 23, 2021

dan-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-j commented Aug 19, 2021

dan-j commented Jun 9, 2021 •

edited

Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading