Skip to content

Commit

Permalink
Fix text-format delimited field handling
Browse files Browse the repository at this point in the history 
This updates all our text parsers and serializers to better handle tag-delimited fields under editions.  Under proto2, groups were the only tag-delimited fields possible, and the group name (i.e. the message type) was guaranteed to be unique.  Text-format and various generators used this instead of the synthetic field name (lower-cased group name) to represent these fields.

Under editions, we've removed group syntax and allowed any message field to be tag-delimited.  This breaks those cases when adding new tag-delimited fields where the message type might not be unique or correspond to the field name.  Code generators have already been fixed to treat "group-like" fields using the old behavior, and treat new fields like any other sub-message.

This change addresses the text-format issue.  Text parsers will accept *either* the type or field name for "group-like" fields, and only the field name for every other message field.  Text serializers will continue to emit the message name for "group-like" fields, but also use the field name for everything else.

This creates some awkward capitalization behavior for fields that happen to *look* like proto2 groups, but it won't lead to any conflicts or invalid encodings.  A feature will likely be added to edition 2024 to allow for migration off this legacy behavior.

PiperOrigin-RevId: 622260327
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Apr 5, 2024
1 parent 7dc243c commit 29c69ff
Show file tree
Hide file tree
Showing 11 changed files with 464 additions and 44 deletions.
21 changes: 10 additions & 11 deletions conformance/text_format_conformance_suite.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,12 +260,11 @@ void TextFormatConformanceTestSuiteImpl<MessageType>::RunDelimitedTests() {
"DelimitedFieldExtension", REQUIRED,
"[protobuf_test_messages.editions.delimited_ext] { c: 1 }");

// Test that lower-cased group name (i.e. implicit field name) is not accepted
// for now.
ExpectParseFailure("DelimitedFieldLowercased", REQUIRED,
"groupliketype { group_int32: 1 }");
ExpectParseFailure("DelimitedFieldLowercasedDifferent", REQUIRED,
"delimited_field { group_int32: 1 }");
// Test that lower-cased group name (i.e. implicit field name) are accepted.
RunValidTextFormatTest("DelimitedFieldLowercased", REQUIRED,
"groupliketype { group_int32: 1 }");
RunValidTextFormatTest("DelimitedFieldLowercasedDifferent", REQUIRED,
"delimited_field { group_int32: 1 }");

