Add test coverage and fix real bugs in grails-datamapping-core's services package

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractDetachedCriteriaServiceImplementor.groovy

@@ -123,6 +123,9 @@ abstract class AbstractDetachedCriteriaServiceImplementor extends AbstractReadOp
         }
     }
 
+    // domainClassNode is unused here, but kept so subclasses can override this as a
+    // polymorphic extension point and pick a DetachedCriteria type based on the domain class
+    @SuppressWarnings(['unused', 'MethodMayBeStatic'])
     protected ClassNode getDetachedCriteriaType(ClassNode domainClassNode) {
         DETACHED_CRITERIA
     }
@@ -150,6 +153,9 @@ abstract class AbstractDetachedCriteriaServiceImplementor extends AbstractReadOp
      * Whether lookup by id is allowed by this implementation
      * @return True if it is
      */
+    // Not static: AbstractProjectionImplementer overrides this to return false, and
+    // doImplement() dispatches on it polymorphically. A static method would break that override.
+    @SuppressWarnings('MethodMayBeStatic')
     protected boolean lookupById() {
         return true
     }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractSaveImplementer.groovy

@@ -51,7 +51,7 @@ import static org.grails.datastore.gorm.transform.AstMethodDispatchUtils.namedAr
 @CompileStatic
 abstract class AbstractSaveImplementer extends AbstractWriteOperationImplementer {
 
-    protected Statement bindParametersAndSave(ClassNode domainClassNode, MethodNode abstractMethodNode, MethodNode newMethodNode, Parameter[] parameters, BlockStatement body, VariableExpression entityVar) {
+    protected Statement bindParametersAndSave(ClassNode domainClassNode, MethodNode abstractMethodNode, Parameter[] parameters, BlockStatement body, VariableExpression entityVar) {
         Expression argsExpression = null
 
         for (Parameter parameter in parameters) {
@@ -64,8 +64,8 @@ abstract class AbstractSaveImplementer extends AbstractWriteOperationImplementer
                 argsExpression = varX(parameter)
             } else {
                 AstUtils.error(
-                        newMethodNode.declaringClass.module.context,
-                        newMethodNode,
+                        abstractMethodNode.declaringClass.module.context,
+                        abstractMethodNode,
                         "Cannot implement method for argument [${parameterName}]. No property exists on domain class [$domainClassNode.name]"
                 )
             }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractServiceImplementer.groovy

@@ -21,28 +21,24 @@ package org.grails.datastore.gorm.services.implementers
 
 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.Expression
 import org.codehaus.groovy.transform.trait.Traits
 
 import grails.gorm.multitenancy.TenantService
-import grails.gorm.transactions.TransactionService
 import org.grails.datastore.gorm.GormEnhancer
 import org.grails.datastore.gorm.multitenancy.transform.TenantTransform
 import org.grails.datastore.gorm.services.ServiceImplementer
 import org.grails.datastore.gorm.transactions.transform.TransactionalTransform
-import org.grails.datastore.gorm.transform.AstMethodDispatchUtils
 import org.grails.datastore.gorm.transform.AstPropertyResolveUtils
 import org.grails.datastore.mapping.core.Ordered
 import org.grails.datastore.mapping.model.config.GormProperties
 import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
 import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore
 import org.grails.datastore.mapping.reflect.AstUtils
 import org.grails.datastore.mapping.services.ServiceRegistry
-import org.grails.datastore.mapping.transactions.TransactionCapableDatastore
 
 import static org.codehaus.groovy.ast.ClassHelper.make
 import static org.codehaus.groovy.ast.tools.GeneralUtils.args
@@ -116,7 +112,7 @@ abstract class AbstractServiceImplementer implements PrefixedServiceImplementer,
      * @return True if it is
      */
     protected boolean isValidParameter(ClassNode domainClassNode, Parameter parameter, String parameterName) {
-        if (GormProperties.IDENTITY.equals(parameterName)) {
+        if (GormProperties.IDENTITY == parameterName) {
             return true
         }
         else {
@@ -135,18 +131,11 @@ abstract class AbstractServiceImplementer implements PrefixedServiceImplementer,
         return propX(varX('this'), 'targetDatastore')
     }
 
-    /**
-     * @return The datastore expression
-     */
-    protected Expression transactionalDatastore() {
-        return castX(ClassHelper.make(TransactionCapableDatastore), propX(varX('this'), 'targetDatastore'))
-    }
-
     /**
      * @return The datastore expression
      */
     protected Expression multiTenantDatastore() {
-        return castX(ClassHelper.make(MultiTenantCapableDatastore), propX(varX('this'), 'targetDatastore'))
+        return castX(make(MultiTenantCapableDatastore), propX(varX('this'), 'targetDatastore'))
     }
 
     /**
@@ -156,16 +145,9 @@ abstract class AbstractServiceImplementer implements PrefixedServiceImplementer,
         return callD(ServiceRegistry, 'targetDatastore', 'getService', classX(make(TenantService)))
     }
 
-    /**
-     * @return The transaction service
-     */
-    protected Expression transactionService() {
-        return callD(ServiceRegistry, 'targetDatastore', 'getService', classX(make(TransactionService)))
-    }
-
     protected Expression findConnectionId(MethodNode methodNode) {
         if (TenantTransform.hasTenantAnnotation(methodNode)) {
-            return callD(classX(ClassHelper.make(MultiTenancySettings)), 'resolveConnectionForTenantId', args(
+            return callD(classX(make(MultiTenancySettings)), 'resolveConnectionForTenantId', args(
                 propX(multiTenantDatastore(), 'multiTenancyMode'), callD(tenantService(), 'currentId')
             ))
         }
@@ -180,27 +162,17 @@ abstract class AbstractServiceImplementer implements PrefixedServiceImplementer,
     }
 
     protected Expression buildInstanceApiLookup(ClassNode domainClass, Expression connectionId) {
-        return AstMethodDispatchUtils.callD(
+        return callD(
             classX(GormEnhancer), 'findInstanceApi', args(classX(domainClass), connectionId)
         )
     }
 
     protected Expression buildStaticApiLookup(ClassNode domainClass, Expression connectionId) {
-        return AstMethodDispatchUtils.callD(
+        return callD(
                 classX(GormEnhancer), 'findStaticApi', args(classX(domainClass), connectionId)
         )
     }
 
-    protected Expression findInstanceApiForConnectionId(ClassNode domainClass, MethodNode methodNode) {
-        Expression connectionId = findConnectionId(methodNode)
-        if (connectionId != null) {
-            return buildInstanceApiLookup(domainClass, connectionId)
-        }
-        else {
-            return classX(domainClass.plainNodeReference)
-        }
-    }
-
     protected Expression findStaticApiForConnectionId(ClassNode domainClass, MethodNode methodNode) {
         Expression connectionId = findConnectionId(methodNode)
         if (connectionId != null) {

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractWhereImplementer.groovy

@@ -117,6 +117,9 @@ abstract class AbstractWhereImplementer extends AbstractReadOperationImplementer
         }
     }
 
+    // domainClassNode is unused here, but kept so subclasses can override this as a
+    // polymorphic extension point and pick a DetachedCriteria type based on the domain class
+    @SuppressWarnings(['unused', 'MethodMayBeStatic'])
     protected ClassNode getDetachedCriteriaType(ClassNode domainClassNode) {
         ClassHelper.make(DetachedCriteria)
     }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/DeleteImplementer.groovy

@@ -64,7 +64,7 @@ class DeleteImplementer extends AbstractDetachedCriteriaServiceImplementor imple
 
     @Override
     protected boolean isCompatibleReturnType(ClassNode domainClass, MethodNode methodNode, ClassNode returnType, String prefix) {
-        return ClassHelper.VOID_TYPE.equals(returnType) || AstUtils.isSubclassOfOrImplementsInterface(returnType, Number.name)
+        return ClassHelper.VOID_TYPE == returnType || AstUtils.isSubclassOfOrImplementsInterface(returnType, Number.name)
     }
 
     @Override
@@ -79,7 +79,7 @@ class DeleteImplementer extends AbstractDetachedCriteriaServiceImplementor imple
 
     @Override
     void implementById(ClassNode domainClassNode, MethodNode abstractMethodNode, MethodNode newMethodNode, ClassNode targetClassNode, BlockStatement body, Expression byIdLookup) {
-        boolean isVoidReturnType = ClassHelper.VOID_TYPE.equals(newMethodNode.returnType)
+        boolean isVoidReturnType = ClassHelper.VOID_TYPE == newMethodNode.returnType
         VariableExpression obj = varX('$obj')
         Expression connectionId = findConnectionId(abstractMethodNode)
         Statement deleteStatement
@@ -115,7 +115,7 @@ class DeleteImplementer extends AbstractDetachedCriteriaServiceImplementor imple
     void implementWithQuery(ClassNode domainClassNode, MethodNode abstractMethodNode, MethodNode newMethodNode, ClassNode targetClassNode, BlockStatement body, VariableExpression detachedCriteriaVar, Expression queryArgs) {
 
         MethodCallExpression deleteCall = callX(detachedCriteriaVar, 'deleteAll')
-        boolean isVoidReturnType = ClassHelper.VOID_TYPE.equals(newMethodNode.returnType)
+        boolean isVoidReturnType = ClassHelper.VOID_TYPE == newMethodNode.returnType
 
         body.addStatements([
                 // return query.deleteAll()

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/DeleteWhereImplementer.groovy

@@ -58,7 +58,7 @@ class DeleteWhereImplementer extends AbstractWhereImplementer {
 
     @Override
     protected boolean isCompatibleReturnType(ClassNode domainClass, MethodNode methodNode, ClassNode returnType, String prefix) {
-        return ClassHelper.VOID_TYPE.equals(returnType) || AstUtils.isSubclassOfOrImplementsInterface(returnType, Number.name)
+        return ClassHelper.VOID_TYPE == returnType || AstUtils.isSubclassOfOrImplementsInterface(returnType, Number.name)
     }
 
     @Override

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindAllByImplementer.groovy

@@ -36,7 +36,6 @@ import org.grails.datastore.gorm.finders.DynamicFinder
 import org.grails.datastore.gorm.finders.MatchSpec
 import org.grails.datastore.gorm.services.transform.ServiceTransformation
 import org.grails.datastore.mapping.core.Ordered
-import org.grails.datastore.mapping.reflect.AstUtils
 
 import static org.codehaus.groovy.ast.ClassHelper.MAP_TYPE
 import static org.codehaus.groovy.ast.tools.GeneralUtils.args
@@ -109,7 +108,7 @@ class FindAllByImplementer extends AbstractArrayOrIterableResultImplementer impl
         }
 
         if (matchSpec == null) {
-            AstUtils.error(abstractMethodNode.declaringClass.module.context, abstractMethodNode, ServiceTransformation.NO_IMPLEMENTATIONS_MESSAGE)
+            error(abstractMethodNode.declaringClass.module.context, abstractMethodNode, ServiceTransformation.NO_IMPLEMENTATIONS_MESSAGE)
         }
         else {
             // validate the properties

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneImplementer.groovy

@@ -75,6 +75,11 @@ class FindOneImplementer extends AbstractDetachedCriteriaServiceImplementor impl
         )
     }
 
+    // domainClassNode/newMethodNode are unused here, but kept so subclasses
+    // (e.g. FindAndDeleteImplementer, FindOneInterfaceProjectionImplementer) can
+    // override this as a polymorphic extension point and vary the query method
+    // by domain class/method
+    @SuppressWarnings('unused')
     protected String findMethodToInvoke(ClassNode domainClassNode, MethodNode newMethodNode) {
         'find'
     }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneStringQueryImplementer.groovy

@@ -66,6 +66,10 @@ class FindOneStringQueryImplementer extends AbstractStringQueryImplementer imple
         )
     }
 
+    // classNode/methodNode are unused here, but kept so subclasses (e.g.
+    // FindOneInterfaceProjectionStringQueryImplementer) can override this as a
+    // polymorphic extension point and vary the query method by domain class/method
+    @SuppressWarnings('unused')
     protected String getFindMethodToInvoke(ClassNode classNode, MethodNode methodNode, ClassNode returnType) {
         if (AstUtils.isDomainClass(returnType)) {
             return 'find'

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/InterfaceProjectionBuilder.groovy

@@ -60,26 +60,39 @@ trait InterfaceProjectionBuilder {
     @Generated
     boolean isInterfaceProjection(ClassNode domainClass, MethodNode methodNode, ClassNode returnType) {
         if (returnType.isInterface() && !returnType.packageName?.startsWith('java.')) {
-            List<String> interfacePropertyNames = AstPropertyResolveUtils.getPropertyNames(returnType)
+            return hasCompatibleProperties(domainClass, returnType)
+        }
+        return false
+    }
 
-            for (prop in interfacePropertyNames) {
-                ClassNode existingType = AstPropertyResolveUtils.getPropertyType(domainClass, prop)
-                ClassNode propertyType = AstPropertyResolveUtils.getPropertyType(returnType, prop)
-                if (existingType == null) {
-                    return false
-                }
-                else if (!AstUtils.isSubclassOfOrImplementsInterface(existingType, propertyType)) {
-                    return false
-                }
+    /**
+     * Whether every property declared by the candidate interface has a compatible
+     * property of the same name on the domain class
+     *
+     * @param domainClass The domain class
+     * @param candidateType The interface being considered as a projection
+     * @return True if every property is compatible
+     */
+    @Generated
+    boolean hasCompatibleProperties(ClassNode domainClass, ClassNode candidateType) {
+        List<String> interfacePropertyNames = AstPropertyResolveUtils.getPropertyNames(candidateType)
+
+        for (prop in interfacePropertyNames) {
+            ClassNode existingType = AstPropertyResolveUtils.getPropertyType(domainClass, prop)
+            ClassNode propertyType = AstPropertyResolveUtils.getPropertyType(candidateType, prop)
+            if (existingType == null) {
+                return false
+            }
+            else if (!AstUtils.isSubclassOfOrImplementsInterface(existingType, propertyType)) {
+                return false
             }
-            return true
         }
-        return false
+        return true
     }
 
     @Generated
     MethodNode buildInterfaceImpl(ClassNode interfaceNode, ClassNode declaringClass, ClassNode targetDomainClass, MethodNode abstractMethodNode) {
-        List<Expression> getterNames = (List<Expression>) AstPropertyResolveUtils.getPropertyNames(interfaceNode)
+        List<Expression> getterNames = (List<Expression>) (List) AstPropertyResolveUtils.getPropertyNames(interfaceNode)
                 .collect() {
                     new ConstantExpression(NameUtils.getGetterName(it))
                 }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/IterableInterfaceProjectionBuilder.groovy

@@ -31,7 +31,6 @@ import org.codehaus.groovy.ast.expr.VariableExpression
 import org.codehaus.groovy.ast.stmt.Statement
 
 import org.grails.datastore.gorm.services.ServiceImplementer
-import org.grails.datastore.gorm.transform.AstPropertyResolveUtils
 import org.grails.datastore.mapping.reflect.AstGenericsUtils
 import org.grails.datastore.mapping.reflect.AstUtils
 
@@ -71,20 +70,7 @@ trait IterableInterfaceProjectionBuilder extends InterfaceProjectionBuilder {
         if (AstUtils.isSubclassOfOrImplementsInterface(returnType, Iterable.name) || returnType.isArray()) {
             ClassNode genericType = AstGenericsUtils.resolveSingleGenericType(returnType)
             if (genericType != null && genericType.isInterface() && !genericType.packageName?.startsWith('java.')) {
-
-                List<String> interfacePropertyNames = AstPropertyResolveUtils.getPropertyNames(genericType)
-
-                for (prop in interfacePropertyNames) {
-                    ClassNode existingType = AstPropertyResolveUtils.getPropertyType(domainClass, prop)
-                    ClassNode propertyType = AstPropertyResolveUtils.getPropertyType(genericType, prop)
-                    if (existingType == null) {
-                        return false
-                    }
-                    else if (!AstUtils.isSubclassOfOrImplementsInterface(existingType, propertyType)) {
-                        return false
-                    }
-                }
-                return true
+                return hasCompatibleProperties(domainClass, genericType)
             }
         }
         return false

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/SaveImplementer.groovy

@@ -84,7 +84,7 @@ class SaveImplementer extends AbstractSaveImplementer implements SingleResultSer
                 declS(entityVar, ctorX(domainClassNode))
             )
             body.addStatement(
-                bindParametersAndSave(domainClassNode, abstractMethodNode, newMethodNode, parameters, body, entityVar)
+                bindParametersAndSave(domainClassNode, abstractMethodNode, parameters, body, entityVar)
             )
 
         }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/UpdateOneImplementer.groovy

@@ -90,7 +90,8 @@ class UpdateOneImplementer extends AbstractSaveImplementer implements SingleResu
             declS(entityVar, lookupCall)
         )
         BlockStatement ifBody = block()
-        Statement saveStmt = bindParametersAndSave(domainClassNode, abstractMethodNode, newMethodNode, parameters[1..-1] as Parameter[], ifBody, entityVar)
+        Parameter[] propertyParameters = Arrays.copyOfRange(parameters, 1, parameters.length)
+        Statement saveStmt = bindParametersAndSave(domainClassNode, abstractMethodNode, propertyParameters, ifBody, entityVar)
         ifBody.addStatement(saveStmt)
         body.addStatement(
             ifS(notNullX(entityVar),