Find a solution to the collection coping problem involving findAll, grep etc. that causes Hibernate lazy loading to break

I propose to change DefaultGroovyMethodsSupport.createCollectionFromClass and createMapFromClass to be more selective about the classes that they try the "instance's class constructor" scheme on. For starters they will be limited to java.util.AbstractCollection and java.util.AbstractMap respectively.

Java 1.5 classes that implement java.util.Collection but do not extend java.util.AbstractCollection:

• java.beans.beancontext.BeanContextSupport
• java.util.concurrent.CopyOnWriteArrayList

Java 1.5 classes that implement java.util.Map but do not extend java.util.AbstractMap:

• java.util.jar.Attributes
• java.util.Hashtable
• java.awt.RenderingHints
• javax.management.openmbean.TabularDataSupport

It may make sense to extend the methods to some (all?) of these.

I've committed this proposed fix to the trunk (cs15169).

I went ahead and added the Hashtable check along with AbstractMap because it was already being handled by createSimilarMap (whose instanceof checks are probably superfluous now).

In addition I made the same change to the cloneXXXFromClass methods since they do the same thing as the createXXXFromClass methods. It is quite strange that there is all that code doing cloning but no use of Object.clone which is specifically for performing this kind of shallow copy.

Here's the proposed fix as a patch for the 1.6 branch.

I haven't had time to digest all this - being on hols this week. I agree something needs to be looked at and I appreciate Jim's suggestions but I am not sure (given my really quick glance) that I think this does what we want. It means that findAll etc. doesn't work for my own collection like classes if I implement Map or List but don't extend from the Abstract collections? Not sure this is what we want but need to look further.

I agree this is only a part of an answer. Not only does this treat AbstractCollection and AbstractMap in a special way, it still isn't really any assurance or control on whether the particular class is willing to have its default constructor used.

The benefit of doing this is it is simple and safe, at least in terms of not causing new problems than the current 1.6 approach.

As I discuss in the groovy-dev thread, I think a proper solution would involve something MOPish using DGM along with categories and/or a marker interface.

Of course something that addresses the Hibernate PersistentCollection problem in a simple and general (enough) way would be dandy. This was just the first thing that occurred to me that seemed plausible.

So what's the verdict? Do I commit this to 1.6 (in expectation of getting into 1.6.0) or what?

I am just trying to replicate the issue now (as in next day or two) to see if I can think of an alternate solution. I think the current proposed solution is (partially) broken but agree that the original is (partially) broken too.

Here are two scripts that work fine with RC1 and RC2 but fail with the current quick fix in TRUNK:

import com.google.common.collect.HashBiMap
@Grab(group='com.google.code.google-collections', module='google-collect', version='snapshot-20080530')
def getFruit() { [grape:'purple', lemon:'yellow', orange:'orange'] as HashBiMap }
assert fruit.lemon == 'yellow'
def citrus = fruit.findAll{ k, v -> k != 'grape' }
assert citrus.size() == 2
assert citrus.inverse().yellow == 'lemon'

and

class Person { def first, last }
def bp = new Person(first:'Brad', last:'Pitt')
def tc = new Person(first:'Tom', last:'Cruise')
class People {
    @Delegate private List delegate = []
    def getNames() { iterator().collect{"$it.first $it.last"}.join(", ") }
}
def actors = new People()
[bp, tc].each{ actors.add(it) }
assert actors.names == 'Brad Pitt, Tom Cruise'
def selected = actors.findAll{ it.first.size() == it.last.size() }
assert selected.names == 'Brad Pitt'

The grails app attached to this issue should exhibit the Hibernate problem: GRAILS-3831

With the latest changes in GROOVY_1_6_0 branch, the sample Grails application from GRAILS-3831 now pass the tests successfully.

How is it that the change is made on the 1.6.0 branch first (and so far only)? It should go to trunk first, or at least GROOVY_1_6_X.

because it seems as a reasonable short term fix. But for the other branches we can look into this issue a bit more. Paul wanted to think about this a bit more for example. The applied patch is more or less a roll back. As we discussed on the list I considered a roll back if no solution was found till today. Maybe cloning this issue would be good... with fix targets for 1.6.1 and 1.7

I am thinking though that we should align all of the codebases to be what we have in 1.6.0 and then explore modifications from there. Otherwise I know I will find discussions confusing.