matrei commented on PR #16369:
URL: https://github.com/apache/grails-core/pull/16369#issuecomment-5781700512
## Review Findings, round 3
Head `c82dd69b25`, base `8.0.x` at `1fadd5c812`; the branch still merges
clean against the current `8.0.x`. This round covers the five commits since
`190ac9c388`: `077f33c2c9` (configuration metadata), `12497cfb51`
(`recreateOnConflict` for a taken index name), `fdd67c9244` (the client the
Boot auto-configuration builds), `07f68726a4` (`withNewSession(tenantId)` binds
the tenant) and `c82dd69b25` (a class a block does not name keeps its own
connection). They were pushed in the twenty minutes after my round-2 comment,
so that round's two P3s and two nits are unaddressed and still stand; I have
not repeated them here beyond one line each at the end.
What I ran on the head, all green (build cache off, `cleanTest` first,
result XML timestamps from this run):
- `:grails-datamapping-core-test:test` and `:grails-datamapping-core:test`,
both whole modules: 624 + 1 082 tests, 0 failures.
- `:grails-data-hibernate5-core:test` and
`:grails-data-hibernate7-core:test` for `MultipleDataSourceConnectionsSpec` and
the refresh-lock specs: 110 + 109 tests, 0 failures.
- `:grails-data-mongodb-core:test` for `MultipleConnectionsSpec`, every
`*Index*` spec, every `*Tenan*` spec, `MongoConfigurationMetadataSpec`,
`CheckpointRestoreConnectionsSpec`, `MongoDatastoreLifecycleSpec` and
`MongoDatastoreExternalClientSpec`: 39 specs, 120 tests, 0 failures, against a
real container.
- `:grails-data-mongodb-spring-boot:test`, the whole module: 4 tests, 0
failures.
- `codeStyle` on `grails-datamapping-core`, `grails-data-mongodb-core` and
`grails-data-mongodb-spring-boot`: no violations.
- A throwaway Hibernate 7 spec, deleted afterwards, run on both `c82dd69b25`
and the base, for the bystander case under Confirmed.
And one thing that is not green, which CI found first.
### [P1] `@Rollback("moreBooks")` and `@Transactional("moreBooks")` now
switch every multi-tenant class to that tenant, and the schema-per-tenant
examples fail
References:
- CI: `Hibernate7 Functional Tests`, shards 1 and 2 on Java 21 and 25,
`:grails-test-examples-hibernate7-grails-schema-per-tenant:test` FAILED; the
Hibernate 5 shards are still pending and will fail the same way
-
`grails-test-examples/hibernate7/grails-schema-per-tenant/src/test/groovy/schemapertenant/SchemaPerTenantSpec.groovy:58-66`
and the identical Hibernate 5 spec
-
`grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionTemplate.groovy:96-100`
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:583-590`
```
SchemaPerTenantSpec > Test should rollback changes in a previous test FAILED
Expected exception of type
'org.grails.datastore.mapping.multitenancy.exceptions.TenantNotFoundException',
but no exception was thrown
at
schemapertenant.SchemaPerTenantSpec.$tt__$spock_feature_1_0(SchemaPerTenantSpec.groovy:66)
...
at
grails.gorm.transactions.GrailsTransactionTemplate.doExecuteAndRollback(GrailsTransactionTemplate.groovy:105)
at
org.grails.datastore.gorm.GormRegistry.runInScope(GormRegistry.groovy:546)
at
org.grails.datastore.gorm.GormRegistry.withConnectionScope(GormRegistry.groovy:531)
at
grails.gorm.transactions.GrailsTransactionTemplate.inConnectionScope(GrailsTransactionTemplate.groovy:96)
at
grails.gorm.transactions.GrailsTransactionTemplate.executeAndRollback(GrailsTransactionTemplate.groovy:100)
```
The cause is `190ac9c388`, not this push: both example modules pass on
`9e0312eb15` and fail from `190ac9c388` on, 1 of 2 in each, reproduced locally
on Hibernate 5 and 7. The test annotates a method `@Rollback("moreBooks")`,
where `moreBooks` is a schema tenant added with `addTenantForSchema`, and
asserts that `Book.count()` inside it, with no tenant set, throws
`TenantNotFoundException`. In SCHEMA and DATABASE mode a tenant is a child
datastore registered as a connection, and `allQualifiers` maps every
multi-tenant entity to every connection, so the transform's connection reaches
`GrailsTransactionTemplate`, `runInScope(null, 'moreBooks')` is pushed,
`isMappedToConnection(Book, 'moreBooks')` is true, and `Book.count()` is routed
to the tenant. Nothing throws, and the test's second half, which then sets the
tenant and expects one book, is never reached.
This is a contract change for every existing multi-tenant application, and
it is not in the commit, the description, or any doc. Until `190ac9c388`, the
`connection` of `@Transactional` and `@Rollback` chose which datastore's
transaction manager opens the transaction, and the tenant resolver still
decided which tenant the calls inside reached, throwing when it found none. Now
the annotation is also a tenant switch, and the safety net inside such a method
is gone: `@Transactional("moreBooks")` around code that was written to run for
the current tenant reads and writes `moreBooks` regardless of who is logged in.
The class-level `inConnectionScope` behaves the same way for
`Book.moreBooks.withTransaction { }`, and the guide documents that ("the
block's connection takes precedence over the current tenant"), but that is a
block that names the tenant on the class; the annotation was never documented
as one.
Two ways out:
- Keep tenants out of the connection-wide scope: in `isMappedToConnection`,
a multi-tenant entity whose datastore is in DATABASE or SCHEMA mode is not
"mapped" to a qualifier that is one of its tenants, so the annotation keeps
choosing the transaction manager only, as before, and the tenant resolver keeps
deciding. The example tests then pass unchanged, and nothing in existing
applications moves. This is the one I would take: it is what the annotation has
always meant, and the `@Transactional(connection)` change was described as
covering the classes mapped to a *datasource*.
- Or keep the new behaviour, in which case the two example specs need
changing to whatever the new contract is, the upgrade guide's section 72 and
the multi-tenancy pages need a paragraph saying that `@Transactional` and
`@Rollback` naming a SCHEMA or DATABASE tenant now route every multi-tenant
class to it and no longer throw `TenantNotFoundException` inside, and
`ConnectionScopeMultiTenancySpec` needs the case. I would not do this on a
maintenance branch.
Either way, the description's "7,490 tests on the seven multi-datasource
example apps" did not include the two schema-per-tenant examples;
`grails-test-examples/hibernate5` and `hibernate7` have more multi-tenant apps
than those seven, and the Hibernate functional CI jobs are the run that covers
them. For what it is worth, round 2 saw this mechanism ("a tenant child
datastore's `TransactionService` pushes the tenant's connection ... so it
routes", under Confirmed) and should have called the contract change out then;
I did not, and the CI run did.
### Nit: `withNewSession(tenantId)` now opens two sessions in DATABASE mode
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy:1048-1061`
-
`grails-datamapping-core/src/main/groovy/grails/gorm/multitenancy/Tenants.groovy:302-372`
The fix binds the tenant through `Tenants.withId(datastore, tenantId,
inNewSession)`. In a shared-connection mode that only binds and calls the
closure, which is what is wanted. In DATABASE mode, with no session bound for
the tenant, `Tenants.withId` runs `datastore.withNewSession(tenantId)`, which
opens a session on the tenant's child datastore, and *then* `inNewSession`
opens the second one through `executeWithNewSession(tenantDatastore)`, which is
the session the closure receives; the outer one is never used.
`Tenants.withTenant(persistentClass, tenantId) { }` (or
`CurrentTenantHolder.withTenant(defaultDatastore, tenantId) { }`, which is what
it wraps) is the bind-only entry point that exists for exactly this, and gives
the commit what its message says, "with the tenant bound", without the extra
session. The new test runs on the in-memory datastore, where the extra session
costs nothing, so it does not show.
### Nit: two docs still say the default host is `localhost`
References:
-
`grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc:111`
("`localhost:27017` unless those are set")
- `grails-data-mongodb/docs/src/docs/asciidoc/testing.adoc:25` ("by default,
`localhost` and port `27017`")
`077f33c2c9` corrects the metadata to `127.0.0.1`, which is what
`ServerAddress.defaultHost()` returns and what `MongoConfigurationMetadataSpec`
now pins. These two sentences state the old value; the first was written in
this PR's own upgrade note.
### Confirmed
- `077f33c2c9`: `AbstractMongoConnectionSourceSettings` has `host =
ServerAddress.defaultHost()`, `port = ServerAddress.defaultPort()` and
`decimalType = true`, so the three metadata corrections are right, and the spec
compares the published defaults with a `new MongoConnectionSourceSettings()`
for all six settings and the url, so the next drift fails.
- `12497cfb51`: error code 86 is `IndexKeySpecsConflict`;
`findIndexByKeyPattern` returns nothing for it, so the name lookup finds the
index that holds the name, the TTL shortcut is skipped for it (it is a
different index), `recreateOnConflict` drops it by name and creates the
declared one, and without the flag the new message names the taken name and the
build counts one failure. Both cases are tested against a real server with an
index pre-created on other keys.
- `fdd67c9244`: the new constructor builds the first client from the
supplier with `closeable = true`, keeps the supplier, and
`createReplacementDefaultClient` prefers it over the configuration and the
options builder; the auto-configuration passes `{
MongoClients.create(mongoOptions) }` where it used to create the client itself,
and the `DisposableBean` that closed that client is gone because
`MongoDatastore.close()` now owns it. The rewritten `CloseSpec` builds a real
context with a 50 ms server-selection timeout and no server, and distinguishes
closed from open by the driver's `IllegalStateException` versus
`MongoTimeoutException`, so the four boot-plugin tests need no MongoDB. The
limitation the description listed for this case is now removed from the code
but is still listed under "Limitations" in the PR description; that line should
go.
- `07f68726a4`: the new test in `ConnectionScopeMultiTenancySpec` reads
tenant `bar` through `withNewSession('bar')` while `foo` is current, which
failed before. `withId` two methods up has used
`Tenants.withId(defaultDatastore, ...)` in the same way for years, so a
datastore in mode NONE handles the call as it always has (Hibernate falls back
to a plain new session, MongoDB to itself).
- `c82dd69b25`: the guard skips the bound-session selector only while a
scope is running, and only that selector; the qualified and default selectors
are untouched. It does not change DATABASE- or SCHEMA-mode tenant routing,
because `shouldSkipActiveDatastore` already ignores a bound tenant session
unless it matches the resolved tenant. On Hibernate it changes nothing either:
with `Author` mapped `ALL`, `Book.books.withTransaction { new Author(name:
n).save(flush: true) }` gives `No Session found for current thread` standalone
and `TransactionRequiredException` under an outer default session on both
`c82dd69b25` and the base, so the change is MongoDB's and Neo4j's, where a
class declared for the block's connection followed the block's session; the new
`BystanderCompany` test covers it. One consequence worth knowing: a session
opened on a datastore *itself* inside a block,
`datastore.getDatastoreForConnection('test2').withSession { Bystander.count()
}` nested in `CompanyA.withConne
ction('test2') { }`, no longer routes `Bystander` to `test2` either.
- All five modules' `codeStyle` and the docs build were not run here beyond
the three `codeStyle` tasks above; the RxGORM note is right,
`grails-datamapping-rx` is commented out of `settings.gradle`.
### Still open from round 2
- [P3] `TransactionService` on the default datastore undoes an enclosing
block while `@Transactional` without a connection does not
(`DefaultTransactionService.newTemplate` passing `DEFAULT`).
- [P3] "mapped to the connection" means "declared in the mapping", which the
MongoDB and Neo4j pages should say, and no MongoDB or Neo4j test covers the
annotation.
- Nit: the `default`-scope assertion in `ConnectionScopeMultiTenancySpec:66`
still never pushes a scope.
- Nit: the "left alone" branch of `inConnectionScope`
(`GormStaticApi:741-742`) still has no test.
## Verification
- `./gradlew --no-build-cache :grails-datamapping-core-test:cleanTest
:grails-datamapping-core-test:test :grails-datamapping-core:cleanTest
:grails-datamapping-core:test :grails-data-hibernate5-core:cleanTest
:grails-data-hibernate5-core:test --tests MultipleDataSourceConnectionsSpec
--tests Hibernate5RefreshLockSpec :grails-data-hibernate7-core:cleanTest
:grails-data-hibernate7-core:test --tests MultipleDataSourceConnectionsSpec
--tests Hibernate7RefreshLockSpec :grails-data-mongodb-core:cleanTest
:grails-data-mongodb-core:test --tests MultipleConnectionsSpec --tests
'*Index*' --tests MongoConfigurationMetadataSpec --tests
CheckpointRestoreConnectionsSpec --tests MongoDatastoreLifecycleSpec --tests
MongoDatastoreExternalClientSpec --tests SchemaBasedMultiTenancySpec --tests
'*Tenan*' :grails-data-mongodb-spring-boot:cleanTest
:grails-data-mongodb-spring-boot:test :grails-datamapping-core:codeStyle
:grails-data-mongodb-core:codeStyle :grails-data-mongodb-spring-boot:codeStyle`
on
`c82dd69b25`: BUILD SUCCESSFUL; 624 + 1 082 + 110 + 109 + 120 + 4 tests, 0
failures, 0 errors; result XML written 20:04-20:08 today.
- `./gradlew --no-build-cache --continue
:grails-test-examples-hibernate7-grails-schema-per-tenant:cleanTest
:grails-test-examples-hibernate7-grails-schema-per-tenant:test
:grails-test-examples-hibernate5-grails-schema-per-tenant:cleanTest
:grails-test-examples-hibernate5-grails-schema-per-tenant:test`: on
`c82dd69b25`, BUILD FAILED, `SchemaPerTenantSpec` 2 tests, 1 failure in each
module; on `9e0312eb15`, BUILD SUCCESSFUL, 2 tests, 0 failures in each.
- CI job `106862262476` log (`Hibernate7 Functional Tests (Java 21,
indy=false, shard 1)`): `> Task
:grails-test-examples-hibernate7-grails-schema-per-tenant:test FAILED`, the
stack trace quoted above, `2 tests completed, 1 failed`; the other three failed
shards carry the same annotation. TestLens's "All tests passed but jobs failed"
on this head is wrong about that job.
- Throwaway `BystanderProbeSpec` in `grails-data-hibernate7/core` (its own
`HibernateDatastore` over `Book` and `Author` from
`MultipleDataSourceConnectionsSpec`, H2), run on `c82dd69b25` and `1fadd5c812`,
printed identical results on both; deleted afterwards, and the worktree is back
on the original branch with no tracked changes.
--
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]
