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.

Sign up for GitHub

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

fix: Replace deprecated protobuf methods. #2764

Merged
merged 6 commits into from
May 15, 2024
+2 −4
Merged

fix: Replace deprecated protobuf methods. #2764

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
53549f6
fix: Replace deprecated protobuf methods.
blakeli0 May 11, 2024
5a2060d
fix: Use getRealContainingOneof() == null to replace isSynthetic()
blakeli0 May 14, 2024
8b7f3f8
Merge branch 'main' into fix-deprecated-proto-methods
blakeli0 May 14, 2024
0d45723
Merge branch 'main' into fix-deprecated-proto-methods
blakeli0 May 14, 2024
28b1139
fix: format
blakeli0 May 14, 2024
17390e8
Merge branch 'main' into fix-deprecated-proto-methods
blakeli0 May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
View file
Original file line number Diff line number Diff line change
@@ -1063,12 +1063,10 @@ private static Field parseField(
.setType(TypeParser.parseType(fieldDescriptor))
.setIsMessage(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE)
.setIsEnum(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM)
.setIsContainedInOneof(
fieldDescriptor.getContainingOneof() != null
&& !fieldDescriptor.getContainingOneof().isSynthetic())
.setIsContainedInOneof(fieldDescriptor.getRealContainingOneof() != null)
.setIsProto3Optional(
fieldDescriptor.getContainingOneof() != null
&& fieldDescriptor.getContainingOneof().isSynthetic())
&& fieldDescriptor.getRealContainingOneof() == null)
Comment on lines 1068 to +1069
Copy link
Contributor

@lqiu96 lqiu96 May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can this just be fieldDescriptor.getRealContainingOneof() == null)? The implementation looks like it's already checking for fieldDescriptor.getContaintingOneof() != null: https://github.com/protocolbuffers/protobuf/blob/dde03553c92867184ff5d351b7f087c052f39459/java/core/src/main/java/com/google/protobuf/Descriptors.java#L1435-L1438

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a fieldDescriptor does not contain any oneof(real or synthetic) field, fieldDescriptor.getRealContainingOneof() == null would return true, but fieldDescriptor.getContainingOneof() != null && fieldDescriptor.getRealContainingOneof() == null would return false.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It needs to check that it's a oneOf before checking for the optional implementation or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. That being said, based on the field name isProto3Optional, I'm not sure the current logic is what it intended to be. I feel like fieldDescriptor.isOptional() may make more sense but it would introduce changes to the generated code. So I decided to go with the safest approach for now, and we can improve it in the future if we see issues.

Copy link
Contributor

@lqiu96 lqiu96 May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like fieldDescriptor.isOptional() may make more sense but it would introduce changes to the generated code.

Hmm, the more I look at this, the more I'm confusing myself. My assumption is that this logic is for code written with pre protobuf v3.15 compatibility in mind (i.e. people writing their own optional workaround -- writing their own oneof with a single field).

Looking at the implementation of isOptional makes it seems like it should work except for the optional workaround case (i.e. writing their own oneof with a single field).

I'm not entirely sure if protoc would have something like this marked as synthetic. This is where the user explicitly writes their optional workaround:

  oneof _foo {
    int32 foo = 1;
  }

I know that under the hood that an optional int32 foo is converted to it above, but I'm not sure the descriptor marks things correctly if the user explicitly writes it out. I'm guessing this is the case since they had backwards compatibility in mind and I'll probably need to test this myself whenever I get some time.

I think the method probably should be:

// Renamed to IsOptional and checks for the optional label and the optional workaround
// to keep the existing generator behavior the same. If protobuf decides to remove the
// under the hood conversion of optional -> oneof, then `isOptional()` should suffice.
.setIsOptional( 
            fieldDescriptor.isOptional() ||
            (fieldDescriptor.getContainingOneof() != null
                && fieldDescriptor.getRealContainingOneof() == null))

but the existing implementation should work and this can be something we look again in the future.

.setIsRepeated(fieldDescriptor.isRepeated())
.setIsRequired(isRequired)
.setFieldInfoFormat(fieldInfoFormat)