UriComponentsBuilder.uriComponents doesn't properly clone unmodifiable queryParams [SPR-17256]

[spring-projects-issues]

Ruslan Stelmachenko opened SPR-17256 and commented

When UriComponentsBuilder.uriComponents clones queryParams from given UriComponents, it leaves them as unmodifiable MultiValueMap.

This prevents to futher adding query params with the same name into the builder.

For example:

UriComponentsBuilder
		.fromUriString("http://localhost:8081")
		.uriComponents(UriComponentsBuilder.fromUriString("/{path}?sort={sort}").build())
		.queryParam("sort", "another_value")
		.build();

The .queryParam("sort", "another_value") line throws an exception on attempt to add another value to unmodifiable MultiValueMap.

While this code works:

UriComponentsBuilder
		.fromUriString("http://localhost:8081/{path}?sort={sort}")
		.queryParam("sort", "another_value")
		.build();

The interested part of code to investigate this problem is:

org.springframework.web.util.HierarchicalUriComponents#copyToUriComponentsBuilder
Here is line builder.queryParams(getQueryParams()) which sets the builder's queryParams to unmodifiable MultiValueMap.

I think we can clone unmodifiable queryParams here to be modifiable, or maybe better inside org.springframework.web.util.UriComponentsBuilder#queryParams methods to make this logic independent of UriComponents implementation.

---

To make the intention more clear: this pattern of use UriComponentsBuilder is actually used in org.springframework.web.util.DefaultUriBuilderFactory.DefaultUriBuilder#initUriComponentsBuilder where I first faced this problem.

It allows to set baseUrl and then uriString to concatenate them.
So this code also throws an exception:

new DefaultUriBuilderFactory("http://localhost:8081")
		.uriString("/{path}?sort={sort}")
		.queryParam("sort", "another_value")
		.build(params);

---

Affects: 5.0.8, 5.1 RC2

Issue Links:

• ConcurrentModificationException when calling SimpMessagingTemplate.convertAndSend [SPR-13185] #17777 ConcurrentModificationException when calling SimpMessagingTemplate.convertAndSend

Referenced from: commits 2820831, 1d58fac, c06b952

Backported to: 5.0.10

[spring-projects-issues]

Ruslan Stelmachenko commented

When I more think about this problem, I think maybe it is even better to fix org.springframework.util.LinkedMultiValueMap#putAll method to make it always copy underlying lists into new LinkedList's...

This is how it look now:

@Override
public void putAll(Map<? extends K, ? extends List<V>> map) {
     this.targetMap.putAll(map);
}

At now, when we call this method passing map where values are unmodifiable lists, this multimap becomes strange half-modifiable multimap:

• Some underlying lists are unmodifiable, and some are modifiable.
• We can add a value to this multimap when there is no value with this key inside yet.
• We can't add a value to this multimap when there is already a value with this key inside (but only when the key was part of previously putted multimap).

Also, after calling this method, some underlying lists can be not LinkedList's, because it accepts just a map of lists. Any lists. But this is LinkedMultiValueMap so I think all underlying lists should be LinkedList's).

So I think we should copy lists (values) of given map into new LinkedList's in putAll method instead of just doing this.targetMap.putAll(map), which just puts all map values as is.

The same is true for org.springframework.util.LinkedMultiValueMap#put method, which also just puts passed in list (which can be any implementation, including immutable) into targetMap.

Edit:

Actually, the same also appies for LinkedMultiValueMap's constructor. I see #17777 which introduced new deepCopy method into code, which uses LinkedMultiValueMap, but I think that all LinkedMultiValueMap's methods, that receive List or Map<?, List> as method argument, should do preventive copy of List by itself because we cannot guarantee that it's not immutable list (and if it is immutable, then all futher add/addAll methods of this multimap will fail).

After all, the use of List in MultiValueMap to store multiple values of single key, is just an implementation details, and when we use some List that came from outside to fill the map, we should not rely on implementation of this external list to store the internal state.

That is just an IMHO, of course. The problem with UriComponentsBuilder can be solved using deepCopy from the UriComponentsBuilder's code, but I think it will be just a workaround for misbehaving MultiValueMap.

[spring-projects-issues]

Juergen Hoeller commented

LinkedMultiValueMap's implementation of the Map methods adhere to standard Map semantics as far as possible, not taking defensive copies of the List values at that level. However, as of 5.0, we have MultiValueMap.addAll methods with multi-value copy semantics which we should simply use in UriComponentsBuilder.queryParams.

[spring-projects-issues]

Ruslan Stelmachenko commented

The standard Map semantic is unaware of value types in the map. But LinkedMultiValueMap is definitely aware that these are multiple values (and not a single value), that should be stored into multimap at specified key.

For map - the value argument of put/putAll methods is just a opaque value. But for multimap it is multiple values, not a single value.

I don't see any violation of contract if LinkedMultiValueMap's put/putAll will create a copy of passed in value. But I agree that this can be breaking change for some LinkedMultiValueMap's client code that rely on the fact that passed in List instance should be the same.

Still, such implementation of put/putAll breaks the contract of MultiValueMap itself, because after calling put passing arbitrary list implementation (immutable for example), we can no longer call add/addAll etc methods. They will throw for no reason (from the point of view of caller). Because for the MultiValueMap (in contrast to map) storing multiple values in the list is just an implementation details and not a requirement to store them in the passed in list.

[spring-projects-issues]

Juergen Hoeller commented

Well, semantically a Map.put call should store the given value as-is, with a subsequent Map.get call finding the same value again. By defensively creating copied List instances internally, we'd violate that standard Map assumption.

As you noted, this might be a breaking change for existing usage where specific List instances are given intentionally, possibly even for the purpose of rejecting further values for certain keys. In any case, for our own usage of LinkedMultiValueMap, we should make sure to always leave it in proper modifiable state through avoiding accidental use of putAll and co.

[spring-projects-issues]

Ruslan Stelmachenko commented

I agree. Thanks you, Juergen.

[cdalexndr]

Also affects 4.3.24.
If this version is still supported can you push the fix to 4.x versions?