Open mwisnicki opened 3 years ago
Turn's out it wasn't that hard to add using little known ConfigurationPropertiesBindHandlerAdvisor
@Configuration
public class ImmutableConfigurationSupport {
@Bean
ConfigurationPropertiesBindHandlerAdvisor configurationPropertiesBindHandlerAdvisor() {
return bindHandler -> new AbstractBindHandler(bindHandler) {
int immutable = 0;
private boolean isImmutableTarget(Bindable<?> target) {
var klass = target.getType().resolve();
return klass != null && klass.isAnnotationPresent(ConstructorBinding.class);
}
@Override
public <T> Bindable<T> onStart(ConfigurationPropertyName name, Bindable<T> target, BindContext context) {
if (isImmutableTarget(target))
immutable++;
return super.onStart(name, target, context);
}
@Override
public Object onSuccess(ConfigurationPropertyName name, Bindable<?> target, BindContext context, Object result) {
var object = super.onSuccess(name, target, context, result);
var targetClass = target.getType().resolve();
if (immutable > 0 && targetClass != null) {
if (object instanceof List && targetClass.isAssignableFrom(List.class))
return Collections.unmodifiableList((List<?>) object);
if (object instanceof Set && targetClass.isAssignableFrom(Set.class))
return Collections.unmodifiableSet((Set<?>) object);
if (object instanceof Map && targetClass.isAssignableFrom(Map.class))
return Collections.unmodifiableMap((Map<?, ?>) object);
}
return object;
}
@Override
public void onFinish(ConfigurationPropertyName name, Bindable<?> target, BindContext context, Object result) throws Exception {
super.onFinish(name, target, context, result);
if (isImmutableTarget(target))
immutable--;
}
};
}
}
Is this something the project would consider adding or should I create a library?
I'm glad the ConfigurationPropertiesBindHandlerAdvisor
is working, but I think we should probably look at doing something directly in the CollectionBinder
. I quite like the idea of using unmodifiable collections everywhere, but I'm worried a bit about back compatibility.
We need to discuss this a bit as a team before we make any final decision.
We discussed this today and decided that unmodifiable would be a good default but we need to provide an escape hatch for those that need mutable binding.
Isn't ConfigurationPropertiesBindHandlerAdvisor
such an escape hatch? :)
While it can achieve the same end result, it's too low-level and verbose to recommend as a way to restore the current behaviour for users who are relying on it.
I am interested to work in this issue. I have been working with Spring Mvc and Spring Boot based enterprise application for five years and every now and then I had a peek inside the framework code.
So far I have tried cloning the code, setting up the environment, ide. Now I am trying to build it. As a first timer I may need a few feedback and acquaintance with the process to get started.
Thanks, @rakibmail22. There's some documentation in the wiki on working with the code.
For this particular issue, I think we'll need changes to CollectionBinder
and MapBinder
so that the collections and maps that they create are immutable by default. Some updates to the corresponding tests (CollectionBinderTests
and MapBinderTests
) will also need to be made to check that the collections and maps created are now immutable.
We'll also need to provide a way for someone to opt out of this behaviour so that things behave as they do today. I'm not exactly sure how that should look at the moment. Perhaps an attribute on @ConfigurationProperties
.
@wilkinsona I looked into the CollectionBinder
where the initial Collection is created and values are bound. But after that in the returning process there are few conversions that are done. The bind
mehod of AggregateBinder
returns an Object
and that Object
in a later step with help of a cnverter is converted to the desired list. During this conversion process it fails if I make the Collection
immutable directly inside the CollectionBinder
. Still, I am trying to have a better understanding of the whole binding process.
Thanks for taking a look. No conversions should be necessary if the Collection
created by the binder is of the required type. How are you making the Collection
unmodifiable? I don't think Collections.unmodifiableCollection
will be adequate in this case as the result is a Collection
rather than something more specific such as a List
. Instead, I think it'll be necessary to do something like this:
@SuppressWarnings({ "unchecked", "rawtypes" })
private Collection<Object> unmodifiable(Collection<Object> collection) {
if (collection instanceof SortedSet) {
return Collections.unmodifiableSortedSet((SortedSet) collection);
}
if (collection instanceof Set) {
return Collections.unmodifiableSet((Set) collection);
}
if (collection instanceof List) {
return Collections.unmodifiableList((List) collection);
}
return Collections.unmodifiableCollection(collection);
}
I am trying something similar like below.
@Override
protected Collection<Object> merge(Supplier<Collection<Object>> existing, Collection<Object> additional) {
Collection<Object> existingCollection = getExistingIfPossible(existing);
if (existingCollection == null) {
return unmodifiable(additional);
}
try {
existingCollection.clear();
existingCollection.addAll(additional);
return unmodifiable(copyIfPossible(existingCollection));
}
catch (UnsupportedOperationException ex) {
return unmodifiable(createNewCollection(additional));
}
}
private Collection<Object> unmodifiable(Collection<Object> collection) {
if (collection instanceof SortedSet) {
SortedSet<Object> result = (SortedSet<Object>) collection;
return Collections.unmodifiableSortedSet(result);
}
if (collection instanceof Set) {
Set<Object> result = (Set<Object>) collection;
return Collections.unmodifiableSet(result);
}
if (collection instanceof List) {
List<Object> result = (List<Object>) collection;
return Collections.unmodifiableList(result);
}
throw new IllegalArgumentException("Unsupported Collection interface: ");
}
Two test cases failed in CollectionBinderTests
bindToCollectionWhenHasExistingCollectionShouldReplaceAllContents()
bindToCollectionWithNoDefaultConstructor()
The first one is failing because it is exactly checking for a LinkedList
The second one is failing with the following underlying error
Caused by: org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.util.Collections$UnmodifiableRandomAccessList<?>] to type [org.springframework.boot.context.properties.bind.CollectionBinderTests$MyCustomNoDefaultConstructorList] for value '[a, b, c, c]'; nested exception is java.lang.IllegalArgumentException: Could not instantiate Collection type: org.springframework.boot.context.properties.bind.CollectionBinderTests$MyCustomNoDefaultConstructorList at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:47)
I'm not sure why merge creates a new Collection
from additional
at the moment. additional
should be the result of calling bindAggregate
so I think it can just be used as-is. In other words, as long as bindAggregate
returns an unmodifiable collection, I think the catch
block in merge
can just return additional
. If there is a need for additional
to be turned into a new collection, @mbhave and @philwebb may recall the details.
There will be some cases where I don't think we should bind an unmodifiable collection (or Map). A property that's a LinkedList
would be one such case as I don't think we should go to the lengths of creating a custom LinkedList
subclass that's unmodifiable.
To get an immutable collection or map, I think the property should be declared as a Set
, SortedSet
, List
, Map
, etc. With the property's actual type being used to influence the creation of any new collections or maps. For example, if a Set
property is actually a TreeSet
then a TreeSet
is what should be created and populated before making it an unmodifiable Set
.
The binder's one of the most complex areas of Boot's code base so if this starts consuming more of your time than you have realised it would, please do feel free to step away.
I can't remember the details I'm afraid. Looks like the change was made for #9290. I think we can return additional
from that catch block.
yeah, I don't think creating a new collection is necessary in the catch block. Seems like an oversight.
Shall this be a separate issue or this change should be a part of the current one?
@rakibmail22 It's fine to change it as part of this issue. We can always separately apply the same change to other branches if we decide that it's worth it.
I'd like to also point out that, specifically for Map
types, Spring Boot configuration support is currently fetching the Map
via the getters in order to fill it in, rather than first building the Map
and injecting it. This prevents even manual workarounds for immutable configuration properties.
We tried records and the values don't get set (I presume because the record returns a copy via the getter). Then we tried a POJO with a CTOR which copied the input and made them unmodifiable (Map.copyOf()
). That produced a (not very helpful) message that there was no setter for the properties, "IllegalStateException: No setter found for property". Finally we tried just returning an unmodifiable reference via the getter and that also failed with that same message.
We had to give up on it, but this feature would really help us for this use case. We were using a map as we wanted to generically allow the override of default Kafka properties and did not want to have to manually replicate the entire set of properties.
Consider simple example:
Even though I'm trying to make immutable class here, Spring will unhelpfully inject mutable
ArrayList
.Now in a very simple example like this you can of course wrap it yourself in
Collections.unmodifiableList()
but doing so quickly becomes tedious especially when you nest collection types.Imagine the code to deeply freeze e.g.
Map<String, List<String>>
:(It's even worse when you just want to use
lombok.Value
or maybe Kotlin (didn't check it) to avoid writing boilerplate completely.Not sure why these property classes should ever be mutable but if desired one can add property to
@ConfigurationProperties
to control it. It should automatically be enabled when using@ConstructorBinding
though as that's a sure sign someone is trying to make immutable properties.