feat: EXPOSED-225 Support transaction timeout in SpringTransactionManager#1897
Conversation
| if (definition.timeout != TransactionDefinition.TIMEOUT_DEFAULT) { | ||
| queryTimeout = definition.timeout | ||
| } |
There was a problem hiding this comment.
DefaultTimeout has a value of -1. If it is not this value, the user has set a new value, so I added this condition.
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/QueryTimeoutTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
There was a problem hiding this comment.
spring-transaction tests run on H2 only, so an H2 hack will be needed.
I'm not the biggest fan of this, but if it's just one test and it gives us peace of mind for this feature, an option is using a recursive query to simulate a long-running query:
tm.executeAssert(initializeConnection = true, timeout = 1) {
try {
TransactionManager.current().exec(
"""
WITH RECURSIVE T(N) AS (
SELECT 1
UNION ALL
SELECT N+1 FROM T WHERE N<10000000
)
SELECT * FROM T;
""".trimIndent(),
explicitStatementType = StatementType.SELECT
)
fail("Should have thrown a timeout exception")
} catch (cause: ExposedSQLException) {
assertTrue { cause.cause is SQLTimeoutException }
}
}I also tried this in the ExposedTransactionManagerTest.kt file using the annotation @Transactional(timeout = 1) and it worked.
Do you think this is solid enough or do you think we need to try something in the spring boot tests?
There was a problem hiding this comment.
Since timeout has already been tested in Exposed statements and in Transactions, I thought it would be okay to check that the SpringTransactionManager using Transactions sets the value in Transactions correctly.
However, since I know the implementation of this code, I'm thinking this way, and obviously I need to test the actual DB, I think the timeout using recursion is meaningful enough, so I'll add the test code you suggested. Thank you.
related issue #1671
Simply changed the Spring Transaction's timeout to apply. Change the implementation to use the timeout implemented in the previous PR.
The test code for SpringTransactionManager is not database-specific, so I think we need to implement a module like
withDBin the exposed-test module to test the actual behaviour. So I'm going to change the existing SpringTransactionManagerTest to be more extensible, at which point I'll add a real DB test for queryTimeout.I wonder if the tests included in this PR are sufficient for this feature to be deployed.