borinquenkid opened a new pull request, #16063: URL: https://github.com/apache/grails-core/pull/16063
# fix: preserve full transaction attribute state in CustomizableRollbackTransactionAttribute copy constructors Split out of the GormRegistry consolidation per review discussion on #15779 (this class's copy semantics are an independent bug fix and deserve their own review). ## Problem The copy constructors of `CustomizableRollbackTransactionAttribute` were lossy: - The `TransactionAttribute`/`TransactionDefinition` overloads copied only the five `TransactionDefinition` fields (propagation, isolation, timeout, readOnly, name) — **rollback rules, qualifier, labels, descriptor, and `timeoutString` were silently dropped**. In practice this meant a `NoRollbackRuleAttribute` configured on an attribute passed into `GrailsTransactionTemplate` or `TransactionService.withTransaction`/`withNewTransaction` was ignored: every exception rolled back regardless. - The `RuleBasedTransactionAttribute` overload copied almost **nothing** — not even the definition fields — because it never called `super(other)`. - The previous implementation's `copyFrom` helper also mutated the *source* object: calling `getRollbackRules()` on a `RuleBasedTransactionAttribute` lazily installs a new list into it, and `setLabels(other.getLabels())` aliased the label collection between source and copy. - Two trace log statements in this `.java` file used GString-style placeholders (`"$ex"`, `"$winner"`) that never interpolate in Java. ## Fix - The `RuleBasedTransactionAttribute`/`CustomizableRollbackTransactionAttribute` overloads now delegate to Spring's own copy constructor via `super(other)`, which snapshots the rule list from the field (never the lazily-materializing getter). - The `TransactionDefinition`/`TransactionAttribute` overloads copy the definition fields and then recover the dynamic type: rules are snapshotted through a temporary `RuleBasedTransactionAttribute` copy (so the source is never mutated), and attribute-level state is copied explicitly. - `descriptor`, `timeoutString`, and `qualifier` are carried over explicitly — as of Spring Framework 7, `DefaultTransactionAttribute(TransactionAttribute)` copies only the `TransactionDefinition` fields, so these would otherwise be lost. - `labels` gets a defensive copy (`setLabels` stores the given reference). - `connection` and `inheritRollbackOnly` survive every copy path when the source is a `CustomizableRollbackTransactionAttribute`. - Log placeholders fixed; the remaining unguarded trace call is now behind `isTraceEnabled()`. ## Behavior change (release-note material) `@Transactional` / `@ReadOnly` / `@Rollback` semantics are **unchanged** — the AST transform builds the attribute directly and never goes through these copy constructors. For **programmatic** API users: an application that passes its own rule-bearing `TransactionAttribute` (e.g. a `RuleBasedTransactionAttribute` with `NoRollbackRuleAttribute`s) into `DomainClass.withTransaction(definition)`, `TransactionService.withTransaction(definition)`, or `withNewTransaction(definition)` will now have those rules **honored**: an exception matching a no-rollback rule commits the transaction (the exception still propagates). Previously the rules were silently dropped and every exception rolled back. The change is one-directional — no scenario turns a commit into a rollback; with no matching rule the class still rolls back on every exception, checked or unchecked, as documented. Suggested upgrade note: > Rollback rules configured on a `TransactionAttribute` passed to the programmatic transaction APIs (`withTransaction`, `withNewTransaction`, `GrailsTransactionTemplate`) are now honored. Previously `noRollbackFor`-style rules supplied this way were silently ignored and every exception caused a rollback. ## Tests - `CustomizableRollbackTransactionAttributeSpec` (13 tests): deep-copy independence of rule lists and labels in both directions, non-mutation of the source's internal rule list, preservation of every definition- and attribute-level property across all four constructor dispatch paths (including statically-dispatched `TransactionDefinition`/`TransactionAttribute` entry points via `@CompileStatic` helpers), and the rollback-on-everything default. - `TransactionRollbackRulePropagationSpec` (4 tests): behavior through the public APIs — `GrailsTransactionTemplate.execute` and `DefaultTransactionService.withNewTransaction` against a real H2 `DataSourceTransactionManager` — asserting commit-vs-rollback outcomes and `REQUIRES_NEW` propagation. All fix-specific assertions fail against the previous implementation (verified — the tests are not vacuous). ## Known follow-up (out of scope) `grails.gorm.transactions.GrailsTransactionAttribute` (grails-datamapping-core) and `org.grails.transaction.GrailsTransactionAttribute` (grails-core) contain the same lossy copy-constructor pattern on their `TransactionAttribute`/`TransactionDefinition` overloads. Fixing them belongs in a separate change with its own tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
