Loading from 2LC broken by HHH-9028

Description

See last comment on HHH-9028. Approach and fix were incorrect. Instead, that test case should throw a WrongClassException.

Environment

None

Activity

Show:
Shawn Clowater
April 19, 2014, 12:06 AM

Good to see you guys already nabbed this one, this issue just tripped one of my tests during my upgrade validation. It looks like the behaviour where if you look up an entity by either its root class or explicit type that you should get a hit like it was prior to HHH-9028?

Guillaume Smet
April 9, 2014, 12:11 AM

My point is that from the application developer point of view, the object does not exist. The fact that the class is wrong or the id is wrong is an implementation detail: you don't have an object from this class for this id, the fact that there is inheritance and so on shouldn't be seen from the session.get perspective IMHO.

So, I surprisingly vote for consistently returning null.

Steve Ebersole
April 8, 2014, 11:08 PM

So here is what we have atm wrt Session#get:

  • When match found in session cache we effectively return null. Technically we return a marker because get's options state that is in fact can return nulls. See DefaultLoadEventListener#loadFromSessionCache and the INCONSISTENT_RTN_CLASS_MARKER branch in its caller for details.

  • When match found in L2 cache, afaict CCE was always the result in the "wrong class" case.

  • When hitting the database we return null because we get back an empty ResultSet because the discriminator is included. I verified this happened in the older Loader code too; it was not just the move to LoadPlans.

So definitely there is an inconsistency here. The previous solution is still not correct though. What should happen is that either we consistently throw WrongClassException, or we consistently return null (when possible).

TBH, I just do not see the benefit of simply returning null here. But I'll let convince me

Steve Ebersole
April 8, 2014, 10:09 PM

Looking at this some more, I think there are a number of things going on here.

First, Session#get seems to be missing the same checking since the move to LoadPlan-based loading.

Second, I wonder if there should be some handling for the "wrong class" case in the load listener in the branch where we pull from second-level cache (and possibly from database) like there is when we pull from the session-cache.

Fixed

Assignee

Brett Meyer

Reporter

Brett Meyer

Fix versions

Labels

None

backPortable

None

Suitable for new contributors

None

Requires Release Note

None

Pull Request

None

backportDecision

None

Priority

Critical