-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
HHH-16572 Incorrect enhancement for PROPERTY attributes with mismatched field and method names #9136
HHH-16572 Incorrect enhancement for PROPERTY attributes with mismatched field and method names #9136
Conversation
...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java
Outdated
Show resolved
Hide resolved
One of the test failures in https://github.com/hibernate/hibernate-orm/blob/19e495d8da7610503afd50083b003de631d25564/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhance/internal/bytebuddy/DirtyCheckingWithEmbeddedOnGetterTest.java seems to because the CardGame entity has an @id on the property getter getId() which as per this pull request will not be enhanced which I am guessing breaks the the test code attempt to do things like:
|
87e12da
to
26e7958
Compare
boolean result = false; | ||
// Check for use of ID/AccessType(PROPERTY) on methods | ||
|
||
for (MethodDescription.InDefinedShape shape : managedCtClass.getDeclaredMethods()) { |
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.
@raphw Does this use of getDeclaredMethods look correct to you?
I'm trying to check the passed application class to see if it has any methods that have @jakarta.persistence.Id or @jakarta.persistence.Access (with a certain value) which seems to work with some testing but I'm seeing some Hibernate test failures that I do not yet understand. This is to avoid rewriting classes that have those annotations to avoid the https://hibernate.atlassian.net/browse/HHH-16572 problem.
Thank you for your advice!
Scott
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.
Are you only interested in the declared methods of a class, or also in the ones that are inherited from super classes. For the former, this is correct..For the latter, you'd need to use a MethodGraph.Compiler and resolve the hierarchy.
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.
@raphw I'm interested in the declared methods of a class and also the ones that are inherited from super classes. I'm also interested in examining the declared fields (methodDescription.getDeclaringType().getDeclaredFields()) of each methodDescription.getDeclaringType()
.
Is it correct for me to depend on methodDescription.getDeclaringType().getDeclaredFields()
to access each class's set of fields in the hierarchy?
MethodGraph.Linked methodGraph = MethodGraph.Compiler.Default.forJavaHierarchy().compile(managedCtClass);
for (MethodGraph.Node node : methodGraph.listNodes()) {
MethodDescription methodDescription = node.getRepresentative();
ArrayList<AnnotatedFieldDescription> fieldList = new ArrayList<>();
for (FieldDescription ctField : methodDescription.getDeclaringType().getDeclaredFields()) {
AnnotatedFieldDescription annotatedField = new AnnotatedFieldDescription(enhancementContext, ctField);
if (enhancementContext.isPersistentField(annotatedField)) {
fieldList.add(annotatedField);
}
}
}
... validate each declared method name against the fields in methodDescription.getDeclaringType()
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.
As far as I can tell ^ code is fine except I should instead be looping through each class fields and validate those against the class declared methods.
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.
Or would it be better to use net.bytebuddy.dynamic.scaffold.FieldLocator to walk through all of the fields of each class in the hierarchy?
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.
A FieldLocator
would resolve a field by its name as the Java language would do it, so this sounds like the right approach to me.
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 removed the unneeded checks that duplicated checks done in other code and stuck with the MethodGraph.Compiler as that covers the needs for the added check done by this change.
...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java
Fixed
Show resolved
Hide resolved
hibernate-core/src/test/java/org/hibernate/orm/test/proxy/MissingSetterWithEnhancementTest.java
Outdated
Show resolved
Hide resolved
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
55d4385
to
9fd9d8e
Compare
Signed-off-by: Scott Marlow <smarlow@redhat.com>
…ed field and method names Signed-off-by: Scott Marlow <smarlow@redhat.com>
9fd9d8e
to
f09fecd
Compare
Signed-off-by: Scott Marlow <smarlow@redhat.com>
Change is merged as faebabd |
https://hibernate.atlassian.net/browse/HHH-16572