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

[julek-kal]

Connection with issue(s)

Close #1339

Solution description

When targetType is nullable and the class is annotated with both converters for nullable and non-nullable type, _compatibleMatch returns two of them. So I've added a check if targetType is nullable and matchingAnnotations length is equal to 2, if so I proceed to try to find a non-null converter and remove it from the list of matchedConverters.

To Do

• Check the original issue to confirm it is fully satisfied
• Add solution description to help guide reviewers
• Add unit test to verify new or fixed behaviour
• If apply, add documentation to code properties and package readme

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[kevmoo]

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?

[julek-kal]

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

[t-beckmann]

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.

[julek-kal]

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

[jiechic]

any update for this PR？

[julek-kal]

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.

[ljmatan]

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.