Spring-boot: Binding to a collection passes in the current instance to the setter

[dsyer]

In a @ConfigurationProperties bean if you bind to a collection Spring Boot binds the value and calls the setter (with the same instance). It's a bit confusing and probably unexpected.

Failing test:

@RunWith(SpringRunner.class)
@SpringBootTest(properties = "app.values=foo")
public class BindingApplicationTests {

    @Autowired
    private ConfigProps props;

    @Test
    public void contextLoads() {
        assertThat(props.getValues()).contains("foo");
    }

}

@ConfigurationProperties("app")
@Component
class ConfigProps {
    private Set<String> values = new HashSet<>();

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

    public void setValues(Set<String> values) {
        this.values.clear();
        this.values.addAll(values);
    }
}

Set a debug breakpoint in the setter to see what I mean.

[philwebb]

I have a feeling this was intentional in case the setter performed some additional validation or reset caches etc.

[mbhave]

It was definitely intentional.

[dsyer]

But is it really a good idea (see the test above)?

[philwebb]

Probably 6 of one, half-a-dozen of the other. Issue #9290 has more background.

[philwebb]

Perhaps if there's a setter we should try to create a new collection. Then only mutate an existing one if that fails?

[wilkinsona]

Changing the behaviour based on whether or not we can create a new collection sounds confusing to me.

FWIW, I think the setter in the test above is broken. It’s not a setter method as it doesn’t actually set the field’s value. I think it’s perfectly reasonable for the binder to expect that when it calls a setter, the passed in value will actually be set. It’s easy to describe and should be easy for users to implement.

[dsyer]

Whatever you think about the test (it's a little artificial I admit, but I don't fully concur with your definition of "setter"), don't you think it's a bit surprising to be passed the current instance into a setter? It's easy to work round, but quite hard to work out why it needs to be.

[wilkinsona]

I can't think of a situation where I've written a setter method and cared about the relationship between the value that's currently held and the one that's being passed in. Perhaps it is a bit surprising, but it's definitely less surprising to me than a setter method that cares about the difference. I can't think why you'd want to write one like that, particularly in the context of a configuration properties bean. Perhaps a penny would drop for me if you shared the real-world problem that prompted this issue?

[dsyer]

The real life use case was this (it's not really very different to the test):

    @Component
    protected static class RefreshScopeBeanDefinitionEnhancer
            implements BeanDefinitionRegistryPostProcessor {

        /**
         * Class names for beans to post process into refresh scope. Useful when you don't
         * control the bean definition (e.g. it came from auto-configuration).
         */
        private Set<String> refreshables = new HashSet<>(
                Arrays.asList("com.zaxxer.hikari.HikariDataSource"));

        private Environment environment;

        public Set<String> getRefreshable() {
            return this.refreshables;
        }

        public void setRefreshable(Set<String> refreshables) {
            if (this.refreshables != refreshables) {
                this.refreshables.clear();
            }
            this.refreshables.addAll(refreshables);
        }

        public void setExtraRefreshable(Set<String> refreshables) {
            this.refreshables.addAll(refreshables);
        }
...

It's easy to workaround (as above, or by not having a setter at all), but hard to know why it doesn't work if you just write the setter assuming that the incoming collection was a different one.

[philwebb]

The more I think about this, the more I feel like a fresh collection if you have a setter is the correct approach. We went down the mutate route with the assumption that the collection might be special. For example, it might be a private subclass that does something extra. The thing is, if you have a special subclass _and_ a setter, your setter needs to deal with that anyway.