Changes in bindings cause exception due to unsupported method in collection

[michael-simons]

Hello everybody,

I just noticed a changed behavior in bindings. The following code works in Spring Boot 1.5.3:

@Component
@ConfigurationProperties(prefix = "foobar")
public class Foobar {

    private Set<String> scope = Collections.emptySet();

    public Set<String> getScope() {
        return scope;
    }

    public void setScope(Collection<String> scope) {
        this.scope = scope == null ? Collections
                .<String>emptySet() : new LinkedHashSet<>(scope);
    }
}

Using properties like

foobar.scope = foo

The code in question is actually not my own, I found it in Spring Security OAuth, inside BaseClientDetails /cc @jgrandja but I wanted to provide a bare example (See oauth-binding-problem.zip)

Prior to Spring Boot 2.0.0.M1 setters seemed to be called and made it work. Now it tries to add to the default collection (which is immutable) and it fails with:

Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'foobar': Could not bind properties to Foobar (prefix=foobar, ignoreInvalidFields=false, ignoreUnknownFields=true); nested exception is org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'foobar.scope' to java.util.Collection<java.lang.String>
	at org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor.postProcessBeforeInitialization(ConfigurationPropertiesBindingPostProcessor.java:340)
	at org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor.postProcessBeforeInitialization(ConfigurationPropertiesBindingPostProcessor.java:305)
// More...
Caused by: org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'foobar.scope' to java.util.Collection<java.lang.String>
	at org.springframework.boot.context.properties.bind.Binder.handleBindError(Binder.java:225)
	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:200)
	at org.springframework.boot.context.properties.bind.Binder.lambda$bindBean$4(Binder.java:298)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:70)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:59)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:51)
	at org.springframework.boot.context.properties.bind.Binder.lambda$null$5(Binder.java:306)
// More...
Caused by: java.lang.UnsupportedOperationException: null
	at java.util.AbstractCollection.add(AbstractCollection.java:262)
	at java.util.AbstractCollection.addAll(AbstractCollection.java:344)
	at org.springframework.boot.context.properties.bind.CollectionBinder.merge(CollectionBinder.java:55)
	at org.springframework.boot.context.properties.bind.CollectionBinder.merge(CollectionBinder.java:31)
	at org.springframework.boot.context.properties.bind.AggregateBinder.bind(AggregateBinder.java:57)
	at org.springframework.boot.context.properties.bind.Binder.lambda$bindAggregate$2(Binder.java:273)
	at org.springframework.boot.context.properties.bind.Binder$Context.withIncreasedDepth(Binder.java:400)
	at org.springframework.boot.context.properties.bind.Binder.bindAggregate(Binder.java:272)
	at org.springframework.boot.context.properties.bind.Binder.bindObject(Binder.java:244)
	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:196)
	at org.springframework.boot.context.properties.bind.Binder.lambda$bindBean$4(Binder.java:298)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:70)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:59)
	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:51)
	at org.springframework.boot.context.properties.bind.Binder.lambda$null$5(Binder.java:306)

I'm not quite sure what you should do with the info, but I would guess that this can break a lot of projects, as it does with Spring Security OAuth atm.

[wilkinsona]

Interesting. Irrespective of the binding issue, the code in Spring Security OAuth is, arguably, a bit broken. By default, the Set returned from getScope() will be immutable but as soon as you call setScope(Set) with a non-null value, getScope() will return a mutable LinkedHashSet. That inconsistency feels to me like something that could be improved.

[mbhave]

This seems similar to #8602.

[michael-simons]

Yep, Arrays.asList is immutable, too.

[snicoll]

@mbhave can we please reconsider this fix? Or clearly document the change in behaviour? Regardless of the code, we used to call a setter if present and we don't anymore which can caught users that were doing some extra stuff in the setter. I am not saying we should support such weird use case but it should be crystal clear that we no longer do that.

If that's documented already, please ignore me :)

[michael-simons]

If I may ask, what's the rational behind not using a setter if there's any?

[philwebb]

@michael-simons we've seen configuration properties like this:

public class Foo {

    public List<Bar> getBar() {
        // ...
    }

}

It's legitimate to have a no setter so I think that's why the current code works the way it does. We also don't know that we can create the appropriate list type (say it's some funky custom subclass), that's why we mutate it rather than call the setter. I think we can probably mutate and call the setter if one exists.