Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullPointerException during cascading saves of bidirectional domain objects #9290

Closed
joemccall86 opened this issue Sep 26, 2015 · 19 comments

Comments

@joemccall86
Copy link
Contributor

commented Sep 26, 2015

Reproducible example: https://github.com/joemccall86/grails-bidirectional-cascade-test (includes working 2.4.5 project and non-working 3.0.8 project).

This is behavior that worked in Grails 2.4.5, but seems broken in Grails 3.0.x. Given a domain class Team that hasMany = [players: Player], and each player hasOne = [contract: Contract], and the bi-directional associations are setup properly, a save(flush: true) on a Team object should cascade saves down to Player and Contract. What happens instead is a NullPointerException.

This seems very similar to #698. Running JDK 8 on Windows 10.

Here is the stacktrace:

java.lang.NullPointerException
    at org.hibernate.engine.spi.BatchFetchQueue.removeBatchLoadableEntityKey(BatchFetchQueue.java:163)
    at org.hibernate.engine.internal.StatefulPersistenceContext.addEntity(StatefulPersistenceContext.java:389)
    at org.hibernate.engine.internal.StatefulPersistenceContext.addEntity(StatefulPersistenceContext.java:462)
    at org.hibernate.action.internal.AbstractEntityInsertAction.makeEntityManaged(AbstractEntityInsertAction.java:143)
    at org.hibernate.engine.spi.ActionQueue.addResolvedEntityInsertAction(ActionQueue.java:203)
    at org.hibernate.engine.spi.ActionQueue.addInsertAction(ActionQueue.java:181)
    at org.hibernate.engine.spi.ActionQueue.addAction(ActionQueue.java:216)
    at org.hibernate.event.internal.AbstractSaveEventListener.addInsertAction(AbstractSaveEventListener.java:334)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:289)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:195)
    at org.hibernate.event.internal.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:126)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.saveWithGeneratedOrRequestedId(DefaultSaveOrUpdateEventListener.java:209)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.entityIsTransient(DefaultSaveOrUpdateEventListener.java:194)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.performSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:114)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.onSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:90)
    at org.grails.orm.hibernate.support.ClosureEventTriggeringInterceptor.onSaveOrUpdate(ClosureEventTriggeringInterceptor.java:113)
    at org.hibernate.internal.SessionImpl.fireSaveOrUpdate(SessionImpl.java:684)
    at org.hibernate.internal.SessionImpl.saveOrUpdate(SessionImpl.java:676)
    at org.hibernate.engine.spi.CascadingActions$5.cascade(CascadingActions.java:235)
    at org.hibernate.engine.internal.Cascade.cascadeToOne(Cascade.java:350)
    at org.hibernate.engine.internal.Cascade.cascadeAssociation(Cascade.java:293)
    at org.hibernate.engine.internal.Cascade.cascadeProperty(Cascade.java:161)
    at org.hibernate.engine.internal.Cascade.cascade(Cascade.java:118)
    at org.hibernate.event.internal.AbstractSaveEventListener.cascadeAfterSave(AbstractSaveEventListener.java:470)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:295)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:195)
    at org.hibernate.event.internal.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:126)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.saveWithGeneratedOrRequestedId(DefaultSaveOrUpdateEventListener.java:209)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.entityIsTransient(DefaultSaveOrUpdateEventListener.java:194)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.performSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:114)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.onSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:90)
    at org.grails.orm.hibernate.support.ClosureEventTriggeringInterceptor.onSaveOrUpdate(ClosureEventTriggeringInterceptor.java:113)
    at org.hibernate.internal.SessionImpl.fireSaveOrUpdate(SessionImpl.java:684)
    at org.hibernate.internal.SessionImpl.saveOrUpdate(SessionImpl.java:676)
    at org.hibernate.engine.spi.CascadingActions$5.cascade(CascadingActions.java:235)
    at org.hibernate.engine.internal.Cascade.cascadeToOne(Cascade.java:350)
    at org.hibernate.engine.internal.Cascade.cascadeAssociation(Cascade.java:293)
    at org.hibernate.engine.internal.Cascade.cascadeProperty(Cascade.java:161)
    at org.hibernate.engine.internal.Cascade.cascadeCollectionElements(Cascade.java:379)
    at org.hibernate.engine.internal.Cascade.cascadeCollection(Cascade.java:319)
    at org.hibernate.engine.internal.Cascade.cascadeAssociation(Cascade.java:296)
    at org.hibernate.engine.internal.Cascade.cascadeProperty(Cascade.java:161)
    at org.hibernate.engine.internal.Cascade.cascade(Cascade.java:118)
    at org.hibernate.event.internal.AbstractSaveEventListener.cascadeAfterSave(AbstractSaveEventListener.java:470)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:295)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:195)
    at org.hibernate.event.internal.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:126)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.saveWithGeneratedOrRequestedId(DefaultSaveOrUpdateEventListener.java:209)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.entityIsTransient(DefaultSaveOrUpdateEventListener.java:194)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.performSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:114)
    at org.hibernate.event.internal.DefaultSaveOrUpdateEventListener.onSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:90)
    at org.grails.orm.hibernate.support.ClosureEventTriggeringInterceptor.onSaveOrUpdate(ClosureEventTriggeringInterceptor.java:113)
    at org.hibernate.internal.SessionImpl.fireSaveOrUpdate(SessionImpl.java:684)
    at org.hibernate.internal.SessionImpl.saveOrUpdate(SessionImpl.java:676)
    at org.hibernate.internal.SessionImpl.saveOrUpdate(SessionImpl.java:671)
    at org.grails.orm.hibernate.AbstractHibernateGormInstanceApi.performSave_closure3(AbstractHibernateGormInstanceApi.groovy:255)
    at groovy.lang.Closure.call(Closure.java:426)
    at org.grails.orm.hibernate.GrailsHibernateTemplate.doExecute(GrailsHibernateTemplate.java:198)
    at org.grails.orm.hibernate.GrailsHibernateTemplate.execute(GrailsHibernateTemplate.java:142)
    at org.grails.orm.hibernate.GrailsHibernateTemplate.execute(GrailsHibernateTemplate.java:112)
    at org.grails.orm.hibernate.AbstractHibernateGormInstanceApi.performSave(AbstractHibernateGormInstanceApi.groovy:254)
    at org.grails.orm.hibernate.AbstractHibernateGormInstanceApi.save(AbstractHibernateGormInstanceApi.groovy:179)
    at org.grails.datastore.gorm.GormEntity$Trait$Helper.save(GormEntity.groovy:165)
    at baseball.test.BiDirectionalAssociationSpecSpec.$tt__setupData(BiDirectionalAssociationSpecSpec.groovy:42)
    at baseball.test.BiDirectionalAssociationSpecSpec.setupData_closure1(BiDirectionalAssociationSpecSpec.groovy)
    at groovy.lang.Closure.call(Closure.java:426)
    at groovy.lang.Closure.call(Closure.java:442)
    at grails.transaction.GrailsTransactionTemplate$1.doInTransaction(GrailsTransactionTemplate.groovy:67)
    at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:133)
    at grails.transaction.GrailsTransactionTemplate.executeAndRollback(GrailsTransactionTemplate.groovy:64)
    at baseball.test.BiDirectionalAssociationSpecSpec.$tt__$spock_feature_0_0(BiDirectionalAssociationSpecSpec.groovy:20)
    at baseball.test.BiDirectionalAssociationSpecSpec.a valid team is bootstrapped into the test_closure2(BiDirectionalAssociationSpecSpec.groovy)
    at groovy.lang.Closure.call(Closure.java:426)
    at groovy.lang.Closure.call(Closure.java:442)
    at grails.transaction.GrailsTransactionTemplate$1.doInTransaction(GrailsTransactionTemplate.groovy:67)
    at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:133)
    at grails.transaction.GrailsTransactionTemplate.executeAndRollback(GrailsTransactionTemplate.groovy:64)
    at baseball.test.BiDirectionalAssociationSpecSpec.a valid team is bootstrapped into the test(BiDirectionalAssociationSpecSpec
@jd73

This comment has been minimized.

Copy link

commented Sep 28, 2015

I am experiencing the same issue with grails 3.0.7

@denis111

This comment has been minimized.

Copy link

commented Sep 29, 2015

It happens in 2.5.1 also if there's beforeSave method in the domain class.

a quick workaround in BootStrap.groovy would be:

def oldRemoveBatchLoadableEntityKey = BatchFetchQueue.metaClass.&removeBatchLoadableEntityKey

        BatchFetchQueue.metaClass.removeBatchLoadableEntityKey = { EntityKey key ->
            return key ? oldRemoveBatchLoadableEntityKey.invoke(key) : null
        }
@graemerocher graemerocher self-assigned this Oct 13, 2015
@graemerocher

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

The issue is that the player property is unassigned when save() is called:

The following corrects the problem:

    def c = new Contract(
            expiration: new Date(),
            salary: 40_000_000
    )
    def p = new Player(
            firstName: "John",
            lastName: "Doe",
            position: "Pitcher",
            contract: c
    )
    c.player = p
    padres.addToPlayers(p)

Probably earlier versions of Grails aligned these associations before saving, I'll try track down the change, but it is not as serious as I thought and the above is a workaround

@graemerocher graemerocher removed this from the grails-3.0.9 milestone Oct 13, 2015
@joemccall86

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

Thanks for taking a look at it. I'd like to help any way I could if you could point me in the right direction. A lot of our code was written based on those cascading behaviors, and my time would likely be better spent contributing a patch than adding this workaround everywhere we save an object.

@graemerocher

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

I believe the problem is that in the latest versions of the Hibernate plugin this method is no longer present:

https://github.com/grails/grails-data-mapping/blob/3.x/grails-datastore-gorm-hibernate4/src/main/groovy/org/codehaus/groovy/grails/orm/hibernate/metaclass/AbstractSavePersistentMethod.java#L246

This is because the logic for save() was replaced / moved to:

https://github.com/grails/grails-data-mapping/blob/4.x/grails-datastore-gorm/src/main/groovy/org/grails/datastore/gorm/GormInstanceApi.groovy#L169

But obviously some behaviour seems to have not been retained. Since this is a Hibernate specific problem, it would probably make sense to override and provide an impl in the child class:

https://github.com/grails/grails-data-mapping/blob/4.x/grails-datastore-gorm-hibernate4/src/main/groovy/org/grails/orm/hibernate/HibernateGormInstanceApi.groovy

Thanks for the willingness to contribute.

@joemccall86

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2015

Wow, it turns out I am in completely over my head on this one :-).

