From a1cfc0478b05af642326e917de633146ccf15546 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Fri, 15 Nov 2019 13:07:28 +0100 Subject: [PATCH 1/5] Handle unknown fields part of a oneof --- protobuf/CHANGELOG.md | 5 +++++ protobuf/lib/src/protobuf/coded_buffer.dart | 1 + protobuf/lib/src/protobuf/field_set.dart | 18 +++++++++++------- .../lib/src/protobuf/generated_message.dart | 7 +++++-- .../lib/src/protobuf/unknown_field_set.dart | 18 ++++++++++++++++++ protobuf/pubspec.yaml | 2 +- 6 files changed, 41 insertions(+), 10 deletions(-) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index cf6846be6..afe59602e 100644 --- a/protobuf/CHANGELOG.md +++ b/protobuf/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.0.2 + +* Handle cases where a field that is part of a oneof is removed by the + protobuf aware treeshaker. + ## 1.0.1 * Fix issue with the non-json name of a field (`protoName`) not being set correctly. diff --git a/protobuf/lib/src/protobuf/coded_buffer.dart b/protobuf/lib/src/protobuf/coded_buffer.dart index b7cd01603..0788c5ce1 100644 --- a/protobuf/lib/src/protobuf/coded_buffer.dart +++ b/protobuf/lib/src/protobuf/coded_buffer.dart @@ -42,6 +42,7 @@ void _mergeFromCodedBufferReader( } if (fi == null || !_wireTypeMatches(fi.type, wireType)) { + fs._updateOneOfCase(tagNumber); if (!fs._ensureUnknownFields().mergeFieldFromBuffer(tag, input)) { return; } diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 04256f124..217784a1b 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -265,8 +265,9 @@ class _FieldSet { } } - // neither a regular field nor an extension. - // TODO(skybrian) throw? + if (_hasUnknownFields) { + _unknownFields._clearField(tagNumber); + } } /// Sets a non-repeated field with error-checking. @@ -342,14 +343,17 @@ class _FieldSet { return newValue; } - /// Sets a non-extended field and fires events. - void _setNonExtensionFieldUnchecked(FieldInfo fi, value) { - int tag = fi.tagNumber; - int oneofIndex = _meta.oneofs[tag]; + void _updateOneOfCase(int newTagnumber) { + int oneofIndex = _meta.oneofs[newTagnumber]; if (oneofIndex != null) { _clearField(_oneofCases[oneofIndex]); - _oneofCases[oneofIndex] = tag; + _oneofCases[oneofIndex] = newTagnumber; } + } + + /// Sets a non-extended field and fires events. + void _setNonExtensionFieldUnchecked(FieldInfo fi, value) { + _updateOneOfCase(fi.tagNumber); // It is important that the callback to the observers is not moved to the // beginning of this method but happens just before the value is set. diff --git a/protobuf/lib/src/protobuf/generated_message.dart b/protobuf/lib/src/protobuf/generated_message.dart index 09445693c..64dc31dd1 100644 --- a/protobuf/lib/src/protobuf/generated_message.dart +++ b/protobuf/lib/src/protobuf/generated_message.dart @@ -300,8 +300,11 @@ abstract class GeneratedMessage { /// Clears the contents of a given field. /// - /// If it's an extension field, the Extension will be kept. - /// The tagNumber should be a valid tag or extension. + /// If it's an extension field, the value will be removed, but the Extension + /// will be kept. + /// + /// If the tagNumber is present as an unknown field, that field will be + /// removed. void clearField(int tagNumber) => _fieldSet._clearField(tagNumber); int $_whichOneof(int oneofIndex) => _fieldSet._oneofCases[oneofIndex] ?? 0; diff --git a/protobuf/lib/src/protobuf/unknown_field_set.dart b/protobuf/lib/src/protobuf/unknown_field_set.dart index a136f36de..5488491d4 100644 --- a/protobuf/lib/src/protobuf/unknown_field_set.dart +++ b/protobuf/lib/src/protobuf/unknown_field_set.dart @@ -31,8 +31,15 @@ class UnknownFieldSet { UnknownFieldSetField getField(int tagNumber) => _fields[tagNumber]; + /// Returns `true` if [tagNumber] is set in [this]. bool hasField(int tagNumber) => _fields.containsKey(tagNumber); + /// Removes [tagNumber] from [this]. + /// + /// Does nothing if [tagNumber] is unset. + void _clearField(int tagNumber) => _fields.remove(tagNumber); + + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void addField(int number, UnknownFieldSetField field) { _ensureWritable('addField'); _checkFieldNumber(number); @@ -49,6 +56,10 @@ class UnknownFieldSet { ..groups.addAll(field.groups); } + /// Merges the field following [tag] into [this]. + /// + /// Returns `false` if the tag is the end of a group. `true` otherwise. + // TODO(sigurdm): Does not update oneofs. Consider deprecating. bool mergeFieldFromBuffer(int tag, CodedBufferReader input) { _ensureWritable('mergeFieldFromBuffer'); int number = getTagFieldNumber(tag); @@ -76,6 +87,7 @@ class UnknownFieldSet { } } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeFromCodedBufferReader(CodedBufferReader input) { _ensureWritable('mergeFromCodedBufferReader'); while (true) { @@ -86,6 +98,7 @@ class UnknownFieldSet { } } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeFromUnknownFieldSet(UnknownFieldSet other) { _ensureWritable('mergeFromUnknownFieldSet'); for (int key in other._fields.keys) { @@ -99,26 +112,31 @@ class UnknownFieldSet { } } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeFixed32Field(int number, int value) { _ensureWritable('mergeFixed32Field'); _getField(number).addFixed32(value); } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeFixed64Field(int number, Int64 value) { _ensureWritable('mergeFixed64Field'); _getField(number).addFixed64(value); } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeGroupField(int number, UnknownFieldSet value) { _ensureWritable('mergeGroupField'); _getField(number).addGroup(value); } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeLengthDelimitedField(int number, List value) { _ensureWritable('mergeLengthDelimitedField'); _getField(number).addLengthDelimited(value); } + // TODO(sigurdm): Does not update oneofs. Consider deprecating. void mergeVarintField(int number, Int64 value) { _ensureWritable('mergeVarintField'); _getField(number).addVarint(value); diff --git a/protobuf/pubspec.yaml b/protobuf/pubspec.yaml index 0f6616bb9..d18f58691 100644 --- a/protobuf/pubspec.yaml +++ b/protobuf/pubspec.yaml @@ -1,5 +1,5 @@ name: protobuf -version: 1.0.1 +version: 1.0.2 author: Dart Team description: > Runtime library for protocol buffers support. From 2e9934d4ae5ecc5cabc34c5e224204246de46395 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Fri, 15 Nov 2019 14:08:01 +0100 Subject: [PATCH 2/5] Use helper from _ --- protobuf/lib/src/protobuf/field_set.dart | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 217784a1b..ff3f3891d 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -502,13 +502,7 @@ class _FieldSet { if (_hasObservers) { _eventPlugin.beforeSetField(_nonExtensionInfoByIndex(index), value); } - int tag = _meta.byIndex[index].tagNumber; - int oneofIndex = _meta.oneofs[tag]; - - if (oneofIndex != null) { - _clearField(_oneofCases[oneofIndex]); - _oneofCases[oneofIndex] = tag; - } + _updateOneOfCase(_meta.byIndex[index].tagNumber); _values[index] = value; } From d53a9fbc7c15b3c1e738bb9cf6e743f85494d50d Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Thu, 21 Nov 2019 14:55:11 +0100 Subject: [PATCH 3/5] Address review --- protobuf/CHANGELOG.md | 5 +++++ .../lib/src/protobuf/extension_field_set.dart | 16 +++++++++++++++- protobuf/lib/src/protobuf/field_set.dart | 6 +++++- protobuf/lib/src/protobuf/generated_message.dart | 2 ++ protoc_plugin/test/oneof_test.dart | 8 ++++++++ protoc_plugin/test/unknown_field_set_test.dart | 5 +++++ 6 files changed, 40 insertions(+), 2 deletions(-) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index afe59602e..83e4dc61c 100644 --- a/protobuf/CHANGELOG.md +++ b/protobuf/CHANGELOG.md @@ -1,5 +1,10 @@ ## 1.0.2 +* Fixed generic handling of unknown fields. + * `GeneratedMessage.clearField(tagNumber)` will now also clear an unknown + field. + * `GeneratedMessage.hasField(tagNumber)` will now return true if there is an + unknown field. * Handle cases where a field that is part of a oneof is removed by the protobuf aware treeshaker. diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index fe301db83..331945e7a 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -21,7 +21,10 @@ class _ExtensionFieldSet { // I think this was originally here for repeated extensions. _addInfoUnchecked(fi); var value = _getFieldOrNull(fi); - if (value == null) return fi.makeDefault(); + if (value == null) { + _checkNotInUnknown(fi); + return fi.makeDefault(); + } return value; } @@ -50,6 +53,7 @@ class _ExtensionFieldSet { List _getList(Extension fi) { var value = _values[fi.tagNumber]; if (value != null) return value as List; + _checkNotInUnknown(fi); if (_isReadOnly) return List.unmodifiable(const []); return _addInfoAndCreateList(fi); } @@ -184,4 +188,14 @@ class _ExtensionFieldSet { } } } + + void _checkNotInUnknown(Extension extension) { + if (_parent._hasUnknownFields && + _parent._unknownFields.hasField(extension.tagNumber)) { + throw StateError( + 'Trying to get $extension that is present as an unknown field. ' + 'Parse the message with this extension in the extension registry or use ' + '`ExtensionRegistry.reparseMessage`.'); + } + } } diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index ff3f3891d..9525eadc6 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -236,6 +236,9 @@ class _FieldSet { bool _hasField(int tagNumber) { var fi = _nonExtensionInfo(tagNumber); if (fi != null) return _$has(fi.index); + if (_hasUnknownFields && _unknownFields.hasField(tagNumber)) { + return true; + } if (!_hasExtensions) return false; return _extensions._hasField(tagNumber); } @@ -346,7 +349,8 @@ class _FieldSet { void _updateOneOfCase(int newTagnumber) { int oneofIndex = _meta.oneofs[newTagnumber]; if (oneofIndex != null) { - _clearField(_oneofCases[oneofIndex]); + final currentTagnumber = _oneofCases[oneofIndex]; + if (currentTagnumber != null) _clearField(currentTagnumber); _oneofCases[oneofIndex] = newTagnumber; } } diff --git a/protobuf/lib/src/protobuf/generated_message.dart b/protobuf/lib/src/protobuf/generated_message.dart index 64dc31dd1..9659cf235 100644 --- a/protobuf/lib/src/protobuf/generated_message.dart +++ b/protobuf/lib/src/protobuf/generated_message.dart @@ -358,6 +358,8 @@ abstract class GeneratedMessage { _fieldSet._extensions._getFieldOrNull(extension) != null; /// Returns [:true:] if this message has a field associated with [tagNumber]. + /// + /// Will return [:true:] even if that field is an unknown field. bool hasField(int tagNumber) => _fieldSet._hasField(tagNumber); /// Merges the contents of the [other] into this message. diff --git a/protoc_plugin/test/oneof_test.dart b/protoc_plugin/test/oneof_test.dart index f6659cf20..90e17ec29 100644 --- a/protoc_plugin/test/oneof_test.dart +++ b/protoc_plugin/test/oneof_test.dart @@ -183,6 +183,14 @@ void main() { expect(foo.hasIndex(), true); expect(foo.index, Bar()); }); + + test('clone', () { + Foo foo = Foo()..first = 'oneof'; + expectFirstSet(foo); + + final foo2 = Foo()..mergeFromMessage(foo); + expectFirstSet(foo2); + }); } void expectSecondSet(Foo foo) { diff --git a/protoc_plugin/test/unknown_field_set_test.dart b/protoc_plugin/test/unknown_field_set_test.dart index 2971df5c4..324cc184d 100755 --- a/protoc_plugin/test/unknown_field_set_test.dart +++ b/protoc_plugin/test/unknown_field_set_test.dart @@ -330,4 +330,9 @@ void main() { _checkNotEqual(f1, f2); }); + + test('GeneratedMessage.hasField sees unknown fields', () { + expect(emptyMessage.hasField(testAllTypes.getTagNumber('optionalInt32')), + true); + }); } From 5f66ac058987db7719e3ca81c7ca4e95c3172a0c Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Thu, 21 Nov 2019 15:16:04 +0100 Subject: [PATCH 4/5] Revert unrelated changes --- .../lib/src/protobuf/extension_field_set.dart | 16 +--------------- protoc_plugin/test/oneof_test.dart | 8 -------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index 331945e7a..fe301db83 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -21,10 +21,7 @@ class _ExtensionFieldSet { // I think this was originally here for repeated extensions. _addInfoUnchecked(fi); var value = _getFieldOrNull(fi); - if (value == null) { - _checkNotInUnknown(fi); - return fi.makeDefault(); - } + if (value == null) return fi.makeDefault(); return value; } @@ -53,7 +50,6 @@ class _ExtensionFieldSet { List _getList(Extension fi) { var value = _values[fi.tagNumber]; if (value != null) return value as List; - _checkNotInUnknown(fi); if (_isReadOnly) return List.unmodifiable(const []); return _addInfoAndCreateList(fi); } @@ -188,14 +184,4 @@ class _ExtensionFieldSet { } } } - - void _checkNotInUnknown(Extension extension) { - if (_parent._hasUnknownFields && - _parent._unknownFields.hasField(extension.tagNumber)) { - throw StateError( - 'Trying to get $extension that is present as an unknown field. ' - 'Parse the message with this extension in the extension registry or use ' - '`ExtensionRegistry.reparseMessage`.'); - } - } } diff --git a/protoc_plugin/test/oneof_test.dart b/protoc_plugin/test/oneof_test.dart index 90e17ec29..f6659cf20 100644 --- a/protoc_plugin/test/oneof_test.dart +++ b/protoc_plugin/test/oneof_test.dart @@ -183,14 +183,6 @@ void main() { expect(foo.hasIndex(), true); expect(foo.index, Bar()); }); - - test('clone', () { - Foo foo = Foo()..first = 'oneof'; - expectFirstSet(foo); - - final foo2 = Foo()..mergeFromMessage(foo); - expectFirstSet(foo2); - }); } void expectSecondSet(Foo foo) { From 309d4cfc6d78ba55ac82b69939009521ccc2fb0c Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Fri, 22 Nov 2019 12:13:26 +0100 Subject: [PATCH 5/5] Fix merging unknown fields --- protobuf/lib/src/protobuf/field_set.dart | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 9525eadc6..3f7b50d16 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -245,18 +245,14 @@ class _FieldSet { void _clearField(int tagNumber) { _ensureWritable(); + int oneofIndex = _meta.oneofs[tagNumber]; + if (oneofIndex != null) _oneofCases.remove(oneofIndex); + var fi = _nonExtensionInfo(tagNumber); if (fi != null) { // clear a non-extension field if (_hasObservers) _eventPlugin.beforeClearField(fi); _values[fi.index] = null; - - if (_meta.oneofs.containsKey(fi.tagNumber)) { - _oneofCases.remove(_meta.oneofs[fi.tagNumber]); - } - - int oneofIndex = _meta.oneofs[fi.tagNumber]; - if (oneofIndex != null) _oneofCases[oneofIndex] = 0; return; } @@ -715,7 +711,12 @@ class _FieldSet { } if (other._hasUnknownFields) { - _ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields); + final unknownFieldSet = _ensureUnknownFields(); + final otherUnknownFieldSet = other._unknownFields; + otherUnknownFieldSet._fields.forEach((key, value) { + _updateOneOfCase(key); + unknownFieldSet.mergeField(key, value); + }); } }