Spring Binding support for copies of collections to be returned from canonically-named accessor methods [SPR-13749]

Adam Burley opened SPR-13749 and commented

I got directed here by the Spring Boot team. If you have an accessor (getter) in a @ConfigurationProperties-annotated class to retrieve a collection, and if you call the copy constructor in that accessor, the returned collection appears to be empty.

Example YAML file ("example.yml"):

one:
   two:
      - three
      - four
      - five

Example Spring Controller:

@RestController
public class ExampleController {
    @Autowired
    private ExampleYamlStore exampleYamlStore;

    @RequestMapping({"/*"})
    public String index() {
        List<String> two = exampleYamlStore.getTwo();
        String str;
        if (two.size() == 0) {
            str = "Size was zero";
        } else {
            str = two.get(0) + ","
                    + two.get(1) + ","
                    + two.get(2);
        }
        return "RESULT: " + str;
    }
}

Example YAML store (without copy constructor in accessor):

@Component
@ConfigurationProperties(locations = "classpath:/example.yml", prefix = "one")
public class ExampleYamlStore {
    private List<String> two = new ArrayList<>();

    public List<String> getTwo() {
        return two; // ***MARKED LINE***
    }

    public void setTwo(List<String> two) {
        this.two = new ArrayList<>(two);
    }
}

Running this code and hitting http://localhost:8080/foo will give the response "RESULT: three,four,five" as expected.

However if you change the marked line above to:

return new ArrayList<>(two);

Then hit http://localhost:8080/foo - you will get the response "RESULT: Size was zero".

Spring should allow consumers to return a copy of a collection, to prevent any caller from modifying the internal collection. Are you retrieving the list somewhere internally and then modifying it but not setting it back onto the @ConfigurationProperties-annotated class?

I am using the latest version of Spring Boot (1.3.0) and Java 1.7. I got told by the team at Spring Boot that "Binding is actually handled by the core Spring Framework."

---

Affects: 4.2.3

Comment From: spring-projects-issues

Adam Burley commented

"Accessor" is another word for "getter". Therefore I don't quite understand the revised title. I'll rephrase.

I presume your intention was to highlight that the current behaviour is not considered (by your team) to be a bug but merely a current restriction of the Spring Framework that all YAML stores must provide accessors which expose the internal object.

Comment From: spring-projects-issues

Juergen Hoeller commented

Thanks for the rephrasing, Adam Burley, that's indeed what I had in mind. The current YamlProcessor implementation operates on a flattened data structure which expects the raw collections in it, as far as I can see. We may rearchitect this, of course, but only really for 4.3.

That said, if you have an idea for a simple enough patch to the current YamlProcessor, we're certainly open to rolling it into 4.2.x as well.

Juergen

Comment From: spring-projects-issues

Juergen Hoeller commented

Thanks for the rephrasing, Adam Burley, that's indeed what I had in mind. The current YamlProcessor implementation operates on a flattened data structure which expects the raw collections in it, as far as I can see. We may rearchitect this, of course, but only really for 4.3.

That said, if you have an idea for a simple enough patch to the current YamlProcessor, we're certainly open to rolling it into 4.2.x as well.

Juergen

Comment From: spring-projects-issues

Adam Burley commented

Thanks...to be honest I don't know much about the internal implementation of binding. From looking at the code, it seems that the real work happens inside AbstractNestablePropertyAccessor, in particular the method setPropertyValue(PropertyTokenHolder,PropertyValue). In very very simplified terms, what it does is:

• First it gets the property value to be set via getPropertyValue(PropertyTokenHolder). This is ultimately resolved via BeanWrapperImpl.BeanPropertyHandler.getValue() (via the abstract signature PropertyHandler.getValue()) which looks up a "read method" and then invokes it
• Second, it adds the value to the list (list.add(convertedValue)) (and similar handling exists for other collections)

The third obvious step would seem to be to do the opposite of the first step. In other words, invoke a method like setRealPropertyValue(PropertyTokenHolder,Object) (you can call it whatever you like) which mirrors (at least the first few lines of) getPropertyValue(PropertyTokenHolder). After getting the PropertyHandler, you can call the abstract method PropertyHandler.setValue(Object,Object) (implementation in BeanWrapperImpl.BeanPropertyHandler again, mirroring the first step) to set the list back onto the store object, passing this.object as the first argument. (The setValue method looks up a "write method" and then invokes it, again mirroring the first step.)

So I think adding this third step would be a start. Of course, I have probably vastly oversimplified the problem and not considered lots of edge cases, etc.

Comment From: spring-projects-issues

Stéphane Nicoll commented

Another example of such problem with our Flyway integration

