Skip to content
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

Conversation

scottmarlow
Copy link
Member

@scottmarlow
Copy link
Member Author

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:

assertThat( entity )
				.extracting( NEXT_FIELD_NAME ).isNull();

@scottmarlow scottmarlow force-pushed the HHH-16572_filter_property_enhancement branch from 87e12da to 26e7958 Compare October 24, 2024 17:59
boolean result = false;
// Check for use of ID/AccessType(PROPERTY) on methods

for (MethodDescription.InDefinedShape shape : managedCtClass.getDeclaredMethods()) {
Copy link
Member Author

@scottmarlow scottmarlow Oct 24, 2024

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

Copy link
Member

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.

Copy link
Member Author

@scottmarlow scottmarlow Oct 31, 2024

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()



Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Oct 30, 2024

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@scottmarlow scottmarlow force-pushed the HHH-16572_filter_property_enhancement branch 3 times, most recently from 55d4385 to 9fd9d8e Compare November 3, 2024 09:23
@scottmarlow scottmarlow force-pushed the HHH-16572_filter_property_enhancement branch from 9fd9d8e to f09fecd Compare November 4, 2024 15:38
@scottmarlow
Copy link
Member Author

Change is merged as faebabd

@scottmarlow scottmarlow closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants