jdaugherty commented on code in PR #16782: URL: https://github.com/apache/grails-core/pull/16782#discussion_r4393000038
########## grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md: ########## @@ -433,6 +433,12 @@ GORM behavior changes: - Nested transaction propagation was corrected in [Grails commit bd268672976](https://github.com/apache/grails-core/commit/bd268672976fff60853021dd659cd6d55482e631): `REQUIRED` joins an active transaction, while `REQUIRES_NEW` suspends it and uses a separate session, then restores the outer session. This also affects SimpleMap-backed domain/service unit tests. A thread-bound test session does not mean every service invocation shares one transaction or first-level cache; check the service annotations and explicit `withNewTransaction` calls before attributing changed fixture visibility to a regression. - Verify `ident()` and reload/proxy behavior for composite keys and renamed single-property identifiers. In the checked pre-release build, `ident()` read only the ordinary `id` property, returning null for a persisted composite-key domain and for a key mapped with `id name: 'code'`; Grails 7 used the mapped property or constructed a serializable identifier from the composite key fields. Consequently `load(entity.ident())` returned null before any proxy was created. Regression tests reproduced both cases on Hibernate 5 and Hibernate 7. Treat this as a framework regression to isolate and fix, rather than excluding composite-key domains from coverage or interpreting the null as a proxy-initialization change. - Initialization does not change a Hibernate base-class proxy into its concrete subtype. Before subclass/trait-only access, unwrap through the supported Hibernate API and retain the resulting instance; a cast alone is not unwrapping. A collection `find` still returns the original element even if its predicate unwrapped it; use `findResult` when the intended result is the unwrapped matching object. Equivalent warmed-call-site failures were reproduced on the old stack too, so distinguish a pre-existing proxy assumption from a new compiler regression. +- Unidirectional `hasMany` join-table foreign-key column names under Hibernate 7 are derived from the physical table name of the associated domain class (e.g. `catalog_book_id` instead of `book_id`). Configure `joinTable: [name: '...', key: '...', column: '...']` explicitly to preserve an existing Grails 7 schema. +- Multiple unidirectional `hasMany` associations to the same class sharing a join table use unique keys per association over their key column and the element (or a composite unique key over both key columns and element on Oracle and Db2 for LUW); list/map/bag associations retain index-based join tables. +- Removing an element from a unidirectional `hasMany` set or bag join table uses GORM's collection persister to delete only the specified row by owner key and element, matching Hibernate 5 behavior. Review Comment: The caveat that makes this actionable is the override: an application that sets `hibernate.persister.resolver` keeps its own resolver, and with it the Hibernate 7 behaviour, so #16691 (one `removeFrom` deleting every row of the owner) comes back. Worth a sentence so an upgrade pass looks for that property before concluding the fix is in place. ########## grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md: ########## @@ -433,6 +433,12 @@ GORM behavior changes: - Nested transaction propagation was corrected in [Grails commit bd268672976](https://github.com/apache/grails-core/commit/bd268672976fff60853021dd659cd6d55482e631): `REQUIRED` joins an active transaction, while `REQUIRES_NEW` suspends it and uses a separate session, then restores the outer session. This also affects SimpleMap-backed domain/service unit tests. A thread-bound test session does not mean every service invocation shares one transaction or first-level cache; check the service annotations and explicit `withNewTransaction` calls before attributing changed fixture visibility to a regression. - Verify `ident()` and reload/proxy behavior for composite keys and renamed single-property identifiers. In the checked pre-release build, `ident()` read only the ordinary `id` property, returning null for a persisted composite-key domain and for a key mapped with `id name: 'code'`; Grails 7 used the mapped property or constructed a serializable identifier from the composite key fields. Consequently `load(entity.ident())` returned null before any proxy was created. Regression tests reproduced both cases on Hibernate 5 and Hibernate 7. Treat this as a framework regression to isolate and fix, rather than excluding composite-key domains from coverage or interpreting the null as a proxy-initialization change. - Initialization does not change a Hibernate base-class proxy into its concrete subtype. Before subclass/trait-only access, unwrap through the supported Hibernate API and retain the resulting instance; a cast alone is not unwrapping. A collection `find` still returns the original element even if its predicate unwrapped it; use `findResult` when the intended result is the unwrapped matching object. Equivalent warmed-call-site failures were reproduced on the old stack too, so distinguish a pre-existing proxy assumption from a new compiler regression. +- Unidirectional `hasMany` join-table foreign-key column names under Hibernate 7 are derived from the physical table name of the associated domain class (e.g. `catalog_book_id` instead of `book_id`). Configure `joinTable: [name: '...', key: '...', column: '...']` explicitly to preserve an existing Grails 7 schema. +- Multiple unidirectional `hasMany` associations to the same class sharing a join table use unique keys per association over their key column and the element (or a composite unique key over both key columns and element on Oracle and Db2 for LUW); list/map/bag associations retain index-based join tables. Review Comment: A bag has no index. The guide says "its join table is keyed by the index (for lists and maps)", and the binder treats a bag as having no key at all; "list/map/bag associations retain index-based join tables" is wrong for the bag. The part of §26.12 an upgrade pass has to act on is also missing: a schema created by an earlier Grails 8 release has only the single unique key Hibernate made for the first association; `dbCreate: 'update'` adds the second one, and on Oracle and Db2 for Linux, UNIX and Windows a migration has to replace that key with the wider one. Something like: ```suggestion - Two unidirectional `hasMany` associations to the same class share one join table by default (a key column each, one element column), and the table is now written. A set gets a unique key per association over its key column and the element; on Oracle and Db2 for Linux, UNIX and Windows it is one unique key over both key columns and the element. A list or a map stays keyed by its index and a bag has no key; their elements may repeat. A schema from an earlier Grails 8 release has only the first association's unique key: `dbCreate: 'update'` adds the second, and on Oracle and Db2 add a migration that replaces it with the wider key rather than expecting `update` to do it. ``` ########## grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md: ########## @@ -433,6 +433,12 @@ GORM behavior changes: - Nested transaction propagation was corrected in [Grails commit bd268672976](https://github.com/apache/grails-core/commit/bd268672976fff60853021dd659cd6d55482e631): `REQUIRED` joins an active transaction, while `REQUIRES_NEW` suspends it and uses a separate session, then restores the outer session. This also affects SimpleMap-backed domain/service unit tests. A thread-bound test session does not mean every service invocation shares one transaction or first-level cache; check the service annotations and explicit `withNewTransaction` calls before attributing changed fixture visibility to a regression. - Verify `ident()` and reload/proxy behavior for composite keys and renamed single-property identifiers. In the checked pre-release build, `ident()` read only the ordinary `id` property, returning null for a persisted composite-key domain and for a key mapped with `id name: 'code'`; Grails 7 used the mapped property or constructed a serializable identifier from the composite key fields. Consequently `load(entity.ident())` returned null before any proxy was created. Regression tests reproduced both cases on Hibernate 5 and Hibernate 7. Treat this as a framework regression to isolate and fix, rather than excluding composite-key domains from coverage or interpreting the null as a proxy-initialization change. - Initialization does not change a Hibernate base-class proxy into its concrete subtype. Before subclass/trait-only access, unwrap through the supported Hibernate API and retain the resulting instance; a cast alone is not unwrapping. A collection `find` still returns the original element even if its predicate unwrapped it; use `findResult` when the intended result is the unwrapped matching object. Equivalent warmed-call-site failures were reproduced on the old stack too, so distinguish a pre-existing proxy assumption from a new compiler regression. +- Unidirectional `hasMany` join-table foreign-key column names under Hibernate 7 are derived from the physical table name of the associated domain class (e.g. `catalog_book_id` instead of `book_id`). Configure `joinTable: [name: '...', key: '...', column: '...']` explicitly to preserve an existing Grails 7 schema. +- Multiple unidirectional `hasMany` associations to the same class sharing a join table use unique keys per association over their key column and the element (or a composite unique key over both key columns and element on Oracle and Db2 for LUW); list/map/bag associations retain index-based join tables. +- Removing an element from a unidirectional `hasMany` set or bag join table uses GORM's collection persister to delete only the specified row by owner key and element, matching Hibernate 5 behavior. +- `is empty` on associations sharing a key column configures `RestrictedCollectionSqmTranslatorFactory` (translating to a count query) under `hibernate.query.sqm.translator` by default. +- Collection criteria such as `isNotEmpty`, `isEmpty`, or `sizeEq` resolve properly to the parent collection property even when placed adjacent to an association block of the same name. Review Comment: Matches §26.13. Non-blocking: as guidance, the useful half is the negative. An application that reordered its criteria, or replaced `isEmpty('pets')` with a size projection, to get past the `ClassCastException` of an earlier 8.0 build can keep its original criteria; the workaround is not needed on the fixed release. ########## grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md: ########## @@ -433,6 +433,12 @@ GORM behavior changes: - Nested transaction propagation was corrected in [Grails commit bd268672976](https://github.com/apache/grails-core/commit/bd268672976fff60853021dd659cd6d55482e631): `REQUIRED` joins an active transaction, while `REQUIRES_NEW` suspends it and uses a separate session, then restores the outer session. This also affects SimpleMap-backed domain/service unit tests. A thread-bound test session does not mean every service invocation shares one transaction or first-level cache; check the service annotations and explicit `withNewTransaction` calls before attributing changed fixture visibility to a regression. - Verify `ident()` and reload/proxy behavior for composite keys and renamed single-property identifiers. In the checked pre-release build, `ident()` read only the ordinary `id` property, returning null for a persisted composite-key domain and for a key mapped with `id name: 'code'`; Grails 7 used the mapped property or constructed a serializable identifier from the composite key fields. Consequently `load(entity.ident())` returned null before any proxy was created. Regression tests reproduced both cases on Hibernate 5 and Hibernate 7. Treat this as a framework regression to isolate and fix, rather than excluding composite-key domains from coverage or interpreting the null as a proxy-initialization change. - Initialization does not change a Hibernate base-class proxy into its concrete subtype. Before subclass/trait-only access, unwrap through the supported Hibernate API and retain the resulting instance; a cast alone is not unwrapping. A collection `find` still returns the original element even if its predicate unwrapped it; use `findResult` when the intended result is the unwrapped matching object. Equivalent warmed-call-site failures were reproduced on the old stack too, so distinguish a pre-existing proxy assumption from a new compiler regression. +- Unidirectional `hasMany` join-table foreign-key column names under Hibernate 7 are derived from the physical table name of the associated domain class (e.g. `catalog_book_id` instead of `book_id`). Configure `joinTable: [name: '...', key: '...', column: '...']` explicitly to preserve an existing Grails 7 schema. +- Multiple unidirectional `hasMany` associations to the same class sharing a join table use unique keys per association over their key column and the element (or a composite unique key over both key columns and element on Oracle and Db2 for LUW); list/map/bag associations retain index-based join tables. +- Removing an element from a unidirectional `hasMany` set or bag join table uses GORM's collection persister to delete only the specified row by owner key and element, matching Hibernate 5 behavior. +- `is empty` on associations sharing a key column configures `RestrictedCollectionSqmTranslatorFactory` (translating to a count query) under `hibernate.query.sqm.translator` by default. Review Comment: `is empty` does not configure anything. GORM sets `hibernate.query.sqm.translator` to `RestrictedCollectionSqmTranslatorFactory` by default, and that translator renders `is empty` / `is not empty` on an association that shares a join table and a key column as a size test (a count subquery), so the test does not see the other association's rows (#16748). The sentence #16775 added to the guide, which this bullet is meant to mirror, is the override: an application that configures `hibernate.query.sqm.translator` itself keeps its translator and loses the fix. That is the thing an upgrade pass should flag. Something like: ```suggestion - GORM sets `hibernate.query.sqm.translator` to `RestrictedCollectionSqmTranslatorFactory` by default, which translates `is empty` / `is not empty` on an association that shares a join table and a key column as a size test, so the test does not see the other association's rows (#16748). An application that configures `hibernate.query.sqm.translator` itself keeps its translator and the Hibernate 7 behaviour; flag that setting, and `hibernate.persister.resolver` above, rather than treating a wrong `is empty` result as a query bug. ``` ########## grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md: ########## @@ -433,6 +433,12 @@ GORM behavior changes: - Nested transaction propagation was corrected in [Grails commit bd268672976](https://github.com/apache/grails-core/commit/bd268672976fff60853021dd659cd6d55482e631): `REQUIRED` joins an active transaction, while `REQUIRES_NEW` suspends it and uses a separate session, then restores the outer session. This also affects SimpleMap-backed domain/service unit tests. A thread-bound test session does not mean every service invocation shares one transaction or first-level cache; check the service annotations and explicit `withNewTransaction` calls before attributing changed fixture visibility to a regression. - Verify `ident()` and reload/proxy behavior for composite keys and renamed single-property identifiers. In the checked pre-release build, `ident()` read only the ordinary `id` property, returning null for a persisted composite-key domain and for a key mapped with `id name: 'code'`; Grails 7 used the mapped property or constructed a serializable identifier from the composite key fields. Consequently `load(entity.ident())` returned null before any proxy was created. Regression tests reproduced both cases on Hibernate 5 and Hibernate 7. Treat this as a framework regression to isolate and fix, rather than excluding composite-key domains from coverage or interpreting the null as a proxy-initialization change. - Initialization does not change a Hibernate base-class proxy into its concrete subtype. Before subclass/trait-only access, unwrap through the supported Hibernate API and retain the resulting instance; a cast alone is not unwrapping. A collection `find` still returns the original element even if its predicate unwrapped it; use `findResult` when the intended result is the unwrapped matching object. Equivalent warmed-call-site failures were reproduced on the old stack too, so distinguish a pre-existing proxy assumption from a new compiler regression. +- Unidirectional `hasMany` join-table foreign-key column names under Hibernate 7 are derived from the physical table name of the associated domain class (e.g. `catalog_book_id` instead of `book_id`). Configure `joinTable: [name: '...', key: '...', column: '...']` explicitly to preserve an existing Grails 7 schema. +- Multiple unidirectional `hasMany` associations to the same class sharing a join table use unique keys per association over their key column and the element (or a composite unique key over both key columns and element on Oracle and Db2 for LUW); list/map/bag associations retain index-based join tables. +- Removing an element from a unidirectional `hasMany` set or bag join table uses GORM's collection persister to delete only the specified row by owner key and element, matching Hibernate 5 behavior. +- `is empty` on associations sharing a key column configures `RestrictedCollectionSqmTranslatorFactory` (translating to a count query) under `hibernate.query.sqm.translator` by default. +- Collection criteria such as `isNotEmpty`, `isEmpty`, or `sizeEq` resolve properly to the parent collection property even when placed adjacent to an association block of the same name. +- Custom `UserType` implementations must be rewritten for Hibernate 7 API changes; types spanning multiple columns must implement `CompositeUserType` and map attributes in alphabetical column order. Review Comment: "map attributes in alphabetical column order" inverts the rule. Per §26.11, the columns of the `type` mapping are assigned to the embeddable's attributes in the alphabetical order of the *attribute* names, which is also the index that `getPropertyValue()` and `instantiate()` use; Hibernate 5 followed `getPropertyNames()` instead. The column list in the mapping is not reordered; what has to be checked is which attribute each listed column lands on. Something like: ```suggestion - Custom `UserType` implementations must be rewritten for the Hibernate 7 contract (generic in the Java type, `getSqlType()`, position-based `nullSafeGet()` / `nullSafeSet()`). A type that spans several columns implements `CompositeUserType`; the columns of its `type` mapping are assigned to the embeddable's attributes in the alphabetical order of the attribute names, which is also the index `getPropertyValue()` and `instantiate()` use, where Hibernate 5 followed `getPropertyNames()`. Check the column-to-attribute assignment of every multi-column type when porting it. ``` This is a Hibernate API change rather than a GORM behaviour change; it fits the "Also check for:" list above, next to the removed annotations, better than this list. -- 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]
