Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions GEMINI.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ This document outlines foundational mandates, architectural patterns, and projec
- **CRITICAL MISTAKES TO AVOID:**
- NEVER use `toInstant(clock.nowUtc())` or `toInstant(fakeClock.nowUtc())`. Both `Clock` and `FakeClock` have a `now()` method that natively returns a `java.time.Instant`. You MUST use `clock.now()` or `fakeClock.now()` directly.
- NEVER double-wrap conversions like `toInstant(toDateTime(...))` or `toDateTime(toInstant(...))`.
- NEVER mark `Instant` parameters or local variables as `final` unnecessarily, as it clutters the codebase.
- NEVER mark method parameters or local variables as `final` unnecessarily, as it clutters the codebase. For class fields and constants, use `final` where applicable (i.e. when the field is assigned once and never mutated) to enforce and communicate immutability.
- When using test helpers like `assertThatCommand().atTime(...)` or `ForeignKeyUtils.loadResource(...)`, ALWAYS use the `Instant` overloads. DO NOT wrap `Instant` instances in `toDateTime(...)` just to pass them to deprecated overloads.
- **UTC Timezones:** Do not use `ZoneId.of("UTC")`. Use a statically imported `UTC` from `ZoneOffset` instead (`import static java.time.ZoneOffset.UTC;`).
- **Millisecond Precision:** Always truncate `Instant.now()` to milliseconds (using `.truncatedTo(ChronoUnit.MILLIS)`) to maintain consistency with Joda `DateTime` and the PostgreSQL schema (which enforces millisecond precision via JPA converters).
Expand Down Expand Up @@ -77,6 +77,7 @@ Based on historical PR reviews, avoid the following common mistakes:
- **Immutable Types:** Declare variables, fields, and return types explicitly as Guava immutable types (e.g., `ImmutableList<T>`, `ImmutableMap<K, V>`) instead of their generic interfaces (`List<T>`, `Map<K, V>`) to clearly communicate immutability contracts to callers. Use `toImmutableList()` and `toImmutableMap()` collectors in streams rather than manually accumulating into an `ArrayList` or `HashMap`.
- **Constructors:** Do not perform heavy logic, I/O, or external API calls inside constructors. Initialization logic should be deferred or handled in a factory method or a dedicated startup routine.
- **Exception Handling:** Do not catch generic `Exception` or `Throwable` if a more specific exception is expected. Never "log and re-throw" the same exception; either handle it entirely (and log), or throw it up the chain. For batch processes, catch exceptions at the individual item/chunk level so one failure doesn't abort the entire batch.
- **Optional Assertions:** Prefer Truth's `.hasValue(...)` over `.isEqualTo(Optional.of(...))` for cleaner and more descriptive assertions on `Optional` types.
- **Fail Fast:** Validate inputs and fail fast (using `Preconditions.checkArgument` or similar) at the highest level possible rather than passing invalid state (like `null`s) deeper into business logic.
- **Magic Numbers:** Always document magic numbers or hardcoded limits (like `50.0` or `30`) with inline comments explaining the rationale.
- **Null Safety and Optional:** Prefer using `Optional` for any variable that is expected to potentially be null. For any other variable that can be null but cannot use an `Optional` (e.g., function parameters or return types where `Optional` is not idiomatic), it MUST be annotated with `@Nullable`. Always use the `javax.annotation.Nullable` annotation.
Expand Down Expand Up @@ -119,6 +120,7 @@ Before finalizing any PR or declaring a task complete, you MUST perform a thorou
5. **Diff Scope:** Are there any formatting-only changes in files that I did not functionally modify? If so, revert them. Does the total line count of the diff align with the approved scope (e.g., ~1,000 lines for migrations)?
6. **Commit Message:** Does the commit message title fit within 50 characters? Does the body encapsulate the entirety of the changes across the diff cleanly and professionally?
7. **Missing Tests & Coverage:** *Perform a structured check for any new methods or modified behavior.* Did I add a new utility method (like `plusMonths(Instant, int)`) or change core logic? If so, I MUST open the corresponding test file and write tests to cover the new functionality (including edge cases, negative values, and leap years) before considering the task complete. A code review is not thorough if it only checks for compilation. I must actively ensure every new branch of logic has a test.
8. **Package Lock:** Did I include `console-webapp/package-lock.json` in my diff? If so, I MUST revert it (`git checkout console-webapp/package-lock.json`) unless I explicitly intended to modify NPM dependencies. This file is often modified by the build process and should not be committed accidentally.

Only after actively confirming these checks against your diff are you permitted to finalize the task.

Expand Down Expand Up @@ -156,20 +158,20 @@ This project treats Error Prone warnings as errors.

---

# GitHub and Pull Request Protocol
## GitHub and Pull Request Protocol

This protocol defines the standard for interacting with GitHub repositories and processing Pull Request (PR) feedback.

## 1. Interaction via `gh` CLI
### 1. Interaction via `gh` CLI
- **Primary Tool:** ALWAYS use the `gh` CLI for all GitHub-related operations (listing PRs, viewing PR content, checking status, adding comments).
- **Credential Safety:** Never expose tokens or credentials in shell commands.

## 2. Processing PR Feedback
### 2. Processing PR Feedback
- **Systematic Review:** When asked to address PR comments, first fetch all comments using `gh pr view <number> --json reviews,comments`.
- **Minimal Scope Expansion:** Address comments surgically. If a fix requires changes beyond a few lines or expands the PR's original scope significantly, DO NOT implement it without explicit user approval. Instead, report the issue to the user.
- **Verification:** After addressing feedback, run the full build (`./gradlew build`) and relevant tests to ensure no regressions were introduced.

## 3. PR Lifecycle Management
### 3. PR Lifecycle Management
- **One Commit Per PR:** Ensure all changes are squashed into a single, clean commit. Use `git commit --amend --no-edit` for follow-up fixes.
- **Clean Workspace:** Always run `git status` and verify the repository state before declaring a task complete.
- **Package Lock:** The Gradle build automatically modifies `console-webapp/package-lock.json` via the `npmInstallDeps` task. ALWAYS revert this file (`git checkout console-webapp/package-lock.json`) before staging changes or finalizing a commit unless you explicitly modified NPM dependencies.
38 changes: 32 additions & 6 deletions common/src/main/java/google/registry/util/DateTimeUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package google.registry.util;

import static com.google.common.base.Preconditions.checkArgument;
import static org.joda.time.DateTimeZone.UTC;

import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
Expand All @@ -30,14 +31,14 @@
import java.time.temporal.ChronoUnit;
import javax.annotation.Nullable;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.LocalDate;
import org.joda.time.ReadableDuration;

/** Utilities methods and constants related to Joda {@link DateTime} objects. */
public abstract class DateTimeUtils {

/** The start of the epoch, in a convenient constant. */
@Deprecated public static final DateTime START_OF_TIME = new DateTime(0, DateTimeZone.UTC);
@Deprecated public static final DateTime START_OF_TIME = new DateTime(0, UTC);

/** The start of the UNIX epoch (which is defined in UTC), in a convenient constant. */
public static final Instant START_INSTANT = Instant.ofEpochMilli(0);
Expand All @@ -49,8 +50,7 @@ public abstract class DateTimeUtils {
* but Java uses milliseconds, so this is the largest representable date that will survive a
* round-trip through the database.
*/
@Deprecated
public static final DateTime END_OF_TIME = new DateTime(Long.MAX_VALUE / 1000, DateTimeZone.UTC);
@Deprecated public static final DateTime END_OF_TIME = new DateTime(Long.MAX_VALUE / 1000, UTC);

/**
* An instant in the far future that we can treat as infinity.
Expand All @@ -76,6 +76,10 @@ public abstract class DateTimeUtils {
.toFormatter()
.withZone(ZoneOffset.UTC);

/** A formatter that produces lowercase, filename-safe and job-name-safe timestamps. */
public static final DateTimeFormatter LOWERCASE_TIMESTAMP_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd't'HH-mm-ss'z'").withZone(ZoneOffset.UTC);

/** Formats an {@link Instant} to an ISO-8601 string. */
public static String formatInstant(Instant instant) {
return ISO_8601_FORMATTER.format(instant);
Expand Down Expand Up @@ -147,6 +151,18 @@ public static boolean isBeforeOrAt(DateTime timeToCheck, DateTime timeToCompareT
return !timeToCheck.isAfter(timeToCompareTo);
}

/** Converts a Joda-Time Duration to a java.time.Duration. */
@Nullable
public static java.time.Duration toJavaDuration(@Nullable ReadableDuration duration) {
return duration == null ? null : java.time.Duration.ofMillis(duration.getMillis());
}

/** Converts a java.time.Duration to a Joda-Time Duration. */
@Nullable
public static org.joda.time.Duration toJodaDuration(@Nullable java.time.Duration duration) {
return duration == null ? null : org.joda.time.Duration.millis(duration.toMillis());
}

/** Returns whether the first {@link Instant} is equal to or earlier than the second. */
public static boolean isBeforeOrAt(Instant timeToCheck, Instant timeToCompareTo) {
return !timeToCheck.isAfter(timeToCompareTo);
Expand Down Expand Up @@ -237,7 +253,17 @@ public static Date toSqlDate(LocalDate localDate) {
}

public static LocalDate toLocalDate(Date date) {
return new LocalDate(date.getTime(), DateTimeZone.UTC);
return new LocalDate(date.getTime(), UTC);
}

/** Converts a java.time.LocalDate to a Joda-Time LocalDate. */
public static LocalDate toJodaLocalDate(java.time.LocalDate localDate) {
return new LocalDate(localDate.getYear(), localDate.getMonthValue(), localDate.getDayOfMonth());
}

/** Converts an Instant to a Joda-Time LocalDate in UTC. */
public static LocalDate toJodaLocalDate(Instant instant) {
return new LocalDate(instant.toEpochMilli(), UTC);
}

/** Convert a joda {@link DateTime} to a java.time {@link Instant}, null-safe. */
Expand All @@ -249,7 +275,7 @@ public static Instant toInstant(@Nullable DateTime dateTime) {
/** Convert a java.time {@link Instant} to a joda {@link DateTime}, null-safe. */
@Nullable
public static DateTime toDateTime(@Nullable Instant instant) {
return (instant == null) ? null : new DateTime(instant.toEpochMilli(), DateTimeZone.UTC);
return (instant == null) ? null : new DateTime(instant.toEpochMilli(), UTC);
}

/** Convert a java.time {@link java.time.Instant} to a joda {@link org.joda.time.Instant}. */
Expand Down
38 changes: 38 additions & 0 deletions common/src/main/java/google/registry/util/Sleeper.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package google.registry.util;

import java.time.Duration;
import javax.annotation.concurrent.ThreadSafe;
import org.joda.time.ReadableDuration;

Expand All @@ -32,6 +33,15 @@ public interface Sleeper {
*/
void sleep(ReadableDuration duration) throws InterruptedException;

/**
* Puts the current thread to sleep.
*
* @throws InterruptedException if this thread was interrupted
*/
default void sleep(Duration duration) throws InterruptedException {
sleep(DateTimeUtils.toJodaDuration(duration));
}

/**
* Puts the current thread to sleep, ignoring interrupts.
*
Expand All @@ -42,6 +52,18 @@ public interface Sleeper {
*/
void sleepUninterruptibly(ReadableDuration duration);

/**
* Puts the current thread to sleep, ignoring interrupts.
*
* <p>If {@link InterruptedException} was caught, then {@code Thread.currentThread().interrupt()}
* will be called at the end of the {@code duration}.
*
* @see com.google.common.util.concurrent.Uninterruptibles#sleepUninterruptibly
*/
default void sleepUninterruptibly(Duration duration) {
sleepUninterruptibly(DateTimeUtils.toJodaDuration(duration));
}

/**
* Puts the current thread to interruptible sleep.
*
Expand All @@ -57,4 +79,20 @@ default void sleepInterruptibly(ReadableDuration duration) {
throw new RuntimeException("Interrupted.", e);
}
}

/**
* Puts the current thread to interruptible sleep.
*
* <p>This is a convenience method for {@link #sleep} that properly converts an {@link
* InterruptedException} to a {@link RuntimeException}.
*/
default void sleepInterruptibly(Duration duration) {
try {
sleep(duration);
} catch (InterruptedException e) {
// Restore current thread's interrupted state.
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted.", e);
}
}
}
4 changes: 2 additions & 2 deletions common/src/main/java/google/registry/util/SystemClock.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@

package google.registry.util;

import static java.time.temporal.ChronoUnit.MILLIS;
import static org.joda.time.DateTimeZone.UTC;

import jakarta.inject.Inject;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import javax.annotation.concurrent.ThreadSafe;
import org.joda.time.DateTime;

Expand All @@ -43,6 +43,6 @@ public Instant now() {
// (which uses millisecond precision via DateTimeConverter). This prevents subtle comparison
// bugs where a high-precision Instant would be considered "after" a truncated database
// timestamp.
return Instant.now().truncatedTo(ChronoUnit.MILLIS);
return Instant.now().truncatedTo(MILLIS);
}
}
4 changes: 2 additions & 2 deletions common/src/main/java/google/registry/util/SystemSleeper.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
package google.registry.util;

import static com.google.common.base.Preconditions.checkArgument;
import static google.registry.util.DateTimeUtils.toJavaDuration;

import com.google.common.util.concurrent.Uninterruptibles;
import jakarta.inject.Inject;
import java.io.Serializable;
import java.time.Duration;
import javax.annotation.concurrent.ThreadSafe;
import org.joda.time.ReadableDuration;

Expand All @@ -41,6 +41,6 @@ public void sleep(ReadableDuration duration) throws InterruptedException {
@Override
public void sleepUninterruptibly(ReadableDuration duration) {
checkArgument(duration.getMillis() >= 0);
Uninterruptibles.sleepUninterruptibly(Duration.ofMillis(duration.getMillis()));
Uninterruptibles.sleepUninterruptibly(toJavaDuration(duration));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ public void advanceBy(ReadableDuration duration) {
currentTimeMillis.addAndGet(duration.getMillis());
}

/** Advances clock by some duration. */
public void advanceBy(java.time.Duration duration) {
currentTimeMillis.addAndGet(duration.toMillis());
}

/** Sets the time to the specified instant. */
public void setTo(ReadableInstant time) {
currentTimeMillis.set(time.getMillis());
Expand Down
Loading
Loading