Skip to content

fix: preserve full transaction attribute state in GrailsTransactionAttribute (GORM) copy constructors - #16064

Open
borinquenkid wants to merge 1 commit into
apache:8.0.xfrom
borinquenkid:fix/gorm-transaction-attribute-copy
Open

fix: preserve full transaction attribute state in GrailsTransactionAttribute (GORM) copy constructors#16064
borinquenkid wants to merge 1 commit into
apache:8.0.xfrom
borinquenkid:fix/gorm-transaction-attribute-copy

Conversation

@borinquenkid

@borinquenkid borinquenkid commented Jul 29, 2026

Copy link
Copy Markdown
Member

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 in org.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 by GrailsTransactionTemplate) had lossy copy constructors:

  • The (TransactionAttribute) overload copied only the five TransactionDefinition fields (propagation, isolation, timeout, readOnly, name) — rollback rules, qualifier, labels, descriptor, and timeoutString were silently dropped.
  • The (TransactionDefinition) overload had the same gap.
  • The (RuleBasedTransactionAttribute) overload called super(other) (so definition fields and rules were copied) but still dropped qualifier, labels, descriptor, and timeoutString.

In practice, a NoRollbackRuleAttribute configured on an attribute passed into GrailsTransactionTemplate was ignored: every exception rolled back regardless of the configured rule.

Fix

Same pattern as the CRTA fix (4042a87d54), adapted to this class's @CompileStatic Groovy source:

  • (TransactionAttribute) now delegates to (TransactionDefinition).
  • (TransactionDefinition) copies the five definition fields, then recovers the dynamic type via instanceof: copyAttributeState for TransactionAttribute sources, and a rollback-rules snapshot through a temporary new RuleBasedTransactionAttribute(other).getRollbackRules() — never the source's lazy getter, so the source is never mutated.
  • (RuleBasedTransactionAttribute) does super(other) + copyAttributeState + copyGrailsState.
  • New private helpers copyAttributeState (descriptor/timeoutString when source is DefaultTransactionAttribute, qualifier, defensive new ArrayList<String>(labels) copy) and copyGrailsState (inheritRollbackOnly, this class's only Grails-specific field).

No other production files touched — CustomizableRollbackTransactionAttribute and GrailsTransactionTemplate are unmodified.

Behavior change (release-note material)

Same shape as the CRTA change: an application that passes its own rule-bearing TransactionAttribute into GrailsTransactionTemplate will now have those rules honored — an exception matching a NoRollbackRuleAttribute commits 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 on CustomizableRollbackTransactionAttributeSpec): rule-list and labels deep-copy independence in both directions with explicit non-mutation-of-source assertions (source.getRollbackRules().is(originalList)), qualifier/labels/inheritRollbackOnly preservation, all five definition fields, descriptor/timeoutString, a plain RuleBasedTransactionAttribute source, rollback-on behavior (no-rollback rule honored, deepest rule wins, rollback-everything default), and the statically-dispatched TransactionDefinition/TransactionAttribute entry points via @CompileStatic private static helpers.

No end-to-end GrailsTransactionTemplate rules test is included here: GrailsTransactionTemplate's internal conversion to CustomizableRollbackTransactionAttribute is 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

Copilot AI review requested due to automatic review settings July 29, 2026 17:06
@borinquenkid borinquenkid mentioned this pull request Jul 29, 2026
Open
Copilot AI reviewed Jul 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 GrailsTransactionAttribute copy 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 rollbackOn behavior 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.

@borinquenkid borinquenkid mentioned this pull request Jul 29, 2026
Open
3 tasks
@borinquenkid

borinquenkid commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

The TestLens failures for GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default and GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer are a pre-existing bug on 8.0.x, unrelated to this PR's changes -- reproducible on plain 8.0.x today. DefaultStackTraceFilterer.STACK_LOG routes through a jcl-over-slf4j commons-logging binding, so the tests' System.setErr() capture never observes the emitted message.

Fix: #16067 (replaces the System.err capture with a Logback appender attached directly to the logger). Once that merges, rebasing onto 8.0.x should clear this failure here.

@borinquenkid borinquenkid moved this to Todo in Apache Grails Jul 30, 2026
@borinquenkid borinquenkid added this to the grails:8.0.0-RC2 milestone Jul 30, 2026
@borinquenkid

borinquenkid commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

The CI failures on this PR (Build Grails-Core failing on ubuntu-latest/macos-latest, "Rerunning all Tasks") are pre-existing on 8.0.x and unrelated to this change. Both failures are the same root cause:

  • GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer
  • GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default

Both assert on a System.err-backed ByteArrayOutputStream capture of the stack trace filterer's output, but 8.0.x now routes that output through a Logback appender instead of System.err, so the capture is empty. (Confirmed the same two tests fail identically on #16063 and #16065, which are also based on 8.0.x.)

This branch depends on #16067 (fix: capture STACK_LOG output via a Logback appender instead of System.err) merging into 8.0.x first. Once that lands, I'll rebase this branch onto the updated 8.0.x and CI should go green.

@borinquenkid borinquenkid mentioned this pull request Jul 30, 2026
Open
@borinquenkid @claude
fix: preserve full transaction attribute state in GrailsTransactionAt…
8885e6f 
…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>
@borinquenkid
borinquenkid force-pushed the fix/gorm-transaction-attribute-copy branch from 42faa33 to 8885e6f Compare July 31, 2026 01:22
@testlens-app

testlens-app Bot commented Jul 31, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 8885e6f
▶️ Tests: 57539 executed
⚪️ Checks: 60/60 completed

Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

Apache Grails
Status: Todo

Milestone

grails:8.0.0-RC2

Development

Successfully merging this pull request may close these issues.

2 participants

@borinquenkid