From bef02ba6d9d7ad19dddedbc2ef8c1583a517e90a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Thu, 22 Jun 2023 09:40:23 +0200 Subject: [PATCH 1/8] Fix unknown enum handling Fixes #849. --- protobuf/lib/src/protobuf/proto3_json.dart | 35 ++++++++++++------- protoc_plugin/Makefile | 1 + protoc_plugin/test/protos/enum_test.proto | 16 +++++++++ protoc_plugin/test/unknown_enums_test.dart | 40 ++++++++++++++++++++++ 4 files changed, 80 insertions(+), 12 deletions(-) create mode 100644 protoc_plugin/test/protos/enum_test.proto create mode 100644 protoc_plugin/test/unknown_enums_test.dart diff --git a/protobuf/lib/src/protobuf/proto3_json.dart b/protobuf/lib/src/protobuf/proto3_json.dart index c40806d4a..14e8c4bd9 100644 --- a/protobuf/lib/src/protobuf/proto3_json.dart +++ b/protobuf/lib/src/protobuf/proto3_json.dart @@ -164,6 +164,8 @@ void _mergeFromProto3Json( ignoreUnknownFields, supportNamesWithUnderscores, permissiveEnums); 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)) { @@ -174,14 +176,12 @@ void _mergeFromProto3Json( 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', value); @@ -264,14 +264,12 @@ void _mergeFromProto3Json( 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', value); @@ -368,9 +366,12 @@ void _mergeFromProto3Json( 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 { @@ -382,7 +383,10 @@ void _mergeFromProto3Json( 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 { @@ -402,8 +406,15 @@ void _mergeFromProto3Json( original.mergeFromMessage(parsedSubMessage); } } else { - fieldSet._setFieldUnchecked( - meta, fieldInfo, convertProto3JsonValue(value, fieldInfo)); + final parsedValue = convertProto3JsonValue(value, fieldInfo); + if (parsedValue == null) { + // Unknown enum + if (!ignoreUnknownFields) { + throw context.parseException('Unknown enum value', value); + } + } else { + fieldSet._setFieldUnchecked(meta, fieldInfo, parsedValue); + } } context.popIndex(); }); diff --git a/protoc_plugin/Makefile b/protoc_plugin/Makefile index ec693bdca..e5130634f 100644 --- a/protoc_plugin/Makefile +++ b/protoc_plugin/Makefile @@ -31,6 +31,7 @@ TEST_PROTO_LIST = \ entity \ enum_extension \ enum_name \ + enum_test \ extend_unittest \ ExtensionEnumNameConflict \ ExtensionNameConflict \ diff --git a/protoc_plugin/test/protos/enum_test.proto b/protoc_plugin/test/protos/enum_test.proto new file mode 100644 index 000000000..f80dc6bc9 --- /dev/null +++ b/protoc_plugin/test/protos/enum_test.proto @@ -0,0 +1,16 @@ +// Copyright (c) 2023, 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 000000000..c891f7865 --- /dev/null +++ b/protoc_plugin/test/unknown_enums_test.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2023, 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 '../out/protos/enum_test.pb.dart'; + +void main() { + 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]); + }); + }); +} From c6cad9a7e110b275a0756ac6b939af3a94f23d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Tue, 5 Aug 2025 09:32:10 +0100 Subject: [PATCH 2/8] Revert non-repeated non-map unknown enum value behavior --- protobuf/lib/src/protobuf/proto3_json.dart | 14 +++++--------- protoc_plugin/test/unknown_enums_test.dart | 12 ++++++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/protobuf/lib/src/protobuf/proto3_json.dart b/protobuf/lib/src/protobuf/proto3_json.dart index c7e0615bb..460498c73 100644 --- a/protobuf/lib/src/protobuf/proto3_json.dart +++ b/protobuf/lib/src/protobuf/proto3_json.dart @@ -452,15 +452,11 @@ void _mergeFromProto3Json( original.mergeFromMessage(parsedSubMessage); } } else { - final parsedValue = convertProto3JsonValue(value, fieldInfo); - if (parsedValue == null) { - // Unknown enum - if (!ignoreUnknownFields) { - throw context.parseException('Unknown enum value', value); - } - } else { - fieldSet._setFieldUnchecked(meta, fieldInfo, parsedValue); - } + fieldSet._setFieldUnchecked( + meta, + fieldInfo, + convertProto3JsonValue(value, fieldInfo), + ); } context.popIndex(); }); diff --git a/protoc_plugin/test/unknown_enums_test.dart b/protoc_plugin/test/unknown_enums_test.dart index c891f7865..0b5d5bcdb 100644 --- a/protoc_plugin/test/unknown_enums_test.dart +++ b/protoc_plugin/test/unknown_enums_test.dart @@ -3,8 +3,9 @@ // BSD-style license that can be found in the LICENSE file. import 'package:test/test.dart'; +import 'package:collection/collection.dart'; -import '../out/protos/enum_test.pb.dart'; +import 'gen/enum_test.pb.dart'; void main() { group('Enum parsing in maps, lists, messages', () { @@ -32,9 +33,12 @@ void main() { 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]); + expect(msg.enumField, A.X); // unknown value defaults the enum value + expect(msg.mapValueField.values.toList()..sortBy((e) => e.value), [ + A.X, + A.Y, + ]); + expect(msg.repeatedEnumField..sortBy((e) => e.value), [A.X, A.Y]); }); }); } From 6c1306ef054efc1c0f99737c953f12cd72aedd58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 6 Aug 2025 10:11:54 +0100 Subject: [PATCH 3/8] Revert "Revert non-repeated non-map unknown enum value behavior" This reverts commit c6cad9a7e110b275a0756ac6b939af3a94f23d05. --- protobuf/lib/src/protobuf/proto3_json.dart | 14 +++++++++----- protoc_plugin/test/unknown_enums_test.dart | 12 ++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/protobuf/lib/src/protobuf/proto3_json.dart b/protobuf/lib/src/protobuf/proto3_json.dart index 460498c73..c7e0615bb 100644 --- a/protobuf/lib/src/protobuf/proto3_json.dart +++ b/protobuf/lib/src/protobuf/proto3_json.dart @@ -452,11 +452,15 @@ void _mergeFromProto3Json( original.mergeFromMessage(parsedSubMessage); } } else { - fieldSet._setFieldUnchecked( - meta, - fieldInfo, - convertProto3JsonValue(value, fieldInfo), - ); + final parsedValue = convertProto3JsonValue(value, fieldInfo); + if (parsedValue == null) { + // Unknown enum + if (!ignoreUnknownFields) { + throw context.parseException('Unknown enum value', value); + } + } else { + fieldSet._setFieldUnchecked(meta, fieldInfo, parsedValue); + } } context.popIndex(); }); diff --git a/protoc_plugin/test/unknown_enums_test.dart b/protoc_plugin/test/unknown_enums_test.dart index 0b5d5bcdb..c891f7865 100644 --- a/protoc_plugin/test/unknown_enums_test.dart +++ b/protoc_plugin/test/unknown_enums_test.dart @@ -3,9 +3,8 @@ // BSD-style license that can be found in the LICENSE file. import 'package:test/test.dart'; -import 'package:collection/collection.dart'; -import 'gen/enum_test.pb.dart'; +import '../out/protos/enum_test.pb.dart'; void main() { group('Enum parsing in maps, lists, messages', () { @@ -33,12 +32,9 @@ void main() { final msg = Message(); msg.enumField = A.Y; msg.mergeFromProto3Json(json, ignoreUnknownFields: true); - expect(msg.enumField, A.X); // unknown value defaults the enum value - expect(msg.mapValueField.values.toList()..sortBy((e) => e.value), [ - A.X, - A.Y, - ]); - expect(msg.repeatedEnumField..sortBy((e) => e.value), [A.X, A.Y]); + expect(msg.enumField, A.Y); + expect(msg.mapValueField.values.toList(), [A.X, A.Y]); + expect(msg.repeatedEnumField, [A.X, A.Y]); }); }); } From c83f7967a35812d767bfce07604fd27b7d4f06df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 6 Aug 2025 10:23:05 +0100 Subject: [PATCH 4/8] Update tests --- protobuf/test/json_test.dart | 47 ++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/protobuf/test/json_test.dart b/protobuf/test/json_test.dart index beec88f2d..b20afe994 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 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 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); From 6b88703fc74cc88fec2f76d382ffe207330fff05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 6 Aug 2025 10:24:15 +0100 Subject: [PATCH 5/8] Fix proto path --- protoc_plugin/test/unknown_enums_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protoc_plugin/test/unknown_enums_test.dart b/protoc_plugin/test/unknown_enums_test.dart index c891f7865..a2394e40c 100644 --- a/protoc_plugin/test/unknown_enums_test.dart +++ b/protoc_plugin/test/unknown_enums_test.dart @@ -4,7 +4,7 @@ import 'package:test/test.dart'; -import '../out/protos/enum_test.pb.dart'; +import 'gen/enum_test.pb.dart'; void main() { group('Enum parsing in maps, lists, messages', () { From c9907880c493174a7b1b28fbab7d1031c3baf2c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 6 Aug 2025 10:25:00 +0100 Subject: [PATCH 6/8] Update test names --- protobuf/test/json_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/protobuf/test/json_test.dart b/protobuf/test/json_test.dart index b20afe994..5688f0cee 100644 --- a/protobuf/test/json_test.dart +++ b/protobuf/test/json_test.dart @@ -14,7 +14,7 @@ import 'package:test/test.dart'; import 'mock_util.dart' show T, mockEnumValues; void main() { - test('mergeFromProto3Json unknown fields with names', () { + test('mergeFromProto3Json unknown enum fields with names', () { final example = T(); // No enum value specified. @@ -57,7 +57,7 @@ void main() { expect(example.hasEnm, isTrue); }); - test('mergeFromProto3Json unknown fields with indices', () { + test('mergeFromProto3Json unknown enum fields with indices', () { // Similar to above, but with indices. final example = T(); From ed5da7c9b48f2253f3918788705a0f0dd1d3e949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 6 Aug 2025 10:45:06 +0100 Subject: [PATCH 7/8] Update new file copyrights --- protoc_plugin/test/protos/enum_test.proto | 2 +- protoc_plugin/test/unknown_enums_test.dart | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/protoc_plugin/test/protos/enum_test.proto b/protoc_plugin/test/protos/enum_test.proto index f80dc6bc9..d9bf9219a 100644 --- a/protoc_plugin/test/protos/enum_test.proto +++ b/protoc_plugin/test/protos/enum_test.proto @@ -1,4 +1,4 @@ -// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// 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. diff --git a/protoc_plugin/test/unknown_enums_test.dart b/protoc_plugin/test/unknown_enums_test.dart index a2394e40c..8961f26fa 100644 --- a/protoc_plugin/test/unknown_enums_test.dart +++ b/protoc_plugin/test/unknown_enums_test.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// 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. @@ -7,6 +7,7 @@ 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 = { From 06b5cee9daa6bf78f2c8f1283e4d55f3a1f62fdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Fri, 15 Aug 2025 08:23:59 +0100 Subject: [PATCH 8/8] Update changelog --- protobuf/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index 83335f373..2ea180534 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