Skip to content
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

feat: support Discovery "format" specifier for google.protobuf.{Value,ListValue,Struct} #131

Merged
merged 18 commits into from
Oct 10, 2024

Conversation

vchudnov-g
Copy link
Collaborator

@vchudnov-g vchudnov-g commented Jul 31, 2024

The converter will now recognize a format specifier in certain schemas. Specifically, the following combinations are now recognized and result in a protobuf field whose type is the specified format:

Discovery "type" Discovery "format"
object google.protobuf.Struct
any google.protobuf.Value
array google.protobuf.ListValue

Note that, as was the case previously, other schemas with "type" = "any" and any other format, including none, are not accepted unless they are in an error.details field. (Googlers, see also cl/683528084.)

Incidentally, this PR also restores proto imports in the intended order, undoing the workaround in #108 due to protobufjs/protobuf.js#1954 now that the solution protobufjs/protobuf.js#1960 is implemented.

This commit restores proto imports in the intended order, undoing the
workaround in #108 due
to protobufjs/protobuf.js#1954 now that the
solution protobufjs/protobuf.js#1960 is
implemented.
The schema "type" corresponding to each of these formats is as seen in
the baseline file `compute.any-format.proto.baseline`.

Googlers, also see cl/683528084

NOTE: that we continue to not handle `google.protobuf.Any`, whether it
arises from a format specification with that value or from a `"type":
"any"` with no "format" specified.
@vchudnov-g vchudnov-g changed the title feat: support Discovery "format" specifier for google.protobuf.{Value,ListValue,Struct,Any} feat: support Discovery "format" specifier for google.protobuf.{Value,ListValue,Struct} Oct 9, 2024
The command I ran was:

gbazelisk run google_java_format_binary -- -r `pwd`/{src/main/java/com/google/cloud/discotoproto3converter/proto3/{DocumentToProtoConverter,Message},src/test/java/com/google/cloud/discotoproto3converter/DiscoToProto3ConverterAppTest}.java
@vchudnov-g vchudnov-g marked this pull request as ready for review October 9, 2024 22:54
@vchudnov-g vchudnov-g requested review from a team as code owners October 9, 2024 22:54
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks solid! A few comment nits but not blocking

The command I ran was:

gbazelisk run google_java_format_binary -- -r `pwd`/{src/main/java/com/google/cloud/discotoproto3converter/proto3/{DocumentToProtoConverter,Message},src/test/java/com/google/cloud/discotoproto3converter/DiscoToProto3ConverterAppTest}.java
@vchudnov-g vchudnov-g merged commit 82aeac1 into main Oct 10, 2024
4 checks passed
@vchudnov-g vchudnov-g deleted the any-value-struct branch October 10, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants