matrei commented on PR #16369:
URL: https://github.com/apache/grails-core/pull/16369#issuecomment-5780716262
## Review Findings, round 2
Head `190ac9c388`, base `8.0.x` at `1fadd5c812`; the branch still merges
clean against the current `8.0.x` (`848d29e0ce`). This round covers everything
since `076ff09324`: the #16368 review changes (`b34471f68c`, `5e5b35a599`,
merged in through `9e0312eb15`), this PR's review changes (`2d8770e5cd`), and
the new `@Transactional(connection)` routing (`190ac9c388`). CI on the head was
still running when I wrote this (4 passed, 67 pending, none failed); the
earlier `Update Release Draft` non-green on `9e0312eb15` was *cancelled* with
no steps run, which is what that workflow does on a fork PR.
What I ran on the head, all green (build cache off, `cleanTest` first,
result XML timestamps from this run):
- `:grails-datamapping-core-test:test`, the whole module: 623 tests, 0
failures, including the new `ConnectionScopeMultiTenancySpec` (2) and the ten
in `MultipleDataSourceSpec`.
- `:grails-datamapping-core:test`, the whole module (the transform lives
here): 1 082 tests, 0 failures.
- `:grails-data-hibernate5-core:test` for
`MultipleDataSourceConnectionsSpec` and `Hibernate5RefreshLockSpec`: 7 + 103
tests, 0 failures.
- `:grails-data-hibernate7-core:test` for
`MultipleDataSourceConnectionsSpec` and `Hibernate7RefreshLockSpec`: 7 + 102
tests, 0 failures.
- `:grails-data-mongodb-core:test --tests MultipleConnectionsSpec`: 8 tests,
0 failures, against a real container.
- `codeStyle` on `grails-datamapping-core` and `grails-data-mongodb-core`:
no violations.
- Two throwaway specs on `SimpleMapDatastore`, deleted afterwards: one for
the multi-tenant numbers under Confirmed and the two nits, and one run on both
`190ac9c388` and `9e0312eb15` for the before/after numbers in the two P3s.
Every finding from the previous rounds is addressed the way the replies
describe, the resolution change in `b34471f68c` is the simplification I
suggested, made correctly, and the `@Transactional(connection)` routing does
what the description says for the case it describes: a method under
`@Transactional(connection = 'books')`, or the `books` datastore's
`TransactionService`, now routes the classes mapped to `books` there. Nothing
blocks the merge. The two P3s are about what `190ac9c388` does beyond that
case, and both have a small fix.
### [P3] `TransactionService` on the *default* datastore now undoes an
enclosing block; `@Transactional` without a connection does not
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/DefaultTransactionService.groovy:128-135`
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:534-541`,
`:583-590`
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/transform/TransactionalTransform.groovy:389-391`
`newTemplate` sets `connectionName` to
`resolveDefaultConnectionSourceName(datastore)` for *every* datastore, and for
the root datastore that is `DEFAULT`. `runInScope(null, DEFAULT)` inside
another block pushes a connection-wide scope that `isMappedToConnection`
applies to every entity, so the root datastore's `TransactionService` now
undoes whatever block it is called from. The transform, by contrast, passes a
connection only when the annotation names one, so a plain `@Transactional`
method in the same position keeps the block's routing. On a
`SimpleMapDatastore` with `Player` mapped to `default` and `one`, one row in
`default` and two in `one`:
```groovy
Player.one.withTransaction { plainService.countPlayers() }
// @Transactional, no connection
// 9e0312eb15: 2 190ac9c388: 2
Player.one.withTransaction {
datastore.getService(TransactionService).withTransaction { Player.count() } }
// 9e0312eb15: 2 190ac9c388: 1
```
The commit message only claims the routing for "the `TransactionService` of
a named connection's datastore", so the default case looks like a side effect
rather than a decision, and it is not in any doc. It reaches application code
that injects `TransactionService` (the `StudentService` in both
`grails-data-service` examples does) and `DefaultGormDataFetcher`, which runs
every GraphQL fetch through `datastore.getService(TransactionService)`. The
generated data services are *not* affected: a `@Service(Player)` method inside
the same block read `default` on both heads, so that path already behaved this
way before `190ac9c388`.
The fix that keeps 8.0.x's behaviour for the common case is a one-liner in
`newTemplate`: pass the name only when it is not `DEFAULT`, so the root
datastore's `TransactionService` behaves like an unqualified `@Transactional`.
If the undo is wanted instead, the transform should pass `DEFAULT` for an
unqualified `@Transactional` too, and the guide should say that a
default-connection transaction resets the routing; but that is a bigger
behaviour change for every service, and I would not make it here. Either way,
the third new test in `MultipleDataSourceSpec` only exercises the two services
*outside* any block; add the nested form above so the choice is pinned.
### [P3] "Mapped to the connection" is narrower than what the namespace API
accepts, on every implementation but Hibernate
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:578-590`,
`:244-258`
-
`grails-data-mongodb/docs/src/docs/asciidoc/multipleDataSources/mongoClientSwitching.adoc:31`
-
`grails-data-neo4j/docs/src/docs/asciidoc/multipleDataSources/boltDriverSwitching.adoc:31`
`isMappedToConnection` reads `entityDatastores`, which holds the connections
an entity *declares* (`ALL` expanded). `getDatastoreDirect`, which every
`Book.one` lookup goes through, has a fallback the new check does not share:
any registered connection whose mapping context contains the entity. On
Hibernate the two agree, because an entity not declared for a datasource is not
in that datasource's `SessionFactory`. On MongoDB, Neo4j and the in-memory
datastore the child datastores share the parent's mapping context, so an entity
with no `connections` mapping is reachable, and storable, through every named
API, and the two block kinds now disagree about it. With `Coach` declared for
nothing, two rows in `default` and one saved through `Coach.one.save(...)`:
```groovy
Coach.one.count() == 1 // reachable
and stored there
Coach.one.withTransaction { Coach.count() } == 1 // the class's
own block routes it
@Transactional('one') ... { Coach.count() } == 2 // the
annotation leaves it on default
```
The commit's reasoning, "rather than send it somewhere it cannot be stored",
is right for Hibernate and wrong for the others, and the MongoDB and Neo4j
pages now promise routing "for every domain class mapped to `moreBooks`"
without saying what mapped means there. Two ways out, and I would take the
first:
- Say "declared in its `connections` mapping, `ALL` included" on the MongoDB
and Neo4j pages and in their upgrade notes, since a MongoDB or Neo4j entity
that uses a named connection without declaring it is exactly the reader who
will be surprised.
- Or make `isMappedToConnection` ask
`getDatastoreDirect(normalizedClassName, qualifier) != null`, so "mapped" means
"reachable through the namespace" on every implementation. The `Coach` test in
`MultipleDataSourceSpec` would then read `[1, 0]` on the in-memory datastore,
which is the consistent answer there, and Hibernate would be unchanged.
Either way, no MongoDB or Neo4j test covers the annotation although both
guides now claim it. `ScopedCompany` in the MongoDB `MultipleConnectionsSpec`
is declared for `test2`, so one `@Transactional(connection = 'test2')` service
with a `save()` and a `count()` would close that.
### Nit: the `default`-scope assertion in `ConnectionScopeMultiTenancySpec`
never pushes a scope
References:
-
`grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/ConnectionScopeMultiTenancySpec.groovy:63-69`
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormRegistry.groovy:536-539`
```groovy
and: 'so does a scope for it, rather than the default connection itself'
GormRegistry.withConnectionScope(ScopedTenantBook, ConnectionSource.DEFAULT)
{ ScopedTenantBook.count() } == 2
```
`runInScope` for `DEFAULT` outside any block returns `callable.call()`
without touching the thread-local, so this assertion is
`ScopedTenantBook.count() == 2` under another name and does not reach the
`scopedConnection` branch that `b34471f68c` changed. It would have failed on
`8056ea18db`, which had no short-circuit, so the commit message is right about
that; but the case the label names only happens when `DEFAULT` is pushed
*inside* another block. On the head the nested form reads 2 (tenant `foo`), as
does the public `ScopedTenantBook.bar.withTransaction {
ScopedTenantBook.'default'.withTransaction { ScopedTenantBook.count() } }`,
which is what the javadoc's "undoes an enclosing one" promises for a tenant
entity. Either would do; the public form also tests through the API an
application calls.
### Nit: the "left alone" branch of `inConnectionScope` has no test
References:
-
`grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy:741-742`
The PR body and the javadoc both say a qualifier that is not a connection
name, such as a DISCRIMINATOR tenant id from `withTenant`, is left alone.
Codecov confirms these are the only lines of the scope code nothing reaches,
and no spec has a `withTenant(...).withTransaction { }` or `.withNewSession {
}`. On a DISCRIMINATOR-mode `SimpleMapDatastore` with tenant `foo` current (two
rows) and one row in `bar`, the head gives:
```groovy
ProbeDiscBook.withTenant('bar').count()
== 1
ProbeDiscBook.withTenant('bar').withTransaction { ProbeDiscBook.count() }
== 2 // current tenant, not bar
ProbeDiscBook.withTenant('bar').withNewSession { ProbeDiscBook.count() }
== 2
```
with the stack passing through `inConnectionScope`'s `callable.call()`. The
second and third lines are the test: add them to
`ConnectionScopeMultiTenancySpec` (a second datastore in DISCRIMINATOR mode,
`[ConnectionSource.DEFAULT]` as its connections, an entity with a `String
tenantId`) or to `PartitionMultiTenancySpec`, which has that setup already.
### Confirmed
- The scope stack. `scopedConnection` walks innermost first; a per-entity
entry matches by key, a connection-wide entry by `isMappedToConnection`, and a
connection-wide `DEFAULT` entry matches everything. `pop()` in `finally`
restores the outer state on exception, and the thread-local is removed when the
stack empties. The previous map's `previous`-restore is subsumed: a block for
`Book` on `books` inside a connection-wide `moreBooks` block, with `Book`
mapped to both, resolves `Book` to `books` and everything else to `moreBooks`.
`MultipleDataSourceSpec` still covers the `[1, 2]` nesting and the throw.
- The resolution change in `b34471f68c` is equivalent to the old code for
every case that worked and fixes the one that did not: a non-tenant entity ends
in the same `getDirect`, a tenant entity with a non-default scope hits Priority
1, which is the same `getDirect`, and a tenant entity with a `default` scope,
or none, hits the tenant resolution. The dropped fallback loses nothing,
because `AbstractGormApiRegistry.getDirect` returns `null` only when the entity
has no default API at all.
- On the DATABASE-mode probe (`foo` current with two rows, `bar` with one):
`withConnectionScope(Book, 'bar') { Book.count() }` is 1, the nested `bar {
default { Book.count() } }` is 2, and `Book.withTenant('bar').withTransaction {
Book.count() }` is 1, since a DATABASE tenant id is a connection name and the
tenant's block scopes it, which is the guide's "takes precedence over the
current tenant". `ConnectionScopeMultiTenancySpec`'s second feature covers the
#16368 P3 as now documented.
- The transform passes the annotation's `connection`, or `value`, member
cast to `String` only when one is present; the new three-argument
`GrailsTransactionTemplate` constructor stores it and the old ones delegate
with `null`; `execute` and `executeAndRollback` are both wrapped, so
`@Rollback` follows. A `value` that is a transaction-manager qualifier rather
than a connection name pushes a scope that nothing matches, which is harmless.
- `ALL` is expanded into every connection name by
`GormEnhancer.allQualifiers` before `registerEntityDatastores` runs, so
`isMappedToConnection` is true for each; the Hibernate 5 and 7 `Author` in the
new tests is `datasource 'ALL'`, and its `saveAuthor` under
`@Transactional(connection = "books")` lands in `books` and not in the default
database on both.
- Multi-tenant entities: in DATABASE mode a tenant child datastore's
`TransactionService` pushes the tenant's connection, which the entity resolves
to a child for, so it routes; in DISCRIMINATOR mode tenant qualifiers are
skipped at registration, so no scope applies and nothing changes.
- The merge `9e0312eb15` resolved its three conflicts correctly: `git diff
9e0312eb15^2 9e0312eb15` is exactly this PR's own changes.
- The new `withSession` test in `MultipleDataSourceSpec` and the
`withStatelessSession` test in the MongoDB `MultipleConnectionsSpec`
(`MongoDatastore` is the one `StatelessDatastore` implementation) both exercise
the wrapper they name, and the two new `@Transactional('one')` tests and the
`TransactionService` test check both sides on the in-memory datastore.
- Docs: the guide and both Hibernate guides replace the paragraph that said
the annotation does not route with one that says it does, for the classes
mapped to the datasource, and "its default `DataSource`" for `ZipCode` is
`lookup`, the first mapped one. Section 72 has the silent case, the
cross-connection copy, the annotation and the `Book.'default'` way out. The
MongoDB and Neo4j guides, release notes and upgrade notes all mention the
annotation; the `Transactional.connection()` javadoc says the same. The two
Neo4j errors the release note quotes are real: `The event loop thread may not
be smaller than 1, but was %d.` in driver 4.4.13's `Config$ConfigBuilder` (the
note drops the trailing period) and `Cannot flush write operations without an
active transaction!` at `Neo4jSession.java:906`.
## 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
:grails-datamapping-core:codeStyle` on `190ac9c388`: BUILD SUCCESSFUL; 623 + 1
082 + 110 + 109 + 8 tests, 0 failures, 0 errors; result XML written 18:54-18:57
today.
- Codecov line data for `9e0312eb15` (`api.codecov.io` report endpoint): in
`GormStaticApi.groovy` the unhit lines in the scope code are 741 (partial) and
742 (miss).
- Throwaway `ScopeProbeSpec` (DATABASE- and DISCRIMINATOR-mode datastores,
`SystemPropertyTenantResolver`) on `9e0312eb15` and `ScopeProbe2Spec` (`ProbeP`
mapped to `default` and `one`, `ProbeC` unmapped, a `@Transactional` service, a
`@Service(ProbeP)` data service, `TransactionService` of the root and of `one`)
on both `190ac9c388` and `9e0312eb15` produced the numbers quoted above; both
deleted, and the worktree is back on the original branch with no tracked
changes.
- `gh api repos/apache/grails-core/actions/jobs/106810528343`:
`"conclusion":"cancelled"`, `"steps":[]`.
--
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]
