I recently had an issue in a project that was calling app.setDefaultProperties(Map) and passing in an immutable map. Worked great, until we integrated with a cloud config server, in which case, it was trying to put() in an immutable map, which didn't go so well.
I copied the code from app.setDefaultProperties(Properties), which created an internal HashMap, and populated it with the contents. This seems like the safer approach, rather than passing in the data structure that is to be used for the inner workings of the properties system.
With the old approach, I could pass in ANY implementation of Map, including ones that make get/put non-functional, and Spring will try to use them. The new approach fixes that by normalizing the internal data structure used for properties, and hiding that from the outside.
Comment From: pivotal-issuemaster
@AaronTraas Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-issuemaster
@AaronTraas Thank you for signing the Contributor License Agreement!
Comment From: AaronTraas
I can write tests. Just to clarify the scope, I'm thinking two tests:
- Sets the properties with an immutable map, tries to set a property, and setting the property works
- Sets a property with a new hashmap, then try to set the property of the original hashmap, and compare it with the property in the system, and assert that they should not be the same
Do you think that would cover the regression? I can hammer that out this evening if so.
Comment From: wilkinsona
I was thinking of a test that covers what Spring Cloud Config server is doing. It would fail without your changes and pass with them. The test would need to get the defaultProperties property source from the Environment, call getSource() and then try to put a property into the source.
Comment From: AaronTraas
Gotcha. Thanks for the clarification.
I was able to make two tests, both of which would fail against the current code, but do not fail with my changes.
Comment From: AaronTraas
One last question -- did I make this in the right branch? This really should be for everything going forward, and could also be back-ported to 2.1, etc.
Comment From: wilkinsona
No need to worry about the branch. We can cherry-pick the change back if needed and then merge it forwards.
Comment From: wilkinsona
We discussed one and I’m afraid we’ve decided not to merge it. In addition to the possibly breaking change I described above, the team also pointed out that users may want the current behaviour where an immutable map remains immutable. We can’t fix your specific problem without changing that behaviour for everyone. If you need the default properties to be mutable, we think you should pass in a mutable map. Thank you anyway for the proposal.
You may want to consider raising this with the Spring Cloud Config project. They may be able to do something to cope more elegantly with a situation where the default properties are immutable.