HHH-14212 Ultimate Fetch Graph fix #3548
Conversation
| // we can return false invariably for if the entity has been covered by entity graph, | ||
| // its associated JOIN has been present in the SQL generated and hence it would be loaded anyway | ||
| if ( session.isEnforcingFetchGraph() ) { | ||
| return false; |
There was a problem hiding this comment.
The above one-liner is the gist of the ultimate fix. Its comment is self-explanatory.
There was a problem hiding this comment.
Ok, I understand better. Thanks for the clarification. So essentially this leverages the pre-existing "load graph" implementation, adding just one flag in the session to let us know whether we're in "fetch graph" mode or "load graph" mode, and according to that flag we'll decide whether attributes that are not mentioned in the graph are forced to lazy or left to their default value.
Looks good to me!
|
Do we want/need anyone else to review this? If not, I'd say we merge it! 😃 |
The session tweaking is not new and it was applied for my previous fix direction (in HHH-8776). What is really inspiring is the simple logic to return 'false' always regardless of whether the entity should be eagerly loaded or not (we have added JOINs for those entity graph nodes already so they will be loaded sooner or later; what really matters is the NEW nodes). The inspiration occurred to me when I was accompanying my little kid in playground. It only took 15 mins to verify my idea. I am really excited by its beauty and simplicity. |
|
@NathanQingyangXu so I suppose this one needs to be replicated in Hibernate Reactive? |
|
Perhaps it even breaks HR? |
|
When it will be available to the public? I mean the release version. |
guess in next release. @beikov could we have it merged? |
…etOverridingEager() when Fetch Graph is being enforced
NathanQingyangXu
force-pushed
the
HHH-14212-again
branch
from
4685606
to
311c180
Compare
|
What are the concerns here @gavinking ? Do you think that merging this PR will cause problems for HR? |
|
No, I'm just saying that we will (I think) need to make changes to HR once this is merged. |
|
I see, thanks for the fast response. I'll merge this then :) |
Feel free to ask for my help on that, though I have zero experience on HR right now. |
|
@NathanQingyangXu why does this patch remove type arguments and go back there to using raw types? |
@NathanQingyangXu Well for a start we need to just test HR against the latest ORM snapshot. |
That change is done in yrodiere's PR, which has nothing to do with entity graph. I reverted his PR totally so this change is reverted as well. This PR involves many revertings and I need to be very cautious so I focus on the core issue instead. |
Right, OK, gotcha. |
|
HR issue is here hibernate/hibernate-reactive#384 |
|
@NathanQingyangXu @beikov Shouldn't this be backported to the 5.4 branch, since the bug was reported on 5.4? |
|
@Sanne many people are asking whether we can back port it into 5.4 for it is a regression issue. Seems failure to back port it might hurt our credit for entity graph is a enormously popular feature of JPA. |
|
thanks @NathanQingyangXu , yes I'll backport this. Sorry I've been a bit swamped, hope to "batch" a series of backports so to test them all together in downstream projects. |
|
Has this been back ported? We're seeing it on v5.4.21, so we're having to downgrade to v5.4.18, but there are other vulnerabilities that were addressed post v5.4.18 so we'd really like to use the latest release int he 5.4.x release. |
|
See https://hibernate.atlassian.net/browse/HHH-14212 . It's been backported in 5.4.22. |
https://hibernate.atlassian.net/browse/HHH-14212
This ticket exposes another regression issue and it forced me to re-consider the previous Fetch Graph implementation I initiated previously. It is becoming more and more difficult to maintain and obviously it is full of bugs and terribly fragile.
Finally I figured out we have a much simpler way to implement Fetch Graph we overlooked previously:
TwoPhaseLoadclass to check whether Fetch Graph is in effective for now; return false if so in the internalgetOverridingEager()method inTwoPhaseLoadclass.The reason is our HQL to SQL step has added all the JOINs already and we simply keep from loading any other entities during hydrating phase. We don't need to duplicate Fetch Graph enforcement for another time! Boom, all of sudden, we solve the Fetch Graph issue in a simple and elegant way. My previous initial implementation is purely misled.
I intentionally organized the commit list to streamline code review. All the commits except the last one are reverting back previous PR commits. You can go to the last commit directly to understand the new implementation: 4685606