Return null early in the event of the CollectionEntry being null.

[Jezza]

Avoids NPE, as the collection entry could not exist.

We were hitting this NPE in production.
We're not doing anything fancy with our database management.

We do manually set the SharedSessionContractImplementor on collections and proxies, as we need to load them.

This NPE stops us from using the simple code path of set the session, and just load the thing.
Unfortunately, while the AbstractCollectionEvent is being built, it tries to fetch the owner id and friends from the context.
But seemingly, because we don't do the normal thing, it's not there.

I've already built quite a bit of code around it, to make sure the context is in the state that hibernate expects, but ideally, I'd just like to be able to set the session, and force load the collection/proxy.

Thanks!

PS, if we could also backport this to 5.4, and if possible 4.3 (Although, I think 4.3 is EOL.), that would be grand, thanks!

[NathanQingyangXu]

I think the fix does make sense for the getCollectionEntry() might return null from its code

@Override
public Object getLoadedCollectionOwnerOrNull(PersistentCollection collection) {
	final CollectionEntry ce = getCollectionEntry( collection );
	if ( ce.getLoadedPersister() == null ) {
		return null;
	}

An inspiring ticket for it exposed some generic NPE issue. Wondering whether some automatic tool (as primitive as Intellij's Inspect Code) could help to spot such issue.

[Sanne]

thanks, and yes I can backport this to 5.4, but I won't port it to 4.3 as we no longer release anything from that branch and have no way to reproduce / verify this.

If you want to send a PR for branch 4.3 we can merge it if you like.

[Sanne]

This looks like the same issue as https://hibernate.atlassian.net/browse/HHH-9578 , which is embarassingly old. I'll see to include the integration test and then merge this.

[Jezza]

thanks, and yes I can backport this to 5.4, but I won't port it to 4.3 as we no longer release anything from that branch and have no way to reproduce / verify this.

If you want to send a PR for branch 4.3 we can merge it if you like.

I created #3503 for the 4.3 branch.

[Sanne]

I think there's no harm in merging this fix, but have been struggling to create a test which actually triggers this.

The test attached to HHH-9578 will throw an NPE without this fix, but the fix isn't enough to make it pass as that test specifically is doing other weird things which make no sense.. so I can't really use it.

[Sanne]

@Jezza do you have a test or some reproducer which we could use to avoid future regressions?

[Jezza]

@Sanne I don't have a test, but we did have a pretty consistent reproduction.
I'll see if I can rip it out, and reproduce it.

[Sanne]

thanks, that would be great

[Jezza]

Ok, I've got a repro up and working.

All that is needed is a mysql database and the attached project.
hibernate-bug.zip

I should point out, I know why the code wouldn't load in the first place.
We set "hibernate.current_session_context_class" to "thread".

We then open up a session whenever we want to do database stuff and then subsequently close it after finishing whatever we needed to do, which of course, doesn't play nice with lazy loading.

Thankfully, we can work around that by setting the session, and forcefully load it.
We use lombok a lot, so we just check if we need to force a load in the getter.

Unfortunately, I've inherited this codebase, and it's quite large, so changing hibernate's functionality without more complete knowledge is a bit of a risk, and a time sink.

When you pass false to loadChildren it'll bypass the workaround, and show the bug.

Reproducible:

Hope it's been of some help!

[Sanne]

Thanks, yes that's very interesting and helpful.

[Sanne]

Even with your reproducer & applying your PR, it's still not enough as I get org.hibernate.HibernateException: collection was evicted - like I had with my previous experiments.

Should I assume you just wanted to fix the NPE? I would agree that an NPE is worse as it's embarassing, but you'd still be stuck, no?

[Jezza]

Yes, by the looks of it, I'd still be relying on my workaround.
As you said, fixing the NPE would still be great, as that exception makes it a lot easier to find the underlying issue.

I'm fine going forward with my workaround.
It might make maintenance a bit more difficult than necessary, as I can't imagine it being brought into Hibernate.

If anything, it might give me enough reason to start paving a way forward away from this weird setup.

Thanks for all of your hard work!

[Sanne]

Ok, I've merged this as https://hibernate.atlassian.net/browse/HHH-14175 - without a test since it doesn't really solve ther root of the problem.

[Jezza]

Awesome, thanks for your time.