WIP - Joda time to JSR310 #58
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aurambaj
force-pushed
the
ja/joda-up-wip
branch
3 times, most recently
from
c5825d2 to
2719aea
Compare
… are the dependencies related to it
So even if we remove from the webapp pom.xml, it stays in the dependencies (and it is an older version) We probably need to setup checks to avoid Joda to be used in the future https://aws.amazon.com/blogs/developer/java-sdk-bundle/ [INFO] | +- com.amazonaws:aws-java-sdk-core:jar:1.11.163:compile [INFO] | | +- commons-logging:commons-logging:jar:1.1.3:compile [INFO] | | +- software.amazon.ion:ion-java:jar:1.0.2:compile [INFO] | | +- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:jar:2.13.5:compile [INFO] | | \- joda-time:joda-time:jar:2.8.1:compile
the "common" module does not have direct dependency on Joda, instead it through: com.fasterxml.jackson.datatype:jackson-datatype-joda Generate Maven dependency with and without the dependencies to see the differences. The interesting sub tree is [INFO] +- com.fasterxml.jackson.datatype:jackson-datatype-joda:jar:2.13.5:compile [INFO] | +- com.fasterxml.jackson.core:jackson-annotations:jar:2.13.5:compile [INFO] | +- com.fasterxml.jackson.core:jackson-core:jar:2.13.5:compile [INFO] | +- com.fasterxml.jackson.core:jackson-databind:jar:2.13.5:compile [INFO] | \- joda-time:joda-time:jar:2.10.8:compile There are 7 import org.joda.time.DateTime; in that module Reference doc: https://blog.joda.org/2014/11/converting-from-joda-time-to-javatime.html DateTime has 2 replacement classes: ZonedDateTime and OffsetDateTime Interesting note, there is a library with extra classes because not everything can make it to the JDK https://www.threeten.org/threeten-extra/ detailed explanation beyond java doc, about different between ZonedDateTime and OffsetDateTime: https://stackoverflow.com/a/30234992 It is said that ZonedDateTime is not advised for database storage, also see javadoc. I guess this is really a matter of how this is transformed and saved in the DB. We need to understand Hibernate convertion type and Mysql storage.
…some unit test to compare new/old implementation The "common" module has UTC hardcoded in the maven surefire plugin configuration, the TZ can be changed in those settings. The unit test tries different TZ for investigation purposes. Most usage is just for POJO fields, with no calls to any specific methods. Some test that uses a constructor. Replacing the type should be straightforward, yet there could be side effect from "toString()" and other not identify differences.
…or common module Code refactoring can be done by first replacing the current code with the function equivalent and change its signature to match the new version. Or just replace in a more manual way. Either way there are manual edit, to fix import ect. Summary new DateTime() = JodaMigration.newDateTimeCtorOld --> JodaMigration.newDateTimeCtor DateTime.now() = JodaMigration.newDateTimeCtorOld --> JodaMigration.dateTimeNow org.joda.time.DateTime --> java.time.ZonedDateTime (+ import changes) UT are working with no extra modification
Add Joda time as explicit dependency for testing the migration. It will need to be removed later. UT are still fine at this point without changes
…fro rest client migration This introduces the "threeten-extra" library that is required to replace Period formatting. There is a small difference in format for large value which is not reach by current test most likely so the output would always be the same. The code is probably not used anywhere else, so it should be fine. To confirm with the rest of the migration.
Must review a comment later regarding jackson: This helps with joda deserialize of joda date time string" - removes jackson-datatype-joda from pom.xml - org.joda.time.DateTime --> java.time.ZonedDateTime (+ import changes) - DateTime.getMillis() --> JSR310Migration.getMillis() - PeriodFormat.getDefault().print(new Period(start, end)) --> JSR310Migration.toWordBasedDuration() With toWordBasedDuration, there is a small difference in format for large value which is not reach by current test most likely so the output would always be the same. The code is probably not used anywhere else, so it should be fine. To confirm with the rest of the migration.
new DateTime() --> JodaMigration.newDateTimeEmptyCtor()
new DateTime(0) / new DateTime(EPOCH) --> JodaMigration.newDateTimeAtEpoch()
dateTime.toDate() --> JSR310Migration.dateTimeToDate
dateTime.isAfter(long) --> JSR310Migration.dateTimeIsAfterEpochMillis
dateTime.getMillisOfSecond --> JSR310Migration.dateTimeGetMillisOfSecond dateTime.withMillisOfSecond --> JSR310Migration.dateTimeWithMillisOfSeconds So far only on test uses this logic, so inlining/refactoring to be considered
new DateTime(Date) --> JSR310Migration.dateTimeToDate()
dateTime.withTime(localTime) --> JSR310Migration.dateTimeWithLocalTime
new LocalTime(str) --> JSR310Migration.newLocalTimeWithString new LocalTime(h,m,s) --> JSR310Migration.newLocalTimeWithHMS
dateTime.getDayOfWeek() --> JSR310Migration.dateTimeGetDayOfWeek dateTime.withDayOfWeek(dayOfWeek) --> JSR310Migration.dateTimeWithDayOfWeek
new DateTime(instant: String|Long) --> JSR310Migration.newDateTimeCtorWithLongAndString
Joda Period equivalent is ThreeTen DurationPeriod since we're using the time part of it in current implementation Period --> DurationPeriod new Period(long) --> JSR310Migration.newPeriodCtorWithLong new Period(hmsm) --> JSR310Migration.newPeriodCtorWithHMSM
will need more testing but there is nothing to do on the controllers themselves
new DateTime(dateTime, dateTimeZone) --> JSR310Migration.newDateTimeCtorWithStringAndDateTimeZone new DateTime(dateTimeZone) --> JSR310Migration.newDateTimeCtorWithDateTimeZone DateTimeZone.forID(zoneId) --> JSR310Migration.dateTimeZoneForId
new DateTime(YMDHM) --> JSR310Migration.newDateTimeCtor(2021, 11, 25, 0, 0);
I wish I didn't commit that/work on top of it but it is hard to change now.
need to register a DateTimeProvider. By default it seems to use LocalDateTime ... but that's a bit odd to not support other types by default. Will check more the doc later.
This has quite a bit to be reviewed though, specifically right now it rely on try/catch parsing exception
… use millisecond To be backward compatible with the Joda time implementation, we need to make sure the timestamp don't include the nano seconds. This is done by SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS, DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS for the record, it helps fix the TMServiceTest.testExportAssetAsXLIFF, and also fixed the frontend (date would show all wrong)
…loss of precision supposedly due to loss of precision happens with JSON serialization/deserialization. Must understand better the overall impact.
import org.joda.time.DateTime --> import java.time.ZonedDateTime; import org.joda.time.DateTimeZone --> import java.time.ZoneId; DateTime --> ZonedDateTime DateTimeZone --> ZoneId
dateTime.withMillisOfSecond(0).getMillis() --> JSR310Migration.dateTimeWith0MillisAsMillis To be inlined later
There is a small different to figure out
System.out.println(JSR310Migration.dateTimeNowInUTC()); // 2023-12-07T21:04:04.304416Z
System.out.println(ZonedDateTime.now(ZoneId.of("UTC"))); // 2023-12-07T21:04:04.304619Z[UTC]
Instant.ofEpochSecond(epochSecond).toDateTime() --> JSR310Migration.dateTimeOfEpochSecond
- test pass on CI but the container must on UTC (if reproducing locally with docker compose, it also passes) - CLI local build does not pass (unless for UTC timezone in the maven surefire plugin) - Not clear at this point why it used to work before Moving the query to use ZonedDateTime seems to fix the conversion issue. Since we use ZonedDateTime in all Hibernate entity changing this query seems to make the most sense
Since it is causing issue with timezone convertion and we use ZonedDateTime everywhere else
…st and fix other tests It seems wiser to not hardcode the test timezone in the Maven configuration. If there are some discrepancies between local and CI, this should be kept in mind. Also for local testing the docker build can be used which will be UTC This actually surface on issue in some test. JSR310MigrationTest.zonedDateTimeEquals() documents some edge cases to keep in mind
aurambaj
force-pushed
the
ja/joda-up-wip
branch
from
2719aea to
8fcd3c4
Compare
HSQL persists ZonedDateTime without TZ info. Comparing ZonedDateTime against unix_timestamp() then fails because of the TZ difference Fix queries to accommodate this.
aurambaj
force-pushed
the
ja/joda-up-wip
branch
from
8fcd3c4 to
5796c30
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.