jdaugherty opened a new issue, #16349:
URL: https://github.com/apache/grails-core/issues/16349
### Summary
On GORM for Hibernate 7 (8.1.x), a newly saved entity whose identifier is
generated **before** the insert (`id generator: 'increment'`, `'table'`, and
presumably any other non-`identity` strategy) is written with an `INSERT`
immediately followed by an `UPDATE` of the same row in the first flush. The row
therefore ends up at `version == 1` right after `save(flush: true)` instead of
`0`, and every plain save costs two DML statements instead of one. Entities
using the default `identity` generator are unaffected. GORM for Hibernate 5
does not have the problem for any generator.
### Reproduction
```groovy
@Entity
class Book {
Long id
Long version
String title
static mapping = { id generator: 'increment' }
}
```
```groovy
def statistics = sessionFactory.statistics
statistics.statisticsEnabled = true
statistics.clear()
def book = new Book(title: 'original').save(flush: true, failOnError: true)
assert book.version == 0 // fails: version is 1
assert statistics.entityInsertCount == 1 // passes
assert statistics.entityUpdateCount == 0 // fails: 1
```
Observed with `Statistics` on H2, Hibernate ORM 7.4.x, in a
`HibernateGormDatastoreSpec`-style test (`grails-data-hibernate7-core`),
against both the current `8.1.x` (`0980623`) and a feature branch on top of it:
| Entity | before flush | after flush | version |
|---|---|---|---|
| default `identity` generator | `hasChanged() == false`, insert already
executed | 1 insert, 0 updates | 0 |
| `id generator: 'increment'` | `hasChanged() == true`, `hasChanged('title')
== true`, insert pending | 1 insert, **1 update** | **1** |
| `id generator: 'increment'`, `trackChanges()` called manually before the
flush | | 1 insert, 0 updates | 0 |
| `tablePerConcreteClass true` hierarchies (which require a non-identity
generator) | as `increment` | 1 insert, 1 update | 1 |
The same happens with a bare `session.persist(book); session.flush()`, so it
is not specific to the GORM `save()` path.
### Cause
GORM's `DirtyCheckable` reports an instance whose change tracking has never
been activated (`$changedProperties == null`) as changed on **every** property:
both `hasChanged()` and `hasChanged(name)` return `true` until `trackChanges()`
has been called.
`GrailsEntityDirtinessStrategy` (the `CustomEntityDirtinessStrategy` GORM
registers with Hibernate) relies on tracking having been activated by the time
Hibernate dirty-checks an entity at flush. Where that activation happens
differs between the two modules:
- **Hibernate 5**
(`org.grails.orm.hibernate.support.ClosureEventTriggeringInterceptor`):
`onSaveOrUpdate` calls `activateDirtyChecking(entity)` when the entity is
saved, i.e. before any flush, regardless of the id strategy. It also calls it
in `onPostInsert`.
- **Hibernate 7**: Hibernate 6+ removed `SaveOrUpdateEvent`. The Hibernate 7
interceptor implements `onPersist`, but that override only publishes the GORM
persist event and delegates to the default listener; `activateDirtyChecking` is
now called **only** from `onPostInsert`.
With the `identity` generator, Hibernate executes the insert inside
`persist`, so `onPostInsert` fires immediately and the entity is tracked (and
clean) by the time of the flush. With a pre-insert generator, the
`EntityInsertAction` is queued and only executed during flush, **after**
`flushEntities` has already dirty-checked the entity. At that point the
instance is still untracked, `GrailsEntityDirtinessStrategy.findDirty` marks
every attribute dirty, and Hibernate schedules an `UPDATE` (bumping the
version) right behind the `INSERT`. The manual `trackChanges()` row in the
table above confirms this is the whole story.
### Impact
- `version` starts at 1 for every entity with a non-identity generator;
anything comparing against `0`, or relying on the version to count updates, is
off by one.
- Every save of such an entity issues an extra `UPDATE` that rewrites all
columns (and, for a `lastUpdated` property, stamps it at insert time).
- Table-per-concrete-class hierarchies are always affected, since Hibernate
rejects the identity generator for union subclasses.
- Tests written on Hibernate 5 with literal version assertions fail when
moved to Hibernate 7. The `tablePerConcreteClass` feature added to
`Hibernate7RefreshLockSpec` in #16344 had to assert versions relative to the
saved instance for this reason.
### Suggested fix
Activate dirty checking in the Hibernate 7 interceptor at persist time,
mirroring what the Hibernate 5 `onSaveOrUpdate` override does, e.g. call
`activateDirtyChecking(entity)` for an initialized entity in both `onPersist`
overloads before delegating (or, equivalently, from the persist event
publication). The existing `onPostInsert` call can stay for entities inserted
outside `persist`. A regression test should save an entity with `id generator:
'increment'` and assert `version == 0` and `entityUpdateCount == 0` after the
flush, alongside the existing identity-generator coverage; a
`tablePerConcreteClass true` pair covers the case users cannot avoid.
--
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]
