Migrate core temporal models and related entities to java.time.Instant

[CydeWeys]

This comprehensive refactor continues the migration from Joda-Time to java.time (Instant), focusing on core timestamp models, transition properties, and their integration across the codebase.

Key changes:

• Migrated CreateAutoTimestamp and UpdateAutoTimestamp to use Instant internally, providing Joda-Time bridge methods for backward compatibility.
• Updated TimedTransitionProperty to handle Instant-based transition maps and updated corresponding Hibernate UserTypes (TimedTransitionBaseUserType).
• Migrated GracePeriod, BillingBase, BillingEvent, PollMessage, and PendingActionNotificationResponse fields (e.g., expirationTime, eventTime) to Instant.
• Updated Tld and FeatureFlag models to use Instant for claimsPeriodEnd, bsaEnrollStartTime, and status transitions.
• Enhanced CLI tools and parameters (TransitionListParameter, InstantParameter) to support Instant-based input and output.
• Updated EntityYamlUtils with custom Instant serializers/deserializers to maintain format consistency (e.g., .SSSZ precision) required for YAML-based tests.
• Implemented UtcInstantAdapter to ensure JAXB XML serialization maintains millisecond accuracy, matching legacy Joda-Time behavior.
• Resolved Hibernate 6 type mismatches in JPQL and Native queries by ensuring consistent use of Instant for comparisons.
• Updated GEMINI.md with project-specific engineering standards, including the 'one commit per PR' mandate.
• Added necessary @JsonIgnore and @JsonProperty annotations to ensure consistent Jackson serialization across models.

All changes have been verified with a full suite of standard and SQL integration tests.

---

This change is 

[gbrodman]

@gbrodman reviewed 129 files and all commit messages, and made 44 comments.
Reviewable status: all files reviewed, 61 unresolved discussions (waiting on CydeWeys).

---

common/src/testing/java/google/registry/testing/truth/DateTimeSubject.java line 91 at r7 (raw file):

  }

  /** Internal factory for Truth. */

do we need this? i don't think so?

---

common/src/testing/java/google/registry/testing/truth/InstantSubject.java line 91 at r7 (raw file):

  }

  /** Internal factory for Truth. */

i also don't think we need this?

---

common/src/testing/java/google/registry/testing/truth/RegistryTruth.java line 27 at r4 (raw file):

 * during the migration from Joda-Time to {@link java.time}.
 *
 * <p>Usage: {@code import static google.registry.testing.truth.RegistryTruth.assertAt;}

i don't think this class is necessary? we can just call the subjects directly like we do with every other subject

---

core/src/main/java/google/registry/beam/billing/ExpandBillingRecurrencesPipeline.java line 307 at r7 (raw file):

        difference(
            eventTimes,
            existingEventTimes.stream().map(DateTimeUtils::toDateTime).collect(toImmutableSet()));

instead of converting to date times here, convert to instants when we load them

---

core/src/main/java/google/registry/bsa/persistence/DownloadSchedule.java line 40 at r7 (raw file):

public record DownloadSchedule(
    long jobId,
    DateTime jobCreationTime,

switch this to Instant so we don't need to convert in the factory methods

---

core/src/main/java/google/registry/bsa/persistence/Queries.java line 196 at r7 (raw file):

  }

  private static Instant convertToInstant(Object obj) {

what object does Hibernate return to us? is this necessary? or, why do we have both options here?

---

core/src/main/java/google/registry/bsa/persistence/RefreshSchedule.java line 57 at r7 (raw file):

    return new RefreshSchedule(
        job.getJobId(),
        toDateTime(job.getCreationTime()),

same here, change to Instant so we don't need to convert in the factory method

---

core/src/main/java/google/registry/flows/poll/PollFlowUtils.java line 85 at r7 (raw file):

      // Move the eventTime of this autorenew poll message forward by a year.
      Instant nextEventTime = leapSafeAddYears(toInstant(autorenewPollMessage.getEventTime()), 1);

Convert all these getEventTime calls to getEventTimeInstant. Otherwise you're just converting from instant to DT back to instant.

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 190 at r7 (raw file):

        (T)
            HistoryEntryDao.loadHistoryObjectsForResource(
                    resource.createVKey(), toInstant(START_OF_TIME), toInstant(timestamp))

START_INSTANT

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 244 at r7 (raw file):

  /**
   * Returns whether this resource is linked to any domains at the given time.

Bad javadoc update. It's a host.

---

core/src/main/java/google/registry/model/common/FeatureFlag.java line 186 at r7 (raw file):

  }

  @JsonIgnore

I don't think this annotation is necessary. Did gemini just add this across all the classes? If so, you should check all of them to make sure that we're not adding it superfluously

---

core/src/main/java/google/registry/model/common/TimedTransitionProperty.java line 52 at r7 (raw file):

  /**
   * Initial value for a property that transitions from this value at {@code START_INSTANT}.

i don't think this sentence makes sense

actually wtf i don't think this inner class is ever used

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 258 at r7 (raw file):

   * happen to be in the autorenew grace period now but should be deleted in roughly a year.
   */
  DateTime autorenewEndTime;

Why is only the given subset of classes migrated to Instant, and not like, this one too?

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 511 at r7 (raw file):

                    domain.getRepoId(),
                    transferExpirationTime.plus(
                        java.time.Duration.ofMillis(

i don't think we need the qualified names here, can import Duration

---

core/src/main/java/google/registry/model/tld/Tld.java line 361 at r7 (raw file):

   */
  @Column(nullable = false)
  @JsonProperty

i don't think these need jsonification? if so, why now?

---

core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java line 343 at r7 (raw file):

    if (desiredRegistrar.isPresent()) {
      Optional<Host> host =
          ForeignKeyUtils.loadResourceByCache(Host.class, queryString, toDateTime(timeToQuery));

don't think you need to convert to datetime (same for all of these below in this class)

---

core/src/main/java/google/registry/rdap/RdapJsonFormatter.java line 327 at r7 (raw file):

            Event.builder()
                .setEventAction(EventAction.EXPIRATION)
                .setEventDate(toInstant(domain.getRegistrationExpirationDateTime()))

i don't think we need the conversion here? doesn't this return (or at least, shouldn't it return) an Instant already?

---

core/src/main/java/google/registry/rdap/RdapNameserverAction.java line 69 at r7 (raw file):

            Host.class,
            pathSearchString,
            toDateTime(shouldIncludeDeleted() ? START_INSTANT : getRequestTime()));

i don't think we need the conversion to datetime here either

---

core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java line 179 at r7 (raw file):

      RdapSearchPattern partialStringQuery) {
    Optional<Domain> domain =
        ForeignKeyUtils.loadResourceByCache(

FKUtils exposes Instant methods right ? so we shouldn't need all these conversions either

---

core/src/main/java/google/registry/tools/CountDomainsCommand.java line 54 at r7 (raw file):

                tm().createQueryComposer(Domain.class)
                    .where("tld", Comparator.EQ, tld)
                    .where("deletionTime", Comparator.GT, toInstant(now))

just change now to be an instant, we just get it straight above

---

core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java line 68 at r7 (raw file):

      historyEntries =
          HistoryEntryDao.loadHistoryObjectsForResource(
              parentKey, toInstant(after), toInstant(before));

we can just make the parameters instants, right?

---

core/src/main/java/google/registry/tools/UpdateRecurrenceCommand.java line 121 at r7 (raw file):

  private ImmutableList<BillingRecurrence> internalExecute() {
    ImmutableMap<Domain, BillingRecurrence> domainsAndRecurrences = loadDomainsAndRecurrences();
    DateTime now = tm().getTransactionTime();

this file mixes datetimes and instants, can't we just change it so the input is an instant?

---

core/src/main/java/google/registry/tools/javascrap/RecreateBillingRecurrencesCommand.java line 100 at r7 (raw file):

              TimeOfYear timeOfYear = existingRecurrence.getRecurrenceTimeOfYear();
              DateTime newLastExpansion =
                  timeOfYear.getLastInstanceBeforeOrAt(tm().getTransactionTime());

just get the transaction time as an instant

---

core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java line 165 at r7 (raw file):

        tm().query("FROM Domain WHERE tld = :tld AND deletionTime > :exportTime", Domain.class)
            .setParameter("tld", tld)
            .setParameter("exportTime", toInstant(exportTime))

just make exportTime an instant instead, right?

---

core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java line 124 at r7 (raw file):

        };
    checkPermission(user, request.getRegistrarId(), requiredVerifyPermission);
    if (request.getRequestTime().plus(java.time.Duration.ofHours(1)).isBefore(tm().getTxTime())) {

i don't think we need the qualified class reference

---

core/src/test/java/google/registry/batch/BulkDomainTransferActionTest.java line 98 at r7 (raw file):

    fakeClock.advanceOneMilli();

    DateTime runTime = fakeClock.nowUtc();

grab these as instants so you don't need conversions

---

core/src/test/java/google/registry/bsa/persistence/BsaDomainRefreshTest.java line 44 at r7 (raw file):

        tm().transact(() -> tm().getEntityManager().merge(new BsaDomainRefresh()));
    assertThat(persisted.jobId).isNotNull();
    assertThat(persisted.creationTime.getTimestamp()).isEqualTo(toInstant(fakeClock.nowUtc()));

change the clock method, don't convert toInstant

---

core/src/test/java/google/registry/model/domain/GracePeriodTest.java line 44 at r7 (raw file):

      new JpaTestExtensions.Builder().buildIntegrationTestExtension();

  private final DateTime now = DateTime.now(UTC);

make this an instant to avoid converting everywhere

---

core/src/test/java/google/registry/bsa/persistence/BsaDownloadTest.java line 47 at r7 (raw file):

    BsaDownload persisted = tm().transact(() -> tm().getEntityManager().merge(new BsaDownload()));
    assertThat(persisted.jobId).isNotNull();
    assertThat(persisted.creationTime.getTimestamp()).isEqualTo(toInstant(fakeClock.nowUtc()));

change clock method, con't convert

---

core/src/test/java/google/registry/model/console/PasswordResetRequestTest.java line 50 at r7 (raw file):

        DatabaseHelper.loadByKey(VKey.create(PasswordResetRequest.class, verificationCode));
    assertAboutImmutableObjects().that(fromDatabase).isEqualExceptFields(request, "requestTime");
    assertThat(fromDatabase.getRequestTime()).isEqualTo(toInstant(fakeClock.nowUtc()));

just get the instant from the clock

---

core/src/test/java/google/registry/model/CreateAutoTimestampTest.java line 45 at r7 (raw file):

  public static class CreateAutoTimestampTestObject extends CrossTldSingleton {
    @Id long id = SINGLETON_ID;
    CreateAutoTimestamp createTime = CreateAutoTimestamp.create((java.time.Instant) null);

why qualified name?

---

core/src/test/java/google/registry/model/CreateAutoTimestampTest.java line 54 at r7 (raw file):

  @Test
  void testSaveSetsTime() {
    DateTime transactionTime =

use txnTime, don't convert

---

core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java line 182 at r7 (raw file):

                .build());
    // Use registrar key for contacts' parent.
    DateTime registrarCreationTime = toDateTime(persistResource(registrar).getCreationTime());

can't this be Instant?

---

core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java line 56 at r7 (raw file):

    ClaimsList insertedClaimsList = ClaimsListDao.get();
    assertClaimsListEquals(claimsList, insertedClaimsList);
    assertThat(toDateTime(insertedClaimsList.getCreationTime())).isEqualTo(fakeClock.nowUtc());

clock.now()

---

core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java line 76 at r7 (raw file):

        .isEqualTo(claimsList.getTmdbGenerationTime());
    assertThat(insertedClaimsList.getLabelsToKeys()).isEqualTo(claimsList.getLabelsToKeys());
    assertThat(toDateTime(insertedClaimsList.getCreationTime())).isEqualTo(fakeClock.nowUtc());

clock.now()

---

core/src/test/java/google/registry/model/domain/DomainSqlTest.java line 392 at r7 (raw file):

    persistDomain();
    Domain persisted = loadByKey(domain.createVKey());
    DateTime originalUpdateTime = toDateTime(persisted.getUpdateTimestamp().getTimestamp());

just use instants everywhere in this file instead of converting all over

---

core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java line 340 at r7 (raw file):

            .setRegistrationExpirationTime(DateTime.parse("2005-11-26T22:00:00.0Z"))
            .setLastTransferTime(null)
            .setLastEppUpdateTime((java.time.Instant) null)

why qualified class name?

---

core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java line 50 at r7 (raw file):

  public static class UpdateAutoTimestampTestObject extends CrossTldSingleton {
    @Id long id = SINGLETON_ID;
    UpdateAutoTimestamp updateTime = UpdateAutoTimestamp.create((java.time.Instant) null);

why qualified class name?

---

core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java line 69 at r7 (raw file):

                  return tm().getTransactionTime();
                });
    assertThat(reload().updateTime.getTimestamp()).isEqualTo(toInstant(transactionTime));

use txnTime, don't convert

---

core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java line 88 at r7 (raw file):

  @Test
  void testReadingTwiceDoesNotModify() {
    DateTime originalTime = DateTime.parse("1999-01-01T00:00:00Z");

just use an instant instead of converting

---

core/src/test/java/google/registry/model/domain/token/AllocationTokenTest.java line 147 at r7 (raw file):

    assertThat(tokenBeforePersisting.getCreationTime()).isEmpty();
    AllocationToken tokenAfterPersisting = persistResource(tokenBeforePersisting);
    assertThat(tokenAfterPersisting.getCreationTime()).hasValue(toInstant(fakeClock.nowUtc()));

now()

---

common/src/main/java/google/registry/util/DateTimeUtils.java line 120 at r4 (raw file):

  /** Returns the latest element in a {@link DateTime} iterable. */
  public static DateTime latestDateTimeOf(Iterable<DateTime> dates) {

why is this name changed and not the latestOf(DateTime first, DateTIme... rest) above? we should prob do both or neither

---

core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java line 92 at r7 (raw file):

    assertThat(persisted.getRegistrarId()).isEqualTo("registrarId");
    assertThat(persisted.getRegistrarName()).isEqualTo("registrarName");
    assertThat(persisted.getCreationTime()).isEqualTo(toInstant(fakeClock.nowUtc()));

clock.now()

---

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java line 105 at r7 (raw file):

    assertThat(registrar.getClientCertificateHash()).isEmpty();
    assertThat(registrar.getPhonePasscode()).isEqualTo("01234");
    assertThat(registrar.getCreationTime()).isIn(Range.closed(toInstant(before), toInstant(after)));

just use clock.now() to get the datetimes in both places

[CydeWeys]

@CydeWeys made 42 comments.
Reviewable status: 73 of 165 files reviewed, 58 unresolved discussions (waiting on gbrodman).

---

common/src/main/java/google/registry/util/DateTimeUtils.java line 120 at r4 (raw file):

Previously, gbrodman wrote…

why is this name changed and not the latestOf(DateTime first, DateTIme... rest) above? we should prob do both or neither

Because, thanks to runtime type erasure, you cannot have overloads of the same method name that both type-erase to identical Iterable<?> parameters. Whereas above, the name can be overloaded, as the type parameters of DateTime vs Instant are clearly different.

In the desired end state, both latestOf(DateTime first, DateTime... rest) and latestDateTimeOf(Iterable<DateTime> dates) will be deleted once the migration is complete, leaving only latestOf(Instant first, Instant... rest).

---

core/src/main/java/google/registry/beam/billing/ExpandBillingRecurrencesPipeline.java line 307 at r7 (raw file):

Previously, gbrodman wrote…

instead of converting to date times here, convert to instants when we load them

Done.

---

core/src/main/java/google/registry/bsa/persistence/DownloadSchedule.java line 40 at r7 (raw file):

Previously, gbrodman wrote…

switch this to Instant so we don't need to convert in the factory methods

Done.

---

core/src/main/java/google/registry/bsa/persistence/RefreshSchedule.java line 57 at r7 (raw file):

Previously, gbrodman wrote…

same here, change to Instant so we don't need to convert in the factory method

Done.

---

core/src/main/java/google/registry/flows/poll/PollFlowUtils.java line 85 at r7 (raw file):

Previously, gbrodman wrote…

Convert all these getEventTime calls to getEventTimeInstant. Otherwise you're just converting from instant to DT back to instant.

Done.

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 190 at r7 (raw file):

Previously, gbrodman wrote…

START_INSTANT

Done.

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 244 at r7 (raw file):

Previously, gbrodman wrote…

Bad javadoc update. It's a host.

Done.

---

core/src/main/java/google/registry/model/common/FeatureFlag.java line 186 at r7 (raw file):

Previously, gbrodman wrote…

I don't think this annotation is necessary. Did gemini just add this across all the classes? If so, you should check all of them to make sure that we're not adding it superfluously

Done.

---

core/src/main/java/google/registry/model/common/TimedTransitionProperty.java line 52 at r7 (raw file):

Previously, gbrodman wrote…

i don't think this sentence makes sense

actually wtf i don't think this inner class is ever used

Can you re-evaluate this now after the latest changes?

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 258 at r7 (raw file):

Previously, gbrodman wrote…

Why is only the given subset of classes migrated to Instant, and not like, this one too?

Done, but ... the problem is, the PR keeps growing in size when we migrate additional stuff, and at some point we have to stop well short of doing the entire codebase in one go.

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 511 at r7 (raw file):

Previously, gbrodman wrote…

i don't think we need the qualified names here, can import Duration

Done.

---

core/src/main/java/google/registry/model/tld/Tld.java line 361 at r7 (raw file):

Previously, gbrodman wrote…

i don't think these need jsonification? if so, why now?

Done.

---

core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java line 343 at r7 (raw file):

Previously, gbrodman wrote…

don't think you need to convert to datetime (same for all of these below in this class)

Done.

---

core/src/main/java/google/registry/rdap/RdapJsonFormatter.java line 327 at r7 (raw file):

Previously, gbrodman wrote…

i don't think we need the conversion here? doesn't this return (or at least, shouldn't it return) an Instant already?

Done.

---

core/src/main/java/google/registry/rdap/RdapNameserverAction.java line 69 at r7 (raw file):

Previously, gbrodman wrote…

i don't think we need the conversion to datetime here either

Done.

---

core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java line 179 at r7 (raw file):

Previously, gbrodman wrote…

FKUtils exposes Instant methods right ? so we shouldn't need all these conversions either

Done.

---

core/src/main/java/google/registry/tools/CountDomainsCommand.java line 54 at r7 (raw file):

Previously, gbrodman wrote…

just change now to be an instant, we just get it straight above

Done.

---

core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java line 68 at r7 (raw file):

Previously, gbrodman wrote…

we can just make the parameters instants, right?

Done, but, it made the PR larger.

---

core/src/main/java/google/registry/tools/UpdateRecurrenceCommand.java line 121 at r7 (raw file):

Previously, gbrodman wrote…

this file mixes datetimes and instants, can't we just change it so the input is an instant?

There are no DateTimes left in this file.

---

core/src/main/java/google/registry/tools/javascrap/RecreateBillingRecurrencesCommand.java line 100 at r7 (raw file):

Previously, gbrodman wrote…

just get the transaction time as an instant

Done.

---

core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java line 165 at r7 (raw file):

Previously, gbrodman wrote…

just make exportTime an instant instead, right?

Done.

---

core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java line 124 at r7 (raw file):

Previously, gbrodman wrote…

i don't think we need the qualified class reference

Done.

---

core/src/test/java/google/registry/batch/BulkDomainTransferActionTest.java line 98 at r7 (raw file):

Previously, gbrodman wrote…

grab these as instants so you don't need conversions

Done.

---

core/src/test/java/google/registry/bsa/persistence/BsaDomainRefreshTest.java line 44 at r7 (raw file):

Previously, gbrodman wrote…

change the clock method, don't convert toInstant

Done.

---

core/src/test/java/google/registry/bsa/persistence/BsaDownloadTest.java line 47 at r7 (raw file):

Previously, gbrodman wrote…

change clock method, con't convert

Done.

---

core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java line 182 at r7 (raw file):

Previously, gbrodman wrote…

can't this be Instant?

Done.

---

core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java line 340 at r7 (raw file):

Previously, gbrodman wrote…

why qualified class name?

Otherwise it won't know which overload to call, as null would work for both.

---

core/src/test/java/google/registry/model/CreateAutoTimestampTest.java line 45 at r7 (raw file):

Previously, gbrodman wrote…

why qualified name?

Same.

---

core/src/test/java/google/registry/model/CreateAutoTimestampTest.java line 54 at r7 (raw file):

Previously, gbrodman wrote…

use txnTime, don't convert

Done.

---

core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java line 69 at r7 (raw file):

Previously, gbrodman wrote…

use txnTime, don't convert

Done.

---

core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java line 88 at r7 (raw file):

Previously, gbrodman wrote…

just use an instant instead of converting

Done.

---

core/src/test/java/google/registry/model/console/PasswordResetRequestTest.java line 50 at r7 (raw file):

Previously, gbrodman wrote…

just get the instant from the clock

Done.

---

core/src/test/java/google/registry/model/domain/DomainSqlTest.java line 392 at r7 (raw file):

Previously, gbrodman wrote…

just use instants everywhere in this file instead of converting all over

Done.

---

core/src/test/java/google/registry/model/domain/GracePeriodTest.java line 44 at r7 (raw file):

Previously, gbrodman wrote…

make this an instant to avoid converting everywhere

Done.

---

core/src/test/java/google/registry/model/domain/token/AllocationTokenTest.java line 147 at r7 (raw file):

Previously, gbrodman wrote…

now()

Done.

---

core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java line 56 at r7 (raw file):

Previously, gbrodman wrote…

clock.now()

Done.

---

core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java line 76 at r7 (raw file):

Previously, gbrodman wrote…

clock.now()

Done.

---

core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java line 92 at r7 (raw file):

Previously, gbrodman wrote…

clock.now()

Done.

---

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java line 105 at r7 (raw file):

Previously, gbrodman wrote…

just use clock.now() to get the datetimes in both places

Done.

---

common/src/testing/java/google/registry/testing/truth/DateTimeSubject.java line 91 at r7 (raw file):

Previously, gbrodman wrote…

do we need this? i don't think so?

Done.

---

common/src/testing/java/google/registry/testing/truth/InstantSubject.java line 91 at r7 (raw file):

Previously, gbrodman wrote…

i also don't think we need this?

Done.

---

common/src/testing/java/google/registry/testing/truth/RegistryTruth.java line 27 at r4 (raw file):

Previously, gbrodman wrote…

i don't think this class is necessary? we can just call the subjects directly like we do with every other subject

Done.

[CydeWeys]

PTAL

@CydeWeys made 1 comment.
Reviewable status: 73 of 165 files reviewed, 58 unresolved discussions (waiting on gbrodman).

[CydeWeys]

@CydeWeys made 1 comment.
Reviewable status: 73 of 165 files reviewed, 58 unresolved discussions (waiting on gbrodman).

---

core/src/main/java/google/registry/bsa/persistence/Queries.java line 196 at r7 (raw file):

Previously, gbrodman wrote…

what object does Hibernate return to us? is this necessary? or, why do we have both options here?

Done.

[gbrodman]

it's very fortunate that the SQL serialization is the same and we don't really need to worry about DB compatibility so much

@gbrodman reviewed 101 files and all commit messages, made 15 comments, and resolved 44 discussions.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on CydeWeys).

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 258 at r7 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Done, but ... the problem is, the PR keeps growing in size when we migrate additional stuff, and at some point we have to stop well short of doing the entire codebase in one go.

I mean yeah I wasn't necessarily saying we must do everything. I was asking what the thought process / decision mechanism was.

---

core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java line 340 at r7 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Otherwise it won't know which overload to call, as null would work for both.

yeah i was referring to the full java.time bit but that's moot now

---

common/src/main/java/google/registry/util/DateTimeUtils.java line 68 at r13 (raw file):

   */
  public static final DateTimeFormatter ISO_8601_FORMATTER =
      new java.time.format.DateTimeFormatterBuilder()

more unnecessary qualified class names

---

core/src/main/java/google/registry/beam/billing/ExpandBillingRecurrencesPipeline.java line 451 at r12 (raw file):

                                          .orElse(
                                              Cursor.createGlobal(
                                                  RECURRING_BILLING, toDateTime(START_INSTANT)))

converting to a datetime just to call the deprecated method???? also same below

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 560 at r12 (raw file):

          plusYears(
              domain.getRegistrationExpirationTime(),
              java.time.temporal.ChronoUnit.YEARS.between(

why does it love doing this fully-qualified naming when it's not necessary?

---

core/src/main/java/google/registry/model/tld/Tlds.java line 168 at r12 (raw file):

  @SuppressWarnings("InlineMeSuggester")
  public static boolean hasActiveBsaEnrollment(DateTime now) {
    return hasActiveBsaEnrollment(google.registry.util.DateTimeUtils.toInstant(now));

more qualification! maybe add this to the GEMINI.md file?

---

core/src/main/java/google/registry/request/RequestParameters.java line 270 at r12 (raw file):

    try {
      return DateTimeUtils.parseInstant(stringValue);
    } catch (java.time.format.DateTimeParseException | IllegalArgumentException e) {

odd, more qualification

---

core/src/test/java/google/registry/beam/billing/ExpandBillingRecurrencesPipelineTest.java line 338 at r12 (raw file):

            .build());
    Instant otherCreateTime = plusYears(minusYears(startTime, 1), 0).plus(5, ChronoUnit.HOURS);
    // Wait, minusYears(startTime, 1).plus(5, HOURS) is enough.

haha what

---

core/src/test/java/google/registry/beam/billing/ExpandBillingRecurrencesPipelineTest.java line 544 at r12 (raw file):

  private static void assertCursorAt(DateTime expectedCursorTime) {
    assertCursorAt(google.registry.util.DateTimeUtils.toInstant(expectedCursorTime));

just import it

---

core/src/test/java/google/registry/beam/resave/ResaveAllEppResourcesPipelineTest.java line 103 at r12 (raw file):

                "domain",
                "tld",
                toDateTime(now.minus(5, java.time.temporal.ChronoUnit.DAYS)),

imports?

---

core/src/test/java/google/registry/bsa/persistence/DownloadSchedulerTest.java line 47 at r12 (raw file):

class DownloadSchedulerTest {

  static final Duration DOWNLOAD_INTERVAL = org.joda.time.Duration.standardMinutes(30);

we already import duration

---

core/src/test/java/google/registry/bsa/persistence/QueriesTest.java line 247 at r12 (raw file):

    // Now is time 2, ask for domains created since time 1
    assertThat(
            (ImmutableSet<String>)

is this cast necessary? same below

---

core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java line 94 at r12 (raw file):

    persistResource(
        Cursor.createGlobal(
            SYNC_REGISTRAR_SHEET, clock.now().minus(java.time.Duration.ofHours(1))));

import

---

core/src/test/java/google/registry/model/domain/DomainSqlTest.java line 105 at r12 (raw file):

            .setLaunchNotice(
                LaunchNotice.create(
                    "tcnid", "validatorId", (Instant) START_INSTANT, (Instant) START_INSTANT))

i wouldn't expect these casts to be necessary

[CydeWeys]

@CydeWeys made 12 comments.
Reviewable status: 157 of 175 files reviewed, 25 unresolved discussions (waiting on gbrodman).

---

common/src/main/java/google/registry/util/DateTimeUtils.java line 68 at r13 (raw file):

Previously, gbrodman wrote…

more unnecessary qualified class names

Done.

---

core/src/main/java/google/registry/beam/billing/ExpandBillingRecurrencesPipeline.java line 451 at r12 (raw file):

Previously, gbrodman wrote…

converting to a datetime just to call the deprecated method???? also same below

Done.

---

core/src/main/java/google/registry/model/domain/DomainBase.java line 560 at r12 (raw file):

Previously, gbrodman wrote…

why does it love doing this fully-qualified naming when it's not necessary?

I don't know. I've switched to 3.1 Pro and had it write even more instructions to stop doing this.

---

core/src/main/java/google/registry/model/tld/Tlds.java line 168 at r12 (raw file):

Previously, gbrodman wrote…

more qualification! maybe add this to the GEMINI.md file?

Done.

---

core/src/main/java/google/registry/request/RequestParameters.java line 270 at r12 (raw file):

Previously, gbrodman wrote…

odd, more qualification

Done.

---

core/src/test/java/google/registry/beam/billing/ExpandBillingRecurrencesPipelineTest.java line 338 at r12 (raw file):

Previously, gbrodman wrote…

haha what

Done.

---

core/src/test/java/google/registry/beam/billing/ExpandBillingRecurrencesPipelineTest.java line 544 at r12 (raw file):

Previously, gbrodman wrote…

just import it

Done.

---

core/src/test/java/google/registry/beam/resave/ResaveAllEppResourcesPipelineTest.java line 103 at r12 (raw file):

Previously, gbrodman wrote…

imports?

Done.

---

core/src/test/java/google/registry/bsa/persistence/DownloadSchedulerTest.java line 47 at r12 (raw file):

Previously, gbrodman wrote…

we already import duration

Done.

---

core/src/test/java/google/registry/bsa/persistence/QueriesTest.java line 247 at r12 (raw file):

Previously, gbrodman wrote…

is this cast necessary? same below

Done.

---

core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java line 94 at r12 (raw file):

Previously, gbrodman wrote…

import

Done.

---

core/src/test/java/google/registry/model/domain/DomainSqlTest.java line 105 at r12 (raw file):

Previously, gbrodman wrote…

i wouldn't expect these casts to be necessary

Done.

[CydeWeys]

I don't think that's by accident -- fundamentally they're representing the same kind of ISO 8601 datetime, and both were written by the same guy. I imagine not having them be compatible in this manner would've represented a failure of the adoption of Joda into java.time.

And PTAL.

@CydeWeys made 1 comment.
Reviewable status: 157 of 175 files reviewed, 25 unresolved discussions (waiting on gbrodman).

[gbrodman]

@gbrodman reviewed 21 files and all commit messages, and resolved 12 discussions.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on CydeWeys).