fix: preserve full transaction attribute state in CustomizableRollbackTransactionAttribute copy constructors

grails-datamapping-core/src/test/groovy/grails/gorm/transactions/TransactionRollbackRulePropagationSpec.groovy

@@ -0,0 +1,159 @@
+/*
+ *  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 javax.sql.DataSource
+
+import org.springframework.jdbc.datasource.DataSourceTransactionManager
+import org.springframework.jdbc.datasource.DriverManagerDataSource
+import org.springframework.transaction.TransactionDefinition
+import org.springframework.transaction.TransactionStatus
+import org.springframework.transaction.interceptor.NoRollbackRuleAttribute
+import org.springframework.transaction.interceptor.RuleBasedTransactionAttribute
+import org.springframework.transaction.support.DefaultTransactionStatus
+import spock.lang.Specification
+
+import org.grails.datastore.gorm.services.DefaultTransactionService
+import org.grails.datastore.mapping.transactions.TransactionCapableDatastore
+
+/**
+ * Verifies that rollback rules configured on a transaction definition survive the conversion
+ * to {@link org.grails.datastore.mapping.transactions.CustomizableRollbackTransactionAttribute}
+ * performed by the public transaction APIs.
+ */
+class TransactionRollbackRulePropagationSpec extends Specification {
+
+    RecordingTransactionManager transactionManager
+
+    void setup() {
+        def dataSource = new DriverManagerDataSource(
+                "jdbc:h2:mem:${TransactionRollbackRulePropagationSpec.name};LOCK_TIMEOUT=10000", 'sa', '')
+        dataSource.driverClassName = 'org.h2.Driver'
+        transactionManager = new RecordingTransactionManager(dataSource)
+    }
+
+    void "GrailsTransactionTemplate honors a NoRollbackRuleAttribute on the supplied transaction attribute"() {
+        given: "a rule based attribute that must not roll back on the business exception"
+        def attribute = new RuleBasedTransactionAttribute()
+        attribute.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)])
+        def template = new GrailsTransactionTemplate(transactionManager, attribute)
+
+        when: "the transactional closure throws the matching exception"
+        template.execute { TransactionStatus status ->
+            throw new TestBusinessException()
+        }
+
+        then: "the exception propagates but the transaction is committed, not rolled back"
+        thrown(TestBusinessException)
+        transactionManager.committed
+        !transactionManager.rolledBack
+    }
+
+    void "GrailsTransactionTemplate still rolls back when no rule matches the thrown exception"() {
+        given:
+        def attribute = new RuleBasedTransactionAttribute()
+        attribute.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)])
+        def template = new GrailsTransactionTemplate(transactionManager, attribute)
+
+        when:
+        template.execute { TransactionStatus status ->
+            throw new IllegalStateException('no rule matches this')
+        }
+
+        then:
+        thrown(IllegalStateException)
+        transactionManager.rolledBack
+        !transactionManager.committed
+    }
+
+    void "withNewTransaction honors rollback rules from a RuleBasedTransactionAttribute definition"() {
+        given:
+        def service = new DefaultTransactionService()
+        service.datastore = Stub(TransactionCapableDatastore) {
+            getTransactionManager() >> transactionManager
+        }
+        def definition = new RuleBasedTransactionAttribute()
+        definition.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)])
+
+        when: "the transactional closure throws the matching exception"
+        service.withNewTransaction((TransactionDefinition) definition) { TransactionStatus status ->
+            throw new TestBusinessException()
+        }
+
+        then: "the exception propagates but the transaction is committed, not rolled back"
+        thrown(TestBusinessException)
+        transactionManager.committed
+        !transactionManager.rolledBack
+
+        and: "the new transaction was started with REQUIRES_NEW propagation"
+        transactionManager.definition.propagationBehavior == TransactionDefinition.PROPAGATION_REQUIRES_NEW
+    }
+
+    void "withNewTransaction still rolls back when no rule matches the thrown exception"() {
+        given:
+        def service = new DefaultTransactionService()
+        service.datastore = Stub(TransactionCapableDatastore) {
+            getTransactionManager() >> transactionManager
+        }
+        def definition = new RuleBasedTransactionAttribute()
+        definition.setRollbackRules([new NoRollbackRuleAttribute(TestBusinessException)])
+
+        when:
+        service.withNewTransaction((TransactionDefinition) definition) { TransactionStatus status ->
+            throw new IllegalStateException('no rule matches this')
+        }
+
+        then:
+        thrown(IllegalStateException)
+        transactionManager.rolledBack
+        !transactionManager.committed
+    }
+
+    static class TestBusinessException extends RuntimeException {
+    }
+
+    static class RecordingTransactionManager extends DataSourceTransactionManager {
+
+        boolean committed = false
+        boolean rolledBack = false
+        TransactionDefinition definition
+
+        RecordingTransactionManager(DataSource dataSource) {
+            super(dataSource)
+        }
+
+        @Override
+        protected void doBegin(Object transaction, TransactionDefinition definition) {
+            this.definition = definition
+            super.doBegin(transaction, definition)
+        }
+
+        @Override
+        protected void doCommit(DefaultTransactionStatus status) {
+            committed = true
+            super.doCommit(status)
+        }
+
+        @Override
+        protected void doRollback(DefaultTransactionStatus status) {
+            rolledBack = true
+            super.doRollback(status)
+        }
+    }
+}

grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java

@@ -19,15 +19,19 @@
 
 package org.grails.datastore.mapping.transactions;
 
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 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;
 
 /**
  * Extended version of {@link RuleBasedTransactionAttribute} that ensures all exception types are rolled back and allows inheritance of setRollbackOnly
@@ -51,13 +55,8 @@ public CustomizableRollbackTransactionAttribute(int propagationBehavior, List<Ro
         super(propagationBehavior, rollbackRules);
     }
 
-    public CustomizableRollbackTransactionAttribute(org.springframework.transaction.interceptor.TransactionAttribute other) {
-        super();
-        setPropagationBehavior(other.getPropagationBehavior());
-        setIsolationLevel(other.getIsolationLevel());
-        setTimeout(other.getTimeout());
-        setReadOnly(other.isReadOnly());
-        setName(other.getName());
+    public CustomizableRollbackTransactionAttribute(TransactionAttribute other) {
+        this((TransactionDefinition) other);
     }
 
     public CustomizableRollbackTransactionAttribute(TransactionDefinition other) {
@@ -67,22 +66,56 @@ public CustomizableRollbackTransactionAttribute(TransactionDefinition other) {
         setTimeout(other.getTimeout());
         setReadOnly(other.isReadOnly());
         setName(other.getName());
+        if (other instanceof TransactionAttribute attribute) {
+            copyAttributeState(attribute);
+        }
+        if (other instanceof RuleBasedTransactionAttribute ruleBased) {
+            // 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(ruleBased).getRollbackRules());
+        }
+        copyCustomizableState(other);
     }
 
     public CustomizableRollbackTransactionAttribute(CustomizableRollbackTransactionAttribute other) {
         this((RuleBasedTransactionAttribute) other);
     }
 
     public CustomizableRollbackTransactionAttribute(RuleBasedTransactionAttribute other) {
-        if (other instanceof CustomizableRollbackTransactionAttribute) {
-            this.inheritRollbackOnly = ((CustomizableRollbackTransactionAttribute) other).inheritRollbackOnly;
+        super(other);
+        copyAttributeState(other);
+        copyCustomizableState(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 defaultAttribute) {
+            setDescriptor(defaultAttribute.getDescriptor());
+            setTimeoutString(defaultAttribute.getTimeoutString());
+        }
+        setQualifier(other.getQualifier());
+        Collection<String> labels = other.getLabels();
+        if (labels != null) {
+            // defensive copy: setLabels stores the given reference
+            setLabels(new ArrayList<>(labels));
+        }
+    }
+
+    private void copyCustomizableState(TransactionDefinition other) {
+        if (other instanceof CustomizableRollbackTransactionAttribute custom) {
+            this.inheritRollbackOnly = custom.inheritRollbackOnly;
+            this.connection = custom.connection;
         }
     }
 
     @Override
     public boolean rollbackOn(Throwable ex) {
         if (log.isTraceEnabled()) {
-            log.trace("Applying rules to determine whether transaction should rollback on $ex");
+            log.trace("Applying rules to determine whether transaction should rollback on " + ex);
         }
 
         RollbackRuleAttribute winner = null;
@@ -100,12 +133,14 @@ public boolean rollbackOn(Throwable ex) {
         }
 
         if (log.isTraceEnabled()) {
-            log.trace("Winning rollback rule is: $winner");
+            log.trace("Winning rollback rule is: " + winner);
         }
 
         // User superclass behavior (rollback on unchecked) if no rule matches.
         if (winner == null) {
-            log.trace("No relevant rollback rule found: applying default rules");
+            if (log.isTraceEnabled()) {
+                log.trace("No relevant rollback rule found: applying default rules");
+            }
 
             // always rollback regardless if it is a checked or unchecked exception since Groovy doesn't differentiate those
             return true;