Skip to content

Commit 29c69ff

Browse files
mkruskal-googlecopybara-github
mkruskal-google
authored and
copybara-github
committed
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
1 parent 7dc243c commit 29c69ff

File tree

11 files changed

+464
-44
lines changed

11 files changed

+464
-44
lines changed

‎conformance/text_format_conformance_suite.cc‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -260,12 +260,11 @@ void TextFormatConformanceTestSuiteImpl<MessageType>::RunDelimitedTests() {
260260
"DelimitedFieldExtension", REQUIRED,
261261
"[protobuf_test_messages.editions.delimited_ext] { c: 1 }");
262262

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

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

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

293292
// Test extensions of group type
294293
RunValidTextFormatTest("GroupFieldExtension", REQUIRED,

‎java/core/src/main/java/com/google/protobuf/Descriptors.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,6 +1470,35 @@ public boolean hasPresence() {
14701470
|| this.features.getFieldPresence() != DescriptorProtos.FeatureSet.FieldPresence.IMPLICIT;
14711471
}
14721472

1473+
/**
1474+
* Returns true if this field is structured like the synthetic field of a proto2 group. This
1475+
* allows us to expand our treatment of delimited fields without breaking proto2 files that have
1476+
* been upgraded to editions.
1477+
*/
1478+
boolean isGroupLike() {
1479+
if (features.getMessageEncoding() != DescriptorProtos.FeatureSet.MessageEncoding.DELIMITED) {
1480+
// Groups are always tag-delimited.
1481+
return false;
1482+
}
1483+
1484+
if (!getMessageType().getName().toLowerCase().equals(getName())) {
1485+
// Group fields always are always the lowercase type name.
1486+
return false;
1487+
}
1488+
1489+
if (getMessageType().getFile() != getFile()) {
1490+
// Groups could only be defined in the same file they're used.
1491+
return false;
1492+
}
1493+
1494+
// Group messages are always defined in the same scope as the field. File level extensions
1495+
// will compare NULL == NULL here, which is why the file comparison above is necessary to
1496+
// ensure both come from the same file.
1497+
return isExtension()
1498+
? getMessageType().getContainingType() == getExtensionScope()
1499+
: getMessageType().getContainingType() == getContainingType();
1500+
}
1501+
14731502
/**
14741503
* For extensions defined nested within message types, gets the outer type. Not valid for
14751504
* non-extension fields. For example, consider this {@code .proto} file:

‎java/core/src/main/java/com/google/protobuf/TextFormat.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,7 @@ private void printSingleField(
554554
}
555555
generator.print("]");
556556
} else {
557-
if (field.getType() == FieldDescriptor.Type.GROUP) {
557+
if (field.isGroupLike()) {
558558
// Groups must be serialized with their original capitalization.
559559
generator.print(field.getMessageType().getName());
560560
} else {
@@ -1720,15 +1720,12 @@ private void mergeField(
17201720
final String lowerName = name.toLowerCase(Locale.US);
17211721
field = type.findFieldByName(lowerName);
17221722
// If the case-insensitive match worked but the field is NOT a group,
1723-
if (field != null && field.getType() != FieldDescriptor.Type.GROUP) {
1723+
if (field != null && !field.isGroupLike()) {
1724+
field = null;
1725+
}
1726+
if (field != null && !field.getMessageType().getName().equals(name)) {
17241727
field = null;
17251728
}
1726-
}
1727-
// Again, special-case group names as described above.
1728-
if (field != null
1729-
&& field.getType() == FieldDescriptor.Type.GROUP
1730-
&& !field.getMessageType().getName().equals(name)) {
1731-
field = null;
17321729
}
17331730

17341731
if (field == null) {

‎java/core/src/test/java/com/google/protobuf/TextFormatTest.java‎

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@
2525
import com.google.protobuf.testing.proto.TestProto3Optional;
2626
import com.google.protobuf.testing.proto.TestProto3Optional.NestedEnum;
2727
import any_test.AnyTestProto.TestAny;
28+
import editions_unittest.GroupLikeFileScope;
29+
import editions_unittest.MessageImport;
30+
import editions_unittest.NotGroupLikeScope;
31+
import editions_unittest.TestDelimited;
32+
import editions_unittest.UnittestDelimited;
2833
import map_test.MapTestProto.TestMap;
2934
import protobuf_unittest.UnittestMset.TestMessageSetExtension1;
3035
import protobuf_unittest.UnittestMset.TestMessageSetExtension2;
@@ -1590,6 +1595,202 @@ public void testParseShortRepeatedFormOfNonRepeatedFields() throws Exception {
15901595
"1:17: Couldn't parse integer: For input string: \"[\"", "optional_int32: []\n");
15911596
}
15921597

1598+
// =======================================================================
1599+
// test delimited
1600+
1601+
@Test
1602+
public void testPrintGroupLikeDelimited() throws Exception {
1603+
TestDelimited message =
1604+
TestDelimited.newBuilder()
1605+
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(1).build())
1606+
.build();
1607+
assertThat(TextFormat.printer().printToString(message)).isEqualTo("GroupLike {\n a: 1\n}\n");
1608+
}
1609+
1610+
@Test
1611+
public void testPrintGroupLikeDelimitedExtension() throws Exception {
1612+
TestDelimited message =
1613+
TestDelimited.newBuilder()
1614+
.setExtension(
1615+
UnittestDelimited.groupLikeFileScope,
1616+
GroupLikeFileScope.newBuilder().setA(1).build())
1617+
.build();
1618+
assertThat(TextFormat.printer().printToString(message))
1619+
.isEqualTo("[editions_unittest.grouplikefilescope] {\n a: 1\n}\n");
1620+
}
1621+
1622+
@Test
1623+
public void testPrintGroupLikeNotDelimited() throws Exception {
1624+
TestDelimited message =
1625+
TestDelimited.newBuilder()
1626+
.setLengthprefixed(TestDelimited.LengthPrefixed.newBuilder().setA(1).build())
1627+
.build();
1628+
assertThat(TextFormat.printer().printToString(message))
1629+
.isEqualTo("lengthprefixed {\n a: 1\n}\n");
1630+
}
1631+
1632+
@Test
1633+
public void testPrintGroupLikeMismatchedName() throws Exception {
1634+
TestDelimited message =
1635+
TestDelimited.newBuilder()
1636+
.setNotgrouplike(TestDelimited.GroupLike.newBuilder().setA(1).build())
1637+
.build();
1638+
assertThat(TextFormat.printer().printToString(message))
1639+
.isEqualTo("notgrouplike {\n a: 1\n}\n");
1640+
}
1641+
1642+
@Test
1643+
public void testPrintGroupLikeExtensionMismatchedName() throws Exception {
1644+
TestDelimited message =
1645+
TestDelimited.newBuilder()
1646+
.setExtension(
1647+
UnittestDelimited.notGroupLikeScope, NotGroupLikeScope.newBuilder().setA(1).build())
1648+
.build();
1649+
assertThat(TextFormat.printer().printToString(message))
1650+
.isEqualTo("[editions_unittest.not_group_like_scope] {\n a: 1\n}\n");
1651+
}
1652+
1653+
@Test
1654+
public void testPrintGroupLikeMismatchedScope() throws Exception {
1655+
TestDelimited message =
1656+
TestDelimited.newBuilder()
1657+
.setNotgrouplikescope(NotGroupLikeScope.newBuilder().setA(1).build())
1658+
.build();
1659+
assertThat(TextFormat.printer().printToString(message))
1660+
.isEqualTo("notgrouplikescope {\n a: 1\n}\n");
1661+
}
1662+
1663+
@Test
1664+
public void testPrintGroupLikeExtensionMismatchedScope() throws Exception {
1665+
TestDelimited message =
1666+
TestDelimited.newBuilder()
1667+
.setExtension(
1668+
UnittestDelimited.grouplike, TestDelimited.GroupLike.newBuilder().setA(1).build())
1669+
.build();
1670+
assertThat(TextFormat.printer().printToString(message))
1671+
.isEqualTo("[editions_unittest.grouplike] {\n a: 1\n}\n");
1672+
}
1673+
1674+
@Test
1675+
public void testPrintGroupLikeMismatchedFile() throws Exception {
1676+
TestDelimited message =
1677+
TestDelimited.newBuilder()
1678+
.setMessageimport(MessageImport.newBuilder().setA(1).build())
1679+
.build();
1680+
assertThat(TextFormat.printer().printToString(message))
1681+
.isEqualTo("messageimport {\n a: 1\n}\n");
1682+
}
1683+
1684+
@Test
1685+
public void testParseDelimitedGroupLikeType() throws Exception {
1686+
TestDelimited.Builder message = TestDelimited.newBuilder();
1687+
TextFormat.merge("GroupLike { a: 1 }", message);
1688+
assertThat(message.build())
1689+
.isEqualTo(
1690+
TestDelimited.newBuilder()
1691+
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(1).build())
1692+
.build());
1693+
}
1694+
1695+
@Test
1696+
public void testParseDelimitedGroupLikeField() throws Exception {
1697+
TestDelimited.Builder message = TestDelimited.newBuilder();
1698+
TextFormat.merge("grouplike { a: 2 }", message);
1699+
assertThat(message.build())
1700+
.isEqualTo(
1701+
TestDelimited.newBuilder()
1702+
.setGroupLike(TestDelimited.GroupLike.newBuilder().setA(2).build())
1703+
.build());
1704+
}
1705+
1706+
@Test
1707+
public void testParseDelimitedGroupLikeExtension() throws Exception {
1708+
TestDelimited.Builder message = TestDelimited.newBuilder();
1709+
ExtensionRegistry registry = ExtensionRegistry.newInstance();
1710+
registry.add(UnittestDelimited.grouplike);
1711+
TextFormat.merge("[editions_unittest.grouplike] { a: 2 }", registry, message);
1712+
assertThat(message.build())
1713+
.isEqualTo(
1714+
TestDelimited.newBuilder()
1715+
.setExtension(
1716+
UnittestDelimited.grouplike,
1717+
TestDelimited.GroupLike.newBuilder().setA(2).build())
1718+
.build());
1719+
}
1720+
1721+
@Test
1722+
public void testParseDelimitedGroupLikeInvalid() throws Exception {
1723+
TestDelimited.Builder message = TestDelimited.newBuilder();
1724+
try {
1725+
TextFormat.merge("GROUPlike { a: 3 }", message);
1726+
assertWithMessage("Expected parse exception.").fail();
1727+
} catch (TextFormat.ParseException e) {
1728+
assertThat(e)
1729+
.hasMessageThat()
1730+
.isEqualTo(
1731+
"1:1: Input contains unknown fields and/or extensions:\n"
1732+
+ "1:1:\teditions_unittest.TestDelimited.GROUPlike");
1733+
}
1734+
}
1735+
1736+
@Test
1737+
public void testParseDelimitedGroupLikeInvalidExtension() throws Exception {
1738+
TestDelimited.Builder message = TestDelimited.newBuilder();
1739+
ExtensionRegistry registry = ExtensionRegistry.newInstance();
1740+
registry.add(UnittestDelimited.grouplike);
1741+
try {
1742+
TextFormat.merge("[editions_unittest.GroupLike] { a: 2 }", registry, message);
1743+
assertWithMessage("Expected parse exception.").fail();
1744+
} catch (TextFormat.ParseException e) {
1745+
assertThat(e)
1746+
.hasMessageThat()
1747+
.isEqualTo(
1748+
"1:20: Input contains unknown fields and/or extensions:\n"
1749+
+ "1:20:\teditions_unittest.TestDelimited.[editions_unittest.GroupLike]");
1750+
}
1751+
}
1752+
1753+
@Test
1754+
public void testParseDelimited() throws Exception {
1755+
TestDelimited.Builder message = TestDelimited.newBuilder();
1756+
TextFormat.merge("notgrouplike { b: 3 }", message);
1757+
assertThat(message.build())
1758+
.isEqualTo(
1759+
TestDelimited.newBuilder()
1760+
.setNotgrouplike(TestDelimited.GroupLike.newBuilder().setB(3).build())
1761+
.build());
1762+
}
1763+
1764+
@Test
1765+
public void testParseDelimitedInvalid() throws Exception {
1766+
TestDelimited.Builder message = TestDelimited.newBuilder();
1767+
try {
1768+
TextFormat.merge("NotGroupLike { a: 3 }", message);
1769+
assertWithMessage("Expected parse exception.").fail();
1770+
} catch (TextFormat.ParseException e) {
1771+
assertThat(e)
1772+
.hasMessageThat()
1773+
.isEqualTo(
1774+
"1:1: Input contains unknown fields and/or extensions:\n"
1775+
+ "1:1:\teditions_unittest.TestDelimited.NotGroupLike");
1776+
}
1777+
}
1778+
1779+
@Test
1780+
public void testParseDelimitedInvalidScope() throws Exception {
1781+
TestDelimited.Builder message = TestDelimited.newBuilder();
1782+
try {
1783+
TextFormat.merge("NotGroupLikeScope { a: 3 }", message);
1784+
assertWithMessage("Expected parse exception.").fail();
1785+
} catch (TextFormat.ParseException e) {
1786+
assertThat(e)
1787+
.hasMessageThat()
1788+
.isEqualTo(
1789+
"1:1: Input contains unknown fields and/or extensions:\n"
1790+
+ "1:1:\teditions_unittest.TestDelimited.NotGroupLikeScope");
1791+
}
1792+
}
1793+
15931794
// =======================================================================
15941795
// test oneof
15951796

0 commit comments

Comments
 (0)