The ObjectToObject converter currently checks the return type for non-static methods; however, it does no such check for static methods. This means that it's possible for canConvert to return true when the actual conversion would throw an exception.

The following test shows the problem:

public class ObjectToObjectConverterTests {

    @Test
    void fromWithDifferentReturnType() {
        GenericConversionService conversionService = new GenericConversionService();
        conversionService.addConverter(new ObjectToObjectConverter());
        // assertThat(conversionService.canConvert(String.class, Data.class)).isFalse();
        Data data = conversionService.convert("test", Data.class);
    }

    static class Data {

        private final String value;

        private Data(String value) {
            this.value = value;
        }

        @Override
        public String toString() {
            return this.value;
        }

        public static Optional<Data> valueOf(String string) {
            return (string != null) ? Optional.of(new Data(string)) : Optional.empty();
        }

    }

}

Comment From: sbrannen

Related Issues

• 15884

• 16315

Related Commits

• https://github.com/spring-projects/spring-framework/commit/3234d9ede3da6c87a354000f2fd73ec7019930c2

Comment From: sbrannen

The following test cases pass with the change (to verify compatible return type for static factory methods).

class ObjectToObjectConverterTests {

    private final GenericConversionService conversionService = new GenericConversionService() {{
        addConverter(new ObjectToObjectConverter());
    }};


    /**
     * This test effectively verifies that the {@link ObjectToObjectConverter}
     * was properly registered with the {@link GenericConversionService}.
     */
    @Test
    void nonStaticToTargetTypeSimpleNameMethodWithMatchingReturnType() {
        assertThat(conversionService.canConvert(Source.class, Data.class))
            .as("can convert Source to Data").isTrue();
        Data data = conversionService.convert(new Source("test"), Data.class);
        assertThat(data).asString().isEqualTo("test");
    }

    @Test
    void nonStaticToTargetTypeSimpleNameMethodWithDifferentReturnType() {
        assertThat(conversionService.canConvert(Text.class, Data.class))
            .as("can convert Text to Data").isFalse();
        assertThat(conversionService.canConvert(Text.class, Optional.class))
            .as("can convert Text to Optional").isFalse();
        assertThatExceptionOfType(ConverterNotFoundException.class)
            .as("convert Text to Data")
            .isThrownBy(() -> conversionService.convert(new Text("test"), Data.class));
    }

    @Test
    void staticValueOfFactoryMethodWithDifferentReturnType() {
        assertThat(conversionService.canConvert(String.class, Data.class))
            .as("can convert String to Data").isFalse();
        assertThatExceptionOfType(ConverterNotFoundException.class)
            .as("convert String to Data")
            .isThrownBy(() -> conversionService.convert("test", Data.class));
    }


    static class Source {

        private final String value;

        private Source(String value) {
            this.value = value;
        }

        public Data toData() {
            return new Data(this.value);
        }

    }

    static class Text {

        private final String value;

        private Text(String value) {
            this.value = value;
        }

        public Optional<Data> toData() {
            return Optional.of(new Data(this.value));
        }

    }

    static class Data {

        private final String value;

        private Data(String value) {
            this.value = value;
        }

        @Override
        public String toString() {
            return this.value;
        }

        public static Optional<Data> valueOf(String string) {
            return (string != null) ? Optional.of(new Data(string)) : Optional.empty();
        }

    }

}

But... DefaultConversionServiceTests.convertObjectToStringWithJavaTimeOfMethodPresent() now fails, since ZoneId.of(String) declares that it returns ZoneId instead of ZoneRegion (which is a subclass of ZoneId).

I believe the new behavior is desired, but this will require some clarification since it may be a breaking change for applications that depend on the current behavior.