From 97d696acd4020219d0c2aa0c59f62e94b448b7bc Mon Sep 17 00:00:00 2001 From: Sarah Zakarias Date: Tue, 22 Jan 2019 12:18:53 +0100 Subject: [PATCH] Fix issue with not being able to read extensions after freeze (#186) --- protobuf/CHANGELOG.md | 4 ++ .../lib/src/protobuf/extension_field_set.dart | 53 ++++++++++++++++--- protobuf/lib/src/protobuf/field_set.dart | 4 +- .../lib/src/protobuf/generated_message.dart | 1 - protobuf/pubspec.yaml | 2 +- protoc_plugin/test/to_builder_test.dart | 10 ++-- 6 files changed, 61 insertions(+), 13 deletions(-) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index 79d0ad6709510..8c32d404ac456 100644 --- a/protobuf/CHANGELOG.md +++ b/protobuf/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.13.0 + +* Breaking change: Fix issue with not being able to read extensions after freezing. + ## 0.12.0 * Breaking change: Changed `BuilderInfo.m()` to take class and package name of the protobuf message representing the map diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index ae6a4d4faee98..2ebb0f9434148 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -8,16 +8,14 @@ class _ExtensionFieldSet { final _FieldSet _parent; final Map _info = {}; final Map _values = {}; + bool _isReadOnly = false; - _ExtensionFieldSet(this._parent) { - // Read-only fieldsets shouldn't have extension fields. - assert(!_parent._isReadOnly); - } + _ExtensionFieldSet(this._parent); Extension _getInfoOrNull(int tagNumber) => _info[tagNumber]; _getFieldOrDefault(Extension fi) { - if (fi.isRepeated) return _ensureRepeatedField(fi); + if (fi.isRepeated) return _getList(fi); _validateInfo(fi); // TODO(skybrian) seems unnecessary to add info? // I think this was originally here for repeated extensions. @@ -39,13 +37,24 @@ class _ExtensionFieldSet { /// If it doesn't exist, creates the list and saves the extension. /// Suitable for public API and decoders. List _ensureRepeatedField(Extension fi) { + assert(!_isReadOnly); assert(fi.isRepeated); assert(fi.extendee == _parent._messageName); var list = _values[fi.tagNumber]; if (list != null) return list as List; - // Add info and create list. + return _addInfoAndCreateList(fi); + } + + List _getList(Extension fi) { + var value = _values[fi.tagNumber]; + if (value != null) return value as List; + if (_isReadOnly) return new List.unmodifiable(const []); + return _addInfoAndCreateList(fi); + } + + List _addInfoAndCreateList(Extension fi) { _validateInfo(fi); var newList = fi._createRepeatedField(_parent._message); _addInfoUnchecked(fi); @@ -61,6 +70,7 @@ class _ExtensionFieldSet { } void _clearField(Extension fi) { + _ensureWritable(); _validateInfo(fi); if (_parent._hasObservers) _parent._eventPlugin.beforeClearField(fi); _values.remove(fi.tagNumber); @@ -78,6 +88,7 @@ class _ExtensionFieldSet { throw new ArgumentError(_parent._setFieldFailedMessage( fi, value, 'repeating field (use get + .add())')); } + _ensureWritable(); _parent._validateField(fi, value); _setFieldUnchecked(fi, value); } @@ -85,16 +96,22 @@ class _ExtensionFieldSet { /// Sets a non-repeated value and extension. /// Overwrites any existing extension. void _setFieldAndInfo(Extension fi, value) { + _ensureWritable(); if (fi.isRepeated) { throw new ArgumentError(_parent._setFieldFailedMessage( fi, value, 'repeating field (use get + .add())')); } + _ensureWritable(); _validateInfo(fi); _parent._validateField(fi, value); _addInfoUnchecked(fi); _setFieldUnchecked(fi, value); } + void _ensureWritable() { + if (_isReadOnly) frozenMessageModificationHandler(_parent._messageName); + } + void _validateInfo(Extension fi) { if (fi.extendee != _parent._messageName) { throw new ArgumentError( @@ -138,11 +155,33 @@ class _ExtensionFieldSet { final value = original._getFieldOrNull(extension); if (value == null) continue; if (extension.isRepeated) { - assert(value is PbList); + assert(value is PbListBase); _ensureRepeatedField(extension)..addAll(value); } else { _setFieldUnchecked(extension, value); } } } + + void _markReadOnly() { + if (_isReadOnly) return; + _isReadOnly = true; + for (Extension field in _info.values) { + if (field.isRepeated) { + final entries = _values[field.tagNumber]; + if (entries == null) continue; + if (field.isGroupOrMessage) { + for (var subMessage in entries as List) { + subMessage.freeze(); + } + } + _values[field.tagNumber] = entries.toFrozenPbList(); + } else if (field.isGroupOrMessage) { + final entry = _values[field.tagNumber]; + if (entry != null) { + (entry as GeneratedMessage).freeze(); + } + } + } + } } diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 4723b0422445e..90814d6a6b2bb 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -83,7 +83,6 @@ class _FieldSet { bool get hasUnknownFields => _unknownFields != null; _ExtensionFieldSet _ensureExtensions() { - _ensureWritable(); if (!_hasExtensions) _extensions = new _ExtensionFieldSet(this); return _extensions; } @@ -142,6 +141,9 @@ class _FieldSet { } } } + if (_hasExtensions) { + _ensureExtensions()._markReadOnly(); + } } void _ensureWritable() { diff --git a/protobuf/lib/src/protobuf/generated_message.dart b/protobuf/lib/src/protobuf/generated_message.dart index c78e11d85cf89..8eb3d69e4d830 100644 --- a/protobuf/lib/src/protobuf/generated_message.dart +++ b/protobuf/lib/src/protobuf/generated_message.dart @@ -250,7 +250,6 @@ abstract class GeneratedMessage { /// /// If not set, returns the extension's default value. getExtension(Extension extension) { - if (_fieldSet._isReadOnly) return extension.readonlyDefault; return _fieldSet._ensureExtensions()._getFieldOrDefault(extension); } diff --git a/protobuf/pubspec.yaml b/protobuf/pubspec.yaml index 05ba43ee9cc29..c3c8ab5dadccb 100644 --- a/protobuf/pubspec.yaml +++ b/protobuf/pubspec.yaml @@ -1,5 +1,5 @@ name: protobuf -version: 0.12.0 +version: 0.13.0 author: Dart Team description: > Runtime library for protocol buffers support. diff --git a/protoc_plugin/test/to_builder_test.dart b/protoc_plugin/test/to_builder_test.dart index 0ebe8beae09df..9fca5abb1cc1d 100644 --- a/protoc_plugin/test/to_builder_test.dart +++ b/protoc_plugin/test/to_builder_test.dart @@ -13,18 +13,22 @@ main() { expect(original.getExtension(FooExt.inner).value, 'extension'); expect( original.getExtension(FooExt.inners)[0].value, 'repeatedExtension'); - }, skip: 'https://github.com/dart-lang/protobuf/issues/171'); + }); test('frozen message cannot be modified', () { expect(() => original.inner = (Inner()..value = 'bar'), throwsA(TypeMatcher())); expect(() => original.inner..value = 'bar', throwsA(TypeMatcher())); + expect(() => original.inners.add(Inner()..value = 'bar'), + throwsA(TypeMatcher())); }); test('extensions cannot be modified', () { expect(() => original.setExtension(FooExt.inner, Inner()..value = 'bar'), throwsA(TypeMatcher())); + expect(() => original.getExtension(FooExt.inner).value = 'bar', + throwsA(TypeMatcher())); expect( () => original.getExtension(FooExt.inners).add(Inner()..value = 'bar'), @@ -38,7 +42,7 @@ main() { test('builder extensions are also copied shallowly', () { expect(builder.getExtension(FooExt.inner), same(original.getExtension(FooExt.inner))); - }, skip: 'https://github.com/dart-lang/protobuf/issues/171'); + }); test('repeated fields are cloned', () { expect(builder.inners, isNot(same(original.inners))); @@ -50,7 +54,7 @@ main() { isNot(same(original.getExtension(FooExt.inners)))); expect(builder.getExtension(FooExt.inners)[0], same(original.getExtension(FooExt.inners)[0])); - }, skip: 'https://github.com/dart-lang/protobuf/issues/171'); + }); test( 'the builder is only a shallow copy, the nested message is still frozen.',