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.

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

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

Closed
spring-projects-issues opened this issue Aug 15, 2012 · 8 comments
Closed

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

spring-projects-issues opened this issue Aug 15, 2012 · 8 comments
Assignees
Labels
in: core type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 15, 2012

Wilhelm Kleu opened SPR-9692 and commented

I noticed the following issue while upgrading my Spring version from version 3.0.6 to version 3.1.2.

getMatchingConverterForTarget in GenericConversionService will return the default converter StringToEnumConverterFactory even if I implement my own ConverterFactory<String, MyInterface> and the enum implements MyInterface.

In GenericConversionService.getMatchingConverterForTarget(...) the class queue first tries to match a converter of the base class and if none is found it will try to match the super class (Enum.class) and will find StringToEnumConverterFactory. It does not try to match converters to the interfaces.

Possible solution:
Don't add superclass to classQueue if an enum:
GenericConversionService:433:

 
if (superClass != null && superClass != Object.class && superClass != Enum.class) {

And then try to match a converter for Enum.class if no other converter is found:
GenericConversionService:447:

if (targetObjectType.isEnum()) {
    GenericConverter converter = matchConverter(converters.get(Enum.class), sourceType, targetType);
    if (converter != null) {
        return converter;
    }
}
return matchConverter(converters.get(Object.class), sourceType, targetType);

Test case attached.


Affects: 3.1.2

Attachments:

Issue Links:

  • #14560 Refactor GenericConversionService ("depends on")
  • #16666 String to Enum Interface conversion not working
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2012

Wilhelm Kleu commented

The reverse is also true (MyEnumInterfaceToStringConverter not used but EnumToStringConverter is).
Updated test cases.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 29, 2012

Wilhelm Kleu commented

Pull request submitted: #127

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 29, 2012

Phil Webb commented

This actually feels like a more general problem and not one specifically tied to Enums. The root cause of difference in behavior between 3.0 and 3.1 appears to be this commit:
f43d0e1

For example consider

class A extends B implements D
class B extends C
interface D extends E

If a converter is registered for class C and interface D then the class converter will be picked. The order of evaluation is: A, B, C, D, E

I wonder if the algorithm should be changed to work recursively outwards from the class, considering superclass then interfaces, ie: A, B, D, C, E.

The concern is that this change may break existing code and presumably the difference between 3.0 and 3.1 was driven by some real need.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2012

Phil Webb commented

#132

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 25, 2012

Chris Beams commented

Phil, assigning back to you after an initial review: philwebb#1

Could you also please update this JIRA's subject line to reflect the actual changes you've made? And in the final analysis, is this actually a bug or an improvement (it can be a fine line sometimes).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 30, 2012

Chris Beams commented

commit f82c6ed7cc8639991bac0afde787ea00ad4331ae
Author: Phillip Webb <pwebb@vmware.com>
Commit: Chris Beams <cbeams@vmware.com>

    Support conversion from Enum Interface
    
    EnumToStringConverter in now conditional and only matches Enums that
    do not implement interfaces that can be converted.
    
    Issue: SPR-9692

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 28, 2014

David Haraburda commented

The pull request that was committed for this issue doesn't actually fix the problem originally described... the original description details a problem converting from String to Enum (although the JIRA title has it the other way around -- a bit confusing). A comment the next day indicates that the "reverse", Enum to String, is also broken. It looks like this logic was the only thing fixed with the Phil's pull request.

Unfortunately, String to Enum still appears to be broken, in fact the original test case attached with this issue still fails (I just tried it on 4.0.6.RELEASE).

It looks like the logic in Converters.getClassHierarchy needs to check if the superclass is Enum.class and if it is, first add any implemented interfaces to the list/hierarchy before adding Enum.class (similar to what the original reporter had suggested).

Can this issue be reopened? Should I create a new issue?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 28, 2014

Phil Webb commented

David Haraburda Could you please raise a new issue for this, this one is quite old now and has been closed for some time so I think it would be good to start afresh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants