Preserve the original order of proto fields when generating metadata. (#241)
* Preserve the original order of proto fields.
This corrects metadata edges pointing to the wrong field if the fields were not originally sorted by tag number.
* Fix dartanalyzer lints.
diff --git a/protoc_plugin/CHANGELOG.md b/protoc_plugin/CHANGELOG.md
index cc8adb2..d434e8a 100644
--- a/protoc_plugin/CHANGELOG.md
+++ b/protoc_plugin/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 16.0.6
+
+* Track the original order of proto fields and include it in metadata
+ to ensure the correct fields are being referenced.
+
## 16.0.5
* Fix generation of invalid Dart code for oneof enums
diff --git a/protoc_plugin/lib/message_generator.dart b/protoc_plugin/lib/message_generator.dart
index 41746a3..514b190 100644
--- a/protoc_plugin/lib/message_generator.dart
+++ b/protoc_plugin/lib/message_generator.dart
@@ -439,11 +439,11 @@
_oneofNames
.forEach((OneofNames oneof) => generateoneOfAccessors(out, oneof));
- for (var i = 0; i < _fieldList.length; i++) {
+ for (var field in _fieldList) {
out.println();
List<int> memberFieldPath = List.from(fieldPath)
- ..addAll([_messageFieldTag, i]);
- generateFieldAccessorsMutators(_fieldList[i], out, memberFieldPath);
+ ..addAll([_messageFieldTag, field.sourcePosition]);
+ generateFieldAccessorsMutators(field, out, memberFieldPath);
}
}
@@ -524,7 +524,7 @@
NamedLocation(
name: names.hasMethodName,
fieldPathSegment: memberFieldPath,
- start: 'bool '.length)
+ start: '$_coreImportPrefix.bool '.length)
]);
_emitDeprecatedIf(field.isDeprecated, out);
_emitOverrideIf(field.overridesClearMethod, out);
diff --git a/protoc_plugin/lib/names.dart b/protoc_plugin/lib/names.dart
index cac4e54..bac5055 100644
--- a/protoc_plugin/lib/names.dart
+++ b/protoc_plugin/lib/names.dart
@@ -21,6 +21,10 @@
/// The same index will be stored in FieldInfo.index.
final int index;
+ /// The position of this field as it appeared in the original DescriptorProto.
+ /// Used to construct metadata.
+ final int sourcePosition;
+
/// Identifier for generated getters/setters.
final String fieldName;
@@ -34,7 +38,7 @@
/// `null` for repeated fields.
final String clearMethodName;
- FieldNames(this.descriptor, this.index, this.fieldName,
+ FieldNames(this.descriptor, this.index, this.sourcePosition, this.fieldName,
{this.hasMethodName, this.clearMethodName});
}
@@ -81,7 +85,7 @@
/// Chooses the Dart name of an extension.
String extensionName(FieldDescriptorProto descriptor, Set<String> usedNames) {
- return _unusedMemberNames(descriptor, null, usedNames).fieldName;
+ return _unusedMemberNames(descriptor, null, null, usedNames).fieldName;
}
Iterable<String> extensionSuffixes() sync* {
@@ -218,7 +222,10 @@
MemberNames messageMemberNames(DescriptorProto descriptor,
String parentClassName, Set<String> usedTopLevelNames,
{Iterable<String> reserved = const []}) {
- var sorted = List<FieldDescriptorProto>.from(descriptor.field)
+ var fieldList = List<FieldDescriptorProto>.from(descriptor.field);
+ var sourcePositions =
+ fieldList.asMap().map((index, field) => MapEntry(field.name, index));
+ var sorted = fieldList
..sort((FieldDescriptorProto a, FieldDescriptorProto b) {
if (a.number < b.number) return -1;
if (a.number > b.number) return 1;
@@ -255,8 +262,8 @@
// Explicitly setting a name that's already taken is a build error.
for (var field in sorted) {
if (_nameOption(field).isNotEmpty) {
- takeFieldNames(_memberNamesFromOption(
- descriptor, field, indexes[field.name], existingNames));
+ takeFieldNames(_memberNamesFromOption(descriptor, field,
+ indexes[field.name], sourcePositions[field.name], existingNames));
}
}
@@ -265,7 +272,9 @@
for (var field in sorted) {
if (_nameOption(field).isEmpty) {
var index = indexes[field.name];
- takeFieldNames(_unusedMemberNames(field, index, existingNames));
+ var sourcePosition = sourcePositions[field.name];
+ takeFieldNames(
+ _unusedMemberNames(field, index, sourcePosition, existingNames));
}
}
@@ -313,8 +322,12 @@
///
/// If the explicitly-set Dart name is already taken, throw an exception.
/// (Fails the build.)
-FieldNames _memberNamesFromOption(DescriptorProto message,
- FieldDescriptorProto field, int index, Set<String> existingNames) {
+FieldNames _memberNamesFromOption(
+ DescriptorProto message,
+ FieldDescriptorProto field,
+ int index,
+ int sourcePosition,
+ Set<String> existingNames) {
// TODO(skybrian): provide more context in errors (filename).
var where = "${message.name}.${field.name}";
@@ -336,7 +349,7 @@
checkAvailable(name);
if (_isRepeated(field)) {
- return FieldNames(field, index, name);
+ return FieldNames(field, index, sourcePosition, name);
}
String hasMethod = "has${_capitalize(name)}";
@@ -345,7 +358,7 @@
String clearMethod = "clear${_capitalize(name)}";
checkAvailable(clearMethod);
- return FieldNames(field, index, name,
+ return FieldNames(field, index, sourcePosition, name,
hasMethodName: hasMethod, clearMethodName: clearMethod);
}
@@ -357,12 +370,13 @@
}
}
-FieldNames _unusedMemberNames(
- FieldDescriptorProto field, int index, Set<String> existingNames) {
+FieldNames _unusedMemberNames(FieldDescriptorProto field, int index,
+ int sourcePosition, Set<String> existingNames) {
if (_isRepeated(field)) {
return FieldNames(
field,
index,
+ sourcePosition,
disambiguateName(_defaultFieldName(_fieldMethodSuffix(field)),
existingNames, _memberNamesSuffix(field.number)));
}
@@ -378,7 +392,7 @@
String name = disambiguateName(_fieldMethodSuffix(field), existingNames,
_memberNamesSuffix(field.number),
generateVariants: generateNameVariants);
- return FieldNames(field, index, _defaultFieldName(name),
+ return FieldNames(field, index, sourcePosition, _defaultFieldName(name),
hasMethodName: _defaultHasMethodName(name),
clearMethodName: _defaultClearMethodName(name));
}
diff --git a/protoc_plugin/lib/protobuf_field.dart b/protoc_plugin/lib/protobuf_field.dart
index f058f26..ed04415 100644
--- a/protoc_plugin/lib/protobuf_field.dart
+++ b/protoc_plugin/lib/protobuf_field.dart
@@ -45,6 +45,9 @@
/// `null` for an extension.
int get index => memberNames?.index;
+ /// The position of this field as it appeared in the original DescriptorProto.
+ int get sourcePosition => memberNames.sourcePosition;
+
/// True if the field is to be encoded with [deprecated = true] encoding.
bool get isDeprecated => descriptor.options?.deprecated;
diff --git a/protoc_plugin/pubspec.yaml b/protoc_plugin/pubspec.yaml
index 35528ff..14f1bd1 100644
--- a/protoc_plugin/pubspec.yaml
+++ b/protoc_plugin/pubspec.yaml
@@ -1,5 +1,5 @@
name: protoc_plugin
-version: 16.0.5-dev-1
+version: 16.0.6
author: Dart Team <misc@dartlang.org>
description: Protoc compiler plugin to generate Dart code
homepage: https://github.com/dart-lang/protobuf
diff --git a/protoc_plugin/test/goldens/messageGenerator.meta b/protoc_plugin/test/goldens/messageGenerator.meta
index e91d72a..e573df9 100644
--- a/protoc_plugin/test/goldens/messageGenerator.meta
+++ b/protoc_plugin/test/goldens/messageGenerator.meta
@@ -16,7 +16,7 @@
path: 4
path: 0
path: 2
- path: 0
+ path: 1
sourceFile:
begin: 1205
end: 1211
@@ -25,7 +25,7 @@
path: 4
path: 0
path: 2
- path: 0
+ path: 1
sourceFile:
begin: 1236
end: 1242
@@ -34,16 +34,16 @@
path: 4
path: 0
path: 2
- path: 0
+ path: 1
sourceFile:
- begin: 1289
- end: 1298
+ begin: 1295
+ end: 1304
}
annotation: {
path: 4
path: 0
path: 2
- path: 0
+ path: 1
sourceFile:
begin: 1327
end: 1338
@@ -52,7 +52,7 @@
path: 4
path: 0
path: 2
- path: 1
+ path: 0
sourceFile:
begin: 1388
end: 1392
@@ -61,7 +61,7 @@
path: 4
path: 0
path: 2
- path: 1
+ path: 0
sourceFile:
begin: 1413
end: 1417
@@ -70,16 +70,16 @@
path: 4
path: 0
path: 2
- path: 1
+ path: 0
sourceFile:
- begin: 1470
- end: 1477
+ begin: 1476
+ end: 1483
}
annotation: {
path: 4
path: 0
path: 2
- path: 1
+ path: 0
sourceFile:
begin: 1506
end: 1515
@@ -108,8 +108,8 @@
path: 2
path: 2
sourceFile:
- begin: 1638
- end: 1645
+ begin: 1644
+ end: 1651
}
annotation: {
path: 4
@@ -144,8 +144,8 @@
path: 2
path: 3
sourceFile:
- begin: 1973
- end: 1991
+ begin: 1979
+ end: 1997
}
annotation: {
path: 4
diff --git a/protoc_plugin/test/goldens/oneMessage.pb.meta b/protoc_plugin/test/goldens/oneMessage.pb.meta
index 74ed759..fb67861 100644
--- a/protoc_plugin/test/goldens/oneMessage.pb.meta
+++ b/protoc_plugin/test/goldens/oneMessage.pb.meta
@@ -36,8 +36,8 @@
path: 2
path: 0
sourceFile: test
- begin: 1474
- end: 1483
+ begin: 1480
+ end: 1489
}
annotation: {
path: 4
@@ -72,8 +72,8 @@
path: 2
path: 1
sourceFile: test
- begin: 1641
- end: 1648
+ begin: 1647
+ end: 1654
}
annotation: {
path: 4
@@ -108,8 +108,8 @@
path: 2
path: 2
sourceFile: test
- begin: 1809
- end: 1816
+ begin: 1815
+ end: 1822
}
annotation: {
path: 4
diff --git a/protoc_plugin/test/message_generator_test.dart b/protoc_plugin/test/message_generator_test.dart
index 30fcb10..bc67df3 100755
--- a/protoc_plugin/test/message_generator_test.dart
+++ b/protoc_plugin/test/message_generator_test.dart
@@ -5,6 +5,8 @@
library message_generator_test;
+import 'dart:collection';
+import 'package:collection/collection.dart';
import 'package:protoc_plugin/indenting_writer.dart';
import 'package:protoc_plugin/protoc.dart';
import 'package:test/test.dart';
@@ -15,9 +17,12 @@
import 'golden_file.dart';
void main() {
- test('testMessageGenerator', () {
- FileDescriptorProto fd = FileDescriptorProto();
- EnumDescriptorProto ed = EnumDescriptorProto()
+ FileDescriptorProto fd;
+ EnumDescriptorProto ed;
+ DescriptorProto md;
+ setUp(() async {
+ fd = FileDescriptorProto();
+ ed = EnumDescriptorProto()
..name = 'PhoneType'
..value.addAll([
EnumValueDescriptorProto()
@@ -33,7 +38,7 @@
..name = 'BUSINESS'
..number = 2
]);
- DescriptorProto md = DescriptorProto()
+ md = DescriptorProto()
..name = 'PhoneNumber'
..field.addAll([
// optional PhoneType type = 2 [default = HOME];
@@ -63,6 +68,8 @@
..options = (FieldOptions()..deprecated = true),
])
..enumType.add(ed);
+ });
+ test('testMessageGenerator', () {
var options =
parseGenerationOptions(CodeGeneratorRequest(), CodeGeneratorResponse());
@@ -87,4 +94,45 @@
expectMatchesGoldenFile(writer.sourceLocationInfo.toString(),
'test/goldens/messageGeneratorEnums.meta');
});
+
+ test('testMetadataIndices', () {
+ var options =
+ parseGenerationOptions(CodeGeneratorRequest(), CodeGeneratorResponse());
+ FileGenerator fg = FileGenerator(fd, options);
+ MessageGenerator mg =
+ MessageGenerator.topLevel(md, fg, {}, null, Set<String>(), 0);
+
+ var ctx = GenerationContext(options);
+ mg.register(ctx);
+ mg.resolve(ctx);
+
+ var writer = IndentingWriter(filename: '');
+ mg.generate(writer);
+
+ var eq = ListEquality();
+ var fieldStringsMap = HashMap(
+ equals: eq.equals, hashCode: eq.hash, isValidKey: eq.isValidKey);
+ fieldStringsMap[[4, 0]] = ['PhoneNumber'];
+ fieldStringsMap[[4, 0, 2, 0]] = ['type', 'hasType', 'clearType'];
+ fieldStringsMap[[4, 0, 2, 1]] = ['number', 'hasNumber', 'clearNumber'];
+ fieldStringsMap[[4, 0, 2, 2]] = ['name', 'hasName', 'clearName'];
+ fieldStringsMap[[4, 0, 2, 3]] = [
+ 'deprecatedField',
+ 'hasDeprecatedField',
+ 'clearDeprecatedField'
+ ];
+
+ String generatedContents = writer.toString();
+ GeneratedCodeInfo metadata = writer.sourceLocationInfo;
+ for (var annotation in metadata.annotation) {
+ String annotatedName =
+ generatedContents.substring(annotation.begin, annotation.end);
+ var expectedStrings = fieldStringsMap[annotation.path];
+ if (expectedStrings == null) {
+ fail('The field path ${annotation.path} '
+ 'did not match any expected field path.');
+ }
+ expect(annotatedName, isIn(expectedStrings));
+ }
+ });
}
