Skip synthetic history entries for resources that don't need them (#1320)

* Skip synthetic history entries for resources that don't need them

The reason for creating synthetic history entries is so that we can
guarantee that each EppResource's most recent *History object contains
that resource at that point in time. If the most recent *History object
in SQL contains that resource already, there is no need to create a
synthetic *History object for that resource.

config/presubmits.py

@@ -206,15 +206,12 @@ PRESUBMITS = {
          "RegistryJpaIO.java",
          # TODO(b/179158393): Remove everything below, which should be done
          # using Criteria
-         "ForeignKeyIndex.java",
-         "HistoryEntryDao.java",
          "JpaTransactionManager.java",
          "JpaTransactionManagerImpl.java",
          # CriteriaQueryBuilder is a false positive
          "CriteriaQueryBuilder.java",
          "RdapDomainSearchAction.java",
          "RdapNameserverSearchAction.java",
-         "RdapSearchActionBase.java",
          "ReadOnlyCheckingEntityManager.java",
          "RegistryQuery",
          },

core/src/main/java/google/registry/model/index/ForeignKeyIndex.java

@@ -228,7 +228,7 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
           tm().transact(
                   () ->
                       jpaTm()
-                          .query(
+                          .criteriaQuery(
                               CriteriaQueryBuilder.create(clazz)
                                   .whereFieldIsIn(property, foreignKeys)
                                   .build())

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

@@ -24,6 +24,7 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Streams;
 import google.registry.model.EppResource;
 import google.registry.model.contact.ContactHistory;
@@ -49,6 +50,25 @@ import org.joda.time.DateTime;
  */
 public class HistoryEntryDao {
 
+  public static ImmutableMap<Class<? extends EppResource>, Class<? extends HistoryEntry>>
+      RESOURCE_TYPES_TO_HISTORY_TYPES =
+          ImmutableMap.of(
+              ContactResource.class,
+              ContactHistory.class,
+              DomainBase.class,
+              DomainHistory.class,
+              HostResource.class,
+              HostHistory.class);
+
+  public static ImmutableMap<Class<? extends HistoryEntry>, String> REPO_ID_FIELD_NAMES =
+      ImmutableMap.of(
+          ContactHistory.class,
+          "contactRepoId",
+          DomainHistory.class,
+          "domainRepoId",
+          HostHistory.class,
+          "hostRepoId");
+
   /** Loads all history objects in the times specified, including all types. */
   public static ImmutableList<HistoryEntry> loadAllHistoryObjects(
       DateTime afterTime, DateTime beforeTime) {
@@ -164,7 +184,7 @@ public class HistoryEntryDao {
   private static <T extends HistoryEntry> Stream<T> loadHistoryObjectFromSqlByRegistrars(
       Class<T> historyClass, ImmutableCollection<String> registrarIds) {
     return jpaTm()
-        .query(
+        .criteriaQuery(
             CriteriaQueryBuilder.create(historyClass)
                 .whereFieldIsIn("clientId", registrarIds)
                 .build())
@@ -188,34 +208,32 @@ public class HistoryEntryDao {
 
     return ImmutableList.sortedCopyOf(
         Comparator.comparing(HistoryEntry::getModificationTime),
-        jpaTm().query(criteriaQuery).getResultList());
+        jpaTm().criteriaQuery(criteriaQuery).getResultList());
   }
 
   private static Class<? extends HistoryEntry> getHistoryClassFromParent(
       Class<? extends EppResource> parent) {
-    if (parent.equals(ContactResource.class)) {
+    if (!RESOURCE_TYPES_TO_HISTORY_TYPES.containsKey(parent)) {
-      return ContactHistory.class;
-    } else if (parent.equals(DomainBase.class)) {
-      return DomainHistory.class;
-    } else if (parent.equals(HostResource.class)) {
-      return HostHistory.class;
-    }
       throw new IllegalArgumentException(
           String.format("Unknown history type for parent %s", parent.getName()));
     }
+    return RESOURCE_TYPES_TO_HISTORY_TYPES.get(parent);
+  }
 
   private static String getRepoIdFieldNameFromHistoryClass(
       Class<? extends HistoryEntry> historyClass) {
-    return historyClass.equals(ContactHistory.class)
+    if (!REPO_ID_FIELD_NAMES.containsKey(historyClass)) {
-        ? "contactRepoId"
+      throw new IllegalArgumentException(
-        : historyClass.equals(DomainHistory.class) ? "domainRepoId" : "hostRepoId";
+          String.format("Unknown history type %s", historyClass.getName()));
+    }
+    return REPO_ID_FIELD_NAMES.get(historyClass);
   }
 
   private static <T extends HistoryEntry> List<T> loadAllHistoryObjectsFromSql(
       Class<T> historyClass, DateTime afterTime, DateTime beforeTime) {
     CriteriaBuilder criteriaBuilder = jpaTm().getEntityManager().getCriteriaBuilder();
     return jpaTm()
-        .query(
+        .criteriaQuery(
             CriteriaQueryBuilder.create(historyClass)
                 .where("modificationTime", criteriaBuilder::greaterThanOrEqualTo, afterTime)
                 .where("modificationTime", criteriaBuilder::lessThanOrEqualTo, beforeTime)

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

@@ -36,7 +36,7 @@ public interface JpaTransactionManager extends TransactionManager {
   <T> TypedQuery<T> query(String sqlString, Class<T> resultClass);
 
   /** Creates a JPA SQU query for the given criteria query. */
-  <T> TypedQuery<T> query(CriteriaQuery<T> criteriaQuery);
+  <T> TypedQuery<T> criteriaQuery(CriteriaQuery<T> criteriaQuery);
 
   /**
    * Creates a JPA SQL query for the given query string.

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

@@ -135,7 +135,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   }
 
   @Override
-  public <T> TypedQuery<T> query(CriteriaQuery<T> criteriaQuery) {
+  public <T> TypedQuery<T> criteriaQuery(CriteriaQuery<T> criteriaQuery) {
     return new DetachingTypedQuery<>(getEntityManager().createQuery(criteriaQuery));
   }
 

core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java

@@ -591,7 +591,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
                               cursorString.get());
                     }
                     jpaTm()
-                        .query(queryBuilder.build())
+                        .criteriaQuery(queryBuilder.build())
                         .getResultStream()
                         .filter(this::isAuthorized)
                         .forEach(

core/src/main/java/google/registry/rdap/RdapSearchActionBase.java

@@ -202,7 +202,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase {
               desiredRegistrar.get());
     }
     List<T> queryResult =
-        jpaTm().query(builder.build()).setMaxResults(querySizeLimit).getResultList();
+        jpaTm().criteriaQuery(builder.build()).setMaxResults(querySizeLimit).getResultList();
     if (checkForVisibility) {
       return filterResourcesByVisibility(queryResult, querySizeLimit);
     } else {

core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java

@@ -15,7 +15,10 @@
 package google.registry.tools.javascrap;
 
 import static google.registry.model.ofy.ObjectifyService.auditedOfy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.model.reporting.HistoryEntryDao.REPO_ID_FIELD_NAMES;
+import static google.registry.model.reporting.HistoryEntryDao.RESOURCE_TYPES_TO_HISTORY_TYPES;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 
 import com.google.appengine.tools.mapreduce.Mapper;
 import com.google.common.collect.ImmutableList;
@@ -26,12 +29,16 @@ import google.registry.mapreduce.inputs.EppResourceInputs;
 import google.registry.model.EppResource;
 import google.registry.model.domain.DomainHistory;
 import google.registry.model.reporting.HistoryEntry;
+import google.registry.persistence.transaction.CriteriaQueryBuilder;
 import google.registry.rde.RdeStagingAction;
 import google.registry.request.Action;
 import google.registry.request.Response;
 import google.registry.request.auth.Auth;
 import google.registry.tools.server.GenerateZoneFilesAction;
+import java.util.Optional;
 import javax.inject.Inject;
+import javax.persistence.criteria.CriteriaBuilder;
+import javax.persistence.criteria.CriteriaQuery;
 
 /**
  * A mapreduce that creates synthetic history objects in SQL for all {@link EppResource} objects.
@@ -61,6 +68,9 @@ import javax.inject.Inject;
     auth = Auth.AUTH_INTERNAL_OR_ADMIN)
 public class CreateSyntheticHistoryEntriesAction implements Runnable {
 
+  private static final String HISTORY_REASON =
+      "Backfill EppResource history objects during Cloud SQL migration";
+
   private final MapreduceRunner mrRunner;
   private final Response response;
   private final String registryAdminRegistrarId;
@@ -97,6 +107,50 @@ public class CreateSyntheticHistoryEntriesAction implements Runnable {
         .sendLinkToMapreduceConsole(response);
   }
 
+  // Lifted from HistoryEntryDao
+  private static Optional<? extends HistoryEntry> mostRecentHistoryFromSql(EppResource resource) {
+    return jpaTm()
+        .transact(
+            () -> {
+              // The class we're searching from is based on which parent type (e.g. Domain) we have
+              Class<? extends HistoryEntry> historyClass =
+                  getHistoryClassFromParent(resource.getClass());
+              // The field representing repo ID unfortunately varies by history class
+              String repoIdFieldName = getRepoIdFieldNameFromHistoryClass(historyClass);
+              CriteriaBuilder criteriaBuilder = jpaTm().getEntityManager().getCriteriaBuilder();
+              CriteriaQuery<? extends HistoryEntry> criteriaQuery =
+                  CriteriaQueryBuilder.create(historyClass)
+                      .where(repoIdFieldName, criteriaBuilder::equal, resource.getRepoId())
+                      .orderByDesc("modificationTime")
+                      .build();
+              return jpaTm()
+                  .criteriaQuery(criteriaQuery)
+                  .setMaxResults(1)
+                  .getResultStream()
+                  .findFirst();
+            });
+  }
+
+  // Lifted from HistoryEntryDao
+  private static Class<? extends HistoryEntry> getHistoryClassFromParent(
+      Class<? extends EppResource> parent) {
+    if (!RESOURCE_TYPES_TO_HISTORY_TYPES.containsKey(parent)) {
+      throw new IllegalArgumentException(
+          String.format("Unknown history type for parent %s", parent.getName()));
+    }
+    return RESOURCE_TYPES_TO_HISTORY_TYPES.get(parent);
+  }
+
+  // Lifted from HistoryEntryDao
+  private static String getRepoIdFieldNameFromHistoryClass(
+      Class<? extends HistoryEntry> historyClass) {
+    if (!REPO_ID_FIELD_NAMES.containsKey(historyClass)) {
+      throw new IllegalArgumentException(
+          String.format("Unknown history type %s", historyClass.getName()));
+    }
+    return REPO_ID_FIELD_NAMES.get(historyClass);
+  }
+
   /** Mapper to re-save all EPP resources. */
   public static class CreateSyntheticHistoryEntriesMapper
       extends Mapper<Key<EppResource>, Void, Void> {
@@ -109,20 +163,27 @@ public class CreateSyntheticHistoryEntriesAction implements Runnable {
 
     @Override
     public final void map(final Key<EppResource> resourceKey) {
-      tm().transact(
-              () -> {
       EppResource eppResource = auditedOfy().load().key(resourceKey).now();
-                tm().put(
+      // Only save new history entries if the most recent history for this object in SQL does not
+      // have the resource at that point in time already
+      Optional<? extends HistoryEntry> maybeHistory = mostRecentHistoryFromSql(eppResource);
+      if (maybeHistory
+          .map(history -> !history.getResourceAtPointInTime().isPresent())
+          .orElse(true)) {
+        ofyTm()
+            .transact(
+                () ->
+                    ofyTm()
+                        .put(
                             HistoryEntry.createBuilderForResource(eppResource)
                                 .setRegistrarId(registryAdminRegistrarId)
                                 .setBySuperuser(true)
                                 .setRequestedByRegistrar(false)
-                            .setModificationTime(tm().getTransactionTime())
+                                .setModificationTime(ofyTm().getTransactionTime())
-                            .setReason(
+                                .setReason(HISTORY_REASON)
-                                "Backfill EppResource history objects during Cloud SQL migration")
                                 .setType(HistoryEntry.Type.SYNTHETIC)
-                            .build());
+                                .build()));
-              });
+      }
     }
   }
 }

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

@@ -59,7 +59,7 @@ class CriteriaQueryBuilderTest {
                 .transact(
                     () ->
                         jpaTm()
-                            .query(
+                            .criteriaQuery(
                                 CriteriaQueryBuilder.create(CriteriaQueryBuilderTestEntity.class)
                                     .build())
                             .getResultList()))
@@ -80,7 +80,7 @@ class CriteriaQueryBuilderTest {
                               jpaTm().getEntityManager().getCriteriaBuilder()::equal,
                               "zztz")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity2);
   }
@@ -96,7 +96,7 @@ class CriteriaQueryBuilderTest {
                           .where(
                               "data", jpaTm().getEntityManager().getCriteriaBuilder()::like, "a%")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity3);
   }
@@ -112,7 +112,7 @@ class CriteriaQueryBuilderTest {
                           .where(
                               "data", jpaTm().getEntityManager().getCriteriaBuilder()::like, "%a%")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity1, entity3).inOrder();
   }
@@ -132,7 +132,7 @@ class CriteriaQueryBuilderTest {
                           .where(
                               "data", jpaTm().getEntityManager().getCriteriaBuilder()::like, "%t%")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity1);
   }
@@ -147,7 +147,7 @@ class CriteriaQueryBuilderTest {
                       CriteriaQueryBuilder.create(CriteriaQueryBuilderTestEntity.class)
                           .whereFieldIsIn("data", ImmutableList.of("aaa", "bbb"))
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity3).inOrder();
   }
@@ -162,7 +162,7 @@ class CriteriaQueryBuilderTest {
                       CriteriaQueryBuilder.create(CriteriaQueryBuilderTestEntity.class)
                           .whereFieldIsNotIn("data", ImmutableList.of("aaa", "bbb"))
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity1, entity2).inOrder();
   }
@@ -177,7 +177,7 @@ class CriteriaQueryBuilderTest {
                       CriteriaQueryBuilder.create(CriteriaQueryBuilderTestEntity.class)
                           .whereFieldIsIn("data", ImmutableList.of("aaa", "bbb", "data"))
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity1, entity3).inOrder();
   }
@@ -194,7 +194,7 @@ class CriteriaQueryBuilderTest {
                           .where(
                               "data", jpaTm().getEntityManager().getCriteriaBuilder()::like, "%a%")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity3, entity1).inOrder();
   }
@@ -209,7 +209,7 @@ class CriteriaQueryBuilderTest {
                       CriteriaQueryBuilder.create(CriteriaQueryBuilderTestEntity.class)
                           .orderByDesc("data")
                           .build();
-                  return jpaTm().query(query).getResultList();
+                  return jpaTm().criteriaQuery(query).getResultList();
                 });
     assertThat(result).containsExactly(entity2, entity1, entity3).inOrder();
   }

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

