groovy

Custom constructors added via metaclass are sometimes not cleaned up

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.1
  • Fix Version/s: 1.7.2, 1.8-beta-1
  • Component/s: None
  • Labels:
    None
  • Testcase included:
    yes
  • Number of attachments :
    2

Description

There seems to be an issue with constructors added via metaclass and cleanup:

class MetaClassMagicTests extends GroovyTestCase {
    void testIt() {
        ExpandoMetaClass.enableGlobally()
        
        // Child.metaClass // uncomment and everything works
        
        // Save the old meta class
        def oldMetaClass = Parent.metaClass
        
        // Install a new one
        def emc = new ExpandoMetaClass(Parent, true, true)
        emc.initialize()
        GroovySystem.metaClassRegistry.setMetaClass(Parent, emc)
        
        // Install a map based constructor
        emc.constructor = { Map m -> Parent.newInstance() }
        
        // Parent constructor is used, all good
        assert new Child([:]) instanceof Parent
        
        // Reinstate the old meta class
        GroovySystem.metaClassRegistry.removeMetaClass(Parent) 
        GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass)

        // This fails, this calls the custom constructor from above and returns an instance of Parent
        assert new Child([:]) instanceof Child
    }
    
}

class Parent { def a }
class Child extends Parent { def b }
  1. Text File
    4069_v18x_Patch.txt
    28/Feb/10 9:00 AM
    8 kB
    Roshan Dawrani
  2. Text File
    V2_4069_v18x_Patch.txt
    02/Mar/10 10:46 AM
    6 kB
    Roshan Dawrani

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. GRAILS-5883 Order of Unit Test matters when one Domain class extends other Domain class

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

Isn't it the Child metaClass that you should be manipulating instead of Parent's?

The following code goes through:

class MetaClassMagicTests extends GroovyTestCase {
    void testIt() {
        ExpandoMetaClass.enableGlobally()
        
        def oldMetaClass = Child.metaClass
        
        def emc = new ExpandoMetaClass(Child, true, true)
        emc.initialize()
        GroovySystem.metaClassRegistry.setMetaClass(Child, emc)
        
        emc.constructor = { Map m -> Child.newInstance() }
        
        assert new Child([:]) instanceof Parent
        
        GroovySystem.metaClassRegistry.removeMetaClass(Child) 
        GroovySystem.metaClassRegistry.setMetaClass(Child, oldMetaClass)

        assert new Child([:]) instanceof Child
    }
    
}

class Parent { def a }
class Child extends Parent { def b }
Show
Roshan Dawrani added a comment - Isn't it the Child metaClass that you should be manipulating instead of Parent's? The following code goes through: class MetaClassMagicTests extends GroovyTestCase { void testIt() { ExpandoMetaClass.enableGlobally() def oldMetaClass = Child.metaClass def emc = new ExpandoMetaClass(Child, true , true ) emc.initialize() GroovySystem.metaClassRegistry.setMetaClass(Child, emc) emc.constructor = { Map m -> Child.newInstance() } assert new Child([:]) instanceof Parent GroovySystem.metaClassRegistry.removeMetaClass(Child) GroovySystem.metaClassRegistry.setMetaClass(Child, oldMetaClass) assert new Child([:]) instanceof Child } } class Parent { def a } class Child extends Parent { def b }
Hide
Permalink
Luke Daley added a comment -

No, the point is that after I have removed the constructor from the parent's metaclass, it is still called when constructing the child. This is definitely unexpected.

Note the...

// Child.metaClass // uncomment and everything works

line. If that get's executed then everything works, which seems like that if the Child metaClass is initialised before diddling with the Parent's metaClass you get the right behaviour.

Show
Luke Daley added a comment - No, the point is that after I have removed the constructor from the parent's metaclass, it is still called when constructing the child. This is definitely unexpected. Note the... // Child.metaClass // uncomment and everything works line. If that get's executed then everything works, which seems like that if the Child metaClass is initialised before diddling with the Parent's metaClass you get the right behaviour.
Hide
Permalink
Roshan Dawrani added a comment - - edited

While your point about inconsistency is right, I think your asserts convey the wrong expectations (as far as I understand metaClass thing). I think when you add a constructor through Parent EMC, "new Child()" should not get that constructor.

So, the following should be the behavior:

