Fix map decoding when key or value fields are missing (#745)

Map fields are encoded as repeated message fields where the message
field 1 is the key and 2 is the value.

In proto syntax, a map field like

    map<int32, MyMessage> map_field = 1;

is encoded as if it was

    repeated MapFieldEntry map_field = 1;

where

    message MapFieldEntry {
        optional int32 key = 1;
        optional MyMessage value = 2;
    }

Since map entries are ordinary messages, it's possible for a map entry
to have no key or value fields.

This PR updates map decoding to handle missing key and value fields.
Three tests added for (1) missing key field (2) missing value field (3)
missing key and value fields (entry message has 0 length prefix).

Fixes #719

Author: osa1

protobuf/CHANGELOG.md

@@ -16,6 +16,8 @@
 * Avoid copying when reading map fields of read-only messages. ([#741])
 * Fix `PbMap._isReadonly` field initialization in `PbMap.unmodifiable`.
   ([#741])
+* Fix decoding map fields when key or value (or both) fields of a map entry is
+  missing. ([#719], [#745])
 
 [#183]: https://github.com/google/protobuf.dart/issues/183
 [#644]: https://github.com/google/protobuf.dart/pull/644
@@ -29,6 +31,8 @@
 [#721]: https://github.com/google/protobuf.dart/pull/721
 [#681]: https://github.com/google/protobuf.dart/pull/681
 [#741]: https://github.com/google/protobuf.dart/pull/741
+[#719]: https://github.com/google/protobuf.dart/issues/719
+[#745]: https://github.com/google/protobuf.dart/pull/745
 
 ## 2.1.0
 

protobuf/lib/src/protobuf/builder_info.dart

@@ -241,11 +241,12 @@ class BuilderInfo {
       List<ProtobufEnum>? enumValues,
       ProtobufEnum? defaultEnumValue,
       PackageName packageName = const PackageName(''),
-      String? protoName}) {
+      String? protoName,
+      dynamic valueDefaultOrMaker}) {
     var mapEntryBuilderInfo = BuilderInfo(entryClassName, package: packageName)
       ..add(PbMap._keyFieldNumber, 'key', keyFieldType, null, null, null, null)
-      ..add(PbMap._valueFieldNumber, 'value', valueFieldType, null,
-          valueCreator, valueOf, enumValues);
+      ..add(PbMap._valueFieldNumber, 'value', valueFieldType,
+          valueDefaultOrMaker, valueCreator, valueOf, enumValues);
 
     addMapField<K, V>(tagNumber, name, keyFieldType, valueFieldType,
         mapEntryBuilderInfo, valueCreator,

protoc_plugin/CHANGELOG.md

@@ -22,10 +22,14 @@
   ```
 * Export public dependencies (`import public`s in proto files) in
   `.pbenum.dart` files, same as `.pb.dart` files. ([9aad6aa])
+* Fix decoding map fields when key or value (or both) fields of a map entry is
+  missing. ([#719], [#745])
 
 [#679]: https://github.com/google/protobuf.dart/pull/679
 [#703]: https://github.com/google/protobuf.dart/pull/703
 [9aad6aa]: https://github.com/google/protobuf.dart/commits/9aad6aa
+[#719]: https://github.com/google/protobuf.dart/issues/719
+[#745]: https://github.com/google/protobuf.dart/pull/745
 
 ## 20.0.1
 

protoc_plugin/lib/src/protobuf_field.dart

@@ -204,8 +204,13 @@ class ProtobufField {
       final generator = baseType.generator as MessageGenerator;
       var key = generator._fieldList[0];
       var value = generator._fieldList[1];
-      var keyType = key.baseType.getDartType(parent.fileGen!);
-      var valueType = value.baseType.getDartType(parent.fileGen!);
+
+      // Key type is an integer type or string. No need to specify the default
+      // value as the library knows the defaults for integer and string fields.
+      final keyType = key.baseType.getDartType(parent.fileGen!);
+
+      // Value type can be anything other than another map.
+      final valueType = value.baseType.getDartType(parent.fileGen!);
 
       invocation = 'm<$keyType, $valueType>';
 
@@ -214,10 +219,12 @@ class ProtobufField {
       named['valueFieldType'] = value.typeConstant;
       if (value.baseType.isMessage || value.baseType.isGroup) {
         named['valueCreator'] = '$valueType.create';
+        named['valueDefaultOrMaker'] = value.generateDefaultFunction();
       }
       if (value.baseType.isEnum) {
         named['valueOf'] = '$valueType.valueOf';
         named['enumValues'] = '$valueType.values';
+        named['valueDefaultOrMaker'] = value.generateDefaultFunction();
         named['defaultEnumValue'] = value.generateDefaultFunction();
       }
       if (package != '') {

protoc_plugin/test/map_field_test.dart

@@ -370,4 +370,81 @@ void main() {
     map2[1] = 2;
     expect(msg2.int32ToInt32Field[1], 2);
   });
+
+  test('Parses empty map fields', () {
+    // Map fields are encoded as messages (as length-delimited fields). Check
+    // that we handle 0 length fields. (#719)
+    {
+      final messageBytes = <int>[
+        (5 << 3) | 2, // tag = 5, wire type = 2 (length delimited)
+        0, // length = 0
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(
+          message, TestMap()..int32ToMessageField[0] = TestMap_MessageValue());
+    }
+
+    {
+      final messageBytes = <int>[
+        (4 << 3) | 2, // tag = 4, wire type = 2 (length delimited)
+        0, // length = 0
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(
+          message, TestMap()..int32ToEnumField[0] = TestMap_EnumValue.DEFAULT);
+    }
+  });
+
+  test('Parses map field with just key', () {
+    // Similar to the case above, but the field just has key (no value)
+    {
+      final messageBytes = <int>[
+        (5 << 3) | 2, // tag = 5, wire type = 2 (length delimited)
+        2, // length = 2
+        (1 << 3) | 0, // tag = 1 (map key), wire type = 0 (varint)
+        1, // key = 1
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(
+          message, TestMap()..int32ToMessageField[1] = TestMap_MessageValue());
+    }
+
+    {
+      final messageBytes = <int>[
+        (4 << 3) | 2, // tag = 4, wire type = 2 (length delimited)
+        2, // length = 2
+        (1 << 3) | 0, // tag = 1 (map key), wire type = 0 (varint)
+        1, // key = 1
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(
+          message, TestMap()..int32ToEnumField[1] = TestMap_EnumValue.DEFAULT);
+    }
+  });
+
+  test('Parses map field with just key', () {
+    // Similar to the case above, but the field just has value (no key)
+    {
+      final messageBytes = <int>[
+        (5 << 3) | 2, // tag = 5, wire type = 2 (length delimited)
+        2, // length = 2
+        (2 << 3) | 2, // tag = 2 (map value), wire type = 2 (length delimited)
+        0, // length = 0 (empty message)
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(
+          message, TestMap()..int32ToMessageField[0] = TestMap_MessageValue());
+    }
+
+    {
+      final messageBytes = <int>[
+        (4 << 3) | 2, // tag = 4, wire type = 2 (length delimited)
+        2, // length = 2
+        (2 << 3) | 2, // tag = 2 (map value), wire type = 2 (length delimited)
+        1, // enum value = 1
+      ];
+      final message = TestMap.fromBuffer(messageBytes);
+      expect(message, TestMap()..int32ToEnumField[0] = TestMap_EnumValue.BAR);
+    }
+  });
 }