[protoc-gen-openapi] Support google.protobuf.Any and google.rpc.Status #327

jeffsawatzky · 2022-03-29T17:33:52Z

Note that this PR is based off of my previous PR #324 which is not yet merged. So while the changes here look big, here are the actual changes relevant to this PR:
jeffsawatzky/gnostic@fq-names...jeffsawatzky:grpc-status

Purpose

Envoy and grpc-gateway use the google.rpc.Status message to report error messages back as can be seen here:
https://github.com/envoyproxy/envoy/blob/9dd8ae3c69a9917d97a886ed03e1c010dcd9b098/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L827L860

google.rpc.Status provides a way to report additional error details in the details property, which is a list of google.protobuf.Any which has special JSON encoding as can be seen here:
https://developers.google.com/protocol-buffers/docs/proto3#json

Note that google.protobuf.Any and google.protobuf.Value are different. google.protobuf.Value can be any JSON value, whereas google.protobuf.Any can be any protobuf message type and has a special @type property when transcoded so that you can determine which protobuf message definition to use.

We don't have control over the value of @type and it will always be the fully qualified name of the protobuf message. So if people what to be able to use this to map back to an OpenAPI schema definition then they will need to use fully qualified names (which I added in PR #324)

jeffsawatzky · 2022-03-29T17:35:17Z

@timburks here is another one for you

timburks · 2022-03-30T18:30:28Z

@jeffsawatzky could you merge the main branch (now containing #324) and resolve any conflicts? I'm not expecting any problems, but that would clarify what's new here.

… qualified naming of google.protobuf.Value

jeffsawatzky · 2022-03-30T18:38:10Z

@timburks done (rebased everything)

timburks

Just a few initial questions while I'm reading this...

timburks · 2022-03-30T20:26:30Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'


On my first read-through, I'm wondering why this didn't change to google.protobuf.Value

This file was generated with fq_schema_naming=false (the default) so it uses the older AnyJSONValue schema name for backwards compatibility.

There was a bug in my previous PR with google.protobuf.Value not getting converted to the fully qualified name, which I didn't catch until I added support for google.protobuf.Any. This is why you see the schema name change in the openapi_fq_schema_naming.yaml file (which uses fully qualified naming) since I have fixed the issue in this PR.

I was wondering the same thing.
The library is at < v1.0.0, isn't it better to be consistent/correct rather than backwards compatible at this point?

timburks · 2022-03-30T20:26:58Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi_json.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'


also here, why isn't this google.protobuf.Value?

Same with this file, it was generated with fq_schema_naming=false (the default) so it uses the older AnyJSONValue schema name for backwards compatibility.

morphar

Just a thought regarding the description of google.protobuf.Value:
Isn't the description a bit wrong? The value can never be null, right?
OpenAPI (v3.0.x) doesn't support null as type and the schema says oneOf, so it must be one of the types - therefore it can not be null.

morphar · 2022-03-31T00:55:41Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'


I was wondering the same thing.
The library is at < v1.0.0, isn't it better to be consistent/correct rather than backwards compatible at this point?

morphar · 2022-03-31T00:57:20Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'
-            description: AnyJSONValue is a "catch all" type that can hold any JSON value, except null as this is not allowed in OpenAPI
+            description: '`Value` represents a dynamically typed value which can be either null, a number, a string, a boolean, a recursive struct value, or a list of values. A producer of value is expected to set one of that variants, absence of any variant indicates an error. The JSON representation for `Value` is JSON value.'


If AnyJSONValue is kept, this description should also be kept.

This description is coming straight off of the google.protobuf.Value message here:
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto#L56L61
Which followed the pattern of using the message comments for the schema descriptions.

I can switch it back to a hard coded value though if that is preferred.

When running the schema through a JSON validator, you will get an error, if the value is null, but not if the property is not set at all.
So basically: it can not be null - it is our definition that is wrong, since it does not have null as a possible type, but we can not have null as a type, because OpenAPI v3.0.x does not allow that.
It will be doable in OpenAPI v3.1.0 as full JSON Schema is supported.

@jeffsawatzky and @timburks what is your opinions here?

I'm not actually a big fan of the comment for the Any message...it's way too long. So I might hard code that to something smaller anyway. If you want me to change the comment here for Value too I can do that.

As it is actually incorrect, due to the need of breaking that definition, I think it would make sense to use something like the original AnyJSONValue is a "catch all" type that can hold any JSON value, except null as this is not allowed in OpenAPI.
Maybe borrow some from the protobuf comment:
Value represents a dynamically typed value which can be any JSON value, except null.

For both AnyJSONValue and Value

@morphar I was referring to this big long comment for the Any message. It's way too huge.
https://github.com/google/gnostic/pull/327/files#diff-190e6a851c89c364cd83e2bd05fc44555cdb0d940341e5c531a4eae9c0c66308R29
I'm going to fix that up along with the one for the Value message.

Aaahh... Gotcha :) 👍

Also, while the "null" type is not allowed, we could make the schema nullable which would allow null values.

Nice catch!
That would also allow for proper handling of google.protobuf.Value, as it can then also be set to required:
... absence of any variant indicates an error ... 👍

morphar · 2022-03-31T00:58:34Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi_json.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'
-            description: AnyJSONValue is a "catch all" type that can hold any JSON value, except null as this is not allowed in OpenAPI
+            description: '`Value` represents a dynamically typed value which can be either null, a number, a string, a boolean, a recursive struct value, or a list of values. A producer of value is expected to set one of that variants, absence of any variant indicates an error. The JSON representation for `Value` is JSON value.'


Also keep, if AnyJSONValue is kept.

morphar · 2022-03-31T00:59:06Z

cmd/protoc-gen-openapi/examples/tests/protobuftypes/openapi_string_enum.yaml

@@ -246,7 +246,7 @@ components:
                - type: array
                  items:
                    $ref: '#/components/schemas/AnyJSONValue'
-            description: AnyJSONValue is a "catch all" type that can hold any JSON value, except null as this is not allowed in OpenAPI
+            description: '`Value` represents a dynamically typed value which can be either null, a number, a string, a boolean, a recursive struct value, or a list of values. A producer of value is expected to set one of that variants, absence of any variant indicates an error. The JSON representation for `Value` is JSON value.'


One more, if keep AnyJSONValue, then keep this as well

morphar · 2022-03-31T01:11:36Z

cmd/protoc-gen-openapi/generator/openapi-v3.go

 	name := getMessageName(message.Desc)
+	if !*g.conf.FQSchemaNaming {


This is also a question of: should we be consistent or backwards compatible.
I would personally prefer that types are the same, regardless of the naming.
I would be surprised if I changed the naming option and got different types.

I'm not sure about the history of the naming here, but without it being fully qualified with the package prefix then the name of the schema would just be Value which seems a little too general perhaps. Same with Any. I am happy to switch them both to just be Value and Any in the case non-fully-qualified names if that is what you prefer. I personally will always use the fully qualified names so it doesn't matter to me. :)

Aahh... Good point! You're absolutely right 👍 It would easily risk conflicting with a user's naming, which is why the AnyJSONValue name was chosen in the first place, if I remember correctly.

Would it make more sense to have GoogleProtobufValue instead of AnyJSONValue?

If you are ok with a possibly breaking change, that is fine with me. I'll do what the repo owners here want. :)

:) It makes sense to me, as one of the overall goals is to be able to convert both ways, so to have the "full" name, is probably more sane, than AnyJSONValue. What are your thoughts @timburks?

No strong opinion, but GoogleProtobufValue sounds more specific. I'm fine with whatever you two work out.

morphar · 2022-03-31T02:08:24Z

Isn't the description a bit wrong? The value can never be null, right?

I just checked a whole bunch of scenarios and validators.
It can not be null, but it's o.k. to not set the value, as long as it is not required.
So only the description is a bit wrong 👍

…ed their descriptions, and added nullable to Value

jeffsawatzky · 2022-03-31T04:30:31Z

@morphar I updated the name and descriptions of the Value and Any messages. I also added nullable to the Value so that it should allow null values now. This will only work with OpenAPI 3.0.x though as 3.1 removed nullable in favour of the null type. Though the version we specify is 3.0.3 so I'm not sure if this is a big deal.

morphar

Only the 1 comment. Otherwise it looks good to me 👍

morphar · 2022-03-31T04:34:53Z

cmd/protoc-gen-openapi/generator/openapi-v3.go

@@ -951,9 +951,9 @@ func (g *OpenAPIv3Generator) addSchemasToDocumentV3(d *v3.Document, messages []*
 					Value: &v3.SchemaOrReference{
 						Oneof: &v3.SchemaOrReference_Schema{
 							Schema: &v3.Schema{
-								Description: messageDescription,
+								Description: "Represents a dynamically typed value which can be either null, a number, a string, a boolean, a recursive struct value, or a list of values.",
+								Nullable:    true, // this only works with OpenAPI 3.0.x


Is it possible to set this to required as well?
Then it would adhere to both OpenAPI v3.0.x and the protobuf description, right?

morphar · 2022-03-31T04:39:52Z

@morphar I updated the name and descriptions of the Value and Any messages. I also added nullable to the Value so that it should allow null values now. This will only work with OpenAPI 3.0.x though as 3.1 removed nullable in favour of the null type. Though the version we specify is 3.0.3 so I'm not sure if this is a big deal.

I agree. We define 3.0.3, so we should adhere to that spec, ignoring 3.1.x in this code.
It would be nice to support v3.1.x as well though, but that's a completely different topic, how to actually handle that, I think :)

jeffsawatzky · 2022-03-31T05:09:26Z

I don't think I need to add "required" to the Value schema as the "Oneofs" handle that (the value HAS to be one of the defined types, or null since we allow nullable).
Reading up on the Value type here I noticed that the "struct" or object type has to have Value values, so I did add that restriction though.

morphar · 2022-03-31T06:53:35Z

Great! 👍

Regarding required: you're right 👍 It would have to be the property, that defines type as google.protobuf.Value, that also has required set to true, right?

absence of any variant indicates an error.

jeffsawatzky · 2022-03-31T12:00:58Z

Great! 👍

Regarding required: you're right 👍 It would have to be the property, that defines type as google.protobuf.Value, that also has required set to true, right?

absence of any variant indicates an error.

Yes, required would have to be set on the property of whatever parent schema defines a Value property.

morphar · 2022-04-01T05:23:02Z

Yes, required would have to be set on the property of whatever parent schema defines a Value property.

I'm just trying to understand if this is actually 100% compatible now.
So google.protobuf.Value can basically be anything, including null.
If a field uses this type, it MUST be set, otherwise it's an error.
All the the type constraints are now correctly converted to OpenAPI spec.

Is the required part also converted to OpenAPI?
Is required set on the property that has type set to google.protobuf.Value? Or is this not possible at all?

jeffsawatzky · 2022-04-01T13:41:42Z

Yes, required would have to be set on the property of whatever parent schema defines a Value property.

I'm just trying to understand if this is actually 100% compatible now. So google.protobuf.Value can basically be anything, including null. If a field uses this type, it MUST be set, otherwise it's an error. All the the type constraints are now correctly converted to OpenAPI spec.

Is the required part also converted to OpenAPI? Is required set on the property that has type set to google.protobuf.Value? Or is this not possible at all?

@morphar I'm not sure if it is possible or even needed to tell you the truth. From what I can tell, it isn't possible for a schema to require itself to be present in a parent schema. For example, if a Message has a Value field, it is up to the Message schema to specify that the Value field is required, and Value has no way of controlling this requirement itself.

With that said, I don't think it is needed anyway. According to the Protocol Buffer Language Guide:

If a value is missing in the JSON-encoded data or if its value is null, it will be interpreted as the appropriate default value
So whether the Value field is there and set to null, or missing, it will be treated the same way.

Furthermore, I think what we have for Value might actually be overkill. google.protobuf.Value just means that the JSON value can be any JSON value. You can achieve the same thing with a "typeless" schema
https://stackoverflow.com/a/43328994/2233608

FWIW, the protoc-gen-openapiv2 generator just uses "object" for Value types.

"v1Message": {
  "type": "object",
  "properties": {
    "value": {
      "type": "object"
    }
  }
}

morphar · 2022-04-01T15:05:33Z

@jeffsawatzky sorry, I was unclear. I meant if we should set the OpenAPI property as required.
If we could define it typeless / schemaless, that would simplify it hugely... But... As I read it, if a field (in proto) is defined as google.protobuf.Value, it is wrong to not set a value, though it can be null, it can not, not be set.
Do you agree with that understanding?
If that is true, a property in OpenAPI, should be defined as required AND have the type defined as the Value schema as it is now, right?

jeffsawatzky · 2022-04-01T15:45:10Z

@morphar I read the following:

Absence of any variant indicates an error

As:
"If any of the variants are missing when deserializing the binary protocol buffer version of the message then it's an error."

However, when it comes to JSON I don't think it matters since the JSON transcoding will treat a missing value as an implicit null value. The docs say:

The JSON representation for Value is JSON value

So I think we can represent Value as a typeless schema in OpenAPI which would allow any JSON value.

So, to answer your question, I don't think a property in OpenAPI needs to be defined as required AND have the type defined as the Value schema. If the property is missing the JSON transcoding will just treat it as null, which would satisfy the "NullType" variant automatically.

jeffsawatzky · 2022-04-01T17:46:54Z

@morphar seeing as how I have a few PRs queuing up, can we keep this PR focused on the Status and Any schemas? We can discuss the Value schema in another issue perhaps?

morphar · 2022-04-01T18:10:07Z

@jeffsawatzky Sorry, just did a test against Envoy. You're completely right, it is not required to be set, when the protobuf type is google.protobuf.Value 👍 So it is possible to go typeless / schemaless!

morphar

Everything looks good to me 👍
It's fully up to you, if you want the schemaless / typeless to be in another PR :)

morphar

I thought I approved the last review :)

morphar · 2022-04-01T18:23:40Z

Maybe @timburks needs to approve, for the merging to be unblocked?

…h less code

jeffsawatzky · 2022-04-01T18:39:09Z

@morphar since you confirmed we can go typeless I have made that change to make the openapi doc smaller.

morphar

👍 Beautiful! Thank you @jeffsawatzky :)

morphar · 2022-04-01T18:47:54Z

And sorry for the long discussion back and forth @jeffsawatzky! But nice that it resulted in a way better solution 👍

jeffsawatzky · 2022-04-01T21:18:24Z

@morphar No worries. I'm glad we were able to work out a solution together!

timburks · 2022-04-02T20:04:14Z

@morphar @jeffsawatzky thanks for all this! merging now

morphar · 2022-04-02T20:35:16Z

Great! Thanks @timburks 👍

jeffsawatzky requested a review from a team as a code owner March 29, 2022 17:33

jeffsawatzky mentioned this pull request Mar 29, 2022

[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

Merged

support google.rpc.Status and google.protobuf.Any. Also fix the fully…

9db6849

… qualified naming of google.protobuf.Value

jeffsawatzky force-pushed the grpc-status branch from f55cc29 to 9db6849 Compare March 30, 2022 18:36

timburks reviewed Mar 30, 2022

View reviewed changes

morphar reviewed Mar 31, 2022

View reviewed changes

updated the non-fully-qualified names for Value and Any types, modifi…

2f46098

…ed their descriptions, and added nullable to Value

morphar reviewed Mar 31, 2022

View reviewed changes

add restriction on the object type for value messages.

9f15636

jeffsawatzky mentioned this pull request Mar 31, 2022

[protoc-gen-openapi] Refactoring some code around #328

Merged

jeffsawatzky mentioned this pull request Apr 1, 2022

Add a Default Response for Error Responses Based on google.rpc.Status #329

Merged

morphar approved these changes Apr 1, 2022

View reviewed changes

make GoogleProtobufValue typeless which accomplishes what we want wit…

b0b537d

…h less code

morphar approved these changes Apr 1, 2022

View reviewed changes

timburks approved these changes Apr 2, 2022

View reviewed changes

timburks merged commit e954645 into google:master Apr 2, 2022

jeffsawatzky deleted the grpc-status branch April 2, 2022 23:17

		name := getMessageName(message.Desc)
		if !*g.conf.FQSchemaNaming {

[protoc-gen-openapi] Support google.protobuf.Any and google.rpc.Status #327

[protoc-gen-openapi] Support google.protobuf.Any and google.rpc.Status #327

Conversation

jeffsawatzky commented Mar 29, 2022

Purpose

jeffsawatzky commented Mar 29, 2022

timburks commented Mar 30, 2022 • edited Loading

jeffsawatzky commented Mar 30, 2022

timburks 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

Choose a reason for hiding this comment

morphar left a comment • edited Loading

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

morphar Mar 31, 2022 • edited Loading

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

jeffsawatzky Mar 31, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

morphar commented Mar 31, 2022 • edited Loading

jeffsawatzky commented Mar 31, 2022

morphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morphar commented Mar 31, 2022

jeffsawatzky commented Mar 31, 2022

morphar commented Mar 31, 2022

jeffsawatzky commented Mar 31, 2022

morphar commented Apr 1, 2022

jeffsawatzky commented Apr 1, 2022

morphar commented Apr 1, 2022 • edited Loading

jeffsawatzky commented Apr 1, 2022 • edited Loading

jeffsawatzky commented Apr 1, 2022

morphar commented Apr 1, 2022

morphar left a comment

Choose a reason for hiding this comment

morphar left a comment

Choose a reason for hiding this comment

morphar commented Apr 1, 2022 • edited Loading

jeffsawatzky commented Apr 1, 2022

morphar left a comment

Choose a reason for hiding this comment

morphar commented Apr 1, 2022

jeffsawatzky commented Apr 1, 2022

timburks commented Apr 2, 2022

morphar commented Apr 2, 2022

timburks commented Mar 30, 2022 •

edited

Loading

morphar left a comment •

edited

Loading

morphar Mar 31, 2022 •

edited

Loading

jeffsawatzky Mar 31, 2022 •

edited

Loading

morphar commented Mar 31, 2022 •

edited

Loading

morphar commented Apr 1, 2022 •

edited

Loading

jeffsawatzky commented Apr 1, 2022 •

edited

Loading

morphar commented Apr 1, 2022 •

edited

Loading