ExpandoMetaClass.enableGlobally()

def oldMetaClass = Parent.metaClass

def emc = new ExpandoMetaClass(Parent, true, true)
emc.initialize()
GroovySystem.metaClassRegistry.setMetaClass(Parent, emc)

emc.constructor = { Map m -> Parent.newInstance() }

/* here also you should get a new Child because you have manipulated Parent MC */
assert new Child([:]) instanceof Child

GroovySystem.metaClassRegistry.removeMetaClass(Parent) 
GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass)

assert new Child([:]) instanceof Child

class Parent { def a }
class Child extends Parent { def b }

For the behavior that you are trying, you should manipulate Child MC.

Can someone please verify the behavior - one way or another? Thanks.

Show
Roshan Dawrani added a comment - - edited While your point about inconsistency is right, I think your asserts convey the wrong expectations (as far as I understand metaClass thing). I think when you add a constructor through Parent EMC, "new Child()" should not get that constructor. So, the following should be the behavior: ExpandoMetaClass.enableGlobally() def oldMetaClass = Parent.metaClass def emc = new ExpandoMetaClass(Parent, true , true ) emc.initialize() GroovySystem.metaClassRegistry.setMetaClass(Parent, emc) emc.constructor = { Map m -> Parent.newInstance() } /* here also you should get a new Child because you have manipulated Parent MC */ assert new Child([:]) instanceof Child GroovySystem.metaClassRegistry.removeMetaClass(Parent) GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass) assert new Child([:]) instanceof Child class Parent { def a } class Child extends Parent { def b } For the behavior that you are trying, you should manipulate Child MC. Can someone please verify the behavior - one way or another? Thanks.
Hide
Permalink
Roshan Dawrani added a comment -

Attaching a patch for review based on my understanding of the behavior in this scenario.

Basically the change made in the patch is that EMC should not create a constructor site if the type <init> has been invoked on does not match the type it finds <init> on at runtime. That way when "Parent" defines a new constructor through EMC, "new Child()" will not match it.

Tests are added to verify the behavior when you manipulate the MC of Parent and of Child and variations "ChildX.metaClass" commented/uncommented that lead to the inconsistent behavior.

Show
Roshan Dawrani added a comment - Attaching a patch for review based on my understanding of the behavior in this scenario. Basically the change made in the patch is that EMC should not create a constructor site if the type <init> has been invoked on does not match the type it finds <init> on at runtime. That way when "Parent" defines a new constructor through EMC, "new Child()" will not match it. Tests are added to verify the behavior when you manipulate the MC of Parent and of Child and variations "ChildX.metaClass" commented/uncommented that lead to the inconsistent behavior.
Hide
Permalink
blackdrag blackdrag added a comment -

hmm... is the type parameter really needed? I assume the declaring class of a method for a method added by closure will always not be the same as the current class. So I wonder if it wouldn't be enough to change

(type == null) || (method.getDeclaringClass().getTheClass().equals(type)

to

method.getDeclaringClass().getTheClass().equals(getTheClass())

As I understand it, this will always false if the method is added through a closure... or is getTheType() returning something else than what "type" would get?

Show
blackdrag blackdrag added a comment - hmm... is the type parameter really needed? I assume the declaring class of a method for a method added by closure will always not be the same as the current class. So I wonder if it wouldn't be enough to change (type == null ) || (method.getDeclaringClass().getTheClass().equals(type) to method.getDeclaringClass().getTheClass().equals(getTheClass()) As I understand it, this will always false if the method is added through a closure... or is getTheType() returning something else than what "type" would get?
Hide
Permalink
Roshan Dawrani added a comment -

Thanks for the feedback, Jochen. I didn't notice that I could get the type from MC#getTheClass() as well.

I have made that change and I am attaching V2 of the patch here.

Please let me know if you see any other issue.

Show
Roshan Dawrani added a comment - Thanks for the feedback, Jochen. I didn't notice that I could get the type from MC#getTheClass() as well. I have made that change and I am attaching V2 of the patch here. Please let me know if you see any other issue.
Hide
Permalink
Roshan Dawrani added a comment -

Fixed

Show
Roshan Dawrani added a comment - Fixed

People

  • Assignee:
    Roshan Dawrani
    Reporter:
    Luke Daley
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: