ObjectToObjectConverter doesn't consider return type of static methods

[philwebb]

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();
		}

	}

}

[sbrannen]

Related Issues

• Add Converter implementations that convert legacy Date instances into JDK 8 date/time types [SPR-11259] #15884
• ConversionService cannot convert from Object to String anymore by default [SPR-11693] #16315

Related Commits

• 3234d9e

[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.