Fix unknown enum handling (google#853)

This fixes handling of unknown enum values in repeated and map fields and in
top-level fields.

Current behavior:

- Unknown enums in map and repeated fields throw an exception.
- Unknown enums in top-level fields reset the field value to the default one.

With this PR we just "ignore" the values:

- Unknown enums in map and repeated fields are skipped.
- Unknown enums in top-level fields do not reset the field's value to the
  default.

Fixes google#849.

Author: osa1

protobuf/CHANGELOG.md

@@ -1,8 +1,11 @@
 ## 5.0.0-wip
 
 * Improve performance of `GeneratedMessage.deepCopy`. ([#742])
+* Fix unknown enum handling in `GeneratedMessage.mergeFromProto3Json` when
+  the `ignoreUnknownFields` optional argument is `true`. ([#853])
 
 [#742]: https://github.com/google/protobuf.dart/pull/742
+[#853]: https://github.com/google/protobuf.dart/pull/853
 
 ## 4.2.0
 

protobuf/lib/src/protobuf/proto3_json.dart

@@ -307,6 +307,8 @@ void _mergeFromProto3JsonWithContext(
   fieldSet._ensureWritable();
 
   void recursionHelper(Object? json, FieldSet fieldSet) {
+    // Convert a JSON object to proto object. Returns `null` on unknown enum
+    // values when [ignoreUnknownFields] is [true].
     Object? convertProto3JsonValue(Object value, FieldInfo fieldInfo) {
       final fieldType = fieldInfo.type;
       switch (PbFieldType.baseType(fieldType)) {
@@ -317,16 +319,14 @@ void _mergeFromProto3JsonWithContext(
           throw context.parseException('Expected bool value', json);
         case PbFieldType.BYTES_BIT:
           if (value is String) {
-            Uint8List result;
             try {
-              result = base64Decode(value);
+              return base64Decode(value);
             } on FormatException {
               throw context.parseException(
                 'Expected bytes encoded as base64 String',
                 json,
               );
             }
-            return result;
           }
           throw context.parseException(
             'Expected bytes encoded as base64 String',
@@ -427,16 +427,14 @@ void _mergeFromProto3JsonWithContext(
         case PbFieldType.SFIXED64_BIT:
           if (value is int) return Int64(value);
           if (value is String) {
-            Int64 result;
             try {
-              result = Int64.parseInt(value);
+              return Int64.parseInt(value);
             } on FormatException {
               throw context.parseException(
                 'Expected int or stringified int',
                 value,
               );
             }
-            return result;
           }
           throw context.parseException(
             'Expected int or stringified int',
@@ -677,8 +675,14 @@ void _mergeFromProto3JsonWithContext(
                 throw context.parseException('Expected a String key', subKey);
               }
               context.addMapIndex(subKey);
-              fieldValues[decodeMapKey(subKey, mapFieldInfo.keyFieldType)] =
-                  convertProto3JsonValue(subValue, mapFieldInfo.valueFieldInfo);
+              final key = decodeMapKey(subKey, mapFieldInfo.keyFieldType);
+              final value = convertProto3JsonValue(
+                subValue,
+                mapFieldInfo.valueFieldInfo,
+              );
+              if (value != null) {
+                fieldValues[key] = value;
+              }
               context.popIndex();
             });
           } else {
@@ -690,7 +694,10 @@ void _mergeFromProto3JsonWithContext(
             for (var i = 0; i < value.length; i++) {
               final entry = value[i];
               context.addListIndex(i);
-              values.add(convertProto3JsonValue(entry, fieldInfo));
+              final parsedValue = convertProto3JsonValue(entry, fieldInfo);
+              if (parsedValue != null) {
+                values.add(parsedValue);
+              }
               context.popIndex();
             }
           } else {
@@ -712,11 +719,15 @@ void _mergeFromProto3JsonWithContext(
             original.mergeFromMessage(parsedSubMessage);
           }
         } else {
-          fieldSet._setFieldUnchecked(
-            meta,
-            fieldInfo,
-            convertProto3JsonValue(value, fieldInfo),
-          );
+          final parsedValue = convertProto3JsonValue(value, fieldInfo);
+          if (parsedValue == null) {
+            // Unknown enum
+            if (!context.ignoreUnknownFields) {
+              throw context.parseException('Unknown enum value', value);
+            }
+          } else {
+            fieldSet._setFieldUnchecked(meta, fieldInfo, parsedValue);
+          }
         }
         context.popIndex();
       });

protobuf/test/json_test.dart

@@ -14,22 +14,21 @@ import 'package:test/test.dart';
 import 'mock_util.dart' show T, mockEnumValues;
 
 void main() {
-  final example =
-      T()
-        ..val = 123
-        ..str = 'hello'
-        ..int32s.addAll(<int>[1, 2, 3]);
+  test('mergeFromProto3Json unknown enum fields with names', () {
+    final example = T();
 
-  test('testProto3JsonEnum', () {
     // No enum value specified.
     expect(example.hasEnm, isFalse);
+
     // Defaults to first when it doesn't exist.
     expect(example.enm, equals(mockEnumValues.first));
     expect((example..mergeFromProto3Json({'enm': 'a'})).enm.name, equals('a'));
+
     // Now it's explicitly set after merging.
     expect(example.hasEnm, isTrue);
 
     expect((example..mergeFromProto3Json({'enm': 'b'})).enm.name, equals('b'));
+
     // "c" is not a legal enum value.
     expect(
       () => example..mergeFromProto3Json({'enm': 'c'}),
@@ -42,21 +41,26 @@ void main() {
         ),
       ),
     );
+
     // `example` hasn't changed.
     expect(example.hasEnm, isTrue);
     expect(example.enm.name, equals('b'));
 
     // "c" is not a legal enum value, but we are ignoring unknown fields, so
-    // default behavior is to unset `enm`, returning the default value "a"
+    // `enm` value shouldn't change.
     expect(
       (example..mergeFromProto3Json({'enm': 'c'}, ignoreUnknownFields: true))
           .enm
           .name,
-      equals('a'),
+      equals('b'),
     );
-    expect(example.hasEnm, isFalse);
+    expect(example.hasEnm, isTrue);
+  });
+
+  test('mergeFromProto3Json unknown enum fields with indices', () {
+    // Similar to above, but with indices.
+    final example = T();
 
-    // Same for index values...
     expect((example..mergeFromProto3Json({'enm': 2})).enm.name, 'b');
     expect(
       () => example..mergeFromProto3Json({'enm': 3}),
@@ -69,34 +73,35 @@ void main() {
         ),
       ),
     );
+
     // `example` hasn't changed.
     expect(example.hasEnm, isTrue);
     expect(example.enm.name, equals('b'));
 
-    // "c" is not a legal enum value, but we are ignoring unknown fields, so
-    // default behavior is to unset `enm`, returning the default value "a"
+    // "c" is not a legal enum value, but we are ignoring unknown fields, so the
+    // value shouldn't change.
     expect(
       (example..mergeFromProto3Json({'enm': 3}, ignoreUnknownFields: true))
           .enm
           .name,
-      equals('a'),
+      equals('b'),
     );
-    expect(example.hasEnm, isFalse);
+    expect(example.hasEnm, isTrue);
   });
 
   test('testWriteToJson', () {
-    final json = example.writeToJson();
+    final json = makeTestJson().writeToJson();
     checkJsonMap(jsonDecode(json));
   });
 
   test('testWriteFrozenToJson', () {
-    final frozen = example.clone()..freeze();
+    final frozen = makeTestJson().clone()..freeze();
     final json = frozen.writeToJson();
     checkJsonMap(jsonDecode(json));
   });
 
   test('writeToJsonMap', () {
-    final Map m = example.writeToJsonMap();
+    final Map m = makeTestJson().writeToJsonMap();
     checkJsonMap(m);
   });
 
@@ -134,13 +139,19 @@ void main() {
   });
 
   test('testJsonMapWithUnknown', () {
-    final m = example.writeToJsonMap();
+    final m = makeTestJson().writeToJsonMap();
     m['9999'] = 'world';
     final t = T()..mergeFromJsonMap(m);
     checkJsonMap(t.writeToJsonMap(), unknownFields: {'9999': 'world'});
   });
 }
 
+T makeTestJson() =>
+    T()
+      ..val = 123
+      ..str = 'hello'
+      ..int32s.addAll(<int>[1, 2, 3]);
+
 void checkJsonMap(Map m, {Map<String, dynamic>? unknownFields}) {
   expect(m.length, 3 + (unknownFields?.length ?? 0));
   expect(m['1'], 123);

protoc_plugin/Makefile

@@ -31,6 +31,7 @@ TEST_PROTO_LIST = \
 	entity \
 	enum_extension \
 	enum_name \
+	enum_test \
 	enums \
 	extend_unittest \
 	ExtensionEnumNameConflict \

protoc_plugin/test/protos/enum_test.proto

@@ -0,0 +1,16 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+syntax = "proto3";
+
+enum A {
+  X = 0;
+  Y = 1;
+}
+
+message Message {
+  A enum_field = 1;
+  map<int32, A> map_value_field = 2;
+  repeated A repeated_enum_field = 3;
+}

protoc_plugin/test/unknown_enums_test.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:test/test.dart';
+
+import 'gen/enum_test.pb.dart';
+
+void main() {
+  // 'Z' below is an unknown enum value. Known values are 'X' and 'Y'.
+  group('Enum parsing in maps, lists, messages', () {
+    test('Parse known fields', () {
+      final json = {
+        'enumField': 'Y',
+        'mapValueField': {'1': 'Y'},
+        'repeatedEnumField': ['Y'],
+      };
+
+      final msg = Message();
+      msg.mergeFromProto3Json(json);
+      expect(msg.enumField, A.Y);
+      expect(msg.mapValueField.values.toList(), [A.Y]);
+      expect(msg.repeatedEnumField, [A.Y]);
+    });
+
+    test('Skip unknown fields', () {
+      final json = {
+        'enumField': 'Z',
+        'mapValueField': {'1': 'X', '2': 'Z', '3': 'Y'},
+        'repeatedEnumField': ['X', 'Z', 'Y'],
+      };
+
+      final msg = Message();
+      msg.enumField = A.Y;
+      msg.mergeFromProto3Json(json, ignoreUnknownFields: true);
+      expect(msg.enumField, A.Y);
+      expect(msg.mapValueField.values.toList(), [A.X, A.Y]);
+      expect(msg.repeatedEnumField, [A.X, A.Y]);
+    });
+  });
+}