Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle unknown fields part of a oneof #324

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+69 −27
Open

Handle unknown fields part of a oneof #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a1cfc04
Handle unknown fields part of a oneof
sigurdm Nov 15, 2019
2e9934d
Use helper from _
sigurdm Nov 15, 2019
d53a9fb
Address review
sigurdm Nov 21, 2019
5f66ac0
Revert unrelated changes
sigurdm Nov 21, 2019
2f7be7e
Merge to master
sigurdm Nov 21, 2019
309d4cf
Fix merging unknown fields
sigurdm Nov 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions protobuf/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
## 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.

## 1.0.1

* Fix issue with `ExtensionRegistry.reparseMessage` not handling map fields with
Expand Down
1 change: 1 addition & 0 deletions protobuf/lib/src/protobuf/coded_buffer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ void _mergeFromCodedBufferReader(
}

if (fi == null || !_wireTypeMatches(fi.type, wireType)) {
fs._updateOneOfCase(tagNumber);
if (!fs._ensureUnknownFields().mergeFieldFromBuffer(tag, input)) {
return;
}
Expand Down
49 changes: 26 additions & 23 deletions protobuf/lib/src/protobuf/field_set.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,24 +236,23 @@ 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);
}

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;
}

Expand All @@ -265,8 +264,9 @@ class _FieldSet {
}
}

// neither a regular field nor an extension.
// TODO(skybrian) throw?
if (_hasUnknownFields) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that _clearField(tagNumber) checks unknown field set, but e.g. _hasField(tagNumber) does not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question!
I think we could change the _hasField semantics also.
Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are now changing semantics to include unknowns, I agree that it is inconsistent if we don't also update _hasField. Good catch!

_unknownFields._clearField(tagNumber);
}
}

/// Sets a non-repeated field with error-checking.
Expand Down Expand Up @@ -342,14 +342,18 @@ 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) {
sigurdm marked this conversation as resolved.
Show resolved Hide resolved
int oneofIndex = _meta.oneofs[newTagnumber];
if (oneofIndex != null) {
_clearField(_oneofCases[oneofIndex]);
_oneofCases[oneofIndex] = tag;
final currentTagnumber = _oneofCases[oneofIndex];
if (currentTagnumber != null) _clearField(currentTagnumber);
_oneofCases[oneofIndex] = newTagnumber;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe only call it conditionally

final currentTag = _oneofCases[oneofIndex];
if (currentTag != null) _clearField(currentTag);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - that's better

}
}

/// 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.
Expand Down Expand Up @@ -498,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;
}

Expand Down Expand Up @@ -713,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);
});
}
}

Expand Down
9 changes: 7 additions & 2 deletions protobuf/lib/src/protobuf/generated_message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GeneratedMessage.mergeFromMessage() to call _FieldSet._mergeFromMessage(), which looks like this:

  void _mergeFromMessage(_FieldSet other) {
    for (FieldInfo fi in other._infosSortedByTag) {
      var value = other._values[fi.index];
      if (value != null) _mergeField(fi, value, isExtension: false);
    }
    if (other._hasExtensions) {
      var others = other._extensions;
      for (int tagNumber in others._tagNumbers) {
        var extension = others._getInfoOrNull(tagNumber);
        var value = others._getFieldOrNull(extension);
        _mergeField(extension, value, isExtension: true);
      }
    }
  
    if (other._hasUnknownFields) {
      _ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields);
    }
  }

This merging does not seem to account for oneofs. Does it need to be updated as well? Are there more places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_FieldSet._mergeFromMessage() calls _mergeField that ends up calling _setNonExtensionFieldUnchecked that handles the oneofs

Copy link
Collaborator

@mkustermann mkustermann Nov 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my comment was not clear, what I meant is the following:

We seem to have an invariant now

  • there is at most one of the oneof cases set (either a) it's set in field set b) it's set in unknown field set c) none are set)
  • whenever we add a new field value, we are guaranteed to clear the old one if one exists.

This invariant is violated because of this line:

    if (other._hasUnknownFields) {
      _ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields);
    }

We might have already a oneof candidate in the fieldset (or the unknown fieldset). Yet this code blindly copies other._unknownFields, which possibly has a new oneof value, but we do not clear the old one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - now I get it - thanks! I updated the merging of unknown fields to go field by field, and also update the one-ofs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, Martin. Thanks.

void clearField(int tagNumber) => _fieldSet._clearField(tagNumber);

int $_whichOneof(int oneofIndex) => _fieldSet._oneofCases[oneofIndex] ?? 0;
Expand Down Expand Up @@ -355,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.
Expand Down
18 changes: 18 additions & 0 deletions protobuf/lib/src/protobuf/unknown_field_set.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -76,6 +87,7 @@ class UnknownFieldSet {
}
}

// TODO(sigurdm): Does not update oneofs. Consider deprecating.
void mergeFromCodedBufferReader(CodedBufferReader input) {
_ensureWritable('mergeFromCodedBufferReader');
while (true) {
Expand All @@ -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) {
Expand All @@ -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<int> 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);
Expand Down
4 changes: 2 additions & 2 deletions protobuf/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: protobuf
version: 1.0.1
description: >
version: 1.0.2
description:
Runtime library for protocol buffers support.
Use https://pub.dartlang.org/packages/protoc_plugin to generate dart code for your '.proto' files.
homepage: https://github.com/dart-lang/protobuf
Expand Down
5 changes: 5 additions & 0 deletions protoc_plugin/test/unknown_field_set_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,4 +330,9 @@ void main() {

_checkNotEqual(f1, f2);
});

test('GeneratedMessage.hasField sees unknown fields', () {
expect(emptyMessage.hasField(testAllTypes.getTagNumber('optionalInt32')),
true);
});
}