From 29c69ff00b58b60e67fcf40fd810009bd39b86c6 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Fri, 5 Apr 2024 13:00:59 -0700 Subject: [PATCH] Fix text-format delimited field handling 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 --- conformance/text_format_conformance_suite.cc | 21 +- .../java/com/google/protobuf/Descriptors.java | 29 +++ .../java/com/google/protobuf/TextFormat.java | 13 +- .../com/google/protobuf/TextFormatTest.java | 201 ++++++++++++++++++ .../protobuf/internal/text_format_test.py | 98 +++++++++ python/google/protobuf/text_format.py | 43 +++- src/google/protobuf/text_format.cc | 18 +- src/google/protobuf/text_format_unittest.cc | 48 ++++- upb/reflection/field_def.c | 34 +++ upb/reflection/field_def.h | 1 + upb/text/encode.c | 2 +- 11 files changed, 464 insertions(+), 44 deletions(-) diff --git a/conformance/text_format_conformance_suite.cc b/conformance/text_format_conformance_suite.cc index 2cb2fff732..e14de5c567 100644 --- a/conformance/text_format_conformance_suite.cc +++ b/conformance/text_format_conformance_suite.cc @@ -260,12 +260,11 @@ void TextFormatConformanceTestSuiteImpl::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. @@ -284,11 +283,11 @@ void TextFormatConformanceTestSuiteImpl::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, 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 5c46c168d6..9aaa4799b2 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -1470,6 +1470,35 @@ public final class Descriptors { || 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: diff --git a/java/core/src/main/java/com/google/protobuf/TextFormat.java b/java/core/src/main/java/com/google/protobuf/TextFormat.java index 39838adc5f..eb05fb2f41 100644 --- a/java/core/src/main/java/com/google/protobuf/TextFormat.java +++ b/java/core/src/main/java/com/google/protobuf/TextFormat.java @@ -554,7 +554,7 @@ public final class TextFormat { } 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 { @@ -1720,15 +1720,12 @@ public final class TextFormat { 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) { diff --git a/java/core/src/test/java/com/google/protobuf/TextFormatTest.java b/java/core/src/test/java/com/google/protobuf/TextFormatTest.java index 695e3f58b3..d66a8b0f98 100644 --- a/java/core/src/test/java/com/google/protobuf/TextFormatTest.java +++ b/java/core/src/test/java/com/google/protobuf/TextFormatTest.java @@ -25,6 +25,11 @@ import com.google.protobuf.TextFormat.Parser.SingularOverwritePolicy; 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; @@ -1590,6 +1595,202 @@ public class TextFormatTest { "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 diff --git a/python/google/protobuf/internal/text_format_test.py b/python/google/protobuf/internal/text_format_test.py index d9d9d00f33..a527e5bd67 100644 --- a/python/google/protobuf/internal/text_format_test.py +++ b/python/google/protobuf/internal/text_format_test.py @@ -31,6 +31,8 @@ from google.protobuf import any_test_pb2 from google.protobuf import map_unittest_pb2 from google.protobuf import unittest_mset_pb2 from google.protobuf import unittest_custom_options_pb2 +from google.protobuf import unittest_delimited_pb2 +from google.protobuf import unittest_delimited_import_pb2 from google.protobuf import unittest_pb2 from google.protobuf import unittest_proto3_arena_pb2 # pylint: enable=g-import-not-at-top @@ -2311,6 +2313,102 @@ class TokenizerTest(unittest.TestCase): else: self.assertEqual('RepeatedGroup {\n a: 1\n}\n', str(msg)) + def testPrintGroupLikeDelimited(self): + msg = unittest_delimited_pb2.TestDelimited( + grouplike=unittest_delimited_pb2.TestDelimited.GroupLike(a=1) + ) + if api_implementation.Type() == 'upb': + self.assertEqual(str(msg), 'grouplike {\n a: 1\n}\n') + else: + self.assertEqual(str(msg), 'GroupLike {\n a: 1\n}\n') + + def testPrintGroupLikeDelimitedExtension(self): + msg = unittest_delimited_pb2.TestDelimited() + msg.Extensions[unittest_delimited_pb2.grouplikefilescope].b = 5 + self.assertEqual( + str(msg), '[editions_unittest.grouplikefilescope] {\n b: 5\n}\n' + ) + + def testPrintGroupLikeNotDelimited(self): + msg = unittest_delimited_pb2.TestDelimited( + lengthprefixed=unittest_delimited_pb2.TestDelimited.LengthPrefixed(b=9) + ) + self.assertEqual(str(msg), 'lengthprefixed {\n b: 9\n}\n') + + def testPrintGroupLikeMismatchedName(self): + msg = unittest_delimited_pb2.TestDelimited( + notgrouplike=unittest_delimited_pb2.TestDelimited.GroupLike(b=2) + ) + self.assertEqual(str(msg), 'notgrouplike {\n b: 2\n}\n') + + def testPrintGroupLikeExtensionMismatchedName(self): + msg = unittest_delimited_pb2.TestDelimited() + msg.Extensions[unittest_delimited_pb2.not_group_like_scope].b = 5 + self.assertEqual( + str(msg), '[editions_unittest.not_group_like_scope] {\n b: 5\n}\n' + ) + + def testPrintGroupLikeMismatchedScope(self): + msg = unittest_delimited_pb2.TestDelimited( + notgrouplikescope=unittest_delimited_pb2.NotGroupLikeScope(b=9) + ) + self.assertEqual(str(msg), 'notgrouplikescope {\n b: 9\n}\n') + + def testPrintGroupLikeExtensionMismatchedScope(self): + msg = unittest_delimited_pb2.TestDelimited() + msg.Extensions[unittest_delimited_pb2.grouplike].b = 1 + self.assertEqual(str(msg), '[editions_unittest.grouplike] {\n b: 1\n}\n') + + def testPrintGroupLikeMismatchedFile(self): + msg = unittest_delimited_pb2.TestDelimited( + messageimport=unittest_delimited_import_pb2.MessageImport(b=9) + ) + self.assertEqual(str(msg), 'messageimport {\n b: 9\n}\n') + + def testParseDelimitedGroupLikeType(self): + msg = unittest_delimited_pb2.TestDelimited() + text_format.Parse('GroupLike { a: 1 }', msg) + self.assertEqual(msg.grouplike.a, 1) + self.assertFalse(msg.HasField('notgrouplike')) + + def testParseDelimitedGroupLikeField(self): + msg = unittest_delimited_pb2.TestDelimited() + text_format.Parse('grouplike { a: 2 }', msg) + self.assertEqual(msg.grouplike.a, 2) + self.assertFalse(msg.HasField('notgrouplike')) + + def testParseDelimitedGroupLikeExtension(self): + msg = unittest_delimited_pb2.TestDelimited() + text_format.Parse('[editions_unittest.grouplike] { a: 2 }', msg) + self.assertEqual(msg.Extensions[unittest_delimited_pb2.grouplike].a, 2) + + def testParseDelimitedGroupLikeInvalid(self): + msg = unittest_delimited_pb2.TestDelimited() + with self.assertRaises(text_format.ParseError): + text_format.Parse('GROUPlike { b:1 }', msg) + + def testParseDelimitedGroupLikeInvalidExtension(self): + msg = unittest_delimited_pb2.TestDelimited() + with self.assertRaises(text_format.ParseError): + text_format.Parse('[editions_unittest.GroupLike] { a: 2 }', msg) + + def testParseDelimited(self): + msg = unittest_delimited_pb2.TestDelimited() + text_format.Parse('notgrouplike { b: 1 }', msg) + self.assertEqual(msg.notgrouplike.b, 1) + self.assertFalse(msg.HasField('grouplike')) + + def testParseDelimitedInvalid(self): + msg = unittest_delimited_pb2.TestDelimited() + with self.assertRaises(text_format.ParseError): + text_format.Parse('NotGroupLike { b:1 }', msg) + + def testParseDelimitedInvalidScope(self): + msg = unittest_delimited_pb2.TestDelimited() + with self.assertRaises(text_format.ParseError): + text_format.Parse('NotGroupLikeScope { b:1 }', msg) + + # Tests for pretty printer functionality. @_parameterized.parameters((unittest_pb2), (unittest_proto3_arena_pb2)) class PrettyPrinterTest(TextFormatBase): diff --git a/python/google/protobuf/text_format.py b/python/google/protobuf/text_format.py index ff7caa637a..2244ccca3c 100644 --- a/python/google/protobuf/text_format.py +++ b/python/google/protobuf/text_format.py @@ -185,6 +185,39 @@ def _IsMapEntry(field): field.message_type.GetOptions().map_entry) +def _IsGroupLike(field): + """Determines if a field is consistent with a proto2 group. + + Args: + field: The field descriptor. + + Returns: + True if this field is group-like, false otherwise. + """ + # Groups are always tag-delimited. + if ( + field._GetFeatures().message_encoding + != descriptor._FEATURESET_MESSAGE_ENCODING_DELIMITED + ): + return False + + # Group fields always are always the lowercase type name. + if field.name != field.message_type.name.lower(): + return False + + if field.message_type.file != field.file: + 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 ( + field.message_type.containing_type == field.extension_scope + if field.is_extension + else field.message_type.containing_type == field.containing_type + ) + + def PrintMessage(message, out, indent=0, @@ -531,7 +564,7 @@ class _Printer(object): else: out.write(field.full_name) out.write(']') - elif field.type == descriptor.FieldDescriptor.TYPE_GROUP: + elif _IsGroupLike(field): # For groups, use the capitalized name. out.write(field.message_type.name) else: @@ -933,12 +966,10 @@ class _Parser(object): # names. if not field: field = message_descriptor.fields_by_name.get(name.lower(), None) - if field and field.type != descriptor.FieldDescriptor.TYPE_GROUP: + if field and not _IsGroupLike(field): + field = None + if field and field.message_type.name != name: field = None - - if (field and field.type == descriptor.FieldDescriptor.TYPE_GROUP and - field.message_type.name != name): - field = None if not field and not self.allow_unknown_field: raise tokenizer.ParseErrorPreviousToken( diff --git a/src/google/protobuf/text_format.cc b/src/google/protobuf/text_format.cc index 878ee9406c..3f4d3ba260 100644 --- a/src/google/protobuf/text_format.cc +++ b/src/google/protobuf/text_format.cc @@ -584,23 +584,19 @@ class TextFormat::Parser::ParserImpl { } } else { field = descriptor->FindFieldByName(field_name); - // Group names are expected to be capitalized as they appear in the - // .proto file, which actually matches their type names, not their - // field names. + // Group-like delimited fields will accept both the capitalized type + // names as well. if (field == nullptr) { std::string lower_field_name = field_name; absl::AsciiStrToLower(&lower_field_name); field = descriptor->FindFieldByName(lower_field_name); // If the case-insensitive match worked but the field is NOT a group, - if (field != nullptr && - field->type() != FieldDescriptor::TYPE_GROUP) { + if (field != nullptr && !internal::cpp::IsGroupLike(*field)) { + field = nullptr; + } + if (field != nullptr && field->message_type()->name() != field_name) { field = nullptr; } - } - // Again, special-case group names as described above. - if (field != nullptr && field->type() == FieldDescriptor::TYPE_GROUP && - field->message_type()->name() != field_name) { - field = nullptr; } if (field == nullptr && allow_case_insensitive_field_) { @@ -2062,7 +2058,7 @@ void TextFormat::FastFieldValuePrinter::PrintFieldName( generator->PrintLiteral("["); generator->PrintString(field->PrintableNameForExtension()); generator->PrintLiteral("]"); - } else if (field->type() == FieldDescriptor::TYPE_GROUP) { + } else if (internal::cpp::IsGroupLike(*field)) { // Groups must be serialized with their original capitalization. generator->PrintString(field->message_type()->name()); } else { diff --git a/src/google/protobuf/text_format_unittest.cc b/src/google/protobuf/text_format_unittest.cc index 5f456e9d5c..02b1d38167 100644 --- a/src/google/protobuf/text_format_unittest.cc +++ b/src/google/protobuf/text_format_unittest.cc @@ -45,6 +45,7 @@ #include "google/protobuf/test_util2.h" #include "google/protobuf/unittest.pb.h" #include "google/protobuf/unittest.pb.h" +#include "google/protobuf/unittest_delimited.pb.h" #include "google/protobuf/unittest_mset.pb.h" #include "google/protobuf/unittest_mset_wire_format.pb.h" #include "google/protobuf/unittest_proto3.pb.h" @@ -374,6 +375,19 @@ TEST_F(TextFormatTest, Utf8DebugString) { EXPECT_EQ(correct_string, debug_string); } +TEST_F(TextFormatTest, DelimitedPrintToString) { + editions_unittest::TestDelimited proto; + proto.mutable_grouplike()->set_a(9); + proto.mutable_notgrouplike()->set_b(8); + proto.mutable_nested()->mutable_notgrouplike()->set_a(7); + + std::string output; + TextFormat::PrintToString(proto, &output); + EXPECT_EQ(output, + "nested {\n notgrouplike {\n a: 7\n }\n}\nGroupLike {\n a: " + "9\n}\nnotgrouplike {\n b: 8\n}\n"); +} + TEST_F(TextFormatTest, PrintUnknownFields) { // Test printing of unknown fields in a message. @@ -2018,13 +2032,12 @@ TEST_F(TextFormatParserTest, InvalidFieldName) { 1, 14); } -TEST_F(TextFormatParserTest, InvalidCapitalization) { - // We require that group names be exactly as they appear in the .proto. - ExpectFailure( - "optionalgroup {\na: 15\n}\n", - "Message type \"protobuf_unittest.TestAllTypes\" has no field named " - "\"optionalgroup\".", - 1, 15); +TEST_F(TextFormatParserTest, GroupCapitalization) { + // We allow group names to be the field or message name. + unittest::TestAllTypes proto; + EXPECT_TRUE(parser_.ParseFromString("optionalgroup {\na: 15\n}\n", &proto)); + EXPECT_TRUE(parser_.ParseFromString("OptionalGroup {\na: 15\n}\n", &proto)); + ExpectFailure( "OPTIONALgroup {\na: 15\n}\n", "Message type \"protobuf_unittest.TestAllTypes\" has no field named " @@ -2037,6 +2050,27 @@ TEST_F(TextFormatParserTest, InvalidCapitalization) { 1, 16); } +TEST_F(TextFormatParserTest, DelimitedCapitalization) { + editions_unittest::TestDelimited proto; + EXPECT_TRUE(parser_.ParseFromString("grouplike {\na: 1\n}\n", &proto)); + EXPECT_EQ(proto.grouplike().a(), 1); + EXPECT_TRUE(parser_.ParseFromString("GroupLike {\na: 12\n}\n", &proto)); + EXPECT_EQ(proto.grouplike().a(), 12); + EXPECT_TRUE(parser_.ParseFromString("notgrouplike {\na: 15\n}\n", &proto)); + EXPECT_EQ(proto.notgrouplike().a(), 15); + + ExpectFailure( + "groupLike {\na: 15\n}\n", + "Message type \"editions_unittest.TestDelimited\" has no field named " + "\"groupLike\".", + 1, 11, &proto); + ExpectFailure( + "notGroupLike {\na: 15\n}\n", + "Message type \"editions_unittest.TestDelimited\" has no field named " + "\"notGroupLike\".", + 1, 14, &proto); +} + TEST_F(TextFormatParserTest, AllowIgnoreCapitalizationError) { TextFormat::Parser parser; protobuf_unittest::TestAllTypes proto; diff --git a/upb/reflection/field_def.c b/upb/reflection/field_def.c index eba36dc5ee..98fa37092b 100644 --- a/upb/reflection/field_def.c +++ b/upb/reflection/field_def.c @@ -250,6 +250,40 @@ bool _upb_FieldDef_ValidateUtf8(const upb_FieldDef* f) { UPB_DESC(FeatureSet_VERIFY); } +bool _upb_FieldDef_IsGroupLike(const upb_FieldDef* f) { + // Groups are always tag-delimited. + if (UPB_DESC(FeatureSet_message_encoding)(upb_FieldDef_ResolvedFeatures(f)) != + UPB_DESC(FeatureSet_DELIMITED)) { + return false; + } + + const upb_MessageDef* msg = upb_FieldDef_MessageSubDef(f); + + // Group fields always are always the lowercase type name. + const char* mname = upb_MessageDef_Name(msg); + const char* fname = upb_FieldDef_Name(f); + size_t name_size = strlen(fname); + if (name_size != strlen(mname)) return false; + for (size_t i = 0; i < name_size; ++i) { + if ((mname[i] | 0x20) != fname[i]) { + // Case-insensitive ascii comparison. + return false; + } + } + + if (upb_MessageDef_File(msg) != upb_FieldDef_File(f)) { + 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 upb_FieldDef_IsExtension(f) ? upb_FieldDef_ExtensionScope(f) == + upb_MessageDef_ContainingType(msg) + : upb_FieldDef_ContainingType(f) == + upb_MessageDef_ContainingType(msg); +} + uint64_t _upb_FieldDef_Modifiers(const upb_FieldDef* f) { uint64_t out = upb_FieldDef_IsPacked(f) ? kUpb_FieldModifier_IsPacked : 0; diff --git a/upb/reflection/field_def.h b/upb/reflection/field_def.h index 2923228ca8..a4019a0aa0 100644 --- a/upb/reflection/field_def.h +++ b/upb/reflection/field_def.h @@ -59,6 +59,7 @@ UPB_API upb_Label upb_FieldDef_Label(const upb_FieldDef* f); uint32_t upb_FieldDef_LayoutIndex(const upb_FieldDef* f); UPB_API const upb_MessageDef* upb_FieldDef_MessageSubDef(const upb_FieldDef* f); bool _upb_FieldDef_ValidateUtf8(const upb_FieldDef* f); +bool _upb_FieldDef_IsGroupLike(const upb_FieldDef* f); // Creates a mini descriptor string for a field, returns true on success. bool upb_FieldDef_MiniDescriptorEncode(const upb_FieldDef* f, upb_Arena* a, diff --git a/upb/text/encode.c b/upb/text/encode.c index ae894be5e3..055d005f58 100644 --- a/upb/text/encode.c +++ b/upb/text/encode.c @@ -238,7 +238,7 @@ static void txtenc_field(txtenc* e, upb_MessageValue val, if (ctype == kUpb_CType_Message) { // begin:google_only // // TODO: Turn this into a feature check and opensource it. -// if (upb_FieldDef_Type(f) == kUpb_FieldType_Group) { +// if (_upb_FieldDef_IsGroupLike(f)) { // const upb_MessageDef* m = upb_FieldDef_MessageSubDef(f); // name = upb_MessageDef_Name(m); // }