matrei commented on PR #16369:
URL: https://github.com/apache/grails-core/pull/16369#issuecomment-5774081372
## Review Findings
Head `076ff09324`, base `8.0.x` at `1fadd5c812`; the branch sits directly on
the base and merges clean. The first six commits are #16368 (reviewed
separately); this review is the last commit only, diffed against `8056ea18db`.
81 of 82 CI checks pass; the one failure, `Build Grails-Core (macOS JDK 21)`,
is `:grails-doc:publishGuide` dying with `Java heap space` after 3h58m with
every test before it green (78 465 executed per TestLens), so it is the runner,
not this change.
What I ran on the head, all green (build cache off, `cleanTest` first,
result XML timestamps checked):
- `:grails-datamapping-core-test:test`, the whole module: 616 tests, 0
failures.
- `:grails-data-hibernate5-core:test` for
`MultipleDataSourceConnectionsSpec` and `Hibernate5RefreshLockSpec`: 6 + 103
tests, 0 failures.
- `:grails-data-hibernate7-core:test` for
`MultipleDataSourceConnectionsSpec` and `Hibernate7RefreshLockSpec`: 6 + 102
tests, 0 failures.
- `:grails-data-neo4j-core:test --tests NamedConnectionsSpec`: 3 tests, 0
failures.
- `:grails-data-mongodb-core:test --tests MultipleConnectionsSpec`: 7 tests,
0 failures, against a real container.
- `codeStyle` on `grails-datamapping-core`, `grails-data-hibernate5-core`
and `grails-data-hibernate7-core`: no violations.
- A throwaway spec on a DATABASE-mode `SimpleMapDatastore` (tenants `foo`
and `bar` are also its connections), deleted afterwards, for the multi-tenant
behaviour described under Confirmed.
The change is correct and I found nothing that blocks it. The design
decision, that a session or transaction opened through `Book.secondary`
reroutes `Book`'s own calls for its whole block, is the right one: it is what
the guide already implied for `withConnection`, it is what makes `book.save()`
inside `Book.secondary.withTransaction { }` reach the transaction that is
actually open, and the two ways out (`Book.'default'` and any other named API)
are both tested. The findings are about how the change is described and one gap
it leaves.
### [P3] The upgrade note says the old save always threw; in an application
it usually did not
References:
- `grails-doc/src/en/guide/upgrading/upgrading80x.adoc:4045-4053`
-
`grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc:115`
> With only the named connection's session open, a save failed with `No
Session found for current thread`
That is the standalone case. In a Grails web request the
open-session-in-view interceptor keeps a default-connection session open, and
in a service an outer `@Transactional` does the same, so
`Book.secondary.withTransaction { book.save() }` did not throw there: it wrote
`book` to the *default* database, silently. The two refresh-lock specs this PR
had to change show the same thing in miniature (`HibernateGormDatastoreSpec`
keeps a default session open, and the old
`Hibernate7RefreshLockRoutedBook.withSession { }` inside the secondary block
found it). That silent case is the one an upgrading application needs to hear
about, because a cross-datasource copy written as
```groovy
Book.legacy.withTransaction {
Book.legacy.list().each { new Book(name: it.name).save() } // meant
for the default database
}
```
worked before and now writes back into `legacy`. Suggested wording for the
guide's section 72, after the code block: "Whether the save threw or not
depended on what else was open: with a default-connection session open further
out, as the open-session-in-view interceptor or an outer `@Transactional`
provides, it succeeded and wrote the instance to the default database." The
MongoDB upgrade note already says "written to the default database", which is
right for MongoDB since it needs no session; the Neo4j note is fine as is.
### [P3] `@Transactional(connection = 'books')` opens the same transaction
and does not scope
References:
-
`grails-doc/src/en/guide/conf/dataSource/multipleDatasources.adoc:201-212`,
`:309-320`
After this PR there are two ways to open a transaction on `books`, and they
route `Book`'s own calls differently: `Book.books.withTransaction { new
Author(...).save() }` now writes to `books`, while a hand-written method under
`@Transactional(connection = 'books')` (or `TransactionService.withTransaction`
on the `books` datastore) still sends the same `save()` to the default
connection, because `TransactionalTransform` opens the transaction through the
transaction manager and never enters a `withConnectionScope`. The guide's Data
Service section promises routing for the generated methods and for the static
API of an entity *mapped* to `books`, so nothing documented breaks, but a
reader of the new "covers the whole block" paragraph will reasonably expect the
annotation form to behave the same. Either a sentence in that paragraph saying
the routing is what the *namespace's* `withTransaction`/`withSession` provide
and `@Transactional(connection)` does not do it, or a follow-up issue
to give the annotation the same scope. I would not hold this PR for the
latter; it is a bigger change and the annotation has been consistent with
itself for years.
### Nit: the scope stops at the class
References:
- `grails-doc/src/en/guide/conf/dataSource/multipleDatasources.adoc:201`
-
`grails-data-hibernate5/docs/src/docs/asciidoc/multipleDataSources/dataSourceNamespaces.adoc:46`
-
`grails-data-hibernate7/docs/src/docs/asciidoc/multipleDataSources/dataSourceNamespaces.adoc:46`
"A session or transaction opened through a namespace covers the whole block"
reads as if everything in the block is on `auditing`. The scope is per entity,
so `new AuditEntry(...).save()` inside `ZipCode.auditing.withTransaction { }`
still goes to `AuditEntry`'s own connection, and on Hibernate with nothing else
open still throws. The next sentence does say "the calls on `ZipCode` itself",
so it is recoverable; a half-sentence, "other domain classes are not affected",
would close it.
### Nit: `withSession` and `withStatelessSession` are wrapped but not covered
The new tests exercise `withNewSession`, `withTransaction` and
`withNewTransaction` on every implementation. `withSession` is only reached
through the `'default'` undo path in the refresh-lock specs, and
`withStatelessSession` (`GormStaticApi.groovy:849`) not at all, which is where
Codecov's remaining partials in `GormStaticApi` come from. One
`Player.one.withSession { Player.count() }` and, on a `StatelessDatastore`, the
stateless twin in `MultipleDataSourceSpec` would cover both.
### Confirmed
- `inConnectionScope` treats a `null` or `DEFAULT` qualifier as the default
connection, a qualifier that is a connection source name as a scope for it, and
anything else (a DISCRIMINATOR tenant id from `withTenant`) as no scope.
`getQualifier()` is virtual, so the Hibernate 5 and 7 static APIs, which keep
their own `qualifier` field and derive the first mapped datasource from the
mapping when none was given, feed the right name in: for the H7 `Book` mapped
to `['books', 'moreBooks']`, `Book.withTransaction { }` scopes `books`, which
is where its default-registered API already pointed.
- Every session/transaction entry point on `GormStaticApi` goes through the
scope: `withSession`, `withTransaction(Closure)`,
`withTransaction(TransactionDefinition)` (and so the `Map` and
`withNewTransaction` variants that build a definition and call it),
`withNewSession`, `withStatelessSession`. The Hibernate 5 and 7 overrides of
`withSession`/`withNewSession` wrap their bodies; neither module overrides the
transaction methods, and MongoDB and Neo4j override none of them.
`withDatastoreSession` is deliberately left alone and is not a user-facing
block. The tenant entry points (`withId`, `withNewSession(tenantId, ...)`) are
outside this change's stated scope.
- `withConnectionScope` for `DEFAULT`: outside any block it returns
`callable.call()` without touching the thread-local; inside one it pushes
`DEFAULT`, which `scopedConnection` reports as `null`, so the ordinary
resolution runs, and the outer name is restored afterwards.
`MultipleDataSourceSpec` covers the `[1, 2]` nesting;
`Hibernate7RefreshLockSpec` covers `'default'.withSession` inside a `secondary`
session on a real second database.
- Because a `DEFAULT` scope now means "ordinary resolution" rather than "the
literal default API", the #16368 P2 disagreement for a `MultiTenant` entity is
gone for the default case: on the DATABASE-mode probe with tenant `foo` bound,
`ProbeBook.bar.withTransaction { ProbeBook.'default'.withTransaction {
ProbeBook.count() } }` returns `foo`'s count, the same as
`ProbeBook.'default'.count()` would. The remaining `getDirect` short-circuit
for a non-default scope is identical to Priority 1 of the tenant resolution, so
the simplification suggested there is now only a simplification.
- On the same probe, a tenant's own API scopes the tenant's connection:
`ProbeBook.bar.withNewSession { ProbeBook.count() }`,
`ProbeBook.bar.withTransaction { ... }` and
`ProbeBook.withTenant('bar').withTransaction { ProbeBook.count() }` all read
`bar`, and a save inside `ProbeBook.bar.withTransaction { }` lands in `bar`. A
`Tenants.withId(datastore, 'foo')` nested inside the `bar` block still reads
`bar` (the #16368 P3, unchanged by this commit).
- `scopedConnection` lookups use `getDirect`, which materialises a
not-yet-allocated connection API on demand
(`AbstractGormApiRegistry.getDirect`), so an `ALL`-mapped entity whose `books`
instance API was never touched before the block still routes `save()` to
`books`; the H5/H7 `Author` tests are exactly that case.
- The refresh-lock spec changes preserve what the assertions check:
`'default'.withSession` inside the secondary session still lands on the default
connection's open session, and the H7 assertion that the secondary row is
absent from the default database is unchanged.
- Docs: the guide, both Hibernate guides, the MongoDB and Neo4j guides, the
MongoDB and Neo4j release and upgrade notes and section 72 of the Grails 8
upgrade guide all describe the change; none references `inConnectionScope`,
`withConnectionScope` or any other internal.
## Verification
- `./gradlew --no-build-cache :grails-datamapping-core-test:cleanTest
:grails-datamapping-core-test:test`: 616 tests, 0 failures (a 617th, my probe,
ran in the same invocation and was deleted afterwards).
- `./gradlew --no-build-cache :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-neo4j-core:cleanTest
:grails-data-neo4j-core:test --tests NamedConnectionsSpec
:grails-data-mongodb-core:cleanTest :grails-data-mongodb-core:test --tests
MultipleConnectionsSpec`: BUILD SUCCESSFUL, result XML timestamps from this run.
- `./gradlew :grails-datamapping-core:codeStyle
:grails-data-hibernate5-core:codeStyle :grails-data-hibernate7-core:codeStyle`:
no violations.
- The macOS CI failure read from the job log: `Execution failed for task
':grails-doc:publishGuide'` > `Java heap space`, after `3219 actionable tasks:
3219 executed`.
- A throwaway `ScopeProbeSpec` in `grails-datamapping-core-test`
(DATABASE-mode `SimpleMapDatastore`, connections `default`/`foo`/`bar`,
`SystemPropertyTenantResolver`) produced the numbers quoted under Confirmed,
then was deleted; the worktree is clean apart from the pre-existing untracked
review files.
--
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]
