jdaugherty commented on PR #16344: URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5759754725
> *This is an AI-assisted review: the analysis and the comments below were produced with Claude and checked by me before posting.* > > Follow-up pass over `e635ada7`. Both of my earlier findings are properly fixed — and probing the second one clearly turned up a deeper bug than I reported (the Hibernate 5 cascade running over the pre-reload graph, so the post-refresh walk bailed out at `Hibernate.isInitialized` and never reset the cascaded children). `CollectionType.getElementsIterator` is a better answer than the `isArray()` guard I suggested. My non-blocking multi-tenant note is addressed by `lockedLoad`, and two things I had considered and let go — a null `EntityEntry` in `recordLockMode`, and follow-on locking silently dropping the lock clause — are now handled explicitly with `Locking.FollowOn.DISALLOW` and `locksThroughTheLoader`. Nice. > > One of the fixes reported as landed is **missing from the branch** — see the two `HibernateGormStaticApi` comments. Please double-check nothing else was lost in the same merge or force-push. > > ### On scope > > The PR has grown well past its title. Alongside the locking work it now: > > - re-enables the Hibernate 7 `merge`, `create` and `create-onflush` GORM event listeners that were previously commented out in `HibernateConnectionSourceSettings`, and adds `ObservingEventListener` / `PersistOnFlushEventListener` plus group-retention logic to `EventListenerIntegrator`; > - fixes the dirty-checking bug in issue #16349; > - fixes an implicit sequence-name resolution failure in `GrailsSequenceStyleGenerator`; > - reroutes every instance operation in `GormStaticApi` through `qualifier`. > > Each is plausible on its own, but firing persist and merge events where none fired before is a behavioural change for **every** Hibernate 7 application, and it is not what a reviewer of "enhance supported lock modes" is looking for. The sequence-generator fix in particular looks unrelated to everything else here. I would rather see these land as their own PRs against their own issues, where they get the review attention they deserve — per the contributor checklist's single-focused-change item. > > I am not re-litigating the `mutex(Closure)` semantics change; that is a deliberate, documented decision you have already defended. @sbglasius we have never released Hibernate 7 so this was a bug fix to match the Hibernate 5 behaviors. -- 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]
