Skip to content

Conversation

@Asanio06
Copy link
Contributor

@Asanio06 Asanio06 commented Jul 12, 2025

Today, Jpa's NamedEntityGraph annotation supports the use of subclassSubgraphs to define graphs linked to the subtypes of an Entity. It is also possible, via the entityManager, to do the same thing via the addTreatedSubgraph function.

The goal of this ticket is to improve GraphParser so that it supports a syntax that does the same thing. In this way, Hibernate's own NamedEntityGraph annotation will have 100% of the same functionality as Jpa’s.

I suggest the following syntax for defining a subclassSubgraphs:

(GraphParsingTestSubEntity:sub), name, description

Here we have a sub-graph specific to the GraphParsingTestSubEntity type, which is a sub-type of GraphParsingTestEntity.

The ticket related to my proposal: https://hibernate.atlassian.net/browse/HHH-19610


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.


https://hibernate.atlassian.net/browse/HHH-19610

@sebersole
Copy link
Member

sebersole commented Jul 14, 2025

Hi @Asanio06, thanks for the PR!

Overall it's a good feature to add :)

I'm not a fan of using parenthesis here though - those are already used for sub-graphs so it introduces a bit of syntatic ambiguity. Sure, the use of the colon disambiguites it, but it also does so on its own:

GraphParsingTestSubEntity:sub, name, description

Bacause otherwise, something like this just looks odd -

(GraphParsingTestSubEntity:sub(subName, subOtherStuff)), name, description

whereas this is a little bit better:

GraphParsingTestSubEntity:sub(subName, subOtherStuff), name, description

@Asanio06 Asanio06 force-pushed the HHH-19610 branch 3 times, most recently from 4d9e0dd to 9ba482a Compare August 4, 2025 12:06
@Asanio06 Asanio06 force-pushed the HHH-19610 branch 3 times, most recently from 280d6cc to 13b3d84 Compare November 18, 2025 17:23
Copy link
Member

@sebersole sebersole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general comments -

  1. I'd "reverse" the naming of the lexer and parser files - GraphLanguageLexer -> LegacyGraphLanguageLexer, etc. The thought process is that "Modern" will stick around.
  2. I might have missed it, but I think another test/check that ought to be performed is the case of NamedEntityGraph#root being specified with LEGACY mode - this ought to result in an error.
* When the annotation is applied to a class, the class itself is assumed.
* When applied to a package, this attribute is required.
*/
Class<?> root() default void.class;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit torn whether we want to consider String rootEntityName() ... here to support dynamic models. The annotation can appear at the package level and apply to any entity...

But for posterity, I think this definition is fine.

@sebersole
Copy link
Member

  1. The thought process is that "Modern" will stick around.

I should have been more clear here. The lexer/parser you currently have named Modern* will stay around, at which point the "Modern" bit is wrong/pointless.

}

@Test
public void testNullsEqual(EntityManagerFactoryScope scope) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'scope' is never used.
@Asanio06
Copy link
Contributor Author

  1. I might have missed it, but I think another test/check that ought to be performed is the case of NamedEntityGraph#root being specified with LEGACY mode - this ought to result in an error.

Hi @sebersole , on this point, I find it interesting to leave open the possibility of using NamedEntityGraph#root for Legacy mode.
Today, if a user in legacy mode includes a TypeIndicator, they will see the following logged:
Deprecated syntax when using @NamedEntityGraph: 'Type: attr1, attr2' is deprecated. Specify the root entity using the 'root' attribute instead of prefixing the graph with the entity type.
I can always change it if needed.

@Asanio06
Copy link
Contributor Author

  1. The thought process is that "Modern" will stick around.

I should have been more clear here. The lexer/parser you currently have named Modern* will stay around, at which point the "Modern" bit is wrong/pointless.

Done 🙂

@Asanio06 Asanio06 requested a review from sebersole December 18, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants