groovy-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nick Knowlson (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (GROOVY-8101) DefaultGroovyMethods.plus() copies internal state of the left Map
Date Mon, 27 Feb 2017 23:50:45 GMT

     [ https://issues.apache.org/jira/browse/GROOVY-8101?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Nick Knowlson updated GROOVY-8101:
----------------------------------
    Description: 
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}

  was:
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}


> 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)

Mime
View raw message