borinquenkid commented on PR #16066: URL: https://github.com/apache/grails-core/pull/16066#issuecomment-5207516411
Went through every comment individually (replies below on each thread) and also diffed this branch against your own AI-assisted pass at the same review (`fix/gorm-registry-review-feedback`, `4d2b832628`) for every case where the two of us landed on different fixes, so I wasn't re-deriving something you'd already solved or contradicting your own diff without checking it first. Full reasoning trail (including the cases where I kept my approach over yours, with the evidence) is written up as I went; happy to share that working doc if useful. **Blocker 1 (no documentation)** — resolved. New `grails-doc/.../GORM/multiTenancy.adoc` covering `Tenants` and `CurrentTenantHolder` end to end, plus a new upgrade-note section in `upgrading80x.adoc`. I didn't adapt your upgrade-notes draft wholesale — ran a diff of `GormEnhancer`'s actual current surface against real `origin/8.0.x` first, since a few of the removals your draft describes (`enhance()`, `dynamicEnhance`, the constructors, `multiTenancyMode`) are restored on this branch and would be actively wrong to document as gone. **Blocker 2 (unrelated fixes bundled in)** — not split out. Traced each of the four (`getAll()` ordering/null-slot semantics, the `HibernateQuery` junction overrides, `CriteriaBuilder`'s guards, `removeConstraints()`) against git blame and the shared TCK before deciding, rather than against the PR description's framing. Detail is on the `HibernateSession.java` thread below, but short version: three of the four are demonstrated pre-existing 8.0.x contract violations this refactor surfaced and fixed (with existing, unchanged TCK coverage proving the contract), and the fourth (`removeConstraints`) is the direct undo-counterpart of the double-constraint-registration bug you flagged separately. Genuinely open to splitting any one of them out if the trace doesn't change your mind. **Blocker 3 (TCK exclusion instead of a fix)** — fixed for real. Root cause was the shared `GormStaticApi.first(Map)`/`last(Map)` losing an H7-specific override at merge-base (not H7-specific after all — H5 has the same gap). Fixed generically, exclusion reverted, composite-key H7 case passes 12/12 again. **The CI-blind-spot note** — `TenantDelegatingGormOperations`'s ~100 class-based `Tenants.withId` call sites are fixed (detail on the `Tenants.groovy:229` thread) — though it turns out that class is dead code on this branch, nothing currently constructs it outside its own test. Confirmed `grails-datamapping-rx` has zero diff in this PR and uses its own fully independent `Tenants` class, so there's no actual compilation gap there. On `GormApiAllocationSpec` driving internal selectors: real, still-open gap on the DATABASE-mode qualified-routing case specifically — documented as a recommended follow-up rather than closing it with an unreviewed new spec. Two things worth a look on your own branch, found while cross-checking (not blockers, just flagging): the `getMultiTenancyMode()` rationale (on the `MongoStaticApi.groovy` thread) doesn't hold against your own `GormRegistry.createStaticApi`; and the static-side `methodMissing` clobber guard (on the `GormEnhancer.groovy` thread) is unreachable dead code on both our branches — confirmed with a few isolated `ExpandoMetaClass` probes, happy to share them. Separately: just merged current `8.0.x` into this branch (191 commits since the fork point) to keep it current before this goes further — one doc-section-numbering conflict, resolved; full suite green after. -- 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]
