Begin migration of joda DateTime to java.time.Instant#2992
Begin migration of joda DateTime to java.time.Instant#2992CydeWeys wants to merge 1 commit intogoogle:masterfrom
Conversation
2745693 to
7248b29
Compare
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
0ae2854 to
218f732
Compare
java.time has been around since Java 8 and was based on joda DateTime, so this is an overdue migration. We're migrating specifically to Instant in most places rather than ZonedDateTime because we were always using DateTimes in UTC to reference a specific instant, which is exactly what Instants are for. ZonedDateTime set to UTC may still be useful in some places that are heavy on date math (especially in tests). There is a lot more work to be done after this, but I wanted to put together a manual PR showing my overall approach for how to do the migration that I can then hopefully follow along with AI to continue making these changes throughout the codebase. The basic approach is to migrate a small number of methods at a time, marking the old methods as @deprecated when possible (not always possible because of @InlineMe restrictions). This PR doesn't yet migrate any DateTime fields in the model classes, so that's the one remaining type of refactor to figure out after this. We won't be changing how any of the data is actually stored in the database. BUG= http://b/496985355
218f732 to
20a2301
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 66 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on CydeWeys and ptkach).
common/src/main/java/google/registry/util/DateTimeUtils.java line 102 at r1 (raw file):
/** Returns whether the first {@link DateTime} is equal to or later than the second. */ public static boolean isAtOrAfter(DateTime timeToCheck, DateTime timeToCompareTo) {
i would assume that these DateTime methods should be deprecated as well
common/src/main/java/google/registry/util/DateTimeUtils.java line 123 at r1 (raw file):
/** * Adds years to a date, in the {@code Duration} sense of semantic years. Use this instead of * {@link DateTime#plusYears} to ensure that we never end up on February 29.
I don't think this comment is up to date. You can't directly add years to an instant for other reasons though.
common/src/main/java/google/registry/util/DateTimeUtils.java line 36 at r2 (raw file):
@Deprecated public static final DateTime START_OF_TIME = new DateTime(0, DateTimeZone.UTC); /** The start of the UNIX epoch in UTC, in a convenient constant. */
Technically, this isn't "in UTC" -- we treat it as being in UTC but a Java instant is not explicitly zoned
common/src/main/java/google/registry/util/DateTimeUtils.java line 133 at r2 (raw file):
/** * Subtracts years from a date, in the {@code Duration} sense of semantic years. Use this instead
ditto re: this comment not up to date (mentioning DateTime)
util/src/test/java/google/registry/util/DateTimeUtilsTest.java line 42 at r2 (raw file):
/** Unit tests for {@link DateTimeUtils}. */ class DateTimeUtilsTest {
I'm assuming that the plan is to just change all the existing arithmetic tests to use Instants instead of DateTimes in the future when we remove DateTimes. Assuming that's the case, we should probably make a note of it in a TODO comment somewhere in this class
java.time has been around since Java 8 and was based on joda DateTime, so this is an overdue migration. We're migrating specifically to Instant in most places rather than ZonedDateTime because we were always using DateTimes in UTC to reference a specific instant, which is exactly what Instants are for. ZonedDateTime set to UTC may still be useful in some places that are heavy on date math (especially in tests).
There is a lot more work to be done after this, but I wanted to put together a manual PR showing my overall approach for how to do the migration that I can then hopefully follow along with AI to continue making these changes throughout the codebase. The basic approach is to migrate a small number of methods at a time, marking the old methods as @deprecated when possible (not always possible because of @InlineMe restrictions). This PR doesn't yet migrate any DateTime fields in the model classes, so that's the one remaining type of refactor to figure out after this. We won't be changing how any of the data is actually stored in the database.
BUG= http://b/496985355
This change is