Add loadAllOfSorted to TransactionManager (#3136)

During our implementation of Expiry Access Period (XAP), adding a field to the
Registrar entity altered database heap scan order when loadAllOf() was
called without an explicit ORDER BY clause. This caused non-deterministic
row shifting in the Angular console registrar table (/console/registrars),
breaking golden image comparisons in ConsoleScreenshotTest.

To permanently fix this root cause and guarantee deterministic UI rendering
and test stability across schema evolutions, this commit introduces sorted
database loading across our persistence layer.

Specifically, this commit:
- Adds loadAllOfSorted and loadAllOfSortedStream to TransactionManager,
  JpaTransactionManagerImpl, and DelegatingReplicaJpaTransactionManager.
- Enforces strict allowlist regex validation (^[a-zA-Z0-9_.]+$) on property
  names in JpaTransactionManagerImpl before appending dynamic ORDER BY
  clauses, preventing JPQL/SQL injection since bind parameters cannot be
  used for schema identifiers.
- Adds Registrar.loadAllSorted and updates RegistrarsAction to explicitly
  sort registrars alphabetically by registrarName and registrarId.
- Updates golden screenshot
  ConsoleScreenshotTest_globalRole_registrars_registrarsPage.png to
  reflect the newly enforced deterministic alphabetical registrar ordering.
- Adds comprehensive unit test coverage in JpaTransactionManagerImplTest
  verifying exact sorted entity retrieval across multiple fields and regex
  rejection of invalid field names.

TAG=agy
CONV=f2488c74-8b4a-43f1-9c22-d1dddbdbb4e0
BUG=http://b/531889856

GEMINI.md

@@ -177,3 +177,4 @@ This protocol defines the standard for interacting with GitHub repositories and
 - **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.
+- **PR Description Synchronization:** Whenever you amend or update a commit's description, you **MUST** check whether the corresponding GitHub PR description (if one exists) matches the previous commit description. If the PR description was not manually customized (i.e., it simply reflects the older commit description), you must automatically update it via `gh pr edit` to keep it in sync with your newly updated commit description—**strictly after the updated commit has been pushed to the remote GitHub PR branch**. Do not update the remote PR description when changes are only committed locally. **CRITICAL:** When updating the PR description, you must strictly preserve any automated review links or footer blocks (such as Reviewable link blocks: `<!-- Reviewable:start -->...<!-- Reviewable:end -->`) at the bottom of the description.

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -1030,6 +1030,11 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J
     return tm().transact(() -> tm().loadAllOf(Registrar.class));
   }
 
+  /** Loads all registrar entities directly from the database, sorted by the given field names. */
+  public static Iterable<Registrar> loadAllSorted(String... sortFields) {
+    return tm().transact(() -> tm().loadAllOfSorted(Registrar.class, sortFields));
+  }
+
   /** Loads all registrar entities using an in-memory cache. */
   public static Iterable<Registrar> loadAllCached() {
     return CACHE_BY_REGISTRAR_ID.get().values();

core/src/main/java/google/registry/persistence/transaction/DelegatingReplicaJpaTransactionManager.java

@@ -257,11 +257,21 @@ public class DelegatingReplicaJpaTransactionManager implements JpaTransactionMan
     return getReplica().loadAllOf(clazz);
   }
 
+  @Override
+  public <T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields) {
+    return getReplica().loadAllOfSorted(clazz, sortFields);
+  }
+
   @Override
   public <T> Stream<T> loadAllOfStream(Class<T> clazz) {
     return getReplica().loadAllOfStream(clazz);
   }
 
+  @Override
+  public <T> Stream<T> loadAllOfSortedStream(Class<T> clazz, String... sortFields) {
+    return getReplica().loadAllOfSortedStream(clazz, sortFields);
+  }
+
   @Override
   public <T> Optional<T> loadSingleton(Class<T> clazz) {
     return getReplica().loadSingleton(clazz);

core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java

@@ -76,6 +76,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Supplier;
 import java.util.function.UnaryOperator;
+import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 import javax.annotation.Nullable;
@@ -88,6 +89,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
   private static final Retrier retrier = new Retrier(new SystemSleeper(), 6);
+
+  /**
+   * Strict allowlist regex for property/field names in dynamic JPQL ORDER BY clauses.
+   *
+   * <p>JPA and database engines forbid bind parameters (e.g. ? or :param) for schema identifiers or
+   * property names in ORDER BY clauses. To prevent JPQL/SQL injection when dynamically constructing
+   * sort queries, every sort field MUST be validated against this pattern before concatenation.
+   */
+  private static final Pattern VALID_SORT_FIELD_PATTERN = Pattern.compile("^[a-zA-Z0-9_.]+$");
+
   private static final String NESTED_TRANSACTION_MESSAGE =
       "Nested transaction detected. Try refactoring to avoid nested transactions. If unachievable,"
           + " use reTransact() in nested transactions";
@@ -528,15 +539,38 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   @Override
   public <T> ImmutableList<T> loadAllOf(Class<T> clazz) {
-    return loadAllOfStream(clazz).collect(toImmutableList());
+    return loadAllOfSorted(clazz);
   }
 
   @Override
   public <T> Stream<T> loadAllOfStream(Class<T> clazz) {
+    return loadAllOfSortedStream(clazz);
+  }
+
+  @Override
+  public <T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields) {
+    return loadAllOfSortedStream(clazz, sortFields).collect(toImmutableList());
+  }
+
+  @Override
+  public <T> Stream<T> loadAllOfSortedStream(Class<T> clazz, String... sortFields) {
     checkArgumentNotNull(clazz, "clazz must be specified");
+    checkArgumentNotNull(sortFields, "sortFields must not be null");
     assertInTransaction();
+    StringBuilder queryString =
+        new StringBuilder(String.format("FROM %s", getEntityType(clazz).getName()));
+    if (sortFields.length > 0) {
+      for (String field : sortFields) {
+        checkArgument(
+            VALID_SORT_FIELD_PATTERN.matcher(field).matches(),
+            "Invalid sort field name: %s",
+            field);
+      }
+      queryString.append(" ORDER BY ");
+      queryString.append(String.join(", ", sortFields));
+    }
     return getEntityManager()
-        .createQuery(String.format("FROM %s", getEntityType(clazz).getName()), clazz)
+        .createQuery(queryString.toString(), clazz)
         .getResultStream()
         .map(this::detach);
   }

core/src/main/java/google/registry/persistence/transaction/TransactionManager.java

@@ -219,6 +219,14 @@ public interface TransactionManager {
    */
   <T> ImmutableList<T> loadAllOf(Class<T> clazz);
 
+  /**
+   * Returns a list of all entities of the given type that exist in the database, ordered by the
+   * specified field names in ascending order.
+   *
+   * <p>The resulting list is empty if there are no entities of this type.
+   */
+  <T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields);
+
   /**
    * Returns a stream of all entities of the given type that exist in the database.
    *
@@ -226,6 +234,14 @@ public interface TransactionManager {
    */
   <T> Stream<T> loadAllOfStream(Class<T> clazz);
 
+  /**
+   * Returns a stream of all entities of the given type that exist in the database, ordered by the
+   * specified field names in ascending order.
+   *
+   * <p>The resulting stream is empty if there are no entities of this type.
+   */
+  <T> Stream<T> loadAllOfSortedStream(Class<T> clazz, String... sortFields);
+
   /**
    * Loads the only instance of this particular class, or empty if none exists.
    *

core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java

@@ -59,6 +59,7 @@ public class RegistrarsAction extends ConsoleApiAction {
       """
             SELECT * FROM "Registrar"
             WHERE registrar_id in :registrarIds
+            ORDER BY registrar_name ASC, registrar_id ASC
       """;
   static final String PATH = "/console-api/registrars";
   private final Optional<Registrar> registrar;
@@ -83,7 +84,7 @@ public class RegistrarsAction extends ConsoleApiAction {
       ImmutableSet<Registrar.Type> allowedRegistrarTypes =
           user.getUserRoles().isAdmin() ? TYPES_ALLOWED_FOR_ADMINS : TYPES_ALLOWED_FOR_USERS;
       ImmutableList<Registrar> registrars =
-          Streams.stream(Registrar.loadAll())
+          Streams.stream(Registrar.loadAllSorted("registrarName", "registrarId"))
               .filter(r -> allowedRegistrarTypes.contains(r.getType()))
               .collect(ImmutableList.toImmutableList());
       consoleApiParams.response().setPayload(consoleApiParams.gson().toJson(registrars));

core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java

@@ -404,6 +404,39 @@ class JpaTransactionManagerImplTest {
         .containsExactlyElementsIn(moreEntities);
   }
 
+  @Test
+  void loadAllOfSorted() {
+    TestEntity entityB = new TestEntity("b_entity", "gamma");
+    TestEntity entityC = new TestEntity("c_entity", "alpha");
+    TestEntity entityA = new TestEntity("a_entity", "beta");
+    persistResources(ImmutableList.of(entityB, entityC, entityA));
+
+    assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "name")))
+        .containsExactly(entityA, entityB, entityC)
+        .inOrder();
+
+    assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "data")))
+        .containsExactly(entityC, entityA, entityB)
+        .inOrder();
+
+    assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "data", "name")))
+        .containsExactly(entityC, entityA, entityB)
+        .inOrder();
+  }
+
+  @Test
+  void loadAllOfSorted_invalidFieldName_throwsException() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                tm().transact(
+                        () ->
+                            tm().loadAllOfSorted(
+                                    TestEntity.class, "name; DROP TABLE TestEntity;")));
+    assertThat(thrown).hasMessageThat().contains("Invalid sort field name");
+  }
+
   @Test
   void saveAllNew_rollsBackWhenFailure() {
     moreEntities.forEach(entity -> assertThat(tm().transact(() -> tm().exists(entity))).isFalse());