@ConfigurationProperties creates mutable collections, even on immutable classes

[mwisnicki]

Consider simple example:

@ConfigurationProperties("some.prefix")
@ConstructorBinding
class Props {
  final List<String> strings;
  public Props(List<String> strings) {
    this.strings = strings;
  }
}

Even though I'm trying to make immutable class here, Spring will unhelpfully inject mutable ArrayList.

Now in a very simple example like this you can of course wrap it yourself in Collections.unmodifiableList() but doing so quickly becomes tedious especially when you nest collection types.

Imagine the code to deeply freeze e.g. Map<String, List<String>> :(

It's even worse when you just want to use lombok.Value or maybe Kotlin (didn't check it) to avoid writing boilerplate completely.

Not sure why these property classes should ever be mutable but if desired one can add property to @ConfigurationProperties to control it. It should automatically be enabled when using @ConstructorBinding though as that's a sure sign someone is trying to make immutable properties.

[mwisnicki]

Turn's out it wasn't that hard to add using little known ConfigurationPropertiesBindHandlerAdvisor

@Configuration
public class ImmutableConfigurationSupport {

    @Bean
    ConfigurationPropertiesBindHandlerAdvisor configurationPropertiesBindHandlerAdvisor() {
        return bindHandler -> new AbstractBindHandler(bindHandler) {
            int immutable = 0;

            private boolean isImmutableTarget(Bindable<?> target) {
                var klass = target.getType().resolve();
                return klass != null && klass.isAnnotationPresent(ConstructorBinding.class);
            }

            @Override
            public <T> Bindable<T> onStart(ConfigurationPropertyName name, Bindable<T> target, BindContext context) {
                if (isImmutableTarget(target))
                    immutable++;
                return super.onStart(name, target, context);
            }

            @Override
            public Object onSuccess(ConfigurationPropertyName name, Bindable<?> target, BindContext context, Object result) {
                var object = super.onSuccess(name, target, context, result);
                var targetClass = target.getType().resolve();
                if (immutable > 0 && targetClass != null) {
                    if (object instanceof List && targetClass.isAssignableFrom(List.class))
                        return Collections.unmodifiableList((List<?>) object);
                    if (object instanceof Set && targetClass.isAssignableFrom(Set.class))
                        return Collections.unmodifiableSet((Set<?>) object);
                    if (object instanceof Map && targetClass.isAssignableFrom(Map.class))
                        return Collections.unmodifiableMap((Map<?, ?>) object);
                }
                return object;
            }

            @Override
            public void onFinish(ConfigurationPropertyName name, Bindable<?> target, BindContext context, Object result) throws Exception {
                super.onFinish(name, target, context, result);
                if (isImmutableTarget(target))
                    immutable--;
            }
        };
    }

}

Is this something the project would consider adding or should I create a library?

[philwebb]

I'm glad the ConfigurationPropertiesBindHandlerAdvisor is working, but I think we should probably look at doing something directly in the CollectionBinder. I quite like the idea of using unmodifiable collections everywhere, but I'm worried a bit about back compatibility.

We need to discuss this a bit as a team before we make any final decision.

[philwebb]

We discussed this today and decided that unmodifiable would be a good default but we need to provide an escape hatch for those that need mutable binding.

[mwisnicki]

Isn't ConfigurationPropertiesBindHandlerAdvisor such an escape hatch? :)

[wilkinsona]

While it can achieve the same end result, it's too low-level and verbose to recommend as a way to restore the current behaviour for users who are relying on it.

[rakibmail22]

I am interested to work in this issue. I have been working with Spring Mvc and Spring Boot based enterprise application for five years and every now and then I had a peek inside the framework code.

So far I have tried cloning the code, setting up the environment, ide. Now I am trying to build it. As a first timer I may need a few feedback and acquaintance with the process to get started.

[wilkinsona]

Thanks, @rakibmail22. There's some documentation in the wiki on working with the code.

For this particular issue, I think we'll need changes to CollectionBinder and MapBinder so that the collections and maps that they create are immutable by default. Some updates to the corresponding tests (CollectionBinderTests and MapBinderTests) will also need to be made to check that the collections and maps created are now immutable.

We'll also need to provide a way for someone to opt out of this behaviour so that things behave as they do today. I'm not exactly sure how that should look at the moment. Perhaps an attribute on @ConfigurationProperties.

[rakibmail22]

