borinquenkid commented on PR #15678: URL: https://github.com/apache/grails-core/pull/15678#issuecomment-4811711172
Thanks for the detailed read — happy to connect the dots. The single root cause is commit 8f1500dd03, which replaced GormEnhancer's per-entity static maps (one (entityClass, qualifier) → GormApi entry per entity × per connection — O(M×N) memory) with a single process-wide GormRegistry that resolves APIs on demand using a selector chain. Every other change in this PR is a downstream ripple from that one architectural swap. The two threads you named map to the two hardest ripples: --- Thread 1 — Multi-tenancy resolution regressions The old static maps gave each entity a direct pointer to its API for a given connection at enhancement time. The registry replaces that with a runtime selector chain (ActiveSession → Preferred → Qualified → Default). Several call sites assumed the old direct-lookup contract and started resolving to the wrong (usually default) datastore for non-default connections or tenants: - Routing fixes (DatastoreResolver, GormApiResolver, ActiveSessionDatastoreSelector, PreferredDatastoreSelector): correct the selector chain for non-default connection/tenant resolution paths that the static maps used to short-circuit. - Transaction changes (GormStaticApi.withTransaction(Map), GrailsHibernateTransactionManager): the withTransaction(Map) overload called definition.setProperty(k, v) — no such method on DefaultTransactionDefinition (a Java bean), which was silently tolerated before but now hits a hot path under registry routing. Also fixed GrailsHibernateTransactionManager to see child-datastore sessions when participating in a parent transaction. - Mongo/Neo4j/Simple edits: each datastore previously managed its own static enhancement maps. This PR registers each with GormRegistry (registerDatastore() / registerApiFactory()) and adds the getDatastoreForTenantId fallback to MongoDatastore (the multi-tenancy tenant resolver was calling into the registry before the child datastore was registered). SimpleMapSession had a latent rollbackOnly bug (a rolled-back tx set a session-level flag that was never cleared) that the registry's new session-per-call paths surfaced. - Test isolation changes (DataTestSetupInterceptor / DataTestCleanupInterceptor): the harness only bound a session for the default datastore. A non-default-connection entity now resolves to a dedicated child datastore — without a bound session, save() opened a throwaway per-call session that was discarded on return. The fix binds and unbinds a session per connection source (no-op for single-datasource specs). --- Thread 2 — Child datastore initialization order ChildHibernateDatastore.initialize() returned null in the 8.0.x-hibernate7 base — the child was intentionally not enhanced. Under the registry, each child must register its own APIs (so multi-datasource entities resolve to the right child). The new initialize() calls new HibernateGormEnhancer(this, txManager, settings, datastoresByConnectionSource). The critical ordering constraint: datastoresByConnectionSource in the parent must be populated before the child is constructed (the parent builds its connection map during its own initialize(), then calls new ChildHibernateDatastore(parent, ...)). The bindParent() / PARENT_HOLDER thread-local in ChildHibernateDatastore handles the case where getPrimaryDatastore() is called during the super-constructor chain before this.parent is assigned. - TCK rewrites (GrailsDataTckManager): the manager now calls setTargetDatastore(childDatastore) after the parent initializes, so the TCK spec's manager.session and service lookups go to the right child. The domain-class list was changed from Set to List because entity registration order now matters for the registry's MappingContext traversal. --- Two items that need explicit acknowledgment: - Debug specs (DebugGetSpec, DebugGeoJSONDecodeSpec, etc. in grails-data-mongodb): these were written to isolate get() failures during root-cause investigation. Their underlying test logic is sound, but the Debug prefix and println calls shouldn't be merged. I'll rename them to production spec names and remove the debug output before this lands. - Deleted cross-layer tests (CrossLayerMultiDataSourceSpec, CrossLayerMultiTenantMultiDataSourceSpec): these called manager.getServiceForConnection(SomeService, 'secondary') to fetch a service bean from the child datastore's Spring context — internal wiring that the registry no longer exposes that way. Rather than patching around a removed internal, the contract is now covered in two purpose-built specs: DomainMultiDataSourceSpec (domain-API routing to secondary) and DataServiceConnectionRoutingSpec (service routing via @Service(datasource='secondary')). The net line count for multi-datasource coverage went up, not down. That said, I can explicitly document the equivalence in the spec Javadoc if that helps reviewers verify the coverage isn't lost. --- tl;dr: The PR is wider than a typical bug-fix because the root change was architectural — every datastore (H5, H7, Mongo, Neo4j, SimpleMap), the test harness, and the TCK had local copies of the enhancement pattern that all needed updating. The scope tracks the scope of the original GormRegistry rewrite. -- 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]
