Merge pull request #9786 from Grails-Plugin-Consortium/master

Switching default for transactions in servics

grails-async/src/test/groovy/grails/async/PromiseSpec.groovy

@@ -81,7 +81,7 @@ class PromiseSpec extends Specification {
                 result = v
             }
 
-            sleep 200
+            sleep 400
         then:"The result is correct"
             result == [2,4]
 
@@ -92,7 +92,7 @@ class PromiseSpec extends Specification {
                 result = v
             }
 
-            sleep 200
+            sleep 400
         then:"The result is correct"
             result == [2,4]
 

grails-core/src/main/groovy/org/grails/core/DefaultGrailsServiceClass.java

@@ -31,25 +31,25 @@ public class DefaultGrailsServiceClass extends AbstractInjectableGrailsClass imp
     public DefaultGrailsServiceClass(Class<?> clazz) {
         super(clazz, SERVICE);
 
-        Object tmpTransactional = getPropertyOrStaticPropertyOrFieldValue(TRANSACTIONAL, Boolean.class);
-        transactional = tmpTransactional == null || tmpTransactional.equals(Boolean.TRUE);
+        Boolean tmpTransactional = getPropertyOrStaticPropertyOrFieldValue(TRANSACTIONAL, Boolean.class);
+        transactional = Boolean.TRUE.equals(tmpTransactional);
     }
 
     public boolean isTransactional() {
         return transactional;
     }
 
