feat!: EXPOSED-109 change implement of spring transaction manager

[FullOfOrange]

I implemented all the necessary override functions and tried to have as much functionality as the current SpringTransactionManager has.

transaction { } method to set the TransactionManager, I used the same approach for Spring's transactions.

At doBegin(), we check for the existence of an external transaction manager and create a new transaction.

At doCommit() and doRollback(), we finalize the transaction and either return to the outer transaction or set the TransactionManager to null.

The second approach, which is still in the works, is to implement two new interfaces, TransactionManager and TransactionInterface from Exposed, and use them for Spring's transactions. I think this approach will be much more difficult to maintain than ThreadLocalTransactionManager, so I'm hoping that the first approach will be adopted

Sorry for my poor english, I hope my opinion is clear.
Thanks for your review

[FullOfOrange]

Is there a script to create a new API file?

Oh, I found it

[bog-walk]

@FullOfOrange Thanks for putting a PR together so quickly, it's looking good so far!

I'll checkout the branch more thoroughly over the next coming days, but two general questions from a glance:

1. What's the reasoning behind removing the default databaseConfig argument in class SpringTransactionManager()?
2. Given that you've had to make very few changes to the test suite, do you anticipate that this change will have few/no breaking changes? Are there any special migration steps that will be necessary for users to know if this new manager is released?

[FullOfOrange]

First, It's my mistakes.. :) i'll add default value

Second, I wrote it to work as well as possible with the current test version, and the existing tests cover the test cases I expect. However, there are a few expected issues

• If you're using it with an ORM that uses DataSourceTransactionManager as its default TransactionManager, such as JOOQ or Spring Data JDBC, then simply initializing SpringTransactionManager may not work properly because there is no longer a DataSourceTransactionManager. In this case, you need to add two beans, SpringTransactionManager and DataSourceTransactionManager, and then define a Composite Transaction Manager that combines the two managers.

• For compatibility, we implemented an AbstractPlatformTransactionManager that has the basic functionality of a DataSourceTransactionManager. Because of this, it is expected that most of the functionality, such as globalRollbackOnParticipationFailure, will be the same.

• I proposed this feature to make it easier to implement Propagation and IsolationLevel. Previously, the DataSourceTransactionManager satisfied these two features, but the SpringTransactionManager implementation did not, so there were some inconsistencies. The current implementation doesn't fulfill any of the above, so it needs to be improved.

I've written an implementation as quickly as I could to get your thoughts. I'll try to write the expected tests, such as rollbacks, after I confirm this implementation direction.

[FullOfOrange]

@bog-walk It's been a while!

I've tried a few things, but the current implementation seems to be the best.

So I wrote some test code to see if SpringTransactionManager properly sets up a TransactionManager. I referenced this link for how to write the test code, and used the TransactionTemplate and mockk to test that the connection was managed well.

I didn't write tests for Propagation because it is not supported in previous versions. We will add tests in PRs that implement these features in the future. If you think there are other tests that should be included, or if you need to write a test for Propagation support, please comment.

[bog-walk]

A question about breaking changes and migration for users since TransactionManager interface will no longer be implemented:

• All interface functions will no longer be callable on a SpringTransactionManager instance, namely currentOrNull() and newTransaction():
  • For example, the user in this issue uses newTransaction() to set a non-default isolation level. Is the plan that this use would be replaced with @Transactional(isolation = ?)?
  • And would it be possible to return the current transaction, by the user going through the database, like tm.database.transactionManager.currentOrNull()? Or is the intention that this implementation delegates all transaction operations to Spring, so the user should not need to call these functions anymore?

[FullOfOrange]

I didn't realize there was a case where a user could create and use a new instance object that wasn't a bean.

If used in this way, I think it's possible that Spring's transaction management and Exposed's transaction management could get mixed up, making it difficult to manage connections.

1. I would prefer not to use newTransaction and currentOrNull in a SpringTransactionManager instance in this situation, and if you agree, it would be a good idea to implement a TransactionManager that throws an error if newTransaction and currentOrNull are used.

2. this following code provides an isolation level by using @transactional(isolation = ?), but I haven't verified that this properly implements Spring's policy. I'll try to implement Isolation and Propagation as per Spring's policy in the following PR.

    override fun doBegin(transaction: Any, definition: TransactionDefinition) {
        val trxObject = transaction as ExposedTransactionObject

        val currentTransactionManager = trxObject.manager
        TransactionManager.resetCurrent(currentTransactionManager)

        currentTransactionManager.currentOrNull()
            ?: currentTransactionManager.newTransaction(
                isolation = definition.isolationLevel,
                readOnly = definition.isReadOnly,
            ).apply {
                if (showSql) {
                    addLogger(StdOutSqlLogger)
                }
            }
    }

3. I exposed the database to check if the TransactionManager is set up properly when testing, but if there is another way to check the database at test time, I will prevent the database instance from being accessed with tm.database. Can you give me any advice?

[bog-walk]

1. For the next release, the loss of TransactionManager functions would be mentioned as a breaking change and I'm drafting a migration section to make clear any changes. For now we'll see what issues pop up for example if users choose to call a new transaction within a SpringTransactionManager instance in spite of our suggestions.
2. Sounds good to me.
3. Our core unit tests usually involve passing in the database directly so it is available as a test property. But in special cases, it can be checked via TransactionManager.currentOrNull()?.db (see MultidatabaseTest.kt for an example). I ran your tests using the following assertion and they all passed so it could be a valid replacement for tm.database:

