DATAMONGO-795 - When adding custom converters to the mongo template it i...

[eamonnmoloney]

DATAMONGO-795 - When adding custom converters to the mongo template it is possible to get unpredictable behaviour

• Added a check for duplicate convertible pairs when the targetType is unknown

@pullrequest emoloney

[odrotbohm]

Thanks for the pull request. Does it really make sense to throw an exception here? This would break the entire conversion and thus reading a set of data just because we have multiple candidates to resolve types. I think simply changing the internal usage of Sets to List should guarantee the order and thus that we use the same conversion for every method call. What do you think?

[eamonnmoloney]

When I first looked at the issue I thought of replacing the Sets with Lists but this is hiding the issue. While it would result in predicable behaviour, there would still be the confusion about why the converter is not using the custom converter. Perhaps a log statement would be enough enough of a hint that something is wrong however as the application is in an expected state I see the exception as cleaner. The designed application simply wouldn't work.

[odrotbohm]

Well, I thought the PR should resolve the issue reported in the ticket, which clearly is pointing out the unpredictability.

Is it really invalid to register two different write conversions for a single type? Assume you have one converter FooToDbObject and a FooToString. The MappingMongoConverter explicitly checks for a conversion to DbObject and should find the former. For nested conversions, you might want to convert the same type into a simple String as it's more convenient. With the list based approach, you'd gain explicit control over this by the order you register the converters. If you reject the duplicate converters right away, you prevent this use case entirely.

This is by no means a "doesn't work". It simply would work predictably here. Actually the test case you have added is a perfect example for that. I'd argue that by registering a custom write converter for DateTime you simply want to set that to work, overriding the default conversion mechanisms and not blow up DateTime conversion entirely, wouldn't you?

[eamonnmoloney]

You are right that registering multiple converters for a single type is valid however the problem occurs when MappingMongoConverter passes null for the targetType in writePropertyInternal. In this case there is no way to resolve which sourceType to targetType to use when multiple converters are registered.

Without some resolution strategy mechanism, there is no way to understand which converter to in any particular use case.

e.g.
If level 1 requires the FooToDBObject but level 2 requires FooToString, if FooToDBObject is first in the list, level2 can never apply the FooToString converter.

In my use case, the issue is that Joda converters are registered in the constructor before any custom converters are registered. With the current implementation, my custom Joda converter is apply randomly. Introducing a LinkedHashSet or List would remove the random nature but still leaving me wondering why my converter is not applied.

With the application in this state, for me, the application doesn't work even though the code is executing in a predicable manner. For this case I would like to see some logging or an exception. As my application doesn't give me the result I expect, I would prefer the exception but maybe I'm missing the underlaying mean of null being pass in the writePropertyInternal method and throwing an exception is inappropriate?

[odrotbohm]

Right, I see your point. But that's a matter of defining the rules :). I'd like to define them in a way that the users gain control over the registration order. So in the test case you have, I'd argue that the custom converter should be preferred over the default one. I'll go ahead and take it from there.