Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

javax.transaction.Transactional does not work at the individual method level #1293

Closed
Jim-Harrington opened this issue Jan 25, 2022 · 24 comments
Labels
type: improvement A minor improvement to an existing feature
Milestone

Comments

@Jim-Harrington
Copy link

Expected Behavior

That methods carrying the javax.transaction.Transactional annotation run in a transaction with a configuration matching the parameters specified in the annotation and that methods not carrying the javax.transaction.Transactional successfully run outside a transaction context.

javax.transaction.Transactional is clearly intended to be a method level annotation, so it must be possible to non-transactional methods on a class.

Actual Behaviour

Methods not carrying the javax.transaction.Transactional annotation fail with:

io.micronaut.transaction.exceptions.NoTransactionException: No current transaction present. Consider declaring @Transactional on the surrounding method

Steps To Reproduce

As an example, I modified the test case: data-tx:io.micronaut.transaction.jdbc.TransactionAnnotationSpec as follows:

dhcp-10-39-204-9:data-tx GK4J0B$ git diff 
diff --git a/data-tx/src/test/groovy/io/micronaut/transaction/jdbc/TransactionAnnotationSpec.groovy b/data-tx/src/test/groovy/io/micronaut/transaction/jdbc/TransactionAnnotationSpec.groovy
index 86bf641a3..5d8510202 100644
--- a/data-tx/src/test/groovy/io/micronaut/transaction/jdbc/TransactionAnnotationSpec.groovy
+++ b/data-tx/src/test/groovy/io/micronaut/transaction/jdbc/TransactionAnnotationSpec.groovy
@@ -60,6 +60,11 @@ class TransactionAnnotationSpec extends Specification {
         testService.readTransactionally() == 2
         testService.lastEvent
 
+        when:"A non-transactional method is called"
+        def z = testService.readNonTransactionally()
+
+        then:
+        z == 2
 
         when:"Test that connections are never exhausted"
         int i = 0
@@ -133,6 +138,19 @@ class TransactionAnnotationSpec extends Specification {
 
         }
 
+        int readNonTransactionally() {
+            def ps2 = connection.prepareStatement("select count(*) as count from book")
+            def rs = ps2.executeQuery()
+            try {
+                rs.next()
+                rs.getInt("count")
+            } finally {
+                ps2.close()
+                rs.close()
+            }
+
+        }
+

and get this when running:

  io.micronaut.transaction.exceptions.NoTransactionException: No current transaction present. Consider declaring @Transactional on the surrounding method
      at app//io.micronaut.transaction.jdbc.TransactionalConnectionInterceptor.intercept(TransactionalConnectionInterceptor.java:65)
      at app//io.micronaut.aop.chain.MethodInterceptorChain.proceed(MethodInterceptorChain.java:137)
      at app//io.micronaut.transaction.jdbc.TransactionAnnotationSpec$TestService.readNonTransactionally(TransactionAnnotationSpec.groovy:142)
      at io.micronaut.transaction.jdbc.TransactionAnnotationSpec.test transactional annotation handling(TransactionAnnotationSpec.groovy:64)
  Caused by: io.micronaut.transaction.jdbc.exceptions.CannotGetJdbcConnectionException: No current JDBC Connection found. Consider wrapping this call in transactional boundaries.
      at app//io.micronaut.transaction.jdbc.DataSourceUtils.doGetConnection(DataSourceUtils.java:135)
      at app//io.micronaut.transaction.jdbc.DataSourceUtils.getConnection(DataSourceUtils.java:93)
      at app//io.micronaut.transaction.jdbc.TransactionalConnectionInterceptor.intercept(TransactionalConnectionInterceptor.java:63)
      ... 3 more

Environment Information

Mac OS
JDK 11

Example Application

No response

Version

micronaut parent 3.2.1, but I tried version 3.2.7 of the parent and got the same result

@dstepanov
Copy link
Contributor

We would need to have some annotation like @ConnectionAdvice that would open/close the connection.

@Jim-Harrington
Copy link
Author

Couldn't the interceptor just bypass the single connection/transactional data source when a call to get a connection comes outside a transaction context?

The error shows that we know we're outside a transaction context.

@dstepanov
Copy link
Contributor

The error might be misleading, we don't need to have a transaction but we do need to have a connection.

Right now we have an interceptor of the Connection interface that delegates to the existing open connection that is initialized when the transactional annotation is added.

@Jim-Harrington
Copy link
Author

Well, in my case the error diagnoses the problem correctly (not in a transaction), but the resolution advice is just plain wrong. It is absolutely reasonable, given the accepted usage of javax.transaction.Transactional, to have non-transactional methods on a class that also has transactional methods.

As a workaround for the lack of a connection, we found that using io.micronaut.transaction.annotation.ReadOnly on non-transactional methods avoids the error. So, at an absolute minimum, the advice the exception gives needs to mentioned this option.

That said, the implementation around javax.transaction.Transactional is still not correct in that the effect of setting a transaction context on a method extends way beyond the bounds of the annotated method.

@dstepanov
Copy link
Contributor

Transaction advice doesn’t affect other methods. You are injecting a connection which in the current implementation delegates to the connection from the transaction, that’s why you get an exception when it’s missing. In any case there needs to be some aspect that would open and close the data source connection.

Different people requested for JDBC implementation to be used without transactions around, so I want to introduce a connection aspect for that.

@matt-snider
Copy link

@dstepanov I came across this while trying to get to the bottom of transaction issues described in micronaut-sql#558. Essentially I had problems getting @Transactional to work without Spring, but it turns out it was working all along, just the error (same as in this ticket) led me to the conclusion that something was misconfigured.

I created a sample project and in the TestController I mix @Transactional and non-transactional methods.

I'm not sure what to take away from the above conversation. Is this not possible or should I just be doing it differently? I recently upgraded to Micronaut 3 and before that I'm quite sure this was working (albeit with transaction management from Spring). Any tips would be appreciated!

@dstepanov
Copy link
Contributor

@matt-snider Can you please provide a test inside of the project with testcontainers.

@matt-snider
Copy link

@dstepanov I can provide this if you think it's necessary. There are steps to reproduce in the comment above the controller. It only takes 3 curl requests so reproduction is very straight forward.

To be clear, I'm willing to write the tests I just want to make sure you actually need this before I spend the time. I need to fix the error I'm getting on test ("Could not create task ':testNativeImage'. The value for this property is final and cannot be changed any further.") and figure out how to use testcontainers, since I've never used them.

@graemerocher
Copy link
Contributor

we need a reproducer yes

@dstepanov
Copy link
Contributor

@matt-snider Yes please, I prefer a reproducer that doesn’t require me running some db locally. You might try to use the latest micronaut plugin

@matt-snider
Copy link

@dstepanov @graemerocher Sure, I will provide that. Did you look at the example controller though? It just runs H2 so there is no need to run any db locally

@matt-snider
Copy link

@dstepanov I added a test to the repo linked above. I didn't use test containers because the issue is reproducible with H2.

@dstepanov
Copy link
Contributor

@matt-snider Sorry I didn't realize it on the first look. Your issue is the same as that of the reporter, Connection is at this moment being intercepted re-delegated to the connection bound for the transaction (in TransactionalConnectionInterceptor). So if you don't have a transaction you get the error.

Now, what exactly would you expect to happen in the case without a transaction?

In my opinion, we should call DataSource#getConnection but the correct usage of this API is to close the connection when you are done with it, which means we need to have some kind of method aspect, that will open a connection and close it after. That's why I suggested that we should add something like @ConnectionAdvice.

@matt-snider
Copy link

@dstepanov Thank you for taking a look and explaining! I appreciate it.

I would have expected it to work like Spring's @Transactional. I think this is what other users would expect too. In the guides it is always communicated as if the two transaction management modules are interchangeable. Perhaps documenting this difference would avoid future misunderstandings.

In the example project, I changed to Spring in this commit and everything worked as expected after that. So at least on my end it's not a big deal to just switch to Spring Transaction Management

@dstepanov
Copy link
Contributor

@matt-snider I have debugged the Spring TX and I see the difference.

The original poster is injecting Connection which doesn't work because of what I wrote above, but the Jooq is injection the data source where getConnection is delegating to TransactionalConnectionInterceptor which is failing on the same problem. SpringTX is actually checking if the transaction is missing and returns a simple connection.

That can be fixed, can you please create a new issue?

@matt-snider
Copy link

@dstepanov Thank you for looking into this! I've created #1399

@Jim-Harrington
Copy link
Author

@dstepanov Actually, the original reporter is injecting javax.sql.DataSource in the situation that lead to this problem entry. The reproduction example does inject java.sql.Connection, but the bottom line is the same as with Connection injection, the logic has to be request scoped. In the end, both the injected DataSource and Connection are wrappers with logic inside them to provide the actual resources when appropriate and that logic is driven by micronaut design decisions. As you've noted, the micronaut design produces behavior that is decidedly different than that produced by the spring based alternative. I think this is a mistake and arguably a bug, but as you have also noted, at an absolute minimum it should be documented in detail so that users know what to expect.

@dstepanov
Copy link
Contributor

@Jim-Harrington If you are actually using the datasource to get the connection the fix should be for both issues. In Micronaut we allow injecting the connection which Spring doesn't support so that's the confusion.

@matt-snider
Copy link

@dstepanov @Jim-Harrington Perhaps you can suggest a better title for #1399 if it isn't JOOQ specific.

@Jim-Harrington
Copy link
Author

@matt-snider Given that annotating the non-transactional methods with io.micronaut.transaction.annotation.ReadOnly avoids this trouble, it shows that spring tx like behavior is possible using native micronaut tx. My suggestion for a solution would be to have methods on transactional classes that don't carry @transactional see behavior that equivalent to what they get if annotated with @readonly. In other words, transactional behavior should apply only to methods that have declared their desire for it by being annotated with @transactional.

@dstepanov
Copy link
Contributor

@Jim-Harrington ReadOnly is just the transactional annotation (in our case TransactionalAdvice) with the read-only set to true. The other solution is to use "raw" data source, but that requires the fix.

@apankowski
Copy link

While trying to set up db-scheduler with Micronaut & transactions I encountered the same issue.
db-scheduler is code I can't control, so I couldn't just add @Transactional or @ReadOnly.

Since I spent considerable time trying to marry those, I wanted to share a fairly simple solution to my problem in hope that it might be helpful to someone experiencing the same problem. I've shared it in this StackOverflow answer but posting relevant bits here (just in case anything happens with the link).

Basically I have this adapter method:

    private fun configureDataSource(dataSource: DataSource): DataSource {
        // Is transaction-aware DataSource proxy active? (Note: class is private so we match by name)
        return if (dataSource::class.qualifiedName == "io.micronaut.transaction.jdbc.TransactionAwareDataSource.DataSourceProxy") {
            log.info("Adding support for out-of-transaction connection fetching since the data source is a TransactionAwareDataSource")
            object : DelegatingDataSource(dataSource) {
                override fun getConnection() =
                    if (TransactionSynchronizationManager.isSynchronizationActive()) dataSource.connection
                    else DelegatingDataSource.unwrapDataSource(dataSource).connection
            }
        } else {
            dataSource
        }
    }

It detects if the DataSource is transaction-aware. If it is, it wraps it in an adapter that:

  1. if there is a transaction scope open, fetches the connection from the transaction-aware DataSource
  2. if there isn't -- fetches connection from the underlying DataSource (typically a connection pool)

I then pass this adapted DataSource to any code requiring Spring-like transaction handling semantics (e.g. db-scheduler builder).

@dstepanov
Copy link
Contributor

@apankowski You can simple you DelegatingDataSource.unwrapDataSource(dataSource). BTW v4 replaces the TransactionSynchronizationManager with the new API

@dstepanov
Copy link
Contributor

Closing this issue as the transaction manager has been rewritten in the V4.
Please report any new findings in new issues.

@dstepanov dstepanov added this to the 4.0.0 milestone Jul 11, 2023
@dstepanov dstepanov added the type: improvement A minor improvement to an existing feature label Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants