[
https://issues.apache.org/jira/browse/GROOVY-8101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15893588#comment-15893588
]
Nick Knowlson commented on GROOVY-8101:
---------------------------------------
> The current implementation is our best (and by no means perfect) compromise
> on achieving that given that the Java collection api doesn't have rigid
> enough conventions in place to allow us to enforce that. The oldCollection
> could be a Set, List, Stack etc. or some arbitrary custom extension class. It
> may or may not have a no-arg constructor. Indeed, it might have no public
> constructors and use factory methods.
That is a good point. I recognize that you are in a tight spot as far as design
constraints go.
> Relying on a clone() method which obeys the method's stated conventions has
> been a reasonable approach for us to adopt.
The part of me that wants to write robust software really doesn't want to rely
on libraries I don't have control over to follow a convention. But with the
design goals of running on the JVM, replacing the '+' operator and keeping
backward compatibility I can see how there might not be many/any other options.
> While we could try to code around poorly designed libraries, it isn't our
> first preference.
I think we are both feeling this pain right now from the same library, I
understand the sentiment!
> But if you can think of some improvements, we welcome any suggestions or PRs.
I have two suggestions that I think could help me / other people in this
situation:
* Updated documentation on the Groovy default methods that rely on clone()
internally. As you said, it is your best compromise on achieving your goals
(and I can't see a better one), but I would like to opt-in to that with full
knowledge.
* Some kind of {{concat()}} method defined against {{Map<K, V>}} that doesn't
have the same type preserving constraints. I realize that if you're going to
add one to {{Map}} that it may be better to add one to all standard collection
interfaces and that this could turn into a larger feature. I have no
expectation of this happening from just me requesting it, but it sure would be
nice.
Thank you both for your time, [~paulk] and [~blackdrag]
> 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)
