google / json_serializable.dart

Generates utilities to aid in serializing to/from JSON.
https://pub.dev/packages/json_serializable
BSD 3-Clause "New" or "Revised" License
1.55k stars 394 forks source link

Bug report - @JsonKey(includeFromJson: false) malfunctions #1334

Open APPXOTICA opened 1 year ago

APPXOTICA commented 1 year ago

Here's a test class

@JsonSerializable()
class TestClass {
  TestClass({
    this.a,
  });

  @JsonKey(includeFromJson: false)
  String? a;

  factory TestClass.fromJson(Map<String, dynamic> json) => _$TestClassFromJson(json);
  Map<String, dynamic> toJson() => _$TestClassToJson(this);
}

Generated code.

TestClass _$TestClassFromJson(Map<String, dynamic> json) => TestClass();

Map<String, dynamic> _$TestClassToJson(TestClass instance) =>
    <String, dynamic>{};

TestClassToJson should include String a but it doesn't. It excludes String a from TestClassFromJson and TestClassToJson both.

json_serializable: ^6.7.0 json_annotation: ^4.8.1

CicadaCinema commented 1 year ago

Note the docstring:

https://github.com/google/json_serializable.dart/blob/2185e8b80d8d0c12e2adbf897d920b6f5725cded/json_annotation/lib/src/json_key.dart#L91-L93

The current behaviour can be summarised by these test cases: ToJsonNullFromJsonFalsePublic and ToJsonFalseFromJsonNullPublic

Summary:

I take round-tripped to-from JSON to mean a round trip from TestClass to Map<String, dynamic> and back to TestClass, however the following docstrings seem to imply that we must implement toJson if and only if we implement fromJson:

https://github.com/google/json_serializable.dart/blob/2185e8b80d8d0c12e2adbf897d920b6f5725cded/json_annotation/lib/src/json_key.dart#L41-L44

https://github.com/google/json_serializable.dart/blob/2185e8b80d8d0c12e2adbf897d920b6f5725cded/json_annotation/lib/src/json_key.dart#L137-L140

To me it sounds like we only care about the to-from round-trip and that line 42 is a mistake (it should be replaced by the contents of line 138).

Maybe this is an incorrect assumption, given that these docstrings have survived this long without modification.

In either case, it seems that the current behaviour can break if we attempt a to-from round trip, as described above:

@JsonSerializable()
class TestClass {
  TestClass({
    required this.a,
  });

  @JsonKey(includeToJson: false)
  String a;

  factory TestClass.fromJson(Map<String, dynamic> json) =>
      _$TestClassFromJson(json);
  Map<String, dynamic> toJson() => _$TestClassToJson(this);
}

void main() {
  final json = TestClass(a: 'aaa').toJson();
  final test = TestClass.fromJson(json);
}

Generated code:

TestClass _$TestClassFromJson(Map<String, dynamic> json) => TestClass(
      a: json['a'] as String,
    );

Map<String, dynamic> _$TestClassToJson(TestClass instance) =>
    <String, dynamic>{};
$ dart run lib/testme.dart 
Unhandled exception:
type 'Null' is not a subtype of type 'String' in type cast
#0      _$TestClassFromJson (package:testme/testme.g.dart:10:20)
#1      new TestClass.fromJson (package:testme/testme.dart:14:7)
#2      main (package:testme/testme.dart:20:26)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:296:19)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

Dependency versions:

json_annotation 4.8.1
json_serializable 6.7.1
CicadaCinema commented 1 year ago

If my assumption is correct, I propose we correct the docs to only refer to a to-from round-trip, and fix the current behaviour when attempting this round trip.

Also, I see no reason to add to the docstrings for includeFromJson and includeToJson, adding specifically what will happen if the class is private or if it can't be round-tripped.

References:

https://github.com/google/json_serializable.dart/pull/1256

https://github.com/google/json_serializable.dart/blob/2185e8b80d8d0c12e2adbf897d920b6f5725cded/json_serializable/lib/src/json_key_utils.dart#L260

gmaggio commented 9 months ago

You may have to specify explicitly includeToJson: true, too

  @JsonKey(
    includeFromJson: false,
    includeToJson: true,
  )
  String? a;
edlman commented 3 months ago

I encountered the same (for me strange) behavior. Why is field excluded from "toJson" method when I set "includeFromJson: false"? I don't see any reason for this.

So "includeToJson: true" must be explicitly set when using "includeFromJson: false"