-
Notifications
You must be signed in to change notification settings - Fork 197
Description
Currently we have 3 field set implementations:
-
For known fields: https://github.com/google/protobuf.dart/blob/faa266368030556340650b7eb79bc0a98d6e4dc9/protobuf/lib/src/protobuf/field_set.dart
A field is known if we have the field in the .proto file for the message.
-
For unknown fields: https://github.com/google/protobuf.dart/blob/faa266368030556340650b7eb79bc0a98d6e4dc9/protobuf/lib/src/protobuf/unknown_field_set.dart
A field that we see in a serialized message is unknown if we don't have it defined in the .proto file for the message.
-
For extension fields: https://github.com/google/protobuf.dart/blob/faa266368030556340650b7eb79bc0a98d6e4dc9/protobuf/lib/src/protobuf/extension_field_set.dart
I don't understand why we need 3 different implementations and it's not documented. Conceptually we just need to map integers (field tags) to proto values. As far as I understand, the only difference between a known field and an unknown field is that for unknown fields FieldInfo won't be available. We don't need this much code for this.
We will probably need a new value class (like PbMap, PbList) for unknown length-delimited values (strings, bytes, messages, repeated fields). The representation will just be Uint8List.
I'm not sure if we need to support groups, but we will need another value class for unknown groups, with Uint8List to store the group contents.
When we need to check whether the field for a tag is known, unknown, or extension, we could have Set<int> unknownFields and Set<int> extensionFields in _FieldInfo.
One question is how to merge two unknown groups or length-delimiteds. Merging values is not specified in proto spec. We have two options:
-
Collect values of merged groups and length-delimited stuff. This means
PbUnknownLengthDelimited(or whatever we want to call it) will storeList<Uint8List>instead of justUint8List, and add to the list as we merge values. Same for groups.This is currently what we do in
UnknownFieldSet. -
Override the current value. This is simpler.
If I'm missing something and we really need 2 extra classes and a few hundred extra lines and tests then we should document why this is needed.