Refine CharSequenceToObjectConverter logic

Update `CharSequenceToObjectConverter` so that conversion that would
apply using an `ObjectTo...` converter now favors `toString()` based
conversion.

Prior to this commit, when converting a `CharSequence` to a `Collection`
the `ObjectToCollectionConveter` would be picked instead of the
`StringToCollectionConverter`. This resulted in a `Collection`
containing a single `String` value, rather than the expected list
of values split around ",".

Fixes gh-25057

Author: philwebb

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/ApplicationConversionService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2020 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,9 +21,11 @@
 
 import org.springframework.beans.factory.ListableBeanFactory;
 import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConverterRegistry;
 import org.springframework.core.convert.converter.GenericConverter;
+import org.springframework.core.convert.converter.GenericConverter.ConvertiblePair;
 import org.springframework.core.convert.support.ConfigurableConversionService;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.format.Formatter;
@@ -61,6 +63,28 @@ public ApplicationConversionService(StringValueResolver embeddedValueResolver) {
 		configure(this);
 	}
 
+	/**
+	 * Return {@code true} if objects of {@code sourceType} can be converted to the
+	 * {@code targetType} and the converter has {@code Object.class} as a supported source
+	 * type.
+	 * @param sourceType the source type to test
+	 * @param targetType the target type to test
+	 * @return is conversion happens via an {@code ObjectTo...} converter
+	 * @since 2.4.3
+	 */
+	public boolean isConvertViaObjectSourceType(TypeDescriptor sourceType, TypeDescriptor targetType) {
+		GenericConverter converter = getConverter(sourceType, targetType);
+		Set<ConvertiblePair> pairs = (converter != null) ? converter.getConvertibleTypes() : null;
+		if (pairs != null) {
+			for (ConvertiblePair pair : pairs) {
+				if (Object.class.equals(pair.getSourceType())) {
+					return true;
+				}
+			}
+		}
+		return false;
+	}
+
 	/**
 	 * Return a shared default application {@code ConversionService} instance, lazily
 	 * building it once needed.

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/CharSequenceToObjectConverter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2020 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,6 +33,8 @@ class CharSequenceToObjectConverter implements ConditionalGenericConverter {
 
 	private static final TypeDescriptor STRING = TypeDescriptor.valueOf(String.class);
 
+	private static final TypeDescriptor BYTE_ARRAY = TypeDescriptor.valueOf(byte[].class);
+
 	private static final Set<ConvertiblePair> TYPES;
 
 	private final ThreadLocal<Boolean> disable = new ThreadLocal<>();
@@ -59,14 +61,41 @@ public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
 		}
 		this.disable.set(Boolean.TRUE);
 		try {
-			return !this.conversionService.canConvert(sourceType, targetType)
-					&& this.conversionService.canConvert(STRING, targetType);
+			boolean canDirectlyConvertCharSequence = this.conversionService.canConvert(sourceType, targetType);
+			if (canDirectlyConvertCharSequence && !isStringConversionBetter(sourceType, targetType)) {
+				return false;
+			}
+			return this.conversionService.canConvert(STRING, targetType);
 		}
 		finally {
 			this.disable.set(null);
 		}
 	}
 
+	/**
+	 * Return if String based conversion is better based on the target type. This is
+	 * required when ObjectTo... conversion produces incorrect results.
+	 * @param sourceType the source type to test
+	 * @param targetType the target type to test
+	 * @return id string conversion is better
+	 */
+	private boolean isStringConversionBetter(TypeDescriptor sourceType, TypeDescriptor targetType) {
+		if (this.conversionService instanceof ApplicationConversionService) {
+			ApplicationConversionService applicationConversionService = (ApplicationConversionService) this.conversionService;
+			if (applicationConversionService.isConvertViaObjectSourceType(sourceType, targetType)) {
+				// If and ObjectTo... converter is being used then there might be a better
+				// StringTo... version
+				return true;
+			}
+		}
+		if ((targetType.isArray() || targetType.isCollection()) && !targetType.equals(BYTE_ARRAY)) {
+			// StringToArrayConverter / StringToCollectionConverter are better than
+			// ObjectToArrayConverter / ObjectToCollectionConverter
+			return true;
+		}
+		return false;
+	}
+
 	@Override
 	public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
 		return this.conversionService.convert(source.toString(), STRING, targetType);

spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/ApplicationConversionServiceTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2019 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@
 package org.springframework.boot.convert;
 
 import java.text.ParseException;
+import java.util.List;
 import java.util.Locale;
 import java.util.Set;
 
@@ -32,6 +33,7 @@
 import org.springframework.format.Parser;
 import org.springframework.format.Printer;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -91,6 +93,26 @@ void addBeansWhenHasParserBeanAddParser() {
 		}
 	}
 
+	@Test
+	void isConvertViaObjectSourceTypeWhenObjectSourceReturnsTrue() {
+		// Uses ObjectToCollectionConverter
+		ApplicationConversionService conversionService = new ApplicationConversionService();
+		TypeDescriptor sourceType = TypeDescriptor.valueOf(Long.class);
+		TypeDescriptor targetType = TypeDescriptor.valueOf(List.class);
+		assertThat(conversionService.canConvert(sourceType, targetType)).isTrue();
+		assertThat(conversionService.isConvertViaObjectSourceType(sourceType, targetType)).isTrue();
+	}
+
+	@Test
+	void isConvertViaObjectSourceTypeWhenNotObjectSourceReturnsFalse() {
+		// Uses StringToCollectionConverter
+		ApplicationConversionService conversionService = new ApplicationConversionService();
+		TypeDescriptor sourceType = TypeDescriptor.valueOf(String.class);
+		TypeDescriptor targetType = TypeDescriptor.valueOf(List.class);
+		assertThat(conversionService.canConvert(sourceType, targetType)).isTrue();
+		assertThat(conversionService.isConvertViaObjectSourceType(sourceType, targetType)).isFalse();
+	}
+
 	static class ExampleGenericConverter implements GenericConverter {
 
 		@Override

spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/CharSequenceToObjectConverterTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2020 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,12 +16,17 @@
 
 package org.springframework.boot.convert;
 
+import java.util.List;
 import java.util.stream.Stream;
 
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.provider.Arguments;
 
 import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.Converter;
+import org.springframework.format.support.DefaultFormattingConversionService;
+import org.springframework.format.support.FormattingConversionService;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -45,6 +50,29 @@ void convertWhenCanConvertDirectlySkipsStringConversion(ConversionService conver
 		}
 	}
 
+	@Test
+	@SuppressWarnings("unchecked")
+	void convertWhenTargetIsList() {
+		ConversionService conversionService = new ApplicationConversionService();
+		StringBuilder source = new StringBuilder("1,2,3");
+		TypeDescriptor sourceType = TypeDescriptor.valueOf(StringBuilder.class);
+		TypeDescriptor targetType = TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(String.class));
+		List<String> conveted = (List<String>) conversionService.convert(source, sourceType, targetType);
+		assertThat(conveted).containsExactly("1", "2", "3");
+	}
+
+	@Test
+	@SuppressWarnings("unchecked")
+	void convertWhenTargetIsListAndNotUsingApplicationConversionService() {
+		FormattingConversionService conversionService = new DefaultFormattingConversionService();
+		conversionService.addConverter(new CharSequenceToObjectConverter(conversionService));
+		StringBuilder source = new StringBuilder("1,2,3");
+		TypeDescriptor sourceType = TypeDescriptor.valueOf(StringBuilder.class);
+		TypeDescriptor targetType = TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(String.class));
+		List<String> conveted = (List<String>) conversionService.convert(source, sourceType, targetType);
+		assertThat(conveted).containsExactly("1", "2", "3");
+	}
+
 	static Stream<? extends Arguments> conversionServices() {
 		return ConversionServiceArguments.with((conversionService) -> {
 			conversionService.addConverter(new StringToIntegerConverter());