+    /**
+     * If service is transactional then get data source will always apply
+     *
+     * @return name of data source
+     */
     public String getDatasource() {
         if (datasourceName == null) {
-            if (isTransactional()) {
-                CharSequence name = getStaticPropertyValue(DATA_SOURCE, CharSequence.class);
-                datasourceName = name == null ? null : name.toString();
-                if (datasourceName == null) {
-                    datasourceName = DEFAULT_DATA_SOURCE;
-                }
-            }
-            else {
-                datasourceName = "";
+            CharSequence name = getStaticPropertyValue(DATA_SOURCE, CharSequence.class);
+            datasourceName = name == null ? null : name.toString();
+            if (datasourceName == null) {
+                datasourceName = DEFAULT_DATA_SOURCE;
             }
         }
 

grails-plugin-services/src/main/groovy/org/grails/plugins/services/ServicesGrailsPlugin.groovy

@@ -20,8 +20,6 @@ import grails.config.Settings
 import grails.plugins.Plugin
 import grails.util.GrailsUtil
 import groovy.transform.CompileStatic
-import org.grails.spring.context.support.MapBasedSmartPropertyOverrideConfigurer
-import org.springframework.context.ConfigurableApplicationContext
 
 import java.lang.reflect.Method
 import java.lang.reflect.Modifier
@@ -121,22 +119,49 @@ class ServicesGrailsPlugin extends Plugin  {
         serviceBeanAliasPostProcessor(ServiceBeanAliasPostProcessor)
     }}
 
+    /**
+     * Will use proxy in the case of service being transactional and the absence of the grails.transaction.Transactional.
+     * Using org.springframework.transaction.annotation.Transactional will lead to creation of proxy instead of AST transform.
+     * @param serviceClass
+     * @return
+     */
     @CompileStatic
-    boolean shouldCreateTransactionalProxy(GrailsServiceClass serviceClass) {
-        Class javaClass = serviceClass.clazz
-
+    private static boolean shouldCreateTransactionalProxy(GrailsServiceClass serviceClass) {
         try {
-            serviceClass.transactional &&
-              !AnnotationUtils.findAnnotation(javaClass, grails.transaction.Transactional) &&
-              !AnnotationUtils.findAnnotation(javaClass, Transactional) &&
-                 !javaClass.methods.any { Method m -> AnnotationUtils.findAnnotation(m, Transactional) != null ||
-                                                        AnnotationUtils.findAnnotation(m, grails.transaction.Transactional) != null}
+            return isTransactionalOrHasSpringTransactionalAnnotation(serviceClass) &&
+                    !hasGrailsTransactionalAnnotation(serviceClass)
         }
         catch (e) {
             return false
         }
     }
 
+    /**
+     * Determines if grails service class has grails.transactional.Transactional annotation present on class or methods
+     * @param serviceClass grails service class
+     * @return boolean
+     */
+    @CompileStatic
+    public static hasGrailsTransactionalAnnotation(GrailsServiceClass serviceClass){
+        Class javaClass = serviceClass.clazz
+        return AnnotationUtils.findAnnotation(javaClass, grails.transaction.Transactional) != null ||
+                javaClass.methods.any { Method m -> AnnotationUtils.findAnnotation(m, grails.transaction.Transactional) != null}
+    }
+
+    /**
+     * Determines if grails service class has org.springframework.transaction.annotation.Transactional annotation
+     * present on class or methods OR if service class has transactional = true static property
+     * @param serviceClass grails service class
+     * @return boolean
+     */
+    @CompileStatic
+    public static isTransactionalOrHasSpringTransactionalAnnotation(GrailsServiceClass serviceClass){
+        Class javaClass = serviceClass.clazz
+        return serviceClass.isTransactional() ||
+                AnnotationUtils.findAnnotation(javaClass, Transactional) != null ||
+                javaClass.methods.any { Method m -> AnnotationUtils.findAnnotation(m, Transactional) != null}
+    }
+
     void onChange(Map<String,Object> event) {
         if (!event.source || !applicationContext) {
             return

grails-test-suite-persistence/src/test/groovy/org/grails/plugins/services/ServicesGrailsPluginTests.groovy

@@ -1,11 +1,11 @@
 package org.grails.plugins.services
 
-import org.grails.web.servlet.context.support.WebRuntimeSpringConfiguration
+import grails.config.Settings
 import org.grails.commons.test.AbstractGrailsMockTests
 import org.grails.plugins.DefaultGrailsPlugin
 import org.grails.plugins.MockHibernateGrailsPlugin
+import org.grails.web.servlet.context.support.WebRuntimeSpringConfiguration
 import org.springframework.context.ApplicationContext
-import org.springframework.transaction.NoTransactionException
 
 class ServicesGrailsPluginTests extends AbstractGrailsMockTests {
 
@@ -38,35 +38,138 @@ class NonTransactionalService {
 
 @Transactional(readOnly = true)
 class SpringTransactionalService {
+
     def serviceMethod() {
-        def status = TransactionAspectSupport.currentTransactionStatus()
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
         return "hasTransaction = \${status!=null}"
     }
 }
 
 class PerMethodTransactionalService {
+
     @Transactional
     def methodOne() {
-        def status = TransactionAspectSupport.currentTransactionStatus()
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
         return "hasTransaction = \${status!=null}"
     }
 
     def methodTwo() {
-        def status = TransactionAspectSupport.currentTransactionStatus()
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
         return "hasTransaction = \${status!=null}"
     }
 }
+
+class TransactionalFalseService {
+
+    static transactional = false
+
+    def methodOne() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+
+    def methodTwo() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+}
+
+class TransactionalTrueService {
+
+    static transactional = true
+
+    def methodOne() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+
+    def methodTwo() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+}
+
+
+class TransactionalAbsentService {
+
+    def methodOne() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+
+    def methodTwo() {
+        def status = null
+        try { status = TransactionAspectSupport.currentTransactionStatus() } catch(e){ println e.message }
+        return "hasTransaction = \${status != null}"
+    }
+}
 """
     }
 
+    void testTransactionalFalseService() {
+        def appCtx = initializeContext()
+
+        def springService = appCtx.getBean("transactionalFalseService")
+        assertEquals "hasTransaction = false", springService.methodOne()
+        springService.methodTwo()
+    }
+
+    void testTransactionalFalseServiceConfigDisabled() {
+        def appCtx = initializeContext(false)
+
+        def springService = appCtx.getBean("transactionalFalseService")
+        assertEquals "hasTransaction = false", springService.methodOne()
+        springService.methodTwo()
+    }
+
+    void testTransactionalTrueService() {
+        def appCtx = initializeContext()
+
+        def springService = appCtx.getBean("transactionalTrueService")
+        assertEquals "hasTransaction = true", springService.methodOne()
+        assertEquals "hasTransaction = true", springService.methodTwo()
+    }
+
+    void testTransactionalTrueServiceConfigDisabled() {
+        def appCtx = initializeContext(false)
+
+        def springService = appCtx.getBean("transactionalTrueService")
+        assertEquals "hasTransaction = false", springService.methodOne()
+        assertEquals "hasTransaction = false", springService.methodTwo()
+    }
+
+    void testTransactionalAbsentService() {
+        def appCtx = initializeContext()
+
+        def springService = appCtx.getBean("transactionalAbsentService")
+        assertEquals "hasTransaction = false", springService.methodOne()
+        springService.methodTwo()
+    }
+
     void testPerMethodTransactionAnnotations() {
         def appCtx = initializeContext()
 
         def springService = appCtx.getBean("perMethodTransactionalService")
         assertEquals "hasTransaction = true", springService.methodOne()
-        shouldFail(NoTransactionException) {
-            springService.methodTwo()
-        }
+
+        //Given one method having annotation, then the whole service will be be proxied
+        assertEquals "hasTransaction = true", springService.methodTwo()
+    }
+
+    void testPerMethodTransactionAnnotationsConfigDisabled() {
+        def appCtx = initializeContext(false)
+
+        def springService = appCtx.getBean("perMethodTransactionalService")
+        assertEquals "hasTransaction = false", springService.methodOne()
+        assertEquals "hasTransaction = false", springService.methodTwo()
     }
 
     void testSpringConfiguredService() {
@@ -76,6 +179,13 @@ class PerMethodTransactionalService {
         assertEquals "hasTransaction = true", springService.serviceMethod()
     }
 
+    void testSpringConfiguredServiceConfigDisabled() {
+        def appCtx = initializeContext(false)
+
+        def springService = appCtx.getBean("springTransactionalService")
+        assertEquals "hasTransaction = false", springService.serviceMethod()
+    }
+
     void testServicesPlugin() {
         def appCtx = initializeContext()
 
@@ -84,10 +194,12 @@ class PerMethodTransactionalService {
         assertTrue appCtx.containsBean("nonTransactionalService")
     }
 
-    private ApplicationContext initializeContext() {
+    private ApplicationContext initializeContext(boolean transactionManagement = true) {
 
+        ga.getConfig().put(Settings.SPRING_TRANSACTION_MANAGEMENT, transactionManagement)
+        ga.getConfig().put("dataSources", [dataSource: [:]])
         def corePluginClass = gcl.loadClass("org.grails.plugins.CoreGrailsPlugin")
-        def corePlugin = new DefaultGrailsPlugin(corePluginClass,ga)
+        def corePlugin = new DefaultGrailsPlugin(corePluginClass, ga)
         def dataSourcePluginClass = gcl.loadClass("org.grails.plugins.datasource.DataSourceGrailsPlugin")
 
         def domainPluginClass = gcl.loadClass("org.grails.plugins.domain.DomainClassGrailsPlugin")
@@ -105,8 +217,10 @@ class PerMethodTransactionalService {
 
         def pluginClass = gcl.loadClass("org.grails.plugins.services.ServicesGrailsPlugin")
         def plugin = new DefaultGrailsPlugin(pluginClass, ga)
+
         plugin.doWithRuntimeConfiguration(springConfig)
 
+
         springConfig.getApplicationContext()
     }
 }