Ensure that ConversionService.canConvert(Enum) no longer throws an exception

[moonfruit]

In my project, I use the following code to find an appropriate method to execute for a given class. It throws an IllegalArgumentException: The target type java.lang.Enum does not refer to an enum when there is a more appropriate “method”: asString(CharSequence).

public class MyTest {

    public static void main(String[] args) throws AccessException {
        EvaluationContext evaluationContext = SimpleEvaluationContext.forReadOnlyDataBinding().build();
        ReflectiveMethodResolver methodResolver = new ReflectiveMethodResolver();
        MethodExecutor asString = methodResolver.resolve(evaluationContext, MyTest.class, "asString",
                Collections.singletonList(TypeDescriptor.valueOf(String.class)));
        TypedValue result = asString.execute(evaluationContext, MyTest.class, "value");
        System.out.println(result);
    }

    public static String asString(Enum<?> left) {
        return left.name();
    }

    public static String asString(CharSequence left) {
        return left.toString();
    }
}

After doing some research I realized that the root cause was that StringToEnumConverterFactory.getConverter(Enum.class) threw an exception when it couldn't convert and resolve the concrete Enum. This directly causes ConversionService.canConvert(String.class,Enum.class) to throw an exception. I don't think the method canConvert() should throw an exception, it returns true if it can convert and false if it can't. That's what this method is for, to avoid throwing an exception when calling the convert method directly.

So I've modified StringToEnumConverterFactory and IntegerToEnumConverterFactory a little and hopefully this PR will be accepted.

[sbrannen]

Hi @moonfruit,

Congratulations on submitting your first PR for the Spring Framework! 👍

And thanks for bringing this to our attention.

I agree that canConvert() ideally should not throw an exception. To address that, I've requested some changes in the PR (see other comments). In addition, let's have IntegerToEnumConverterFactory and StringToEnumConverterFactory implement ConditionalConverter. Their implementations could then look something like this:

	@Override
	public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
		return (ConversionUtils.resolveEnumType(targetType.getType()) != null);
	}

If you feel comfortable making these changes, go for it!

Otherwise, just let me know, and I can take it from here.

Cheers,

Sam

[moonfruit]

Hi @sbrannen,

I've updated the PR with all the requested changes:

• Renamed ConversionUtils.getEnumType(...) to resolveEnumType(...) and added Javadoc pointing out that it returns null if the enum type cannot be resolved.
• Removed the NonConvertableToEnum converter. StringToEnumConverterFactory and IntegerToEnumConverterFactory now implement ConditionalConverter, with matches(...) implemented as you suggested, and their getConverter(...) methods keep the original Assert.notNull(...) semantics.
• Applied your suggested test, canConvertToEnumDoesNotThrowForNonEnumTargetType().

GenericConversionServiceTests and checkstyleMain/checkstyleTest pass locally for spring-core.

[sbrannen]

I think this looks good now. Thanks for making the requested changes! 👍

However, your commit is failing the DCO check. So please address that.

[moonfruit]

Sorry about that — the commit was missing the Signed-off-by trailer. I have amended the commit to add the sign-off and force-pushed the branch; the DCO check is passing now. No changes were made to the code itself.

[github-actions]

Fixed via e2fae06

[sbrannen]

This has been merged into 7.0.x and main.

Thanks