-
Notifications
You must be signed in to change notification settings - Fork 751
fix: EXPOSED-731 Timestamp support for SQLite is broken #2568
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
Conversation
3434f22 to
691b5ff
Compare
691b5ff to
e4aa2ef
Compare
| } | ||
|
|
||
| private val MYSQL_FRACTION_DATE_TIME_STRING_FORMATTER by lazy { | ||
| private val SQLITE_AND_ORACLE_TIMESTAMP_STRING_FORMATTER |
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.
This split into <FORMATTER>_NOTZ and FORMATTER is intended. It solves flaky issue with timezones.
The problem is that lazy caches the value, and that value is cached for the whole test run. And before .withZone() was inside the lazy call, so the first call was caching formatter with particular timezone.
For users it was not a problem, because usually people do not change current timezone in the code. But we do that in tets.
| // TODO MYSQL_V8 test does not work on R2DBC now. The problem is that received timestamp is shifted by timezone. | ||
| withTables(excludeSettings = listOf(TestDB.MYSQL_V8), tester) { | ||
| // Cairo time zone | ||
| java.util.TimeZone.setDefault(java.util.TimeZone.getTimeZone("Africa/Cairo")) |
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.
Is there any chance these changes fix this TODO, or is that being too hopeful? 😢
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.
It doesn't fix (I'll recheck one more time) the problem for SQLite
I was moving these setDefault() higher when understood that lazy caches the values, and I wanted that even it's cached, the value for the whole test is the same. But actually it doesn't help on run multiple tests, because in this case the first test would init lazy and all other tests use it.
But I was surprised, default value of time zone is reset before every test (even if one gradle job runs multiple 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.
Actually, I found an intersting issue, I can't move that line back under withTables, it failes (with timestamps errors), but error seems to be in threading now...
The test is green if TimeZone.setDefault() is before withTables, and red otherwise. I tried to find the moment when moving TimeZone.setDefault() starts to break the test.
I stopped in the function withConnection:
private suspend fun <T> withConnection(body: suspend Connection.() -> T): T {
if (localConnection == null) {
// If the `setDefault()` here, test is **Green**
TimeZone.setDefault(TimeZone.getTimeZone("Africa/Cairo"))
val awaitFirst = connection.awaitFirst()
// If the `setDefault()` here, test is **Red**
TimeZone.setDefault(TimeZone.getTimeZone("Africa/Cairo"))
localConnection = awaitFirst.also {
// this starts an explicit transaction with autoCommit mode off
it.beginTransaction().awaitFirstOrNull()
}
}
return localConnection!!.body()
}I'm not sure why exactly that line breaks the TimeZone. The main hypothesis is that TimeZone() set default changes time zone only or current thread, and it's not friendly to koroutines, and we should expect that time zone is not changing on runtime.
Description
It's the fix for
timestamp()column. That column doesn't support time zones, and should store the values in local timezones.But it was working different and converting values into UTC timezone. On reading that converted value was recognized as local, and shifted one more time to convert it to UTC.
After fix Exposed will store values with the value in local time zone, and expect that the value in local time zone in the moment of reading.
Tests also read the value from database as string, and check the value, to be sure that the value was not shifted to UTC in the moment of insert, and back in the moment of reading.
Type of Change
Please mark the relevant options with an "X":
Affected databases:
Checklist
Related Issues
EXPOSED-731 Timestamp support for SQLite is broken