-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
HHH-19610 make GraphParser support subTypeSubGraph #10521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
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:
Bacause otherwise, something like this just looks odd -
whereas this is a little bit better:
|
|
See also discussion at #hibernate-orm-dev > HHH-19610 Make GraphParser support subTypeSubGraph |
4d9e0dd to
9ba482a
Compare
280d6cc to
13b3d84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments -
- I'd "reverse" the naming of the lexer and parser files -
GraphLanguageLexer->LegacyGraphLanguageLexer, etc. The thought process is that "Modern" will stick around. - I might have missed it, but I think another test/check that ought to be performed is the case of
NamedEntityGraph#rootbeing 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; |
There was a problem hiding this comment.
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.
I should have been more clear here. The lexer/parser you currently have named |
Hi @sebersole , on this point, I find it interesting to leave open the possibility of using NamedEntityGraph#root for Legacy mode. |
Done 🙂 |
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