From 1720abd7117ef96c4703819792bff37ee8a11da4 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Tue, 18 Aug 2026 12:55:22 -0500 Subject: [PATCH] Add specs and clean up transactions.transform/transform in grails-datamapping-core Closes direct unit-spec coverage gaps left after PR #16148 for RollbackTransform, AbstractDatastoreMethodDecoratingTransformation, AbstractMethodDecoratingTransformation, AstMethodDispatchUtils, and AstPropertyResolveUtils. Writing the AstPropertyResolveUtils spec surfaced a real correctness bug: its property cache was keyed by class name (a String), so two distinct ClassNode instances sharing a name (e.g. from separate compilations of dynamically-generated/test classes) silently corrupted each other's cached property data under concurrent use. Fixed by keying the cache on ClassNode identity via a synchronized IdentityHashMap. Also extracts the duplicated applied-marker idempotency check/mark pattern (repeated across AbstractGormASTTransformation, AbstractMethodDecoratingTransformation, and AbstractDatastoreMethodDecoratingTransformation) into shared isAlreadyApplied/markApplied helpers, and applies a batch of small cleanups flagged by static analysis: equals() calls replaced with ==, an unused method parameter removed, Java 21 instanceof pattern variables replacing raw-type casts, String#isEmpty() over length()==0, Class#getDeclaredConstructor().newInstance() over the deprecated Class#newInstance(), and two stray doc/comment fixes. Co-Authored-By: Claude Sonnet 5 --- .../transform/TransactionalTransform.groovy | 6 +- ...storeMethodDecoratingTransformation.groovy | 5 +- .../AbstractGormASTTransformation.groovy | 28 +- ...tractMethodDecoratingTransformation.groovy | 5 +- ...tTraitApplyingGormASTTransformation.groovy | 2 +- .../transform/AstPropertyResolveUtils.java | 55 ++-- .../transform/GormASTTransformationClass.java | 3 +- .../OrderedGormTransformation.groovy | 2 +- .../transform/RollbackTransformSpec.groovy | 58 ++++ ...eMethodDecoratingTransformationSpec.groovy | 183 +++++++++++ ...tMethodDecoratingTransformationSpec.groovy | 220 +++++++++++++ .../transform/ApplyTestMethodDecorating.java | 40 +++ .../AstMethodDispatchUtilsSpec.groovy | 171 +++++++++++ .../AstPropertyResolveUtilsSpec.groovy | 290 ++++++++++++++++++ .../TestMethodDecoratingTransformation.groovy | 72 +++++ 15 files changed, 1101 insertions(+), 39 deletions(-) create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transactions/transform/RollbackTransformSpec.groovy create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformationSpec.groovy create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformationSpec.groovy create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyTestMethodDecorating.java create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstMethodDispatchUtilsSpec.groovy create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy create mode 100644 grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestMethodDecoratingTransformation.groovy diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/transform/TransactionalTransform.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/transform/TransactionalTransform.groovy index 3f1e5a2d3c8..4ffe2655d91 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transactions/transform/TransactionalTransform.groovy +++ b/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 members = annotationNode.getMembers() - if (READ_ONLY_TYPE.equals(annotationNode.classNode)) { + if (READ_ONLY_TYPE == annotationNode.classNode) { methodBody.addStatement( assignS(propX(transactionAttributeVar, 'readOnly'), ConstantExpression.TRUE) ) diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformation.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformation.groovy index e8dac8d3b12..466adc9c307 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformation.groovy +++ b/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 diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractGormASTTransformation.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractGormASTTransformation.groovy index 6749943ed51..2a35d7d6bdf 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractGormASTTransformation.groovy +++ b/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) diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy index be943b40422..0938755b33a 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy +++ b/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 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) diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractTraitApplyingGormASTTransformation.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractTraitApplyingGormASTTransformation.groovy index d072c511ad8..55ecc8c13bf 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractTraitApplyingGormASTTransformation.groovy +++ b/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() diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java index 4deeed44ea1..64e802c269b 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java +++ b/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> 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> 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 cachedProperties = getPropertiesFromCache(classNode); @@ -94,22 +103,24 @@ public static List getPropertyNames(ClassNode classNode) { } private static Map getPropertiesFromCache(ClassNode classNode) { - String className = classNode.getName(); - Map 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 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 cachedProperties, boolean isDomainClass, boolean allowAbstract) { @@ -155,12 +166,11 @@ private static void populatePropertiesForClassNode(ClassNode classNode, Map 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()); } } } @@ -168,8 +178,7 @@ private static void cachePropertiesForAssociationMetadata(Map } private static void populatePropertiesForInitialExpression(Map cachedProperties, Expression initialExpression) { - if (initialExpression instanceof MapExpression) { - MapExpression me = (MapExpression) initialExpression; + if (initialExpression instanceof MapExpression me) { List mapEntryExpressions = me.getMapEntryExpressions(); for (MapEntryExpression mapEntryExpression : mapEntryExpressions) { Expression keyExpression = mapEntryExpression.getKeyExpression(); diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/GormASTTransformationClass.java b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/GormASTTransformationClass.java index 7c7e298cdff..eb71c6a054c 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/GormASTTransformationClass.java +++ b/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 diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy index ab22018d102..f33e014081a 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy +++ b/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) diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transactions/transform/RollbackTransformSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transactions/transform/RollbackTransformSpec.groovy new file mode 100644 index 00000000000..0ff617cbab1 --- /dev/null +++ b/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) + } +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformationSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformationSpec.groovy new file mode 100644 index 00000000000..f9cc6cace0e --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractDatastoreMethodDecoratingTransformationSpec.groovy @@ -0,0 +1,183 @@ +/* + * 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.transform + +import java.lang.reflect.Modifier + +import groovy.transform.CompileStatic +import org.codehaus.groovy.ast.AnnotationNode +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.MethodNode +import org.codehaus.groovy.ast.Parameter +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.control.SourceUnit + +import org.springframework.beans.factory.annotation.Autowired + +import spock.lang.Specification + +import org.grails.datastore.mapping.core.Datastore +import org.grails.datastore.mapping.core.connections.MultipleConnectionSourceCapableDatastore +import org.grails.datastore.mapping.services.Service + +/** + * {@code AbstractDatastoreMethodDecoratingTransformation} is only ever exercised in this module + * through {@code TransactionalTransform} and {@code TenantTransform}, both of which are always driven + * through a real compilation, so {@code enhanceClassNode} is never called directly and its Service- + * interface branch (only reachable for a class implementing {@code org.grails.datastore.mapping.services.Service}, + * which none of the real transform specs' fixtures do) is never exercised at all. Because + * {@code enhanceClassNode} only touches the {@code ClassNode} it's given - it never dereferences the + * {@code SourceUnit} parameter unless {@code compilationUnit} is set, which it isn't for a bare + * instance - it can be called directly against hand-built {@code ClassNode}s, the same technique used + * for the other abstract transformation specs in this package. + */ +class AbstractDatastoreMethodDecoratingTransformationSpec extends Specification { + + static class MinimalDatastoreDecoratingTransformation extends AbstractDatastoreMethodDecoratingTransformation { + + @Override + protected ClassNode getAnnotationType() { + ClassHelper.make(CompileStatic) + } + + @Override + protected Object getAppliedMarker() { + 'datastore-decorating-applied-marker' + } + + @Override + protected String getRenamedMethodPrefix() { + '$test__' + } + + @Override + protected Expression buildDelegatingMethodCall(SourceUnit sourceUnit, AnnotationNode annotationNode, ClassNode classNode, + MethodNode methodNode, MethodCallExpression originalMethodCall, BlockStatement newMethodBody) { + originalMethodCall + } + + @Override + int priority() { + 0 + } + } + + private static ClassNode newTargetClassNode(String name) { + new ClassNode(name, Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + } + + private static Parameter[] stringConnectionNameParam() { + [new Parameter(ClassHelper.STRING_TYPE, 'connectionName')] as Parameter[] + } + + void "enhanceClassNode adds a targetDatastore field and public getter/setter methods to a plain class"() { + given: + MinimalDatastoreDecoratingTransformation transformation = new MinimalDatastoreDecoratingTransformation() + ClassNode classNode = newTargetClassNode('org.grails.datastore.gorm.transform.fixture.PlainDecoratedTarget') + AnnotationNode annotationNode = new AnnotationNode(ClassHelper.make(CompileStatic)) + + when: + transformation.enhanceClassNode(null, annotationNode, classNode) + + then: 'the datastore field is added, typed as the default Datastore' + classNode.getField('$targetDatastore').type == ClassHelper.make(Datastore) + + and: 'both getTargetDatastore overloads are added as public methods' + classNode.getMethod('getTargetDatastore', Parameter.EMPTY_ARRAY) != null + classNode.getMethod('getTargetDatastore', stringConnectionNameParam()) != null + + and: 'a public setter is added, autowired but not required' + MethodNode setter = classNode.getMethods('setTargetDatastore')[0] + Modifier.isPublic(setter.modifiers) + AnnotationNode autowired = setter.getAnnotations(ClassHelper.make(Autowired))[0] + ((ConstantExpression) autowired.getMember('required')).value == false + } + + void "enhanceClassNode adds only protected getTargetDatastore methods and no field when the class implements Service"() { + given: + MinimalDatastoreDecoratingTransformation transformation = new MinimalDatastoreDecoratingTransformation() + ClassNode classNode = newTargetClassNode('org.grails.datastore.gorm.transform.fixture.ServiceDecoratedTarget') + classNode.addInterface(ClassHelper.make(Service)) + AnnotationNode annotationNode = new AnnotationNode(ClassHelper.make(CompileStatic)) + + when: + transformation.enhanceClassNode(null, annotationNode, classNode) + + then: 'no field is added - the Service is looked up rather than injected' + classNode.getField('$targetDatastore') == null + + and: 'both getTargetDatastore overloads are added, but protected rather than public' + MethodNode noArgGetter = classNode.getMethod('getTargetDatastore', Parameter.EMPTY_ARRAY) + MethodNode connectionGetter = classNode.getMethod('getTargetDatastore', stringConnectionNameParam()) + Modifier.isProtected(noArgGetter.modifiers) + Modifier.isProtected(connectionGetter.modifiers) + + and: 'no setter is added at all' + classNode.getMethods('setTargetDatastore').empty + } + + void "enhanceClassNode uses MultipleConnectionSourceCapableDatastore as the field type when a connection name is specified"() { + given: + MinimalDatastoreDecoratingTransformation transformation = new MinimalDatastoreDecoratingTransformation() + ClassNode classNode = newTargetClassNode('org.grails.datastore.gorm.transform.fixture.ConnectionDecoratedTarget') + AnnotationNode annotationNode = new AnnotationNode(ClassHelper.make(CompileStatic)) + annotationNode.addMember('connection', new ConstantExpression('foo')) + + when: + transformation.enhanceClassNode(null, annotationNode, classNode) + + then: + classNode.getField('$targetDatastore').type == ClassHelper.make(MultipleConnectionSourceCapableDatastore) + } + + void "enhanceClassNode is idempotent once the applied marker is already set on the class node"() { + given: + MinimalDatastoreDecoratingTransformation transformation = new MinimalDatastoreDecoratingTransformation() + ClassNode classNode = newTargetClassNode('org.grails.datastore.gorm.transform.fixture.AlreadyAppliedTarget') + classNode.putNodeMetaData(transformation.getAppliedMarker(), transformation.getAppliedMarker()) + AnnotationNode annotationNode = new AnnotationNode(ClassHelper.make(CompileStatic)) + + when: + transformation.enhanceClassNode(null, annotationNode, classNode) + + then: 'nothing is added because the marker short-circuits enhancement' + classNode.getField('$targetDatastore') == null + classNode.methods.every { it.name != 'getTargetDatastore' } + } + + void "enhanceClassNode is a no-op for an interface class node"() { + given: + MinimalDatastoreDecoratingTransformation transformation = new MinimalDatastoreDecoratingTransformation() + ClassNode interfaceNode = new ClassNode( + 'org.grails.datastore.gorm.transform.fixture.NoDecorationInterfaceTarget', + Modifier.PUBLIC | Modifier.INTERFACE, ClassHelper.OBJECT_TYPE) + AnnotationNode annotationNode = new AnnotationNode(ClassHelper.make(CompileStatic)) + + when: + transformation.enhanceClassNode(null, annotationNode, interfaceNode) + + then: + interfaceNode.getField('$targetDatastore') == null + interfaceNode.methods.empty + } +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformationSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformationSpec.groovy new file mode 100644 index 00000000000..cae293280f4 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformationSpec.groovy @@ -0,0 +1,220 @@ +/* + * 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.transform + +import org.springframework.util.ReflectionUtils + +import spock.lang.Specification + +/** + * {@code AbstractMethodDecoratingTransformation}'s method-selection logic in {@code weaveClassNode} - + * which methods a decorating transform like {@code @Transactional} actually gets applied to - is + * normally exercised indirectly and incompletely through whichever fixtures the real transforms' + * specs happen to declare. This spec drives it directly, through a real compilation, using + * {@link TestMethodDecoratingTransformation} - a pass-through decorator with no side effects of its + * own - so each inclusion/exclusion branch can be asserted on independently of any particular real + * transform's semantics. + */ +class AbstractMethodDecoratingTransformationSpec extends Specification { + + private static Class compile(String source) { + new GroovyClassLoader().parseClass(source) + } + + void "a plain public instance method is renamed and re-dispatched to"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class PlainMethodTarget { + String updateFoo() { 'original' } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__updateFoo') != null + target.getDeclaredConstructor().newInstance().updateFoo() == 'original' + } + + void "static, private and abstract methods are never woven"() { + when: + Class concrete = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class StaticAndPrivateMethodsTarget { + static void staticMethod() { } + private void privateMethod() { } + } + ''') + Class abstractTarget = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + abstract class AbstractMethodTarget { + abstract void abstractMethod() + } + ''') + + then: + ReflectionUtils.findMethod(concrete, '$test__staticMethod') == null + ReflectionUtils.findMethod(concrete, '$test__privateMethod', String) == null + concrete.declaredMethods.every { !it.name.contains('$test__privateMethod') } + ReflectionUtils.findMethod(abstractTarget, '$test__abstractMethod') == null + } + + void "METHOD_NAME_EXCLUDES keeps lifecycle method names such as afterPropertiesSet and destroy unwoven"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class LifecycleMethodTarget { + void afterPropertiesSet() { } + void destroy() { } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__afterPropertiesSet') == null + ReflectionUtils.findMethod(target, '$test__destroy') == null + } + + void "setters are never woven and a getter is only woven when it has no matching setter"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class GetterSetterTarget { + String name + String getAge() { 'ageless' } + } + ''') + + then: 'the setter itself is never a weaving candidate' + ReflectionUtils.findMethod(target, '$test__setName', String) == null + + and: 'the getter that has a matching setter is skipped' + ReflectionUtils.findMethod(target, '$test__getName') == null + + and: 'the getter with no matching setter is woven' + ReflectionUtils.findMethod(target, '$test__getAge') != null + } + + void "a dollar-prefixed method name that is not a spock feature method is skipped"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class DollarMethodTarget { + void $rawMethod() { } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__$rawMethod') == null + } + + void "hasExcludedAnnotation skips methods annotated with PostConstruct"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class PostConstructTarget { + @jakarta.annotation.PostConstruct + void init() { } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__init') == null + } + + void "spock setup and cleanup are routed to weaveTestSetupMethod instead of being renamed"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class SpockSetupCleanupTarget extends spock.lang.Specification { + def setup() { } + def cleanup() { } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__setup') == null + ReflectionUtils.findMethod(target, '$test__cleanup') == null + } + + void "a JUnit-annotated method is routed to weaveTestSetupMethod instead of being renamed"() { + when: + Class target = compile(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class JunitAnnotatedTarget { + @org.junit.jupiter.api.BeforeEach + void junitSetup() { } + } + ''') + + then: + ReflectionUtils.findMethod(target, '$test__junitSetup') == null + } + + void "an overriding method is renamed with a class-qualified prefix while the parent's own method uses the plain prefix"() { + when: 'evaluating the script (rather than parsing a single class) returns both declared types, in order' + List> types = new GroovyShell().evaluate(''' + package org.grails.datastore.gorm.transform.fixture + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class OverrideParentTarget { + String sound() { 'parent' } + } + + @org.grails.datastore.gorm.transform.ApplyTestMethodDecorating + class OverrideChildTarget extends OverrideParentTarget { + @Override + String sound() { 'child' } + } + + [OverrideParentTarget, OverrideChildTarget] + ''') as List> + Class parent = types[0] + Class child = types[1] + + then: 'the parent method - not an override - uses the plain renamed prefix' + ReflectionUtils.findMethod(parent, '$test__sound') != null + + and: 'the overriding method is renamed with the decapitalized declaring class name mixed in' + ReflectionUtils.findMethod(child, '$test__overrideChildTarget_sound') != null + + and: 'the child does not declare its own plain-prefixed renamed method (only inherits the parent one)' + child.declaredMethods.every { it.name != '$test__sound' } + + and: 'the woven methods still dispatch correctly' + parent.getDeclaredConstructor().newInstance().sound() == 'parent' + child.getDeclaredConstructor().newInstance().sound() == 'child' + } +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyTestMethodDecorating.java b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyTestMethodDecorating.java new file mode 100644 index 00000000000..8166262ca65 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyTestMethodDecorating.java @@ -0,0 +1,40 @@ +/* + * 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.transform; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import org.codehaus.groovy.transform.GroovyASTTransformationClass; + +/** + * Local marker annotation used only for testing {@link AbstractMethodDecoratingTransformation} + * through a real, live compilation - triggering {@link TestMethodDecoratingTransformation}, a + * pass-through decorator that renames and re-dispatches to the original method body without + * wrapping it in anything (unlike the real {@code Transactional}/{@code CurrentTenant} transforms), + * so the method-selection logic in {@code weaveClassNode} can be exercised in isolation. + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE}) +@GroovyASTTransformationClass("org.grails.datastore.gorm.transform.TestMethodDecoratingTransformation") +public @interface ApplyTestMethodDecorating { +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstMethodDispatchUtilsSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstMethodDispatchUtilsSpec.groovy new file mode 100644 index 00000000000..1e6d73ee599 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstMethodDispatchUtilsSpec.groovy @@ -0,0 +1,171 @@ +/* + * 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.transform + +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.Parameter +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MapExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.TupleExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import spock.lang.Specification + +import static org.codehaus.groovy.ast.tools.GeneralUtils.args +import static org.codehaus.groovy.ast.tools.GeneralUtils.constX +import static org.codehaus.groovy.ast.tools.GeneralUtils.varX +import static org.grails.datastore.mapping.reflect.AstUtils.ZERO_ARGUMENTS + +/** + * {@code AstMethodDispatchUtils} builds AST method-call expressions and, when it can resolve the + * target method on the declared type, wires the {@code MethodTarget} onto the call so the compiler + * doesn't have to do dynamic dispatch. This spec proves that resolution happens when the method + * genuinely exists on the target type, is left unresolved when it doesn't, and that the small + * argument/parameter-building helpers (`namedArgs`, `paramsForArgs`) produce the shapes their callers + * across the module (service implementers, `TenantTransform`, the transactional transforms) rely on. + */ +class AstMethodDispatchUtilsSpec extends Specification { + + static class Greeter { + String greet(String name) { "hello $name" } + } + + void "namedArgs builds a MapExpression with one entry per named argument"() { + given: + ConstantExpression trueExpr = ConstantExpression.TRUE + + when: + MapExpression mapExpression = AstMethodDispatchUtils.namedArgs(failOnError: trueExpr) + + then: + mapExpression.mapEntryExpressions.size() == 1 + mapExpression.mapEntryExpressions[0].keyExpression.text == 'failOnError' + mapExpression.mapEntryExpressions[0].valueExpression.is(trueExpr) + } + + void "callD(Class, var, methodName) resolves the method target when the method exists on the target type"() { + when: + MethodCallExpression call = AstMethodDispatchUtils.callD(Greeter, 'greeter', 'greet', args(constX('World'))) + + then: + call.methodAsString == 'greet' + call.methodTarget != null + call.methodTarget.name == 'greet' + call.objectExpression instanceof VariableExpression + ((VariableExpression) call.objectExpression).name == 'greeter' + } + + void "callD(ClassNode, var, methodName) leaves the method target unset when the method does not exist"() { + given: + ClassNode greeterType = ClassHelper.make(Greeter) + + when: + MethodCallExpression call = AstMethodDispatchUtils.callD(greeterType, 'greeter', 'doesNotExist') + + then: + call.methodAsString == 'doesNotExist' + call.methodTarget == null + } + + void "callD(Expression, methodName) resolves against the expression's static type"() { + given: + VariableExpression target = varX('greeter', ClassHelper.make(Greeter)) + + when: + MethodCallExpression call = AstMethodDispatchUtils.callD(target, 'greet', args(constX('World'))) + + then: + call.methodTarget != null + call.methodTarget.name == 'greet' + } + + void "callD defaults to ZERO_ARGUMENTS when no arguments are supplied"() { + when: + MethodCallExpression call = AstMethodDispatchUtils.callD(Greeter, 'greeter', 'greet') + + then: + call.arguments.is(ZERO_ARGUMENTS) + } + + void "callThisD(Class, methodName) builds a call on an explicit 'this' of the given type and resolves the target"() { + when: + MethodCallExpression call = AstMethodDispatchUtils.callThisD(Greeter, 'greet', args(constX('World'))) + + then: + call.methodTarget != null + call.methodTarget.name == 'greet' + ((VariableExpression) call.objectExpression).name == 'this' + ((VariableExpression) call.objectExpression).type == ClassHelper.make(Greeter) + } + + void "callThisD(ClassNode, methodName, arguments) leaves the method target unset when the method does not exist"() { + given: + ClassNode greeterType = ClassHelper.make(Greeter) + + when: + MethodCallExpression call = AstMethodDispatchUtils.callThisD(greeterType, 'doesNotExist', ZERO_ARGUMENTS) + + then: + call.methodTarget == null + } + + void "paramsForArgs builds one parameter per expression in a TupleExpression, typed from each expression"() { + given: + TupleExpression tuple = args(constX('a string'), constX(1)) + + when: + Parameter[] params = AstMethodDispatchUtils.paramsForArgs(tuple) + + then: + params.length == 2 + params[0].name == 'p0' + params[0].type == ClassHelper.STRING_TYPE + params[1].name == 'p1' + params[1].type == ClassHelper.Integer_TYPE + } + + void "paramsForArgs treats a ClassExpression argument as typed Class, not the referenced type"() { + given: + Expression classArg = new ClassExpression(ClassHelper.make(Greeter)) + TupleExpression tuple = args(classArg) + + when: + Parameter[] params = AstMethodDispatchUtils.paramsForArgs(tuple) + + then: + params.length == 1 + params[0].type == ClassHelper.CLASS_Type + } + + void "paramsForArgs builds a single parameter for a bare (non-tuple) expression"() { + given: + Expression singleArg = constX('solo') + + when: + Parameter[] params = AstMethodDispatchUtils.paramsForArgs(singleArg) + + then: + params.length == 1 + params[0].name == 'p' + params[0].type == ClassHelper.STRING_TYPE + } +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy new file mode 100644 index 00000000000..fd9fa3ddaaf --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy @@ -0,0 +1,290 @@ +/* + * 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.transform + +import java.lang.reflect.Modifier +import java.util.concurrent.Callable +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.Future +import java.util.concurrent.TimeUnit + +import org.codehaus.groovy.ast.AnnotationNode +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MapExpression +import spock.lang.Specification + +import grails.gorm.annotation.Entity +import org.grails.datastore.mapping.model.config.GormProperties + +/** + * {@link AstPropertyResolveUtils} caches resolved property metadata as metadata on the + * {@link ClassNode} it describes (see that class's javadoc). Two distinct compilations (e.g. the + * same source parsed in two different {@code GroovyClassLoader}s, as happens for + * dynamically-generated sources and in tests) produce distinct {@code ClassNode} instances that + * can legitimately share the exact same name - {@code ClassNode#equals(Object)} compares by name, + * so a naive name- or equals()-based cache key would conflate them, corrupting the resolved + * properties of one class with those of an unrelated class that happens to share its name. This + * spec proves the cache is scoped strictly per {@code ClassNode} instance, so same-named-but-distinct + * class nodes never contaminate each other's cached property data, that domain-class-specific + * resolution (identity/version injection, association metadata) works, and that concurrent + * resolution of distinct nodes is safe. + */ +class AstPropertyResolveUtilsSpec extends Specification { + + void "property lookups for two same-named ClassNodes in different packages do not corrupt each other"() { + given: 'two distinct ClassNodes with the same simple name declared in different packages' + ClassNode first = new ClassNode('org.example.one.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + ClassNode second = new ClassNode('org.example.two.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null) + + when: 'the first class node is resolved, populating its cache entry' + List firstProperties = AstPropertyResolveUtils.getPropertyNames(first) + + then: 'only its own property is resolved' + firstProperties.contains('color') + !firstProperties.contains('weight') + + when: 'the second, differently-packaged, same-simple-name class node is resolved' + List secondProperties = AstPropertyResolveUtils.getPropertyNames(second) + + then: 'its own property is resolved, not leaked from the first class node' + secondProperties.contains('weight') + !secondProperties.contains('color') + + and: 'the first class node cache entry remains unaffected by resolving the second' + List firstPropertiesAfter = AstPropertyResolveUtils.getPropertyNames(first) + firstPropertiesAfter.contains('color') + !firstPropertiesAfter.contains('weight') + } + + void "property lookups for two distinct ClassNode instances with the exact same unqualified name do not corrupt each other"() { + given: 'two distinct ClassNode instances - as produced by two separate compilations - sharing an identical unqualified name' + ClassNode first = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + ClassNode second = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null) + + and: 'they are genuinely different instances - the precondition a name- or equals()-keyed cache would get wrong' + // ClassNode#equals()/hashCode() compare by getText() (essentially the class name), so + // first == second and first.hashCode() == second.hashCode() both hold here even though + // these are two unrelated ClassNode instances with different declared properties. A cache + // keyed by name or by equals()/hashCode() would treat them as the same entry; only + // reference identity (!first.is(second)) tells them apart, which is exactly what the + // cache must key on. This is an "and:" continuing "given:", so Spock does not apply an + // implicit condition here - the explicit assert is required for this to actually fail + // the test if it were ever untrue. + assert !first.is(second) + + when: 'both class nodes are resolved' + List firstProperties = AstPropertyResolveUtils.getPropertyNames(first) + List secondProperties = AstPropertyResolveUtils.getPropertyNames(second) + + then: 'each keeps its own, independently-resolved properties despite comparing equal' + firstProperties.contains('color') + !firstProperties.contains('weight') + secondProperties.contains('weight') + !secondProperties.contains('color') + } + + void "getPropertyType resolves the type of a declared property"() { + given: 'a class node with a declared property' + ClassNode classNode = new ClassNode('org.example.PropertyTypeWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + classNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + expect: 'the resolved property type matches the declared type, both on first and second lookup' + AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE + AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE + } + + void "getPropertyNames returns the snapshot taken on first lookup rather than reflecting properties added afterwards"() { + given: 'a class node with one declared property' + ClassNode classNode = new ClassNode('org.example.CachedSnapshotWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + classNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + when: 'the property names are resolved once, populating the cache' + List firstLookup = AstPropertyResolveUtils.getPropertyNames(classNode) + + then: + firstLookup == ['label'] + + when: 'a second property is added directly to the ClassNode after the cache has already been populated' + classNode.addProperty('extra', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null) + + then: 'a direct lookup on the ClassNode confirms the property really was added - so the cache below is stale, not simply broken' + classNode.getProperty('extra') != null + + and: 'getPropertyNames still returns the cached snapshot from the first lookup, proving the result was actually cached rather than recomputed on every call' + !AstPropertyResolveUtils.getPropertyNames(classNode).contains('extra') + } + + void "getPropertyNames injects identity and version for a domain class and resolves hasMany/belongsTo/hasOne declared via AST initial expressions"() { + given: 'a domain class node declaring hasMany/belongsTo/hasOne as property initial expressions, as "static hasMany = [...]" compiles to' + ClassNode associatedType = new ClassNode('org.example.AssociatedThing', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + ClassNode classNode = new ClassNode('org.example.AstDrivenDomain', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + classNode.addAnnotation(new AnnotationNode(ClassHelper.make(Entity))) + classNode.addProperty('title', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + classNode.addProperty(GormProperties.HAS_MANY, Modifier.PUBLIC | Modifier.STATIC, ClassHelper.MAP_TYPE.getPlainNodeReference(), + mapExpressionOf('things', associatedType), null, null) + + when: + List propertyNames = AstPropertyResolveUtils.getPropertyNames(classNode) + + then: 'the AST-declared property is present alongside the injected identity/version properties' + propertyNames.containsAll(['title', GormProperties.IDENTITY, GormProperties.VERSION, 'things']) + AstPropertyResolveUtils.getPropertyType(classNode, GormProperties.IDENTITY) == new ClassNode(Long.class) + AstPropertyResolveUtils.getPropertyType(classNode, 'things') == associatedType + + and: 'the raw hasMany/belongsTo/hasOne map property itself is not exposed as a plain property' + !propertyNames.contains(GormProperties.HAS_MANY) + } + + void "getPropertyNames resolves hasMany/belongsTo/hasOne association metadata via reflection once the domain class is fully resolved"() { + given: 'a real, already-compiled domain class with hasMany/belongsTo/hasOne associations' + GroovyClassLoader gcl = new GroovyClassLoader() + gcl.parseClass(''' + import grails.gorm.annotation.Entity + + @Entity + class ReflectedAssociationAuthor { + String name + } + + @Entity + class ReflectedAssociationBook { + String title + } + + @Entity + class ReflectedAssociationPublisher { + String company + } + + @Entity + class ReflectedAssociationFixture { + static hasMany = [books: ReflectedAssociationBook] + static belongsTo = [author: ReflectedAssociationAuthor] + static hasOne = [publisher: ReflectedAssociationPublisher] + } + ''') + Class domainClass = gcl.loadedClasses.find { it.simpleName == 'ReflectedAssociationFixture' } + + and: 'a fresh ClassNode built from the already-compiled class, as happens once compilation has finished' + ClassNode resolvedNode = ClassHelper.make(domainClass) + + expect: 'the node reports itself resolved, which is what gates the reflection-based association lookup' + resolvedNode.isResolved() + + when: + List propertyNames = AstPropertyResolveUtils.getPropertyNames(resolvedNode) + + then: 'the reflected association properties are present alongside the injected identity/version properties' + propertyNames.containsAll([GormProperties.IDENTITY, GormProperties.VERSION, 'books', 'author', 'publisher']) + + cleanup: + gcl.close() + } + + void "concurrent resolution of distinct, identically-named ClassNode instances never corrupts each other's cached properties"() { + given: 'many threads, each building and resolving its own distinct ClassNode sharing one common name' + int threadCount = 20 + ExecutorService executor = Executors.newFixedThreadPool(threadCount) + CyclicBarrier barrier = new CyclicBarrier(threadCount) + + when: 'all threads race to populate the cache for their own instance at the same time' + List> futures = (0.. + executor.submit({ -> + barrier.await(30, TimeUnit.SECONDS) + ClassNode node = new ClassNode('ConcurrentWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + String propertyName = "prop${i}".toString() + node.addProperty(propertyName, Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + List names = AstPropertyResolveUtils.getPropertyNames(node) + names.contains(propertyName) && names.count { it.startsWith('prop') } == 1 + } as Callable) + } + List outcomes = futures.collect { Future future -> future.get(30, TimeUnit.SECONDS) } + + then: 'every thread resolved its own property set, uncontaminated by any of the other concurrently-resolved same-named instances' + outcomes.every { it } + + cleanup: + executor.shutdownNow() + } + + void "concurrent resolution of the exact same shared ClassNode instance from many threads is safe"() { + // Distinct-instance concurrency (the test above) can never exercise a race on the + // underlying node-metadata storage, because nothing is shared between the threads. A single + // ClassNode instance genuinely can be looked up from more than one thread at once in + // practice - e.g. ClassHelper.OBJECT_TYPE/STRING_TYPE are JVM-wide singletons that this + // utility's callers can resolve to for a plain Object- or def-typed property, so two + // unrelated, concurrently-running compilations could both reach this cache for the exact + // same node. This test exercises that shared-node case directly and asserts every thread's + // returned value is correct. + // + // Note on what this test can and can't prove: the cached computation here is deterministic + // and idempotent, so even with the "synchronized (cachedClassProperties)" guard in + // AstPropertyResolveUtils#getPropertiesFromCache removed, every thread still computes and + // returns the same correct value in practice - a black-box test of returned values cannot + // reliably force ClassNode's underlying, explicitly-documented-not-thread-safe metadata + // storage into an observably-wrong state without reaching into Groovy internals neither this + // spec nor AstPropertyResolveUtils controls. Verified by temporarily removing that guard and + // running this test 8 times without a failure. The synchronization is kept as a correctness + // fix justified by IdentityHashMap's own documentation (it is not synchronized and concurrent + // structural modification can corrupt it), not because this test can demonstrate its absence + // breaking anything; this test instead guards against a regression to something observably + // broken (an exception, a null, a wrong/partial result) under real concurrent load, which is + // the failure mode a future refactor could plausibly introduce. + given: 'one ClassNode instance that every thread will resolve concurrently' + ClassNode sharedNode = new ClassNode('SharedWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + sharedNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + int threadCount = 32 + ExecutorService executor = Executors.newFixedThreadPool(threadCount) + CyclicBarrier barrier = new CyclicBarrier(threadCount) + + when: 'all threads race to resolve properties for the same instance at once' + List>> futures = (0.. + barrier.await(30, TimeUnit.SECONDS) + AstPropertyResolveUtils.getPropertyNames(sharedNode) + } as Callable>) + } + List> results = futures.collect { Future> future -> future.get(30, TimeUnit.SECONDS) } + + then: 'every thread observes the same, fully and correctly populated result - none sees a partial or corrupted map' + results.every { it == ['label'] } + + cleanup: + executor.shutdownNow() + } + + private static Expression mapExpressionOf(String key, ClassNode valueType) { + MapExpression mapExpression = new MapExpression() + mapExpression.addMapEntryExpression(new ConstantExpression(key), new ClassExpression(valueType)) + return mapExpression + } +} diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestMethodDecoratingTransformation.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestMethodDecoratingTransformation.groovy new file mode 100644 index 00000000000..4e69f300032 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestMethodDecoratingTransformation.groovy @@ -0,0 +1,72 @@ +/* + * 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.transform + +import org.codehaus.groovy.ast.AnnotationNode +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.MethodNode +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.ASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation + +/** + * Local, annotation-driven transformation used only to test {@link AbstractMethodDecoratingTransformation} + * through a genuine compilation. It simply renames the decorated method and dispatches straight to + * it - no wrapping closure, no transaction/tenant semantics - so tests can assert purely on which + * methods {@code weaveClassNode} chose to decorate. + * + * @see AbstractMethodDecoratingTransformationSpec + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class TestMethodDecoratingTransformation extends AbstractMethodDecoratingTransformation implements ASTTransformation { + + private static final ClassNode MY_TYPE = ClassHelper.make(ApplyTestMethodDecorating) + private static final Object APPLIED_MARKER = new Object() + + @Override + protected ClassNode getAnnotationType() { + MY_TYPE + } + + @Override + protected Object getAppliedMarker() { + APPLIED_MARKER + } + + @Override + protected String getRenamedMethodPrefix() { + '$test__' + } + + @Override + protected Expression buildDelegatingMethodCall(SourceUnit sourceUnit, AnnotationNode annotationNode, ClassNode classNode, + MethodNode methodNode, MethodCallExpression originalMethodCall, BlockStatement newMethodBody) { + originalMethodCall + } + + @Override + int priority() { + 0 + } +}