Fix unknown JSON handling (#1058)

- Fix hash code and equality handling with unknown JSON data. (cl/613592094)

- Fix converting unknown JSON data to Dart when deserializing, and to JS when
  serializing. (cl/812691555)

- Make unknown JSON data type `Object?` instead of `dynamic` to prevent
  accidentally making dynamic calls. (cl/812691555)

- Also fix a sync error in b761358 where the
  `_unknownJsonData = null` line was added to  `_ensureUnknownFields` instead
  of `_clear`.

Author: osa1

protobuf/CHANGELOG.md

@@ -7,12 +7,16 @@
 * Generalize argument type of `PbList.from` from `List<T>` to `Iterable<T>`.
   ([#1054])
 * Fix clearing oneof fields with `GeneratedMessage.clear`. ([#1057])
+* Fix unknown JSON handling when using `GeneratedMessage` methods
+  `mergeFromJson`, `mergeFromJsonMap`, `writeToJson`, `writeToJsonMap`.
+  ([#1058])
 
 [#742]: https://github.com/google/protobuf.dart/pull/742
 [#853]: https://github.com/google/protobuf.dart/pull/853
 [#1047]: https://github.com/google/protobuf.dart/pull/1047
 [#1054]: https://github.com/google/protobuf.dart/pull/1054
 [#1057]: https://github.com/google/protobuf.dart/pull/1057
+[#1058]: https://github.com/google/protobuf.dart/pull/1058
 
 ## 4.2.0
 

protobuf/lib/src/protobuf/field_set.dart

@@ -40,7 +40,7 @@ class FieldSet {
   UnknownFieldSet? _unknownFields;
 
   /// Contains unknown data for messages deserialized from json.
-  Map<String, dynamic>? _unknownJsonData;
+  Map<String, Object?>? _unknownJsonData;
 
   /// Encodes whether `this` has been frozen, and if so, also memoizes the
   /// hash code.
@@ -114,7 +114,6 @@ class FieldSet {
       if (_isReadOnly) return UnknownFieldSet.emptyUnknownFieldSet;
       _unknownFields = UnknownFieldSet();
     }
-    _unknownJsonData = null;
     return _unknownFields!;
   }
 
@@ -506,6 +505,7 @@ class FieldSet {
     if (_unknownFields != null) {
       _unknownFields!.clear();
     }
+    _unknownJsonData = null;
     if (_values.isNotEmpty) _values.fillRange(0, _values.length, null);
     _extensions?._clearValues();
     _oneofCases?.clear();
@@ -540,7 +540,17 @@ class FieldSet {
       if (_unknownFields != o._unknownFields) return false;
     }
 
-    // Ignore _unknownJsonData to preserve existing equality behavior.
+    if (_unknownJsonData != null || o._unknownJsonData != null) {
+      if ((_unknownJsonData == null) != (o._unknownJsonData == null)) {
+        return false;
+      }
+      if (!DeepCollectionEquality().equals(
+        _unknownJsonData,
+        o._unknownJsonData,
+      )) {
+        return false;
+      }
+    }
 
     return true;
   }
@@ -608,7 +618,12 @@ class FieldSet {
     // Hash with unknown fields.
     hash = HashUtils.combine(hash, _unknownFields?.hashCode ?? 0);
 
-    // Ignore _unknownJsonData to preserve existing hashing behavior.
+    if (_unknownJsonData != null) {
+      hash = HashUtils.combine(
+        hash,
+        DeepCollectionEquality().hash(_unknownJsonData),
+      );
+    }
 
     if (_isReadOnly) {
       _frozenState = hash;
@@ -983,7 +998,7 @@ extension FieldSetInternalExtension on FieldSet {
   List get values => _values;
   ExtensionFieldSet? get extensions => _extensions;
   UnknownFieldSet? get unknownFields => _unknownFields;
-  Map<String, dynamic>? get unknownJsonData => _unknownJsonData;
+  Map<String, Object?>? get unknownJsonData => _unknownJsonData;
   set unknownJsonData(Map<String, dynamic>? value) => _unknownJsonData = value;
   BuilderInfo get meta => _meta;
   GeneratedMessage? get message => _message;

protobuf/lib/src/protobuf/internal.dart

@@ -12,6 +12,7 @@ import 'dart:convert' show Utf8Decoder, Utf8Encoder, base64Decode, base64Encode;
 import 'dart:math' as math;
 import 'dart:typed_data' show ByteData, Endian, Uint8List;
 
+import 'package:collection/collection.dart' show DeepCollectionEquality;
 import 'package:fixnum/fixnum.dart' show Int64;
 import 'package:meta/meta.dart' show UseResult;
 

protobuf/lib/src/protobuf/json/json_web.dart

@@ -174,7 +174,7 @@ JSObject _writeToRawJs(FieldSet fs) {
   final unknownJsonData = fs.unknownJsonData;
   if (unknownJsonData != null) {
     unknownJsonData.forEach((key, value) {
-      result.setProperty(key.toJS, value);
+      result.setProperty(key.toJS, value.jsify());
     });
   }
   return result;
@@ -215,7 +215,8 @@ void _mergeFromRawJsMap(
     if (fi == null) {
       fi = registry?.getExtension(fs.messageName, int.parse(key));
       if (fi == null) {
-        (fs.unknownJsonData ??= {})[key] = json.getProperty<JSAny>(jsKey);
+        (fs.unknownJsonData ??= {})[key] =
+            json.getProperty<JSAny>(jsKey).dartify();
         continue;
       }
     }

protobuf/test/json_test.dart

@@ -95,7 +95,7 @@ void main() {
   });
 
   test('testWriteFrozenToJson', () {
-    final frozen = makeTestJson().clone()..freeze();
+    final frozen = makeTestJson()..freeze();
     final json = frozen.writeToJson();
     checkJsonMap(jsonDecode(json));
   });
@@ -144,6 +144,47 @@ void main() {
     final t = T()..mergeFromJsonMap(m);
     checkJsonMap(t.writeToJsonMap(), unknownFields: {'9999': 'world'});
   });
+
+  test('testJspbLite2WithUnknown', () {
+    final m = makeTestJson().writeToJson();
+    final decoded = jsonDecode(m);
+    decoded['9999'] = 'world';
+    final encoded = jsonEncode(decoded);
+    final t = T()..mergeFromJson(encoded);
+    checkJsonMap(t.writeToJsonMap(), unknownFields: {'9999': 'world'});
+  });
+
+  test('mergeFromJspbLite2 unknown data should be converted to Dart', () {
+    // Testing here is a bit indirect (via `toDebugString`) because
+    // `unknownJsonData` is not exposed to the users.
+    final decoded = {
+      '9999': {
+        '1': ['a', 'b', 'c'],
+      },
+    };
+    final encoded = jsonEncode(decoded);
+    final t = T()..mergeFromJson(encoded);
+    // Without converting JS data to Dart we get `[object Object]` here for the
+    // field value.
+    expect(t.toDebugString(), '{9999: {1: [a, b, c]}}');
+  });
+
+  test('writeToJspbLite unknown data should be converted to JS', () {
+    final m = makeTestJson().writeToJson();
+    final decoded = jsonDecode(m);
+    decoded['9999'] = {
+      '1': ['a', 'b', 'c'],
+    };
+    final encoded = jsonEncode(decoded);
+    final t = T()..mergeFromJson(encoded);
+    // Without converting unknown data (converted to Dart when decoding) to JS,
+    // the unknown field values in the output get weird as JS representation of
+    // Dart data are serialized directly by the browser's `JSON.stringify`.
+    expect(
+      t.writeToJson(),
+      '{"1":123,"2":"hello","4":[1,2,3],"9999":{"1":["a","b","c"]}}',
+    );
+  });
 }
 
 T makeTestJson() =>