fix: Replace deprecated protobuf methods.

[blakeli0]

isSynthetic() is deprecated and will be removed in protobuf-java 4.26.0, replace it so that we can prepare for protobuf-java upgrade.

Per official doc of protobuf, an optional field is implemented as a "synthetic" oneof field internally. Also see b/266950618#comment123 for suggested replacements of isSynthetic().

[sonarcloud]

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[lqiu96]

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

[blakeli0]

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.

[lqiu96]

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

[blakeli0]

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.

[lqiu96]

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.