Migrate TMCH, SMD, and Fee models to java.time#3019
Migrate TMCH, SMD, and Fee models to java.time#3019CydeWeys wants to merge 1 commit intogoogle:masterfrom
Conversation
1aba8af to
ff7f52c
Compare
511b0cc to
1566721
Compare
| boolean isAnchorTenant = expectedBillingFlags.contains(ANCHOR_TENANT); | ||
| // Set up the creation cost. | ||
| boolean isDomainPremium = isDomainPremium(getUniqueIdFromCommand(), clock.nowUtc()); | ||
| boolean isDomainPremium = isDomainPremium(getUniqueIdFromCommand(), clock.now()); |
1566721 to
b926c98
Compare
|
PTAL, ready for code review. The approach I'm taking here is a little different than in previous PRs; you might see some method calls that perform a toInstant() or toDateTime() conversion from the call site. I'd rather do it this way than expand the scope of the PR and add in more function overloads that will only need to be later removed. It seems cleaner to either fully migrate any given function over, or to convert it at callsites as necessary, rather than overloading everything with two versions and adding additional churn. This PR is already creaking at the seams; expanding the scope any further is unlikely to lead to good results. |
b237099 to
8d692f8
Compare
|
This PR is already large enough so I won't do it here, but I am considering adding some methods to the
You'd be able to do:
Or maybe something like a fluent method:
(crucially, none of these would change the clock's state, just make it easier to perform date arithmetic on the current time) |
gbrodman
left a comment
There was a problem hiding this comment.
possibly but people are going to still have the instinct to do something like clock.now().plusDays(1) just because the usual pattern is to get the "current" datetime in some way then modify it. It's probably not good to have both intermixed.
actually yeah i'm not a big fan of having to use DateTimeUtils to do any sort of big datetime addition/subtraction. Is it necessary?
@gbrodman reviewed 101 files and all commit messages, and made 10 comments.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on CydeWeys).
-- commits line 1 at r3:
this commit message is too long and verbose to the point where it's counterproductive
it's TOTALLY SHOCKING that AI wrote something too overly verbose
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 591 at r3 (raw file):
@Nullable BillingRecurrence billingRecurrence) throws EppException { DateTime now = currentDate;
make this into an instant here so that we only need to do the conversion once
core/src/main/java/google/registry/flows/domain/DomainTransferApproveFlow.java line 166 at r3 (raw file):
Tld.get(tldStr), targetId, toInstant(transferData.getTransferRequestTime()),
get as an instant instead
core/src/main/java/google/registry/flows/poll/PollRequestFlow.java line 81 at r4 (raw file):
.setMessageQueueInfo( new MessageQueueInfo.Builder() .setQueueDate(toInstant(pollMessage.getEventTime()))
pls fix
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 343 at r4 (raw file):
assertThat(domainAutorenewEvent.getRegistrarId()).isEqualTo("TheRegistrar"); assertThat(domainAutorenewEvent.getRecurrenceEndTime()) .isEqualTo(toDateTime(implicitTransferTime));
get as instant
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 384 at r4 (raw file):
getOnlyPollMessage( "NewRegistrar", toDateTime(expectedExpirationTime), PollMessage.Autorenew.class); assertThat(transferApprovedPollMessage.getEventTime())
can get as instant
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 386 at r4 (raw file):
assertThat(transferApprovedPollMessage.getEventTime()) .isEqualTo(toDateTime(implicitTransferTime)); assertThat(autorenewPollMessage.getEventTime()).isEqualTo(toDateTime(expectedExpirationTime));
can get as instant
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 414 at r4 (raw file):
.collect(onlyElement()); assertThat(losingTransferPendingPollMessage.getEventTime()).isEqualTo(clock.nowUtc()); assertThat(losingTransferApprovedPollMessage.getEventTime())
can get as instant
core/src/test/java/google/registry/testing/DatabaseHelper.java line 481 at r4 (raw file):
getDomainRenewCost( domain.getDomainName(), google.registry.util.DateTimeUtils.toInstant(costLookupTime),
lol import
gbrodman
left a comment
There was a problem hiding this comment.
also it looks like there are many cases where we replace "clock.nowUtc()" with "toDateTime(clock.now())" which doesn't seem to really help things
@gbrodman made 1 comment.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on CydeWeys).
Continues the project-wide migration from Joda-Time's DateTime to java.time.Instant, focusing on Trademark Clearinghouse (TMCH), Signed Mark Data (SMD), and Fee extension models. Key updates: - TMCH & SMD: Updated SmdRevocationList and domain check/create flows to use Instant for sunrise validations and revocation checks. - Fee Extension Ecosystem: Refactored FeeCheckRequest, FeeCreateCommandExtension, and BaseFee to use Instant for effective dates and period calculations. - EPP Objects: Updated DomainInfoData, TransferResponse, and PollMessage objects to use Instant for event timestamps. - Pricing Logic: DomainPricingLogic methods now accept Instant for cost calculations. Additionally, DateTimeUtils was enhanced with Instant compatibility methods for plusMonths and minusMonths to safely handle leap years. Redundant conversions between DateTime and Instant were eliminated throughout the flows and tests. DomainFlowUtils leverages Instant natively to avoid inline casting, and test assertions now utilize Truth's Instant subjects for cleaner validation.
8d692f8 to
653cff0
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 9 comments and resolved 7 discussions.
Reviewable status: 87 of 102 files reviewed, 9 unresolved discussions (waiting on gbrodman).
Previously, gbrodman wrote…
this commit message is too long and verbose to the point where it's counterproductive
it's TOTALLY SHOCKING that AI wrote something too overly verbose
Done.
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 591 at r3 (raw file):
Previously, gbrodman wrote…
make this into an instant here so that we only need to do the conversion once
Done.
core/src/main/java/google/registry/flows/domain/DomainTransferApproveFlow.java line 166 at r3 (raw file):
Previously, gbrodman wrote…
get as an instant instead
Done.
core/src/main/java/google/registry/flows/poll/PollRequestFlow.java line 81 at r4 (raw file):
Previously, gbrodman wrote…
pls fix
Done.
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 343 at r4 (raw file):
Previously, gbrodman wrote…
get as instant
Done.
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 384 at r4 (raw file):
Previously, gbrodman wrote…
can get as instant
Done.
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 386 at r4 (raw file):
Previously, gbrodman wrote…
can get as instant
Done.
core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java line 414 at r4 (raw file):
Previously, gbrodman wrote…
can get as instant
Done.
core/src/test/java/google/registry/testing/DatabaseHelper.java line 481 at r4 (raw file):
Previously, gbrodman wrote…
lol import
Done.
CydeWeys
left a comment
There was a problem hiding this comment.
Agreed that I don't love the common pattern of static import and then call plusDays(..., 5) ... but the alternatives also aren't amazing. We don't have to address it in this PR (in fact I'd rather not, as it's more easily changed through IntelliJ), but another alternative would be something like .plus(Duration.ofDays(5)). ... would it be dumb to make a statically importable wrapper function around Duration.ofDays() that could make that look like .plus(days(5))? The problem with Duration.ofDays() is that in a lot of files that are still using the Joda version it actually has to look like .plus(java.time.Duration.ofDays(5)), which is obviously sub-ideal.
Anyway, we can solve this later -- it's trivial to change the implementation of plusDays() to whatever we like, and then use IntelliJ to inline it everywhere and delete it.
@CydeWeys made 1 comment.
Reviewable status: 87 of 102 files reviewed, 9 unresolved discussions (waiting on gbrodman).
CydeWeys
left a comment
There was a problem hiding this comment.
PTAL
@CydeWeys made 1 comment.
Reviewable status: 87 of 102 files reviewed, 9 unresolved discussions (waiting on gbrodman).
This commit completes the java.time migration for the Trademark Clearinghouse (TMCH), Signed Mark Data (SMD), and Mark models, the EPP Response & InfoData Objects, and the Fee Extension ecosystem, transitioning them from Joda-Time's DateTime to java.time.Instant.
Changes made:
Core Models Migration:
ClaimsList,TmchCrl,SignedMark,Trademark,BaseFeeand its extensions (FeeCreateCommandExtension,FeeUpdateCommandExtension,FeeTransferCommandExtension,FeeRenewCommandExtension,FeeCheckResponseExtensionetc.) have all been migrated to natively store, parse, and returnInstant.toDateTimebridge methods (e.g.BaseFee'sgetValidDateRangeInstant) have been removed.Flow Integrations & Utilities:
DomainFlowUtils,DomainPricingLogic, andCheckApiActionseamlessly integrate withjava.time.Instantfor fee requests, effective dates, pricing lookups, and premium checks.java.time.Durationis natively utilized instead of Joda-Time'sDuration.plusMonths(Instant, int)andminusMonths(Instant, int)toDateTimeUtilsto safely handle monthly temporal additions onInstantboundaries.XML Binding and Serializers:
UtcInstantAdapterinpackage-info.javadefinitions underfee*,smd,mark,contact, andeppoutputto securely marshal and unmarshal ISO-8601 strings intoInstantrepresentations.Parsers:
ClaimsListParserandSmdrlCsvParsernow strictly validate and build objects utilizingInstant.parse().Testing Ecosystem:
[TruthIncompatibleType]warnings whereDateTimecomparisons againstInstantwere previously triggering ErrorProne. AddedInstantassertions andtoDateTimewraps across test suites includingDomainSubjectandAbstractEppResourceSubject.java.time.Duration,START_INSTANT,END_OF_TIME) and replacedFakeClock.nowUtc()withFakeClock.now().toDateTime(clock.now())withclock.nowUtc()andtoDateTime(END_INSTANT)withEND_OF_TIMEacross tests.This change is