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-18829 Auto generate record ID class if no explicit @IdClass #9220

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cigaly
Copy link
Contributor

@cigaly cigaly commented Nov 8, 2024

Jira Issue HHH-18829

This is "sketch" of the problem solution on the side of Hibernate Processor.

TODO : Change Hibernate Core to use generated ID class.


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-18384

@cigaly
Copy link
Contributor Author

cigaly commented Nov 8, 2024

Last two commits are idea for potential solution on Hibernate Core side.

pw.println( ") {}" );
pw.println();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice and simple, for sure, but it's kind-of a layer-breaker. It would be better to make a new subclass of AnnotationMeta for this, even though I admit that's a PITA.

[I don't love the architecture, but it's not terrible, and it's what we've inherited.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but I think that generated ID class should be member of generated entity (or embeddable?) meta class. Then it should be implementing MetaAttribute which is less than half PITA :-)

Additional thing is that order of record components should be preserved. At least one test in core failed due to that.

@cigaly cigaly force-pushed the HHH-18829 branch 2 times, most recently from 0070126 to 424ba15 Compare November 16, 2024 20:39
@cigaly
Copy link
Contributor Author

cigaly commented Nov 17, 2024

@gavinking Can you, please check those two tests that are now failing? I think that both are either wrong or hiding problem some other than something caused by changes in this PR.

First one is org.hibernate.orm.test.annotations.derivedidentities.bidirectional.CompositeDerivedIdentityTest#testBidirectonalKeyManyToOneId. This one is failing on line 96:

        OrderLine orderLineGotten = session.get( OrderLine.class, orderLine );

Javadoc for org.hibernate.Session#get(java.lang.Class<T>, java.lang.Object) says that id parameter is "persistent instance or null". Here it is complete entity itself. I do not know if this may be acceptable, but same test will fail if comment is removed from @IdClass(OrderLinePK.class) in org.hibernate.orm.test.annotations.derivedidentities.bidirectional.OrderLine, so I have no reason to believe that this has been caused by PK changes.

The other failing test is org.hibernate.orm.test.boot.models.hbm.joinformula.JoinColumnAndFormulaTests#testHbmXmlTransformed. In mainbranch it is annotated with @FailureExpected because of the problem described in HHH-18384. For some reasons (unknown to me, at least for now), changes in this PR fix (at least partially) this problem, so exception has not been thrown. I've suggested solution to mentioned problem in PR #9273

@gavinking
Copy link
Member

@cigaly OK, so this makes perfect sense actually.

Historically Hibernate already allowed you to have an entity class without an @IdClass, in which case the only way you could do a find-by-id was to pass an instance of the entity itself. (Yes, of course this is highly inelegant.)

But now you're generating an @IdClass for this entity, so that test is broken.

What I would suggest is to simply @Exclude that package from annotation processing.

@cigaly
Copy link
Contributor Author

cigaly commented Nov 23, 2024

OK, I've added @Exclude to those three entity classes. Now just one test is failing, but this can be fixed by another PR.

It seems, according to both JPA specification and Hibernate User Guide that composite ID must be either @EmbeddedId or @IdClass. Really confusing sometimes :-)

@gavinking
Copy link
Member

Yes it's an ancient undocumented misfeature but occasionally it's sorta nice to not have to write a composite of class if you don't plan on loading the entity by id. I sometimes take advantage of it when I'm just playing around.

@cigaly
Copy link
Contributor Author

cigaly commented Nov 25, 2024

Yes, backward compatibility ... on one side you must keep it, but on other you can live with it :-)

Anyway, please correct me if I am wrong ... if we simplify OrderLinePK as

public record OrderLinePK( Order order, Product product){}

and remove comment from @IdClass(OrderLinePK.class) in OrderLine,
and finally replace in org.hibernate.orm.test.annotations.derivedidentities.bidirectional.CompositeDerivedIdentityTest#testBidirectonalKeyManyToOneId line

OrderLine orderLineGotten = session.get( OrderLine.class, orderLine );

with

OrderLine orderLineGotten = session.get( OrderLine.class, new OrderLinePK( orderLine.getOrder(), orderLine.getProduct() ) );

test should pass as it is passing when entity is used instead of id?

Is that correct or I've missed something important?

@cigaly cigaly marked this pull request as ready for review December 2, 2024 09:53
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