Add specs and clean up transactions.transform/transform in grails-datamapping-core

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/transform/TransactionalTransform.groovy

@@ -200,7 +200,7 @@ class TransactionalTransform extends AbstractDatastoreMethodDecoratingTransforma
 
     @Override
     protected void enhanceClassNode(SourceUnit source, AnnotationNode annotationNode, ClassNode declaringClassNode) {
-        weaveTransactionManagerAware(sourceUnit, annotationNode, declaringClassNode)
+        weaveTransactionManagerAware(annotationNode, declaringClassNode)
         super.enhanceClassNode(source, annotationNode, declaringClassNode)
     }
 
@@ -221,7 +221,7 @@ class TransactionalTransform extends AbstractDatastoreMethodDecoratingTransforma
 
     }
 
-    protected void weaveTransactionManagerAware(SourceUnit source, AnnotationNode annotationNode, ClassNode declaringClassNode) {
+    protected void weaveTransactionManagerAware(AnnotationNode annotationNode, ClassNode declaringClassNode) {
         if (declaringClassNode.getNodeMetaData(APPLIED_MARKER) == APPLIED_MARKER) {
             return
         }
@@ -419,7 +419,7 @@ class TransactionalTransform extends AbstractDatastoreMethodDecoratingTransforma
         final ClassNode rollbackRuleAttributeClassNode = make(RollbackRuleAttribute)
         final ClassNode noRollbackRuleAttributeClassNode = make(NoRollbackRuleAttribute)
         final Map<String, Expression> members = annotationNode.getMembers()
-        if (READ_ONLY_TYPE.equals(annotationNode.classNode)) {
+        if (READ_ONLY_TYPE == annotationNode.classNode) {
             methodBody.addStatement(
                 assignS(propX(transactionAttributeVar, 'readOnly'), ConstantExpression.TRUE)
             )

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformation.groovy

@@ -79,14 +79,13 @@ abstract class AbstractDatastoreMethodDecoratingTransformation extends AbstractM
 
     @Override
     protected void enhanceClassNode(SourceUnit source, AnnotationNode annotationNode, ClassNode declaringClassNode) {
-        def appliedMarker = getAppliedMarker()
-        if (declaringClassNode.getNodeMetaData(appliedMarker) == appliedMarker) {
+        if (isAlreadyApplied(declaringClassNode)) {
             return
         }
         if (declaringClassNode.isInterface()) {
             return
         }
-        declaringClassNode.putNodeMetaData(appliedMarker, appliedMarker)
+        markApplied(declaringClassNode)
 
         Expression connectionName = annotationNode.getMember('connection')
         boolean hasDataSourceProperty = connectionName != null

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractGormASTTransformation.groovy

@@ -55,18 +55,38 @@ abstract class AbstractGormASTTransformation extends AbstractASTTransformation i
             return
         }
 
-        Object appliedMarker = getAppliedMarker()
-        if (annotatedNode.getNodeMetaData(appliedMarker) == appliedMarker) {
+        if (isAlreadyApplied(annotatedNode)) {
             return
         }
 
         visit(source, annotationNode, annotatedNode)
 
-        annotatedNode.putNodeMetaData(appliedMarker, appliedMarker)
+        markApplied(annotatedNode)
     }
 
     protected boolean isValidAnnotation(AnnotationNode annotationNode, AnnotatedNode classNode) {
-        return getAnnotationType().equals(annotationNode.getClassNode()) || !(classNode instanceof ClassNode)
+        return getAnnotationType() == annotationNode.getClassNode() || !(classNode instanceof ClassNode)
+    }
+
+    /**
+     * Whether the given node already carries this transformation's applied marker.
+     *
+     * @param node The node
+     * @return true if {@link #visit} (or an equivalent per-method/per-class idempotency check in a subclass) has already run for this node
+     */
+    protected boolean isAlreadyApplied(AnnotatedNode node) {
+        Object appliedMarker = getAppliedMarker()
+        node.getNodeMetaData(appliedMarker) == appliedMarker
+    }
+
+    /**
+     * Marks the given node as having had this transformation applied, so a later {@link #isAlreadyApplied} check short-circuits.
+     *
+     * @param node The node
+     */
+    protected void markApplied(AnnotatedNode node) {
+        Object appliedMarker = getAppliedMarker()
+        node.putNodeMetaData(appliedMarker, appliedMarker)
     }
 
     abstract void visit(SourceUnit source, AnnotationNode annotationNode, AnnotatedNode annotatedNode)

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy

@@ -187,15 +187,14 @@ abstract class AbstractMethodDecoratingTransformation extends AbstractGormASTTra
      * @return The new method's body
      */
     protected MethodNode weaveNewMethod(SourceUnit sourceUnit, AnnotationNode annotationNode, ClassNode classNode, MethodNode methodNode, Map<String, ClassNode> genericsSpec) {
-        Object appliedMarker = getAppliedMarker()
-        if (methodNode.getNodeMetaData(appliedMarker) == appliedMarker) {
+        if (isAlreadyApplied(methodNode)) {
             return methodNode
         }
         if (methodNode.isAbstract()) {
             return methodNode
         }
 
-        methodNode.putNodeMetaData(appliedMarker, appliedMarker)
+        markApplied(methodNode)
 
         enhanceClassNode(sourceUnit, annotationNode, classNode)
 

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractTraitApplyingGormASTTransformation.groovy

@@ -116,7 +116,7 @@ abstract class AbstractTraitApplyingGormASTTransformation extends AbstractGormAS
     }
 
     void visitAfterTraitApplied(SourceUnit sourceUnit, AnnotationNode annotationNode, ClassNode classNode) {
-        // no-dop
+        // no-op
     }
 
     protected abstract Class getTraitClass()

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java

@@ -20,7 +20,9 @@
 package org.grails.datastore.gorm.transform;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -47,7 +49,14 @@
  * @since 6.1
  */
 public class AstPropertyResolveUtils {
-    protected static Map<String, Map<String, ClassNode>> cachedClassProperties = new HashMap<>();
+
+    // ClassNode#equals()/hashCode() compare by name, so two distinct ClassNode instances from
+    // separate compilations (as happens with dynamically-generated/test classes) can legitimately
+    // share a name. Keying by name alone would let one class's resolved properties leak into an
+    // unrelated class node that happens to share it, so the cache is keyed by ClassNode identity
+    // instead, and guarded so the check-then-populate-then-store sequence below is atomic.
+    protected static final Map<ClassNode, Map<String, ClassNode>> cachedClassProperties =
+            Collections.synchronizedMap(new IdentityHashMap<>());
 
     /**
      * Resolves the type of of the given property
@@ -57,7 +66,7 @@ public class AstPropertyResolveUtils {
      * @return The type
      */
     public static ClassNode getPropertyType(ClassNode classNode, String propertyName) {
-        if (propertyName == null || propertyName.length() == 0) {
+        if (propertyName == null || propertyName.isEmpty()) {
             return null;
         }
         Map<String, ClassNode> cachedProperties = getPropertiesFromCache(classNode);
@@ -94,22 +103,24 @@ public static List<String> getPropertyNames(ClassNode classNode) {
     }
 
     private static Map<String, ClassNode> getPropertiesFromCache(ClassNode classNode) {
-        String className = classNode.getName();
-        Map<String, ClassNode> cachedProperties = cachedClassProperties.get(className);
-        if (cachedProperties == null) {
-            cachedProperties = new HashMap<>();
-            boolean isDomainClass = AstUtils.isDomainClass(classNode);
-            if (isDomainClass) {
-                cachedProperties.put(GormProperties.IDENTITY, new ClassNode(Long.class));
-                cachedProperties.put(GormProperties.VERSION, new ClassNode(Long.class));
-            }
-            cachedClassProperties.put(className, cachedProperties);
-            ClassNode currentNode = classNode;
-            while (currentNode != null && !currentNode.equals(ClassHelper.OBJECT_TYPE)) {
-                populatePropertiesForClassNode(currentNode, cachedProperties, isDomainClass, !isDomainClass);
-                currentNode = currentNode.getSuperClass();
+        synchronized (cachedClassProperties) {
+            Map<String, ClassNode> cachedProperties = cachedClassProperties.get(classNode);
+            if (cachedProperties == null) {
+                cachedProperties = new HashMap<>();
+                boolean isDomainClass = AstUtils.isDomainClass(classNode);
+                if (isDomainClass) {
+                    cachedProperties.put(GormProperties.IDENTITY, new ClassNode(Long.class));
+                    cachedProperties.put(GormProperties.VERSION, new ClassNode(Long.class));
+                }
+                cachedClassProperties.put(classNode, cachedProperties);
+                ClassNode currentNode = classNode;
+                while (currentNode != null && !currentNode.equals(ClassHelper.OBJECT_TYPE)) {
+                    populatePropertiesForClassNode(currentNode, cachedProperties, isDomainClass, !isDomainClass);
+                    currentNode = currentNode.getSuperClass();
+                }
             }
-        } return cachedProperties;
+            return cachedProperties;
+        }
     }
 
     private static void populatePropertiesForClassNode(ClassNode classNode, Map<String, ClassNode> cachedProperties, boolean isDomainClass, boolean allowAbstract) {
@@ -155,21 +166,19 @@ private static void populatePropertiesForClassNode(ClassNode classNode, Map<Stri
     private static void cachePropertiesForAssociationMetadata(Map<String, ClassNode> cachedProperties, ClassPropertyFetcher propertyFetcher, String associationMetadataName) {
         if (propertyFetcher.isReadableProperty(associationMetadataName)) {
             Object propertyValue = propertyFetcher.getPropertyValue(associationMetadataName);
-            if (propertyValue instanceof Map) {
-                Map hasManyMap = (Map) propertyValue;
+            if (propertyValue instanceof Map<?, ?> hasManyMap) {
                 for (Object propertyName : hasManyMap.keySet()) {
                     Object val = hasManyMap.get(propertyName);
-                    if (val instanceof Class) {
-                        cachedProperties.put(propertyName.toString(), ClassHelper.make((Class) val).getPlainNodeReference());
+                    if (val instanceof Class<?> valType) {
+                        cachedProperties.put(propertyName.toString(), ClassHelper.make(valType).getPlainNodeReference());
                     }
                 }
             }
         }
     }
 
     private static void populatePropertiesForInitialExpression(Map<String, ClassNode> cachedProperties, Expression initialExpression) {
-        if (initialExpression instanceof MapExpression) {
-            MapExpression me = (MapExpression) initialExpression;
+        if (initialExpression instanceof MapExpression me) {
             List<MapEntryExpression> mapEntryExpressions = me.getMapEntryExpressions();
             for (MapEntryExpression mapEntryExpression : mapEntryExpressions) {
                 Expression keyExpression = mapEntryExpression.getKeyExpression();

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/GormASTTransformationClass.java

@@ -24,7 +24,8 @@
 import java.lang.annotation.Target;
 
 /**
- * U
+ * Marker meta-annotation that points a GORM annotation (e.g. {@code @Transactional}, {@code @Rollback},
+ * {@code @Tenant}) at the {@link org.codehaus.groovy.transform.ASTTransformation} class that implements it.
  *
  * @author Graeme Rocher
  * @since 6.1

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy

@@ -83,7 +83,7 @@ class OrderedGormTransformation extends AbstractASTTransformation implements Com
             String transformName = findTransformName(ann)
             if (transformName) {
                 try {
-                    def newTransform = ClassUtils.forName(transformName).newInstance()
+                    def newTransform = ClassUtils.forName(transformName).getDeclaredConstructor().newInstance()
                     if (newTransform instanceof ASTTransformation) {
                         if (newTransform instanceof CompilationUnitAware) {
                             ((CompilationUnitAware) newTransform).setCompilationUnit(compilationUnit)

grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transactions/transform/RollbackTransformSpec.groovy

@@ -0,0 +1,58 @@
+/*
+ *  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 org.grails.datastore.gorm.transactions.transform
+
+import spock.lang.Specification
+
+import grails.gorm.transactions.Rollback
+import org.apache.grails.common.compiler.GroovyTransformOrder
+
+/**
+ * {@code RollbackTransform} only overrides two methods of {@link TransactionalTransform} and its
+ * end-to-end weaving behavior is already exercised (via the {@code @Rollback} annotation) by
+ * {@code TransactionalTransformSpec}. This spec covers what those behavioral tests can't: that the
+ * overrides themselves - the transaction template method name and the transform ordering priority -
+ * are the values that make {@code @Rollback} behave differently from plain {@code @Transactional}.
+ */
+class RollbackTransformSpec extends Specification {
+
+    void "getTransactionTemplateMethodName overrides the parent to route through the rollback-forcing template method"() {
+        given:
+        RollbackTransform transform = new RollbackTransform()
+
+        expect:
+        transform.getTransactionTemplateMethodName() == 'executeAndRollback'
+        new TransactionalTransform().getTransactionTemplateMethodName() == 'execute'
+    }
+
+    void "priority orders RollbackTransform after TransactionalTransform"() {
+        given:
+        RollbackTransform transform = new RollbackTransform()
+
+        expect:
+        transform.priority() == GroovyTransformOrder.ROLLBACK_ORDER
+        transform.priority() < GroovyTransformOrder.TRANSACTIONAL_ORDER
+    }
+
+    void "MY_TYPE identifies the Rollback annotation and the class extends TransactionalTransform"() {
+        expect:
+        RollbackTransform.MY_TYPE.name == Rollback.name
+        TransactionalTransform.isAssignableFrom(RollbackTransform)
+    }
+}