@@ -623,7 +623,7 @@ class JpaTransactionManagerImplTest {
                             .getEntityManager()
                             .contains(
                                 jpaTm()
-                                    .query(
+                                    .criteriaQuery(
                                         CriteriaQueryBuilder.create(TestEntity.class)
                                             .where(
                                                 "name",

core/src/test/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesActionTest.java

@@ -15,12 +15,15 @@
 package google.registry.tools.javascrap;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.testing.DatabaseHelper.createTld;
 import static google.registry.testing.DatabaseHelper.loadByKey;
+import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistActiveDomain;
 import static google.registry.testing.DatabaseHelper.persistActiveHost;
 import static google.registry.testing.DatabaseHelper.persistDomainAsDeleted;
 import static google.registry.testing.DatabaseHelper.persistDomainWithDependentResources;
+import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
@@ -30,9 +33,11 @@ import com.googlecode.objectify.Key;
 import google.registry.model.EppResource;
 import google.registry.model.contact.ContactResource;
 import google.registry.model.domain.DomainBase;
+import google.registry.model.domain.DomainHistory;
 import google.registry.model.host.HostResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.reporting.HistoryEntryDao;
+import google.registry.model.tld.Registry;
 import google.registry.testing.FakeResponse;
 import google.registry.testing.mapreduce.MapreduceTestCase;
 import org.joda.time.DateTime;
@@ -96,6 +101,38 @@ public class CreateSyntheticHistoryEntriesActionTest
     assertThat(Iterables.getLast(historyEntries).getType()).isEqualTo(HistoryEntry.Type.SYNTHETIC);
   }
 
+  @Test
+  void testDoesntSave_ifAlreadyReplayed() throws Exception {
+    DateTime now = DateTime.parse("1999-04-03T22:00:00.0Z");
+    DomainHistory domainHistoryWithoutDomain =
+        persistResource(
+            new DomainHistory.Builder()
+                .setType(HistoryEntry.Type.DOMAIN_CREATE)
+                .setModificationTime(now)
+                .setDomain(domain)
+                .setRegistrarId(domain.getCreationRegistrarId())
+                .build());
+    DomainHistory domainHistoryWithDomain =
+        domainHistoryWithoutDomain.asBuilder().setDomain(domain).build();
+    // Simulate having replayed the domain and history to SQL
+    jpaTm()
+        .transact(
+            () ->
+                jpaTm()
+                    .putAll(
+                        Registry.get("tld"),
+                        loadRegistrar("TheRegistrar"),
+                        contact,
+                        domain,
+                        domainHistoryWithDomain));
+    runMapreduce();
+
+    // Since we already had a DomainHistory with the domain in SQL, we shouldn't create a synthetic
+    // history entry
+    assertThat(HistoryEntryDao.loadHistoryObjectsForResource(domain.createVKey()))
+        .containsExactly(domainHistoryWithoutDomain);
+  }
+
   @Test
   void testCreation_forDeletedResource() throws Exception {
     persistDomainAsDeleted(domain, domain.getCreationTime().plusMonths(6));