Repository navigation
Hibernate 7: give criterion handlers the query context and let a handler order by an expression (8.1.x) #16733
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
83b6c5d
fix(hibernate7): leave the criteria's orders out of a count query
borinquenkid 66f1f26
Hibernate 7: give criterion handlers the query context and let a hand…
borinquenkid 1e47f7e
Hibernate 7: let a handler order by the expression a sqlProjection re…
borinquenkid 54a4454
Hibernate 7: answer the review of the handler context: a closure regi…
borinquenkid 3bdd024
Hibernate 7: getFullyQualifiedExpression resolves like the built-in p…
borinquenkid a46a5c9
Merge branch '8.1.x' into feat/criteria-handler-context-8.1.x
borinquenkid 591bde1
Merge branch 'origin/8.1.x' into feat/criteria-handler-context-8.1.x
borinquenkid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
getFullyQualifiedExpression('total')still asks only the property resolver. ThesqlProjectionalias is recorded withregisterSelectionAlias, so this call returns null, or an entity property namedtotal. The context javadoc says this method accepts the alias of a projection.Return the recorded selection when the path is that alias. Please order through
getFullyQualifiedExpression('total'), not onlygetSelectionAlias.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 probed this at 1e47f7e to see what each lookup returns for a
sqlProjectionalias, withwidth * 10 as scaledwhere noscaledproperty exists andwidth * 10 as heightwhereheightis a property:getFullyQualifiedExpressiongetSelectionAliasorder(alias)eq(alias, …)scalednull;cb.desc(null)then fails with "Ambiguous method overloading for SqmCriteriaNodeBuilder#desc"order by 1 descheightorder by spb1_0.height descorder by 1 descwhere spb1_0.height=?For
sqlGroupProjection 'width, sum(height) as total', a handler usinggetSelectionAlias('total')givesorder by 2 desc, the same SQL asorder('total', 'desc').The two built-in columns of that table are not an accident of the translator. They are the rule SQL, Hibernate 5 and this module already follow:
select width * 10 as height from t order by height descreturns 40, 20;... where height = 9restricts the column;... where scaled = 20fails withColumn "SCALED" not found.CriterionHandlerresolve property paths and order by expressions #16563 is porting from, does the same.Order.toSqlStringresolves throughCriteriaQueryTranslator.getColumnsUsingProjection, which first looks for a projection alias and then for a property;SimpleExpression.toSqlString, behindeqand the other restrictions, resolves throughfindColumns, which looks for a property only (hibernate-core 5.6.11).sqlProjectionlanded:assignOrderBychecks thesqlProjectionalias before the property, andregisterSelectionAliaskeeps the alias out of property resolution, with "Column aliases never shadow properties in restrictions" in cb1f659 (Hibernate 7: support sqlProjection and sqlGroupProjection in criteria queries #16548).Folding the selection into
getFullyQualifiedExpressionchanges that behavior, because the built-in predicates resolve through the same method andassignCriteriaruns after the projections are registered. Selection first makeseq('height', 9)restrict the projection instead of the column on every query whose projection alias matches a property, which is the shadowing the commit above rules out and SQL does not do. Selection as a fallback keepseqas it is, but then a custom order resolvesheightto the property whereorder('height')resolves it to the projection, so the built-in order and the handler order the same alias differently.Two lookups avoid both. An order handler that checks
getSelectionAliasfirst andgetFullyQualifiedExpressionsecond orders exactly asorder(alias)and as Hibernate 5'sOrderdo, and a criterion handler that usesgetFullyQualifiedExpressionalone restricts exactly aseqdoes. So I would keep the accessor this commit adds rather than change the resolver.What is wrong at this commit is the contract. The
QueryPathContext.getFullyQualifiedExpressionjavadoc says it takes "the alias of a projection" and returns "the expression an alias or projection stands for". That holds for an alias the translator registers withregisterAliasand not for the column alias of asqlProjection, which is where this thread started. Could the javadoc say which aliases it resolves and point togetSelectionAliasfor the column alias of asqlProjectionorsqlGroupProjection, and could the guide's order-handler paragraph say to checkgetSelectionAliasfirst, the wayorder(alias)does?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.
Same change as the thread above (54a4454): the path resolves to the recorded selection when no property has the name, and the order goes through
getFullyQualifiedExpression('total')in a spec of its own.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.
Reverted in 3bdd024, on jdaugherty's analysis in his reply to your second comment:
getFullyQualifiedExpressionresolves as the built-in predicates do, so a projection alias is not folded into it. Withwidth * 10 as height, resolving the alias there would makeeq('height', 9)restrict the projection instead of the column, which SQL does not do (a select alias is visible toorder by, not towhere) and neither does Hibernate 5. Your first request, to expose a selection lookup and leave predicate resolution alone, is whatgetSelectionAliasis. An order handler asks it first andgetFullyQualifiedExpressionsecond, and then orders exactly asorder(alias)does, forscaled(no such property) and forheight(a property). The javadoc and the guide say so, andHibernateCriteriaBuilderSqlProjectionSpecpins it, including thateq('height', 9)restricts the property.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.
Done in 3bdd024: the resolver is back to what it was, and
QueryPathContext.getFullyQualifiedExpressiondocuments which aliases it resolves and points togetSelectionAliasfor the column alias of asqlProjectionorsqlGroupProjection. The guide's order-handler paragraph says to askgetSelectionAliasfirst andgetFullyQualifiedExpressionsecond, and that a criterion handler asks the second alone. The new feature inHibernateCriteriaBuilderSqlProjectionSpecruns your two cases (scaled, which no property has, andheight, which one has) and theeq('height', 9)that must restrict the column.