jdaugherty commented on PR #16344: URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5732242897
Worked through the remaining review findings in `5ba25f4c84`. Two were real locking bugs, both confirmed with throwaway probes before and after the fix. **Hibernate 5 took no lock on a joined hierarchy on PostgreSQL-class dialects.** A refresh under a pessimistic mode loads through the entity loader, and `AbstractEntityPersister` substitutes a plain read loader when the entity spans several tables, has subclasses, and the dialect reports no outer-join `FOR UPDATE`, which is every PostgreSQL dialect plus DB2 and CockroachDB. No statement carried a lock clause, yet `PESSIMISTIC_WRITE` was still recorded, so nothing looked wrong. Such a refresh now reloads first and takes a version-checked lock of its own, which reports a writer that slipped in rather than ignoring it. `Hibernate5JoinedLockDialectSpec` runs a dialect that reports no outer-join locking and checks a competing JDBC `FOR UPDATE` on the root row is refused; it fails without the fix. **The #16349 fix missed the merge path.** An entity saved with its identifier already set (`id generator: 'assigned'`) goes through `session.merge`, and Hibernate's merge saves a fresh copy without firing a persist event, so tracking never started and the row was inserted then updated, landing at version 1. Tracking is now activated on the managed copy the merge produces, and `Issue16349Spec` covers it. Also in this commit: - Replacing a listener group keeps any listener GORM contributed, so an application listener extending `DefaultMergeEventListener` or `DefaultPersistEventListener` no longer silently takes GORM's persistence events and its dirty-check activation with it. - The Hibernate 7 scalar lock query sets `Locking.FollowOn.DISALLOW`, so a statement the dialect cannot lock fails rather than returning an unlocked row. - Both static lock forms load a multi-tenant entity through a query, since Hibernate's tenant filter does not apply to a load by identifier. Verified by a new feature in each `HibernateDatastoreMultiTenancySpec`: before the fix, tenant2 could lock tenant1's row by identifier. - `lock(id, refresh: true)` returns `null` for an entity deleted in this session instead of throwing "must be attached", matching `lock(id)`. - The core form rejects a datastore that cannot refresh under a lock before reading anything, `refresh(D)` routes through the connection qualifier like `refresh(D, Map)`, and `refresh:` is parsed as strictly as `lock:` so `refresh: 'yes'` no longer silently means no refresh. - The Hibernate 5 dirty reset walks through a non-dirty-checkable entity and picks up elements queued onto an uninitialized collection. **Documentation.** A new Upgrade Notes entry for 8.0 covers the `version` change for pre-insert identifier generators and the new persist and merge events, both observable in existing Hibernate 7 applications. Corrected the claim that Hibernate 5 publishes `SaveOrUpdateEvent` "at the same points": it publishes nothing for a merge. Also documented the transaction requirement of every `lock(id, ...)` form, that an optimistic mode requested under a stronger held lock is superseded, the Hibernate 5 `LockMode.FORCE` alias and its `NOWAIT` behaviour for `PESSIMISTIC_FORCE_INCREMENT`, and narrowed the `tablePerConcreteClass` note to what the tests actually cover. Each spec gained the feature the other already had, so the embedded-cascade and statement-count cases now run on both versions. **Runs** (`DO_NOT_CACHE_TESTS=1`, `--no-build-cache`): | Run | Result | |---|---| | `grails-datamapping-core:test` (full) | 1019 tests, 0 failures | | `grails-data-hibernate5-core:test` (full, incl. `Hibernate5RefreshLockSpec` 91/91) | 927 tests, 0 failures, 33 skipped | | `grails-data-hibernate7-core:test` (full, incl. `Hibernate7RefreshLockSpec` 93/93) | 3182 tests, 0 failures, 24 skipped | | `codeStyle` on the three modules | clean | -- 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]
