(2/3) feat: GORM O(M+N) scaling — GormRegistry, GormEnhancer, and core API refactor#15780
Open
borinquenkid wants to merge 65 commits into
Open
(2/3) feat: GORM O(M+N) scaling — GormRegistry, GormEnhancer, and core API refactor#15780borinquenkid wants to merge 65 commits into
borinquenkid wants to merge 65 commits into
Conversation
This was referenced Jun 27, 2026
borinquenkid
force-pushed
the
feat/gorm-datastore-infra
branch
from
da5001a to
4b9d006
Compare
borinquenkid
force-pushed
the
feat/gorm-registry-core-impl
branch
3 times, most recently
from
24dd795 to
b49c19f
Compare
Member
Author
|
Note on The Rationale: This is a known trade-off: it increases wall-clock time for those modules. It is intentional and scoped narrowly to GORM-dependent projects only — non-GORM modules keep the configured parallelism. A finer-grained alternative (per-spec registry reset in setup/cleanup) can supersede this later without changing the public API. |
Contributor
|
@borinquenkid does #15771 fully replace this PR? And then does that remove the need for #15790 |
Contributor
|
@borinquenkid I'm not sure I follow all of these PRs. There have been multiple opened and then closed. Can you help me understand what is still valid vs what needs closed and what order they're expected to be reviewed in? I don't see an index anywhere of expected review order. |
4 tasks
borinquenkid
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors core GORM API registration and lookup to use a centralized GormRegistry/GormApiResolver + pluggable GormApiFactory model, reducing eager per-qualifier API allocation and enabling qualifier-aware resolution (connections and multi-tenancy) at call time across core and adapter modules.
Changes:
- Introduces new registry/factory/resolver infrastructure for static/instance/validation APIs and transaction templates.
- Updates core GORM traits, validation, AST transforms, dynamic finders, criteria/query paths, and multi-tenancy listeners to route through the registry and resolver abstractions.
- Wires adapter-specific API factories (Hibernate 5/7, MongoDB) and adjusts impacted specs/build/test configuration.
Reviewed changes
Copilot reviewed 108 out of 108 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-test-examples/graphql/grails-test-app/src/integration-test/groovy/grails/test/app/UserRoleIntegrationSpec.groovy | Filters stdout capture to count only SQL lines |
| grails-test-examples/graphql/grails-test-app/src/integration-test/groovy/grails/test/app/TagIntegrationSpec.groovy | Filters stdout capture to count only SQL lines |
| grails-test-examples/graphql/grails-test-app/src/integration-test/groovy/grails/test/app/CommentIntegrationSpec.groovy | Filters stdout capture to count only SQL lines |
| grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/MultipleConnectionSourceCapableDatastore.java | Adds datastore SPI for multi-connection + routing hint |
| grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FirstAndLastMethodSpec.groovy | Expands pending-feature gating to Hibernate 7 suite |
| grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/base/GrailsDataTckManager.groovy | Simplifies domain class storage; adds deprecated bulk add method |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiWithDatastoreSessionSpec.groovy | Adds regression test for withDatastoreSession semantics |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiStringQueryDelegationSpec.groovy | Adds regression tests for string-query overload delegation |
| grails-datamapping-core/src/test/groovy/grails/gorm/services/ServiceTransformSpec.groovy | Adjusts reflection lookup expectations in service transform tests |
| grails-datamapping-core/src/test/groovy/grails/gorm/services/MethodValidationTransformSpec.groovy | Excludes trait datastore accessors from @Generated assertions |
| grails-datamapping-core/src/test/groovy/grails/gorm/services/CompileStaticServiceInjectionSpec.groovy | Updates expectations for datastore injection internals |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/listener/ValidationEventListener.groovy | Switches validation API lookup to GormRegistry |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/MappingContextTraversableResolver.groovy | Adds cascade-validation short-circuit hook |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/GormValidatorAdapter.groovy | Adds cascade-aware validation pathway + thread-local flag |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/constraints/builtin/UniqueConstraint.groovy | Routes static API lookup via GormRegistry |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy | Expands excluded method set; avoids re-decoration on locally annotated methods |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformation.groovy | Refactors datastore resolution in AST transform to use registry/api resolver |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/TransactionTemplateFactory.groovy | Adds SPI for datastore-specific transaction template creation |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/DefaultTransactionTemplateFactory.groovy | Default implementation using GrailsTransactionTemplate |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/UpdateStringQueryImplementer.groovy | Adjusts implementer ordering |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneStringQueryImplementer.groovy | Improves argument-list handling and query-text detection |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneInterfaceProjectionStringQueryImplementer.groovy | Adjusts implementer ordering for projection variant |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindAllByImplementer.groovy | Adds argument/property-type compatibility checks |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractStringQueryImplementer.groovy | Adds named-parameter extraction for strict parameter validation |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractServiceImplementer.groovy | Updates datastore/service/API lookup generation to use GormRegistry |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/DefaultTransactionService.groovy | Adds datastore getter/setter for service wiring |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/DefaultTenantService.groovy | Adds datastore getter/setter; recursion guard; improves exception messages |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java | Ensures query initialization for more operations; adds list(Closure) helper |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/transform/TenantTransform.groovy | Resolves tenant services via registry/api resolver, with service-specific datastore routing |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/TenantDelegatingGormOperations.groovy | Adds tenant-wrapped deleteAll overloads |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/MultiTenantEventListener.java | Tightens source validation; resolves datastore via registry; improves tenant id handling |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy | Adds schema quoting + (currently) extra stderr debug output |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/MultiTenantConnection.groovy | Changes close behavior for multi-tenant connection wrapper |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormValidationApiRegistry.groovy | New registry for validation APIs |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormValidationApi.groovy | Adds resolver-based construction; qualifier support; safer session/event access |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormValidateable.groovy | Routes validation API resolution through GormRegistry with clearer failure mode |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApiRegistry.groovy | New registry for static APIs |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormInstanceApiRegistry.groovy | New registry for instance APIs |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntityDirtyCheckable.groovy | Uses registry-based instance API lookup with clearer failure mode |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy | Routes entity API access through registry; adds deleteAll overloads; hardens missing-property behavior |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancerRegistry.groovy | Extracts/centralizes thread-local enhancer state |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormApiFactory.groovy | New SPI for per-datastore API object construction |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/ListOrderByFinder.java | Refactors order parsing and multi-property ordering; adjusts criteria application |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindOrSaveByFinder.java | Adds resolver-based constructors; delegates to updated base classes |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindOrCreateByFinder.java | Adds resolver-based constructors |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindByFinder.java | Adds resolver-based constructor |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindByBooleanFinder.java | Fixes method pattern; adds resolver-based constructor |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindAllByFinder.java | Adds resolver-based constructor; adjusts distinct behavior |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/FindAllByBooleanFinder.java | Adds resolver-based constructor; updates docs |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java | Adds resolver-based constructor; improves order-by defaulting (identity/composite ids) |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/CountByFinder.java | Refactors countBy* execution/build logic; adds resolver-based constructor |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/AbstractFinder.java | Adds DatastoreResolver support for call-time datastore selection |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/AbstractFindByFinder.java | Adds resolver-based constructor; adjusts generic callback type |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/AutoTimestampEventListener.java | Exposes lastUpdated property-name lookup (visibility change) |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/DefaultGormApiFactory.groovy | Default core API factory + dynamic finder wiring via resolver |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/DatastoreResolver.groovy | Introduces DatastoreResolver SPI and corrects prior misplaced content |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/ConnectionSourceNameResolver.groovy | Adds helper to extract/normalize connection source names |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/AbstractGormApiRegistry.groovy | Adds base registry with lazy qualifier API materialization + datastore removal |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/AbstractGormApi.groovy | Adds resolver/qualifier support; caches reflection results; qualifier-aware execution model |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/AbstractDatastoreApi.groovy | Moves datastore access to DatastoreResolver |
| grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionTemplate.groovy | Adds debug logging around execute/rollback flow |
| grails-datamapping-core/src/main/groovy/grails/gorm/MultiTenant.groovy | Routes withTenant/eachTenant through GormRegistry (with non-resolving path for enumeration) |
| grails-datamapping-core/src/main/groovy/grails/gorm/multitenancy/CurrentTenantHolder.groovy | Adds thread-local tenant binding helper for DISCRIMINATOR mode |
| grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy | Routes session access through registry static API lookup; returns callable result |
| grails-datamapping-core/src/main/groovy/grails/gorm/CriteriaBuilder.java | Adds call(Closure) convenience to execute criteria |
| grails-datamapping-core/src/main/groovy/grails/gorm/api/GormStaticOperations.groovy | Adds deleteAll overloads to static operations API |
| grails-datamapping-core/build.gradle | Adds grails-data-simple test dependency |
| grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/ExplicitSaveRepersistsSpec.groovy | Adds regression spec for explicit save re-persistence |
| grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/CoreTestSuite.groovy | Expands selected TCK classes in suite |
| grails-data-simple/src/main/groovy/org/grails/datastore/mapping/simple/SimpleMapDatastore.java | Improves connection datastore lookup + routing behavior for unit-test mock |
| grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/DisjunctionQuerySpec.groovy | Marks pre-existing MongoDB count-by-OR bug as pending feature |
| grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/connections/SchemaBasedMultiTenancySpec.groovy | Shares datastore + resets registry state for isolation |
| grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/connections/MultiTenancySpec.groovy | Shares datastore + resets registry state for isolation |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/gorm/mongo/MongoGormEnhancer.groovy | Registers Mongo API factory via GormRegistry |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/gorm/mongo/MongoGormApiFactory.groovy | Adds Mongo-specific static API factory implementation |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/gorm/mongo/api/MongoStaticApi.groovy | Captures persistent entity + tenancy mode locally |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/SchemaTenantGormEnhancerSpec.groovy | Adjusts test annotations for schema tenant entity |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/HibernateGormValidationApiSpec.groovy | Updates validation API lookup to use registry |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/HibernateGormStaticApiSpec.groovy | Adjusts static API access with explicit qualifier |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/HibernateGormInstanceApiSpec.groovy | Updates instance API lookup to use registry |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/connections/GormApiAllocationSpec.groovy | Updates allocation assertions to registry allocation checks |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/GrailsIdentityGeneratorSpec.groovy | Registers additional domain classes in spec setup |
| grails-data-hibernate7/core/src/test/groovy/org/grails/datastore/gorm/GormEnhancerCleanupSpec.groovy | Replaces reflection-based checks with registry mapping/API assertions |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HibernateQuery.java | Fixes junction creation to operate on the detached criteria backing store |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java | Makes retrieveAll preserve requested order/duplicates + type conversion |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy | Delegates propertyMissing to base (finder-closure-first behavior) |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormEnhancer.groovy | Registers Hibernate 7 API factory via registry |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormApiFactory.groovy | Adds Hibernate 7 API factory implementation |
| grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/MultiTenantFinderDispatchSpec.groovy | Adds regression spec for finder dispatch + session passing under tenancy |
| grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/GormApiAllocationSpec.groovy | Updates allocation assertions to registry allocation checks |
| grails-data-hibernate5/core/src/test/groovy/org/grails/datastore/gorm/GormEnhancerCleanupSpec.groovy | Replaces reflection-based checks with registry mapping/API assertions |
| grails-data-hibernate5/core/src/test/groovy/org/apache/grails/data/hibernate5/core/GrailsDataHibernate5TckManager.groovy | Simplifies config handling; sets suite flags/transactional defaults |
| grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java | Makes retrieveAll preserve requested order/duplicates + type conversion |
| grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy | Delegates propertyMissing to base (finder-closure-first behavior) |
| grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormEnhancer.groovy | Registers Hibernate 5 API factory via registry |
| grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormApiFactory.groovy | Adds Hibernate 5 API factory implementation |
| grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/fetcher/manager/GraphQLDataFetcherManagerSpec.groovy | Updates test registry setup to use GormRegistry static API registry |
| gradle/test-config.gradle | Adds Gradle-9-safe transitive project dependency check and serializes GORM-dependent tests |
Comments suppressed due to low confidence (2)
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy:64
- Remove the
System.err.printlndebug output. Emitting SQL text unconditionally to stderr can leak sensitive identifiers into logs and adds noisy output;log.debug(...)already covers trace logging.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy:79 - Remove the
System.err.printlndebug output increateSchema. Unconditional stderr output is noisy and can expose schema names;log.debug(...)already captures the statement when debug logging is enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
borinquenkid
added a commit
that referenced
this pull request
Jul 2, 2026
- MultiTenantConnection.close(): the GormRegistry rewrite dropped the
default-schema restoration before returning a connection to the pool.
In SCHEMA-per-tenant multi-tenancy this risks cross-tenant schema
leakage on connection reuse. Restore the try/finally that resets the
schema before closing the target connection.
- GormValidationApi.getValidator(): the rewrite to resolve the
MappingContext via the (qualifier-aware) datastore dropped the
original caching of the resolved Validator, causing repeated
resolution on every call. Cache it in internalValidator once resolved.
- AbstractStringQueryImplementer: remove a hard-coded substring check
("wrong" / "java.lang.String") in the constant-@query branch. It was
dead code for the tests it appeared to guard (those use GStrings,
validated separately by QueryStringTransformer) and would incorrectly
reject any legitimate constant query containing those substrings.
- Remove leftover println/System.err.println debug output in
GormValidatorAdapter and DefaultSchemaHandler.
- Fix corrupted Javadoc escaping ("in\"" etc.) in AbstractCriteriaBuilder.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…refactor Introduce GormRegistry singleton replacing O(M×N) static maps in GormEnhancer. APIs are registered once at entity-registration time and looked up in O(1). - GormRegistry: singleton keyed by (entityClass, qualifier); handles MultiTenant qualifier expansion, thread-local preferred datastore, and concurrent-safe removal - GormApiFactory / DefaultGormApiFactory: pluggable factory per datastore type - GormApiResolver: routes static/instance/validation API lookups through the registry - GormEnhancer: delegates all registration and lookup to GormRegistry - GormStaticApi / GormInstanceApi / GormValidationApi: use DatastoreResolver instead of holding a direct Datastore reference; support qualifier-aware execution - AbstractGormApi.execute(): distinguishes datasource connection qualifiers from tenant-ID qualifiers to avoid overwriting the active tenant context - CurrentTenantHolder: thread-safe tenant binding for DISCRIMINATOR multi-tenancy - ServiceTransformation / TransactionalTransform: resolve transaction manager via GormRegistry instead of static map lookups Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…efactor HibernateGormEnhancer (H5 and H7) both declare @OverRide registerConstraints as a no-op. The scaling commit's GormEnhancer refactor omitted this protected hook method, making the @OverRide annotation invalid and causing a Java stub compilation error: "method does not override or implement a method from a supertype". Restores the original implementation (loads ConstraintRegistrar via reflection if present) and calls it from the constructor, consistent with the pre-scaling behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ropped by scaling refactor GormEnhancer: restore protected registerConstraints(Datastore) hook that H5/H7 HibernateGormEnhancer override as a no-op. Its absence broke Java stub generation with "@OverRide … method does not override a supertype method". MongoStaticApi: restore persistentEntity and multiTenancyMode fields that GormStaticApi no longer carries after the scaling refactor. Initialise persistentEntity from the mapping context and multiTenancyMode from MongoDatastore.getMultiTenancyMode() so wrapFilterWithMultiTenancy and preparePipeline compile under @CompileStatic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…stractGormApi.execute() The GORM scaling commit introduced a non-default qualifier path in execute() that unconditionally called Tenants.withId(datastore, qualifier) for multi-tenant entities. This was correct for DATABASE mode (qualifier == tenant ID == connection name) but broke DISCRIMINATOR mode: when a @service with @transactional(connection='secondary') executed a query, 'secondary' was bound as the current tenant ID instead of the real tenant from the TenantResolver, causing discriminator filters to match 'secondary' and return 0 rows. Fix: probe getDatastoreForConnection(qualifier) to determine whether the qualifier names a real datasource connection. If it resolves (non-null), it is a connection name — fall through to executeQualified without touching the tenant context. If it throws or returns null, the qualifier is a tenant ID (e.g. from withTenant()) — bind it via Tenants.withId as before. Update GormRegistrySpec to explicitly stub getDatastoreForConnection(_) >> null on the DISCRIMINATOR-mode test stub, mirroring real HibernateDatastore behaviour (which throws ConfigurationException for unknown connection names) and avoiding Spock's covariant- interface default of returning the stub itself. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…istry findStaticApi/InstanceApi/ValidationApi: the tenant-lookup at priority-2 only checked CurrentTenantHolder. In DATABASE and SCHEMA modes the tenant ID is never stored there explicitly — it comes from the TenantResolver (e.g. a subdomain or system-property resolver). Consult the resolver for those strict modes so that per-tenant child APIs are selected correctly even when no tenant has been bound via Tenants.withId(). Guard with TenantNotFoundException propagation so missing tenants surface as errors rather than silently falling back to the default API. Also skip the API redirect when tenantId equals 'default' to avoid self-loops. createStaticApi / createInstanceApi / createValidationApi: replace the caller- supplied DatastoreResolver with a bound lambda that always returns the specific Datastore captured at registration time. The old resolver was evaluated lazily at call time and could invoke tenant-resolution logic before any tenant context was active, causing spurious TenantNotFoundException during bootstrapping. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#15780 coverage gap GormEnhancer.groovy sat at 49.5% patch coverage (47 bad lines): the deprecated static/protected delegators, the findEntity static helper, allQualifiers' foreign-datastore branch, and - hardest to reach - the addStaticMethods/addInstanceMethods closures were untested. Those closures only execute when a real missing-method/property call goes through Groovy's ExpandoMetaClass dispatch on the actual entity class, not when calling the underlying API object directly (as GormStaticApi's own spec does), so covering them required exercising real dynamic-finder calls, unresolvable property access, and unresolvable method calls on a live SimpleMapDatastore-backed entity. 9 tests. The pre-existing GormEnhancerAllQualifiersSpec already covered allQualifiers' same-datastore path and registerEntity/close's happy paths thoroughly via Mock-based datastores; this spec fills the remaining gaps using a real SimpleMapDatastore, since the datastore's own internal GormEnhancer already registers/enhances the entity against the GormRegistry singleton, so most tests don't need to construct their own GormEnhancer at all - only the ones exercising protected/deprecated instance methods or an explicitly foreign datastore need one. Line coverage 89.5% (119/133, up from 55.6%), method coverage 39/42 (up from 21/42). Full module suite green, 0 regressions. codeStyle clean (test-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing PR #15780 coverage gap GormValidationApi.groovy sat at 34.8% patch coverage - the worst in the PR - with 15 of 23 methods fully uncovered: the alternate constructors, forQualifier/executeQualified, getTransactionManager, the entire validate(...) flow (flush-mode save/restore, event firing, the plain/CascadingValidator dispatch, field-filtered errors), and the getErrors/setErrors/hasErrors/clearErrors pair of code paths (a GormValidateable instance stores errors on itself; anything else goes through the datastore's current session). 19 tests. setValidator() proved the key to testing doValidate (private, reached via the public validate(...) overloads) without needing a real GORM validation setup - it bypasses getValidator()'s whole resolution chain, letting each test drive a specific validator implementation (plain Validator vs grails.gorm.validation.CascadingValidator) directly. One dead branch found and deliberately left uncovered: org.grails.datastore.gorm.validation.CascadingValidator extends grails.gorm.validation.CascadingValidator, so the `else if` branch checking for the former is unreachable - any real implementation is already caught by the first `instanceof` check. Line coverage 93.4% (127/136, up from 34.6%), method coverage 22/23 (up from 8/23). Full module suite green, 0 regressions. codeStyle clean (test-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
coverage gap GormInstanceApi.groovy had no dedicated spec at all - 56.8% patch coverage, 42 of 61 methods fully uncovered: the alternate constructors, forQualifier/executeQualified/getTransactionManager, propertyMissing's connection-source/DynamicAttributes/MissingPropertyException branches, instanceOf's EntityProxy unwrapping, the full save/insert/delete/attach/ discard/refresh/read/ident surface, and the DirtyCheckable-backed isDirty/getDirtyPropertyNames/getPersistentValue methods. 26 tests, built around a real SimpleMapDatastore (matching the house style already used for GormStaticApi/GormRegistry). setValidator() on the entity's own registered GormValidationApi (fetched via GormRegistry.instance.getValidationApi(cls) - the same instance save()'s registry.resolveValidationApi(...) resolves to) proved the way to force deterministic validation failure, since static constraints DSL blocks aren't evaluated into a real Validator outside a full Grails app. Also found and documented (not fixed - lives in a different file) a quirk in core ValidationException: its own static newInstance(...) factory always constructs its dynamically-resolved VALIDATION_EXCEPTION_TYPE, ignoring whatever Class GormInstanceApi's own validationException field holds as the call receiver. Line coverage 93.3% (111/119, up from 37.0%), method coverage 53/61 (up from 19/61). Full module suite green, 0 regressions. codeStyle clean (test-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
coverage gap AbstractGormApi.groovy was only exercised incidentally through GormStaticApi/GormInstanceApi/GormValidationApi's own specs (50.7% patch coverage) - the null-datastore guard in execute(), the bound-tenant delegation branch when the DEFAULT qualifier is in use, the reflection-based getMethods()/getExtendedMethods() cataloging, and the unused ConstantDatastoreResolver helper were all untested. 5 tests. Used a minimal purpose-built AbstractGormApi subclass (MinimalGormApi, overriding executeQualified to just record its argument) to test execute()'s tenant-dispatch branch in isolation, rather than routing through GormStaticApi's own resolution - which turns out to depend on the GormRegistry *singleton* regardless of which registry instance the api was constructed with, since GormStaticApi.executeQualified calls the static GormRegistry.findStaticApi(...) delegator, not an instance-scoped lookup. Worth remembering for any future item that needs to control qualified-api resolution precisely. Line coverage 98.7% (78/79, up from 59.5%), method coverage 24/24 (up from 13/24 - 0 methods now fully uncovered). Full module suite green, 0 regressions. codeStyle clean (test-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rage gap Tenants.groovy had no spec at all despite being the primary public API for multi-tenancy: 50.0% patch coverage, 37 of 47 methods fully uncovered - most of the thin static wrapper methods around the swappable Tenants.datastoreLocator field, plus large untested branches in the two methods with real dispatch logic (withId(MultiTenantCapableDatastore,...) and eachTenant(MultiTenantCapableDatastore,...)). 37 tests. Swapped Tenants.datastoreLocator for a test-controlled DatastoreLocator (restored in cleanup()) to deterministically drive the no-arg/domain-class/type-based locator methods without touching the GormRegistry singleton. Covered withId's three dispatch paths (already-bound child session, shared-connection, non-shared withNewSession) each across their 0/1/2-arg closure-arity branches plus the too-many-args guard, and eachTenant's four mode/resolver combinations (DATABASE+AllTenantsResolver, DATABASE+ConnectionSources iteration, shared+AllTenantsResolver, shared without one -> exception). Line coverage 95.4% (145/152, up from 23.7% - the largest baseline-to-result jump of any item so far), method coverage 44/47 (up from 10/47). Full module suite green, 0 regressions. codeStyle clean (test-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#15780 coverage gap DynamicFinder.java had 19.4% Codecov patch coverage (worst remaining item in the PR #15780 coverage checklist). The pre-existing DynamicFinderSpec only covered the static buildMatchSpec helper. Added DynamicFinderCoverageSpec, exercising the real method-name-parsing / expression-building pipeline through actual dynamic finder calls on a SimpleMapDatastore-backed entity (Equal, GreaterThan/LessThan/Between, Like/InList/NotEqual, IsNull/IsNotNull, Not-negation, And/Or combination, MissingMethodException on arg-count/conversion failures), list(Map) argument handling (sort as string/Map, fetch as FetchType map/string alias, cache, no-sort fallback), getFetchMode's alias table, where{}.list() detached criteria fetch/order, registerNewMethodExpression's custom-clause extension hook, the MappingContext-only constructor, the invoke(..., DetachedCriteria, ...) overload, and populateArgumentsForCriteria(BuildableCriteria, Map) directly (confirmed via repo-wide grep to be dead code - no production caller uses this overload, only the (Class, Query, Map) one - but still a public static API worth covering). Notable findings, neither a bug: - The operator-style where{ age > 20 } DSL relies on a compile-time AST transform this plain test-module compilation doesn't apply; the method-call DSL (where { gt('age', 20) }) works without it. - A custom MethodExpression's finder clause keyword is the registered class's simple name exactly (e.g. class AlwaysTrue -> findAllByXAlwaysTrue), not a suffixed variant. - CriteriaBuilder instances from a bare createCriteria() (outside an active query-execution closure) have a null internal query field, so join()/ cache() NPE when called directly; populateArgumentsForCriteria(BuildableCriteria, Map) is therefore only exercised here via its sort/order branches. DynamicFinder.java line coverage: 46.5% (194/417) before this item's work -> 74.0% (305/412) after, per local JaCoCo. Full grails-datamapping-core suite and codeStyle both pass with no regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…closing PR #15780 coverage gap Covers the abstract-class-with-explicit-constructor compile error and the concrete (non-interface, non-abstract) @service class path, both new in the GormRegistry rewrite and previously untested from this module's own JaCoCo perspective. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ng PR #15780 coverage gap No spec existed for this class at all. Covers the new isValidSource guard, the DEFAULT+numeric tenant id coercion, and the existing-property-wins override on insert - all new in the GormRegistry rewrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rageSpec closing PR #15780 coverage gap Covers buildNamedParamsFromQuery's named-parameter binding for constant @query strings, and FindOneStringQueryImplementer's ArgumentListExpression merge branch that consumes it - both new in the GormRegistry rewrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#15780 coverage gap Covers the mode==NONE guard shared by currentId/withoutId/withCurrent/ withId, each method's delegation to Tenants.*, and the new RESOLVING reentrancy guard on withCurrent/withId. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…15780 coverage gap Brand-new file in the GormRegistry rewrite; no dedicated spec existed. Covers the full public contract including the "restore the previous binding" branches of withTenant(Class,...)/withoutTenant when nested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… closing PR #15780 coverage gap Covers the 4 new deleteAll(...) overloads this PR added, matching the pre-existing delegation pattern used by the other ~90 methods on this class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing PR #15780 coverage gap Covers executeAndRollback's functional contract (result passthrough, always-rollback, checked/unchecked exception unwrapping), which had no coverage of its own before this PR added debug logging around it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ct against a null query
AbstractCriteriaBuilder gained ensureQueryIsInitialized() guards on cache/
join/select in this PR to fix the NPE a bare createCriteria() (not inside
.list{}/.get{}) hit on those methods. CriteriaBuilder overrides all three
with its own versions that touch `query` directly, unguarded, so the NPE
was still reachable through the concrete class real callers actually use
(GormStaticApi#createCriteria() returns a CriteriaBuilder, not the abstract
base). Added the same guard to CriteriaBuilder's own overrides.
Also adds CriteriaBuilderSpec, closing PR #15780's coverage gap on both
AbstractCriteriaBuilder and CriteriaBuilder (the concrete class most real
callers get), including the fix's regression test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#15780 coverage gap Brand-new file. Covers qualify()'s null-datastore fallback and findStaticApi(Class, String)'s own instance method, which GormRegistry's static findStaticApi delegator does not route through. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…losing PR #15780 coverage gap Brand-new file. Covers the fallback path for a non-ConnectionSourcesProvider datastore, the one gap left uncovered by incidental SimpleMapDatastore usage elsewhere in the suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…GormInstanceApiRegistrySpec closing PR #15780 coverage gap Structurally identical to GormStaticApiRegistry (same AbstractGormApiRegistry subclass pattern) - both brand-new files with no dedicated spec. Covers qualify()'s null-datastore fallback and each class's own findXApi(Class, String) instance method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… coverage gap Covers the new DatastoreResolver-based constructor + lazy getDatastore(), which DefaultGormApiFactory#createDynamicFinders uses to construct every named dynamic finder (FindAllByFinder, CountByFinder, ListOrderByFinder, etc.) in the GormRegistry rewrite, via real dynamic finder calls against a SimpleMapDatastore-backed entity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…losing PR #15780 coverage gap Covers the new type-compatibility check added to the dynamic-finder property-validation loop: a finder parameter whose type doesn't match the matched property's declared type now fails to compile with a clear message, instead of silently generating a finder that would misbehave at runtime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…15780 coverage gap Covers the simplified shouldSaveOnCreate()-based implementation (replacing a bespoke ~25-line doInvokeInternal override) and the 3 new constructor overloads added to match sibling finder classes' shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#15780 coverage gap Covers the new DatastoreResolver-based constructor + lazy getDatastore() (returns null instead of throwing when unconfigured), via a minimal purpose-built subclass since this class's only real subclass, AbstractGormApi, overrides execute() with its own qualifier-aware version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…coverage gap grails-data-simple had no src/test at all - added testImplementation 'org.spockframework:spock-core' to enable it (matching the pattern already used by grails-datastore-core's build.gradle). Covers the new leaf-datastore idempotent getDatastoreForConnection resolution and the new routesUnqualifiedToMappedConnection() override. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Spec closing PR #15780 coverage gap Covers isValidParameter's GormProperties.IDENTITY shortcut (a method parameter literally named `id` is always a valid identity parameter without needing a matching declared property), via a save-style method binding an explicit id alongside a real domain property. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ng PR #15780 coverage gap grails-datamapping-tck is deliberately not test-configured (shared TCK base class library, no test task of its own) - verified against a downstream adapter's suite per this repo's own TCK constraints. Covers the new @deprecated addAllDomainClasses(Collection) backward-compatible delegate to registerDomainClasses(Class...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erSpec closing PR #15780 coverage gap Covers the new GormValidatorAdapter.CASCADE_VALIDATION short-circuit guard on isCascadable, which skips cascade validation entirely when explicitly disabled regardless of the association/owning-side state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing PR #15780 coverage gap Covers the new explicit datastore/getDatastore()/setDatastore() Service contract this PR added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pec closing PR #15780 coverage gap Brand-new file. Covers all 3 createTransactionTemplate overloads, including the (PlatformTransactionManager, TransactionAttribute) one GormStaticApi doesn't call anywhere but is part of this factory's public contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing PR #15780 coverage gap Covers a method redundantly re-annotated with the same annotation as the class it's in, exercising the new hasLocalAnnotation guard added to AbstractMethodDecoratingTransformation to avoid re-decorating such methods. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
coverage gap Added testImplementation project(':grails-data-simple') to exercise the new constructor-time persistentEntity/multiTenancyMode field assignments' "not a MongoDatastore" fallback branch cheaply, without needing a real Docker-backed MongoDB instance. The existing Docker-backed MongoStaticApiMultiTenancySpec already covers the real-MongoDatastore branch; together both ternary directions are now covered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 3a09015 Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces the
GormRegistrysingleton that replaces the O(M×N) static map allocation inGormEnhancer. APIs are registered once at entity-registration time and looked up in O(1).GormRegistry— singleton keyed by(entityClass, qualifier); handlesMultiTenantqualifier expansion, thread-local preferred datastore, and concurrent-safe removal onclose()GormApiFactory/DefaultGormApiFactory— pluggable factory per datastore type; adapters override this to supply typed API instancesGormApiResolver— routes static/instance/validation API lookups through the registry with fallback to the default datastoreConnectionSourceNameResolver— extracts and normalises connection-source names from a datastore without leakingConnectionSourcesSupportinternalsGormEnhancer— delegates all registration and lookup toGormRegistry;allQualifiers()used only for datastore routing, not eager API allocationGormStaticApi/GormInstanceApi/GormValidationApi— useDatastoreResolverinstead of holding a directDatastorereference; support qualifier-aware execution viaexecuteQualified()AbstractGormApi.execute()— distinguishes datasource connection qualifiers from tenant-ID qualifiers to avoid overwriting the active tenant contextCurrentTenantHolder— thread-safe tenant binding forDISCRIMINATORmulti-tenancyServiceTransformation/TransactionalTransform— resolve transaction manager viaGormRegistryinstead of static map lookupsDefaultTransactionTemplateFactory/TransactionTemplateFactory— pluggable transaction template creation per datastore typeTest plan
./gradlew :grails-datamapping-core:testpasses (tests are in the companion PR)GormEnhancerAllQualifiersSpecStack
feat/gorm-datastore-infra)feat/gorm-registry-core-tests— full test suite for this PR🤖 Generated with Claude Code