Skip to content

Commit

Permalink
Handle clear for Java proto3 optionals (synthetic oneofs) using field…
Browse files Browse the repository at this point in the history 
… descriptor instead of clear method.

This treats clear similarly to has and get to avoids issues from missing clear method for escaped synthetic oneofs (e.g. field _underscore -> oneof X_underscore). Previously, `clear` was using the clear method of the field (which has the same camel-cased name outside of the underscore case).

We also remove synthetic oneof camelCase names from the gencode for FieldAccesorTable since these should not be used / exposed.

Fixes #12880

PiperOrigin-RevId: 539069001
  • Loading branch information
@zhangskz @copybara-github
zhangskz authored and copybara-github committed Jun 9, 2023
1 parent 6831d20 commit a534902
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 46 deletions.
122 changes: 77 additions & 45 deletions java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2096,8 +2096,10 @@ public FieldAccessorTable ensureFieldAccessorsInitialized(
FieldDescriptor field = descriptor.getFields().get(i);
String containingOneofCamelCaseName = null;
if (field.getContainingOneof() != null) {
containingOneofCamelCaseName =
camelCaseNames[fieldsSize + field.getContainingOneof().getIndex()];
int index = fieldsSize + field.getContainingOneof().getIndex();
if (index < camelCaseNames.length) {
containingOneofCamelCaseName = camelCaseNames[index];
}
}
if (field.isRepeated()) {
if (field.getJavaType() == FieldDescriptor.JavaType.MESSAGE) {
Expand Down Expand Up @@ -2153,12 +2155,16 @@ public FieldAccessorTable ensureFieldAccessorsInitialized(
}
}

int oneofsSize = oneofs.length;
for (int i = 0; i < oneofsSize; i++) {
oneofs[i] =
new OneofAccessor(
descriptor, i, camelCaseNames[i + fieldsSize], messageClass, builderClass);
for (int i = 0; i < descriptor.getOneofs().size(); i++) {
if (i < descriptor.getRealOneofs().size()) {
oneofs[i] =
new RealOneofAccessor(
descriptor, i, camelCaseNames[i + fieldsSize], messageClass, builderClass);
} else {
oneofs[i] = new SyntheticOneofAccessor(descriptor, i);
}
}

initialized = true;
camelCaseNames = null;
return this;
Expand Down Expand Up @@ -2230,80 +2236,106 @@ private interface FieldAccessor {
}

/** OneofAccessor provides access to a single oneof. */
private static class OneofAccessor {
OneofAccessor(
private static interface OneofAccessor {
public boolean has(final GeneratedMessageV3 message);

public boolean has(GeneratedMessageV3.Builder<?> builder);

public FieldDescriptor get(final GeneratedMessageV3 message);

public FieldDescriptor get(GeneratedMessageV3.Builder<?> builder);

public void clear(final Builder<?> builder);
}

/** RealOneofAccessor provides access to a single real oneof. */
private static class RealOneofAccessor implements OneofAccessor {
RealOneofAccessor(
final Descriptor descriptor,
final int oneofIndex,
final String camelCaseName,
final Class<? extends GeneratedMessageV3> messageClass,
final Class<? extends Builder<?>> builderClass) {
this.descriptor = descriptor;
OneofDescriptor oneofDescriptor = descriptor.getOneofs().get(oneofIndex);
if (oneofDescriptor.isSynthetic()) {
caseMethod = null;
caseMethodBuilder = null;
fieldDescriptor = oneofDescriptor.getFields().get(0);
} else {
caseMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Case");
caseMethodBuilder = getMethodOrDie(builderClass, "get" + camelCaseName + "Case");
fieldDescriptor = null;
}
caseMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Case");
caseMethodBuilder = getMethodOrDie(builderClass, "get" + camelCaseName + "Case");
clearMethod = getMethodOrDie(builderClass, "clear" + camelCaseName);
}

private final Descriptor descriptor;
private final Method caseMethod;
private final Method caseMethodBuilder;
private final Method clearMethod;
private final FieldDescriptor fieldDescriptor;

@Override
public boolean has(final GeneratedMessageV3 message) {
if (fieldDescriptor != null) {
return message.hasField(fieldDescriptor);
} else {
return ((Internal.EnumLite) invokeOrDie(caseMethod, message)).getNumber() != 0;
}
return ((Internal.EnumLite) invokeOrDie(caseMethod, message)).getNumber() != 0;
}

@Override
public boolean has(GeneratedMessageV3.Builder<?> builder) {
if (fieldDescriptor != null) {
return builder.hasField(fieldDescriptor);
} else {
return ((Internal.EnumLite) invokeOrDie(caseMethodBuilder, builder)).getNumber() != 0;
}
return ((Internal.EnumLite) invokeOrDie(caseMethodBuilder, builder)).getNumber() != 0;
}

@Override
public FieldDescriptor get(final GeneratedMessageV3 message) {
if (fieldDescriptor != null) {
return message.hasField(fieldDescriptor) ? fieldDescriptor : null;
} else {
int fieldNumber = ((Internal.EnumLite) invokeOrDie(caseMethod, message)).getNumber();
if (fieldNumber > 0) {
return descriptor.findFieldByNumber(fieldNumber);
}
int fieldNumber = ((Internal.EnumLite) invokeOrDie(caseMethod, message)).getNumber();
if (fieldNumber > 0) {
return descriptor.findFieldByNumber(fieldNumber);
}
return null;
}

@Override
public FieldDescriptor get(GeneratedMessageV3.Builder<?> builder) {
if (fieldDescriptor != null) {
return builder.hasField(fieldDescriptor) ? fieldDescriptor : null;
} else {
int fieldNumber =
((Internal.EnumLite) invokeOrDie(caseMethodBuilder, builder)).getNumber();
if (fieldNumber > 0) {
return descriptor.findFieldByNumber(fieldNumber);
}
int fieldNumber = ((Internal.EnumLite) invokeOrDie(caseMethodBuilder, builder)).getNumber();
if (fieldNumber > 0) {
return descriptor.findFieldByNumber(fieldNumber);
}
return null;
}

@Override
public void clear(final Builder<?> builder) {
// TODO(b/230609037): remove the unused variable
Object unused = invokeOrDie(clearMethod, builder);
}
}

/** SyntheticOneofAccessor provides access to a single synthetic oneof. */
private static class SyntheticOneofAccessor implements OneofAccessor {
SyntheticOneofAccessor(final Descriptor descriptor, final int oneofIndex) {
OneofDescriptor oneofDescriptor = descriptor.getOneofs().get(oneofIndex);
fieldDescriptor = oneofDescriptor.getFields().get(0);
}

private final FieldDescriptor fieldDescriptor;

@Override
public boolean has(final GeneratedMessageV3 message) {
return message.hasField(fieldDescriptor);
}

@Override
public boolean has(GeneratedMessageV3.Builder<?> builder) {
return builder.hasField(fieldDescriptor);
}

@Override
public FieldDescriptor get(final GeneratedMessageV3 message) {
return message.hasField(fieldDescriptor) ? fieldDescriptor : null;
}

public FieldDescriptor get(GeneratedMessageV3.Builder<?> builder) {
return builder.hasField(fieldDescriptor) ? fieldDescriptor : null;
}

@Override
public void clear(final Builder<?> builder) {
builder.clearField(fieldDescriptor);
}
}

// ---------------------------------------------------------------

@SuppressWarnings("SameNameButDifferent")
Expand Down
3 changes: 2 additions & 1 deletion src/google/protobuf/compiler/java/message.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,8 @@ int ImmutableMessageGenerator::GenerateFieldAccessorTableInitializer(
bytecode_estimate += 6;
printer->Print("\"$field_name$\", ", "field_name", info->capitalized_name);
}
// We reproduce synthetic oneofs here since proto reflection needs these.
// TODO(b/286434301): Once cl/534906231 propagates, only consider real oneofs
// since proto reflection does not use this for synthetic oneofs.
for (int i = 0; i < descriptor_->oneof_decl_count(); i++) {
const OneofDescriptor* oneof = descriptor_->oneof_decl(i);
const OneofGeneratorInfo* info = context_->GetOneofGeneratorInfo(oneof);
Expand Down

0 comments on commit a534902

Please sign in to comment.