Constructor binding with a custom collection type does not work

[wilkinsona]

The problem, originally reported on StackOverflow, can be reproduced with the following application:

package com.example;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
import java.util.Spliterator;
import java.util.function.Consumer;
import java.util.function.IntFunction;
import java.util.function.Predicate;
import java.util.function.UnaryOperator;
import java.util.stream.Stream;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.SpringBootConfiguration;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConfigurationPropertiesBinding;
import org.springframework.boot.context.properties.ConstructorBinding;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.core.convert.converter.Converter;

import com.example.BindingToCustomCollectionApplication.CustomList;

@SpringBootConfiguration
@EnableConfigurationProperties(com.example.BindingToCustomCollectionApplication.CustomListProperties.class)
public class BindingToCustomCollectionApplication {

	public static void main(String[] args) {
		CustomListProperties properties = SpringApplication
				.run(BindingToCustomCollectionApplication.class, "--custom.values[0]=zero", "--custom.values[1]=one")
				.getBean(CustomListProperties.class);
		System.out.println(properties.getValues());
	}
	
	@Bean
	@ConfigurationPropertiesBinding
	static Converter<ArrayList<?>, CustomList<?>> arrayListToCustomList() {
		return new Converter<>() {

			@Override
			public CustomList<?> convert(ArrayList<?> source) {
				return new CustomList<>(source);
			}
			
		};
		
	}
	
	@ConstructorBinding
	@ConfigurationProperties("custom")
	static class CustomListProperties {
		
		private final CustomList<String> values;
		
		CustomListProperties(CustomList<String> values) {
			super();
			this.values = values;
		}

		public CustomList<String> getValues() {
			return values;
		}

	}
	
//	@ConfigurationProperties("custom")
//	static class CustomListProperties {
//		
//		private CustomList<String> values;
//
//		public CustomList<String> getValues() {
//			return values;
//		}
//
//		public void setValues(CustomList<String> values) {
//			this.values = values;
//		}
//
//	}
	
	static class CustomList<E> implements List<E> {
		
		private final List<E> delegate;
		
		private CustomList(List<E> delegate) {
			this.delegate = delegate;
		}

		public void forEach(Consumer<? super E> action) {
			delegate.forEach(action);
		}

		public int size() {
			return delegate.size();
		}

		public boolean isEmpty() {
			return delegate.isEmpty();
		}

		public boolean contains(Object o) {
			return delegate.contains(o);
		}

		public Iterator<E> iterator() {
			return delegate.iterator();
		}

		public Object[] toArray() {
			return delegate.toArray();
		}

		public <T> T[] toArray(T[] a) {
			return delegate.toArray(a);
		}

		public boolean add(E e) {
			return delegate.add(e);
		}

		public boolean remove(Object o) {
			return delegate.remove(o);
		}

		public boolean containsAll(Collection<?> c) {
			return delegate.containsAll(c);
		}

		public boolean addAll(Collection<? extends E> c) {
			return delegate.addAll(c);
		}

		public boolean addAll(int index, Collection<? extends E> c) {
			return delegate.addAll(index, c);
		}

		public boolean removeAll(Collection<?> c) {
			return delegate.removeAll(c);
		}

		public boolean retainAll(Collection<?> c) {
			return delegate.retainAll(c);
		}

		public void replaceAll(UnaryOperator<E> operator) {
			delegate.replaceAll(operator);
		}

		public <T> T[] toArray(IntFunction<T[]> generator) {
			return delegate.toArray(generator);
		}

		public void sort(Comparator<? super E> c) {
			delegate.sort(c);
		}

		public void clear() {
			delegate.clear();
		}

		public boolean equals(Object o) {
			return delegate.equals(o);
		}

		public int hashCode() {
			return delegate.hashCode();
		}

		public E get(int index) {
			return delegate.get(index);
		}

		public E set(int index, E element) {
			return delegate.set(index, element);
		}

		public void add(int index, E element) {
			delegate.add(index, element);
		}

		public boolean removeIf(Predicate<? super E> filter) {
			return delegate.removeIf(filter);
		}

		public E remove(int index) {
			return delegate.remove(index);
		}

		public int indexOf(Object o) {
			return delegate.indexOf(o);
		}

		public int lastIndexOf(Object o) {
			return delegate.lastIndexOf(o);
		}

		public ListIterator<E> listIterator() {
			return delegate.listIterator();
		}

		public ListIterator<E> listIterator(int index) {
			return delegate.listIterator(index);
		}

		public List<E> subList(int fromIndex, int toIndex) {
			return delegate.subList(fromIndex, toIndex);
		}

		public Spliterator<E> spliterator() {
			return delegate.spliterator();
		}

		public Stream<E> stream() {
			return delegate.stream();
		}

		public Stream<E> parallelStream() {
			return delegate.parallelStream();
		}
		
		public String toString() {
			return delegate.toString();
		}
		
	}

}

It works with JavaBean-based binding (as can be seen by uncommenting one CustomListProperties class and commenting out the other one).

[yuhengfdada]

I did some debugging and one thing seems strange to me. Seems in Constructor binding, our custom converter arrayListToCustomList() doesn't even come into play.

In JavaBean-based binding, in AbstractAutowireCapableBeanFactory#doCreateBean, createBeanInstance() is called, then initializeBean(). During the latter we got the custom converter called.
In Constructor binding, we seem to do everything in createBeanInstance() and we only got the Collection entry's converter called, in this case String -> String. But the conversion for the collection itself is missing.

[wilkinsona]

The problem's in CollectionBinder where the type of collection that's used varies depending on whether or not the target has a value:

spring-boot/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java

Line 43 in c587eae

Class<?> collectionType = (target.getValue() != null) ? List.class : target.getType().resolve(Object.class);

With JavaBean binding, the target has a value so List is used. With constructor binding, the target does not have a value so target.getType().resolve(Object.class) which is the custom list implementation is used. When List is used, CollectionFactory creates an ArrayList which is subsequently converted into the custom list type. When the custom list implementation is used, CollectionFactory tries to create an instance of it and fails because there's no default constructor.

This logic was introduced to fix #10106. Without stating that a List should be used, CollectionFactory would use a Set by default and duplicate values would be lost. The logic looked like this originally:

spring-boot/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java

Lines 41 to 42 in 56d820a

	Class<?> collectionType = (type != null ? type
	: ResolvableType.forClassWithGenerics(List.class, Object.class).resolve());

The switch to using the target's value and type was made in this polishing commit.

I think we may just be able to use List all the time. This will result in CollectionFactory creating an ArrayList and we'll then always rely on conversion to turn that ArrayList into a different collection implementation if needed. If there's a custom converter registered, it will be used. If not, CollectionToCollectionConverter converter will be used. It uses CollectionFactory to create the target Collection instance so I don't think we lose anything here in terms of functionality. It may be marginally slower as constructor binding will now create an ArrayList instance and then convert it to the custom collection type – aligning its behavior with JavaBean binding – rather than creating the custom collection instance directly.

The change to always using List is in this branch. I'd like to discuss it with the team before merging.