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.

Sign up for GitHub

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

Jump to bottom

HHH-18274 generics generalized solution #8574

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

HHH-18274 generics generalized solution #8574

wants to merge 2 commits into from
+232 −9

Conversation

cigaly
Copy link
Contributor

@cigaly cigaly commented Jun 17, 2024 •
edited by hibernate-github-bot bot
Loading

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Jun 17, 2024 •
edited
Loading

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from bbdbb9b to a293fe9 Compare June 17, 2024 08:53
@cigaly cigaly changed the title HHH xxxxx generics generalized solution HHH-18274 generics generalized solution Jun 17, 2024
mbladel
mbladel requested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@mbladel mbladel left a comment

Choose a reason for hiding this comment

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

We should not resolve the concrete generic attribute when finding the attribute by name, as this will break JPA spec compliance regarding the type of the attribute object (which should represent the way it was defined in the declaring supertype, even if it was generic). Find more details in my comment on the Jira issue.

Please try resolving the type-checking issues you found locally, and do not change test which correctly assert the JPA attribute's expected type.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from a293fe9 to d467e77 Compare June 24, 2024 09:37
@cigaly
Copy link
Contributor Author

cigaly commented Jun 24, 2024

If this is violating compliance, then I was wrong from the very beginning :-( I've removed all commits except the one with new test class.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from d467e77 to 1fb6297 Compare June 24, 2024 11:13
@cigaly
Copy link
Contributor Author

cigaly commented Jun 24, 2024

Added new commit containing fix for binary and unary expressions. Now only tests with function call are failing.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from 1fb6297 to 163acbc Compare June 25, 2024 13:41
mbladel
mbladel reviewed Jun 25, 2024
View reviewed changes
.../main/java/org/hibernate/query/sqm/produce/function/StandardFunctionReturnTypeResolvers.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/tree/expression/SqmBinaryArithmetic.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/tree/expression/SqmBinaryArithmetic.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/tree/expression/SqmUnaryOperation.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/internal/TypecheckUtil.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/internal/TypecheckUtil.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/query/sqm/internal/TypecheckUtil.java Outdated Show resolved Hide resolved
@cigaly
Copy link
Contributor Author

cigaly commented Jun 25, 2024

All tests passed, now I will apply suggested changes.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from 5cdfe51 to 3b8af03 Compare July 1, 2024 10:39
@cigaly
Copy link
Contributor Author

cigaly commented Jul 1, 2024 •
edited
Loading

All tests are now passing with exception of three (out of four) tests in org.hibernate.orm.test.mapping.converted.converter.ConvertedAttributesTypecheckTest. This will be fixed when pull request #8681 is merged.

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from 3b8af03 to 430b12b Compare July 10, 2024 11:55
@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from 430b12b to e6967e3 Compare August 1, 2024 04:24
@cigaly cigaly marked this pull request as ready for review August 1, 2024 05:19
@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from e6967e3 to a82d636 Compare August 9, 2024 16:48
@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from a82d636 to 4082c9a Compare August 30, 2024 07:09
@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch 7 times, most recently from 1917357 to c4589ed Compare September 19, 2024 06:25
mbladel
mbladel approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@mbladel mbladel left a comment

Choose a reason for hiding this comment

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

This is much cleaner and looks ready to me, if you could also squash the last 2 commits would be great.

wdyt @beikov?

@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from c4589ed to 589826b Compare September 19, 2024 07:26
@cigaly
HHH-18274 Test similar to org.hibernate.orm.test.query.hql.instantiat…
73a8ff2 
…ion.InstantiationWithGenericsTest,

           but testing different operations using generic field overridden in subclass

Test class org.hibernate.orm.test.query.hql.instantiation.InstantiationWithGenericsExpressionTest is derived from
org.hibernate.orm.test.query.hql.instantiation.InstantiationWithGenericsTest by adding test cases for
binary and unary expressions, and function call.
@cigaly
HHH-18274 Properly resolving Java type in binary and unary expressions
4c6f568
@cigaly cigaly force-pushed the HHH-xxxxx-Generics-generalized_solution branch from 589826b to 4c6f568 Compare November 24, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbladel mbladel mbladel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cigaly @mbladel