// Extensions always used the field name, and should never accept the message
// name.
Expand All @@ -284,11 +283,11 @@ void TextFormatConformanceTestSuiteImpl<MessageType>::RunGroupTests() {
RunValidTextFormatTest("GroupFieldMultiWord", REQUIRED,
"MultiWordGroupField { group_int32: 1 }");

// Test that lower-cased group name (i.e. implicit field name) is not accepted
ExpectParseFailure("GroupFieldLowercased", REQUIRED,
"data { group_int32: 1 }");
ExpectParseFailure("GroupFieldLowercasedMultiWord", REQUIRED,
"multiwordgroupfield { group_int32: 1 }");
// Test that lower-cased group name (i.e. implicit field name) is accepted
RunValidTextFormatTest("GroupFieldLowercased", REQUIRED,
"data { group_int32: 1 }");
RunValidTextFormatTest("GroupFieldLowercasedMultiWord", REQUIRED,
"multiwordgroupfield { group_int32: 1 }");

// Test extensions of group type
RunValidTextFormatTest("GroupFieldExtension", REQUIRED,
Expand Down
29 changes: 29 additions & 0 deletions java/core/src/main/java/com/google/protobuf/Descriptors.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1470,6 +1470,35 @@ public boolean hasPresence() {
|| this.features.getFieldPresence() != DescriptorProtos.FeatureSet.FieldPresence.IMPLICIT;
}

/**
* Returns true if this field is structured like the synthetic field of a proto2 group. This
* allows us to expand our treatment of delimited fields without breaking proto2 files that have
* been upgraded to editions.
*/
boolean isGroupLike() {
if (features.getMessageEncoding() != DescriptorProtos.FeatureSet.MessageEncoding.DELIMITED) {
// Groups are always tag-delimited.
return false;
}

if (!getMessageType().getName().toLowerCase().equals(getName())) {
// Group fields always are always the lowercase type name.
return false;
}

if (getMessageType().getFile() != getFile()) {
// Groups could only be defined in the same file they're used.
return false;
}

// Group messages are always defined in the same scope as the field. File level extensions
// will compare NULL == NULL here, which is why the file comparison above is necessary to
// ensure both come from the same file.
return isExtension()
? getMessageType().getContainingType() == getExtensionScope()
: getMessageType().getContainingType() == getContainingType();
}

/**
* For extensions defined nested within message types, gets the outer type. Not valid for
* non-extension fields. For example, consider this {@code .proto} file:
Expand Down
13 changes: 5 additions & 8 deletions java/core/src/main/java/com/google/protobuf/TextFormat.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ private void printSingleField(
}
generator.print("]");
} else {
if (field.getType() == FieldDescriptor.Type.GROUP) {
if (field.isGroupLike()) {
// Groups must be serialized with their original capitalization.
generator.print(field.getMessageType().getName());
} else {
Expand Down Expand Up @@ -1720,15 +1720,12 @@ private void mergeField(
final String lowerName = name.toLowerCase(Locale.US);
field = type.findFieldByName(lowerName);
// If the case-insensitive match worked but the field is NOT a group,
if (field != null && field.getType() != FieldDescriptor.Type.GROUP) {
if (field != null && !field.isGroupLike()) {
field = null;
}
if (field != null && !field.getMessageType().getName().equals(name)) {
field = null;
}
}
// Again, special-case group names as described above.
if (field != null
&& field.getType() == FieldDescriptor.Type.GROUP
&& !field.getMessageType().getName().equals(name)) {
field = null;
}

if (field == null) {
Expand Down
201 changes: 201 additions & 0 deletions java/core/src/test/java/com/google/protobuf/TextFormatTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@
import com.google.protobuf.testing.proto.TestProto3Optional;
import com.google.protobuf.testing.proto.TestProto3Optional.NestedEnum;
import any_test.AnyTestProto.TestAny;
import editions_unittest.GroupLikeFileScope;
import editions_unittest.MessageImport;
import editions_unittest.NotGroupLikeScope;
import editions_unittest.TestDelimited;
import editions_unittest.UnittestDelimited;
import map_test.MapTestProto.TestMap;
import protobuf_unittest.UnittestMset.TestMessageSetExtension1;
import protobuf_unittest.UnittestMset.TestMessageSetExtension2;
Expand Down Expand Up @@ -1590,6 +1595,202 @@ public void testParseShortRepeatedFormOfNonRepeatedFields() throws Exception {
"1:17: Couldn't parse integer: For input string: \"[\"", "optional_int32: []\n");
}

// =======================================================================
// test delimited

@Test
public void testPrintGroupLikeDelimited() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message)).isEqualTo("GroupLike {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeDelimitedExtension() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setExtension(
UnittestDelimited.groupLikeFileScope,
GroupLikeFileScope.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("[editions_unittest.grouplikefilescope] {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeNotDelimited() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setLengthprefixed(TestDelimited.LengthPrefixed.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("lengthprefixed {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeMismatchedName() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setNotgrouplike(TestDelimited.GroupLike.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("notgrouplike {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeExtensionMismatchedName() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setExtension(
UnittestDelimited.notGroupLikeScope, NotGroupLikeScope.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("[editions_unittest.not_group_like_scope] {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeMismatchedScope() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setNotgrouplikescope(NotGroupLikeScope.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("notgrouplikescope {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeExtensionMismatchedScope() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setExtension(
UnittestDelimited.grouplike, TestDelimited.GroupLike.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("[editions_unittest.grouplike] {\n a: 1\n}\n");
}

@Test
public void testPrintGroupLikeMismatchedFile() throws Exception {
TestDelimited message =
TestDelimited.newBuilder()
.setMessageimport(MessageImport.newBuilder().setA(1).build())
.build();
assertThat(TextFormat.printer().printToString(message))
.isEqualTo("messageimport {\n a: 1\n}\n");
}

@Test
public void testParseDelimitedGroupLikeType() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
TextFormat.merge("GroupLike { a: 1 }", message);
assertThat(message.build())
.isEqualTo(
TestDelimited.newBuilder()
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(1).build())
.build());
}

@Test
public void testParseDelimitedGroupLikeField() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
TextFormat.merge("grouplike { a: 2 }", message);
assertThat(message.build())
.isEqualTo(
TestDelimited.newBuilder()
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(2).build())
.build());
}

@Test
public void testParseDelimitedGroupLikeExtension() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
ExtensionRegistry registry = ExtensionRegistry.newInstance();
registry.add(UnittestDelimited.grouplike);
TextFormat.merge("[editions_unittest.grouplike] { a: 2 }", registry, message);
assertThat(message.build())
.isEqualTo(
TestDelimited.newBuilder()
.setExtension(
UnittestDelimited.grouplike,
TestDelimited.GroupLike.newBuilder().setA(2).build())
.build());
}

@Test
public void testParseDelimitedGroupLikeInvalid() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
try {
TextFormat.merge("GROUPlike { a: 3 }", message);
assertWithMessage("Expected parse exception.").fail();
} catch (TextFormat.ParseException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"1:1: Input contains unknown fields and/or extensions:\n"
+ "1:1:\teditions_unittest.TestDelimited.GROUPlike");
}
}

@Test
public void testParseDelimitedGroupLikeInvalidExtension() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
ExtensionRegistry registry = ExtensionRegistry.newInstance();
registry.add(UnittestDelimited.grouplike);
try {
TextFormat.merge("[editions_unittest.GroupLike] { a: 2 }", registry, message);
assertWithMessage("Expected parse exception.").fail();
} catch (TextFormat.ParseException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"1:20: Input contains unknown fields and/or extensions:\n"
+ "1:20:\teditions_unittest.TestDelimited.[editions_unittest.GroupLike]");
}
}

@Test
public void testParseDelimited() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
TextFormat.merge("notgrouplike { b: 3 }", message);
assertThat(message.build())
.isEqualTo(
TestDelimited.newBuilder()
.setNotgrouplike(TestDelimited.GroupLike.newBuilder().setB(3).build())
.build());
}

@Test
public void testParseDelimitedInvalid() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
try {
TextFormat.merge("NotGroupLike { a: 3 }", message);
assertWithMessage("Expected parse exception.").fail();
} catch (TextFormat.ParseException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"1:1: Input contains unknown fields and/or extensions:\n"
+ "1:1:\teditions_unittest.TestDelimited.NotGroupLike");
}
}

@Test
public void testParseDelimitedInvalidScope() throws Exception {
TestDelimited.Builder message = TestDelimited.newBuilder();
try {
TextFormat.merge("NotGroupLikeScope { a: 3 }", message);
assertWithMessage("Expected parse exception.").fail();
} catch (TextFormat.ParseException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"1:1: Input contains unknown fields and/or extensions:\n"
+ "1:1:\teditions_unittest.TestDelimited.NotGroupLikeScope");
}
}

// =======================================================================
// test oneof

Expand Down
Loading

0 comments on commit 29c69ff

Please sign in to comment.