@wilkinsona I looked into the CollectionBinder where the initial Collection is created and values are bound. But after that in the returning process there are few conversions that are done. The bind mehod of AggregateBinder returns an Object and that Object in a later step with help of a cnverter is converted to the desired list. During this conversion process it fails if I make the Collection immutable directly inside the CollectionBinder. Still, I am trying to have a better understanding of the whole binding process.

[wilkinsona]

Thanks for taking a look. No conversions should be necessary if the Collection created by the binder is of the required type. How are you making the Collection unmodifiable? I don't think Collections.unmodifiableCollection will be adequate in this case as the result is a Collection rather than something more specific such as a List. Instead, I think it'll be necessary to do something like this:

@SuppressWarnings({ "unchecked", "rawtypes" })
private Collection<Object> unmodifiable(Collection<Object> collection) {
    if (collection instanceof SortedSet) {
        return Collections.unmodifiableSortedSet((SortedSet) collection);
    }
    if (collection instanceof Set) {
        return Collections.unmodifiableSet((Set) collection);
    }
    if (collection instanceof List) {
        return Collections.unmodifiableList((List) collection);
    }
    return Collections.unmodifiableCollection(collection);
}

[rakibmail22]

I am trying something similar like below.

@Override
protected Collection<Object> merge(Supplier<Collection<Object>> existing, Collection<Object> additional) {
	Collection<Object> existingCollection = getExistingIfPossible(existing);
	if (existingCollection == null) {
		return unmodifiable(additional);
	}
	try {
		existingCollection.clear();
		existingCollection.addAll(additional);
		return unmodifiable(copyIfPossible(existingCollection));
	}
	catch (UnsupportedOperationException ex) {
		return unmodifiable(createNewCollection(additional));
	}
}

private Collection<Object> unmodifiable(Collection<Object> collection) {
	if (collection instanceof SortedSet) {
		SortedSet<Object> result = (SortedSet<Object>) collection;
		return Collections.unmodifiableSortedSet(result);
	}

	if (collection instanceof Set) {
		Set<Object> result = (Set<Object>) collection;
		return Collections.unmodifiableSet(result);
	}

	if (collection instanceof List) {
		List<Object> result = (List<Object>) collection;
		return Collections.unmodifiableList(result);
	}

	throw new IllegalArgumentException("Unsupported Collection interface: ");
}

Two test cases failed in CollectionBinderTests

1. bindToCollectionWhenHasExistingCollectionShouldReplaceAllContents()
2. bindToCollectionWithNoDefaultConstructor()

The first one is failing because it is exactly checking for a LinkedList
The second one is failing with the following underlying error
Caused by: org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.util.Collections$UnmodifiableRandomAccessList<?>] to type [org.springframework.boot.context.properties.bind.CollectionBinderTests$MyCustomNoDefaultConstructorList] for value '[a, b, c, c]'; nested exception is java.lang.IllegalArgumentException: Could not instantiate Collection type: org.springframework.boot.context.properties.bind.CollectionBinderTests$MyCustomNoDefaultConstructorList at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:47)

[wilkinsona]

I'm not sure why merge creates a new Collection from additional at the moment. additional should be the result of calling bindAggregate so I think it can just be used as-is. In other words, as long as bindAggregate returns an unmodifiable collection, I think the catch block in merge can just return additional. If there is a need for additional to be turned into a new collection, @mbhave and @philwebb may recall the details.

There will be some cases where I don't think we should bind an unmodifiable collection (or Map). A property that's a LinkedList would be one such case as I don't think we should go to the lengths of creating a custom LinkedList subclass that's unmodifiable.

To get an immutable collection or map, I think the property should be declared as a Set, SortedSet, List, Map, etc. With the property's actual type being used to influence the creation of any new collections or maps. For example, if a Set property is actually a TreeSet then a TreeSet is what should be created and populated before making it an unmodifiable Set.

The binder's one of the most complex areas of Boot's code base so if this starts consuming more of your time than you have realised it would, please do feel free to step away.

[philwebb]

I can't remember the details I'm afraid. Looks like the change was made for #9290. I think we can return additional from that catch block.

[mbhave]

yeah, I don't think creating a new collection is necessary in the catch block. Seems like an oversight.

[rakibmail22]

Shall this be a separate issue or this change should be a part of the current one?

[wilkinsona]

@rakibmail22 It's fine to change it as part of this issue. We can always separately apply the same change to other branches if we decide that it's worth it.