diff --git a/config/presubmits.py b/config/presubmits.py index 4d6275702..f401453e6 100644 --- a/config/presubmits.py +++ b/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", }, diff --git a/core/src/main/java/google/registry/model/index/ForeignKeyIndex.java b/core/src/main/java/google/registry/model/index/ForeignKeyIndex.java index 8e44460ae..19ac5b258 100644 --- a/core/src/main/java/google/registry/model/index/ForeignKeyIndex.java +++ b/core/src/main/java/google/registry/model/index/ForeignKeyIndex.java @@ -228,7 +228,7 @@ public abstract class ForeignKeyIndex extends BackupGroup tm().transact( () -> jpaTm() - .query( + .criteriaQuery( CriteriaQueryBuilder.create(clazz) .whereFieldIsIn(property, foreignKeys) .build()) diff --git a/core/src/main/java/google/registry/model/reporting/HistoryEntryDao.java b/core/src/main/java/google/registry/model/reporting/HistoryEntryDao.java index fa888f033..988717298 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntryDao.java +++ b/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> + RESOURCE_TYPES_TO_HISTORY_TYPES = + ImmutableMap.of( + ContactResource.class, + ContactHistory.class, + DomainBase.class, + DomainHistory.class, + HostResource.class, + HostHistory.class); + + public static ImmutableMap, 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 loadAllHistoryObjects( DateTime afterTime, DateTime beforeTime) { @@ -164,7 +184,7 @@ public class HistoryEntryDao { private static Stream loadHistoryObjectFromSqlByRegistrars( Class historyClass, ImmutableCollection 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 getHistoryClassFromParent( Class parent) { - if (parent.equals(ContactResource.class)) { - return ContactHistory.class; - } else if (parent.equals(DomainBase.class)) { - return DomainHistory.class; - } else if (parent.equals(HostResource.class)) { - return HostHistory.class; + if (!RESOURCE_TYPES_TO_HISTORY_TYPES.containsKey(parent)) { + throw new IllegalArgumentException( + String.format("Unknown history type for parent %s", parent.getName())); } - 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 historyClass) { - return historyClass.equals(ContactHistory.class) - ? "contactRepoId" - : historyClass.equals(DomainHistory.class) ? "domainRepoId" : "hostRepoId"; + 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); } private static List loadAllHistoryObjectsFromSql( Class 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) diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java index ee6a60169..fbd13f61d 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java @@ -36,7 +36,7 @@ public interface JpaTransactionManager extends TransactionManager { TypedQuery query(String sqlString, Class resultClass); /** Creates a JPA SQU query for the given criteria query. */ - TypedQuery query(CriteriaQuery criteriaQuery); + TypedQuery criteriaQuery(CriteriaQuery criteriaQuery); /** * Creates a JPA SQL query for the given query string. diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java index 4b73f30f0..303173641 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -135,7 +135,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } @Override - public TypedQuery query(CriteriaQuery criteriaQuery) { + public TypedQuery criteriaQuery(CriteriaQuery criteriaQuery) { return new DetachingTypedQuery<>(getEntityManager().createQuery(criteriaQuery)); } diff --git a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java index 2b92d16be..3edd90ffc 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/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( diff --git a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java index fba31d296..c2f61eae3 100644 --- a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java +++ b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java @@ -202,7 +202,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase { desiredRegistrar.get()); } List queryResult = - jpaTm().query(builder.build()).setMaxResults(querySizeLimit).getResultList(); + jpaTm().criteriaQuery(builder.build()).setMaxResults(querySizeLimit).getResultList(); if (checkForVisibility) { return filterResourcesByVisibility(queryResult, querySizeLimit); } else { diff --git a/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java b/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java index 834561423..47352137d 100644 --- a/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java +++ b/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 mostRecentHistoryFromSql(EppResource resource) { + return jpaTm() + .transact( + () -> { + // The class we're searching from is based on which parent type (e.g. Domain) we have + Class historyClass = + getHistoryClassFromParent(resource.getClass()); + // The field representing repo ID unfortunately varies by history class + String repoIdFieldName = getRepoIdFieldNameFromHistoryClass(historyClass); + CriteriaBuilder criteriaBuilder = jpaTm().getEntityManager().getCriteriaBuilder(); + CriteriaQuery 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 getHistoryClassFromParent( + Class 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 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, Void, Void> { @@ -109,20 +163,27 @@ public class CreateSyntheticHistoryEntriesAction implements Runnable { @Override public final void map(final Key resourceKey) { - tm().transact( - () -> { - EppResource eppResource = auditedOfy().load().key(resourceKey).now(); - tm().put( - HistoryEntry.createBuilderForResource(eppResource) - .setRegistrarId(registryAdminRegistrarId) - .setBySuperuser(true) - .setRequestedByRegistrar(false) - .setModificationTime(tm().getTransactionTime()) - .setReason( - "Backfill EppResource history objects during Cloud SQL migration") - .setType(HistoryEntry.Type.SYNTHETIC) - .build()); - }); + EppResource eppResource = auditedOfy().load().key(resourceKey).now(); + // 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 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(ofyTm().getTransactionTime()) + .setReason(HISTORY_REASON) + .setType(HistoryEntry.Type.SYNTHETIC) + .build())); + } } } } diff --git a/core/src/test/java/google/registry/persistence/transaction/CriteriaQueryBuilderTest.java b/core/src/test/java/google/registry/persistence/transaction/CriteriaQueryBuilderTest.java index f06d7a9cd..33e830e70 100644 --- a/core/src/test/java/google/registry/persistence/transaction/CriteriaQueryBuilderTest.java +++ b/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(); } diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java index 55288cf24..ed64065b3 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/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", diff --git a/core/src/test/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesActionTest.java b/core/src/test/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesActionTest.java index dff1f2f5b..0c6833829 100644 --- a/core/src/test/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesActionTest.java +++ b/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));