Repository navigation
Hibernate 7: keep the key of a unidirectional hasMany updatable unless it shares a join table - #16686
borinquenkid wants to merge 20 commits into
Conversation
…nless it shares a join table CollectionKeyColumnUpdater marked the key of every entity collection non-updatable when the owner had two or more unidirectional to-many properties, which was the workaround for issue 10811 (two hasMany to the same class share one join table, so each key column must be nullable). Hibernate writes no join table rows for a collection whose key is not updatable, so addTo on an owner such as `static hasMany = [roles: Role, tags: Tag]` was silently discarded on flush. The key is now non-updatable only when another unidirectional to-many property of the owner uses the same collection table. Collections with join tables of their own keep an updatable key. Fixes #16665 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16686 +/- ##
==================================================
- Coverage 62.1186% 62.1164% -0.0022%
+ Complexity 26780 26777 -3
==================================================
Files 2237 2237
Lines 110442 110433 -9
Branches 20152 20152
==================================================
- Hits 68605 68597 -8
Misses 32616 32616
+ Partials 9221 9220 -1 🚀 New features to boost your workflow:
|
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Thanks, the cause matches the reporter's analysis, and comparing collection tables is the right rule. It also fixes the reporter's second shape, where both hasMany point at the same class but name their own join tables. CI is green at 173e8a3, and the class is identical on 8.1.x, 9.0.x, 9.1.x and 10.0.x, so the merge-up should be clean.
One blocker, inline: the new filter calls getCollection() on every unidirectional to-many of the owner, and a to-many that is not bound as a collection now fails the session factory at startup. The other inline comment asks for a test of the explicit join-table case.
Should we fix this as well: the shared-join-table case still drops addTo* without an error, even with failOnError: true. As a local probe, I made the key always updatable. Both collections of hasMany = [roles: Role, denyRoles: Role] were then written to the shared table and read back correctly, and the hasmany package and the new specs still passed. So the #10811 rule may not be needed on Hibernate 7 at all, as the reporter suggested in #16665. That belongs in its own issue, because the shared table's unique constraint covers only the first key column, and rows of the second collection leave that column null, which some databases count as duplicates.
…hether a key is updatable CollectionKeyColumnUpdater asked every unidirectional to-many of the owner for its Hibernate collection. A to-many mapped with type: 'serializable' or with a user type is bound as a column by GrailsPropertyBinder and has no collection, and getCollection() throws a MappingException rather than returning null, so such an owner failed the session factory build. Filter the siblings with HibernateToManyProperty.isBoundAsCollection, which also stops a column-bound sibling from counting as a sharer of the join table. CollectionKeyColumnUpdaterSpec pins that a serializable and a user-type to-many have no Hibernate collection after binding and that getCollection() throws for them, and that the updater keeps an entity collection's key updatable next to such siblings. MultipleUnidirectionalHasManySpec adds the reporter's second shape, two hasMany to the same class with their own join tables, and an entity hasMany next to a serializable to-many.
…hether a key is updatable Port of 7138500 (#16686) to the Groovy conversion. CollectionKeyColumnUpdater asked every unidirectional to-many of the owner for its Hibernate collection; a to-many mapped with type: 'serializable' or with a user type is bound as a column and has none, and getCollection() throws a MappingException rather than returning null, so such an owner failed the session factory build. Filter the siblings with HibernateToManyProperty.isBoundAsCollection. CollectionKeyColumnUpdaterSpec and MultipleUnidirectionalHasManySpec carry the cases of #16686.
MuhAccount, MuhNote and CKCUColumnBoundOwner join the oracle; the former MuhItem group is regrouped under MuhAccount.
…ane only Since the #16686 fix the updater asks the property for the Hibernate collection the classic binder gave it, which a natively bound datastore does not have.
MuhAccount and MuhNote join the MuhItem group, now keyed MuhAccount.
|
Agreed on the shared-join-table case: the #10811 rule drops rows silently on Hibernate 7, and the native generator on the 9.x branches already treats it as a defect and keeps the key always updatable. Opened #16699 for 8.0.x with your probe and the unique-key caveat (the classic binder's unique key covers only the first collection's key column); the fix will land as a separate PR so this one stays the per-collection join table fix. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Both asks are addressed at 7138500: the siblings are filtered with isBoundAsCollection, so a serializable or user-type to-many is neither asked for its collection nor counted as a sharer, and the specs now cover the column-bound siblings and the same-class case with join tables of their own. The rule cannot be skewed by second-pass order, since a unidirectional entity collection gets its join table in the first pass and the only second-pass table reassignment is for foreign-key-bound collections, which are bidirectional and already excluded. The Groovy port on #16572 (4773d60) carries the same filter, and #16699 tracks the shared-join-table case. CI at this commit is green apart from the known flaky SpringApplicationRunnerSpec on Windows (#16030).
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving yet. The separate-table fix looks right: unrelated unidirectional collections no longer disable writes to each other's join tables, and the same-owner shared-table case still leaves the key non-updatable, which is what 16701 then has to fix. I did not find a functional regression in this delta.
Two gaps are why this is not an approval. The shared-table branch is only asserted through the package-private updater, and there is no user-facing note for the persistence change.
| @@ -103,6 +132,47 @@ class CollectionKeyColumnUpdaterSpec extends HibernateGormDatastoreSpec { | |||
| !key.isUpdateable() | |||
There was a problem hiding this comment.
This asserts !key.isUpdateable() on a key the test built itself and returned from a mocked CollectionKeyBinder. It never saves, flushes, or reloads, so it does not show that two unidirectional associations sharing a join table still behave as intended through GORM.
MultipleUnidirectionalHasManySpec covers separate tables. Please add a public GORM round-trip for the shared-table branch as well: two unidirectional hasMany associations on one join table, save, clear the session, reload, and assert the rows that this non-updatable key is supposed to preserve. The package-private flag check can stay, but it is not the regression.
There was a problem hiding this comment.
Added in 8b7515a, through the public API: MultipleUnidirectionalHasManySpec now has two unidirectional hasMany associations on one join table with a key column each, a row for each put in that table, the owner reloaded, renamed, given one more element and flushed, then reloaded again. It asserts that each association reads the row with its own key column, that the flush keeps both rows, which is what the non-updatable key is for, and that the added element is not written, labelled as issue 10811 and left as it is here (#16701 changes that assertion when it lands). The package-private flag check stays.
| .filter(HibernateToManyProperty::isBoundAsCollection) | ||
| .map(HibernateToManyProperty.class::cast) | ||
| .filter(p -> !p.isBidirectional()) | ||
| .filter(p -> Objects.equals(p.getCollection().getCollectionTable(), collectionTable)) |
There was a problem hiding this comment.
This changes user-visible persistence. An owner with two unidirectional hasMany associations now writes the join table when each association has its own table, and still does not when they share one. That distinction is not in grails-doc or the GORM upgrade notes.
Please add a short public note: separate join tables are written; a shared join table is still the 10811 case and is not fixed by this change alone. A bugfix that changes which rows get written is a user-facing change and needs that coverage.
There was a problem hiding this comment.
Added in 8b7515a: section 26.11 of the Grails 8 upgrade notes says an owner with several unidirectional hasMany associations writes each join table of its own, as under Hibernate 5, and that two associations on one shared join table are read but not written, which is issue 10811 and not fixed by this change; the one-to-many chapter of the GORM for Hibernate 7 guide carries the same paragraph.
…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.
…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.
…ent which join tables are written From the review of #16686. MultipleUnidirectionalHasManySpec gains the shared-table case through the public API: two unidirectional hasMany associations on one join table with a key column each, a row for each put in that table, the owner reloaded, renamed, given one more element and flushed, then reloaded again. Each association reads the row with its own key column, the flush keeps both rows, which is what the non-updatable key is for, and the added element is not written, which is issue 10811 and is left as it is here. The Grails 8 upgrade notes and the one-to-many chapter of the GORM for Hibernate 7 guide say which join tables an owner with several unidirectional hasMany associations writes: each table of its own, as under Hibernate 5, while a shared table is read but not written.
…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.
…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.
… 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.
…ta-hibernate7 conversion
…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
The criteria builder spec asserts the SQL still groups by 'total' as written. Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p
Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
Hibernate 7: one scanner for the query package, and keep a spaced sqlProjection alias (approved by jamesfredley on 6333d95; a928abb adds only the merge of fix/h7-multiple-hasmany-8.0.x). Claude-Session: https://claude.ai/code/session_01MDHn7fC3hsGJ27vFHD2o1p
🚨 All tests passed but jobs failed 🚨Failed Jobs without Test Failures❌ Build Health / Forge Projects 🏷️ Commit: d3d2279 Rerun ControlsClick the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
|
@jdaugherty, what changed since your approval on
#16701, the shared join table fix, sits on top of this and is still under review. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Re-reviewed at d3d2279. The two asks since my approval are addressed in substance: the shared-table branch now has a GORM round trip, and the upgrade note and the one-to-many chapter say which join tables are written. I ran MultipleUnidirectionalHasManySpec and CollectionKeyColumnUpdaterSpec locally at this head: 12 tests, all pass.
Three follow-ups inline, none on the fix itself. The shared-table fixture is the mirror of the #10811 shape (one shared key column, an element column each) and its labels describe the other one, and both doc notes should say that two hasMany to the same class share one join table by default, since that reader is the one the unfixed case concerns.
Not this PR: the two red Forge jobs are repo.grails.org answering 429 for io.spring.nohttp:nohttp-gradle in Forge's buildSrc, so a rerun clears them. The diff now carries #16735 through its merge; it was reviewed there and nothing in it changed after its approval, so I did not re-review those files. The PR description should mention that it includes #16735, so the merge commit's reader does not look for the SQL scanner in a hasMany fix.
| static hasMany = [roles: MuhRole, denyRoles: MuhRole] | ||
| static mapping = { | ||
| roles joinTable: [name: 'muh_shared_account_roles', key: 'account_id', column: 'role_id'] | ||
| denyRoles joinTable: [name: 'muh_shared_account_roles', key: 'account_id', column: 'deny_role_id'] |
There was a problem hiding this comment.
This fixture is the mirror of the #10811 shape. #10811, and CKCUSharedTableOwner in the updater spec, are two hasMany to the same class with no mapping: they share the default join table, which has one element column and a key column per association. On this branch static hasMany = [roles: Role, denyRoles: Role] creates item_role with role_id, item_roles_id and item_deny_roles_id, all nullable. Here both associations share the key column account_id and have an element column each, so the labels do not match what runs: line 105 says each association reads the row with its own key column, but they read it with their own element column, and the upgrade note's "with a key column each" describes the other shape.
The non-updatable key exists for the default shape, where a row of the second collection leaves the first key column null. Could the fixture drop the joinTable mapping so the round trip is #10811 verbatim? The table is then muh_shared_account_muh_role with muh_role_id, muh_shared_account_roles_id and muh_shared_account_deny_roles_id, the two inserted rows become (allow, account, null) and (deny, null, account), and the expect: label is true as written. It is also the shape #16701 changes, so its assertion flip lands on the same fixture.
One more label, line 116: the non-updatable key "stops one association from deleting or nulling the other's" rows, but this flush adds an element to a loaded collection without replacing it, so no delete runs; with an updatable key the result would be a third row, not a lost one. "the flush writes nothing to the shared table" is what the assertion shows.
|
|
||
| Under Hibernate 7, an owner with two or more unidirectional `hasMany` associations wrote no rows to their join tables: an element added to any of them was lost at flush (issue https://github.com/apache/grails-core/issues/16665[#16665]). This is fixed when each association has a join table of its own, which is the default (`item_roles` and `item_tags` for `static hasMany = [roles: Role, tags: Tag]`) and also the case when two associations to the same class map different tables with `joinTable`. Each such join table is written, as under Hibernate 5. | ||
|
|
||
| Two unidirectional associations mapped onto one shared join table, with a key column each, are read but their new elements are still not written. That is issue https://github.com/apache/grails-core/issues/10811[#10811] and is not fixed by this change; map a table per association until it is. |
There was a problem hiding this comment.
A reader with static hasMany = [roles: Role, denyRoles: Role] and no mapping is in this case and cannot tell from the note: two associations to the same class share one join table by default, so for them "the default" in the previous sentence is the unfixed shape. Could the note say when the sharing happens? Something like:
Two unidirectional associations to the same class share one join table by default, named after the owner and the element class, with a key column per association (
item_rolewithitem_roles_idanditem_deny_roles_idforstatic hasMany = [roles: Role, denyRoles: Role]). Such a table is read, but new elements are not written. That is issue #10811 and is not fixed by this change; name ajoinTableper association to give each its own table.
Those are the table and columns this branch creates for that mapping on H2, all nullable.
|
|
||
| The join table name defaults to `author_books` and can be customised — see xref:ormdsl-tableAndColumnNames[Table and Column Names]. | ||
|
|
||
| An owner may declare several unidirectional one-to-many associations, such as `static hasMany = [roles: Role, tags: Tag]`. Each one is written to its own join table, including two associations to the same class that map different tables with `joinTable`. Two associations mapped onto one shared join table, with a key column each, are read but their new elements are not written (issue https://github.com/apache/grails-core/issues/10811[#10811]); give each association a table of its own. |
There was a problem hiding this comment.
Same point as on the upgrade note: two associations to the same class share one join table by default (item_role with item_roles_id and item_deny_roles_id for static hasMany = [roles: Role, denyRoles: Role]), which is the shape the last sentence describes. Saying so here tells the reader with that mapping that "give each association a table of its own" applies to them, and that joinTable is how.
Fixes #16665.
Problem
On Hibernate 7, an owner with two or more unidirectional
hasManyassociations silently losesaddTo*on flush:With one
hasManyit works, and it works on Hibernate 5.Cause
CollectionKeyColumnUpdatermarked the key of an entity collection non-updatable whenever the owner had two or more unidirectional to-many properties. That is the workaround for #10811, where twohasManyto the same class share one join table and each key column must stay nullable. Hibernate writes no join table rows for a collection whose key is not updatable, so the rule also discarded the rows of collections that have a join table of their own.Change
The key is non-updatable only when another unidirectional to-many property of the owner uses the same collection table, which is the #10811 case. Collections with their own join tables keep an updatable key.
Tests
MultipleUnidirectionalHasManySpecreproduces the report: an element added to one of twohasManyto different classes is found after the session is cleared, and so are elements added to both.CollectionKeyColumnUpdaterSpec: the case that expected a non-updatable key for twohasManyto different classes now expects an updatable one, and a new case covers twohasManyto the same class sharing a join table, which stays non-updatable.grails-data-hibernate7-coretest run passes exceptSqlRestrictionHibernate7Specagainst Oracle, whose container cannot start locally.Branches
The class is identical on 8.1.x, 9.0.x, 9.1.x and 10.0.x, so this should merge forward through them. The Groovy conversion of the Hibernate 7 modules (#16572) carries a hand-ported version of the same change.
🤖 Generated with Claude Code