Groovy
  1. Groovy
  2. GROOVY-3316

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.6-rc-2
    • Fix Version/s: 1.6-rc-3
    • Component/s: None
    • Labels:
      None

      Description

      See http://www.nabble.com/findAll-and-Lazy-loaded-Hibernate-JPA-collections-broken-in-Groovy--1.6-td21682669.html

      This issue is critical, we either need to revert to the original behavior or find a solution that supports the new behavior whilst not breaking specialized collection types like those used in Hibernate

      1. conservativeCloneAndCreate.patch
        6 kB
        paulk_asert
      2. proposed-patch.txt
        5 kB
        James P. White

        Activity

        Hide
        James P. White added a comment - - edited

        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.

        Show
        James P. White added a comment - - edited 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.
        Hide
        James P. White added a comment -

        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.

        Show
        James P. White added a comment - 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.
        Hide
        James P. White added a comment -

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

        Show
        James P. White added a comment - Here's the proposed fix as a patch for the 1.6 branch.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        James P. White added a comment -

        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.

        Show
        James P. White added a comment - 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.
        Hide
        James P. White added a comment -

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

        Show
        James P. White added a comment - So what's the verdict? Do I commit this to 1.6 (in expectation of getting into 1.6.0) or what?
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment - - edited

        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'
        
        Show
        Paul King added a comment - - edited 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'
        Hide
        Paul King added a comment -

        Patch to dumb down clone/creation of collections attached. Should fix Hibernate. Someone will need to test.

        Bad that above examples won't work for now as well as others, e.g. this is now broken:

        def q = [1, 2, 3] as LinkedBlockingQueue
        q += 4
        assert q.class == LinkedBlockingQueue
        
        Show
        Paul King added a comment - Patch to dumb down clone/creation of collections attached. Should fix Hibernate. Someone will need to test. Bad that above examples won't work for now as well as others, e.g. this is now broken: def q = [1, 2, 3] as LinkedBlockingQueue q += 4 assert q.class == LinkedBlockingQueue
        Hide
        Guillaume Delcroix added a comment - - edited

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

        Show
        Guillaume Delcroix added a comment - - edited The grails app attached to this issue should exhibit the Hibernate problem: GRAILS-3831
        Hide
        Guillaume Delcroix added a comment -

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

        Show
        Guillaume Delcroix added a comment - With the latest changes in GROOVY_1_6_0 branch, the sample Grails application from GRAILS-3831 now pass the tests successfully.
        Hide
        James P. White added a comment -

        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.

        Show
        James P. White added a comment - 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.
        Hide
        Jochen Theodorou added a comment -

        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

        Show
        Jochen Theodorou added a comment - 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
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.

          People

          • Assignee:
            Jochen Theodorou
            Reporter:
            Graeme Rocher
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development