Grails 3.1 Transactional Behavior & Documentation Appear Incorrect

[ctoestreich]

The current documentation for grails 3.1.x states that transactions are disabled by default for services and can be enabled using grails.spring.transactionManagement: true in yaml. However the property appears to actually be grails.spring.transactionManagement.proxies AND it appears to be true by default as seen here https://github.com/grails/grails-core/search?utf8=%E2%9C%93&q=SPRING_TRANSACTION_MANAGEMENT

We were able to get the default for transactions to be off by setting grails.spring.transactionManagement.proxies: false in our yaml. After doing this we were seeing the correct number of transactions only for those methods annotated with @transactional.

Note the incorrect property setting here https://grails.github.io/grails-doc/latest/guide/services.html

Line where property is defaulting for all services to true:

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

Line 56 in 623d575

final boolean springTransactionManagement = config.getProperty(Settings.SPRING_TRANSACTION_MANAGEMENT, Boolean.class, true)

This appears to go back to DefaultGrailsServiceClass.java where a service with no annotation or static transactional is being wired as transactional true due to transactional = tmpTransactional == null

 public DefaultGrailsServiceClass(Class<?> clazz) {
        super(clazz, SERVICE);

        Object tmpTransactional = getPropertyOrStaticPropertyOrFieldValue(TRANSACTIONAL, Boolean.class);
        transactional = tmpTransactional == null || tmpTransactional.equals(Boolean.TRUE);
    }

then springTransactionManagement is defaulting to true via config.getProperty(Settings.SPRING_TRANSACTION_MANAGEMENT, Boolean.class, true)

Then the logic fires at shouldCreateTransactionalProxy and the evaluation for the logical condition returns true because all these evaluate to true where ALL services with no static transactional are serviceClass.transactional = true have no annotation !AnnotationUtils.findAnnotation(javaClass, grails.transaction.Transactional) = true have no spring annotation !AnnotationUtils.findAnnotation(javaClass, Transactional) = true and no method has spring of grails transactional !javaClass.methods.any { Method m -> AnnotationUtils.findAnnotation(m, Transactional) != null || AnnotationUtils.findAnnotation(m, grails.transaction.Transactional) != null} = true

This is the existing block:

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}

Due to how it checks for transactions simply using grails.spring.transactionManagement.proxies: false works, however this is incorrect in the documentation and the default value is currently true for this value when not provided.

Newly generated projects come ouf the gate with

grails:
    spring:
        transactionManagement: false

but this doesn't actually appear to affect the transactional behavior.

[ctoestreich]

Will be sending pull request following discussion with @jeffbrown using the logic as

if(service has @grails.transaction.Transactional) {
   make it transactional with the AST transformation
} else if(service has org.springframework.transaction.annotation.Transactional ||
          service has transactional=true) {
   make it transactional with a proxy
} else if(grails.spring.transactionManagement.proxies=true &&
          !(service has transactional=false)){
   make it transactional with a proxy
} else {
   not transactional
}

[osscontributor]

I think the logic quoted above is reasonable. Feedback is welcome.

[osscontributor]

I think the proposed change...

serviceClass.transactional && 
        (AnnotationUtils.findAnnotation(javaClass, grails.transaction.Transactional) != null || 
                AnnotationUtils.findAnnotation(javaClass, Transactional) != null || 
                javaClass.methods.any { Method m -> 
                    AnnotationUtils.findAnnotation(m, Transactional) != null || AnnotationUtils.findAnnotation(m, grails.transaction.Transactional) != null 
                }
        )

...will yield the wrong behavior. I think that will create the proxy when the annotation is present.

[ctoestreich]

Yeah that is wrong. Understanding how the static property and annotation work has cleared that up.

[osscontributor]

Just to restate what was described in Slack, I think the only issue here is the config property name and its default value. Is that correct?

[ctoestreich]

Yes, two line change minus tests.

[osscontributor]

I am wondering if we should leave the default config property value as is for 3.1.x and only change it for 3.2 since it is a real change in behavior that could be problematic.

[osscontributor]

I would like @graemerocher's input before we merge any PRs. He is on vacation this week.

[ctoestreich]

I can pull against master in that case. I can PR again against 3.1.x later.

[osscontributor]

I think that is fine. If you make it behave as described in the logic you quoted above and make that into a PR against master, then we can take it from there.

Thanks for all of the help.

[ctoestreich]

The code change was easy... the tests are proving to be a !@#$.

[ctoestreich]

It is actually the datasourceName in the DefaultGrailsServiceClass that is causing some issues when calling getDatasource that is proving difficult.

[osscontributor]

It is actually the datasourceName in the DefaultGrailsServiceClass that is causing some issues when calling getDatasource that is proving difficult.

Is that relevant to the transactional behavior and the docs being out of sync?

[ctoestreich]

I have a work around but the data source method used to work one way when the static property for transactional was null or true. Now if we only detect true it changes the behavior of the ultimate name of the datasource. It would be bad for to inspect annotations in core so I will have to reevaluate and perhaps set transactional during service wiring when annotations present and proxy intended. Will post code here in a bit.

[ctoestreich]

Given the attachment of the datasource name to the service artifact, and it's apparent dependency to knowing the transactionality of the service, even if removing the static property this will probably be necessary.

[ctoestreich]

I probably need to digest how the datasource, the service artifact and transactions all play together a bit more.

[ctoestreich]

The logic that I landed on is as follows

if(service has @grails.transaction.Transactional) {
   make it transactional with the AST transformation
} else if((service has org.springframework.transaction.annotation.Transactional ||
          service has transactional=true) && grails.spring.transactionManagement.proxies=true) {
   make it transactional with a proxy
} else {
   not transactional
}

[osscontributor]

if(service has @grails.transaction.Transactional) {
   make it transactional with the AST transformation
} else if((service has org.springframework.transaction.annotation.Transactional ||
          service has transactional=true) && grails.spring.transactionManagement.proxies=true) {
   make it transactional with a proxy
} else {
   not transactional
}

I think that would mean that if grails.spring.transactionManagement.proxies==false and the service has org.springframework.transaction.annotation.Transactional that no proxy will be generated. I think that is the wrong thing to do, but more importantly that would be a difficult thing to do because I think Spring is going to create the proxy automatically because of the annotation.

[ctoestreich]

We have observed in 3.1.0 when setting grails.spring.transactionManagement.proxies: false that our transaction count goes way down and the speed goes way up. I understand your concern. I think there needs to be clarity on what the real intent of the config value is. Is it to enable property interrogation of proxy wiring or to disable spring proxy wiring.

[osscontributor]

Is it to enable property interrogation of proxy wiring or to disable spring proxy wiring.

Even if you wanted it to prevent proxies for classes marked with org.springframework.transaction.annotation.Transactional, how would you go about making that happen?

[osscontributor]

Is the proposal to remove (or maybe just not to add) the annotation driven transaction management post processor stuff if the config setting is set to false?

[ctoestreich]

I didn't run through the full spring wiring scenario but the code in the PR will skip wiring the TypeSpecifyableTransactionProxyFactoryBean if the comfig property is set to false. It would appear, based on the tests, that this works on the surface. But I defer to you, having more experience in spring code than I, if that isn't sufficient.

[osscontributor]

I don't think skipping the wiring of a TypeSpecifyableTransactionProxyFactoryBean is enough to prevent Spring from doing its normal thing with org.springframework.transaction.annotation.Transactional. I think we were being explicit about using TypeSpecifyableTransactionProxyFactoryBean because that is a way to tell Spring to create a proxy for a class that isn't marked with the annotation.

[osscontributor]

I think it might work if we omitted adding the post processor but I am not sure that is the right thing. Maybe it is.

My thinking is that if the author of a class explicitly marks a class with org.springframework.transaction.annotation.Transactional that they are being explicit in saying "I want to use the Spring proxy".

[ctoestreich]

I think the best course is the simplest and to adjust the config value to simply toggle the support for the static property.

[ctoestreich]

Will update PR tomorrow am.

[osscontributor]

The code at

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

Line 56 in 57df0a1

final boolean springTransactionManagement = config.getProperty(Settings.SPRING_TRANSACTION_MANAGEMENT, Boolean.class, true)

looks like this...

final boolean springTransactionManagement = config.getProperty(Settings.SPRING_TRANSACTION_MANAGEMENT, Boolean.class, true)

if(springTransactionManagement) {
    xmlns tx:"http://www.springframework.org/schema/tx"
    tx.'annotation-driven'('transaction-manager':'transactionManager')
}

That appears to be written with the intent of not registering the annotation driven transaction manager contraption if the config setting is set to false. I think that ignoring the Spring annotation altogether when the config setting is false is potentially problematic. Maybe I am the minority with that concern though.

[osscontributor]

An issue I see is that as the author of a plugin I may be explicit in saying I want the Spring proxy. I do that by using the Spring annotation. If that plugin is used in an app that sets the proxy setting to false, bad things could happen. So far I think I am outnumbered though.

[osscontributor]

The more I think about this, I am backing away from my own position above. I think the intent may be to disable the proxying altogether when setting the config setting to false and the motivation for doing that may be to optimize startup time.

[ctoestreich]

We no longer use any spring annotations in favor of the grails annotation and disabling the default transactionality via the config flag has given us a HUGE boost in performance. I will defer this issue to you and your minions, but wanted to throw that out there.

[osscontributor]

We no longer use any spring annotations in favor of the grails annotation and disabling the default transactionality via the config flag has given us a HUGE boost in performance. I will defer this issue to you and your minions, but wanted to throw that out there.

That all makes sense to me.