Spring-boot: Binding configuration properties to collection cannot replace individual items

[dsyer]

This appears to be a regression in Spring Boot 2.0 (Spring Cloud Context relies on the old behaviour for rebinding to @ConfigurationProperties when the environment changes).

Start with a @ConfigurationProperties that has a List<String> called messages, and 2 items listed in application.properties (messages[0]=one, messages[1]=two). If you add a property source to the environment with messages[0]=foo you expect to be able to rebind and get [foo, two], but failing tests are showing [foo].

I haven't had a chance to dig into what changed. Ignored the tests for now.

[philwebb]

There were quite a few quirks with the old binder and the logic for combining lists was particularly problematic. It was especially hard to describe how YAML lists would get merged. The new one is intentionally much stricter about lists. Once [0] has been found in a property source, all list elements must also come from that source.

If you add a property source to the environment with messages[0]=foo you expect to be able to rebind and get [foo, two], but failing tests are showing [foo].

Although it might lead to some repetition, I prefer the new approach. It's especially helpful if you have something like this:

application.yml:

mylist:
  - foo
  - bar
  - baz

application-prod.yml:

mylist:
  - bin
  - bam

Since you no longer end up with baz left over from the first list.

What's the scenario where you'd end up adding only messages[0]=foo?

[philwebb]

I've flagged this one for team attention, I'm interested to know which approach people prefer.

[dsyer]

I think it's going to jar a bit, because I know we fixed this a few times to make it possible to append to lists from an external property source. It seems like a reasonable thing to want to do. On the other hand I appreciate that the rules need to be specified. Maybe we could be less strict and allow appending somehow (as well as replacing). I don't know how that would work really (it's not really possible to signal in the YAML what behaviour you expect).

The problem I see is that the user doesn't need, and shouldn't need, to know about property sources - there's an Environent which you bind to, and property sources are a detail of the implementation. If environment.getProperty("messages[0]").equals("foo") and environment.getProperty("messages[1]").equals("bar") then it should bind to a list called messages. Otherwise the user has to know too much about the internals.

Footnote: the "same property source" rule doesn't apply to maps. You can bind to Map<String,String> (for instance) with property values drawn from multiple sources. That makes total sense too, it just highlights that the list behaviour is surprising.

[wilkinsona]

I think of all of the entries in a list as a single value and of each entry in a map as a separate value. That means that the new approach for lists makes more sense to me than the old approach. It also means that I don't find the difference between maps and lists to be surprising; in fact it's less surprising to me than the previous behaviour.

[dsyer]

OK, so List<String> is probably artificial. If it's List<Foo> I definitely want to be able to set properties on individual items in the list without caring about which property source I'm in (e.g. test.foos[0].name=bar). Each item in the list is a separate value (just like in your map example), and I need to be able to address and dereference them individually.

[philwebb]

I think the List<Foo> example was a another one that caused a great deal of confusion. Some people want to do this:

application.properties

test.foo[0].name=bar
test.foo[0].role=user
test.foo[0].age=21

application-dev.properties

test.foo[0].role=admin

And expect to get name=bar, role=admin, age=21.

Other times we've seen people do this:

application.properties

test.foo[0].name=bar
test.foo[0].role=user
test.foo[0].age=21

application-dev.properties

test.foo[0].name=baz
test.foo[0].role=admin

And be surprised that the age attribute remains from the first element.

[philwebb]

Perhaps we need to gather some concrete use-cases to help us decide what's best.

[wilkinsona]

The current behaviour of using a single source for configuring the items in a list makes completely overriding the contents of a list easy and modifying a list, albeit with some tedious repetition, possible. The old behaviour of using multiple sources makes modifying a list easy and completely overriding the contents of a list impossible.

If we're just picking between old vs new, I think the new approach is the clear winner. If we want to spend some more effort on this I think it should be post-M1 and it should focus on trying to avoid the repetition when modifying a list without hurting the ease with which a list can be overridden. Quite how we do that, I don't know. It'll probably require some new syntax and require us to look through every source whenever we're binding a list in case there's an "additive" property configured somewhere.

[philwebb]

See #6793 for an example of confusion caused by the old system.

[dsyer]

If we can spend more effort post-M1 that would be good. Maybe a "merge policy" that can be tweaked globally or per named collection, with a default that has the current "new" behaviour (i.e. no merge) . I also noticed exceptions being thrown for unbound list elements, which is going to be something that people might want to switch off. Example:

...
Caused by: org.springframework.boot.context.properties.bind.UnboundConfigurationPropertiesException: The elements [test.names[1]] were left unbound.
    at org.springframework.boot.context.properties.bind.IndexedElementsBinder.assertNoUnboundChildren(IndexedElementsBinder.java:121)
    at org.springframework.boot.context.properties.bind.IndexedElementsBinder.bindIndexed(IndexedElementsBinder.java:95)
    at org.springframework.boot.context.properties.bind.IndexedElementsBinder.bindIndexed(IndexedElementsBinder.java:76)
...

[poznachowski]

I'm having problem with understanding how 'merging' map of maps property should work and it seems that it may be connected with your discussion here. Could you please share your take on https://stackoverflow.com/questions/45859131/spring-boot-merging-map-of-map-property ?

[wilkinsona]

I'm not convinced that we should encourage users to do this. Trying to override part of the configuration of a particular element of a list feels very brittle to me. If an earlier property source is changed such that a list has been reordered, or an element has been added before the one being targeted, then the configuration of the wrong item is going to be overridden. Completely overriding the contents of the list, which is now possible, is a much more robust solution and, IMO, is what we should promote.