matrei commented on PR #16368:
URL: https://github.com/apache/grails-core/pull/16368#issuecomment-5773813029
## Review Findings
Head `8056ea18db`, base `8.0.x` at `1fadd5c812`; the branch sits directly on
the base and merges clean. All 77 completed CI checks pass (macOS build still
pending); the one TestLens failure is `UserControllerSpec > User list` on an
outdated commit, the known 9%-flaky scaffolding Geb test (#16030), unrelated to
this change.
What I ran on the head, all green:
- `:grails-datamapping-core-test:test --tests MultipleDataSourceSpec`: 4
tests, 0 failures.
- `:grails-data-neo4j-core:test --tests NamedConnectionsSpec --tests
Neo4jConnectionSourceSettingsSpec`: 4 tests, 0 failures.
- `:grails-data-mongodb-core:test` for the seven touched specs
(`MultipleConnectionsSpec`, `CheckpointRestoreConnectionsSpec`,
`SchemaBasedMultiTenancySpec`, `MongoDatastoreLifecycleSpec`,
`MongoDatastoreExternalClientSpec`, `MongoConnectionSourceFactorySpec`,
`MongoConnectionSourceSpec`): 34 tests, 0 failures, against a real container.
- `:grails-data-mongodb-spring-data:test`, the whole module: 17 tests, 0
failures.
- `codeStyle` on `grails-datamapping-core`, `grails-data-mongodb-core`,
`grails-data-mongodb-spring-data` and `grails-data-neo4j-core`: no violations.
The CRaC part, the Neo4j startup fix and the Neo4j qualifier fix are all
correct as far as I can verify, and each of the claims in the description that
I could check holds (details under Confirmed). The findings are about the
connection scope in `GormRegistry`, which is the one piece that changes
behaviour for every GORM implementation rather than just MongoDB.
### [P2] A scope named `default` does not mean what `Book.'default'` means,
and the two halves of one `withConnection` block can disagree
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:550-557`,
`603-609`, `659-665`
-
`grails-data-mongodb/core/src/main/groovy/grails/mongodb/MongoEntity.groovy:243-251`
`resolveStaticApi` (and the instance and validation twins) short-circuit to
`staticApiRegistry.getDirect(normalizedClassName, scoped)` before the
`MultiTenant` handling. For a non-tenant entity, and for a tenant entity with a
non-default scope, that is exactly what naming the connection would do:
Priority 1 of the multi-tenant branch is the same `getDirect` call. It differs
only when the scope is `default` on a `MultiTenant` entity in DATABASE or
SCHEMA mode: an explicit `Book.'default'` skips Priority 1 and resolves the
*current tenant's* API at Priority 2, while the scope returns the literal
default connection's API.
I checked this with a throwaway spec on a `SimpleMapDatastore` in DATABASE
mode with connections `default`, `foo` and `bar`, tenant `foo` bound through
`SystemPropertyTenantResolver`, two books in `foo`, one in `bar`:
```
ProbeBook.'default'.count()
== 2 // tenant foo
GormRegistry.withConnectionScope(ProbeBook, ConnectionSource.DEFAULT) {
ProbeBook.count() } == 0 // the default store
```
Inside `Book.withConnection(ConnectionSource.DEFAULT) { }` on such an entity
the closure's delegate is `GormEnhancer.findStaticApi(this, 'default')`, which
resolves the tenant's API, so `list()` reads the tenant and `Book.list()` reads
the default store in the same block. The test in `MultipleDataSourceSpec`
(`withConnectionScope(Player, ConnectionSource.DEFAULT)`) does not see this
because `Player` is not multi-tenant.
The fix is also a simplification. Rather than a separate lookup, feed the
scope in as the qualifier and let the existing resolution run:
```groovy
GormStaticApi resolveStaticApi(Class entityClass, String qualifier) {
String normalizedClassName = normalizeEntityKey(entityClass)
// A lookup that names no connection follows the scope, exactly as if it
had named it.
if (qualifier == null) {
qualifier = scopedConnection(normalizedClassName)
}
String normalizedQualifier = normalizeQualifier(qualifier)
...
```
Same three lines in `resolveInstanceApi` and `resolveValidationApi`, and the
three `getDirect` blocks go. Then "the scope is what naming the connection
would do" is true by construction, the documentation sentence "An operation
that names its own connection keeps it" needs no caveat, and most of the
partial branches Codecov flags in this file go with them. For every case the
PR's tests cover the result is identical.
### [P3] A scope silently overrides a `Tenants.withId` nested inside it
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:553`
Because the scope is consulted before `CurrentTenantHolder`, this reads
`bar`, not `foo` (same probe as above):
```groovy
GormRegistry.withConnectionScope(ProbeBook, 'bar') { Tenants.withId('foo') {
ProbeBook.count() } } // 1, bar's count
```
That is consistent with the explicit form, `Tenants.withId('foo') {
ProbeBook.bar.count() }`, where the named connection also wins, so I would not
change the precedence. But it is a new way for a tenant switch to be ignored
without any error, and DATABASE-mode tenants *are* connections, so
`withConnection` on a multi-tenant entity is not an outlandish thing to write.
One sentence in the `withConnectionScope` javadoc and in the two
`withConnection` guide pages ("the block's connection takes precedence over the
current tenant for that entity") would keep someone from debugging it. The P2
change does not alter this behaviour, since Priority 1 runs before the tenant
lookup either way.
### Nit: the `withConnection` pages could say the routing is per thread
References:
-
`grails-data-mongodb/docs/src/docs/asciidoc/multipleDataSources/mongoClientSwitching.adoc:29`
-
`grails-data-neo4j/docs/src/docs/asciidoc/multipleDataSources/boltDriverSwitching.adoc:29`
The scope is a `ThreadLocal`, so `Book.withConnection('moreBooks') {
executor.submit { Book.list() } }` runs `Book.list()` on the default connection
in the pool thread. The old delegate-based behaviour had the same limit for
`list()`, but the new sentence "Every operation on `Book` in the closure uses
the alternate connection" is broader, and worth a "on the calling thread" so it
stays accurate.
### Nit: comment wording
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:2001`
"// One GORM owns can be replaced after a restore; see start()." is missing
a word; "One that GORM owns can be replaced ..." reads as intended.
### Nit: three fixes, one issue
The PR fixes #16366 and, along the way, a Neo4j datastore that could not
start with any named connection and a Neo4j static API that wrote through the
wrong connection. Both are real bugs that stand on their own and neither has an
issue; they are only findable from this PR's commit messages. Worth a tracking
issue each, or at least the numbers in the release notes, so someone bitten by
"named Neo4j connection fails at startup with `The event loop thread may not be
smaller than 1`" can find the fix. Not a reason to split the PR: the commits
are already separated cleanly.
### Confirmed
- `stop()` closes every owned client (default, configured, added at runtime)
and `start()` replaces exactly those flagged `clientStopped`; a connection
added while stopped is left alone (`clientStopped` false), which matches
`resumeIndexBuild`'s existing handling of the same case. With a supplied
default client and named connections, the supplied one is neither closed nor
replaced and the datastore still goes non-running so Spring calls `start()`:
the `0 * supplied.close()` test covers it.
- `MongoConnectionSourceFactory.create(name, settings)` copies the builder
(`MongoClientSettings.builder(builder.build())`) and re-applies the URL and
every `MongoClientSettingsBuilderCustomizer`, so reusing a child's `settings`
for its replacement is safe and loses nothing. The default connection cannot be
rebuilt from its settings on the supplied-client and `Builder` constructor
paths because `createDefaultConnectionSources` nulls the URL, which is why
`defaultClientOptions` has to be kept; the replacement is built through the
same static `createMongoClient` as the original, so the two paths agree.
- `SingletonConnectionSources` wraps the parent's `ConnectionSource`
instance, so `replaceSource` on a child's default connection source is visible
through the parent's `connectionSources`; the lifecycle spec asserts
`getConnectionSource(name).source.is(client)` for each.
- `close()` after a restore: `MongoConnectionSource.close()` closes the
replacement, then the `inUse` loop closes it again. `MongoClientImpl.close()`
is guarded by an `AtomicBoolean` in driver 5.x, so the double close is a no-op.
With a factory that returns a plain `DefaultConnectionSource`, the loop is what
closes the replacements, and the spec for that case passes.
- `DatastoreMongoClientDatabaseFactory`: in Spring Data MongoDB 5.1.1,
`SimpleMongoClientDatabaseFactory.doGetMongoDatabase`, `getSession` and
`destroy` all call `getMongoCluster()`, and `MongoDatabaseFactorySupport` reads
its own field only inside `getMongoCluster()`, so the override really does
cover every use. `mongoInstanceCreated` is `false` on the constructor used, so
`destroy()` will not close GORM's client.
- `resolveTenantIds()` now reads through `getMongoClient()`, which is the
volatile field `start()` replaces. With the factory-created default connection
source now a `MongoConnectionSource`, the old
`defaultConnectionSource.getSource()` would also have worked, but the datastore
field is right for the custom-factory case too.
- `Neo4jDriverConfigBuilder`: `Config.defaultConfig()` is the static
`EMPTY`, `ConfigBuilder.eventLoopThreads` starts at `0`, and
`withEventLoopThreads(0)` throws "The event loop thread may not be smaller than
1". `ConfigurationBuilder` skips a `null` fallback, so returning `null` for a
value equal to the driver default is the right signal. A value the default
connection sets *to* the driver default is not inherited, but the named
connection gets the same default from the builder, so nothing observable
changes. `withMaxConnectionPoolSize(0)` would be the other sentinel and is
never hit (default 100).
- `Neo4jGormApiFactory` now builds `Neo4jGormStaticApi` with the same
six-argument constructor `MongoGormApiFactory` uses for `MongoStaticApi`; the
new constructor only delegates to `GormStaticApi`, and nothing in
`Neo4jGormStaticApi` cached the datastore the old constructor received
(`datastore` is read through the base resolver at each use).
- `withConnectionScope` nests, restores the previous entry, removes the
thread-local when the map empties, and is undone on exception;
`MultipleDataSourceSpec` covers nesting and the throw. `GormEntity`'s
`currentGormStaticApi()`/`currentGormInstanceApi()` and
`DetachedCriteria.withPopulatedQuery` all go through `resolveStaticApi(cls,
null)` / `resolveInstanceApi(cls, null)`, so `Book.list()`, dynamic finders,
`where` and `book.save()` follow the scope, as tested against MongoDB with
`ScopedCompany` and Neo4j with `RoutedCompany` on two embedded servers.
- Docs: the CRaC section's claim that an embedded MongoDB is stopped after
the clients and started before them holds (`MongoDatastore.LIFECYCLE_PHASE =
-1000` above `EmbeddedMongoLifecycle.PHASE`). The Neo4j upgrading note for the
`withConnection` change is the right place given the old guide documented the
opposite; MongoDB's guide already promised the new behaviour, so a
release-notes line is enough there.
- No user-facing doc references `MongoConnectionSource`,
`DatastoreMongoClientDatabaseFactory` or `withConnectionScope`.
## Verification
- `./gradlew :grails-datamapping-core-test:test --tests
grails.gorm.tests.MultipleDataSourceSpec :grails-data-neo4j-core:test --tests
grails.gorm.tests.NamedConnectionsSpec --tests
org.grails.datastore.gorm.neo4j.config.Neo4jConnectionSourceSettingsSpec`:
BUILD SUCCESSFUL; 4 + 2 + 2 tests, 0 failures.
- `./gradlew :grails-data-mongodb-core:test --tests <the seven specs above>
:grails-data-mongodb-spring-data:test`: BUILD SUCCESSFUL; 34 + 17 tests, 0
failures, Docker available.
- `./gradlew :grails-datamapping-core:codeStyle
:grails-data-mongodb-core:codeStyle :grails-data-mongodb-spring-data:codeStyle
:grails-data-neo4j-core:codeStyle`: no violations.
- A throwaway `ScopeVsTenantProbeSpec` in `grails-datamapping-core-test`
(DATABASE-mode `SimpleMapDatastore`, three connections,
`SystemPropertyTenantResolver`) produced the numbers quoted in the P2 and P3,
then was deleted; the worktree is clean.
- Spring Data MongoDB 5.1.1 and Neo4j driver 4.4.13 sources from the Gradle
cache for the factory and `Config` claims.
--
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]
