String to Enum Interface conversion not working [SPR-12050]

[spring-projects-issues]

David Haraburda opened SPR-12050 and commented

Given an Enum that implements SomeInterface, GenericConversionService will attempt to use the default StringToEnumConverterFactory, even if a custom converter for String -> SomeInterface is registered. This problem was reported and described in #14326 but wasn't actually fixed. Instead, the "opposite" case of SomeInterface -> String conversion which also wasn't working was fixed.

The original test case attached to #14326 fails on 4.0.6; I have also attached to this JIRA an updated test case based on the original that in addition to demonstrating the described failure also shows a failure when a converter is registered for the base interface of some interface that is implemented by an Enum (i.e., SomeInterface extends SomeBaseInterface).

---

Affects: 4.0.6

Attachments:

• spring_conversion_service_tests.zip (15.11 kB)

Issue Links:

• Support conversion from Enum Interface [SPR-9692] #14326 Support conversion from Enum Interface

Referenced from: commits d0e6f0f, c299371

[spring-projects-issues]

David Haraburda commented

I have submitted a fix/pull request: #607

[spring-projects-issues]

Stéphane Nicoll commented

Have you ran the tests before submitting? I have 3 tests failing with your patch in ByteBufferConverterTests and DefaultConversionTests (looks like related to array handling).

Before I dig into this further, it would be great if you double check if you didn't forgot to submit something. Thanks!

[spring-projects-issues]

David Haraburda commented

Oops, this is my fault. I only ran GenericConversionServiceTests before pushing -- I see the problem now and I'll update my pull request shortly.

[spring-projects-issues]

David Haraburda commented

Ok, I have updated the pull request. All tests in spring-core pass, except for one in StandardReflectionParameterNameDiscover, but I see this is also failing on master.

[spring-projects-issues]

Stéphane Nicoll commented

Cheers. That test works for me btw so would be worth investigating why it doesn't for you (separate issue if needed)

[spring-projects-issues]

David Haraburda commented

Nevermind -- was just a silly compiler flag I needed to set. It passes now.

[spring-projects-issues]

Stéphane Nicoll commented

David, thanks for the update. I had a look to your patch and it would most probably break other use cases. Consider the following (badly named) example;

interface Foo {}
interface Bar {}
abstract class AbstractBar implements Bar {}
abstract class AbstractFoo extends AbstractBar implements Foo {}
class FooImpl extends AbstractFoo {}

If you ask the hiearchy of FooImpl we are currently providing its implementation hierarchy and we then process the interfaces (that is FooImpl, AbstractFoo, AbstractBar, Foo, Bar, Object). This sounds what we should do as you may want to register a specific converter on the implementation (for instance, you may have conversions-tuning operations on AbstractBar). With your patch we get FooImpl, AbstractFoo, Foo, AbstractBar, Bar Object so the internal-specific converter on AbstractBar is lost.

The real problem here is that Enum is a very generic base class and it is being assigned "too soon" in the hierarchy. Maybe we should add some kind of logic to make sure this one ends at the very last in the chain. I don't know if other such interfaces would be affected.

[spring-projects-issues]

David Haraburda commented

You are right that the hierarchy returned is in a different order -- but is this really a problem? In your example, the only way you would not get the converter for AbstractBar is if you also had registered a converter for FooImpl, AbstractFoo, or Foo. These three are more "specific" classes/interfaces in the hierarchy than AbstractBar, so it seems the most intuitive choice.

Or am I missing something else when you say the converter for AbstractBar would be lost?

[spring-projects-issues]

Stéphane Nicoll commented

My point was that conversion often implies internal-specific things. If you have a converter for Foo but you happen to have a converter for a member in the hierarchy (AbstractBar), the current code will always prefer the base class (i.e. AbstractBar) and that's a good thing. Your change makes it that the interface is preferred and I don't think that's a good idea.

Your real problem is that enum is a base class but it has the exact opposite characteristics. That's why reversing the order is fixing your issue but is most probably going to break others.

[spring-projects-issues]

David Haraburda commented

Ok, I see your point. I've updated my pull request accordingly.

[spring-projects-issues]

Stéphane Nicoll commented

Alright, I went ahead and modified your patch with what I had in mind. Your last change didn't change much, as AbstractBar was still placed after the Foo interface.

What this issue is showing is that the code in that particular method may have quite a lot of side effects so I kept your tests and revert to what we had before, only adding a special handling for Enum as we discussed.

Thanks for taking the time to refine your PR based on our discussions, much appreciated!

This will be available in 4.1.0.BUILD-SNAPSHOT in a couple of hours, please give it a try.

[spring-projects-issues]

David Haraburda commented

Hmm, are you sure you got the latest from the pull request? I copied the Foo/Bar hierarchy you gave and also the private getClassHierarchy directly into the test with a System.out.println and got:

[class org.springframework.core.convert.support.GenericConversionServiceTests$FooImpl, class org.springframework.core.convert.support.GenericConversionServiceTests$AbstractFoo, class org.springframework.core.convert.support.GenericConversionServiceTests$AbstractBar, interface org.springframework.core.convert.support.GenericConversionServiceTests$Foo, interface org.springframework.core.convert.support.GenericConversionServiceTests$Bar, class java.lang.Object]

so that definitely seems like the correct behavior.

In any case, if both the tests I added pass then that will fix the issue we were having with it.