-
Notifications
You must be signed in to change notification settings - Fork 697
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
feat: EXPOSED-225 Support transaction timeout in SpringTransactionManager #1897
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
withDB
in 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.