[
https://issues.apache.org/jira/browse/GROOVY-8101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15892700#comment-15892700
]
Nick Knowlson edited comment on GROOVY-8101 at 3/2/17 6:09 PM:
---------------------------------------------------------------
[~blackdrag]
> if you provide a clone method (and you do through your super class) we depend
> on it well behaving
Right - I understand that the current implementation of plus() depends on that,
I am trying to get across the consequences of that decision on anyone using
plus().
> The point being here is that you should not have to depend on checking the
> source code, you should depend on a proper clone method implementation.
I agree that I should not have to depend on checking the source code. But
having plus() depend on a proper clone method implementation means the onus
*is* now on me (as someone using plus()) to go check the source code of
third-party libraries to make sure they implement clone() and implement it
correctly before using plus() with them. That in addition to not taking the
return type of generic methods like {{ServletRequest.getParameterMap}} at face
value and figuring out (through source code inspection or debugging) what the
actual instance of the Map I'm using is.
Put another way: plus() relying on a proper clone() implementation means you
are making the people using plus() responsible for the clone() implementations
of all the third party collections they may use.
This would not be an issue if other methods (that did not expose private class
state) were relied on instead of clone().
It sounds like you are tied to plus() relying on clone() internally and do not
want to change that. Ideally I would like that to change so that I (and others)
can use plus() safely in the future. But if it won't change, what I am looking
for is at least an acknowledgement that you understand this decision places a
large burden on the person using plus(), and an update to the documentation so
that people at least know what they are getting into by relying on plus() for
collections.
was (Author: nickknw):
[~blackdrag]
> if you provide a clone method (and you do through your super class) we depend
> on it well behaving
Right - I understand that the current implementation of plus() depends on that,
I am trying to get across the consequences of that decision on anyone using
plus().
> The point being here is that you should not have to depend on checking the
> source code, you should depend on a proper clone method implementation.
I agree that I should not have to depend on checking the source code. But
having plus() depend on a proper clone method implementation means the onus
*is* now on me (as someone using plus()) to go check the source code of
third-party libraries to make sure they implement clone() and implement it
correctly before using plus() with them. That in addition to not taking the
return type of generic methods like {{ServletRequest.getParameterMap}} at face
value and figuring out (through source code inspection or debugging) what the
actual instance of the Map I'm using is.
Put another way: plus() relying on a proper clone() implementation means you
are making the people using plus() responsible for the clone() implementations
of all the third party collections they may use.
This would not be an issue if other methods (that did not expose class
internals) were relied on instead of clone().
It sounds like you are tied to plus() relying on clone() internally and do not
want to change that. Ideally I would like that to change so that I (and others)
can use plus() safely in the future. But if it won't change, what I am looking
for is at least an acknowledgement that you understand this decision places a
large burden on the person using plus(), and an update to the documentation so
that people at least know what they are getting into by relying on plus() for
collections.
> 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)
