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