Skip to content

Commit a912f76

Browse files
osa1osa1
authored
Add more message set tests, fix a bug (#859)
When parsing an item we should consume the "end group" tag before breaking. Remove the `break` statements after parsing the extension.
1 parent 7bebbc6 commit a912f76

File tree

3 files changed

+218
-65
lines changed

3 files changed

+218
-65
lines changed

‎protobuf/lib/src/protobuf/message_set.dart

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,16 +52,15 @@ abstract class $_MessageSet extends GeneratedMessage {
5252
[ExtensionRegistry extensionRegistry = ExtensionRegistry.EMPTY]) {
5353
// Parse items. The field for the items looks like:
5454
//
55-
// repeated Item items = 1;
55+
// repeated group Item items = 1;
5656
//
5757
// Since message sets are compatible with proto1 items can't be packed.
58-
outer:
5958
while (true) {
6059
final tag = input.readTag();
6160
final tagNumber = getTagFieldNumber(tag);
6261

6362
if (tag == 0) {
64-
break;
63+
break; // End of input.
6564
}
6665

6766
if (tagNumber != _messageSetItemsTag) {
@@ -87,7 +86,7 @@ abstract class $_MessageSet extends GeneratedMessage {
8786
final tagNumber = getTagFieldNumber(tag);
8887

8988
if (tag == 0) {
90-
break;
89+
break; // End of input.
9190
}
9291

9392
if (tagNumber == _messageSetItemTypeIdTag) {
@@ -96,20 +95,19 @@ abstract class $_MessageSet extends GeneratedMessage {
9695
_parseExtension(typeId, message, extensionRegistry);
9796
typeId = null;
9897
message = null;
99-
continue outer;
10098
}
10199
} else if (tagNumber == _messageSetItemMessageTag) {
102100
message = input.readBytes();
103101
if (typeId != null) {
104102
_parseExtension(typeId, message, extensionRegistry);
105103
typeId = null;
106104
message = null;
107-
continue outer;
108105
}
109106
} else {
110-
// Skip unknown tags.
107+
// Skip unknown tags. If we're at the end of the group consume the
108+
// EGROUP tag.
111109
if (!input.skipField(tag)) {
112-
break outer; // End of group.
110+
break; // End of group.
113111
}
114112
}
115113
}

‎protoc_plugin/test/message_set_test.dart

Lines changed: 202 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5+
import 'dart:typed_data';
6+
57
import 'package:fixnum/fixnum.dart';
68
import 'package:protobuf/protobuf.dart';
79
import 'package:test/test.dart';
@@ -10,78 +12,173 @@ import '../out/protos/google/protobuf/empty.pb.dart';
1012
import '../out/protos/message_set.pb.dart';
1113

1214
void main() {
13-
final encoded = [
14-
0xda, 0x07, 0x0e, 0x0b, 0x10, 0xc8, 0xa6, 0x6b, 0x1a, //
15-
0x06, 0x08, 0x7b, 0x12, 0x02, 0x68, 0x69, 0x0c
15+
// Example message with a nested message set, encoded with the C++ library.
16+
// `protoc --decode_raw` output:
17+
//
18+
// 1 {
19+
// 1 {
20+
// 2: 1758024
21+
// 3 {
22+
// 1: 123
23+
// 2: "testing"
24+
// 3 {
25+
// 5: 0
26+
// 5: 1
27+
// 5: 2
28+
// }
29+
// }
30+
// }
31+
// 1 {
32+
// 2: 1832098
33+
// 3 {
34+
// 5: 987
35+
// 5: 654
36+
// 5: 321
37+
// }
38+
// }
39+
// }
40+
final encodedNested = [
41+
10, 44, 11, 16, 200, 166, 107, 26, 19, 8, 123, 18, 7, 116, 101, 115, //
42+
116, 105, 110, 103, 26, 6, 40, 0, 40, 1, 40, 2, 12, 11, 16, 162, 233, //
43+
111, 26, 9, 40, 219, 7, 40, 142, 5, 40, 193, 2, 12
1644
];
1745

18-
test('Simple message set field', () {
19-
final registry = ExtensionRegistry()..add(TestMessage.messageSetExtension);
20-
final msg = TestMessage.fromBuffer(encoded, registry);
21-
final extensionValue = msg.info
22-
.getExtension(TestMessage.messageSetExtension) as ExtensionMessage;
23-
expect(extensionValue.a, 123);
24-
expect(extensionValue.b, 'hi');
25-
expect(msg.writeToBuffer(), encoded);
46+
// Example message with a top-level message set, encoded with the C++
47+
// library. `protoc --decode_raw` output:
48+
//
49+
// 1 {
50+
// 2: 1758024
51+
// 3 {
52+
// 1: 123
53+
// }
54+
// }
55+
// 1 {
56+
// 2: 1832098
57+
// 3 {
58+
// 5: 987
59+
// }
60+
// }
61+
final encodedTopLevel = [
62+
11, 16, 200, 166, 107, 26, 2, 8, 123, 12, 11, 16, 162, 233, 111, 26, 3, //
63+
40, 219, 7, 12
64+
];
65+
66+
test('Parse message set extensions (nested)', () {
67+
final registry = ExtensionRegistry()
68+
..add(TestMessage.ext1)
69+
..add(TestMessage.ext2);
70+
final msg = TestMessage.fromBuffer(encodedNested, registry);
71+
72+
final ext1Value =
73+
msg.info.getExtension(TestMessage.ext1) as ExtensionMessage1;
74+
expect(ext1Value.a, 123);
75+
expect(ext1Value.b, 'testing');
76+
expect(ext1Value.c.ints, [0, 1, 2]);
77+
78+
final ext2Value =
79+
msg.info.getExtension(TestMessage.ext2) as ExtensionMessage2;
80+
expect(ext2Value.ints, [987, 654, 321]);
81+
82+
expect(msg.writeToBuffer(), encodedNested);
83+
});
84+
85+
test('Parse message set (top-level)', () {
86+
final registry = ExtensionRegistry()
87+
..add(TestMessage.ext1)
88+
..add(TestMessage.ext2);
89+
final msg = MessageSet.fromBuffer(encodedTopLevel, registry);
90+
91+
final ext1Value = msg.getExtension(TestMessage.ext1) as ExtensionMessage1;
92+
expect(ext1Value.a, 123);
93+
expect(ext1Value.b, ''); // not set
94+
expect(ext1Value.c.ints, []); // not set
95+
96+
final ext2Value = msg.getExtension(TestMessage.ext2) as ExtensionMessage2;
97+
expect(ext2Value.ints, [987]);
98+
99+
expect(msg.writeToBuffer(), encodedTopLevel);
26100
});
27101

28102
test('Parse as unknown fields and serialize', () {
29-
final msg = TestMessage.fromBuffer(encoded);
30-
expect(msg.writeToBuffer(), encoded);
103+
final msg = TestMessage.fromBuffer(encodedNested);
104+
expect(msg.writeToBuffer(), encodedNested);
31105
});
32106

33107
test('Reparse with extensions (nested message)', () {
34-
final msg = TestMessage.fromBuffer(encoded);
35-
final registry = ExtensionRegistry()..add(TestMessage.messageSetExtension);
108+
final msg = TestMessage.fromBuffer(encodedNested);
109+
final registry = ExtensionRegistry()
110+
..add(TestMessage.ext1)
111+
..add(TestMessage.ext2);
36112
final reparsedInfo = registry.reparseMessage(msg.info);
37-
final extensionValue = reparsedInfo
38-
.getExtension(TestMessage.messageSetExtension) as ExtensionMessage;
39-
expect(extensionValue.a, 123);
40-
expect(extensionValue.b, 'hi');
113+
114+
final ext1Value =
115+
reparsedInfo.getExtension(TestMessage.ext1) as ExtensionMessage1;
116+
expect(ext1Value.a, 123);
117+
expect(ext1Value.b, 'testing');
118+
119+
final ext2Value =
120+
reparsedInfo.getExtension(TestMessage.ext2) as ExtensionMessage2;
121+
expect(ext2Value.ints, [987, 654, 321]);
122+
41123
expect(reparsedInfo.unknownFields.isEmpty, true);
42124
});
43125

44126
test('Reparse with extensions (top-level message)', () {
45-
final msg = TestMessage.fromBuffer(encoded);
46-
final registry = ExtensionRegistry()..add(TestMessage.messageSetExtension);
127+
final msg = TestMessage.fromBuffer(encodedNested);
128+
final registry = ExtensionRegistry()
129+
..add(TestMessage.ext1)
130+
..add(TestMessage.ext2);
47131
final reparsedMsg = registry.reparseMessage(msg);
48-
final extensionValue = reparsedMsg.info
49-
.getExtension(TestMessage.messageSetExtension) as ExtensionMessage;
50-
expect(extensionValue.a, 123);
51-
expect(extensionValue.b, 'hi');
132+
133+
final ext1Value =
134+
reparsedMsg.info.getExtension(TestMessage.ext1) as ExtensionMessage1;
135+
expect(ext1Value.a, 123);
136+
expect(ext1Value.b, 'testing');
137+
138+
final ext2Value =
139+
reparsedMsg.info.getExtension(TestMessage.ext2) as ExtensionMessage2;
140+
expect(ext2Value.ints, [987, 654, 321]);
141+
52142
expect(msg.unknownFields.isEmpty, true);
53143
});
54144

55145
test('Ignore extra tags in items', () {
56146
// Generate a message set encoding using unknown fields. Message set item
57147
// will have extra tags.
58148

59-
final itemFieldGroup = UnknownFieldSet();
149+
final encoded = encodeMessageSetWithExtraItemTags(Encoding.group);
150+
151+
final registry = ExtensionRegistry()..add(TestMessage.ext1);
152+
final decodedMsg = MessageSet.fromBuffer(encoded, registry);
153+
final extensionValue =
154+
decodedMsg.getExtension(TestMessage.ext1) as ExtensionMessage1;
155+
156+
expect(extensionValue.a, 123456);
157+
expect(extensionValue.b, 'test');
158+
expect(decodedMsg.unknownFields.isEmpty, true);
159+
});
160+
161+
test('Ignore extra tags in items (length delimited encoding)', () {
162+
// Same as above, but tests length delimited encoding.
60163

61-
// Invalid field with tag 1.
62-
itemFieldGroup.addField(
63-
1, UnknownFieldSetField()..addLengthDelimited([1, 2, 3]));
164+
final encoded = encodeMessageSetWithExtraItemTags(Encoding.lengthDelimited);
64165

65-
// Extension field.
66-
itemFieldGroup.addField(
67-
3,
68-
UnknownFieldSetField()
69-
..addLengthDelimited((ExtensionMessage()
70-
..a = 123456
71-
..b = 'test')
72-
.writeToBuffer()));
166+
final registry = ExtensionRegistry()..add(TestMessage.ext1);
167+
final decodedMsg = MessageSet.fromBuffer(encoded, registry);
168+
final extensionValue =
169+
decodedMsg.getExtension(TestMessage.ext1) as ExtensionMessage1;
73170

74-
// Invalid field with tag 3.
75-
itemFieldGroup.addField(
76-
4, UnknownFieldSetField()..addVarint(Int64(123456)));
171+
expect(extensionValue.a, 123456);
172+
expect(extensionValue.b, 'test');
173+
expect(decodedMsg.unknownFields.isEmpty, true);
174+
});
77175

78-
// Type id field.
79-
itemFieldGroup.addField(
80-
2, UnknownFieldSetField()..addVarint(Int64(1758024)));
176+
test('Ignore invalid tags in repeated items', () {
177+
// Extra fields other than `repeated Item items = 1` in the outer message.
81178

82179
final messageSetUnknownFields = UnknownFieldSet();
83180
messageSetUnknownFields.addField(
84-
1, UnknownFieldSetField()..addGroup(itemFieldGroup));
181+
2, UnknownFieldSetField()..addVarint(Int64(987)));
85182

86183
final messageSetEncoded = CodedBufferWriter();
87184
messageSetUnknownFields.writeToCodedBufferWriter(messageSetEncoded);
@@ -93,17 +190,14 @@ void main() {
93190
..lengthDelimited.add(messageSetEncoded.toBuffer())))
94191
.writeToBuffer();
95192

96-
final registry = ExtensionRegistry()..add(TestMessage.messageSetExtension);
193+
final registry = ExtensionRegistry()..add(TestMessage.ext1);
97194
final msg = TestMessage.fromBuffer(encoded, registry);
98-
final extensionValue = msg.info
99-
.getExtension(TestMessage.messageSetExtension) as ExtensionMessage;
100-
expect(extensionValue.a, 123456);
101-
expect(extensionValue.b, 'test');
102-
expect(msg.unknownFields.isEmpty, true);
195+
expect(msg.info.hasExtension(TestMessage.ext1), false);
103196
});
104197

105-
test('Ignore invalid tags in repeated items', () {
106-
// Extra fields other than `repeated Item items = 1` in the outer message.
198+
test('Encode message set as length prefixed', () {
199+
// Message sets are generally encoded as groups, but we support length
200+
// delimited as well.
107201

108202
final messageSetUnknownFields = UnknownFieldSet();
109203
messageSetUnknownFields.addField(
@@ -119,8 +213,62 @@ void main() {
119213
..lengthDelimited.add(messageSetEncoded.toBuffer())))
120214
.writeToBuffer();
121215

122-
final registry = ExtensionRegistry()..add(TestMessage.messageSetExtension);
216+
final registry = ExtensionRegistry()..add(TestMessage.ext1);
123217
final msg = TestMessage.fromBuffer(encoded, registry);
124-
expect(msg.info.hasExtension(TestMessage.messageSetExtension), false);
218+
expect(msg.info.hasExtension(TestMessage.ext1), false);
125219
});
126220
}
221+
222+
enum Encoding {
223+
lengthDelimited,
224+
group,
225+
}
226+
227+
/// Generate a message set encoding with one extension. Extension will have
228+
/// extra tags.
229+
Uint8List encodeMessageSetWithExtraItemTags(Encoding encoding) {
230+
final itemFieldGroup = UnknownFieldSet();
231+
232+
// Invalid field with tag 1.
233+
itemFieldGroup.addField(
234+
1, UnknownFieldSetField()..addLengthDelimited([1, 2, 3]));
235+
236+
// Extension field.
237+
itemFieldGroup.addField(
238+
3,
239+
UnknownFieldSetField()
240+
..addLengthDelimited((ExtensionMessage1()
241+
..a = 123456
242+
..b = 'test')
243+
.writeToBuffer()));
244+
245+
// Invalid field with tag 3.
246+
itemFieldGroup.addField(4, UnknownFieldSetField()..addVarint(Int64(123456)));
247+
248+
// Type id field.
249+
itemFieldGroup.addField(2, UnknownFieldSetField()..addVarint(Int64(1758024)));
250+
251+
final messageSet = Empty();
252+
final messageSetUnknownFields = messageSet.unknownFields;
253+
254+
switch (encoding) {
255+
case Encoding.lengthDelimited:
256+
final writer = CodedBufferWriter();
257+
itemFieldGroup.writeToCodedBufferWriter(writer);
258+
messageSetUnknownFields.addField(
259+
1, UnknownFieldSetField()..addLengthDelimited(writer.toBuffer()));
260+
break;
261+
case Encoding.group:
262+
messageSetUnknownFields.addField(
263+
1, UnknownFieldSetField()..addGroup(itemFieldGroup));
264+
break;
265+
}
266+
267+
messageSetUnknownFields.addField(
268+
1, UnknownFieldSetField()..addGroup(itemFieldGroup));
269+
270+
final messageSetEncoded = CodedBufferWriter();
271+
messageSetUnknownFields.writeToCodedBufferWriter(messageSetEncoded);
272+
273+
return messageSet.writeToBuffer();
274+
}

‎protoc_plugin/test/protos/message_set.proto

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,20 @@ message MessageSet {
1313

1414
message TestMessage {
1515
extend MessageSet {
16-
optional ExtensionMessage message_set_extension = 1758024;
16+
optional ExtensionMessage1 ext1 = 1758024;
17+
optional ExtensionMessage2 ext2 = 1832098;
1718
}
1819

19-
optional MessageSet info = 123;
20+
optional MessageSet info = 1;
21+
optional int32 i = 2;
2022
}
2123

22-
message ExtensionMessage {
24+
message ExtensionMessage1 {
2325
optional int32 a = 1;
2426
optional string b = 2;
27+
optional ExtensionMessage2 c = 3;
28+
}
29+
30+
message ExtensionMessage2 {
31+
repeated int32 ints = 5;
2532
}

0 commit comments

Comments
 (0)