Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions protobuf/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
1 change: 1 addition & 0 deletions protobuf/lib/src/protobuf/coded_buffer.dart
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
18 changes: 11 additions & 7 deletions protobuf/lib/src/protobuf/field_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,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 +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;
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
7 changes: 5 additions & 2 deletions protobuf/lib/src/protobuf/generated_message.dart
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

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
18 changes: 18 additions & 0 deletions protobuf/lib/src/protobuf/unknown_field_set.dart
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
2 changes: 1 addition & 1 deletion protobuf/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: protobuf
version: 1.0.1
version: 1.0.2
author: Dart Team <[email protected]>
description: >
Runtime library for protocol buffers support.
Expand Down