fix: preserve full transaction attribute state in GrailsTransactionAttribute (GORM) copy constructors - #16064
Conversation
…tribute copy constructors The TransactionAttribute and TransactionDefinition overloads previously copied only the five TransactionDefinition fields (propagation, isolation, timeout, readOnly, name), silently dropping rollback rules, qualifier, labels, descriptor and timeoutString. The RuleBasedTransactionAttribute overload delegated to super(other), which carries the rules but still loses the attribute-level state because Spring 7's DefaultTransactionAttribute(TransactionAttribute) copy constructor only copies the TransactionDefinition fields. The TransactionAttribute overload now delegates to the TransactionDefinition overload, which recovers the dynamic type via instanceof and snapshots rollback rules through a temporary RuleBasedTransactionAttribute copy - reading the source's rule field without invoking its lazy getRollbackRules(), which would mutate the source by assigning a new list into it. All paths now explicitly carry descriptor, timeoutString, qualifier and labels (defensively copied, since setLabels stores the given reference), plus inheritRollbackOnly when the source is a GrailsTransactionAttribute. Mirrors the CustomizableRollbackTransactionAttribute fix split out of the GormRegistry consolidation per review on apache#15779. Covered by GrailsTransactionAttributeSpec (copy independence and state preservation for every constructor dispatch path, including the statically dispatched TransactionDefinition/TransactionAttribute entries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚨 TestLens detected 12 failed tests 🚨Here is what you can do:
Test SummaryCI - Groovy Joint Validation Build / build_grails > :grails-core:test
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: 42faa33 Test Failures (first 10 of 12)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))GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default (: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
This PR fixes lossy copy-constructor behavior in grails.gorm.transactions.GrailsTransactionAttribute so that rule/metadata state (rollback rules, qualifier, labels, descriptor, timeoutString, and Grails-specific inheritRollbackOnly) is preserved when copying from TransactionDefinition / TransactionAttribute / RuleBasedTransactionAttribute. This aligns Grails’ programmatic transaction attribute handling with the intended rollback semantics when attributes are passed into GrailsTransactionTemplate.
Changes:
- Update
GrailsTransactionAttributecopy constructors to preserve full attribute state (including defensive copies for rollback rules and labels) without mutating the source. - Add focused Spock coverage validating deep-copy behavior, preservation of all relevant fields, and
rollbackOnbehavior with rule precedence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionAttribute.groovy | Fixes copy constructors to preserve rollback rules and attribute metadata (qualifier/labels/descriptor/timeoutString) and Grails-specific state. |
| grails-datamapping-core/src/test/groovy/grails/gorm/transactions/GrailsTransactionAttributeSpec.groovy | Adds comprehensive tests covering deep-copy independence, non-mutation of source state, and rollback rule behavior/precedence across constructor entry points. |
💡 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
grails.gorm.transactions.GrailsTransactionAttribute(the attribute type used byGrailsTransactionTemplate) had lossy copy constructors:(TransactionAttribute)overload copied only the fiveTransactionDefinitionfields (propagation, isolation, timeout, readOnly, name) — rollback rules, qualifier, labels, descriptor, andtimeoutStringwere silently dropped.(TransactionDefinition)overload had the same gap.(RuleBasedTransactionAttribute)overload calledsuper(other)(so definition fields and rules were copied) but still dropped qualifier, labels, descriptor, andtimeoutString.In practice, a
NoRollbackRuleAttributeconfigured on an attribute passed intoGrailsTransactionTemplatewas ignored: every exception rolled back regardless of the configured rule.Fix
Same pattern as the CRTA fix (
4042a87d54), adapted to this class's@CompileStaticGroovy source:(TransactionAttribute)now delegates to(TransactionDefinition).(TransactionDefinition)copies the five definition fields, then recovers the dynamic type viainstanceof: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<String>(labels)copy) andcopyGrailsState(inheritRollbackOnly, this class's only Grails-specific field).No other production files touched —
CustomizableRollbackTransactionAttributeandGrailsTransactionTemplateare unmodified.Behavior change (release-note material)
Same shape as the CRTA change: an application that passes its own rule-bearing
TransactionAttributeintoGrailsTransactionTemplatewill 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.Tests
GrailsTransactionAttributeSpec(13 tests, modeled onCustomizableRollbackTransactionAttributeSpec): rule-list and labels deep-copy independence in both directions with explicit non-mutation-of-source assertions (source.getRollbackRules().is(originalList)), 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 via@CompileStaticprivate static helpers.No end-to-end
GrailsTransactionTemplaterules test is included here:GrailsTransactionTemplate's internal conversion toCustomizableRollbackTransactionAttributeis fixed on the sibling CRTA branch, not this one, so an end-to-end test would depend on both PRs merging together. Verified separately with both fixes applied.:grails-datamapping-core:test— 306 tests, 0 failures.:grails-datamapping-core:codeStyle— clean.🤖 Generated with Claude Code