From a5190d2d0d879c2e60b6b1e258c5517118a02358 Mon Sep 17 00:00:00 2001 From: Kevin Moore Date: Mon, 17 Oct 2022 10:21:16 -0700 Subject: [PATCH] Improve encoding of values to handle null cases Specifically with enums and converter functions when `includeIfNull` is `false`. Enum values with `null` as an output where not properly wrapped. This is now fixed. Fixes https://github.com/google/json_serializable.dart/issues/1202 Also 'unwrap' the cases where conversion functions never return `null`. --- json_serializable/CHANGELOG.md | 5 + json_serializable/lib/src/encoder_helper.dart | 39 +++++--- json_serializable/lib/src/enum_utils.dart | 38 ++++++-- .../lib/src/type_helper_ctx.dart | 5 +- .../lib/src/type_helpers/convert_helper.dart | 3 +- .../lib/src/type_helpers/enum_helper.dart | 3 +- .../type_helpers/json_converter_helper.dart | 18 ++++ json_serializable/pubspec.yaml | 2 +- .../test/integration/converter_examples.dart | 97 +++++++++++++++++++ .../integration/converter_examples.g.dart | 60 ++++++++++++ .../test/integration/integration_test.dart | 56 +++++++++++ .../kitchen_sink.g_exclude_null.g.dart | 31 ++---- .../test/src/to_from_json_test_input.dart | 6 +- 13 files changed, 313 insertions(+), 50 deletions(-) create mode 100644 json_serializable/test/integration/converter_examples.dart create mode 100644 json_serializable/test/integration/converter_examples.g.dart diff --git a/json_serializable/CHANGELOG.md b/json_serializable/CHANGELOG.md index 9cda69759..0bf05ed6d 100644 --- a/json_serializable/CHANGELOG.md +++ b/json_serializable/CHANGELOG.md @@ -1,3 +1,8 @@ +## 6.5.2 + +- Better handling of `null` when encoding `enum` values or values with + conversions. + ## 6.5.1 - Fixed `BigInt`, `DateTime`, and `Uri` support for `JsonKey.defaultValue` with diff --git a/json_serializable/lib/src/encoder_helper.dart b/json_serializable/lib/src/encoder_helper.dart index 9bb045852..c21c0380a 100644 --- a/json_serializable/lib/src/encoder_helper.dart +++ b/json_serializable/lib/src/encoder_helper.dart @@ -4,10 +4,10 @@ import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/nullability_suffix.dart'; -import 'package:json_annotation/json_annotation.dart'; import 'package:source_helper/source_helper.dart'; import 'constants.dart'; +import 'enum_utils.dart'; import 'helper_core.dart'; import 'type_helpers/generic_factory_helper.dart'; import 'type_helpers/json_converter_helper.dart'; @@ -163,19 +163,32 @@ abstract class EncodeHelper implements HelperCore { bool _writeJsonValueNaive(FieldElement field) { final jsonKey = jsonKeyFor(field); - return jsonKey.includeIfNull || - (!field.type.isNullableType && !_fieldHasCustomEncoder(field)); - } + if (jsonKey.includeIfNull) { + return true; + } - /// Returns `true` if [field] has a user-defined encoder. - /// - /// This can be either a `toJson` function in [JsonKey] or a [JsonConverter] - /// annotation. - bool _fieldHasCustomEncoder(FieldElement field) { final helperContext = getHelperContext(field); - return helperContext.serializeConvertData != null || - const JsonConverterHelper() - .serialize(field.type, 'test', helperContext) != - null; + + final serializeConvertData = helperContext.serializeConvertData; + if (serializeConvertData != null) { + return !serializeConvertData.returnType.isNullableType; + } + + final nullableEncodeConverter = + hasConverterNullEncode(field.type, helperContext); + + if (nullableEncodeConverter != null) { + return !nullableEncodeConverter; + } + + // We can consider enums as kinda like having custom converters + // same rules apply. If `null` is in the set of encoded values, we + // should not write naive + final enumWithNullValue = enumFieldWithNullInEncodeMap(field.type); + if (enumWithNullValue != null) { + return !enumWithNullValue; + } + + return !field.type.isNullableType; } } diff --git a/json_serializable/lib/src/enum_utils.dart b/json_serializable/lib/src/enum_utils.dart index 61ed15721..b293aba4c 100644 --- a/json_serializable/lib/src/enum_utils.dart +++ b/json_serializable/lib/src/enum_utils.dart @@ -14,9 +14,38 @@ import 'utils.dart'; String constMapName(DartType targetType) => '_\$${targetType.element2!.name}EnumMap'; +/// If [targetType] is not an enum, return `null`. +/// +/// Otherwise, returns `true` if one of the encoded values of the enum is +/// `null`. +bool? enumFieldWithNullInEncodeMap(DartType targetType) { + final enumMap = _enumMap(targetType); + + if (enumMap == null) return null; + + return enumMap.values.contains(null); +} + String? enumValueMapFromType( DartType targetType, { bool nullWithNoAnnotation = false, +}) { + final enumMap = + _enumMap(targetType, nullWithNoAnnotation: nullWithNoAnnotation); + + if (enumMap == null) return null; + + final items = enumMap.entries + .map((e) => ' ${targetType.element2!.name}.${e.key.name}: ' + '${jsonLiteralAsDart(e.value)},') + .join(); + + return 'const ${constMapName(targetType)} = {\n$items\n};'; +} + +Map? _enumMap( + DartType targetType, { + bool nullWithNoAnnotation = false, }) { final annotation = _jsonEnumChecker.firstAnnotationOf(targetType.element2!); final jsonEnum = _fromAnnotation(annotation); @@ -27,7 +56,7 @@ String? enumValueMapFromType( return null; } - final enumMap = { + return { for (var field in enumFields) field: _generateEntry( field: field, @@ -35,13 +64,6 @@ String? enumValueMapFromType( targetType: targetType, ), }; - - final items = enumMap.entries - .map((e) => ' ${targetType.element2!.name}.${e.key.name}: ' - '${jsonLiteralAsDart(e.value)},') - .join(); - - return 'const ${constMapName(targetType)} = {\n$items\n};'; } Object? _generateEntry({ diff --git a/json_serializable/lib/src/type_helper_ctx.dart b/json_serializable/lib/src/type_helper_ctx.dart index 9928e459b..b49008504 100644 --- a/json_serializable/lib/src/type_helper_ctx.dart +++ b/json_serializable/lib/src/type_helper_ctx.dart @@ -135,13 +135,12 @@ ConvertData? _convertData(DartObject obj, FieldElement element, bool isFrom) { 'positional parameter.'); } + final returnType = executableElement.returnType; final argType = executableElement.parameters.first.type; if (isFrom) { final hasDefaultValue = !jsonKeyAnnotation(element).read('defaultValue').isNull; - final returnType = executableElement.returnType; - if (returnType is TypeParameterType) { // We keep things simple in this case. We rely on inferred type arguments // to the `fromJson` function. @@ -176,5 +175,5 @@ ConvertData? _convertData(DartObject obj, FieldElement element, bool isFrom) { } } - return ConvertData(executableElement.qualifiedName, argType); + return ConvertData(executableElement.qualifiedName, argType, returnType); } diff --git a/json_serializable/lib/src/type_helpers/convert_helper.dart b/json_serializable/lib/src/type_helpers/convert_helper.dart index 051f2cb9b..1918fd42a 100644 --- a/json_serializable/lib/src/type_helpers/convert_helper.dart +++ b/json_serializable/lib/src/type_helpers/convert_helper.dart @@ -14,8 +14,9 @@ import '../type_helper.dart'; class ConvertData { final String name; final DartType paramType; + final DartType returnType; - ConvertData(this.name, this.paramType); + ConvertData(this.name, this.paramType, this.returnType); } abstract class TypeHelperContextWithConvert extends TypeHelperContext { diff --git a/json_serializable/lib/src/type_helpers/enum_helper.dart b/json_serializable/lib/src/type_helpers/enum_helper.dart index 2d8e91765..ce7641fdf 100644 --- a/json_serializable/lib/src/type_helpers/enum_helper.dart +++ b/json_serializable/lib/src/type_helpers/enum_helper.dart @@ -29,7 +29,8 @@ class EnumHelper extends TypeHelper { context.addMember(memberContent); - if (targetType.isNullableType) { + if (targetType.isNullableType || + enumFieldWithNullInEncodeMap(targetType) == true) { return '${constMapName(targetType)}[$expression]'; } else { return '${constMapName(targetType)}[$expression]!'; diff --git a/json_serializable/lib/src/type_helpers/json_converter_helper.dart b/json_serializable/lib/src/type_helpers/json_converter_helper.dart index 37523f42a..2b57e1e1d 100644 --- a/json_serializable/lib/src/type_helpers/json_converter_helper.dart +++ b/json_serializable/lib/src/type_helpers/json_converter_helper.dart @@ -143,6 +143,24 @@ class _JsonConvertData { accessor.isEmpty ? '' : '.$accessor'; } +/// If there is no converter for the params, return `null`. +/// +/// Otherwise, returns `true` if the converter has a null return value. +/// +/// Used to make sure we create a smart encoding function. +bool? hasConverterNullEncode( + DartType targetType, + TypeHelperContextWithConfig ctx, +) { + final data = _typeConverter(targetType, ctx); + + if (data == null) { + return null; + } + + return data.jsonType.isNullableType; +} + _JsonConvertData? _typeConverter( DartType targetType, TypeHelperContextWithConfig ctx, diff --git a/json_serializable/pubspec.yaml b/json_serializable/pubspec.yaml index 24c3204d3..7112acfa1 100644 --- a/json_serializable/pubspec.yaml +++ b/json_serializable/pubspec.yaml @@ -1,5 +1,5 @@ name: json_serializable -version: 6.5.1 +version: 6.5.2 description: >- Automatically generate code for converting to and from JSON by annotating Dart classes. diff --git a/json_serializable/test/integration/converter_examples.dart b/json_serializable/test/integration/converter_examples.dart new file mode 100644 index 000000000..736907cd3 --- /dev/null +++ b/json_serializable/test/integration/converter_examples.dart @@ -0,0 +1,97 @@ +import 'dart:convert'; + +import 'package:json_annotation/json_annotation.dart'; + +part 'converter_examples.g.dart'; + +@JsonEnum(valueField: 'value') +enum Issue1202RegressionEnum { + normalValue(42), + nullValue(null); + + const Issue1202RegressionEnum(this.value); + + final int? value; +} + +@JsonSerializable(includeIfNull: false) +class Issue1202RegressionClass { + @JsonKey(fromJson: _fromJson, toJson: _toJson) + final int valueWithFunctions; + + @_Issue1202RegressionNotNullConverter() + final int notNullableValueWithConverter; + + final Issue1202RegressionEnum value; + final int? normalNullableValue; + + @_Issue1202RegressionConverter() + final int notNullableValueWithNullableConverter; + + @JsonKey(fromJson: _fromJsonNullable, toJson: _toJsonNullable) + final int valueWithNullableFunctions; + + Issue1202RegressionClass({ + required this.value, + required this.normalNullableValue, + required this.notNullableValueWithNullableConverter, + required this.notNullableValueWithConverter, + required this.valueWithFunctions, + required this.valueWithNullableFunctions, + }); + + factory Issue1202RegressionClass.fromJson(Map json) => + _$Issue1202RegressionClassFromJson(json); + + Map toJson() => _$Issue1202RegressionClassToJson(this); + + @override + bool operator ==(Object other) => + other is Issue1202RegressionClass && + jsonEncode(other) == jsonEncode(this); + + @override + int get hashCode => jsonEncode(this).hashCode; + + static int _fromJsonNullable(String? json) { + if (json == null) return _default; + return int.parse(json); + } + + static String? _toJsonNullable(int object) { + if (object == _default) return null; + return object.toString(); + } + + static int _fromJson(String json) => int.parse(json); + + static String _toJson(int object) => object.toString(); +} + +const _default = 42; + +class _Issue1202RegressionConverter extends JsonConverter { + const _Issue1202RegressionConverter(); + + @override + int fromJson(String? json) { + if (json == null) return _default; + return int.parse(json); + } + + @override + String? toJson(int object) { + if (object == _default) return null; + return object.toString(); + } +} + +class _Issue1202RegressionNotNullConverter extends JsonConverter { + const _Issue1202RegressionNotNullConverter(); + + @override + int fromJson(String json) => int.parse(json); + + @override + String toJson(int object) => object.toString(); +} diff --git a/json_serializable/test/integration/converter_examples.g.dart b/json_serializable/test/integration/converter_examples.g.dart new file mode 100644 index 000000000..098bc8506 --- /dev/null +++ b/json_serializable/test/integration/converter_examples.g.dart @@ -0,0 +1,60 @@ +// GENERATED CODE - DO NOT MODIFY BY HAND + +// ignore_for_file: lines_longer_than_80_chars, text_direction_code_point_in_literal + +part of 'converter_examples.dart'; + +// ************************************************************************** +// JsonSerializableGenerator +// ************************************************************************** + +Issue1202RegressionClass _$Issue1202RegressionClassFromJson( + Map json) => + Issue1202RegressionClass( + value: $enumDecode(_$Issue1202RegressionEnumEnumMap, json['value']), + normalNullableValue: json['normalNullableValue'] as int?, + notNullableValueWithNullableConverter: + const _Issue1202RegressionConverter().fromJson( + json['notNullableValueWithNullableConverter'] as String?), + notNullableValueWithConverter: + const _Issue1202RegressionNotNullConverter() + .fromJson(json['notNullableValueWithConverter'] as String), + valueWithFunctions: Issue1202RegressionClass._fromJson( + json['valueWithFunctions'] as String), + valueWithNullableFunctions: Issue1202RegressionClass._fromJsonNullable( + json['valueWithNullableFunctions'] as String?), + ); + +Map _$Issue1202RegressionClassToJson( + Issue1202RegressionClass instance) { + final val = { + 'valueWithFunctions': + Issue1202RegressionClass._toJson(instance.valueWithFunctions), + 'notNullableValueWithConverter': + const _Issue1202RegressionNotNullConverter() + .toJson(instance.notNullableValueWithConverter), + }; + + void writeNotNull(String key, dynamic value) { + if (value != null) { + val[key] = value; + } + } + + writeNotNull('value', _$Issue1202RegressionEnumEnumMap[instance.value]); + writeNotNull('normalNullableValue', instance.normalNullableValue); + writeNotNull( + 'notNullableValueWithNullableConverter', + const _Issue1202RegressionConverter() + .toJson(instance.notNullableValueWithNullableConverter)); + writeNotNull( + 'valueWithNullableFunctions', + Issue1202RegressionClass._toJsonNullable( + instance.valueWithNullableFunctions)); + return val; +} + +const _$Issue1202RegressionEnumEnumMap = { + Issue1202RegressionEnum.normalValue: 42, + Issue1202RegressionEnum.nullValue: null, +}; diff --git a/json_serializable/test/integration/integration_test.dart b/json_serializable/test/integration/integration_test.dart index ca6266dab..27fdf99ef 100644 --- a/json_serializable/test/integration/integration_test.dart +++ b/json_serializable/test/integration/integration_test.dart @@ -6,6 +6,7 @@ import 'package:json_annotation/json_annotation.dart'; import 'package:test/test.dart'; import '../test_utils.dart'; +import 'converter_examples.dart'; import 'field_map_example.dart'; import 'json_enum_example.dart'; import 'json_test_common.dart' show Category, Platform, StatusCode; @@ -358,4 +359,59 @@ void main() { {'fullName': 'full-name'}, ); }); + + group('classes with converters', () { + Issue1202RegressionClass roundTripIssue1202RegressionClass(int value) { + final instance = Issue1202RegressionClass( + normalNullableValue: value, + notNullableValueWithConverter: value, + notNullableValueWithNullableConverter: value, + value: Issue1202RegressionEnum.normalValue, + valueWithFunctions: value, + valueWithNullableFunctions: value, + ); + return roundTripObject(instance, Issue1202RegressionClass.fromJson); + } + + test('With default values', () { + final thing = roundTripIssue1202RegressionClass(42); + + expect(thing.toJson(), { + 'valueWithFunctions': '42', + 'notNullableValueWithConverter': '42', + 'value': 42, + 'normalNullableValue': 42, + }); + }); + + test('With non-default values', () { + final thing = roundTripIssue1202RegressionClass(43); + + expect(thing.toJson(), { + 'valueWithFunctions': '43', + 'notNullableValueWithConverter': '43', + 'value': 42, + 'normalNullableValue': 43, + 'notNullableValueWithNullableConverter': '43', + 'valueWithNullableFunctions': '43', + }); + }); + + test('enum with null value', () { + final instance = Issue1202RegressionClass( + normalNullableValue: 42, + notNullableValueWithConverter: 42, + notNullableValueWithNullableConverter: 42, + value: Issue1202RegressionEnum.nullValue, + valueWithFunctions: 42, + valueWithNullableFunctions: 42, + ); + + expect(instance.toJson(), { + 'valueWithFunctions': '42', + 'notNullableValueWithConverter': '42', + 'normalNullableValue': 42, + }); + }); + }); } diff --git a/json_serializable/test/kitchen_sink/kitchen_sink.g_exclude_null.g.dart b/json_serializable/test/kitchen_sink/kitchen_sink.g_exclude_null.g.dart index 017e40a6a..110817593 100644 --- a/json_serializable/test/kitchen_sink/kitchen_sink.g_exclude_null.g.dart +++ b/json_serializable/test/kitchen_sink/kitchen_sink.g_exclude_null.g.dart @@ -190,13 +190,11 @@ Map _$JsonConverterTestClassToJson( val['durationList'] = instance.durationList .map(const DurationMillisecondConverter().toJson) .toList(); - writeNotNull('bigInt', const BigIntStringConverter().toJson(instance.bigInt)); + val['bigInt'] = const BigIntStringConverter().toJson(instance.bigInt); val['bigIntMap'] = instance.bigIntMap .map((k, e) => MapEntry(k, const BigIntStringConverter().toJson(e))); - writeNotNull( - 'nullableBigInt', - _$JsonConverterToJson( - instance.nullableBigInt, const BigIntStringConverter().toJson)); + val['nullableBigInt'] = _$JsonConverterToJson( + instance.nullableBigInt, const BigIntStringConverter().toJson); val['nullableBigIntMap'] = instance.nullableBigIntMap.map((k, e) => MapEntry( k, _$JsonConverterToJson( @@ -247,19 +245,10 @@ JsonConverterGeneric _$JsonConverterGenericFromJson( ); Map _$JsonConverterGenericToJson( - JsonConverterGeneric instance) { - final val = {}; - - void writeNotNull(String key, dynamic value) { - if (value != null) { - val[key] = value; - } - } - - writeNotNull('item', GenericConverter().toJson(instance.item)); - val['itemList'] = - instance.itemList.map(GenericConverter().toJson).toList(); - val['itemMap'] = instance.itemMap - .map((k, e) => MapEntry(k, GenericConverter().toJson(e))); - return val; -} + JsonConverterGeneric instance) => + { + 'item': GenericConverter().toJson(instance.item), + 'itemList': instance.itemList.map(GenericConverter().toJson).toList(), + 'itemMap': instance.itemMap + .map((k, e) => MapEntry(k, GenericConverter().toJson(e))), + }; diff --git a/json_serializable/test/src/to_from_json_test_input.dart b/json_serializable/test/src/to_from_json_test_input.dart index dafd680ea..11ce9f447 100644 --- a/json_serializable/test/src/to_from_json_test_input.dart +++ b/json_serializable/test/src/to_from_json_test_input.dart @@ -170,6 +170,8 @@ class TypedConvertMethods { late String field; } +String? _toStringNullOnEmpty(String input) => input.isEmpty ? null : input; + @ShouldGenerate( r''' Map _$ToJsonNullableFalseIncludeIfNullFalseToJson( @@ -182,14 +184,14 @@ Map _$ToJsonNullableFalseIncludeIfNullFalseToJson( } } - writeNotNull('field', _toString(instance.field)); + writeNotNull('field', _toStringNullOnEmpty(instance.field)); return val; } ''', ) @JsonSerializable(createFactory: false) class ToJsonNullableFalseIncludeIfNullFalse { - @JsonKey(toJson: _toString, includeIfNull: false) + @JsonKey(toJson: _toStringNullOnEmpty, includeIfNull: false) late String field; }