- Notifications
You must be signed in to change notification settings - Fork3.7k
HHH-19626 Hibernate processor may fail to process entities with generics#10584
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 ourterms of service andprivacy 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
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.
Thanks,@cigaly, appreciated. Please see my comments.
First, the fix works, so that's great.
But I think it's generally a bad idea to keep this sort of state in the processor, and so I would lean toward making these just functions.
| private static final Map<Element, GenericTypeParameterResolver> typeResolversMap = new HashMap<>(); | ||
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 don't think we can cache stuff like this in astatic. As I understand it, we have no control over the lifecycle of these classes, and they could in principle fill up with all kinds of stuff over many calls to the processor.
| if ( !( typeArguments.isEmpty() || generic.size() == typeArguments.size() ) ) { | ||
| throw new IndexOutOfBoundsException( | ||
| "Type %s : %d vs %d".formatted( type, generic.size(), typeArguments.size() ) ); | ||
| } |
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 guess I think it's wrong for any of this code to ever throw exceptions. We either produce an error message, or we ignore the condition and trust that the Java compiler itself will pick up on the error. In this case I believe "ignore" is the right option, if I'm understanding correctly.
| public class GenericTypeParameterResolver { | ||
| private final Map<Element, Map<String, TypeMirror>> typeParameterMap; |
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.
Given that I don't think we can hold onto these in astatic (see other comment), I guess I would prefer to not even build up this map here. This search can be stateless, I believe (unless we're concerned we might encounter circularities, but I guess I doubt we would).
| @@ -94,6 +94,8 @@ | |||
| PRIMITIVES.put( TypeKind.DOUBLE, "double" ); | |||
| } | |||
| private static final Map<Element, GenericTypeParameterResolver> typeResolversMap = new HashMap<>(); | |||
Check failure
Code scanning / CodeQL
Container contents are never initialized Error
Uh oh!
There was an error while loading.Please reload this page.
Jira issueHHH-19626
Test case from PR#10574
Created helper class
org.hibernate.processor.util.GenericTypeParameterResolverused for generic type parameters resolutionBy submitting this pull request, I confirm that my contribution is made under the terms of theApache 2.0 license
and can be relicensed under the terms of theLGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please checkhere.