From 343cef1bd940380ae1bf23a53f565ca93ffe6c3e Mon Sep 17 00:00:00 2001 From: Sandy Zhang Date: Mon, 27 Feb 2023 07:58:59 -0800 Subject: [PATCH] Replace FileDescriptor.supportsUnknownEnumValues() with FieldDescriptor.legacyEnumFieldTreatedAsClosed() to support legacy enum field behavior. This documents Java's non-conformant behavior where closedness is determined by the field rather than the enum definition. PiperOrigin-RevId: 512627297 --- .../java/com/google/protobuf/Descriptors.java | 28 +++- .../com/google/protobuf/GeneratedMessage.java | 4 +- .../google/protobuf/GeneratedMessageV3.java | 4 +- .../google/protobuf/MessageReflection.java | 14 +- .../com/google/protobuf/DescriptorsTest.java | 124 ++++++++++++++++++ 5 files changed, 159 insertions(+), 15 deletions(-) diff --git a/java/core/src/main/java/com/google/protobuf/Descriptors.java b/java/core/src/main/java/com/google/protobuf/Descriptors.java index e5b973f158..5e458da85b 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -634,10 +634,6 @@ public final class Descriptors { extensions[i].setProto(proto.getExtension(i)); } } - - boolean supportsUnknownEnumValue() { - return getSyntax() == Syntax.PROTO3; - } } // ================================================================= @@ -1328,6 +1324,30 @@ public final class Descriptors { return enumType; } + /** + * Determines if the given enum field is treated as closed based on legacy non-conformant + * behavior. + * + *

Conformant behavior determines closedness based on the enum and can be queried using + * {@code EnumDescriptor.isClosed()}. + * + *

Some runtimes currently have a quirk where non-closed enums are treated as closed when + * used as the type of fields defined in a `syntax = proto2;` file. This quirk is not present in + * all runtimes; as of writing, we know that: + * + *

+ * + *

