HHH-18714 add support for inheritance in entity graph #9088

Asanio06 · 2024-10-14T07:40:42Z

[Please describe here what your change is about]
This change enables support for inheritance with EntityGraphs as described in the JPA specification.
I've added some functions such as addTreatedSubgraph to facilitate future integration with Hibernate 7 (Jpa 3.2).
The ticket related to my proposal: https://hibernate.atlassian.net/browse/HHH-18714

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

.../org/hibernate/orm/test/entitygraph/named/multiple/NamedEntityGraphsWithInheritanceTest.java

...te-core/src/test/java/org/hibernate/orm/test/entitygraph/EntityGraphWithInheritanceTest.java

Asanio06 · 2024-10-14T11:09:48Z

@gavinking I don't understand why h2 build fail. Do I need to relaunch the build for h2? Is it possible that this is coming from scope.getSessionFactory().getSchemaManager().truncateMappedObjects(); which breaks h2 ?

Edit: Its work now

gavinking · 2024-10-14T14:36:29Z

@gavinking I don't understand why h2 build fail. Do I need to relaunch the build for h2? Is it possible that this is coming from scope.getSessionFactory().getSchemaManager().truncateMappedObjects(); which breaks h2 ?

Edit: Its work now

Maybe I had spaces instead of tabs in my suggestion.

Asanio06 · 2024-10-14T16:42:42Z

@gavinking I don't understand why h2 build fail. Do I need to relaunch the build for h2? Is it possible that this is coming from scope.getSessionFactory().getSchemaManager().truncateMappedObjects(); which breaks h2 ?
Edit: Its work now

Maybe I had spaces instead of tabs in my suggestion.

I think it was me, I had forgotten some unused imports.

Asanio06 · 2024-10-16T05:16:38Z

Hello @gavinking What do you think of this change ? Should I plan to add documentation on how it works, or is it too risky ?

beikov · 2024-10-21T13:59:47Z

hibernate-core/src/main/java/org/hibernate/graph/internal/RootGraphImpl.java

-		super(original, mutable);
+		super( original, mutable );
 		this.name = name;
+		this.subclassSubgraphs = new HashMap<>();


This should IMO be instantiated lazily.

beikov · 2024-10-21T14:02:15Z

hibernate-core/src/main/java/org/hibernate/graph/internal/AbstractGraph.java

