[
https://issues.apache.org/jira/browse/GROOVY-8101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15891342#comment-15891342
]
Nick Knowlson commented on GROOVY-8101:
---------------------------------------
> I think what Groovy is doing is proper, trying to return the same type and if
> cloning fails falling back to trying to find the most specific type (among
> the Map types known to it)
I agree that this part makes sense, I just don't agree that "trying to return
the same type" has to be connected to "copy the internal state of the original
map".
Say we did go with the solution of changing
{{org.apache.catalina.util.ParameterMap}} to provide a custom clone method - it
means if I want to safely use plus() I have to know another piece of
information: In addition to knowing which implementation of the Map<K, V>
interface I am using, I have to go look at the source code for that
implementation to make sure it implements clone() and then check that it resets
the state of all its private variables. It is all information at a level of
detail way below what plus() says it operates at, involving private variables
that aren't supposed to be exposed.
I don't want to do that extra work each time - I view plus() as a helpful
utility method that is supposed to save time and increase readability. I don't
want it to cost me extra time and introduce complexity.
> 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)
