matrei commented on PR #16214:
URL: https://github.com/apache/grails-core/pull/16214#issuecomment-5703927219
# Review
**Head:** `f25990cdbc31fa975c6fa4181e04a3567729713e` (4 commits, merge of
`8.0.x` at `c1b532aef9`), **base:** `8.0.x`, mergeable/clean per GitHub.
**CI:** all 30 check runs green on the head SHA; codecov patch 82.35 % (3
uncovered lines).
**Open review thread:** jdaugherty asked for a What's New entry and
documentation of the behaviour change. No docs are in the PR yet.
## What was run locally
| What | Result |
|------|--------|
| `:grails-datastore-core:test` (`--no-build-cache`, `cleanTest`) | 152
tests, 0 failures |
| `:grails-data-simple:test` | 4 tests, 0 failures |
| `:grails-data-mongodb-core:test` | 673 tests, 0 failures, 45 skipped
(embedded replica set, no Docker) |
| `:grails-data-mongodb-spring-data:test` | 15 tests, 0 failures, incl. both
new `UnifiedMongoTransactionSpec` cases |
| `:grails-data-neo4j-core:compileJava/compileGroovy` | compiles against the
new `AbstractSession` |
| New tests against the merge-base sources (five main files swapped to
`c1b532aef9`) | all three new read-only cases fail (2 in
`grails-data-mongodb-core`, 1 in `grails-data-mongodb-spring-data`); the
read-write cases pass either way, as the PR body claims |
| Style checks on `grails-datastore-core`, `grails-data-mongodb-core`,
`grails-data-mongodb-spring-data` | `checkstyleMain/Test`, `codenarcMain/Test`,
`codeStyle` all pass on the three projects (no PMD/SpotBugs tasks exist on
them; the root `aggregateViolations` was not run) |
| Scratch probe (not committed): `withNewSession { save(); read-only tx;
count() }` then `count()` in a fresh session | flush mode `AUTO` → `COMMIT`
after the read-only commit; same-session `count()` = 0; after session close = 0
|
## Verdict
The code change is correct and small, and it does what the title says. What
blocks merging is documentation: this is a user-visible behaviour change on
every `DatastoreTransactionManager` datastore that overrides the new hook
(Mongo today), jdaugherty's request for docs is still open, and the repo's rule
is that user-facing changes do not merge without `grails-doc` coverage. Two
wording problems in the PR narrative also need fixing so the docs that get
written are accurate.
## Findings
### 1. Documentation is missing for a behaviour change (blocking)
The PR removes a write that applications on Mongo currently get. There is
nothing in `grails-doc/src/en/guide/introduction/whatsNew.adoc`, nothing in
`grails-doc/src/en/guide/upgrading/upgrading80x.adoc` (section *20. Other
Default Behavior Changes* is the natural home for a bullet), and nothing in
`grails-data-mongodb/docs/src/docs/asciidoc/transactions.adoc`, which still
opens with "MongoDB doesn't support transactions directly" and never mentions
read-only semantics.
The docs need to say precisely what happens to a write queued before a
read-only transaction, because it is not "left where it was" (see finding 2).
Verified with the scratch probe above and by reading the code:
- `DatastoreTransactionManager.doBegin`
(`grails-datastore-core/.../transactions/DatastoreTransactionManager.java:121-126`)
sets the session to `FlushModeType.COMMIT` for a read-only definition and
nothing ever restores it. `AbstractSession`'s default is `AUTO`
(`AbstractSession.java:109`), so after the read-only commit the same session no
longer auto-flushes before queries (`Query.java:760`), and
`OpenSessionInViewInterceptor.postHandle`
(`grails-datastore-web/.../OpenSessionInViewInterceptor.java:82`) skips its
end-of-request flush for the same reason.
- When the session closes, `AbstractAttributeStoringSession.disconnect()`
(`:112-116`) calls `clear()`, so the queued insert is discarded.
So the honest description is: *a write saved without `flush: true` before a
read-only transaction in the same session is no longer persisted by that
transaction, is invisible to later queries in that session, and is dropped when
the session ends. Use `save(flush: true)` or a read-write transaction.* This
matches Hibernate, where `GrailsHibernateTransactionManager.doBegin` sets
`MANUAL` and `GrailsOpenSessionInViewInterceptor.postHandle` skips
`MANUAL`/`COMMIT` sessions, so "the same meaning as on Hibernate" holds. It
just has to be written down, with the migration advice, because on Mongo it
used to work by accident.
The Spring Data limitation from the new `UnifiedMongoTransactionSpec` case
also belongs in the Mongo docs: on `GormSharedSessionMongoTransactionManager`,
`readOnly` only governs the GORM flush. A `MongoTemplate` write inside a
read-only transaction still commits, because it goes straight into the shared
`ClientSession`. The test enshrines that; users should not have to discover it
from a test name.
### 2. The PR narrative misstates the flush-mode default and the fate of the
queued write
- Commit `05fb17e722` and the PR body say `FlushModeType.COMMIT` is "already
the `Session` default". It is not; `AbstractSession.flushMode` initialises to
`AUTO` and `ConnectionSourceSettings.flushMode` is `AUTO` too. The new comment
inside `doBegin` is accurate ("keeps an AUTO session from flushing ahead of
queries"), so only the commit message and PR body need correcting. Worth doing
before squash, since the message becomes the permanent record.
- The `then:` labels in `MongoTransactionDisabledSpec.groovy:78` and
`MongoTransactionSpec.groovy:191` read "the queued write was left where it
was". It was left in a session that is now in `COMMIT` mode and will `clear()`
on close, so "left queued and discarded when the session closes" is what the
test actually demonstrates. Same for the `UnifiedMongoTransactionSpec` name,
which says "drops": that one is right, the other two should agree with it.
### 3.
`AbstractMongoSession.beginTransactionInternal(TransactionDefinition)` has no
null guard (nit)
`grails-data-mongodb/core/.../AbstractMongoSession.java:277` calls
`definition.isReadOnly()` unconditionally.
`Session.beginTransaction(TransactionDefinition)` carries no nullability
contract, the base `AbstractSession` default tolerates `null`, and
`HibernateSessionSpec` already exercises the interface with `null`. Both
in-tree callers (`DatastoreTransactionManager`,
`Neo4jDatastoreTransactionManager`) always pass a real definition, so this is
not reachable today, but `definition != null && definition.isReadOnly()` costs
nothing and keeps the override as forgiving as the default it replaces.
### 4. `SessionOnlyTransaction` gains a branch with no test in its own
module (nit)
The read-only branch in
`grails-datastore-core/.../SessionOnlyTransaction.java:70-72` is only covered
from `grails-data-mongodb-core` via `MongoTransactionDisabledSpec`. A
`SessionOnlyTransactionSpec` in `grails-datastore-core` with a `Mock(Session)`
asserting `0 * session.flush()` for `readOnly = true`, `1 * session.flush()`
otherwise, and `1 * session.clear()` on rollback would cover the class where it
lives, lock the contract the deprecation Javadoc promises to out-of-tree
datastores, and probably recover the codecov patch gap. The deprecated two-arg
constructor could be asserted to behave as read-write in the same spec.
### 5. Non-blocking observations
- `MongoTransaction.commit()` failure path (`MongoTransaction.java:92-113`)
still calls `session.clear()` when a read-only commit fails, which discards the
surrounding session's queued writes that the transaction never owned.
`DatastoreTransactionManager.doRollback` does the same on any rollback, and
Spring's Hibernate manager clears pre-bound sessions on rollback too, so this
is consistent and pre-existing. Mentioning it only because the new Javadoc says
a read-only transaction "has no pending operations of its own".
- `@Deprecated(since = "8.0", forRemoval = true)` matches the convention
used elsewhere in the tree. Removing `MongoTransaction(AbstractMongoSession,
ClientSession)` outright is fine: it was introduced in an 8.0.0 milestone
(#15744) and `AbstractMongoSession` is its only caller.
- Codecov's three uncovered lines are almost certainly the deprecated
`SessionOnlyTransaction` constructor and the no-arg
`AbstractMongoSession.beginTransactionInternal()`, which is now only reachable
via `Session.beginTransaction()` with no argument and has no in-tree caller for
Mongo. Finding 4 covers the first; the second is dead-ish but harmless.
## Verified as correct
- `AbstractSession.beginTransaction(TransactionDefinition)` now forwards the
definition, and the default `beginTransactionInternal(TransactionDefinition)`
delegates to the no-arg method, so `SimpleMapSession` is unchanged (its
`MockTransaction.commit()` never flushed; the read-write flush still comes from
`doCommit`). `Neo4jSession` overrides `beginTransaction(TransactionDefinition)`
directly and never enters this path. Both Hibernate sessions throw
`UnsupportedOperationException` for it regardless.
- No remaining callers of the removed `MongoTransaction` constructor or the
deprecated `SessionOnlyTransaction` constructor anywhere in the tree, including
`grails-data-neo4j`.
- Read-write commits are unchanged: `DatastoreTransactionManager.doCommit`
still flushes before `transaction.commit()`, so the flush inside
`MongoTransaction.commit()` / `SessionOnlyTransaction.commit()` remains a no-op
on that path and still covers callers that commit the `Transaction` directly.
The read-write tests in `MongoTransactionDisabledSpec` and
`UnifiedMongoTransactionSpec` assert this.
- The new tests genuinely test the change: with the five main files swapped
back to the merge-base, `MongoTransactionSpec`, `MongoTransactionDisabledSpec`
and `UnifiedMongoTransactionSpec` each fail exactly their read-only case and
nothing else.
- Read-only server-side transactions still open and commit a `ClientSession`
transaction; with nothing flushed into it the commit is empty, and
`commitWithRetry` / the failure cleanup are untouched.
- The Spring Data interop manager inherits the behaviour with no code
change, which the two new `UnifiedMongoTransactionSpec` cases pin down.
--
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]
