Repository navigation
Hibernate 7: one scanner for the query package, and keep a spaced sqlProjection alias and a bracket identifier with an apostrophe working - #16735
Conversation
…Projection alias and a bracket identifier with an apostrophe working Fixes #16717. Starts #16616 with its step 0. Three scanners in org.grails.orm.hibernate.query each decided on their own what to skip: HqlQueryContext.indexOfTopLevel and getCommas, SqlRestriction's placeholder scan, and SqlProjection.splitColumns. Every recent bug of this family was a rule mismatch between them (#16597, #16712, #16717). SqlText now owns the rules once and reads the text once: when an instance is created, each character is classified as plain text, quoted text or comment, and each plain character gets its parenthesis depth, so every later question (indexOfTopLevel, splitTopLevel, countTopLevel, plainIndexes, unwrapOuterParentheses, withoutTrailingComments, endsInLineComment) walks the two arrays and nothing rescans the string. The rules: string literals and quoted identifiers with a doubled quote for the quote; in native SQL also [bracket identifiers] with ]] and dollar-quoted strings; line and block comments; parentheses for a top-level position. The three scanners delegate to it, and HqlQueryContext creates one instance per operation instead of lowercasing and rescanning for the select, the from and the commas. The two defects of #16717: - A sqlProjection alias with a space broke a count over the projection: JpaProjectionAdapter gave a subquery selection a generated alias only when it had none, so the derived table of `createCriteria().count { projections { sqlProjection ... } }` was rendered as derived1_0(box width), which H2 rejects. Nothing reads a subquery's selections by alias, so every subquery selection now gets a generated one; the main query keeps the caller's alias. - An apostrophe inside a bracket identifier swallowed the rest of the SQL, because quotes were tested before the bracket state. Inside brackets only brackets count now. Two behaviours improve as a side effect: the HQL projection count ignores a comma inside a comment or a backquoted identifier, and the HQL keyword search skips line comments. unwrapOuterParentheses is in place for #16719. SqlTextSpec holds one table per capability; SqlProjectionSpec and HqlQueryContextSpec gain the rows the fix adds; the sqlProjection integration spec gains the count over a spaced alias, which fails before the fix.
HqlQueryContext.prepare read the same query four times through separate static helpers: normalise it, then inside getTarget normalise the normalised text again, count its projections, take its single projection clause and find its root alias, each one lowercasing and scanning the text on its own. HqlText is one reading: it lowercases once, scans once with SqlText, finds the select and the from once, and derives the projection count, the select clause, the root alias, the normalized form and the result type of that normalized form from those positions, without reading the normalized text. prepare creates one HqlText and takes the query text and the target class from it. The static helpers the spec calls remain as thin entry points, so HqlQueryContextSpec keeps its 64 cases as the safety net; HqlTextSpec shows the one-reading contract on a table of queries. rootAlias() also reads a query that starts with from, which the old search for " from " never did.
HqlText carried the keywords as six string constants and a hand-kept set of the ones that start a clause, all pulled from HibernateQueryArgument, whose other entries are query arguments (max, offset, sort, fetch) and whose HQL_* entries nothing else used. HqlKeyword holds the text and the one fact the reader needs about each keyword: whether it starts a clause after the root entity, which is what makes a token there a clause and not an alias. The HQL_* entries of HibernateQueryArgument are deprecated for removal in 9.0, since the enum is public.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16735 +/- ##
==================================================
+ Coverage 62.1821% 62.2227% +0.0405%
- Complexity 26900 26925 +25
==================================================
Files 2241 2241
Lines 110736 110741 +5
Branches 20218 20218
==================================================
+ Hits 68858 68906 +48
+ Misses 32615 32572 -43
Partials 9263 9263 🚀 New features to boost your workflow:
|
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. The spaced alias and [box's width] cases are fixed. The shared scanner treats array brackets as quoted identifiers, so previously accepted array SQL is now one column or loses its placeholders. Native SQL is also analyzed as HQL and can throw before it reaches Hibernate.
…ier, and native SQL is never read as HQL From the review of #16735. The scanner read every [ in SQL as the start of a bracket identifier, so ARRAY['['], height was one column and the placeholders of ANY(ARRAY[?, ?]) were hidden, both of which worked before it. A [ now opens an identifier only where nothing to subscript precedes it: after an identifier, a closing parenthesis or a closing bracket it is structure, grouping like a parenthesis, so [box's width] stays one identifier while the string literals and placeholders inside an array literal stay visible. HQL groups brackets the same way and still has no bracket identifiers. HqlQueryContext.prepare read a native query as HQL, and HqlText lowercased with String.toLowerCase, which lengthens a dotted capital I and made the reader index the original past its end. Native SQL is no longer read at all, and the reader lowercases character by character so every index of the lowercase text is an index of the original. Covered through the criteria API (an array literal with a bracket in a sqlProjection, placeholders inside an array in a sqlRestriction), through findAllWithSql with such a literal, and in the unit specs of SqlText, SqlProjection, HqlText and HqlQueryContext.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier comments are fixed. ARRAY['['], height splits, ARRAY[?, ?] keeps both placeholders, and native SQL is not read as HQL. [box's width] and the spaced count alias still work. The targeted specs passed: 238 tests, 0 failures. Not approving. Two other bracket forms still hide placeholders.
| if (index == 0) { | ||
| return true; | ||
| } | ||
| char previous = text.charAt(index - 1); |
There was a problem hiding this comment.
This looks only at the character before [. ANY(ARRAY [CAST(? AS INTEGER)]) and "items"[?] = ? are treated as bracket identifiers, so the ? is hidden and sqlRestriction rejects the values. Both worked before this scanner.
Decide from the preceding token, not one character. A space or a quoted name before [ can still open an array.
There was a problem hiding this comment.
Replaced in e3c7fc7, not refined: there is no lookback any more. SqlText now reads a text in a SqlSyntax resolved from the Hibernate dialect, the way RegexDialectPattern picks the regex operator. Under TRANSACT_SQL (SQL Server, Sybase) a [ always opens an identifier; under SQL (everything else) brackets nest like parentheses, since those databases have arrays and no bracket identifiers. ANY(ARRAY [CAST(? AS INTEGER)]) and "items"[?] = ? keep their placeholders on H2 and PostgreSQL, where they are valid, and HibernateCriteriaBuilderSqlRestrictionSpec binds both against H2. The builder resolves the syntax from its session factory, the function descriptors from the function contributions, the query creator from Hibernate's criteria builder and the detached-criteria extension from the entity's datastore, so no reader guesses.
| int i = start + 1; | ||
| while (i < text.length()) { | ||
| char c = text.charAt(i); | ||
| if (c == '[') { |
There was a problem hiding this comment.
[a[b] = ? is one SQL Server identifier. The inner [ raises the depth, so the scanner never sees the ?. A bracket identifier closes at the first ] that is not escaped as ]].
Do not nest identifier brackets. Array nesting is already a separate path.
There was a problem hiding this comment.
Fixed in e3c7fc7: a bracket identifier closes at the first ] not written ]] and never nests, and only exists under the Transact-SQL syntax. [a[b] = ? is one identifier and one placeholder in SqlTextSpec and SqlRestrictionSpec, and HibernateCriteriaBuilderTransactSqlSpec runs H2 in its SQL Server compatibility mode under SQLServerDialect, so {alias}.[width] > ? renders as written with the value bound and a ? inside brackets is rejected as a value. Array nesting stays the separate path, under the other syntax.
…from its dialect A bracket may quote an identifier or open an array, and the two never coexist on one database, so SqlText no longer guesses from the character before it. SqlSyntax, keyed on the Hibernate dialect, says what a text may hold: TRANSACT_SQL (SQL Server, Sybase) quotes identifiers with brackets, closed by the first ] not written ]], never nested; SQL (every other database) nests brackets like parentheses, so ANY(ARRAY [CAST(? AS INTEGER)]) and "items"[?] keep their placeholders; HQL has neither. Every reader resolves the syntax from what it holds: the criteria builder from its session factory, the query creator from Hibernate's criteria builder, the function contributors from the function contributions, the detached-criteria extension from the entity's datastore. SqlProjection.of, SqlGroupProjection.of and the new SqlRestriction.of take it; the restriction record no longer counts placeholders itself. The alias and unquoting regexes of SqlProjection move into SqlText, so nothing outside it scans SQL. The dialect resolution shared by RegexDialectPattern and SqlSyntax is one interface, DialectKeyed, which the shared join table uniqueness of #16701 adopts after this. HibernateCriteriaBuilderTransactSqlSpec runs H2 in its SQL Server compatibility mode under SQLServerDialect, so bracket identifiers, a ? inside one, a bracket alias with an apostrophe and a bracket group-by name go through the real render path. The H2 and container specs that used bracket aliases on databases without bracket identifiers use double quotes and backquotes now.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier bracket comments are fixed. A space before [ and a [ inside a bracket identifier no longer hide placeholders. ARRAY['['] and [box's width] still split. Not approving. Two new failures reject queries that used to run.
…bernate does The scanner read every database the same way, so a string, a comment or a placeholder the database reads differently was misread, and HQL was read with comment rules it does not have. SqlSyntax now has a constant per family of databases, resolved from the Hibernate dialect, and a set of lexical features: - bracket identifiers on SQL Server and Sybase, array brackets elsewhere; - backslash escapes in MySQL and MariaDB strings, and in PostgreSQL E'...' strings only; - dollar quoted strings on PostgreSQL ($tag$) and H2 ($$ only), q'[...]' strings on Oracle; - # comments on MySQL, where -- needs a space or control character after it, // comments on H2; - nested block comments on PostgreSQL and SQL Server; - ?? as a literal question mark on PostgreSQL; - an alias written as a string on MySQL and SQL Server. HQL has block comments only: Hibernate's parser rejects -- and //, so b.height--1 is a subtraction and a select after it is the select of the query (HqlCommentsSpec pins what Hibernate accepts). A detached criteria does not know its database until it runs, so it no longer counts the placeholders of its sqlRestriction against the values when it is built, with the syntax of the default connection: the count is checked where the SQL is rendered, with the syntax of the dialect of the connection it runs on. An entity on H2 by default and on SQL Server by name reads [width?] = ? as an identifier on the SQL Server connection and as an array on H2 (HibernateCriteriaBuilderConnectionSyntaxSpec). A trailing alias is stripped after a closing parenthesis, bracket or quote with no space before the as, with a comment between the as and its name, and for letters outside the basic plane and combining marks. A native query or an update is passed through untrimmed as on 8.0.x, and a subquery selection gets a generated alias only when it has none or is a SQL projection. The one argument constructor of the restriction function is kept, deprecated. The dialect resolution shared with RegexDialectPattern is DialectKeyed.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier comments are fixed. HQL no longer treats -- as a line comment, and a detached criteria no longer validates SQL against the default dialect. Not approving. A MySQL group-by string constant is rewritten as an alias.
| } | ||
| char opening = text.charAt(start); | ||
| char closing = opening == '[' ? ']' : opening; | ||
| boolean identifier = opening == '"' || opening == '`' || |
There was a problem hiding this comment.
On MySQL, GROUP BY 'total' groups by the string constant, not the alias total. assignGroupBy() now unquotes that constant and rewrites sqlGroupProjection to GROUP BY sum(height), which MySQL rejects.
Strip quotes from an AS alias. Do not treat a quoted group-by item as an alias. Please group by 'total' and assert the SQL still contains that string.
There was a problem hiding this comment.
Fixed in e394df9 and pinned in 6333d95. SqlText.unquoted() reads a string in single quotes as an identifier no more; only the alias after an as may be a string, so GROUP BY 'total' stays a literal and assignGroupBy() no longer rewrites it. SqlTextSpec pins it in every syntax, HibernateCriteriaBuilderSqlProjectionSpec asserts the SQL still groups by 'total', and SqlProjectionHibernate7Spec runs it on MySQL (it fails there without the fix).
… Hibernate names the derived column The derived table of a count names its column c0 since the count queries of 8.0.x leave the orders out; the spec asserted the generated alias col_0 of the projection adapter. It asserts that the derived table carries one of Hibernate's or the adapter's generated names and never the alias with the space.
…ral, not an alias SqlText.unquoted() read 'total' as the identifier total in MySQL, whose syntax lets an alias be written as a string, so sqlGroupProjection grouped by the aggregate that the alias named instead of by the literal. Only the alias after an as is read as a string now; the group by item is read as an identifier or left as it is. Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p
|
Fixed in e394df9: |
The criteria builder spec asserts the SQL still groups by 'total' as written. Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Approving. A single-quoted group-by item stays a literal. GROUP BY 'total' is no longer rewritten to GROUP BY sum(height). That comment is resolved.
🔎 No tests executed 🔎🏷️ Commit: a928abb Learn more about TestLens at testlens.app/docs. |
Hibernate 7: keep the key of a unidirectional hasMany updatable unless its join table is shared (fixes #16665). Includes #16735. Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p
Summary
Fixes #16717, and does the first two steps of #16616: one lexical scanner for the query package that reads each text the way its database does, and one reading per HQL query.
Three scanners in
org.grails.orm.hibernate.queryeach decided on their own what to skip, and a fourth set of regexes parsed aliases. Every recent bug of this family was a rule mismatch between them: #16597, #16712 and the second half of #16717.SqlTextowns the rules once and reads a text once: each character is classified as plain, quoted or comment, and each plain character gets its nesting depth, so every later question walks two arrays and nothing rescans the string. It offersindexOfTopLevel,splitTopLevel,countTopLevel,plainIndexes,endsInLineComment,withoutTrailingAliasandunquoted.The syntax of the database. Databases lex SQL differently, so
SqlSyntax(an enum, resolved from the Hibernate dialect throughDialectKeyed, the wayRegexDialectPatternpicks the regular expression operator) says what a text may hold:TRANSACT_SQL[...]quotes an identifier (first]not written]]closes it, no nesting); nested block comments; an alias may be a stringPOSTGRESQL$tag$...$tag$strings;E'...'escapes with a backslash; nested block comments;??is a literal?MYSQL'and"strings;#comments;--needs a space after it; an alias may be a stringORACLEq'[...]'stringsH2//comments;$$...$$stringsSQL--comments,[and]nest like parenthesesHQL/* */comments only: Hibernate's parser rejects--and//The defaults of each server are assumed; the modes that change them (
NO_BACKSLASH_ESCAPES,ANSI_QUOTES) are not read from the server.Every reader resolves the syntax from what it already holds: the criteria builder from its session factory, the query creator from Hibernate's criteria builder, the two function descriptors from the dialect they are registered for.
SqlProjection.of,SqlGroupProjection.ofandSqlRestriction.oftake it. A detached criteria does not know its database until it runs, so its restriction is built with the plain constructor and the number of placeholders is checked where the SQL is rendered, on the connection it runs on.Behavior change on a released type. The canonical constructor of the public record
SqlRestriction(8.0.0) no longer rejects a number of?placeholders that differs from the number of values: that count depends on the database's syntax, which a restriction built without a connection does not know. The check is inSqlRestriction.of(sql, values, syntax), used by the criteria builder, and where the SQL is rendered, so a detached criteria reports a mismatch when the query runs, by an exception whose cause is anIllegalArgumentException. The javadoc and the guide say so.One reading per HQL query.
HqlQueryContext.prepareread the same query four times through separate helpers.HqlTextlowercases once, scans once withSqlText, and derives the projection count, the select clause, the root alias, the normalized form and the result type from those positions. Native SQL and updates pass through as written.#16717. A column alias with a space broke the count over a
sqlProjection(the derived table named its column with the alias, now a generated one), and an apostrophe in a bracket identifier swallowed the SQL.Verification
SqlTextSpec,SqlProjectionSpec,SqlRestrictionSpec,SqlSyntaxSpec,DialectKeyedSpec: each row runs under the syntaxes it names, with the reviewers' strings under the syntax that owns them (ANY(ARRAY [CAST(? AS INTEGER)]),"items"[?],[a[b]).HqlCommentsSpecruns Hibernate's parser to pin which comments HQL accepts;HqlTextSpecandHqlQueryContextSpecpin the reading.HibernateCriteriaBuilderSqlRestrictionSpecruns//and$$..$$against a real H2;HibernateCriteriaBuilderTransactSqlSpecandHibernateCriteriaBuilderConnectionSyntaxSpecrun H2 in its SQL Server mode underSQLServerDialect, including an entity on two connections.:grails-data-hibernate7-core:test4092 tests; the Oracle container spec fails to start under full-module load and passes alone, as it has on earlier runs.