Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try to remove non-nullable converter if duplicate found for nullable targetType #1343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Try to remove non-nullable converter if duplicate found for nullable targetType #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87a2a72
select appropriate converter for nullable fieldtype
julek-kal Aug 6, 2023
d3bcda6
remove unused named constructor
julek-kal Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions json_serializable/lib/src/type_helpers/json_converter_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ _JsonConvertData? _typeConverterFrom(
return null;
}

if (targetType.isNullableType && matchingAnnotations.length == 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if both annotations match? What if there are more than two?

I see how this would be nice for the case you're hitting – but I worry that it has too many edge cases.

Would it be better to find the set of "better" converters in the set – and see if that set of "better" converters has a length of 1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is behaving better than currently and don't break other things.
It's exactly trying to find a "better" converter by its field type. When there are more than two converters or both matches it would throw "Found more than one matching converter for $targetTypeCode."

It's fixing 1339, so I'm able to have a JsonSerializable with converters and use it on a class that contains both nullable and non-nullable of the same type.

What other "algorithm" do you suggest to find better converters? What is the factor of "betterness" of the converter? Maybe you want to suggest to check also a jsonType. Ok, maybe it's a good idea, but I think that the level of complexity will be increased noticeably

Copy link

@t-beckmann t-beckmann Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe implement it as

Suggested change
if (targetType.isNullableType && matchingAnnotations.length == 2) {
if (targetType.isNullableType && matchingAnnotations.length > 1) {
matchingAnnotations
.removeWhere((element) => !element.fieldType.isNullableType);

so that it is not restricted to exactly 2 matches.

Afterwards you could favor an exact type match if the result is not unique yet. Finally, instead of throwing one could pick the first match, assuming the results are in order of declaration.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-beckmann thanks for your suggestion. @kevmoo what do you think about above solution?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any update for this PR？

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still waiting for a response from @kevmoo with his opinion about t-beckmann's implementation and his look at the above topic. Cause I think that he is a decisional person, about that PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whatever reason, the current workaround is to define converters both as annotations and as list members, however, it must be done separately for nullable and non-nullable types, like so:

@JsonSerializable(
  converters: [
    ExampleSerializationHandlerString(),
    ExampleSerializationHandlerBool(),
    ExampleSerializationHandlerInt(),
    ExampleSerializationHandlerDouble(),
    ExampleSerializationHandlerMap(),
  ],
)
@ExampleSerializationHandlerStringNullable()
@ExampleSerializationHandlerBoolNullable()
@ExampleSerializationHandlerIntNullable()
@ExampleSerializationHandlerDoubleNullable()
@ExampleSerializationHandlerMapNullable()

This is with the latest json_serializable (6.8.0) and json_annotation (4.9.0) versions at the time of this writing.

final indexOfNonNullableConverter = matchingAnnotations
.indexWhere((element) => !element.fieldType.isNullableType);
if (indexOfNonNullableConverter >= 0) {
matchingAnnotations.removeAt(indexOfNonNullableConverter);
}
}

if (matchingAnnotations.length > 1) {
final targetTypeCode = typeToCode(targetType);
throw InvalidGenerationSourceError(
Expand Down
1 change: 1 addition & 0 deletions json_serializable/test/json_serializable_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ const _expectedAnnotatedTests = {
'JsonConvertOnField',
'JsonConverterCtorParams',
'JsonConverterDuplicateAnnotations',
'JsonConverterIssue1339',
'JsonConverterNamedCtor',
'JsonConverterNullableToNonNullable',
'JsonConverterOnGetter',
Expand Down
43 changes: 43 additions & 0 deletions json_serializable/test/src/json_converter_test_input.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,38 @@ class JsonConverterDuplicateAnnotations {
late Duration value;
}

@ShouldGenerate(r'''
JsonConverterIssue1339 _$JsonConverterIssue1339FromJson(
Map<String, dynamic> json) =>
JsonConverterIssue1339()
..value =
const _DurationMillisecondConverter().fromJson(json['value'] as int)
..nullableValue = _$JsonConverterFromJson<int, Duration?>(
json['nullableValue'],
const _DurationMillisecondNullConverter().fromJson);

Map<String, dynamic> _$JsonConverterIssue1339ToJson(
JsonConverterIssue1339 instance) =>
<String, dynamic>{
'value': const _DurationMillisecondConverter().toJson(instance.value),
'nullableValue': const _DurationMillisecondNullConverter()
.toJson(instance.nullableValue),
};

Value? _$JsonConverterFromJson<Json, Value>(
Object? json,
Value? Function(Json json) fromJson,
) =>
json == null ? null : fromJson(json as Json);
''')
@JsonSerializable()
@_DurationMillisecondNullConverter()
@_DurationMillisecondConverter()
class JsonConverterIssue1339 {
late Duration value;
late Duration? nullableValue;
}

const _durationConverter = _DurationMillisecondConverter();

class _DurationMillisecondConverter implements JsonConverter<Duration, int> {
Expand All @@ -140,6 +172,17 @@ class _DurationMillisecondConverter implements JsonConverter<Duration, int> {
int toJson(Duration object) => throw UnimplementedError();
}

class _DurationMillisecondNullConverter
implements JsonConverter<Duration?, int> {
const _DurationMillisecondNullConverter();

@override
Duration? fromJson(int json) => throw UnimplementedError();

@override
int toJson(Duration? object) => throw UnimplementedError();
}

@ShouldThrow(
'Generators with constructor arguments are not supported.',
element: '',
Expand Down
Loading