HHH-18904 Improve check for unsupported property access properties during bytecode-enhancement #9372

mbellade · 2024-12-04T18:16:20Z

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-18904
https://hibernate.atlassian.net/browse/HHH-18903

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

mbellade · 2024-12-10T09:44:23Z

Backported via #9343.

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

scottmarlow · 2024-12-12T15:34:07Z

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

+			}
+		}
+		// We can assume AccessType.PROPERTY here
+		return AccessType.PROPERTY;


We might want to walk through the methods to check for jakarta.persistence annotations as if there are no annotations on methods or fields then I think there is no default (e.g. what the spec calls [undefined](https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a113)) if I understand correctly. Perhaps Hibernate ORM prefers to default to one or the other in other parts of the code?

The @Id annotation must be found within an entity mapping, or it would be invalid. The same is not true for mapped superclasses and embeddables, though for these defaulting to PROPERTY is fine: the JPA spec says they should work the same as if they were defined either FIELD or PROPERTY, so the executing the enhancement check seems like the right thing to do.

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

scottmarlow · 2024-12-12T16:04:35Z

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

 					}
 				}
-			}
-			if ( propertyHasAnnotation && !propertyNameMatchesFieldName ) {


I find myself reading https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a113 again for checking this section. My question here is whether we can really remove the check if any method has a Persistence annotation? With the current code in this change determines the access type is PROPERTY than I guess its not FIELD. I'd like to have more confidence but we should move forward with this change and correct later if needed.

I don't understand the question.

Pasting from spec:

The explicit access type may be overridden at the attribute level. That is, a class which explicitly specifies an access type using the Access annotation may also have fields or properties annotated Access, and so the class may have a mix of access types.

When Access(FIELD) is specified at the class level, an individual attribute within the class may be selectively designated for property access by annotating a property getter Access(PROPERTY). Mapping annotations for this attribute must be placed on the getter. If a mapping annotation is placed on a property getter which is not annotated Access(PROPERTY), the behavior is undefined.

When Access(PROPERTY) is specified at the class level, an individual attribute within the class may be selectively designated for field access by annotating an instance variable Access(FIELD). Mapping annotations for this attribute must be placed on the field. If a mapping annotation is placed on a field which is not annotated Access(FIELD), the behavior is undefined.

Question is does ^ mean we need to check property accessors even if the class is tagged with access type FIELD?

According to the spec you only need to check fields and properties for @Access if the class is explicitly annotated @Access. If an entity class has no class-level @Access annotation you simply don't need to search the fields/properties for @Access. That's what this means:

The explicit access type may be overridden at the attribute level. That is, a class which explicitly specifies an access type using the Access annotation may also have fields or properties annotated Access

Now, in Hibernate the rule is a bit different I believe, because @sebersole added additional semantics.

I ran some local Jakarta EE 10 TCK tests and I'm seeing a TCK failure reported by https://hibernate.atlassian.net/browse/HHH-18928:

[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_appmanaged
[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_appmanagedNoTx
[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_pmservlet
[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_puservlet
[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_stateful3
[javatest.batch] FAILED........com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#getJavaMember_from_stateless3

In HHH-18928 I mentioned:

It looks like we are enhancing https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jpa/core/metamodelapi/attribute/Order.java but I think the default access type for Order entity is PROPERTY so with this change shouldn't we not enhance https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jpa/core/metamodelapi/attribute/Order.java?

Note that https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jpa/core/metamodelapi/attribute/Order.java#L47 has an ID annotation so the default access type should be PROPERTY:

@Id public int getId() { return id; }

From test output for https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jpa/core/metamodelapi/attribute/Client.java#L248C16-L248C36:

[javatest.batch] 12-12-2024 11:28:24: SVR-TRACE: attribute JavaMember = total
[javatest.batch] 12-12-2024 11:28:24: SVR-ERROR: Expected: getTotal, actual:total

I'm not 100% sure if this is a bug or should be a challenge.

From the TCK description of the test assertion:

<assertion required="true" impl-spec="false" status="active" testable="true"> <id>PERSISTENCE:JAVADOC:1214</id> <description>Return the java.lang.reflect.Member for the represented attribute.</description> <package>jakarta.persistence.metamodel</package> <class-interface>Attribute</class-interface> <method name="getJavaMember" return-type="java.lang.reflect.Member"/> </assertion>

^ doesn't need to be fixed in 2024 but I'd like to understand if it can be fixed early in 2025. ^ is addressed by the #9405 change but I think #9405 needs more work, including syncing up with changes from #9372

@scottmarlow right, Order has an @Id annotation so it should default to AccessType.PROPERTY. However, the TCK test seems to assert the java member returned by the metamodel, so I do not think it's related with the bytecode enhancement process at all; maybe there's an issue somewhere else in how Hibernate determines which member to return.

You can try creating a test in Hibernate with the same entity mapping and the same assertions, and see if it fails. Then, we can try to identify the cause of this behavior.

@scottmarlow right, Order has an @Id annotation so it should default to AccessType.PROPERTY. However, the TCK test seems to assert the java member returned by the metamodel, so I do not think it's related with the bytecode enhancement process at all; maybe there's an issue somewhere else in how Hibernate determines which member to return.

For what it is worth, the TCK test passes if the Order class is not enhanced. With my #9405 change, I had added an else if access type == PROPERTY check that would ensure we do not enhance the Order class. Of course, this also caused around 20-30 test failures as well (I forget the exact count) so my change was incomplete. Paste of code from that PR:

if ( propertyHasAnnotation && !propertyNameMatchesFieldName ) { switch ( strategy ) { case SKIP: @@ -501,11 +528,71 @@ else if (methodName.startsWith("get") || // We shouldn't even be in this method if using LEGACY, see top of this method. throw new AssertionFailure( "Unexpected strategy at this point: " + strategy ); } } else if ( propertyHasAnnotation && (defaultAccessType == null || AccessType.PROPERTY.equals( defaultAccessType ) ) ) {

Also worth noting that the TCK test is calling https://jakarta.ee/specifications/persistence/3.2/apidocs/jakarta.persistence/jakarta/persistence/metamodel/attribute#getJavaMember() which returns a https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/reflect/Member.html so Java reflection is involved and we need to understand how/why that returns JavaMember that returns the wrong name from the JavaMember.getName() call.

You can try creating a test in Hibernate with the same entity mapping and the same assertions, and see if it fails. Then, we can try to identify the cause of this behavior.

Is there an existing Hibernate ORM test that combines metamodel + bytecode enhancement that we could update to also do what this TCK does?

scottmarlow · 2024-12-12T17:04:03Z

HHH-18928 also fails without this pull request, didn't mean it was caused by this change.

…

On Thu, Dec 12, 2024, 11:22 AM Gavin King ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java <#9372 (comment)> : > } } } - } - if ( propertyHasAnnotation && !propertyNameMatchesFieldName ) { According to the spec you *only* need to check fields and properties for @access if the class is explicitly annotated @access. If an entity class has no class-level @access annotation you simply don't need to search the fields/properties for @access. That's what this means: The explicit access type may be overridden at the attribute level. That is, a class which explicitly specifies an access type using the Access annotation may also have fields or properties annotated Access Now, in *Hibernate* the rule is a bit different I believe, because @sebersole <https://github.com/sebersole> added additional semantics. — Reply to this email directly, view it on GitHub <#9372 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACRJED24MY3KCQ43KWRFET2FGZ4FAVCNFSM6AAAAABTAYRUKWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBQGE2TQOBYHE> . You are receiving this because you commented.Message ID: ***@***.***>

github-advanced-security bot found potential problems Dec 4, 2024

View reviewed changes

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java Fixed Show fixed Hide fixed

mbellade mentioned this pull request Dec 6, 2024

HHH-18917 Follow all of the JavaBeans rules in enhance/internal/bytebuddy/EnhancerImpl when checking if a class can be enhanced #9381

Merged

mbellade added 2 commits December 9, 2024 10:33

HHH-18903 HHH-18904 Add test for issue

42696de

HHH-18903 HHH-18904 Improve accessor methods check during enhancement

4c5803f

mbellade force-pushed the HHH-18904 branch from 8edf5d4 to 4c5803f Compare December 9, 2024 09:40

beikov approved these changes Dec 9, 2024

View reviewed changes

mbellade merged commit 0ee78b5 into hibernate:main Dec 10, 2024
23 of 25 checks passed

mbellade deleted the HHH-18904 branch December 10, 2024 09:41

scottmarlow reviewed Dec 12, 2024

View reviewed changes

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java Show resolved Hide resolved

scottmarlow reviewed Dec 12, 2024

View reviewed changes

...rnate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java Show resolved Hide resolved

scottmarlow reviewed Dec 12, 2024

View reviewed changes

Uh oh!

HHH-18904 Improve check for unsupported property access properties during bytecode-enhancement #9372

HHH-18904 Improve check for unsupported property access properties during bytecode-enhancement #9372

Uh oh!

Conversation

mbellade commented Dec 4, 2024 • edited by hibernate-github-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbellade commented Dec 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmarlow Dec 12, 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 Dec 12, 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!

scottmarlow commented Dec 12, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mbellade commented Dec 4, 2024 •

edited by hibernate-github-bot bot

Loading

scottmarlow Dec 12, 2024 •

edited

Loading

gavinking Dec 12, 2024 •

edited

Loading