fix: preserve full transaction attribute state in GrailsTransactionAttribute (web) copy constructors - #16065
Conversation
…tribute copy constructors Mirrors the CustomizableRollbackTransactionAttribute fix (4042a87) split out of PR apache#15779 review, applied to the web-tier twin in org.grails.transaction. The RuleBasedTransactionAttribute overload now delegates to Spring's own copy constructor (super(other)), which snapshots the rule list from the field without invoking the source's lazy getRollbackRules() (which would mutate the source by assigning a new list into it). The TransactionDefinition and TransactionAttribute overloads recover the dynamic type and snapshot rules through a temporary Spring copy, so the source is never mutated on any path. All paths now explicitly carry the attribute-level state that Spring 7's DefaultTransactionAttribute copy constructor does not: descriptor, timeoutString, qualifier, and labels (defensively copied, since setLabels stores the given reference), plus inheritRollbackOnly. Also fixes GString-style placeholders ("$ex", "$winner") in trace logging that never interpolated in this .java source, and guards the remaining trace call behind isTraceEnabled(). Covered by GrailsTransactionAttributeSpec (copy independence and state preservation for every constructor dispatch path, including statically dispatched entries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚨 TestLens detected 13 failed tests 🚨Here is what you can do:
Test SummaryCI - Groovy Joint Validation Build / build_grails > :grails-core:test
CI / Build Grails Forge (Java 21, indy=false) > :grails-forge-cli:test [grails-forge]
CI / Build Grails-Core (macos-latest, 21) > :grails-core:test
CI / Build Grails-Core (ubuntu-latest, 21) > :grails-core:test
CI / Build Grails-Core (ubuntu-latest, 25) > :grails-core:test
CI / Build Grails-Core (windows-latest, 25) > :grails-core:test
CI / Build Grails-Core Rerunning all Tasks (ubuntu-latest, 21) > :grails-core:test
🏷️ Commit: afe91b0 Test Failures (first 10 of 13)CreateControllerCommandSpec > test app with controller (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer (:grails-core:test in CI / Build Grails-Core (macos-latest, 21))GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default (:grails-core:test in CI / Build Grails-Core (macos-latest, 21))GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer (:grails-core:test in CI / Build Grails-Core (ubuntu-latest, 21))GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default (:grails-core:test in CI / Build Grails-Core (ubuntu-latest, 21))GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer (:grails-core:test in CI / Build Grails-Core (ubuntu-latest, 25))GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default (:grails-core:test in CI / Build Grails-Core (ubuntu-latest, 25))GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer (:grails-core:test in CI / Build Grails-Core (windows-latest, 25))GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default (:grails-core:test in CI / Build Grails-Core (windows-latest, 25))GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer (:grails-core:test in CI / Build Grails-Core Rerunning all Tasks (ubuntu-latest, 21))Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
There was a problem hiding this comment.
Pull request overview
Fixes lossy copy-constructor behavior in org.grails.transaction.GrailsTransactionAttribute so that copy construction preserves transaction attribute state beyond the five TransactionDefinition fields (notably rollback rules, qualifier/labels, descriptor, and timeoutString), matching the intent described in the PR metadata and aligning behavior with the earlier related fixes.
Changes:
- Make
GrailsTransactionAttribute(TransactionAttribute)delegate to theTransactionDefinitionconstructor and enhance theTransactionDefinitionconstructor to preserve attribute state (incl. rollback rules snapshotting without mutating the source). - Ensure the
RuleBasedTransactionAttributecopy path also preserves qualifier/labels/descriptor/timeoutString and Grails-specificinheritRollbackOnly. - Add a comprehensive Spock spec covering deep-copy independence, non-mutation of sources, definition/attribute state preservation, and
rollbackOnbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grails-core/src/main/groovy/org/grails/transaction/GrailsTransactionAttribute.java | Updates copy constructors to preserve full attribute state (rollback rules, qualifier/labels, descriptor/timeoutString, Grails state) and fixes/guards trace logging. |
| grails-core/src/test/groovy/org/grails/transaction/GrailsTransactionAttributeSpec.groovy | Adds targeted regression tests for copy semantics, defensive copying, and rollback rule behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The TestLens failures for Fix: #16067 (replaces the |
fix: preserve full transaction attribute state in GrailsTransactionAttribute copy constructors
Follow-up to #16063 (
fix/customizable-rollback-tx-attribute-copy), which fixed the same bug inorg.grails.datastore.mapping.transactions.CustomizableRollbackTransactionAttribute. That PR's "known follow-up" section named this class as carrying the identical pattern; this PR is that fix.Problem
org.grails.transaction.GrailsTransactionAttribute(grails-core) had the same lossy copy constructors as its GORM-module namesake and the CRTA class:(TransactionAttribute)overload copied only the fiveTransactionDefinitionfields — rollback rules, qualifier, labels, descriptor, andtimeoutStringwere silently dropped.(TransactionDefinition)overload had the same gap.(RuleBasedTransactionAttribute)overload calledsuper(other)(copying definition fields and rules) but still dropped qualifier, labels, descriptor, andtimeoutString."$ex","$winner") that never interpolate in.javasource, and a third trace statement logged unconditionally without anisTraceEnabled()guard.This class has no in-repo constructor callers —
grepingnew GrailsTransactionAttribute(across the whole repo turns up onlyGrailsTransactionTemplate.groovy:55, which resolves to the same-package GORM twin (grails.gorm.transactions.GrailsTransactionAttribute, fixed separately in #16064), not this class. This class is public API consumed by downstream plugins and applications that construct or extend it directly.Fix
Same pattern as the CRTA fix (
4042a87d54):(TransactionAttribute)now delegates to(TransactionDefinition).(TransactionDefinition)copies the five definition fields, then recovers the dynamic type via pattern-matchinginstanceof:copyAttributeStateforTransactionAttributesources, and a rollback-rules snapshot through a temporarynew RuleBasedTransactionAttribute(other).getRollbackRules()— never the source's lazy getter, so the source is never mutated.(RuleBasedTransactionAttribute)doessuper(other)+copyAttributeState+copyGrailsState.copyAttributeState(descriptor/timeoutString when source isDefaultTransactionAttribute, qualifier, defensivenew ArrayList<>(labels)copy) andcopyGrailsState(inheritRollbackOnly, this class's only Grails-specific field — it has noconnectionfield, unlike the CRTA class).isTraceEnabled().No other production files touched.
Behavior change (release-note material)
Same shape as the CRTA change: downstream code that passes its own rule-bearing
TransactionAttributeinto this class's copy constructors will now have those rules honored — an exception matching aNoRollbackRuleAttributecommits instead of rolling back. Previously the rules were silently dropped and every exception rolled back. One-directional: no scenario turns a commit into a rollback. Since this class has no in-repo callers, the practical impact is scoped to plugin/application code outside this repository.Tests
GrailsTransactionAttributeSpec(13 tests, modeled onCustomizableRollbackTransactionAttributeSpec): rule-list and labels deep-copy independence in both directions with explicit non-mutation-of-source assertions, qualifier/labels/inheritRollbackOnlypreservation, all five definition fields, descriptor/timeoutString, a plainRuleBasedTransactionAttributesource, rollback-on behavior (no-rollback rule honored, deepest rule wins, rollback-everything default), and the statically-dispatchedTransactionDefinition/TransactionAttributeentry points.:grails-core:test— 475 tests, 0 failures.:grails-core:codeStyle— clean.🤖 Generated with Claude Code