Basically I started out trying to make the sample application into a unit test on the grails-datastore-gorm-hibernate4 module, based on BidirectionalOneToOneBelongsToCascadeSpec.groovy, making it pass in the 3.x branch (the plugin version in grails 2.x), and seeing if it fails on the 4.x branch (the plugin used in grails 3.x). I couldn't even set up a passing test for the 3.x branch. The integration test with team->player->contract passes, but as soon as I replicate that in the unit test I get the same NPE failure as above (also that workaround makes it pass).

In other words, I can't seem to wrap my head around why Grails 2.x even works in this case. All I can contribute is this discovery: this bug doesn't only happen with hasMany. It can be reproduced with nested hasOne/belongsTo associations.

class Foo {
    static hasOne = [bar: Bar]
}

class Bar {
    static belongsTo = [foo: Foo]
    static hasOne = [baz: Baz]
}

class Baz {
    static belongsTo = [bar: Bar]
}

new Foo(
    bar: new Bar(
        baz: new Baz()
    )
).save(flush: true)

When I run the above code:

Project Test Type Runs
Grails 2.4.5 Integration YES, all three rows inserted
Grails 3.0.8 Integration NO, NullPointerException above
grails-data-mapping 3.x branch Unit NO, NullPointerException above
grails-data-mapping 4.x branch Unit NO, NullPointerException above

One more thing: based on observations, foo.bar.foo == foo, but foo.bar.baz.bar == null, not foo.bar. Thus, this issue only seems to turn up when there is a chain of associations involved.

I hope this helps in some small way. I need to move onto another task, but feel free to ask me anything about the tests I wrote, or any other questions.

@joemccall86

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2016

I upgraded the test project to use 3.0.13 and added one for 3.1.0. Unfortunately the test still fails in both.

@graemerocher graemerocher added this to the grails-3.1.1 milestone Jan 29, 2016
graemerocher added a commit to grails/grails-data-mapping that referenced this issue Feb 1, 2016
@graemerocher

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

So I have added a test for this, this is not easy to fix at a GORM level because the only way to fix it is to modify the constructors via AST for domain classes so that during assignment the associations are aligned. Another option is to use AST to modify the setters to auto align the bidirectional one-to-one, again not a simple fix.

A third option that is less invasive and probably how it worked before is to modify the Grails data binding to align the properties. This would work only for when using GORM within a Grails project at the data binding constructor is only added via AST when you use Grails so would not work for example in standalone GORM.

I am undecided what the best fix is or whether this is even worth fixing yet since there is a workaround available and I do not want to add necessarily logic that traverses the whole relationship tree in order to automatically align associations as this would hurt performance. So the only potential future solution right now is an AST transformation that modifies the setters, but it is a non-trivial solution.

