From 8885e6f7a87acf3741bbc37fc574536c24cba231 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Wed, 29 Jul 2026 11:58:20 -0500 Subject: [PATCH] fix: preserve full transaction attribute state in GrailsTransactionAttribute 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 #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 --- .../GrailsTransactionAttribute.groovy | 43 ++- .../GrailsTransactionAttributeSpec.groovy | 288 ++++++++++++++++++ 2 files changed, 324 insertions(+), 7 deletions(-) create mode 100644 grails-datamapping-core/src/test/groovy/grails/gorm/transactions/GrailsTransactionAttributeSpec.groovy diff --git a/grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionAttribute.groovy b/grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionAttribute.groovy index 505392eddf3..63c987343cc 100644 --- a/grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionAttribute.groovy +++ b/grails-datamapping-core/src/main/groovy/grails/gorm/transactions/GrailsTransactionAttribute.groovy @@ -23,9 +23,11 @@ import groovy.transform.InheritConstructors import groovy.util.logging.Slf4j import org.springframework.transaction.TransactionDefinition +import org.springframework.transaction.interceptor.DefaultTransactionAttribute import org.springframework.transaction.interceptor.NoRollbackRuleAttribute import org.springframework.transaction.interceptor.RollbackRuleAttribute import org.springframework.transaction.interceptor.RuleBasedTransactionAttribute +import org.springframework.transaction.interceptor.TransactionAttribute /** * Used to configure a {@link GrailsTransactionTemplate} @@ -41,13 +43,8 @@ class GrailsTransactionAttribute extends RuleBasedTransactionAttribute { private static final long serialVersionUID = 1L private boolean inheritRollbackOnly = true - GrailsTransactionAttribute(org.springframework.transaction.interceptor.TransactionAttribute other) { - super() - propagationBehavior = other.propagationBehavior - isolationLevel = other.isolationLevel - timeout = other.timeout - readOnly = other.readOnly - name = other.name + GrailsTransactionAttribute(TransactionAttribute other) { + this((TransactionDefinition) other) } GrailsTransactionAttribute(TransactionDefinition other) { @@ -57,6 +54,15 @@ class GrailsTransactionAttribute extends RuleBasedTransactionAttribute { timeout = other.timeout readOnly = other.readOnly name = other.name + if (other instanceof TransactionAttribute) { + copyAttributeState((TransactionAttribute) other) + } + if (other instanceof RuleBasedTransactionAttribute) { + // Spring's copy constructor snapshots the source's rule list from the field, unlike + // getRollbackRules() which would lazily assign a new list into the source object + setRollbackRules(new RuleBasedTransactionAttribute((RuleBasedTransactionAttribute) other).getRollbackRules()) + } + copyGrailsState(other) } GrailsTransactionAttribute(GrailsTransactionAttribute other) { @@ -65,6 +71,29 @@ class GrailsTransactionAttribute extends RuleBasedTransactionAttribute { GrailsTransactionAttribute(RuleBasedTransactionAttribute other) { super(other) + copyAttributeState(other) + copyGrailsState(other) + } + + /** + * Copies the attribute-level state that Spring's copy constructors do not carry over. + * As of Spring Framework 7.0, {@code DefaultTransactionAttribute(TransactionAttribute)} + * only copies the {@link TransactionDefinition} fields. + */ + private void copyAttributeState(TransactionAttribute other) { + if (other instanceof DefaultTransactionAttribute) { + descriptor = ((DefaultTransactionAttribute) other).descriptor + timeoutString = ((DefaultTransactionAttribute) other).timeoutString + } + qualifier = other.qualifier + Collection otherLabels = other.labels + if (otherLabels != null) { + // defensive copy: setLabels stores the given reference + labels = new ArrayList(otherLabels) + } + } + + private void copyGrailsState(TransactionDefinition other) { if (other instanceof GrailsTransactionAttribute) { this.inheritRollbackOnly = ((GrailsTransactionAttribute) other).inheritRollbackOnly } diff --git a/grails-datamapping-core/src/test/groovy/grails/gorm/transactions/GrailsTransactionAttributeSpec.groovy b/grails-datamapping-core/src/test/groovy/grails/gorm/transactions/GrailsTransactionAttributeSpec.groovy new file mode 100644 index 00000000000..652cd94fe5d --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/grails/gorm/transactions/GrailsTransactionAttributeSpec.groovy @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.gorm.transactions + +import groovy.transform.CompileStatic + +import org.springframework.transaction.TransactionDefinition +import org.springframework.transaction.interceptor.DefaultTransactionAttribute +import org.springframework.transaction.interceptor.NoRollbackRuleAttribute +import org.springframework.transaction.interceptor.RollbackRuleAttribute +import org.springframework.transaction.interceptor.RuleBasedTransactionAttribute +import org.springframework.transaction.interceptor.TransactionAttribute +import org.springframework.transaction.support.DefaultTransactionDefinition +import spock.lang.Specification + +class GrailsTransactionAttributeSpec extends Specification { + + void "copy constructor deep-copies the rollback rule list instead of aliasing it"() { + given: + def sourceRules = [new RollbackRuleAttribute(IllegalStateException)] + def source = new GrailsTransactionAttribute() + source.setRollbackRules(sourceRules) + + when: + def copy = new GrailsTransactionAttribute(source) + copy.getRollbackRules().add(new NoRollbackRuleAttribute(IllegalArgumentException)) + + then: "mutating the copy's rule list does not affect the source's list" + source.getRollbackRules().size() == 1 + copy.getRollbackRules().size() == 2 + + when: "the source's rule list is mutated" + source.getRollbackRules().add(new RollbackRuleAttribute(UnsupportedOperationException)) + + then: "the copy's rule list is unaffected" + source.getRollbackRules().size() == 2 + copy.getRollbackRules().size() == 2 + + and: "copying did not replace the source's internal rule list" + source.getRollbackRules().is(sourceRules) + } + + void "copy constructor deep-copies the labels collection instead of aliasing it"() { + given: + def sourceLabels = ['audited'] + def source = new GrailsTransactionAttribute() + source.setLabels(sourceLabels) + + when: + def copy = new GrailsTransactionAttribute(source) + copy.getLabels().add('copy-only') + + then: "mutating the copy's labels does not affect the source's labels" + source.getLabels() as List == ['audited'] + copy.getLabels() as List == ['audited', 'copy-only'] + + when: "the source's labels are mutated" + sourceLabels.add('source-only') + + then: "the copy's labels are unaffected" + source.getLabels() as List == ['audited', 'source-only'] + copy.getLabels() as List == ['audited', 'copy-only'] + + and: "copying did not replace the source's internal labels collection" + source.getLabels().is(sourceLabels) + } + + void "copy constructor preserves qualifier, labels and inheritRollbackOnly metadata"() { + given: + def source = new GrailsTransactionAttribute() + source.setQualifier('secondary') + source.setLabels(['audited']) + source.setInheritRollbackOnly(false) + + when: + def copy = new GrailsTransactionAttribute(source) + + then: + copy.getQualifier() == 'secondary' + copy.getLabels() as Set == ['audited'] as Set + !copy.isInheritRollbackOnly() + } + + void "copy constructor preserves all definition-level properties"() { + given: + def source = new GrailsTransactionAttribute() + source.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW) + source.setIsolationLevel(TransactionDefinition.ISOLATION_SERIALIZABLE) + source.setTimeout(42) + source.setReadOnly(true) + source.setName('sourceTx') + + when: + def copy = new GrailsTransactionAttribute(source) + + then: + copy.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRES_NEW + copy.getIsolationLevel() == TransactionDefinition.ISOLATION_SERIALIZABLE + copy.getTimeout() == 42 + copy.isReadOnly() + copy.getName() == 'sourceTx' + } + + void "copy constructor preserves descriptor and timeoutString"() { + given: + def source = new GrailsTransactionAttribute() + source.setDescriptor('BookService.save') + source.setTimeoutString('${tx.timeout}') + + when: + def copy = new GrailsTransactionAttribute(source) + + then: + copy.getDescriptor() == 'BookService.save' + copy.getTimeoutString() == '${tx.timeout}' + } + + void "copy constructor from a plain RuleBasedTransactionAttribute copies its rollback rules defensively"() { + given: + def source = new RuleBasedTransactionAttribute() + source.setRollbackRules([new RollbackRuleAttribute(RuntimeException)]) + source.setQualifier('books') + source.setLabels(['audited']) + + when: + def copy = new GrailsTransactionAttribute(source) + + then: + copy.getRollbackRules().size() == 1 + !copy.getRollbackRules().is(source.getRollbackRules()) + copy.getQualifier() == 'books' + copy.getLabels() as List == ['audited'] + !copy.getLabels().is(source.getLabels()) + copy.isInheritRollbackOnly() + } + + void "a NoRollbackRuleAttribute on the source is honored by the copy's rollbackOn"() { + given: + def source = new RuleBasedTransactionAttribute() + source.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)]) + + when: + def copy = new GrailsTransactionAttribute(source) + + then: "a matching exception does not trigger rollback" + !copy.rollbackOn(new TestBusinessException()) + + and: "a non-matching exception still triggers the default rollback-everything behavior" + copy.rollbackOn(new IllegalStateException()) + copy.rollbackOn(new Exception()) + } + + void "rollbackOn applies the deepest matching rule"() { + given: + def attribute = new GrailsTransactionAttribute() + attribute.setRollbackRules([ + new NoRollbackRuleAttribute(RuntimeException), + new RollbackRuleAttribute(TestBusinessException) + ]) + + expect: "the rule closest to the thrown exception type wins" + attribute.rollbackOn(new TestBusinessException()) + !attribute.rollbackOn(new IllegalStateException()) + } + + void "rollbackOn rolls back on any exception when no rules are configured"() { + given: + def attribute = new GrailsTransactionAttribute() + + expect: "unchecked and checked exceptions both roll back, unlike Spring's default" + attribute.rollbackOn(new RuntimeException()) + attribute.rollbackOn(new Exception()) + attribute.rollbackOn(new Error()) + } + + void "statically dispatched TransactionDefinition copy still propagates rules and Grails state from the dynamic type"() { + given: "a GrailsTransactionAttribute passed around as a plain TransactionDefinition" + def rules = [new NoRollbackRuleAttribute(TestBusinessException)] + def source = new GrailsTransactionAttribute() + source.setRollbackRules(rules) + source.setInheritRollbackOnly(false) + source.setQualifier('books') + source.setLabels(['audited']) + + when: "copied through the statically chosen TransactionDefinition constructor" + def copy = copyAsTransactionDefinition(source) + + then: + !copy.rollbackOn(new TestBusinessException()) + !copy.isInheritRollbackOnly() + copy.getQualifier() == 'books' + copy.getLabels() as List == ['audited'] + + and: "the copy's rule list is independent of the source's" + !copy.getRollbackRules().is(source.getRollbackRules()) + + and: "the source's internal rule list was not replaced" + source.getRollbackRules().is(rules) + } + + void "statically dispatched TransactionAttribute copy still propagates rules from the dynamic type"() { + given: "a RuleBasedTransactionAttribute passed around as a plain TransactionAttribute" + def source = new RuleBasedTransactionAttribute() + source.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)]) + + when: "copied through the statically chosen TransactionAttribute constructor" + def copy = copyAsTransactionAttribute(source) + + then: + !copy.rollbackOn(new TestBusinessException()) + !copy.getRollbackRules().is(source.getRollbackRules()) + } + + void "copy constructor from a non-rule-based DefaultTransactionAttribute copies qualifier and labels defensively"() { + given: + def sourceLabels = ['audited'] + def source = new DefaultTransactionAttribute() + source.setQualifier('books') + source.setLabels(sourceLabels) + source.setTimeout(21) + + when: + def copy = new GrailsTransactionAttribute((TransactionAttribute) source) + copy.getLabels().add('copy-only') + + then: + copy.getQualifier() == 'books' + copy.getTimeout() == 21 + source.getLabels() as List == ['audited'] + source.getLabels().is(sourceLabels) + } + + void "copy constructor from a plain TransactionDefinition copies only definition-level properties"() { + given: "a source that is neither a TransactionAttribute nor a RuleBasedTransactionAttribute" + TransactionDefinition source = new DefaultTransactionDefinition().tap { + propagationBehavior = TransactionDefinition.PROPAGATION_REQUIRES_NEW + isolationLevel = TransactionDefinition.ISOLATION_SERIALIZABLE + timeout = 42 + readOnly = true + name = 'plainDefinition' + } + + when: + def copy = new GrailsTransactionAttribute(source) + + then: "the base TransactionDefinition properties are copied" + copy.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRES_NEW + copy.getIsolationLevel() == TransactionDefinition.ISOLATION_SERIALIZABLE + copy.getTimeout() == 42 + copy.isReadOnly() + copy.getName() == 'plainDefinition' + + and: "attribute-only metadata that TransactionDefinition doesn't expose is left at its default" + copy.getQualifier() == null + !copy.getLabels() + !copy.getRollbackRules() + copy.isInheritRollbackOnly() + } + + @CompileStatic + private static GrailsTransactionAttribute copyAsTransactionDefinition(TransactionDefinition source) { + return new GrailsTransactionAttribute(source) + } + + @CompileStatic + private static GrailsTransactionAttribute copyAsTransactionAttribute(TransactionAttribute source) { + return new GrailsTransactionAttribute(source) + } + + static class TestBusinessException extends RuntimeException { + } +}