assertEquals(
    TransactionManager.managerFor(this.database),
    TransactionManager.managerFor(TransactionManager.currentOrNull()?.db)
)
assertEquals(
    TransactionManager.managerFor(TransactionManager.currentOrNull()?.db),
    TransactionManager.manager
)

Note that the tests that pre-assign a variable would need to call this database check instead only when needed:

val tm = SpringTransactionManager(ds1)
val tm2 = SpringTransactionManager(ds2)
// val transactionManager2 = TransactionManager.managerFor(tm2.database)

tm2.executeAssert(false) {
    tm.executeAssert(false)
    assertEquals(TransactionManager.managerFor(TransactionManager.currentOrNull()?.db), TransactionManager.manager)
}

[FullOfOrange]

All interface functions will no longer be callable on a SpringTransactionManager instance, namely currentOrNull() and newTransaction():

And would it be possible to return the current transaction, by the user going through the database, like tm.database.transactionManager.currentOrNull()? Or is the intention that this implementation delegates all transaction operations to Spring, so the user should not need to call these functions anymore?

I changed the database instance to be inaccessible directly, and the transaction is now only accessible via TransactionManager.currentOrNull().

My intention is, as you say, that the user should not be able to use the Exposed Transaction directly when using the PlatformTransactionManager Bean.

However, even if the user accesses the transaction with TransactionManager.currentOrNull(), TransactionManager.currentOrNew() it will behave almost identically to the transaction { } in Exposed, so as long as user don't call transaction.close(), it shouldn't have any problems.

The recommendation is to avoid calling the TransactionManager directly.

[bog-walk]

Regarding @Transactional(Propogation.?), the lack of support is certainly a known issue. Tests can be added for it (and isolation) in future PRs that build on the new implementation in this one.

[bog-walk]

Thanks for all the effort you've put into this PR so far! These should be the last change requests from me. Please let me know if you need any help/clarification with the mockk dependency request.

[FullOfOrange]

@bog-walk Thank you so much for the detailed review of PR!

I've fixed most of the things you mentioned. I would be very grateful if you could check PR one more time.

Please leave a comment if you have any other issues you'd like to discuss, or anything else you'd like to see remove implementation of the TransactionManage.

If you don't want to make any breaking changes, I'll try to find out with a new implementation.

[bog-walk]

There's quite a lot of overrides when implementing Connection from scratch.

I realize that the goal of this test suite is to mock transaction and connection calls without relying on an actual database or actual returned results. Would the validity or isolation of these unit tests be significantly compromised if the data source at least started with a base connection from the H2 driver? Especially if all the function calls being tested are overriden in ConnectionSpy?

Please think about it and consider the following alternative:

class ConnectionSpy(private val connection: Connection) : Connection by connection {
    // then remove all overrides not implemented
}

class DataSourceSpy(private val connectionSpy: (Connection) -> Connection) : DataSource {
    // ...
    override fun getConnection() = connection ?: run {
        val testConnection = DriverManager.getConnection("jdbc:h2:mem:test")
        connectionSpy(testConnection)
    }
}

private val ds1 = DataSourceSpy(::ConnectionSpy)
private val con1 = ConnectionSpy(ds1.connection)
// `DriverManager.getConnection()` would only be invoked once on this first access
// then the value would be accessed from the private variable `connection`
// which can continue to be stored using `mockConnection()`
// or by adding an assignment in the null branch of `getConnection()` above.

Otherwise maybe the 2 spy classes need to at least be extracted to their own file and I'll get a second opinion again on what our options are.

[FullOfOrange]

I used the mock object to test the behavior of the connection object and if there is another way to do it, I will use it. If it makes sense to test whether or not the method is called in the way you suggested using the example code above, I would change it to do so.

[FullOfOrange]

private open class DataSourceSpy(private val connectionSpy: (Connection) -> Connection) : DataSource {
    var con: Connection = connectionSpy(DriverManager.getConnection("jdbc:h2:mem:test")

    override fun getConnection() = con
    // other override functions that throw not implement exception
}

private open class ConnectionSpy(private val connection: Connection) : Connection by connection {

    var commitCallCount: Int = 0
    ...
    // mock functions that need to test
}

I changed it to use the H2 connection implementation as you suggested.

Since the purpose is to check whether the method is called or not, I think this should be able to complete the test without any problems. If you think there is a better way to do this, or if you think I should move the Spy class to a different file, please comment.

[e5l]

lgtm

[e5l]

please consider extracting utility classes to a separate with internal modifier

[FullOfOrange]

@e5l Just to clarify your question

1. move the spy classes outside of the test class and then change them to internal classes, 2. just change them to internal classes, or 3. move them to a new file with internal modifier?

[e5l]

a new file is preferred

[FullOfOrange]

I created a file called TestSpys.kt that contains a collection of spy classes.

I think it's a bit of a bad name, but I'll change it if there's a convention or a recommendation.

[e5l]

I would name it same as a class by default

[bog-walk]

@FullOfOrange Just to clarify above, feel free to create 2 new files, each with its own internal class, named after the class. So 1 for DataSourceSpy and 1 for ConnectionSpy.

Also, there should be no conflict, but please consider doing a rebase from main, which has recently had some changes.