@graemerocher graemerocher removed this from the grails-3.1.1 milestone Feb 1, 2016
@magx2

This comment has been minimized.

Copy link

commented May 13, 2016

Is any update on this?

@davydotcom

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

shouldnt the addToPlayer call make the reverse association for you?

@graemerocher

This comment has been minimized.

Copy link
Member

commented May 20, 2016

@davydotcom The issue is not the padres-players association it is the Contract -> player association

@graemerocher

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

Duplicates #9820

@davydotcom

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2016

My issue number is smaller though!
:)
Sent from my iPhone

On Sep 22, 2016, at 2:47 AM, graemerocher notifications@github.com wrote:

Duplicates #9820


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@graemerocher

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

Very true!

@moskiteau

This comment has been minimized.

Copy link

commented Sep 30, 2016

What's the workaround? This?

` def oldRemoveBatchLoadableEntityKey = BatchFetchQueue.metaClass.&removeBatchLoadableEntityKey

    BatchFetchQueue.metaClass.removeBatchLoadableEntityKey = { EntityKey key ->
        return key ? oldRemoveBatchLoadableEntityKey.invoke(key) : null
    }`
@graemerocher

This comment has been minimized.

Copy link
Member

commented Sep 30, 2016

The issue normally results from invalid application code. Either

a) Your domain model is not associated correctly prior to calling save
or
b) You are associating an invalid object with a valid one and trying to save said valid one

So the "workaround" is normally to fix your application logic.

@graemerocher graemerocher added this to the grails-3.2.2 milestone Oct 19, 2016
@graemerocher

This comment has been minimized.

Copy link
Member

commented Oct 21, 2016

In Hibernate 5 the error message has been improved it seems. It is now:

Caused by: org.h2.jdbc.JdbcSQLException: NULL not allowed for column "PLAYER_ID"; SQL statement:
insert into contract (id, version, player_id, salary) values (null, ?, ?, ?) [23502-164]
graemerocher added a commit to grails/gorm-hibernate5 that referenced this issue Oct 21, 2016
graemerocher added a commit that referenced this issue Oct 21, 2016
@magx2

This comment has been minimized.

Copy link

commented Feb 6, 2017

Any update on this? Grails 3.2.4 still has the same problem and it's quite annoying.

@RichardKnowles

This comment has been minimized.

Copy link

commented May 17, 2017

I'm porting an ancient app from 1.3.6 to 2.5.6 and I've discovered exactly this case.

Class A in a bi-directional relationship with Class B.

My original code made the classic newbie Hibernate mistake where I have set up a reference from object A to object B, but no "back reference" from object B to object A. Exactly as @graemerocher describes earlier in this issue (simplified here):

class A {
static hasMany = [bInstances: B]
}

class B {
static belongsTo = [parent: A]
}

Hibernate has always required for bi-directional relationships, both sides of the association are set.

a.bInstances << bInstance
bInstance.parent = a

So as @graemerocher pointed out, it was my invalid application code - the annoying thing was that the older version of Grails was erroneously kind of "autocorrecting" the error. It's not a workaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.