[+] support integer64 as string option #355
Conversation
There was a problem hiding this comment.
Thanks @ppaanngggg looks good 👍
Maybe 64 bit types should default to string as described in the link.
What are your thoughts on this @timburks?
| for _, tt := range openapiTests { | ||
| fixture := path.Join(tt.path, "openapi_string_integer64.yaml") | ||
| if _, err := os.Stat(fixture); errors.Is(err, os.ErrNotExist) { | ||
| if !GENERATE_FIXTURES { |
There was a problem hiding this comment.
Doe this have any relevance to gnostic? Or is it something used locally?
There was a problem hiding this comment.
sorry, I don't understand, I just copy from other unit tests
There was a problem hiding this comment.
Sorry. I thought GENERATE_FIXTURES was new.
| if err != nil { | ||
| t.Fatalf("protoc failed: %+v", err) | ||
| } | ||
| if GENERATE_FIXTURES { |
I think it should be in the future. Because |
|
@ppaanngggg Thanks for catching this! I would prefer to just make this the correct behavior and not have an option. @morphar do you need the old behavior? |
|
@timburks I think it should be default. I just concern about it will break something, afterall I am not familiar with other things in this project. |
@timburks no. I will always go for "most correct behavior". Note: I haven't tested this though. Envoy may allow int values as input for int64. |
|
@ppaanngggg let's please make this fix the new behavior (with no option). The link @morphar cited is a good example for us to follow in keeping things simple and using https://developers.google.com/protocol-buffers/docs/proto3#json as our standard. @ppaanngggg thanks for finding and fixing this! |
According to doc before, protojson allow both integer and string input, but only string output. @timburks Done |
| case protoreflect.Int64Kind, protoreflect.Sint64Kind, protoreflect.Uint64Kind, | ||
| protoreflect.Sfixed64Kind, protoreflect.Fixed64Kind: | ||
| kindSchema = wk.NewStringSchema() | ||
|
|
There was a problem hiding this comment.
This is great - such a clear and simple change! Thanks!
|
Would it be possible to consider reverting this change? My understanding is that the I believe the guidance for protobuf to json encoding is irrelevant here, it's important to recognize that the code in question is not encoding a protobuf message as json, but rather generating an OpenAPI v3 specification from a protobuf definition which can then be used to generate both server and client code in any number of different languages each of which will have its own way of knowing how it needs to handle int64. As the I would be happy to take on the work of reviewing and reverting this change either in totality or in part to restore the functionality of representing int64 data types as integers within the generated v3 specifications. However, I would like to emphasize that this original change seems to be confusing the generation of a OpenAPI v3 spec definition with guidance for serializing a protobuf message as json. |
This reverts commit c62333b. The original change conflated the guidance for encoding protobuf messages as json objects with generating an OpenAPI v3 spec file. OpenAPI v3 explicitly allows for integer types of int64 (https://swagger.io/docs/specification/data-models/data-types/#numbers). The OpenAPI v3 spec generated by this tool will then usually be fed into language specific generators which will determine how they want to handle each of the types depending on the specific target language.
This reverts commit c62333b. The original change conflated the guidance for encoding protobuf messages as json objects with generating an OpenAPI v3 spec file. OpenAPI v3 explicitly allows for integer types of int64 (https://swagger.io/docs/specification/data-models/data-types/#numbers). The OpenAPI v3 spec generated by this tool will then usually be fed into language specific generators which will determine how they want to handle each of the types depending on the specific target language.
as https://developers.google.com/protocol-buffers/docs/proto3#json
In fact, int64, fixed64, uint64 should be string in json.
So add option to turn int64, fixed64, uint64 to string in openapi