+		PersistentAttribute<?, ?> attribute = managedType.findAttributeInSuperTypes( attributeName );
+
+		if ( attribute == null ) {
+			attribute = managedType.findSubTypesAttribute( attributeName );


Please revert this. Implicitly looking into subtypes is not an option. Users will have to explicitly cast the graph to "treat" it and can then access subtype attributes.

Hello @beikov ,
Sorry for the delay. I do understand. I have the impression that the difficulty here is that if I remove this as well as the graph merge below, the subgraphs on which a type is specified will be ignored.
This is because when fields are traversed in StandardEntityGraphTraversalStateImpl, all subtypes attributes are represented as if they belonged to the parent class.
Today on this line, when we have sub-graphs associated with sub-types, we ignore them.

In the case below, for example, the ‘course_sub_graph sub-graph will be ignored:

@NamedEntityGraph( name = "exemple", subgraphs = { @NamedSubgraph( name = "course_sub_graph", type = PayingCourse.class, attributeNodes = { @NamedAttributeNode("moneyReceiver") } ) } ) @DiscriminatorValue("student") public static class Student { @Id Long id; @OneToMany(fetch = FetchType.LAZY, cascade = CascadeType.ALL) List<Course> courses; } @Entity(name = "Course") @DiscriminatorColumn(name = "course_type", discriminatorType = DiscriminatorType.STRING) public static abstract class Course { @Id Long id; String name; } @Entity(name = "FreeCourse") public static class FreeCourse extends Course { } @Entity(name = "PayingCourse") @DiscriminatorValue("paying") public static class PayingCourse extends Course { @ManyToOne(fetch = FetchType.LAZY, cascade = CascadeType.ALL) Teacher moneyReceiver; }

@Asanio06 so what you're saying is that the explicit class = PayingCourse.class is doing some sort of a "treat".

The spec is for this stuff is extremely hard to understand, and I wasn't around when it was added, so I don't know for sure what it's supposed to mean, however, section 10.3.3 and the following Javadoc would appear to support your interpretation:

public @interface NamedSubgraph { /** * (Required) The name of the subgraph as referenced from a * NamedAttributeNode element. */ String name(); /** * (Optional) The type represented by this subgraph. The element * must be specified when this subgraph is extending a definition * on behalf of a subclass. */ Class<?> type() default void.class; /** * (Required) The list of the attributes of the class that must * be included. If the named subgraph corresponds to a subclass * of the class referenced by the corresponding attribute node, * then only subclass-specific attributes are listed. */ NamedAttributeNode[] attributeNodes(); }

@gavinking
Yes, that's exactly it, it's not very well documented.
I have the impression that to support this inheritance functionality on subgraphs, it is necessary to go to the attributes of child classes on these function.
If not, currentGraphContext.findAttributeNode will always return null. And if we don't merge the different sub-graphs, currentGraphContext will correspond to only 1 of the sub-graphs and the others will be ignored.

I have the impression that the other way requires changing the way you browse an entity's attributes to find out if they are to be fetched.

Well, also read section 10.3.3 of the spec, which tends to imply that you are only allowed to specify directly-declared members of PayingCourse.class in that @NamedSubgraph.

So perhaps that rule helps avoid searching through hierarchies?

The NamedSubgraph annotation is used to
further define an attribute node. It is referenced by its name from the
subgraph or keySubgraph element of a NamedAttributeNode element.

The name element is the name used to
reference the subgraph from a NamedAttributeNode definition. In the
case of entity inheritance, multiple subgraph elements have the same
name.

The type element must be specified when the
subgraph corresponds to a subclass of the entity type corresponding to
the referencing attribute node.

The attributeNodes element lists attributes
of the class that must be included. If the subgraph corresponds to a
subclass of the class referenced by the corresponding attribute node,
only subclass-specific attributes are listed.

Also:

The `subgraph` element is used to refer to a `NamedSubgraph` specification that further characterizes an attribute node corresponding to a managed type (entity or embeddable). The value of the `subgraph` element must correspond to the `name` used for the subgraph in the `NamedSubgraph` element. If the referenced attribute is an entity which has entity subclasses, there may be more than one `NamedSubgraph` element with this name, and the `subgraph` element is considered to refer to all of these.

Thanks for looking into this stuff, but the way, @Asanio06, I know it's a hole we have.

@gavinking You're right, these details really help me in my reasoning.
Does this mean that to solve our problem we could potentially make SubGraphImpl contain a ‘subclassSubgraphs’ variable like RootGraphImpl?
So if I create 3 subgraphs of the same name but different types on the same AttributeNode, I'll have 1 subgraph that has a link to the other subgraphs.

Is this correct?

I think so?

@gavinking @beikov Thanks for all your help
I'm going to work on the changes and I'll get back to you as soon as it's done.

Awesome, thanks again man!

Hello @gavinking @beikov ,
I removed that.
Here's a new version.
This time if I consider that each attribute node has only 1 subgraph which itself can have subclassSubgraphs.
This change generates numerous impacts that I've tried to manage as well as possible.

In the StandardEntityGraphTraversalStateImpl, I now use the declaringType of the fetchable to determine whether I should use the graph directly or whether I should use a subclassSubgraph. This is the way I've found to avoid unnecessary attribute node traversals.

beikov · 2024-10-21T14:07:20Z

hibernate-core/src/main/java/org/hibernate/graph/internal/RootGraphImpl.java

+		SubGraphImplementor<S> subgraph = new SubGraphImpl<>( subTypeEntityDomainType, true );
+
+		subclassSubgraphs.putIfAbsent( subTypeEntityDomainType, subgraph );
+
+		return subgraph;


The sub-graph needs to know about this since the javadoc of addTreatedSubgraph says:

Subclass subgraphs automatically include the specified attributes of superclass subgraphs.

also, I think it's desirable to return an existing instance:

Suggested change

SubGraphImplementor<S> subgraph = new SubGraphImpl<>( subTypeEntityDomainType, true );

subclassSubgraphs.putIfAbsent( subTypeEntityDomainType, subgraph );

return subgraph;

final SubGraphImplementor<?> existingSubgraph = subclassSubgraphs.get( subTypeEntityDomainType );

if ( existingSubgraph != null ) {

return existingSubgraph;

}

final SubGraphImplementor<S> subgraph = new SubGraphImpl<>( subTypeEntityDomainType, true );

subclassSubgraphs.put( subTypeEntityDomainType, subgraph );

return subgraph;

Done.
I've started to link the subgraph to its parent. Does this mean that I have to override the findAttributeNode methods in AbstractGraph just to combine the attributes? Or should I also override attributeMap via a getter ?

Hello @beikov ,
I have the impression that the link with the “superclass” is already somewhat taken into account by the “findAttributeNode” method. Even so, one solution I can see to better comply with the specification would be to use the constructor that takes in the “superclass” to hydrate attrNodeMap. What do you think ?

beikov · 2024-10-21T14:12:04Z

...rnate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java

 				);
 			}
 			if ( StringHelper.isNotEmpty( namedAttributeNode.keySubgraph() ) ) {
-				final SubGraphImplementor<?> subgraph = attributeNode.makeKeySubGraph();


Using this method to create a sub-graph for a map-key is vital. This change won't work if a key subgraph uses a subtype subgraph with the same type as a value subgraph.

Oh yes, sorry. I corrected it

beikov · 2024-10-21T14:14:48Z

.../src/main/java/org/hibernate/sql/results/internal/StandardEntityGraphTraversalStateImpl.java

+		GraphImplementor mergedSubgraph = new SubGraphImpl<>( managedDomainType, true );
+
+		for ( GraphImplementor subgraph : subgraphMap.values() ) {
+			mergedSubgraph.merge( subgraph );


This will not be necessary anymore once the SubGraphImpl knows of its parent like the spec requires it. See my previous comment.

yes, you're absolutely right

Asanio06 · 2024-11-19T16:23:41Z

Hello @gavinking @beikov I've made the various changes, I'm having a problem with the pipeline but I don't know where it's coming from. I'm going to analyse

sebersole · 2024-11-19T19:17:15Z

This looks good to me. Let's see what CI says

hibernate-core/src/main/java/org/hibernate/graph/internal/AttributeNodeImpl.java

...rnate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java

Asanio06 · 2024-11-20T08:13:40Z

This looks good to me. Let's see what CI says

@sebersole The code build locally and all test pass but the pipeline seems to be waiting for validation before building :

Asanio06 · 2024-11-27T14:55:10Z

Hello @gavinking , any information?
I hope I'm not harassing you, I imagine you have a lot of work to do. This is my first contribution, so I have no idea how fast it usually goes.

gavinking · 2024-11-28T12:03:05Z

@Asanio06 I was waiting for @sebersole to make the call on this, because he understands that code better than me. Also @beikov should approve.

Asanio06 · 2024-11-28T13:06:24Z

@gavinking Thank you very much for your reply 🙂

gavinking · 2024-11-28T13:07:33Z

Not at all, thank you for your contribution!

Asanio06 · 2024-12-22T19:03:48Z

Hello @gavinking @sebersole @beikov ,

Should I point to version 7 to simplify the work and not do it in 2 times?

gavinking · 2024-12-22T21:39:06Z

Hello @gavinking @sebersole @beikov ,

Should I point to version 7 to simplify the work and not do it in 2 times?

Yes, this should target main.

gavinking · 2025-01-01T11:01:37Z

@Asanio06 I attempted to cherry-pick this onto main, and there were some merge conflicts, which I attempted to fix in the most obvious way, but your new test failed afterward, so either I didn't resolve the conflicts correctly, or something changed on main which broke your fix.

Would you please try getting your patch working on main, please?

hibernate-github-bot · 2025-01-01T12:06:36Z

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

Asanio06 · 2025-01-01T12:11:28Z

Hello @gavinking ,

It's done, I've linked it to the main branch. I'm waiting for the pipeline but the code build well locally
Thanks for the approval. Do you think it will be merged soon?

Happy new year

gavinking · 2025-01-01T17:48:16Z

Thanks man, and Happy NY to you too!

gavinking · 2025-01-02T14:53:55Z

@Asanio06 I am really, really sorry to do this to you, and I feel real bad because you really did do good work on this one.

But when I sat down and looked more closely I saw what an entropic mess we had in the org.hibernate.graph package (unimplemented functionality, incorrect generic types, unsound code, and more) and how this was forcing you to jump through hoops to do things that should have been more straightforward.

So in #9526 I did a major rework of that whole package, filled in missing functionality from JPA 3.2, got rid of lots of unchecked assignments, added some assertions, and by side-effect fixed some of the bugs you fixed in this PR.

I deliberately did not fix everything that's fixed by this PR, to leave you the chance to redo at least part of your fix (which will be way simpler and smaller now, I believe) and I did not push your tests. So you still have an chance to send a new PR and have it integrated with your name on the commits.

Overall, I ended up going in a similar direction as you did (as I said, your work was good) but I took it quite a bit further as you'll see if you take a look.

Again I apologize for this situation, I really don't like pulling the rug out from under people like this. It's just that that package has been in desperate need of a major cleanup for a long time, and I had a couple of days with not many distractions.

Asanio06 · 2025-01-02T15:23:13Z

@gavinking No worries, it was a necessary cleaning.
I'll do that. Would you prefer me to create a brand new PR or put the new commit on this one ?

gavinking · 2025-01-02T16:01:09Z

Probably much easier for you if it's a new PR, but whatever works is fine.

Asanio06 · 2025-01-02T17:42:48Z

@gavinking I create a new PR : #9539

gavinking reviewed Oct 14, 2024

View reviewed changes

.../org/hibernate/orm/test/entitygraph/named/multiple/NamedEntityGraphsWithInheritanceTest.java Outdated Show resolved Hide resolved

gavinking reviewed Oct 14, 2024

View reviewed changes

...te-core/src/test/java/org/hibernate/orm/test/entitygraph/EntityGraphWithInheritanceTest.java Outdated Show resolved Hide resolved

Asanio06 force-pushed the HHH-18714 branch from f6ff6c5 to 1b08837 Compare October 14, 2024 10:37

Asanio06 force-pushed the HHH-18714 branch from 1b08837 to 0eba9c4 Compare October 14, 2024 12:12

Asanio06 requested a review from gavinking October 14, 2024 13:00

sebersole self-requested a review October 18, 2024 14:00

beikov requested changes Oct 21, 2024

View reviewed changes

Asanio06 force-pushed the HHH-18714 branch from 0eba9c4 to 548da11 Compare November 5, 2024 17:26

Asanio06 requested a review from beikov November 5, 2024 17:28

Asanio06 force-pushed the HHH-18714 branch from 548da11 to dfc8848 Compare November 19, 2024 16:09

github-advanced-security bot found potential problems Nov 19, 2024

View reviewed changes

hibernate-core/src/main/java/org/hibernate/graph/internal/AttributeNodeImpl.java Fixed Show fixed Hide fixed

...rnate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java Fixed Show fixed Hide fixed

Asanio06 force-pushed the HHH-18714 branch 3 times, most recently from c82ceb9 to 3ba9cec Compare November 20, 2024 07:44

gavinking approved these changes Jan 1, 2025

View reviewed changes

Asanio06 force-pushed the HHH-18714 branch from 3ba9cec to 1a109d0 Compare January 1, 2025 12:05

Asanio06 changed the base branch from 6.6 to main January 1, 2025 12:06

HHH-18714 add support for inheritance in entity graphs

c16fb1c

Asanio06 force-pushed the HHH-18714 branch from 1a109d0 to c16fb1c Compare January 1, 2025 12:10

Asanio06 closed this Jan 2, 2025

Asanio06 mentioned this pull request Jan 2, 2025

HHH-18714 add support for inheritance in NamedEntityGraph annotation #9539

Merged

-		SubGraphImplementor<S> subgraph = new SubGraphImpl<>( subTypeEntityDomainType, true );
-		subclassSubgraphs.putIfAbsent( subTypeEntityDomainType, subgraph );
-		return subgraph;
+		final SubGraphImplementor<?> existingSubgraph = subclassSubgraphs.get( subTypeEntityDomainType );
+		if ( existingSubgraph != null ) {
+			return existingSubgraph;
+		}
+		final SubGraphImplementor<S> subgraph = new SubGraphImpl<>( subTypeEntityDomainType, true );
+		subclassSubgraphs.put( subTypeEntityDomainType, subgraph );
+		return subgraph;

Uh oh!

HHH-18714 add support for inheritance in entity graph #9088

HHH-18714 add support for inheritance in entity graph #9088

Uh oh!

Conversation

Asanio06 commented Oct 14, 2024

Uh oh!

Uh oh!

Uh oh!

Asanio06 commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking commented Oct 14, 2024

Uh oh!

Asanio06 commented Oct 14, 2024

Uh oh!

Asanio06 commented Oct 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Asanio06 Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavinking Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Asanio06 commented Nov 19, 2024

Uh oh!

sebersole commented Nov 19, 2024

Uh oh!

Uh oh!

Uh oh!

Asanio06 commented Nov 20, 2024

Uh oh!

Asanio06 commented Nov 27, 2024

Uh oh!

gavinking commented Nov 28, 2024

Uh oh!

Asanio06 commented Nov 28, 2024

Uh oh!

gavinking commented Nov 28, 2024

Uh oh!

Asanio06 commented Dec 22, 2024

Uh oh!

gavinking commented Dec 22, 2024

Uh oh!

Asanio06 commented Oct 14, 2024 •

edited

Loading

Asanio06 Oct 29, 2024 •

edited

Loading

gavinking Oct 29, 2024 •

edited

Loading

hibernate-github-bot bot commented Jan 1, 2025 •

edited

Loading