Nick Knowlson created GROOVY-8101:
-------------------------------------
Summary: DefaultGroovyMethods.plus() copies internal state of the
left Map
Key: GROOVY-8101
URL: https://issues.apache.org/jira/browse/GROOVY-8101
Project: Groovy
Issue Type: Bug
Components: groovy-runtime
Affects Versions: 2.4.8
Reporter: Nick Knowlson
Priority: Minor
When upgrading Tomcat to a newer version (8.0.39) we discovered a bunch of new
exceptions being thrown in a Grails application we were using with it. The root
cause ended up being that plus() was copying private internal state of the
specific Map implementation, and so couldn't be used to combine two Maps
together.
Here is an example that shows the problem.
*Setup:*
{code}
// Simpler stand-in for classes like org.apache.catalina.util.ParameterMap
// Important part is: it is a specific implemenation of a map with private
fields
class ParameterMap<K,V> extends LinkedHashMap<K,V> {
private boolean locked = false
public void setLocked(boolean locked) {
this.locked = locked
}
@Override
public V put(K key, V value) {
if (locked) {
throw new IllegalStateException('class is locked');
}
return (super.put(key, value));
}
public void putAll(Map<? extends K,? extends V> map) {
if (locked) {
throw new IllegalStateException('class is locked');
}
super.putAll(map);
}
}
// Stand-in for javax.servlet.ServletRequest.getParameterMap
// Important part is: it is a method that returns a generic (as far as the
caller can tell) Map of values
Map<String, String> getParameterMap() {
def internalMap = new ParameterMap<String, String>(['1': 'one', '2':
'wrong'])
internalMap.setLocked(true)
return internalMap
}
{code}
*Example Scenario:*
{code}
// We have some existing data that we don't want to change, but we also have
some more specific information that should override that existing data.
// We need to combine two maps and return a new one - this is what plus() was
created for, right?
Map<String, String> parameterMap = getParameterMap()
Map<String, String> newData = ['2': 'two', '3': 'three']
// Not this time! This line fails with an IllegalStateException, message 'class
is locked'
// Even though (from calling code's perspective) we are just adding two generic
Maps together
//def combinedMaps1 = parameterMap.plus(newData)
//assert combinedMaps1.get('2') == 'two'
{code}
Why? plus() isn't supposed to mutate the original collection. Turns out it
internally calls cloneSimilarMap(), which clones the whole object including the
class right down to the state of private variables.
Code using plus() should not have to account for plus() doing this - plus()
accepts generic Maps as its arguments, cloning the internal state of objects
like this breaks its contract and prevents encapsulation from working.
*Workaround:*
{code}
// Need to manually create a new Map<String, String> from the existing
Map<String, String> and then add the new data
def combinedMaps2 = parameterMap.collectEntries({key, value -> [key, value]
}).plus(newData)
assert combinedMaps2.get('2') == 'two'
// Can't just change order they are combined in - we want values from right map
to take precedence over values from left map with same key
def combinedMaps3 = newData.plus(parameterMap)
assert combinedMaps3.get('2') == 'two'
{code}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
