Add methods to return subtypes of HistoryEntry when querying (#1172)

This is useful when we expect a specific subtype in the return value so
that we can set the parent resource (e. g. DomainContent for
DomainHistory) on it, or when a specific subtype is needed from the call
site.

This PR also fixes some use of generic return values. It is always better to
return <HistoryEntry> than a wildcard <? extends HistoryEntry>, because for
immutable collections, <? extends HistoryEntry> is no different than
<HistoryEntry> as return value -- you can only get a HistoryEntry from it.
The wildcard return value means that even if you are indeed getting a
<DomainHistory> from the query, the call site has no compile time knowledge of
it and can only assume it is a <HistoryEntry>.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/google/nomulus/1172)
<!-- Reviewable:end -->

core/build.gradle

@@ -317,6 +317,7 @@ dependencies {
   testCompile deps['com.google.monitoring-client:contrib']
   testCompile deps['com.google.truth:truth']
   testCompile deps['com.google.truth.extensions:truth-java8-extension']
+  testCompile deps['org.checkerframework:checker-qual']
   testCompile deps['org.hamcrest:hamcrest']
   testCompile deps['org.hamcrest:hamcrest-core']
   testCompile deps['org.hamcrest:hamcrest-library']

core/src/main/java/google/registry/model/reporting/HistoryEntryDao.java

@@ -14,6 +14,7 @@
 
 package google.registry.model.reporting;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.model.ofy.ObjectifyService.auditedOfy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
@@ -49,7 +50,7 @@ import org.joda.time.DateTime;
 public class HistoryEntryDao {
 
   /** Loads all history objects in the times specified, including all types. */
-  public static ImmutableList<? extends HistoryEntry> loadAllHistoryObjects(
+  public static ImmutableList<HistoryEntry> loadAllHistoryObjects(
       DateTime afterTime, DateTime beforeTime) {
     if (tm().isOfy()) {
       return Streams.stream(
@@ -77,13 +78,22 @@ public class HistoryEntryDao {
   }
 
   /** Loads all history objects corresponding to the given {@link EppResource}. */
-  public static ImmutableList<? extends HistoryEntry> loadHistoryObjectsForResource(
+  public static ImmutableList<HistoryEntry> loadHistoryObjectsForResource(
       VKey<? extends EppResource> parentKey) {
     return loadHistoryObjectsForResource(parentKey, START_OF_TIME, END_OF_TIME);
   }
 
+  /**
+   * Loads all history objects corresponding to the given {@link EppResource} and casted to the
+   * appropriate subclass.
+   */
+  public static <T extends HistoryEntry> ImmutableList<T> loadHistoryObjectsForResource(
+      VKey<? extends EppResource> parentKey, Class<T> subclazz) {
+    return loadHistoryObjectsForResource(parentKey, START_OF_TIME, END_OF_TIME, subclazz);
+  }
+
   /** Loads all history objects in the time period specified for the given {@link EppResource}. */
-  public static ImmutableList<? extends HistoryEntry> loadHistoryObjectsForResource(
+  public static ImmutableList<HistoryEntry> loadHistoryObjectsForResource(
       VKey<? extends EppResource> parentKey, DateTime afterTime, DateTime beforeTime) {
     if (tm().isOfy()) {
       return Streams.stream(
@@ -102,8 +112,35 @@ public class HistoryEntryDao {
     }
   }
 
+  /**
+   * Loads all history objects in the time period specified for the given {@link EppResource} and
+   * casted to the appropriate subclass.
+   *
+   * <p>Note that the subclass must be explicitly provided because we need the compile time
+   * information of T to return an {@code ImmutableList<T>}, even though at runtime we can call
+   * {@link #getHistoryClassFromParent(Class)} to obtain it, which we also did to confirm that the
+   * provided subclass is indeed correct.
+   */
+  public static <T extends HistoryEntry> ImmutableList<T> loadHistoryObjectsForResource(
+      VKey<? extends EppResource> parentKey,
+      DateTime afterTime,
+      DateTime beforeTime,
+      Class<T> subclazz) {
+    Class<? extends HistoryEntry> expectedSubclazz = getHistoryClassFromParent(parentKey.getKind());
+    checkArgument(
+        subclazz.equals(expectedSubclazz),
+        "The supplied HistoryEntry subclass %s is incompatible with the EppResource %s, "
+            + "use %s instead",
+        subclazz.getSimpleName(),
+        parentKey.getKind().getSimpleName(),
+        expectedSubclazz.getSimpleName());
+    return loadHistoryObjectsForResource(parentKey, afterTime, beforeTime).stream()
+        .map(subclazz::cast)
+        .collect(toImmutableList());
+  }
+
   /** Loads all history objects from all time from the given registrars. */
-  public static Iterable<? extends HistoryEntry> loadHistoryObjectsByRegistrars(
+  public static Iterable<HistoryEntry> loadHistoryObjectsByRegistrars(
       ImmutableCollection<String> registrarIds) {
     if (tm().isOfy()) {
       return auditedOfy()
@@ -124,8 +161,8 @@ public class HistoryEntryDao {
     }
   }
 
-  private static Stream<? extends HistoryEntry> loadHistoryObjectFromSqlByRegistrars(
+  private static <T extends HistoryEntry> Stream<T> loadHistoryObjectFromSqlByRegistrars(
-      Class<? extends HistoryEntry> historyClass, ImmutableCollection<String> registrarIds) {
+      Class<T> historyClass, ImmutableCollection<String> registrarIds) {
     return jpaTm()
         .getEntityManager()
         .createQuery(
@@ -135,7 +172,7 @@ public class HistoryEntryDao {
         .getResultStream();
   }
 
-  private static ImmutableList<? extends HistoryEntry> loadHistoryObjectsForResourceFromSql(
+  private static ImmutableList<HistoryEntry> loadHistoryObjectsForResourceFromSql(
       VKey<? extends EppResource> parentKey, DateTime afterTime, DateTime beforeTime) {
     // The class we're searching from is based on which parent type (e.g. Domain) we have
     Class<? extends HistoryEntry> historyClass = getHistoryClassFromParent(parentKey.getKind());
@@ -174,8 +211,8 @@ public class HistoryEntryDao {
         : historyClass.equals(DomainHistory.class) ? "domainRepoId" : "hostRepoId";
   }
 
-  private static List<? extends HistoryEntry> loadAllHistoryObjectsFromSql(
+  private static <T extends HistoryEntry> List<T> loadAllHistoryObjectsFromSql(
-      Class<? extends HistoryEntry> historyClass, DateTime afterTime, DateTime beforeTime) {
+      Class<T> historyClass, DateTime afterTime, DateTime beforeTime) {
     CriteriaBuilder criteriaBuilder = jpaTm().getEntityManager().getCriteriaBuilder();
     return jpaTm()
         .getEntityManager()

core/src/test/java/google/registry/testing/DatabaseHelper.java

@@ -1101,10 +1101,19 @@ public class DatabaseHelper {
   }
 
   /** Returns all of the history entries that are parented off the given EppResource. */
-  public static List<? extends HistoryEntry> getHistoryEntries(EppResource resource) {
+  public static List<HistoryEntry> getHistoryEntries(EppResource resource) {
     return HistoryEntryDao.loadHistoryObjectsForResource(resource.createVKey());
   }
 
+  /**
+   * Returns all of the history entries that are parented off the given EppResource, casted to the
+   * corresponding subclass.
+   */
+  public static <T extends HistoryEntry> List<T> getHistoryEntries(
+      EppResource resource, Class<T> subclazz) {
+    return HistoryEntryDao.loadHistoryObjectsForResource(resource.createVKey(), subclazz);
+  }
+
   /**
    * Returns all of the history entries that are parented off the given EppResource with the given
    * type.
@@ -1112,7 +1121,18 @@ public class DatabaseHelper {
   public static ImmutableList<HistoryEntry> getHistoryEntriesOfType(
       EppResource resource, final HistoryEntry.Type type) {
     return getHistoryEntries(resource).stream()
-        .filter(entry -> entry.getType() == type)
+        .filter(entry -> entry.getType().equals(type))
+        .collect(toImmutableList());
+  }
+
+  /**
+   * Returns all of the history entries that are parented off the given EppResource with the given
+   * type and casted to the corresponding subclass.
+   */
+  public static <T extends HistoryEntry> ImmutableList<T> getHistoryEntriesOfType(
+      EppResource resource, final HistoryEntry.Type type, Class<T> subclazz) {
+    return getHistoryEntries(resource, subclazz).stream()
+        .filter(entry -> entry.getType().equals(type))
         .collect(toImmutableList());
   }
 
@@ -1122,9 +1142,16 @@ public class DatabaseHelper {
    */
   public static HistoryEntry getOnlyHistoryEntryOfType(
       EppResource resource, final HistoryEntry.Type type) {
-    List<HistoryEntry> historyEntries = getHistoryEntriesOfType(resource, type);
+    return Iterables.getOnlyElement(getHistoryEntriesOfType(resource, type));
-    assertThat(historyEntries).hasSize(1);
+  }
-    return historyEntries.get(0);
+
+  /**
+   * Returns the only history entry of the given type, casted to the corresponding subtype, and
+   * throws an AssertionError if there are zero or more than one.
+   */
+  public static <T extends HistoryEntry> T getOnlyHistoryEntryOfType(
+      EppResource resource, final HistoryEntry.Type type, Class<T> subclazz) {
+    return Iterables.getOnlyElement(getHistoryEntriesOfType(resource, type, subclazz));
   }
 
   private static HistoryEntry.Type getHistoryEntryType(EppResource resource) {

dependencies.gradle

@@ -120,6 +120,7 @@ ext {
       'jline:jline:1.0',
       'joda-time:joda-time:2.9.2',
       'junit:junit:4.13',
+      'org.checkerframework:checker-qual:3.9.1',
       'org.junit.jupiter:junit-jupiter-api:5.6.2',
       'org.junit.jupiter:junit-jupiter-engine:5.6.2',
       'org.junit.jupiter:junit-jupiter-migrationsupport:5.6.2',