jdaugherty commented on code in PR #16719: URL: https://github.com/apache/grails-core/pull/16719#discussion_r4413542722
########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlText.java: ########## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.query; + +import java.util.Locale; +import java.util.Set; + +import jakarta.annotation.Nullable; + +/** HQL text and its top-level projection analysis, without changing the text sent to Hibernate. */ +public final class HqlText { Review Comment: This is a second `HqlText`. #16735 merged into 8.0.x on 10-24 with `org.grails.orm.hibernate.query.HqlText` (package-private; `HqlText.of(hql)`, `target()`, `projectionCount()`, `singleProjectionClause()`, `rootAlias()`, `normalized()`), which scans through `SqlText`, and `HqlQueryContext.prepare` already reads every HQL query through it once. This branch is still based on 75a355a, so this file and `HqlTextSpec.groovy` are add/add conflicts and `HqlQueryContext.java` a content conflict. Please rebase onto 8.0.x. This class and the first commit's moves then go away, and what is left is the root unwrap and the first-branch rule inside the existing `HqlText`: the merged `SqlText` has no `unwrapOuterParentheses`, so that part is still to be written (its `depth` array gives the matching `)` of a leading `(` directly). Your rows go into the existing `HqlTextSpec` table, which also has a `normalized` column. One constraint to carry over: on 8.0.x `prepare` sends `reading.normalized()` to Hibernate, so the unwrap has to stay analysis-only, and `normalized()` of `(A) union (B)` must remain the whole text, not `A`. Your two new `HqlQueryContextSpec` rows (`ctx.hql() == hql`) pin exactly that; keep them. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlText.java: ########## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.query; + +import java.util.Locale; +import java.util.Set; + +import jakarta.annotation.Nullable; + +/** HQL text and its top-level projection analysis, without changing the text sent to Hibernate. */ +public final class HqlText { + + private final String hql; + private final String text; + private final String lower; + private final int selectIndex; + private final int fromIndex; + + public HqlText(@Nullable CharSequence hql) { + this(hql, true); + } + + private HqlText(@Nullable CharSequence hql, boolean analyzeRoot) { + this.hql = hql == null ? null : hql.toString(); + this.text = this.hql == null ? "" : analyzeRoot ? unwrapRootQuery(this.hql) : this.hql.trim(); + this.lower = text.toLowerCase(Locale.ROOT); + this.selectIndex = indexOfTopLevel(lower, HibernateQueryArgument.HQL_SELECT.value() + " ", 0); + this.fromIndex = indexOfTopLevel(lower, " " + HibernateQueryArgument.HQL_FROM.value() + " ", + Math.max(selectIndex, 0)); + } + + public @Nullable String hql() { + return hql; + } + + public Class<?> getTarget(Class<?> entityClass) { + return switch (countHqlProjections()) { + case 0 -> entityClass; + case 1 -> { + String clause = getSingleProjectionClause(); + if (clause.startsWith("count(") || clause.startsWith("sum(") || + clause.startsWith("avg(") || clause.startsWith("min(") || + clause.startsWith("max(")) { + yield null; + } + String rootAlias = getRootAlias(); + boolean rootProjection = rootAlias != null ? + clause.equals(rootAlias.toLowerCase(Locale.ROOT)) : + clause.equals(getEntityName().toLowerCase(Locale.ROOT)); + yield rootProjection ? entityClass : Object.class; + } + default -> Object[].class; + }; + } + + public int countHqlProjections() { + String clause = getSingleProjectionClause(); + if (clause == null || clause.isEmpty()) return 0; + return getCommas(clause) == 0 ? 1 : 2; + } + + public @Nullable String getSingleProjectionClause() { + if (selectIndex < 0) return null; + return extractSelectClause(lower, selectIndex, fromIndex); + } + + static String extractSelectClause(String text, int selectIndex, int fromIndex) { + String clause = text.substring(selectIndex + HibernateQueryArgument.HQL_SELECT.value().length(), + fromIndex < 0 ? text.length() : fromIndex).trim(); + if (clause.startsWith(HibernateQueryArgument.HQL_DISTINCT.value() + " ")) { + clause = clause.substring(HibernateQueryArgument.HQL_DISTINCT.value().length() + 1).trim(); + } else if (clause.startsWith(HibernateQueryArgument.HQL_ALL.value() + " ")) { + clause = clause.substring(HibernateQueryArgument.HQL_ALL.value().length() + 1).trim(); + } + return clause; + } + + static int getCommas(String clause) { + int depth = 0; + int commas = 0; + int index = 0; + while (index < clause.length()) { + int skipped = skipIgnored(clause, index); + if (skipped != index) { + index = skipped; + continue; + } + char current = clause.charAt(index); + if (current == '(') { + depth++; + } else if (current == ')' && depth > 0) { + depth--; + } else if (current == ',' && depth == 0) { + commas++; + } + index++; + } + return commas; + } + + private static String unwrapRootQuery(String hql) { + String text = hql.trim(); + while (text.startsWith("(")) { + int depth = 0; + int closing = -1; + int index = 0; + while (index < text.length()) { + int skipped = skipIgnored(text, index); + if (skipped != index) { + index = skipped; + continue; + } + char current = text.charAt(index); + if (current == '(') { + depth++; + } else if (current == ')') { + depth--; + if (depth == 0) { + closing = index; + break; + } + } + index++; + } + if (closing < 0) return text; + int next = skipWhitespaceAndComments(text, closing + 1); + if (next < text.length() && !isSetOperation(text, next)) return text; Review Comment: A leading group followed by anything other than a set operator still hides the `select`, so the entity is chosen. Hibernate accepts a clause after a parenthesized query expression (`order by`, `limit`, `offset`, `fetch`), and on this branch ```groovy HqlResultTypeBook.executeQuery('(select b.title from HqlResultTypeBook b) limit 1') ``` fails with `QueryTypeMismatchException: Incorrect query result type: query produces 'java.lang.String' but type 'grails.gorm.tests.HqlResultTypeBook' was given`, the failure of #16712. Nothing that may follow a leading group at the top level changes the shape of the result: a set operator's other branches have the same columns, and an ordering or limit clause selects rows. So the first group can be analyzed whatever follows it, without this guard; an invalid tail is Hibernate's syntax error either way. Please add the `limit` row to `HqlSelectResultTypeSpec`. ########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/query/HqlTextSpec.groovy: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.query + +import spock.lang.Specification + +class HqlTextSpec extends Specification { + + void 'analyze the root projection without rewriting #hql'() { + given: + HqlText text = new HqlText(hql) + + expect: + text.hql() == hql + text.countHqlProjections() == projections + text.singleProjectionClause == clause + text.getTarget(Number) == target + + where: + hql | projections | clause | target + null | 0 | null | Number + '' | 0 | null | Number + 'from Book b' | 0 | null | Number + '(select b.title from Book b)' | 1 | 'b.title' | Object + '((select b.title from Book b))' | 1 | 'b.title' | Object + '( select b.title from Book b )' | 1 | 'b.title' | Object + '(select b.title from Book b) union (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) union all (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) intersect (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) intersect all (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) except (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) except all (select a.name from Author a)' | 1 | 'b.title' | Object + '((select b.title from Book b) UNION ALL (select a.name from Author a))' | 1 | 'b.title' | Object + '(select b.title from Book b) /* ) select ignored from ignored */ union (select a.name from Author a)' | 1 | 'b.title' | Object + '(select b.title from Book b) /* ) select ignored from ignored\n */ union (select a.name from Author a)' | 1 | 'b.title' | Object + "(select b.title from Book b where b.title <> ')')" | 1 | 'b.title' | Object + "(select 'it''s ), select x from y' from Book b)" | 1 | "'it''s ), select x from y'" | Object + '(select b."title), from ignored" from Book b)' | 1 | 'b."title), from ignored"' | Object + '(select b.`title), from ignored` from Book b)' | 1 | 'b.`title), from ignored`' | Object + '(/* ignore ) and ( select x from y */ select b.title from Book b)' | 1 | 'b.title' | Object + '(/* ignore ) and ( select x from y\n */ select b.title from Book b)' | 1 | 'b.title' | Object + '(select b.title /* ), ignored */ from Book b)' | 1 | 'b.title /* ), ignored */' | Object + '(select b.pages--1 from Book b)' | 1 | 'b.pages--1' | Object Review Comment: Already pinned on 8.0.x: `HqlCommentsSpec` runs Hibernate's parser on `b.height--1`, the merged `HqlTextSpec` has the `b.height--1 > 0 select b.width` row, and `SqlSyntax.HQL` has no line comments. Fine to keep as the parenthesized variant after the rebase. ########## grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/HqlSelectResultTypeSpec.groovy: ########## @@ -83,5 +108,7 @@ class HqlResultTypeAuthor { class HqlResultTypeBook implements HibernateEntity<HqlResultTypeBook> { String title + int pages Review Comment: `pages` is written by `setup()` but no executed query reads it; it only appears in `HqlTextSpec` strings, which are never sent to Hibernate. Drop the property and the three `pages:` values. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
