diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index 83335f37..2ea18053 100644 --- a/protobuf/CHANGELOG.md +++ b/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 + `ignoreUnknownFields` optional arguments is `true`. ([#853]) [#742]: https://github.com/google/protobuf.dart/pull/742 +[#853]: https://github.com/google/protobuf.dart/pull/853 ## 4.2.0 diff --git a/protobuf/lib/src/protobuf/proto3_json.dart b/protobuf/lib/src/protobuf/proto3_json.dart index de0c2428..760f8738 100644 --- a/protobuf/lib/src/protobuf/proto3_json.dart +++ b/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(); }); diff --git a/protobuf/test/json_test.dart b/protobuf/test/json_test.dart index beec88f2..5688f0ce 100644 --- a/protobuf/test/json_test.dart +++ b/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([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([1, 2, 3]); + void checkJsonMap(Map m, {Map? unknownFields}) { expect(m.length, 3 + (unknownFields?.length ?? 0)); expect(m['1'], 123); diff --git a/protoc_plugin/Makefile b/protoc_plugin/Makefile index 217fdfd8..18aaa658 100644 --- a/protoc_plugin/Makefile +++ b/protoc_plugin/Makefile @@ -31,6 +31,7 @@ TEST_PROTO_LIST = \ entity \ enum_extension \ enum_name \ + enum_test \ enums \ extend_unittest \ ExtensionEnumNameConflict \ diff --git a/protoc_plugin/test/protos/enum_test.proto b/protoc_plugin/test/protos/enum_test.proto new file mode 100644 index 00000000..d9bf9219 --- /dev/null +++ b/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 map_value_field = 2; + repeated A repeated_enum_field = 3; +} diff --git a/protoc_plugin/test/unknown_enums_test.dart b/protoc_plugin/test/unknown_enums_test.dart new file mode 100644 index 00000000..8961f26f --- /dev/null +++ b/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]); + }); + }); +}