DomainClassConverter#matches should return false when source- and targetType are equal [DATACMNS-583]

[spring-projects-issues]

Joris Kuipers opened DATACMNS-583 and commented

I ran into a very peculiar issue today, that was hard to reproduce. In a Spring-MVC app that uses Spring Data JPA and has a DomainClassConverter registered I have a controller method that takes some entity as a model attribute parameter. The entity has a String ID field.
What happens now in some cases (haven't identified the exact circumstances) is that the entity instance, which is initially created and bound correctly, is ran through the conversion service, which finds the DomainClassConverter which will happily map the entity instance onto another instance of the same type by mapping the instance to a String using the standard ObjectToStringConverter and treating the result as the primary key of the entity type... This results in always getting null passed in as the parameter for the controller method.
Obviously the DomainClassConverter should not be used at all in this scenario.

Fortunately I was able to reproduce the scenario in a simple app (see attachment). It's really weird: if you remove the extra non-default constructor of the Entity class, for example, you can no longer reproduce the error. Also, if you comment out the spring.version property in the pom, thereby using 4.0.7.RELEASE instead of 4.1.1.RELEASE, you can also not reproduce the error. This might therefore be related to a recent change in Spring Core as well.

Anyways, there is an easy fix for this issue: the DomainClassConvert#matches method should first check if the source- and targetType are equal and if so, return false: I'd say that no domain class can have itself as its key type and thus it should never attempt to convert these types.

If you manage to pinpoint the exact cause you might find that there's a better solution, but this is how I worked around this issue for now

---

Affects: 1.8.4 (Dijkstra SR4)

Attachments:

• domainclassconverter-bug.zip (4.82 kB)

Referenced from: pull request #101

Backported to: 1.9.1 (Evans SR1), 1.8.5 (Dijkstra SR5)

[spring-projects-issues]

Thomas Darimont commented

Hello Joris,

Good catch! Thanks for your example. I gave this a spin and came up with a slightly different solution.
Instead of returning false in org.springframework.data.repository.support.DomainClassConverter.matches(TypeDescriptor, TypeDescriptor)
I'd rather return true in case sourceType.equals(targetType) == true and simply returning the source in that case in org.springframework.data.repository.support.DomainClassConverter.convert(Object, TypeDescriptor, TypeDescriptor).

The reason for this is that we could potentially have multiple conversions registered in the GenericConversionService registered the last one being an org.springframework.core.convert.support.ObjectToObjectConverter which returns the source} object in case {{sourceType and destinationType matches.

In stead of going through potentially multiple converters I'd rather handle this case in DomainClassConverter directly which could save some unnecessary conversion hops.

Cheers,
Thomas

[spring-projects-issues]

Joris Kuipers commented

I'd say that if you'd follow your reasoning, then almost every converter should return true when asked if it can map an X onto an X and return the input as-is. I don't think it's the responsibility of individual converters to worry about the overhead of having multiple converters registered by handling conversions they weren't intended for.
I do think it's rather confusing that the Spring binding mechanism finds it necessary in this case to check for a registered converter, but if that is intended behavior then your solution would prevent people from relying on that mechanism by registering their own X-to-X converter which could e.g. enrich the data in every X: the DomainClassConverter would then get in the way if it happened to be registered earlier in the list

[spring-projects-issues]

Thomas Darimont commented

Hmmm, yes and no and yes :-).

I agree, that this would cause trouble in cases where users use a converter to enrich domain objects
instead of performing an actual type conversion. Unfortunately the API contract of the Conversion infrastructure isn't strict enough about being only about type conversion.

Given that I think we have to stay defensive here, so I'll apply your suggestion of just returning false in org.springframework.data.repository.support.DomainClassConverter.matches(TypeDescriptor, TypeDescriptor) case sourceType.equals(targetType) == true

[spring-projects-issues]

Thomas Darimont commented

After some additional thought and discussions with jhoeller I came to the conclusion that it should be okay to add support for the identity conversion (= convert an input of type T to the same output of type T) as fast-path here.

If users (ab)use the conversion infrastructure for enrichment they should make sure that their converter takes precedence over all other converters for this conversion (T source = T target) anyways. Otherwise you cannot guarantee that other (framework or third-party) converters don't do perform the conversion already.
One way to achieve that would be to register a custom ConversionService potentially extending or delegating to e.g. GenericConversionService. With this you have control over the order of conversion registrations.

Maybe Oliver Drotbohm has an opinion on this as well :)

[spring-projects-issues]

Joris Kuipers commented

OK, if Jűrgen says so then I cannot possibly disagree ;)
I would still be very interested in learning why the identity conversion is attempted here in the first place, and in why it doesn't happen when you remove the non-default constructor from the entity in my sample app or when you use Spring 4.0.7 instead of 4.1.1.