Comment From: spring-projects-issues

Stéphane Nicoll commented

Of course, I have probably vastly oversimplified the problem and not considered lots of edge cases, etc.

You already did a pretty deep analysis. Just to insist on what you wrote already, this has nothing to do with YAML and can be easily reproduced with simple keys (I just did). The binding mechanism in the framework expects the object returned by the getter to be mutable and a whole lot of features are built based on that. Having worked quite deeply in that area recently, I am really not keen to support that use case I am afraid: I think we should be able to add the feature but I am convinced we'll bring additional regressions.

Juergen Hoeller thoughts?

Comment From: spring-projects-issues

Juergen Hoeller commented

From my perspective, it is sensible to require access to the original collections for such bindings. After all, the binding paths explicitly reference specific collection elements there which does suggest taking the collection and modifying it, instead of completely replacing it. If the corresponding getter method happens to return a copy or unmodifiable decorator, you could switch to field access instead, or provide a non-public getter/setter pair (specifically named) just for binding purposes.

Note that Spring does not generally require access to the underlying collections. It's rather just about binding paths that go into a specific collection to replace a specific element.

Juergen

Comment From: spring-projects-issues

Adam Burley commented

you could switch to field access instead, or provide a non-public getter/setter pair (specifically named) just for binding purposes

Sorry, I don't know how to do that... I would certainly be open to providing other methods for Spring to call, but I don't know how I would let Spring know to call those rather than the real getters and setters?

For example:

    public List<String> getTwo() {
        return new ArrayList<>(two);
    }

    public void setTwo(List<String> two) {
        this.two = new ArrayList<>(two);
    }

    private List<String> someOtherMethod() { // This method is only called by Spring
        return two;
    }

Is there some way to specify (e.g. by using a method-level annotation) that Spring should call someOtherMethod rather than getTwo as the "getter"?

Comment From: spring-projects-issues

Juergen Hoeller commented

At this point, it's all driven by the property name: Your external property reference would have to be named e.g. "twoInternal" for a "getTwoInternal" method to be called... The more common solution is to use direct field access in that case, referring to the field names, e.g. on a DataBinder using the initDirectFieldAccess method. Boot's configuration properties might not support that mode though; I'll check with Stéphane.

Juergen

Comment From: spring-projects-issues

Stéphane Nicoll commented

It does not (see #1254). configuration properties add an extra layer with the meta-data generation: I am not keen (to say the least) to open the possibility of mixed use cases at runtime. That would make property discovery quite complicated with lots of edge case. That's why I personally like the Javabean option: this is a (public) configuration option after all so requiring a regular Javabean property makes sense to me.

Comment From: spring-projects-issues

Adam Burley commented

So am I basically stuck with doing this kind of thing:

private List<String> two = new ArrayList<>();
private List<String> getTwo() {
    return two;
}
public List<String> publicGetTwo() {
    return new ArrayList<>(two);
}

Comment From: spring-projects-issues

Stéphane Nicoll commented

I don't know what you mean by "stuck". Given that you bound "an object" to the environment, I think it's reasonable to ask for a certain structure for that object. If you want your public API not to conform to that structure, you can bound a package protected object or bind your configuration using a different mechanism.

I agree that not being able to use direct field binding with Spring Boot is problematic (because you could use reflection to hide said requested structure). Having said that, you're far from being stuck IMO.

Comment From: spring-projects-issues

Adam Burley commented

you can bound a package protected object

Okay, so you mean something like this?

@Component
@ConfigurationProperties(locations = "classpath:/example.yml", prefix = "one")
/*default*/ class ExampleYamlStorePP {
    private List<String> two = new ArrayList<>();

    public List<String> getTwo() {
        return two;
    }

    public void setTwo(List<String> two) {
        this.two = new ArrayList<>(two);
    }
}

@Component
public class ExampleYamlStore {
    @Autowired
    private ExampleYamlStorePP exampleYamlStorePP;

    public List<String> getTwo() {
        return new ArrayList<>(exampleYamlStorePP.getTwo());
    }

    public void setTwo(List<String> two) {
        exampleYamlStorePP.setTwo(two);
    }
}

Comment From: spring-projects-issues

Stéphane Nicoll commented

More or less. You don't necessarily need to keep them consistent once the app has started. You can do the other way around and initialize your object on @PostConstruct based on what was bound from the environment. As I said, many options.

Comment From: spring-projects-issues

Stéphane Nicoll commented

After a throughout consideration, we decided to reject this improvement. Binding is operating on the Environment and external property source (configuration) so it should have access to the original collections.

Comment From: mwisnicki

If you ever decide to reopen and implement interface binding, please also support default property values via default interface method :)