[
https://issues.apache.org/jira/browse/GROOVY-8101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15891181#comment-15891181
]
Nick Knowlson commented on GROOVY-8101:
---------------------------------------
Okay, I can see how it would be expected behaviour if you assume that plus has
to be implemented using clone(). But I think that makes the method less useful:
It means that any time you use plus, you must know the specific implementation
of the left hand collection or you may be introducing a bug. You can't safely
write code that operates on interfaces with this method.
> 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) // will fail
> 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 well.
> *Workaround for now:*
> {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' // will fail
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
