Migrate core entities and billing to java.time#3020
Conversation
| .setRegistrarId("TheRegistrar") | ||
| .setEventTime(domain.getRegistrationExpirationDateTime()) | ||
| .setRecurrenceEndTime(END_OF_TIME) | ||
| .setEventTime(toInstant(domain.getRegistrationExpirationDateTime())) |
| .hasPersistedCurrentSponsorRegistrarId("TheRegistrar") | ||
| .and() | ||
| .hasLastTransferTime(domain.getLastTransferTime()); | ||
| .hasLastTransferTime(toInstant(domain.getLastTransferTime())); |
577e8b6 to
846407f
Compare
846407f to
bf55cc6
Compare
| domain.getCreationTime(), // Used as creation time. | ||
| domain.getLaunchNotice().getAcceptedTime()); | ||
| ISO_8601_FORMATTER.format(domain.getCreationTime()), // Used as creation time. | ||
| ISO_8601_FORMATTER.format(toInstant(domain.getLaunchNotice().getAcceptedTime()))); |
bf55cc6 to
86c0eff
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 157 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on CydeWeys and weiminyu).
common/src/main/java/google/registry/util/DateTimeUtils.java line 256 at r3 (raw file):
public static Instant plusHours(Instant instant, long hours) { return instant.plus(hours, java.time.temporal.ChronoUnit.HOURS);
i'll add an errorprone check so that the AI will automatically fix these
common/src/main/java/google/registry/util/DateTimeUtils.java line 263 at r3 (raw file):
} public static Instant plusMinutes(Instant instant, long minutes) {
given that we've talked about how the behavior of Instant.plus(...) can be wonky, what would you think about adding a simple presubmit to forbid usage of ChronoUnit except in special allowed cases?
i don't know if that wonkiness is going to always be reflected in tests?
core/src/main/java/google/registry/beam/billing/BillingEvent.java line 73 at r3 (raw file):
private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss 'UTC'").withZone(ZoneOffset.UTC);
out of curiosity is the zone specification necessary when the string already includes "UTC"?
core/src/main/java/google/registry/model/tld/Tld.java line 359 at r3 (raw file):
* <p>This will be equal to {@link #tldStr} for ASCII TLDs, but will be non-ASCII for IDN TLDs. We * store this in a field so that it will be retained upon import into BigQuery. import * java.time.Instant;
ai wyd
core/src/test/java/google/registry/tools/DomainLockUtilsTest.java line 496 at r3 (raw file):
.param(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM, "0") .scheduleTime( clock.nowUtc().plusMillis((int) lock.getRelockDuration().get().getMillis())));
wot int
core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java line 79 at r3 (raw file):
.setActingUser(fteUser) .setDescription("TheRegistrar|Some change") .setModificationTime(toDateTime(clock.now()))
it's disappointing that the AI is creating these but given that we're removing this method anyway i'm not gonna worry about it (no action required)
dd5afca to
b26e870
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 4 comments and resolved 5 discussions.
Reviewable status: 151 of 157 files reviewed, 5 unresolved discussions (waiting on gbrodman and weiminyu).
common/src/main/java/google/registry/util/DateTimeUtils.java line 263 at r3 (raw file):
Previously, gbrodman wrote…
given that we've talked about how the behavior of Instant.plus(...) can be wonky, what would you think about adding a simple presubmit to forbid usage of ChronoUnit except in special allowed cases?
i don't know if that wonkiness is going to always be reflected in tests?
Well it's not wonky, it just literally doesn't work at all for any unit greater than a DAY. I don't think a presubmit is necessary when anything executing that code path would fail by throwing a java.time.temporal.UnsupportedTemporalTypeException: Unsupported unit: Months.
core/src/main/java/google/registry/beam/billing/BillingEvent.java line 73 at r3 (raw file):
Previously, gbrodman wrote…
out of curiosity is the zone specification necessary when the string already includes "UTC"?
Yes, because the format string literally just includes the string 'UTC'. It can't format an Instant into a date without giving it an actual time zone, which withZone() is doing.
core/src/main/java/google/registry/model/tld/Tld.java line 359 at r3 (raw file):
Previously, gbrodman wrote…
ai wyd
Done.
core/src/test/java/google/registry/tools/DomainLockUtilsTest.java line 496 at r3 (raw file):
Previously, gbrodman wrote…
wot
int
Backed out this change, but do not the incompatibility between long and int here is real. It'll go away when the Duration is also migrated.
b26e870 to
5abf4fd
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys resolved 1 discussion.
Reviewable status: 150 of 157 files reviewed, 4 unresolved discussions (waiting on gbrodman and weiminyu).
|
PTAL |
acc1c92 to
3521b14
Compare
| Optional.ofNullable(getLastSuperordinateChange()).orElse(getCreationTime()); | ||
| Instant lastTransferOfCurrentSuperordinate = | ||
| Optional.ofNullable(superordinateDomain.getLastTransferTimeInstant()).orElse(START_INSTANT); | ||
| Optional.ofNullable(superordinateDomain.getLastTransferTime()) |
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 16 files and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on CydeWeys and weiminyu).
common/src/main/java/google/registry/util/DateTimeUtils.java line 263 at r3 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Well it's not wonky, it just literally doesn't work at all for any unit greater than a DAY. I don't think a presubmit is necessary when anything executing that code path would fail by throwing a
java.time.temporal.UnsupportedTemporalTypeException: Unsupported unit: Months.
right yeah that's what i meant by it being reflected in tests -- it'll throw, which is good
3521b14 to
a77c165
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on CydeWeys and weiminyu).
Migrated core entity primitives (GracePeriod, RegistryLock, TimeOfYear), transfer objects (BaseTransferObject, DomainTransferData, TransferResponse), and HostBase from Joda-Time DateTime to java.time.Instant as Phases 5 and 7. Migrated the Billing Ecosystem (BillingBase, BillingEvent, BillingRecurrence, BillingCancellation) and associated Beam pipelines (ExpandBillingRecurrencesPipeline, InvoicingPipeline) to java.time.Instant as Phase 6. Migrated PollMessage event times and all associated Poll flow utilities (PollAckFlow, PollRequestFlow) to use Instant natively. Migrated core timestamp tracking on EppResource (creationTime, lastEppUpdateTime, deletionTime) as well as CreateAutoTimestamp and UpdateAutoTimestamp to Instant, shedding deprecated DateTime accessors. Migrated the entire HistoryEntry reporting ecosystem (HistoryEntry, DomainTransactionRecord, HistoryEntryDao) completely to java.time.Instant. Updated all associated EPP flows, tools, and testing helpers to handle Instants directly where supported.
a77c165 to
9178943
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on CydeWeys and weiminyu).
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on weiminyu).
Migrated core entity primitives (GracePeriod, RegistryLock, TimeOfYear), transfer objects (BaseTransferObject, DomainTransferData, TransferResponse) and HostBase from Joda-Time DateTime to java.time.Instant. Migrated the Billing Ecosystem (BillingBase, BillingEvent, BillingRecurrence, BillingCancellation) and associated Beam pipelines (ExpandBillingRecurrencesPipeline, InvoicingPipeline) to java.time.Instant. Updated all associated EPP flows, tools, and testing helpers to handle Instants directly where supported.
This change is