Care should be taken when using this function to respect the target runtime's enum + * handling quirks. + */ + boolean legacyEnumFieldTreatedAsClosed() { + return getType() == Type.ENUM && getFile().getSyntax() == Syntax.PROTO2; + } + /** * Compare with another {@code FieldDescriptor}. This orders fields in "canonical" order, which * simply means ascending order by field number. {@code other} must be a field of the same type. diff --git a/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java b/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java index 26cc5bb759..e62a38218d 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java @@ -2646,7 +2646,7 @@ public abstract class GeneratedMessage extends AbstractMessage implements Serial valueOfMethod = getMethodOrDie(type, "valueOf", EnumValueDescriptor.class); getValueDescriptorMethod = getMethodOrDie(type, "getValueDescriptor"); - supportUnknownEnumValue = descriptor.getFile().supportsUnknownEnumValue(); + supportUnknownEnumValue = !descriptor.legacyEnumFieldTreatedAsClosed(); if (supportUnknownEnumValue) { getValueMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Value"); getValueMethodBuilder = getMethodOrDie(builderClass, "get" + camelCaseName + "Value"); @@ -2705,7 +2705,7 @@ public abstract class GeneratedMessage extends AbstractMessage implements Serial valueOfMethod = getMethodOrDie(type, "valueOf", EnumValueDescriptor.class); getValueDescriptorMethod = getMethodOrDie(type, "getValueDescriptor"); - supportUnknownEnumValue = descriptor.getFile().supportsUnknownEnumValue(); + supportUnknownEnumValue = !descriptor.legacyEnumFieldTreatedAsClosed(); if (supportUnknownEnumValue) { getRepeatedValueMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Value", int.class); diff --git a/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java b/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java index b5d3c76c5f..3fb6ea2586 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java @@ -2889,7 +2889,7 @@ public abstract class GeneratedMessageV3 extends AbstractMessage implements Seri valueOfMethod = getMethodOrDie(type, "valueOf", EnumValueDescriptor.class); getValueDescriptorMethod = getMethodOrDie(type, "getValueDescriptor"); - supportUnknownEnumValue = descriptor.getFile().supportsUnknownEnumValue(); + supportUnknownEnumValue = !descriptor.legacyEnumFieldTreatedAsClosed(); if (supportUnknownEnumValue) { getValueMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Value"); getValueMethodBuilder = getMethodOrDie(builderClass, "get" + camelCaseName + "Value"); @@ -2950,7 +2950,7 @@ public abstract class GeneratedMessageV3 extends AbstractMessage implements Seri valueOfMethod = getMethodOrDie(type, "valueOf", EnumValueDescriptor.class); getValueDescriptorMethod = getMethodOrDie(type, "getValueDescriptor"); - supportUnknownEnumValue = descriptor.getFile().supportsUnknownEnumValue(); + supportUnknownEnumValue = !descriptor.legacyEnumFieldTreatedAsClosed(); if (supportUnknownEnumValue) { getRepeatedValueMethod = getMethodOrDie(messageClass, "get" + camelCaseName + "Value", int.class); diff --git a/java/core/src/main/java/com/google/protobuf/MessageReflection.java b/java/core/src/main/java/com/google/protobuf/MessageReflection.java index 0404042019..06cb7cd452 100644 --- a/java/core/src/main/java/com/google/protobuf/MessageReflection.java +++ b/java/core/src/main/java/com/google/protobuf/MessageReflection.java @@ -1199,10 +1199,7 @@ class MessageReflection { if (field.getLiteType() == WireFormat.FieldType.ENUM) { while (input.getBytesUntilLimit() > 0) { final int rawValue = input.readEnum(); - if (field.getFile().supportsUnknownEnumValue()) { - target.addRepeatedField( - field, field.getEnumType().findValueByNumberCreatingIfUnknown(rawValue)); - } else { + if (field.legacyEnumFieldTreatedAsClosed()) { final Object value = field.getEnumType().findValueByNumber(rawValue); // If the number isn't recognized as a valid value for this enum, // add it to the unknown fields. @@ -1213,6 +1210,9 @@ class MessageReflection { } else { target.addRepeatedField(field, value); } + } else { + target.addRepeatedField( + field, field.getEnumType().findValueByNumberCreatingIfUnknown(rawValue)); } } } else { @@ -1239,9 +1239,7 @@ class MessageReflection { } case ENUM: final int rawValue = input.readEnum(); - if (field.getFile().supportsUnknownEnumValue()) { - value = field.getEnumType().findValueByNumberCreatingIfUnknown(rawValue); - } else { + if (field.legacyEnumFieldTreatedAsClosed()) { value = field.getEnumType().findValueByNumber(rawValue); // If the number isn't recognized as a valid value for this enum, // add it to the unknown fields. @@ -1251,6 +1249,8 @@ class MessageReflection { } return true; } + } else { + value = field.getEnumType().findValueByNumberCreatingIfUnknown(rawValue); } break; default: diff --git a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java index 38e547c37c..1cd1bb6473 100644 --- a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java +++ b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java @@ -289,6 +289,130 @@ public class DescriptorsTest { assertThat(d.findFieldByName("large_uint64").getDefaultValue()).isEqualTo(-1L); } + @Test + public void testFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { + // Make an open enum definition. + FileDescriptorProto openEnumFile = + FileDescriptorProto.newBuilder() + .setName("open_enum.proto") + .setSyntax("proto3") + .addEnumType( + EnumDescriptorProto.newBuilder() + .setName("TestEnumOpen") + .addValue( + EnumValueDescriptorProto.newBuilder() + .setName("TestEnumOpen_VALUE0") + .setNumber(0) + .build()) + .build()) + .build(); + FileDescriptor openFileDescriptor = + Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); + EnumDescriptor openEnum = openFileDescriptor.getEnumTypes().get(0); + assertThat(openEnum.isClosed()).isFalse(); + + // Create a message that treats enum fields as closed. + FileDescriptorProto closedEnumFile = + FileDescriptorProto.newBuilder() + .setName("closed_enum_field.proto") + .addDependency("open_enum.proto") + .setSyntax("proto2") + .addEnumType( + EnumDescriptorProto.newBuilder() + .setName("TestEnum") + .addValue( + EnumValueDescriptorProto.newBuilder() + .setName("TestEnum_VALUE0") + .setNumber(0) + .build()) + .build()) + .addMessageType( + DescriptorProto.newBuilder() + .setName("TestClosedEnumField") + .addField( + FieldDescriptorProto.newBuilder() + .setName("int_field") + .setNumber(1) + .setType(FieldDescriptorProto.Type.TYPE_INT32) + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .addField( + FieldDescriptorProto.newBuilder() + .setName("open_enum") + .setNumber(2) + .setType(FieldDescriptorProto.Type.TYPE_ENUM) + .setTypeName("TestEnumOpen") + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .addField( + FieldDescriptorProto.newBuilder() + .setName("closed_enum") + .setNumber(3) + .setType(FieldDescriptorProto.Type.TYPE_ENUM) + .setTypeName("TestEnum") + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .build()) + .build(); + Descriptor closedMessage = + Descriptors.FileDescriptor.buildFrom( + closedEnumFile, new FileDescriptor[] {openFileDescriptor}) + .getMessageTypes() + .get(0); + assertThat(closedMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()) + .isFalse(); + + assertThat(closedMessage.findFieldByName("closed_enum").legacyEnumFieldTreatedAsClosed()) + .isTrue(); + assertThat(closedMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()) + .isTrue(); + } + + @Test + public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { + // Make an open enum definition and message that treats enum fields as open. + FileDescriptorProto openEnumFile = + FileDescriptorProto.newBuilder() + .setName("open_enum.proto") + .setSyntax("proto3") + .addEnumType( + EnumDescriptorProto.newBuilder() + .setName("TestEnumOpen") + .addValue( + EnumValueDescriptorProto.newBuilder() + .setName("TestEnumOpen_VALUE0") + .setNumber(0) + .build()) + .build()) + .addMessageType( + DescriptorProto.newBuilder() + .setName("TestOpenEnumField") + .addField( + FieldDescriptorProto.newBuilder() + .setName("int_field") + .setNumber(1) + .setType(FieldDescriptorProto.Type.TYPE_INT32) + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .addField( + FieldDescriptorProto.newBuilder() + .setName("open_enum") + .setNumber(2) + .setType(FieldDescriptorProto.Type.TYPE_ENUM) + .setTypeName("TestEnumOpen") + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .build()) + .build(); + FileDescriptor openEnumFileDescriptor = + Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); + Descriptor openMessage = openEnumFileDescriptor.getMessageTypes().get(0); + EnumDescriptor openEnum = openEnumFileDescriptor.findEnumTypeByName("TestEnumOpen"); + assertThat(openEnum.isClosed()).isFalse(); + assertThat(openMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()).isFalse(); + assertThat(openMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()).isFalse(); + } + @Test public void testEnumDescriptor() throws Exception { EnumDescriptor enumType = ForeignEnum.getDescriptor();