diff --git a/core/src/main/java/google/registry/export/SyncGroupMembersAction.java b/core/src/main/java/google/registry/export/SyncGroupMembersAction.java index 6d80174f1..07b7a6b3d 100644 --- a/core/src/main/java/google/registry/export/SyncGroupMembersAction.java +++ b/core/src/main/java/google/registry/export/SyncGroupMembersAction.java @@ -163,7 +163,7 @@ public final class SyncGroupMembersAction implements Runnable { registrarsToSave.add(result.getKey().asBuilder().setContactsRequireSyncing(false).build()); } } - tm().transactNew(() -> tm().updateAll(registrarsToSave.build())); + tm().transact(() -> tm().updateAll(registrarsToSave.build())); return errors; } diff --git a/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java b/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java index ae89c0fea..e56c2d829 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java @@ -63,7 +63,6 @@ import google.registry.model.domain.fee.FeeTransferCommandExtension; import google.registry.model.domain.fee.FeeTransformResponseExtension; import google.registry.model.domain.metadata.MetadataExtension; import google.registry.model.domain.superuser.DomainTransferRequestSuperuserExtension; -import google.registry.model.domain.token.AllocationToken; import google.registry.model.domain.token.AllocationTokenExtension; import google.registry.model.eppcommon.AuthInfo; import google.registry.model.eppcommon.StatusValue; @@ -170,20 +169,19 @@ public final class DomainTransferRequestFlow implements TransactionalFlow { extensionManager.validate(); DateTime now = tm().getTransactionTime(); Domain existingDomain = loadAndVerifyExistence(Domain.class, targetId, now); - Optional allocationToken = - allocationTokenFlowUtils.verifyAllocationTokenIfPresent( - existingDomain, - Registry.get(existingDomain.getTld()), - gainingClientId, - now, - eppInput.getSingleExtension(AllocationTokenExtension.class)); + allocationTokenFlowUtils.verifyAllocationTokenIfPresent( + existingDomain, + Registry.get(existingDomain.getTld()), + gainingClientId, + now, + eppInput.getSingleExtension(AllocationTokenExtension.class)); Optional superuserExtension = eppInput.getSingleExtension(DomainTransferRequestSuperuserExtension.class); Period period = superuserExtension.isPresent() ? superuserExtension.get().getRenewalPeriod() : ((Transfer) resourceCommand).getPeriod(); - verifyTransferAllowed(existingDomain, period, now, superuserExtension, allocationToken); + verifyTransferAllowed(existingDomain, period, now, superuserExtension); String tld = existingDomain.getTld(); Registry registry = Registry.get(tld); @@ -296,8 +294,7 @@ public final class DomainTransferRequestFlow implements TransactionalFlow { Domain existingDomain, Period period, DateTime now, - Optional superuserExtension, - Optional allocationToken) + Optional superuserExtension) throws EppException { verifyNoDisallowedStatuses(existingDomain, DISALLOWED_STATUSES); if (!isSuperuser) { diff --git a/core/src/main/java/google/registry/flows/poll/PollAckFlow.java b/core/src/main/java/google/registry/flows/poll/PollAckFlow.java index 6a2b15d65..b9c49f757 100644 --- a/core/src/main/java/google/registry/flows/poll/PollAckFlow.java +++ b/core/src/main/java/google/registry/flows/poll/PollAckFlow.java @@ -108,7 +108,7 @@ public final class PollAckFlow implements TransactionalFlow { // acked, then we return a special status code indicating that. Note that the query will // include the message being acked. - int messageCount = tm().doTransactionless(() -> getPollMessageCount(registrarId, now)); + int messageCount = tm().transact(() -> getPollMessageCount(registrarId, now)); if (messageCount <= 0) { return responseBuilder.setResultFromCode(SUCCESS_WITH_NO_MESSAGES).build(); } diff --git a/core/src/main/java/google/registry/model/EppResource.java b/core/src/main/java/google/registry/model/EppResource.java index 41cbe15cc..add9624ed 100644 --- a/core/src/main/java/google/registry/model/EppResource.java +++ b/core/src/main/java/google/registry/model/EppResource.java @@ -365,13 +365,13 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { @Override public EppResource load(VKey key) { - return replicaTm().doTransactionless(() -> replicaTm().loadByKey(key)); + return replicaTm().transact(() -> replicaTm().loadByKey(key)); } @Override public Map, EppResource> loadAll( Iterable> keys) { - return replicaTm().doTransactionless(() -> replicaTm().loadByKeys(keys)); + return replicaTm().transact(() -> replicaTm().loadByKeys(keys)); } }; diff --git a/core/src/main/java/google/registry/model/EppResourceUtils.java b/core/src/main/java/google/registry/model/EppResourceUtils.java index 5c6d7318a..bf27458cd 100644 --- a/core/src/main/java/google/registry/model/EppResourceUtils.java +++ b/core/src/main/java/google/registry/model/EppResourceUtils.java @@ -16,7 +16,6 @@ package google.registry.model; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.DateTimeUtils.START_OF_TIME; @@ -67,7 +66,7 @@ public final class EppResourceUtils { + "AND deletionTime > :now"; // We have to use the native SQL query here because DomainHost table doesn't have its entity - // class so we cannot reference its property like domainHost.hostRepoId in a JPQL query. + // class, so we cannot reference its property like domainHost.hostRepoId in a JPQL query. private static final String HOST_LINKED_DOMAIN_QUERY = "SELECT d.repo_id FROM \"Domain\" d " + "JOIN \"DomainHost\" dh ON dh.domain_repo_id = d.repo_id " @@ -260,7 +259,7 @@ public final class EppResourceUtils { /** * Rewinds an {@link EppResource} object to a given point in time. * - *

This method costs nothing if {@code resource} is already current. Otherwise it needs to + *

This method costs nothing if {@code resource} is already current. Otherwise, it needs to * perform a single fetch operation. * *

Warning: A resource can only be rolled backwards in time, not forwards; therefore @@ -292,7 +291,7 @@ public final class EppResourceUtils { /** * Rewinds an {@link EppResource} object to a given point in time. * - *

This method costs nothing if {@code resource} is already current. Otherwise it returns an + *

This method costs nothing if {@code resource} is already current. Otherwise, it returns an * async operation that performs a single fetch operation. * * @return an asynchronous operation returning resource at {@code timestamp} or {@code null} if @@ -346,50 +345,37 @@ public final class EppResourceUtils { "key must be either VKey or VKey, but it is %s", key); boolean isContactKey = key.getKind().equals(Contact.class); - if (tm().isOfy()) { - com.googlecode.objectify.cmd.Query query = - auditedOfy() - .load() - .type(Domain.class) - .filter(isContactKey ? "allContacts.contact" : "nsHosts", key.getOfyKey()) - .filter("deletionTime >", now); - if (limit != null) { - query.limit(limit); - } - return query.keys().list().stream().map(Domain::createVKey).collect(toImmutableSet()); - } else { - return tm().transact( - () -> { - Query query; - if (isContactKey) { - query = - jpaTm() - .query(CONTACT_LINKED_DOMAIN_QUERY, String.class) - .setParameter("fkRepoId", key) - .setParameter("now", now); - } else { - query = - jpaTm() - .getEntityManager() - .createNativeQuery(HOST_LINKED_DOMAIN_QUERY) - .setParameter("fkRepoId", key.getSqlKey()) - .setParameter("now", now.toDate()); - } - if (limit != null) { - query.setMaxResults(limit); - } - @SuppressWarnings("unchecked") - ImmutableSet> domainKeySet = - (ImmutableSet>) - query - .getResultStream() - .map( - repoId -> - Domain.createVKey(Key.create(Domain.class, (String) repoId))) - .collect(toImmutableSet()); - return domainKeySet; - }); - } + return tm().transact( + () -> { + Query query; + if (isContactKey) { + query = + jpaTm() + .query(CONTACT_LINKED_DOMAIN_QUERY, String.class) + .setParameter("fkRepoId", key) + .setParameter("now", now); + } else { + query = + jpaTm() + .getEntityManager() + .createNativeQuery(HOST_LINKED_DOMAIN_QUERY) + .setParameter("fkRepoId", key.getSqlKey()) + .setParameter("now", now.toDate()); + } + if (limit != null) { + query.setMaxResults(limit); + } + @SuppressWarnings("unchecked") + ImmutableSet> domainKeySet = + (ImmutableSet>) + query + .getResultStream() + .map( + repoId -> + Domain.createVKey(Key.create(Domain.class, (String) repoId))) + .collect(toImmutableSet()); + return domainKeySet; + }); } /** diff --git a/core/src/main/java/google/registry/model/ofy/ObjectifyService.java b/core/src/main/java/google/registry/model/ofy/ObjectifyService.java index 803e711cd..bb7a1fd89 100644 --- a/core/src/main/java/google/registry/model/ofy/ObjectifyService.java +++ b/core/src/main/java/google/registry/model/ofy/ObjectifyService.java @@ -56,11 +56,6 @@ public class ObjectifyService { /** A singleton instance of our Ofy wrapper. */ private static final Ofy OFY = new Ofy(null); - /** Returns the singleton {@link Ofy} instance. */ - public static Ofy ofy() { - return OFY; - } - /** * Returns the singleton {@link Ofy} instance, signifying that the caller has been audited for the * Registry 3.0 conversion. diff --git a/core/src/main/java/google/registry/model/server/Lock.java b/core/src/main/java/google/registry/model/server/Lock.java index 22a3c859a..a583c8128 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -210,9 +210,6 @@ public class Lock extends ImmutableObject implements Serializable { RequestStatusChecker requestStatusChecker, boolean checkThreadRunning) { String scope = tld != null ? tld : GLOBAL; - // It's important to use transactNew rather than transact, because a Lock can be used to control - // access to resources like GCS that can't be transactionally rolled back. Therefore, the lock - // must be definitively acquired before it is used, even when called inside another transaction. Supplier lockAcquirer = () -> { DateTime now = jpaTm().getTransactionTime(); diff --git a/core/src/main/java/google/registry/model/tld/Registries.java b/core/src/main/java/google/registry/model/tld/Registries.java index 34bd1b9b2..d4dfc1188 100644 --- a/core/src/main/java/google/registry/model/tld/Registries.java +++ b/core/src/main/java/google/registry/model/tld/Registries.java @@ -22,22 +22,21 @@ import static com.google.common.base.Strings.emptyToNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Maps.filterValues; import static google.registry.model.CacheUtils.memoizeWithShortExpiration; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.CollectionUtils.entriesToImmutableMap; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import com.google.common.base.Joiner; -import com.google.common.base.Supplier; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import com.google.common.collect.Streams; import com.google.common.net.InternetDomainName; -import com.googlecode.objectify.Key; import google.registry.model.tld.Registry.TldType; +import google.registry.util.DomainNameUtils; import java.util.Optional; +import java.util.function.Supplier; import java.util.stream.Stream; import javax.persistence.EntityManager; @@ -58,32 +57,17 @@ public final class Registries { private static Supplier> createFreshCache() { return memoizeWithShortExpiration( () -> - tm().doTransactionless( + tm().transact( () -> { - if (tm().isOfy()) { - ImmutableSet tlds = - auditedOfy() - .load() - .type(Registry.class) - .keys() - .list() - .stream() - .map(Key::getName) - .collect(toImmutableSet()); - return Registry.get(tlds).stream() - .map(e -> Maps.immutableEntry(e.getTldStr(), e.getTldType())) - .collect(entriesToImmutableMap()); - } else { - EntityManager entityManager = jpaTm().getEntityManager(); - Stream resultStream = - entityManager - .createQuery("SELECT tldStr, tldType FROM Tld") - .getResultStream(); - return resultStream - .map(e -> ((Object[]) e)) - .map(e -> Maps.immutableEntry((String) e[0], ((TldType) e[1]))) - .collect(entriesToImmutableMap()); - } + EntityManager entityManager = jpaTm().getEntityManager(); + Stream resultStream = + entityManager + .createQuery("SELECT tldStr, tldType FROM Tld") + .getResultStream(); + return resultStream + .map(e -> ((Object[]) e)) + .map(e -> Maps.immutableEntry((String) e[0], ((TldType) e[1]))) + .collect(entriesToImmutableMap()); })); } @@ -143,8 +127,7 @@ public final class Registries { * *

Note: This routine will only work on names under TLDs for which this registry is * authoritative. To extract TLDs from domains (not hosts) that other registries control, use - * {@link google.registry.util.DomainNameUtils#getTldFromDomainName(String) - * DomainNameUtils#getTldFromDomainName}. + * {@link DomainNameUtils#getTldFromDomainName(String) DomainNameUtils#getTldFromDomainName}. * * @param domainName domain name or host name (but not TLD) under an authoritative TLD * @return TLD or absent if {@code domainName} has no labels under an authoritative TLD diff --git a/core/src/main/java/google/registry/persistence/JpaRetries.java b/core/src/main/java/google/registry/persistence/JpaRetries.java index d3f2b0596..077beb386 100644 --- a/core/src/main/java/google/registry/persistence/JpaRetries.java +++ b/core/src/main/java/google/registry/persistence/JpaRetries.java @@ -14,12 +14,10 @@ package google.registry.persistence; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableSet; import java.sql.SQLException; import java.util.function.Predicate; import javax.persistence.OptimisticLockException; -import org.hibernate.exception.JDBCConnectionException; /** Helpers for identifying retriable database operations. */ public final class JpaRetries { @@ -35,18 +33,11 @@ public final class JpaRetries { ); private static final Predicate RETRIABLE_TXN_PREDICATE = - Predicates.or( - OptimisticLockException.class::isInstance, - e -> - e instanceof SQLException - && RETRIABLE_TXN_SQL_STATE.contains(((SQLException) e).getSQLState())); - - private static final Predicate RETRIABLE_QUERY_PREDICATE = - Predicates.or( - JDBCConnectionException.class::isInstance, - e -> - e instanceof SQLException - && RETRIABLE_TXN_SQL_STATE.contains(((SQLException) e).getSQLState())); + ((Predicate) OptimisticLockException.class::isInstance) + .or( + e -> + e instanceof SQLException + && RETRIABLE_TXN_SQL_STATE.contains(((SQLException) e).getSQLState())); public static boolean isFailedTxnRetriable(Throwable throwable) { Throwable t = throwable; @@ -58,16 +49,4 @@ public final class JpaRetries { } return false; } - - public static boolean isFailedQueryRetriable(Throwable throwable) { - // TODO(weiminyu): check for more error codes. - Throwable t = throwable; - while (t != null) { - if (RETRIABLE_QUERY_PREDICATE.test(t)) { - return true; - } - t = t.getCause(); - } - return false; - } } 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 c2ea38d95..ca2425c96 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -208,43 +208,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { }); } - // TODO(b/177674699): Remove all transactNew methods as they are same as transact after the - // database migration. - @Override - public T transactNew(Supplier work) { - return transact(work); - } - - @Override - public void transactNew(Runnable work) { - transact(work); - } - - // For now, read-only transactions and "transactNew" methods only create (or use existing) - // standard transactions. Attempting to use a read-only transaction can break larger transactions - // (if we were already in one) so we don't set read-only mode. - // - // TODO(gbrodman): If necessary, implement transactNew and readOnly transactions using Postgres - // savepoints, see https://www.postgresql.org/docs/8.1/sql-savepoint.html - @Override - public T transactNewReadOnly(Supplier work) { - return retrier.callWithRetry(() -> transact(work), JpaRetries::isFailedQueryRetriable); - } - - @Override - public void transactNewReadOnly(Runnable work) { - transactNewReadOnly( - () -> { - work.run(); - return null; - }); - } - - @Override - public T doTransactionless(Supplier work) { - return retrier.callWithRetry(() -> transact(work), JpaRetries::isFailedQueryRetriable); - } - @Override public DateTime getTransactionTime() { assertInTransaction(); @@ -493,16 +456,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { return new JpaQueryComposerImpl<>(entity); } - @Override - public void clearSessionCache() { - // This is an intended no-op method as there is no session cache in Postgresql. - } - - @Override - public boolean isOfy() { - return false; - } - @Override public void assertDelete(VKey key) { if (internalDelete(key) != 1) { @@ -516,8 +469,8 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } private static class EntityId { - private String name; - private Object value; + private final String name; + private final Object value; private EntityId(String name, Object value) { this.name = name; diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java index 2b5778887..69bb02d3a 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -52,42 +52,6 @@ public interface TransactionManager { /** Executes the work in a transaction. */ void transact(Runnable work); - /** - * Pauses the current transaction (if any), executes the work in a new transaction and returns the - * result. - * - *

Note that this function is kept for backward compatibility. We will review the use case - * later when adding the cloud sql implementation. - */ - T transactNew(Supplier work); - - /** - * Pauses the current transaction (if any) and executes the work in a new transaction. - * - *

Note that this function is kept for backward compatibility. We will review the use case - * later when adding the cloud sql implementation. - */ - void transactNew(Runnable work); - - /** - * Executes the work in a read-only transaction and returns the result. - * - *

Note that this function is kept for backward compatibility. We will review the use case - * later when adding the cloud sql implementation. - */ - R transactNewReadOnly(Supplier work); - - /** - * Executes the work in a read-only transaction. - * - *

Note that this function is kept for backward compatibility. We will review the use case - * later when adding the cloud sql implementation. - */ - void transactNewReadOnly(Runnable work); - - /** Executes the work in a transactionless context. */ - R doTransactionless(Supplier work); - /** Returns the time associated with the start of this particular transaction attempt. */ DateTime getTransactionTime(); @@ -210,10 +174,4 @@ public interface TransactionManager { /** Returns a QueryComposer which can be used to perform queries against the current database. */ QueryComposer createQueryComposer(Class entity); - - /** Clears the session cache if the underlying database is Datastore, otherwise it is a no-op. */ - void clearSessionCache(); - - /** Returns true if the transaction manager is DatastoreTransactionManager, false otherwise. */ - boolean isOfy(); } diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManagerFactory.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManagerFactory.java index de0825e32..8efbc7a70 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManagerFactory.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManagerFactory.java @@ -109,7 +109,7 @@ public final class TransactionManagerFactory { * however, this will be a reference to the read-only replica database if one is configured. */ public static TransactionManager replicaTm() { - return tm().isOfy() ? tm() : replicaJpaTm(); + return replicaJpaTm(); } /** Sets the return of {@link #jpaTm()} to the given instance of {@link JpaTransactionManager}. */ @@ -118,7 +118,7 @@ public final class TransactionManagerFactory { checkState( RegistryEnvironment.get().equals(RegistryEnvironment.UNITTEST) || RegistryToolEnvironment.get() != null, - "setJpamTm() should only be called by tools and tests."); + "setJpaTm() should only be called by tools and tests."); jpaTm = Suppliers.memoize(jpaTmSupplier::get); } diff --git a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java index 71a98dc1f..f83a6d099 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java @@ -16,9 +16,7 @@ package google.registry.rdap; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.model.EppResourceUtils.loadByForeignKeyCached; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaJpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.END_OF_TIME; @@ -30,11 +28,9 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; -import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import com.google.common.net.InetAddresses; import com.google.common.primitives.Booleans; -import com.googlecode.objectify.cmd.Query; import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.host.Host; @@ -58,7 +54,6 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.stream.Stream; -import java.util.stream.StreamSupport; import javax.inject.Inject; import javax.persistence.criteria.CriteriaBuilder; import org.hibernate.Hibernate; @@ -207,49 +202,31 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { // keys, because then we have an index on TLD if we need it. int querySizeLimit = RESULT_SET_SIZE_SCALING_FACTOR * rdapResultSetMaxSize; RdapResultSet resultSet; - if (tm().isOfy()) { - Query query = - auditedOfy() - .load() - .type(Domain.class) - .filter("domainName <", partialStringQuery.getNextInitialString()) - .filter("domainName >=", partialStringQuery.getInitialString()); - if (cursorString.isPresent()) { - query = query.filter("domainName >", cursorString.get()); - } - if (partialStringQuery.getSuffix() != null) { - query = query.filter("tld", partialStringQuery.getSuffix()); - } - query = query.limit(querySizeLimit); - // Always check for visibility, because we couldn't look at the deletionTime in the query. - resultSet = getMatchingResources(query, true, querySizeLimit); - } else { - resultSet = - replicaJpaTm() - .transact( - () -> { - CriteriaBuilder criteriaBuilder = - replicaJpaTm().getEntityManager().getCriteriaBuilder(); - CriteriaQueryBuilder queryBuilder = - CriteriaQueryBuilder.create(replicaJpaTm(), Domain.class) - .where( - "domainName", - criteriaBuilder::like, - String.format("%s%%", partialStringQuery.getInitialString())) - .orderByAsc("domainName"); - if (cursorString.isPresent()) { - queryBuilder = - queryBuilder.where( - "domainName", criteriaBuilder::greaterThan, cursorString.get()); - } - if (partialStringQuery.getSuffix() != null) { - queryBuilder = - queryBuilder.where( - "tld", criteriaBuilder::equal, partialStringQuery.getSuffix()); - } - return getMatchingResourcesSql(queryBuilder, true, querySizeLimit); - }); - } + resultSet = + replicaJpaTm() + .transact( + () -> { + CriteriaBuilder criteriaBuilder = + replicaJpaTm().getEntityManager().getCriteriaBuilder(); + CriteriaQueryBuilder queryBuilder = + CriteriaQueryBuilder.create(replicaJpaTm(), Domain.class) + .where( + "domainName", + criteriaBuilder::like, + String.format("%s%%", partialStringQuery.getInitialString())) + .orderByAsc("domainName"); + if (cursorString.isPresent()) { + queryBuilder = + queryBuilder.where( + "domainName", criteriaBuilder::greaterThan, cursorString.get()); + } + if (partialStringQuery.getSuffix() != null) { + queryBuilder = + queryBuilder.where( + "tld", criteriaBuilder::equal, partialStringQuery.getSuffix()); + } + return getMatchingResources(queryBuilder, true, querySizeLimit); + }); return makeSearchResults(resultSet); } @@ -261,30 +238,21 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { // pending deletes. int querySizeLimit = RESULT_SET_SIZE_SCALING_FACTOR * rdapResultSetMaxSize; RdapResultSet resultSet; - if (tm().isOfy()) { - Query query = auditedOfy().load().type(Domain.class).filter("tld", tld); - if (cursorString.isPresent()) { - query = query.filter("domainName >", cursorString.get()); - } - query = query.order("domainName").limit(querySizeLimit); - resultSet = getMatchingResources(query, true, querySizeLimit); - } else { - resultSet = - replicaJpaTm() - .transact( - () -> { - CriteriaQueryBuilder builder = - queryItemsSql( - Domain.class, - "tld", - tld, - Optional.of("domainName"), - cursorString, - DeletedItemHandling.INCLUDE) - .orderByAsc("domainName"); - return getMatchingResourcesSql(builder, true, querySizeLimit); - }); - } + resultSet = + replicaJpaTm() + .transact( + () -> { + CriteriaQueryBuilder builder = + queryItems( + Domain.class, + "tld", + tld, + Optional.of("domainName"), + cursorString, + DeletedItemHandling.INCLUDE) + .orderByAsc("domainName"); + return getMatchingResources(builder, true, querySizeLimit); + }); return makeSearchResults(resultSet); } @@ -337,46 +305,29 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { // incomplete result set if a search asks for something like "ns*", but we need to enforce a // limit in order to avoid arbitrarily long-running queries. Optional desiredRegistrar = getDesiredRegistrar(); - if (tm().isOfy()) { - Query query = - queryItems( - Host.class, - "hostName", - partialStringQuery, - Optional.empty(), - DeletedItemHandling.EXCLUDE, - maxNameserversInFirstStage); - if (desiredRegistrar.isPresent()) { - query = query.filter("currentSponsorClientId", desiredRegistrar.get()); - } - return StreamSupport.stream(query.keys().spliterator(), false) - .map(VKey::from) - .collect(toImmutableSet()); - } else { - return replicaJpaTm() - .transact( - () -> { - CriteriaQueryBuilder builder = - queryItemsSql( - Host.class, - "hostName", - partialStringQuery, - Optional.empty(), - DeletedItemHandling.EXCLUDE); - if (desiredRegistrar.isPresent()) { - builder = - builder.where( - "currentSponsorClientId", - replicaJpaTm().getEntityManager().getCriteriaBuilder()::equal, - desiredRegistrar.get()); - } - return getMatchingResourcesSql(builder, true, maxNameserversInFirstStage) - .resources() - .stream() - .map(Host::createVKey) - .collect(toImmutableSet()); - }); - } + return replicaJpaTm() + .transact( + () -> { + CriteriaQueryBuilder builder = + queryItems( + Host.class, + "hostName", + partialStringQuery, + Optional.empty(), + DeletedItemHandling.EXCLUDE); + if (desiredRegistrar.isPresent()) { + builder = + builder.where( + "currentSponsorClientId", + replicaJpaTm().getEntityManager().getCriteriaBuilder()::equal, + desiredRegistrar.get()); + } + return getMatchingResources(builder, true, maxNameserversInFirstStage) + .resources() + .stream() + .map(Host::createVKey) + .collect(toImmutableSet()); + }); } /** Assembles a list of {@link Host} keys by name when the pattern has no wildcard. */ @@ -471,24 +422,6 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { private DomainSearchResponse searchByNameserverIp(final InetAddress inetAddress) { Optional desiredRegistrar = getDesiredRegistrar(); ImmutableSet> hostKeys; - if (tm().isOfy()) { - Query query = - queryItems( - Host.class, - "inetAddresses", - inetAddress.getHostAddress(), - Optional.empty(), - Optional.empty(), - DeletedItemHandling.EXCLUDE, - maxNameserversInFirstStage); - if (desiredRegistrar.isPresent()) { - query = query.filter("currentSponsorClientId", desiredRegistrar.get()); - } - hostKeys = - StreamSupport.stream(query.keys().spliterator(), false) - .map(VKey::from) - .collect(toImmutableSet()); - } else { // Hibernate does not allow us to query @Converted array fields directly, either // in the CriteriaQuery or the raw text format. However, Postgres does -- so we // use native queries to find hosts where any of the inetAddresses match. @@ -520,7 +453,6 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { .map(repoId -> VKey.create(Host.class, repoId)) .collect(toImmutableSet()); }); - } return searchByNameserverRefs(hostKeys); } @@ -543,27 +475,6 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { int numHostKeysSearched = 0; for (List> chunk : Iterables.partition(hostKeys, 30)) { numHostKeysSearched += chunk.size(); - if (tm().isOfy()) { - Query query = - auditedOfy() - .load() - .type(Domain.class) - .filter( - "nsHosts in", chunk.stream().map(VKey::getOfyKey).collect(toImmutableSet())); - if (!shouldIncludeDeleted()) { - query = query.filter("deletionTime >", getRequestTime()); - // If we are not performing an inequality query, we can filter on the cursor in the query. - // Otherwise, we will need to filter the results afterward. - } else if (cursorString.isPresent()) { - query = query.filter("domainName >", cursorString.get()); - } - Stream stream = Streams.stream(query).filter(this::isAuthorized); - if (cursorString.isPresent()) { - stream = - stream.filter(domain -> (domain.getDomainName().compareTo(cursorString.get()) > 0)); - } - stream.forEach(domainSetBuilder::add); - } else { replicaJpaTm() .transact( () -> { @@ -595,7 +506,6 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { }); } }); - } } List domains = domainSetBuilder.build().asList(); metricInformationBuilder.setNumHostsRetrieved(numHostKeysSearched); diff --git a/core/src/main/java/google/registry/rdap/RdapEntitySearchAction.java b/core/src/main/java/google/registry/rdap/RdapEntitySearchAction.java index 3d8b89057..7a7bc340d 100644 --- a/core/src/main/java/google/registry/rdap/RdapEntitySearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapEntitySearchAction.java @@ -16,7 +16,6 @@ package google.registry.rdap; import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaJpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; @@ -27,7 +26,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.common.primitives.Booleans; import com.google.common.primitives.Longs; -import com.googlecode.objectify.cmd.Query; import google.registry.model.contact.Contact; import google.registry.model.registrar.Registrar; import google.registry.persistence.VKey; @@ -261,39 +259,24 @@ public class RdapEntitySearchAction extends RdapSearchActionBase { || (cursorType == CursorType.REGISTRAR)) { resultSet = RdapResultSet.create(ImmutableList.of()); } else { - if (tm().isOfy()) { - Query query = - queryItems( - Contact.class, - "searchName", - partialStringQuery, - cursorQueryString, // if we get here and there's a cursor, it must be a contact - DeletedItemHandling.EXCLUDE, - rdapResultSetMaxSize + 1); - if (!rdapAuthorization.role().equals(Role.ADMINISTRATOR)) { - query = query.filter("currentSponsorClientId in", rdapAuthorization.registrarIds()); - } - resultSet = getMatchingResources(query, false, rdapResultSetMaxSize + 1); - } else { - resultSet = - replicaJpaTm() - .transact( - () -> { - CriteriaQueryBuilder builder = - queryItemsSql( - Contact.class, - "searchName", - partialStringQuery, - cursorQueryString, - DeletedItemHandling.EXCLUDE); - if (!rdapAuthorization.role().equals(Role.ADMINISTRATOR)) { - builder = - builder.whereFieldIsIn( - "currentSponsorClientId", rdapAuthorization.registrarIds()); - } - return getMatchingResourcesSql(builder, false, rdapResultSetMaxSize + 1); - }); - } + resultSet = + replicaJpaTm() + .transact( + () -> { + CriteriaQueryBuilder builder = + queryItems( + Contact.class, + "searchName", + partialStringQuery, + cursorQueryString, + DeletedItemHandling.EXCLUDE); + if (!rdapAuthorization.role().equals(Role.ADMINISTRATOR)) { + builder = + builder.whereFieldIsIn( + "currentSponsorClientId", rdapAuthorization.registrarIds()); + } + return getMatchingResources(builder, false, rdapResultSetMaxSize + 1); + }); } } return makeSearchResults(resultSet, registrars, QueryType.FULL_NAME); @@ -386,31 +369,18 @@ public class RdapEntitySearchAction extends RdapSearchActionBase { if (subtype == Subtype.REGISTRARS) { contactResultSet = RdapResultSet.create(ImmutableList.of()); } else { - if (tm().isOfy()) { - contactResultSet = - getMatchingResources( - queryItemsByKey( - Contact.class, - partialStringQuery, - cursorQueryString, - getDeletedItemHandling(), - querySizeLimit), - shouldIncludeDeleted(), - querySizeLimit); - } else { - contactResultSet = - replicaJpaTm() - .transact( - () -> - getMatchingResourcesSql( - queryItemsByKeySql( - Contact.class, - partialStringQuery, - cursorQueryString, - getDeletedItemHandling()), - shouldIncludeDeleted(), - querySizeLimit)); - } + contactResultSet = + replicaJpaTm() + .transact( + () -> + getMatchingResources( + queryItemsByKey( + Contact.class, + partialStringQuery, + cursorQueryString, + getDeletedItemHandling()), + shouldIncludeDeleted(), + querySizeLimit)); } return makeSearchResults(contactResultSet, registrars, QueryType.HANDLE); } diff --git a/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java b/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java index 96be81d02..72aad49dd 100644 --- a/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java +++ b/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java @@ -21,7 +21,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static google.registry.model.EppResourceUtils.isLinked; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaJpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.rdap.RdapIcannStandardInformation.CONTACT_REDACTED_VALUE; import static google.registry.util.CollectionUtils.union; @@ -91,8 +90,9 @@ import org.joda.time.DateTime; * *

The JSON format specifies that entities should be supplied with links indicating how to fetch * them via RDAP, which requires the URL to the RDAP server. The linkBase parameter, passed to many - * of the methods, is used as the first part of the link URL. For instance, if linkBase is - * "http://rdap.org/dir/", the link URLs will look like "http://rdap.org/dir/domain/XXXX", etc. + * of the methods, is used as the first part of the link URL. For instance, if linkBase is , the link URLs will look like , etc. * * @see RFC 9083: JSON Responses for the Registration * Data Access Protocol (RDAP) @@ -211,15 +211,15 @@ public class RdapJsonFormatter { .put(HistoryEntry.Type.CONTACT_DELETE, EventAction.DELETION) .put(HistoryEntry.Type.CONTACT_TRANSFER_APPROVE, EventAction.TRANSFER) - /** Not in the Response Profile. */ + /* Not in the Response Profile. */ .put(HistoryEntry.Type.DOMAIN_AUTORENEW, EventAction.REREGISTRATION) - /** Not in the Response Profile. */ + /* Not in the Response Profile. */ .put(HistoryEntry.Type.DOMAIN_DELETE, EventAction.DELETION) - /** Not in the Response Profile. */ + /* Not in the Response Profile. */ .put(HistoryEntry.Type.DOMAIN_RENEW, EventAction.REREGISTRATION) - /** Not in the Response Profile. */ + /* Not in the Response Profile. */ .put(HistoryEntry.Type.DOMAIN_RESTORE, EventAction.REINSTANTIATION) - /** Section 2.3.2.3, optional. */ + /* Section 2.3.2.3, optional. */ .put(HistoryEntry.Type.DOMAIN_TRANSFER_APPROVE, EventAction.TRANSFER) .put(HistoryEntry.Type.HOST_CREATE, EventAction.REGISTRATION) .put(HistoryEntry.Type.HOST_DELETE, EventAction.DELETION) @@ -533,7 +533,7 @@ public class RdapJsonFormatter { // state/province, postal code, country // // Note that in theory we have to show the Organization and state/province and country for the - // REGISTRANT. For now we won't do that until we make sure it's really OK for GDPR + // REGISTRANT. For now, we won't do that until we make sure it's really OK for GDPR // if (!isAuthorized) { // RDAP Response Profile 2.7.4.3: if we redact values from the contact, we MUST include a @@ -749,9 +749,9 @@ public class RdapJsonFormatter { if (outputDataType != OutputDataType.SUMMARY) { ImmutableList registrarContacts = registrar.getContacts().stream() - .map(registrarContact -> makeRdapJsonForRegistrarContact(registrarContact)) - .filter(optional -> optional.isPresent()) - .map(optional -> optional.get()) + .map(RdapJsonFormatter::makeRdapJsonForRegistrarContact) + .filter(Optional::isPresent) + .map(Optional::get) .filter( contact -> outputDataType == OutputDataType.FULL @@ -886,10 +886,6 @@ public class RdapJsonFormatter { // 2.3.2.3 An event of *eventAction* type *transfer*, with the last date and time that the // domain was transferred. The event of *eventAction* type *transfer* MUST be omitted if the // domain name has not been transferred since it was created. - Iterable historyEntries; - if (tm().isOfy()) { - historyEntries = HistoryEntryDao.loadHistoryObjectsForResource(resource.createVKey()); - } else { VKey resourceVkey = resource.createVKey(); Class historyClass = HistoryEntryDao.getHistoryClassFromParent(resourceVkey.getKind()); @@ -903,10 +899,14 @@ public class RdapJsonFormatter { .replace("%entityName%", entityName) .replace("%repoIdField%", repoIdFieldName) .replace("%repoIdValue%", resourceVkey.getSqlKey().toString()); - historyEntries = - replicaJpaTm() - .transact(() -> replicaJpaTm().getEntityManager().createQuery(jpql).getResultList()); - } + Iterable historyEntries = + replicaJpaTm() + .transact( + () -> + replicaJpaTm() + .getEntityManager() + .createQuery(jpql, HistoryEntry.class) + .getResultList()); for (HistoryEntry historyEntry : historyEntries) { EventAction rdapEventAction = HISTORY_ENTRY_TYPE_TO_RDAP_EVENT_ACTION_MAP.get(historyEntry.getType()); @@ -1131,7 +1131,7 @@ public class RdapJsonFormatter { * all these objects are projected to the same "now". * *

This "now" will also be considered the time of the "last update of RDAP database" event that - * RDAP sepc requires. + * RDAP spec requires. * *

We would have set this during the constructor, but the clock is injected after construction. * So instead we set the time during the first call to this function. diff --git a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java index 92d25261c..e82e41546 100644 --- a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -16,7 +16,6 @@ package google.registry.rdap; import static google.registry.model.EppResourceUtils.loadByForeignKeyCached; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaJpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.END_OF_TIME; @@ -26,7 +25,6 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.net.InetAddresses; import com.google.common.primitives.Booleans; -import com.googlecode.objectify.cmd.Query; import google.registry.model.domain.Domain; import google.registry.model.host.Host; import google.registry.persistence.transaction.CriteriaQueryBuilder; @@ -90,7 +88,7 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { } NameserverSearchResponse results; if (nameParam.isPresent()) { - // RDAP Technical Implementation Guilde 2.2.3 - we MAY support nameserver search queries based + // RDAP Technical Implementation Guide 2.2.3 - we MAY support nameserver search queries based // on a "nameserver search pattern" as defined in RFC 9082 // // syntax: /rdap/nameservers?name=exam*.com @@ -219,33 +217,20 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { private NameserverSearchResponse searchByNameUsingPrefix(RdapSearchPattern partialStringQuery) { // Add 1 so we can detect truncation. int querySizeLimit = getStandardQuerySizeLimit(); - if (tm().isOfy()) { - Query query = - queryItems( - Host.class, - "hostName", - partialStringQuery, - cursorString, - getDeletedItemHandling(), - querySizeLimit); - return makeSearchResults( - getMatchingResources(query, shouldIncludeDeleted(), querySizeLimit), CursorType.NAME); - } else { - return replicaJpaTm() - .transact( - () -> { - CriteriaQueryBuilder queryBuilder = - queryItemsSql( - Host.class, - "hostName", - partialStringQuery, - cursorString, - getDeletedItemHandling()); - return makeSearchResults( - getMatchingResourcesSql(queryBuilder, shouldIncludeDeleted(), querySizeLimit), - CursorType.NAME); - }); - } + return replicaJpaTm() + .transact( + () -> { + CriteriaQueryBuilder queryBuilder = + queryItems( + Host.class, + "hostName", + partialStringQuery, + cursorString, + getDeletedItemHandling()); + return makeSearchResults( + getMatchingResources(queryBuilder, shouldIncludeDeleted(), querySizeLimit), + CursorType.NAME); + }); } /** Searches for nameservers by IP address, returning a JSON array of nameserver info maps. */ @@ -253,18 +238,6 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { // Add 1 so we can detect truncation. int querySizeLimit = getStandardQuerySizeLimit(); RdapResultSet rdapResultSet; - if (tm().isOfy()) { - Query query = - queryItems( - Host.class, - "inetAddresses", - inetAddress.getHostAddress(), - Optional.empty(), - cursorString, - getDeletedItemHandling(), - querySizeLimit); - rdapResultSet = getMatchingResources(query, shouldIncludeDeleted(), querySizeLimit); - } else { // Hibernate does not allow us to query @Converted array fields directly, either in the // CriteriaQuery or the raw text format. However, Postgres does -- so we use native queries to // find hosts where any of the inetAddresses match. @@ -301,7 +274,6 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { List resultList = query.getResultList(); return filterResourcesByVisibility(resultList, querySizeLimit); }); - } return makeSearchResults(rdapResultSet, CursorType.ADDRESS); } diff --git a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java index 8353f75f0..5603daa03 100644 --- a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java +++ b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java @@ -15,14 +15,11 @@ package google.registry.rdap; import static com.google.common.base.Charsets.UTF_8; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaJpaTm; import static google.registry.util.DateTimeUtils.END_OF_TIME; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; -import com.googlecode.objectify.Key; -import com.googlecode.objectify.cmd.Query; import google.registry.model.EppResource; import google.registry.model.registrar.Registrar; import google.registry.persistence.transaction.CriteriaQueryBuilder; @@ -142,10 +139,10 @@ public abstract class RdapSearchActionBase extends RdapActionBase { } /** - * Runs the given query, and checks for permissioning if necessary. + * In Cloud SQL, builds and runs the given query, and checks for permissioning if necessary. * - * @param query an already-defined query to be run; a filter on currentSponsorClientId will be - * added if appropriate + * @param builder a query builder that represents the various SELECT FROM, WHERE, ORDER BY, etc. + * clauses that make up this SQL query * @param checkForVisibility true if the results should be checked to make sure they are visible; * normally this should be equal to the shouldIncludeDeleted setting, but in cases where the * query could not check deletion status (due to Datastore limitations such as the limit of @@ -160,38 +157,6 @@ public abstract class RdapSearchActionBase extends RdapActionBase { * number we might have expected */ RdapResultSet getMatchingResources( - Query query, boolean checkForVisibility, int querySizeLimit) { - Optional desiredRegistrar = getDesiredRegistrar(); - if (desiredRegistrar.isPresent()) { - query = query.filter("currentSponsorClientId", desiredRegistrar.get()); - } - List queryResult = query.list(); - if (checkForVisibility) { - return filterResourcesByVisibility(queryResult, querySizeLimit); - } else { - return RdapResultSet.create(queryResult); - } - } - - /** - * In Cloud SQL, builds and runs the given query, and checks for permissioning if necessary. - * - * @param builder a query builder that represents the various SELECT FROM, WHERE, ORDER BY and - * (etc) clauses that make up this SQL query - * @param checkForVisibility true if the results should be checked to make sure they are visible; - * normally this should be equal to the shouldIncludeDeleted setting, but in cases where the - * query could not check deletion status (due to Datastore limitations such as the limit of - * one field queried for inequality, for instance), it may need to be set to true even when - * not including deleted records - * @param querySizeLimit the maximum number of items the query is expected to return, usually - * because the limit has been set - * @return an {@link RdapResultSet} object containing the list of resources and an incompleteness - * warning flag, which is set to MIGHT_BE_INCOMPLETE iff any resources were excluded due to - * lack of visibility, and the resulting list of resources is less than the maximum allowable, - * and the number of items returned by the query is greater than or equal to the maximum - * number we might have expected - */ - RdapResultSet getMatchingResourcesSql( CriteriaQueryBuilder builder, boolean checkForVisibility, int querySizeLimit) { replicaJpaTm().assertInTransaction(); Optional desiredRegistrar = getDesiredRegistrar(); @@ -230,10 +195,10 @@ public abstract class RdapSearchActionBase extends RdapActionBase { } // The incompleteness problem comes about because we don't know how many items to fetch. We want // to return rdapResultSetMaxSize worth of items, but some might be excluded, so we fetch more - // just in case. But how many more? That's the potential problem, addressed with the three way + // just in case. But how many more? That's the potential problem, addressed with the three-way // AND statement: // 1. If we didn't exclude any items, then we can't have the incompleteness problem. - // 2. If have a full result set batch (rdapResultSetMaxSize items), we must by definition be + // 2. If we have a full result set batch (rdapResultSetMaxSize items), we must by definition be // giving the user a complete result set. // 3. If we started with fewer than querySizeLimit items, then there weren't any more items that // we missed. Even if we return fewer than rdapResultSetMaxSize items, it isn't because we @@ -321,56 +286,6 @@ public abstract class RdapSearchActionBase extends RdapActionBase { : (rdapResultSetMaxSize + 1); } - /** - * Handles prefix searches in cases where, if we need to filter out deleted items, there are no - * pending deletes. - * - *

In such cases, it is sufficient to check whether {@code deletionTime} is equal to - * {@code END_OF_TIME}, because any other value means it has already been deleted. This allows us - * to use an equality query for the deletion time. - * - * @param clazz the type of resource to be queried - * @param filterField the database field of interest - * @param partialStringQuery the details of the search string; if there is no wildcard, an - * equality query is used; if there is a wildcard, a range query is used instead; the - * initial string should not be empty, and any search suffix will be ignored, so the caller - * must filter the results if a suffix is specified - * @param cursorString if a cursor is present, this parameter should specify the cursor string, to - * skip any results up to and including the string; empty() if there is no cursor - * @param deletedItemHandling whether to include or exclude deleted items - * @param resultSetMaxSize the maximum number of results to return - * @return the query object - */ - static Query queryItems( - Class clazz, - String filterField, - RdapSearchPattern partialStringQuery, - Optional cursorString, - DeletedItemHandling deletedItemHandling, - int resultSetMaxSize) { - if (partialStringQuery.getInitialString().length() - < RdapSearchPattern.MIN_INITIAL_STRING_LENGTH) { - throw new UnprocessableEntityException( - String.format( - "Initial search string must be at least %d characters", - RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); - } - Query query = auditedOfy().load().type(clazz); - if (!partialStringQuery.getHasWildcard()) { - query = query.filter(filterField, partialStringQuery.getInitialString()); - } else { - // Ignore the suffix; the caller will need to filter on the suffix, if any. - query = - query - .filter(filterField + " >=", partialStringQuery.getInitialString()) - .filter(filterField + " <", partialStringQuery.getNextInitialString()); - } - if (cursorString.isPresent()) { - query = query.filter(filterField + " >", cursorString.get()); - } - return setOtherQueryAttributes(query, deletedItemHandling, resultSetMaxSize); - } - /** * In Cloud SQL, handles prefix searches in cases where, if we need to filter out deleted items, * there are no pending deletes. @@ -390,7 +305,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase { * @param deletedItemHandling whether to include or exclude deleted items * @return a {@link CriteriaQueryBuilder} object representing the query so far */ - static CriteriaQueryBuilder queryItemsSql( + static CriteriaQueryBuilder queryItems( Class clazz, String filterField, RdapSearchPattern partialStringQuery, @@ -420,49 +335,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase { builder = builder.where(filterField, criteriaBuilder::greaterThan, cursorString.get()); } builder = builder.orderByAsc(filterField); - return setDeletedItemHandlingSql(builder, deletedItemHandling); - } - - /** - * Handles searches using a simple string rather than an {@link RdapSearchPattern}. - * - *

Since the filter is not an inequality, we can support also checking a cursor string against - * a different field (which involves an inequality on that field). - * - * @param clazz the type of resource to be queried - * @param filterField the database field of interest - * @param queryString the search string - * @param cursorField the field which should be compared to the cursor string, or empty() if the - * key should be compared to a key created from the cursor string - * @param cursorString if a cursor is present, this parameter should specify the cursor string, to - * skip any results up to and including the string; empty() if there is no cursor - * @param deletedItemHandling whether to include or exclude deleted items - * @param resultSetMaxSize the maximum number of results to return - * @return the query object - */ - static Query queryItems( - Class clazz, - String filterField, - String queryString, - Optional cursorField, - Optional cursorString, - DeletedItemHandling deletedItemHandling, - int resultSetMaxSize) { - if (queryString.length() < RdapSearchPattern.MIN_INITIAL_STRING_LENGTH) { - throw new UnprocessableEntityException( - String.format( - "Initial search string must be at least %d characters", - RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); - } - Query query = auditedOfy().load().type(clazz).filter(filterField, queryString); - if (cursorString.isPresent()) { - if (cursorField.isPresent()) { - query = query.filter(cursorField.get() + " >", cursorString.get()); - } else { - query = query.filterKey(">", Key.create(clazz, cursorString.get())); - } - } - return setOtherQueryAttributes(query, deletedItemHandling, resultSetMaxSize); + return setDeletedItemHandling(builder, deletedItemHandling); } /** @@ -481,7 +354,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase { * @param deletedItemHandling whether to include or exclude deleted items * @return a {@link CriteriaQueryBuilder} object representing the query so far */ - static CriteriaQueryBuilder queryItemsSql( + static CriteriaQueryBuilder queryItems( Class clazz, String filterField, String queryString, @@ -506,50 +379,20 @@ public abstract class RdapSearchActionBase extends RdapActionBase { builder = builder.where("repoId", criteriaBuilder::greaterThan, cursorString.get()); } } - return setDeletedItemHandlingSql(builder, deletedItemHandling); - } - - /** Handles searches where the field to be searched is the key. */ - static Query queryItemsByKey( - Class clazz, - RdapSearchPattern partialStringQuery, - Optional cursorString, - DeletedItemHandling deletedItemHandling, - int resultSetMaxSize) { - if (partialStringQuery.getInitialString().length() - < RdapSearchPattern.MIN_INITIAL_STRING_LENGTH) { - throw new UnprocessableEntityException( - String.format( - "Initial search string must be at least %d characters", - RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); - } - Query query = auditedOfy().load().type(clazz); - if (!partialStringQuery.getHasWildcard()) { - query = query.filterKey("=", Key.create(clazz, partialStringQuery.getInitialString())); - } else { - // Ignore the suffix; the caller will need to filter on the suffix, if any. - query = - query - .filterKey(">=", Key.create(clazz, partialStringQuery.getInitialString())) - .filterKey("<", Key.create(clazz, partialStringQuery.getNextInitialString())); - } - if (cursorString.isPresent()) { - query = query.filterKey(">", Key.create(clazz, cursorString.get())); - } - return setOtherQueryAttributes(query, deletedItemHandling, resultSetMaxSize); + return setDeletedItemHandling(builder, deletedItemHandling); } /** In Cloud SQL, handles searches where the field to be searched is the key. */ - static CriteriaQueryBuilder queryItemsByKeySql( + static CriteriaQueryBuilder queryItemsByKey( Class clazz, RdapSearchPattern partialStringQuery, Optional cursorString, DeletedItemHandling deletedItemHandling) { replicaJpaTm().assertInTransaction(); - return queryItemsSql(clazz, "repoId", partialStringQuery, cursorString, deletedItemHandling); + return queryItems(clazz, "repoId", partialStringQuery, cursorString, deletedItemHandling); } - static CriteriaQueryBuilder setDeletedItemHandlingSql( + static CriteriaQueryBuilder setDeletedItemHandling( CriteriaQueryBuilder builder, DeletedItemHandling deletedItemHandling) { if (!Objects.equals(deletedItemHandling, DeletedItemHandling.INCLUDE)) { builder = @@ -560,12 +403,4 @@ public abstract class RdapSearchActionBase extends RdapActionBase { } return builder; } - - static Query setOtherQueryAttributes( - Query query, DeletedItemHandling deletedItemHandling, int resultSetMaxSize) { - if (deletedItemHandling != DeletedItemHandling.INCLUDE) { - query = query.filter("deletionTime", END_OF_TIME); - } - return query.limit(resultSetMaxSize); - } } diff --git a/core/src/main/java/google/registry/rde/RdeUploadAction.java b/core/src/main/java/google/registry/rde/RdeUploadAction.java index 2ec0b1565..f5fb98aa4 100644 --- a/core/src/main/java/google/registry/rde/RdeUploadAction.java +++ b/core/src/main/java/google/registry/rde/RdeUploadAction.java @@ -103,7 +103,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { // // This prevents making an unnecessary time-expensive (and potentially failing) API call to the // external KMS system when the RdeUploadAction ends up not being used (if the EscrowTaskRunner - // determins this EscrowTask was already completed today). + // determines this EscrowTask was already completed today). @Inject Lazy lazyJsch; @Inject JSchSshSessionFactory jschSshSessionFactory; @@ -128,9 +128,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { runner.lockRunAndRollForward(this, Registry.get(tld), timeout, CursorType.RDE_UPLOAD, interval); HashMultimap params = HashMultimap.create(); params.put(RequestParameters.PARAM_TLD, tld); - if (prefix.isPresent()) { - params.put(RdeModule.PARAM_PREFIX, prefix.get()); - } + prefix.ifPresent(s -> params.put(RdeModule.PARAM_PREFIX, s)); cloudTasksUtils.enqueue( RDE_REPORT_QUEUE, cloudTasksUtils.createPostTask( @@ -142,7 +140,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { // If a prefix is not provided, but we are in SQL mode, try to determine the prefix. This should // only happen when the RDE upload cron job runs to catch up any un-retried (i. e. expected) // RDE failures. - if (!prefix.isPresent() && !tm().isOfy()) { + if (!prefix.isPresent()) { // The prefix is always in the format of: rde-2022-02-21t00-00-00z-2022-02-21t00-07-33z, where // the first datetime is the watermark and the second one is the time when the RDE beam job // launched. We search for the latest folder that starts with "rde-[watermark]". @@ -246,7 +244,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { * } */ @VisibleForTesting - protected void upload( + private void upload( BlobId xmlFile, long xmlLength, DateTime watermark, String name, String nameWithoutPrefix) throws Exception { logger.atInfo().log("Uploading XML file '%s' to remote path '%s'.", xmlFile, uploadUrl); diff --git a/core/src/main/java/google/registry/reporting/icann/ActivityReportingQueryBuilder.java b/core/src/main/java/google/registry/reporting/icann/ActivityReportingQueryBuilder.java index 386ef2b64..18524a19e 100644 --- a/core/src/main/java/google/registry/reporting/icann/ActivityReportingQueryBuilder.java +++ b/core/src/main/java/google/registry/reporting/icann/ActivityReportingQueryBuilder.java @@ -14,8 +14,6 @@ package google.registry.reporting.icann; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; -import static google.registry.reporting.icann.IcannReportingModule.DATASTORE_EXPORT_DATA_SET; import static google.registry.reporting.icann.IcannReportingModule.ICANN_REPORTING_DATA_SET; import static google.registry.reporting.icann.QueryBuilderUtils.getQueryFromFile; import static google.registry.reporting.icann.QueryBuilderUtils.getTableName; @@ -72,19 +70,10 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder { ImmutableMap.Builder queriesBuilder = ImmutableMap.builder(); String operationalRegistrarsQuery; - if (tm().isOfy()) { - operationalRegistrarsQuery = - SqlTemplate.create(getQueryFromFile("registrar_operating_status.sql")) - .put("PROJECT_ID", projectId) - .put("DATASTORE_EXPORT_DATA_SET", DATASTORE_EXPORT_DATA_SET) - .put("REGISTRAR_TABLE", "Registrar") - .build(); - } else { operationalRegistrarsQuery = SqlTemplate.create(getQueryFromFile("cloud_sql_registrar_operating_status.sql")) .put("PROJECT_ID", projectId) .build(); - } queriesBuilder.put( getTableName(REGISTRAR_OPERATING_STATUS, yearMonth), operationalRegistrarsQuery); @@ -125,11 +114,7 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder { queriesBuilder.put(getTableName(WHOIS_COUNTS, yearMonth), whoisQuery); SqlTemplate aggregateQuery = - SqlTemplate.create( - getQueryFromFile( - tm().isOfy() - ? "activity_report_aggregation.sql" - : "cloud_sql_activity_report_aggregation.sql")) + SqlTemplate.create(getQueryFromFile("cloud_sql_activity_report_aggregation.sql")) .put("PROJECT_ID", projectId) .put( "REGISTRAR_OPERATING_STATUS_TABLE", @@ -139,13 +124,6 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder { .put("EPP_METRICS_TABLE", getTableName(EPP_METRICS, yearMonth)) .put("WHOIS_COUNTS_TABLE", getTableName(WHOIS_COUNTS, yearMonth)); - if (tm().isOfy()) { - aggregateQuery = - aggregateQuery - .put("REGISTRY_TABLE", "Registry") - .put("DATASTORE_EXPORT_DATA_SET", DATASTORE_EXPORT_DATA_SET); - } - queriesBuilder.put( getTableName(ACTIVITY_REPORT_AGGREGATION, yearMonth), aggregateQuery.build()); diff --git a/core/src/main/java/google/registry/reporting/icann/IcannReportingModule.java b/core/src/main/java/google/registry/reporting/icann/IcannReportingModule.java index 5c33b8a20..27a78887a 100644 --- a/core/src/main/java/google/registry/reporting/icann/IcannReportingModule.java +++ b/core/src/main/java/google/registry/reporting/icann/IcannReportingModule.java @@ -23,7 +23,6 @@ import com.google.common.util.concurrent.MoreExecutors; import dagger.Module; import dagger.Provides; import google.registry.bigquery.BigqueryConnection; -import google.registry.persistence.transaction.TransactionManager; import google.registry.request.HttpException.BadRequestException; import google.registry.request.Parameter; import java.util.Optional; @@ -44,7 +43,6 @@ public final class IcannReportingModule { static final String PARAM_SUBDIR = "subdir"; static final String PARAM_REPORT_TYPES = "reportTypes"; static final String ICANN_REPORTING_DATA_SET = "icannReportingDataSet"; - static final String DATASTORE_EXPORT_DATA_SET = "latest_datastore_export"; static final String MANIFEST_FILE_NAME = "MANIFEST.txt"; /** Provides an optional subdirectory to store/upload reports to, extracted from the request. */ @@ -104,7 +102,7 @@ public final class IcannReportingModule { @Provides @Named(ICANN_REPORTING_DATA_SET) - static String provideIcannReportingDataSet(TransactionManager tm) { - return tm.isOfy() ? "icann_reporting" : "cloud_sql_icann_reporting"; + static String provideIcannReportingDataSet() { + return "cloud_sql_icann_reporting"; } } diff --git a/core/src/main/java/google/registry/reporting/icann/sql/registrar_operating_status.sql b/core/src/main/java/google/registry/reporting/icann/sql/registrar_operating_status.sql deleted file mode 100644 index c29da8102..000000000 --- a/core/src/main/java/google/registry/reporting/icann/sql/registrar_operating_status.sql +++ /dev/null @@ -1,27 +0,0 @@ -#standardSQL - -- Copyright 2017 The Nomulus Authors. All Rights Reserved. - -- - -- Licensed under the Apache License, Version 2.0 (the "License"); - -- you may not use this file except in compliance with the License. - -- You may obtain a copy of the License at - -- - -- http://www.apache.org/licenses/LICENSE-2.0 - -- - -- Unless required by applicable law or agreed to in writing, software - -- distributed under the License is distributed on an "AS IS" BASIS, - -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - -- See the License for the specific language governing permissions and - -- limitations under the License. - - -- Query that counts the number of real registrars in system. - -SELECT - -- Applies to all TLDs, hence the 'null' magic value. - STRING(NULL) AS tld, - 'operational-registrars' AS metricName, - COUNT(registrarName) AS count -FROM - `%PROJECT_ID%.%DATASTORE_EXPORT_DATA_SET%.%REGISTRAR_TABLE%` -WHERE - (type = 'REAL' OR type = 'INTERNAL') -GROUP BY metricName diff --git a/core/src/main/java/google/registry/tools/AckPollMessagesCommand.java b/core/src/main/java/google/registry/tools/AckPollMessagesCommand.java index 55ff08414..07e0ee4d8 100644 --- a/core/src/main/java/google/registry/tools/AckPollMessagesCommand.java +++ b/core/src/main/java/google/registry/tools/AckPollMessagesCommand.java @@ -16,25 +16,19 @@ package google.registry.tools; import static com.google.common.base.Strings.isNullOrEmpty; import static google.registry.flows.poll.PollFlowUtils.createPollMessageQuery; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.model.poll.PollMessageExternalKeyConverter.makePollMessageExternalId; import static google.registry.persistence.transaction.QueryComposer.Comparator.LIKE; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.common.base.Joiner; -import com.google.common.collect.Iterables; -import com.googlecode.objectify.Key; -import com.googlecode.objectify.cmd.QueryKeys; import google.registry.flows.poll.PollFlowUtils; import google.registry.model.poll.PollMessage; import google.registry.model.poll.PollMessage.Autorenew; import google.registry.model.poll.PollMessage.OneTime; import google.registry.persistence.transaction.QueryComposer; import google.registry.util.Clock; -import java.util.List; import javax.inject.Inject; /** @@ -80,40 +74,9 @@ final class AckPollMessagesCommand implements CommandWithRemoteApi { @Inject Clock clock; - private static final int BATCH_SIZE = 20; - @Override public void run() { - if (tm().isOfy()) { - ackPollMessagesDatastore(); - } else { ackPollMessagesSql(); - } - } - - /** - * Loads and acks the matching poll messages from Datastore. - * - *

We have to first load the poll message keys then batch-load the objects themselves due to - * the Datastore size limits. - */ - private void ackPollMessagesDatastore() { - QueryKeys query = - auditedOfy() - .load() - .type(PollMessage.class) - .filter("clientId", clientId) - .filter("eventTime <=", clock.nowUtc()) - .order("eventTime") - .keys(); - for (List> keys : Iterables.partition(query, BATCH_SIZE)) { - tm().transact( - () -> - // Load poll messages and filter to just those of interest. - auditedOfy().load().keys(keys).values().stream() - .filter(pm -> isNullOrEmpty(message) || pm.getMsg().contains(message)) - .forEach(this::actOnPollMessage)); - } } /** Loads and acks all matching poll messages from SQL in one transaction. */ diff --git a/core/src/main/java/google/registry/tools/BigqueryParameters.java b/core/src/main/java/google/registry/tools/BigqueryParameters.java index d884108ab..1b4995a2e 100644 --- a/core/src/main/java/google/registry/tools/BigqueryParameters.java +++ b/core/src/main/java/google/registry/tools/BigqueryParameters.java @@ -22,7 +22,7 @@ import org.joda.time.Duration; /** Parameter delegate class to handle flag settings for a command's BigqueryConnection object. */ @Parameters(separators = " =") -final class BigqueryParameters { +public final class BigqueryParameters { /** * Default to 20 threads to stay within Bigquery's rate limit of 20 concurrent queries. diff --git a/core/src/main/java/google/registry/tools/DeleteTldCommand.java b/core/src/main/java/google/registry/tools/DeleteTldCommand.java index 8d979fd32..1825f5b43 100644 --- a/core/src/main/java/google/registry/tools/DeleteTldCommand.java +++ b/core/src/main/java/google/registry/tools/DeleteTldCommand.java @@ -71,7 +71,7 @@ final class DeleteTldCommand extends ConfirmingCommand implements CommandWithRem @Override protected String execute() { - tm().transactNew(() -> tm().delete(registry)); + tm().transact(() -> tm().delete(registry)); registry.invalidateInCache(); return String.format("Deleted TLD '%s'.\n", tld); } diff --git a/core/src/main/java/google/registry/tools/RegistryCli.java b/core/src/main/java/google/registry/tools/RegistryCli.java index 127a77a84..e803d08b0 100644 --- a/core/src/main/java/google/registry/tools/RegistryCli.java +++ b/core/src/main/java/google/registry/tools/RegistryCli.java @@ -263,7 +263,7 @@ final class RegistryCli implements AutoCloseable, CommandRunner { ObjectifyService.initOfy(); // Make sure we start the command with a clean cache, so that any previous command won't // interfere with this one. - ObjectifyService.ofy().clearSessionCache(); + ObjectifyService.auditedOfy().clearSessionCache(); // Enable Cloud SQL for command that needs remote API as they will very likely use // Cloud SQL after the database migration. Note that the DB password is stored in Datastore diff --git a/core/src/main/java/google/registry/tools/server/ListDomainsAction.java b/core/src/main/java/google/registry/tools/server/ListDomainsAction.java index 92807b143..f0d1da1c5 100644 --- a/core/src/main/java/google/registry/tools/server/ListDomainsAction.java +++ b/core/src/main/java/google/registry/tools/server/ListDomainsAction.java @@ -16,20 +16,15 @@ package google.registry.tools.server; 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.model.tld.Registries.assertTldsExist; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.POST; import static google.registry.request.RequestParameters.PARAM_TLDS; -import static java.util.Comparator.comparing; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Lists; -import google.registry.model.EppResource; import google.registry.model.EppResourceUtils; import google.registry.model.domain.Domain; import google.registry.request.Action; @@ -37,9 +32,7 @@ import google.registry.request.Parameter; import google.registry.request.auth.Auth; import google.registry.util.Clock; import google.registry.util.NonFinalForTesting; -import java.util.List; import javax.inject.Inject; -import org.joda.time.DateTime; /** An action that lists domains, for use by the {@code nomulus list_domains} command. */ @Action( @@ -76,38 +69,11 @@ public final class ListDomainsAction extends ListObjectsAction { public ImmutableSet loadObjects() { checkArgument(!tlds.isEmpty(), "Must specify TLDs to query"); assertTldsExist(tlds); - ImmutableList domains = tm().isOfy() ? loadDomainsOfy() : loadDomainsSql(); + ImmutableList domains = loadDomains(); return ImmutableSet.copyOf(domains.reverse()); } - private ImmutableList loadDomainsOfy() { - DateTime now = clock.nowUtc(); - ImmutableList.Builder domainsBuilder = new ImmutableList.Builder<>(); - // Combine the batches together by sorting all domains together with newest first, applying the - // limit, and then reversing for display order. - for (List tldsBatch : Lists.partition(tlds.asList(), maxNumSubqueries)) { - auditedOfy() - .load() - .type(Domain.class) - .filter("tld in", tldsBatch) - // Get the N most recently created domains (requires ordering in descending order). - .order("-creationTime") - .limit(limit) - .list() - .stream() - .map(EppResourceUtils.transformAtTime(now)) - // Deleted entities must be filtered out post-query because queries don't allow - // ordering with two filters. - .filter(d -> d.getDeletionTime().isAfter(now)) - .forEach(domainsBuilder::add); - } - return domainsBuilder.build().stream() - .sorted(comparing(EppResource::getCreationTime).reversed()) - .limit(limit) - .collect(toImmutableList()); - } - - private ImmutableList loadDomainsSql() { + private ImmutableList loadDomains() { return jpaTm() .transact( () -> diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index 3fa89a396..ada89e8db 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -104,7 +104,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA * by default. Enqueuing is allowed only if the value of isInTestDriver is false. It's set to true * in start() and set to false in stop() inside TestDriver.java, a class used in testing. */ - private static ThreadLocal isInTestDriver = ThreadLocal.withInitial(() -> false); + private static final ThreadLocal isInTestDriver = ThreadLocal.withInitial(() -> false); @Inject JsonActionRunner jsonActionRunner; @Inject RegistrarConsoleMetrics registrarConsoleMetrics; @@ -231,21 +231,19 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA private RegistrarResult update(final Map args, String registrarId) { // Email the updates sendExternalUpdatesIfNecessary(tm().transact(() -> saveUpdates(args, registrarId))); - // Reload the result outside of the transaction to get the most recent version + // Reload the result outside the transaction to get the most recent version return RegistrarResult.create("Saved " + registrarId, loadRegistrarUnchecked(registrarId)); } /** Saves the updates and returns info needed for the update email */ private EmailInfo saveUpdates(final Map args, String registrarId) { - // We load the registrar here rather than outside of the transaction - to make + // We load the registrar here rather than outside the transaction - to make // sure we have the latest version. This one is loaded inside the transaction, so it's // guaranteed to not change before we update it. Registrar registrar = loadRegistrarUnchecked(registrarId); // Detach the registrar to avoid Hibernate object-updates, since we wish to email // out the diffs between the existing and updated registrar objects - if (!tm().isOfy()) { - jpaTm().getEntityManager().detach(registrar); - } + jpaTm().getEntityManager().detach(registrar); // Verify that the registrar hasn't been changed. // To do that - we find the latest update time (or null if the registrar has been // deleted) and compare to the update time from the args. The update time in the args @@ -262,7 +260,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA "Registrar has been changed by someone else. Please reload and retry."); } - // Keep the current contacts so we can later check that no required contact was + // Keep the current contacts, so we can later check that no required contact was // removed, email the changes to the contacts ImmutableSet contacts = registrar.getContacts(); @@ -297,7 +295,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA ImmutableSet> expandedContacts = Streams.stream(contacts) .map(RegistrarPoc::toDiffableFieldMap) - // Note: per the javadoc, toDiffableFieldMap includes sensitive data but we don't want + // Note: per the javadoc, toDiffableFieldMap includes sensitive data, but we don't want // to display it here .peek( map -> { @@ -416,7 +414,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA throw new ForbiddenException("Can't remove allowed TLDs using the console."); } if (!Sets.difference(updatedAllowedTlds, initialRegistrar.getAllowedTlds()).isEmpty()) { - // If a REAL registrar isn't in compliance with regards to having an abuse contact set, + // If a REAL registrar isn't in compliance with regard to having an abuse contact set, // prevent addition of allowed TLDs until that's fixed. if (Registrar.Type.REAL.equals(initialRegistrar.getType()) && PRODUCTION.equals(RegistryEnvironment.get())) { @@ -430,7 +428,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA } /** - * Makes sure builder.build is different than originalRegistrar only if we have the correct role. + * Makes sure {@code builder.build}is different from {@code originalRegistrar} only if we have the + * correct role. * *

On success, returns {@code builder.build()}. */ @@ -610,7 +609,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA * query as abuse contact (if any). * *

Frontend processing ensures that only one contact can be set as abuse contact in domain - * WHOIS record. Therefore it is possible to return inside the loop once one such contact is + * WHOIS record. Therefore, it is possible to return inside the loop once one such contact is * found. */ private static Optional getDomainWhoisVisibleAbuseContact( diff --git a/core/src/main/java/google/registry/whois/NameserverLookupByIpCommand.java b/core/src/main/java/google/registry/whois/NameserverLookupByIpCommand.java index b5568c155..75b9238a5 100644 --- a/core/src/main/java/google/registry/whois/NameserverLookupByIpCommand.java +++ b/core/src/main/java/google/registry/whois/NameserverLookupByIpCommand.java @@ -16,9 +16,7 @@ package google.registry.whois; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableList.toImmutableList; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; import com.google.common.annotations.VisibleForTesting; @@ -52,35 +50,26 @@ final class NameserverLookupByIpCommand implements WhoisCommand { @SuppressWarnings("unchecked") public WhoisResponse executeQuery(DateTime now) throws WhoisException { Iterable hostsFromDb; - if (tm().isOfy()) { - hostsFromDb = - auditedOfy() - .load() - .type(Host.class) - .filter("inetAddresses", ipAddress) - .filter("deletionTime >", now.toDate()); - } else { - hostsFromDb = - jpaTm() - .transact( - () -> - // We cannot query @Convert-ed fields in HQL so we must use native Postgres. - jpaTm() - .getEntityManager() - /** - * Using array_operator <@ (contained-by) with gin index on inet_address. - * Without gin index, this is slightly slower than the alternative form of - * ':address = ANY(inet_address)'. - */ - .createNativeQuery( - "SELECT * From \"Host\" WHERE " - + "ARRAY[ CAST(:address AS TEXT) ] <@ inet_addresses AND " - + "deletion_time > CAST(:now AS timestamptz)", - Host.class) - .setParameter("address", InetAddresses.toAddrString(ipAddress)) - .setParameter("now", now.toString()) - .getResultList()); - } + hostsFromDb = + jpaTm() + .transact( + () -> + // We cannot query @Convert-ed fields in HQL, so we must use native Postgres. + jpaTm() + .getEntityManager() + /* + * Using array_operator <@ (contained-by) with gin index on inet_address. + * Without gin index, this is slightly slower than the alternative form of + * ':address = ANY(inet_address)'. + */ + .createNativeQuery( + "SELECT * From \"Host\" WHERE " + + "ARRAY[ CAST(:address AS TEXT) ] <@ inet_addresses AND " + + "deletion_time > CAST(:now AS timestamptz)", + Host.class) + .setParameter("address", InetAddresses.toAddrString(ipAddress)) + .setParameter("now", now.toString()) + .getResultList()); ImmutableList hosts = Streams.stream(hostsFromDb) .filter( diff --git a/core/src/test/java/google/registry/batch/CheckPackagesComplianceActionTest.java b/core/src/test/java/google/registry/batch/CheckPackagesComplianceActionTest.java index b76a186f8..e7e4b3252 100644 --- a/core/src/test/java/google/registry/batch/CheckPackagesComplianceActionTest.java +++ b/core/src/test/java/google/registry/batch/CheckPackagesComplianceActionTest.java @@ -23,7 +23,6 @@ import static google.registry.testing.LogsSubject.assertAboutLogs; import com.google.common.testing.TestLogHandler; import google.registry.model.billing.BillingEvent.RenewalPriceBehavior; import google.registry.model.contact.Contact; -import google.registry.model.domain.Domain; import google.registry.model.domain.token.AllocationToken; import google.registry.model.domain.token.AllocationToken.TokenType; import google.registry.model.domain.token.PackagePromotion; @@ -96,12 +95,11 @@ public class CheckPackagesComplianceActionTest { @Test void testSuccess_noPackageOverCreateLimit() { - Domain domain1 = - persistEppResource( - DatabaseHelper.newDomain("foo.tld", contact) - .asBuilder() - .setCurrentPackageToken(token.createVKey()) - .build()); + persistEppResource( + DatabaseHelper.newDomain("foo.tld", contact) + .asBuilder() + .setCurrentPackageToken(token.createVKey()) + .build()); action.run(); assertAboutLogs() diff --git a/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java b/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java index e4d9f3dd2..2c4a95067 100644 --- a/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java +++ b/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java @@ -74,12 +74,8 @@ public class SyncRegistrarsSheetTest { void beforeEach() { createTld("example"); // Remove Registrar entities created by AppEngineExtension (and RegistrarContact's, for jpa). - // We don't do this for ofy because ofy's loadAllOf() can't be called in a transaction but - // _must_ be called in a transaction in JPA. - if (!tm().isOfy()) { tm().transact(() -> tm().loadAllOf(RegistrarPoc.class)) .forEach(DatabaseHelper::deleteResource); - } Registrar.loadAll().forEach(DatabaseHelper::deleteResource); } diff --git a/core/src/test/java/google/registry/flows/EppPointInTimeTest.java b/core/src/test/java/google/registry/flows/EppPointInTimeTest.java index c331a739a..558d518f7 100644 --- a/core/src/test/java/google/registry/flows/EppPointInTimeTest.java +++ b/core/src/test/java/google/registry/flows/EppPointInTimeTest.java @@ -17,7 +17,6 @@ package google.registry.flows; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadAtPointInTime; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadAllOf; import static google.registry.testing.DatabaseHelper.loadByEntity; @@ -94,7 +93,6 @@ class EppPointInTimeTest { clock.setTo(timeAtCreate); eppLoader = new EppLoader(this, "domain_create.xml", ImmutableMap.of("DOMAIN", "example.tld")); runFlow(); - tm().clearSessionCache(); Domain domainAfterCreate = Iterables.getOnlyElement(loadAllOf(Domain.class)); assertThat(domainAfterCreate.getDomainName()).isEqualTo("example.tld"); @@ -102,7 +100,6 @@ class EppPointInTimeTest { DateTime timeAtFirstUpdate = clock.nowUtc(); eppLoader = new EppLoader(this, "domain_update_dsdata_add.xml"); runFlow(); - tm().clearSessionCache(); Domain domainAfterFirstUpdate = loadByEntity(domainAfterCreate); assertThat(domainAfterCreate).isNotEqualTo(domainAfterFirstUpdate); @@ -111,14 +108,12 @@ class EppPointInTimeTest { DateTime timeAtSecondUpdate = clock.nowUtc(); eppLoader = new EppLoader(this, "domain_update_dsdata_rem.xml"); runFlow(); - tm().clearSessionCache(); Domain domainAfterSecondUpdate = loadByEntity(domainAfterCreate); clock.advanceBy(standardDays(2)); DateTime timeAtDelete = clock.nowUtc(); // before 'add' grace period ends eppLoader = new EppLoader(this, "domain_delete.xml", ImmutableMap.of("DOMAIN", "example.tld")); runFlow(); - tm().clearSessionCache(); assertThat(domainAfterFirstUpdate).isNotEqualTo(domainAfterSecondUpdate); @@ -126,17 +121,14 @@ class EppPointInTimeTest { Domain latest = loadByEntity(domainAfterCreate); // Creation time has millisecond granularity due to isActive() check. - tm().clearSessionCache(); assertThat(loadAtPointInTime(latest, timeAtCreate.minusMillis(1))).isNull(); assertThat(loadAtPointInTime(latest, timeAtCreate)).isNotNull(); assertThat(loadAtPointInTime(latest, timeAtCreate.plusMillis(1))).isNotNull(); - tm().clearSessionCache(); assertAboutImmutableObjects() .that(loadAtPointInTime(latest, timeAtCreate.plusDays(1))) .isEqualExceptFields(domainAfterCreate, "updateTimestamp"); - tm().clearSessionCache(); // In SQL, we are not limited by the day granularity, so when we request the object // at timeAtFirstUpdate we should receive the object at that first update, even though the // second update occurred one millisecond later. @@ -144,18 +136,15 @@ class EppPointInTimeTest { .that(loadAtPointInTime(latest, timeAtFirstUpdate)) .isEqualExceptFields(domainAfterFirstUpdate, "updateTimestamp"); - tm().clearSessionCache(); assertAboutImmutableObjects() .that(loadAtPointInTime(latest, timeAtSecondUpdate)) .isEqualExceptFields(domainAfterSecondUpdate, "updateTimestamp"); - tm().clearSessionCache(); assertAboutImmutableObjects() .that(loadAtPointInTime(latest, timeAtSecondUpdate.plusDays(1))) .isEqualExceptFields(domainAfterSecondUpdate, "updateTimestamp"); // Deletion time has millisecond granularity due to isActive() check. - tm().clearSessionCache(); assertThat(loadAtPointInTime(latest, timeAtDelete.minusMillis(1))).isNotNull(); assertThat(loadAtPointInTime(latest, timeAtDelete)).isNull(); assertThat(loadAtPointInTime(latest, timeAtDelete.plusMillis(1))).isNull(); diff --git a/core/src/test/java/google/registry/flows/EppTestCase.java b/core/src/test/java/google/registry/flows/EppTestCase.java index 47ef737fe..bbfa612e4 100644 --- a/core/src/test/java/google/registry/flows/EppTestCase.java +++ b/core/src/test/java/google/registry/flows/EppTestCase.java @@ -16,7 +16,6 @@ package google.registry.flows; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType; import static google.registry.testing.DatabaseHelper.loadAllOf; import static google.registry.testing.DatabaseHelper.stripBillingEventId; @@ -226,7 +225,6 @@ public class EppTestCase { "Running " + inputFilename + " => " + outputFilename, "epp.response.resData.infData.roid", "epp.response.trID.svTRID"); - tm().clearSessionCache(); // Clear the cache like OfyFilter would. return actualOutput; } diff --git a/core/src/test/java/google/registry/flows/FlowTestCase.java b/core/src/test/java/google/registry/flows/FlowTestCase.java index 4ffd90e70..3f51b34f9 100644 --- a/core/src/test/java/google/registry/flows/FlowTestCase.java +++ b/core/src/test/java/google/registry/flows/FlowTestCase.java @@ -277,8 +277,6 @@ public abstract class FlowTestCase { Arrays.toString(marshal(output, ValidationMode.LENIENT))), e); } - // Clear the cache so that we don't see stale results in tests. - tm().clearSessionCache(); return output; } diff --git a/core/src/test/java/google/registry/flows/ResourceFlowTestCase.java b/core/src/test/java/google/registry/flows/ResourceFlowTestCase.java index cd86a1087..1690a0638 100644 --- a/core/src/test/java/google/registry/flows/ResourceFlowTestCase.java +++ b/core/src/test/java/google/registry/flows/ResourceFlowTestCase.java @@ -76,9 +76,6 @@ public abstract class ResourceFlowTestCase T reloadResourceAndCloneAtTime(T resource, DateTime now) { - // Force the session to be cleared. - tm().clearSessionCache(); @SuppressWarnings("unchecked") T refreshedResource = (T) tm().transact(() -> tm().loadByEntity(resource)).cloneProjectedAtTime(now); diff --git a/core/src/test/java/google/registry/model/CreateAutoTimestampTest.java b/core/src/test/java/google/registry/model/CreateAutoTimestampTest.java index cef26dd81..991b54358 100644 --- a/core/src/test/java/google/registry/model/CreateAutoTimestampTest.java +++ b/core/src/test/java/google/registry/model/CreateAutoTimestampTest.java @@ -58,7 +58,6 @@ public class CreateAutoTimestampTest { tm().put(object); return tm().getTransactionTime(); }); - tm().clearSessionCache(); assertThat(reload().createTime.getTimestamp()).isEqualTo(transactionTime); } @@ -71,7 +70,6 @@ public class CreateAutoTimestampTest { object.createTime = CreateAutoTimestamp.create(oldCreateTime); tm().put(object); }); - tm().clearSessionCache(); assertThat(reload().createTime.getTimestamp()).isEqualTo(oldCreateTime); } } diff --git a/core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java b/core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java index 7ad6e4499..c34e63bc2 100644 --- a/core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java +++ b/core/src/test/java/google/registry/model/UpdateAutoTimestampTest.java @@ -65,7 +65,6 @@ public class UpdateAutoTimestampTest { tm().insert(object); return tm().getTransactionTime(); }); - tm().clearSessionCache(); assertThat(reload().updateTime.getTimestamp()).isEqualTo(transactionTime); } @@ -106,7 +105,6 @@ public class UpdateAutoTimestampTest { tm().insert(object); return tm().getTransactionTime(); }); - tm().clearSessionCache(); assertThat(reload().updateTime.getTimestamp()).isEqualTo(transactionTime); } 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 4dc11a2d1..e03748e98 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java @@ -38,7 +38,6 @@ import google.registry.testing.DatabaseHelper; import google.registry.testing.FakeClock; import java.io.Serializable; import java.math.BigInteger; -import java.sql.SQLException; import java.util.NoSuchElementException; import java.util.function.Supplier; import javax.persistence.Entity; @@ -47,7 +46,6 @@ import javax.persistence.Id; import javax.persistence.IdClass; import javax.persistence.OptimisticLockException; import javax.persistence.RollbackException; -import org.hibernate.exception.JDBCConnectionException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -205,59 +203,6 @@ class JpaTransactionManagerImplTest { verify(spyJpaTm, times(6)).delete(theEntityKey); } - @Test - void transactNewReadOnly_retriesJdbcConnectionExceptions() { - JpaTransactionManager spyJpaTm = spy(jpaTm()); - doThrow(JDBCConnectionException.class).when(spyJpaTm).loadByKey(any(VKey.class)); - spyJpaTm.transact(() -> spyJpaTm.insert(theEntity)); - assertThrows( - JDBCConnectionException.class, - () -> spyJpaTm.transactNewReadOnly(() -> spyJpaTm.loadByKey(theEntityKey))); - verify(spyJpaTm, times(3)).loadByKey(theEntityKey); - Supplier supplier = - () -> { - Runnable work = () -> spyJpaTm.loadByKey(theEntityKey); - work.run(); - return null; - }; - assertThrows(JDBCConnectionException.class, () -> spyJpaTm.transactNewReadOnly(supplier)); - verify(spyJpaTm, times(6)).loadByKey(theEntityKey); - } - - @Test - void transactNewReadOnly_retriesNestedJdbcConnectionExceptions() { - JpaTransactionManager spyJpaTm = spy(jpaTm()); - doThrow( - new RuntimeException( - new JDBCConnectionException("connection exception", new SQLException()))) - .when(spyJpaTm) - .loadByKey(any(VKey.class)); - spyJpaTm.transact(() -> spyJpaTm.insert(theEntity)); - assertThrows( - RuntimeException.class, - () -> spyJpaTm.transactNewReadOnly(() -> spyJpaTm.loadByKey(theEntityKey))); - verify(spyJpaTm, times(3)).loadByKey(theEntityKey); - Supplier supplier = - () -> { - Runnable work = () -> spyJpaTm.loadByKey(theEntityKey); - work.run(); - return null; - }; - assertThrows(RuntimeException.class, () -> spyJpaTm.transactNewReadOnly(supplier)); - verify(spyJpaTm, times(6)).loadByKey(theEntityKey); - } - - @Test - void doTransactionless_retriesJdbcConnectionExceptions() { - JpaTransactionManager spyJpaTm = spy(jpaTm()); - doThrow(JDBCConnectionException.class).when(spyJpaTm).loadByKey(any(VKey.class)); - spyJpaTm.transact(() -> spyJpaTm.insert(theEntity)); - assertThrows( - RuntimeException.class, - () -> spyJpaTm.doTransactionless(() -> spyJpaTm.loadByKey(theEntityKey))); - verify(spyJpaTm, times(3)).loadByKey(theEntityKey); - } - @Test void insert_throwsExceptionIfEntityExists() { assertThat(existsInDb(theEntity)).isFalse(); @@ -787,6 +732,7 @@ class JpaTransactionManagerImplTest { String name; int age; + @SuppressWarnings("unused") private CompoundId() {} private CompoundId(String name, int age) { @@ -834,6 +780,7 @@ class JpaTransactionManagerImplTest { String nameField; int ageField; + @SuppressWarnings("unused") private NamedCompoundId() {} private NamedCompoundId(String nameField, int ageField) { diff --git a/core/src/test/java/google/registry/persistence/transaction/QueryComposerTest.java b/core/src/test/java/google/registry/persistence/transaction/QueryComposerTest.java index 4dc060f3e..808995334 100644 --- a/core/src/test/java/google/registry/persistence/transaction/QueryComposerTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/QueryComposerTest.java @@ -72,7 +72,7 @@ public class QueryComposerTest { tm().createQueryComposer(TestEntity.class) .where("name", Comparator.GT, "bravo") .first() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .get())) .isEqualTo(charlie); assertThat( @@ -81,7 +81,7 @@ public class QueryComposerTest { tm().createQueryComposer(TestEntity.class) .where("name", Comparator.GTE, "charlie") .first() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .get())) .isEqualTo(charlie); assertThat( @@ -90,7 +90,7 @@ public class QueryComposerTest { tm().createQueryComposer(TestEntity.class) .where("name", Comparator.LT, "bravo") .first() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .get())) .isEqualTo(alpha); assertThat( @@ -99,7 +99,7 @@ public class QueryComposerTest { tm().createQueryComposer(TestEntity.class) .where("name", Comparator.LTE, "alpha") .first() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .get())) .isEqualTo(alpha); } @@ -120,7 +120,7 @@ public class QueryComposerTest { assertThat( tm().transact( () -> - QueryComposerTest.assertDetachedIfJpa( + DatabaseHelper.assertDetachedFromEntityManager( tm().createQueryComposer(TestEntity.class) .where("name", Comparator.EQ, "alpha") .getSingleResult()))) @@ -169,7 +169,7 @@ public class QueryComposerTest { .createQueryComposer(TestEntity.class) .where("name", Comparator.GT, "alpha") .stream() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .collect(toImmutableList()))) .containsExactly(bravo, charlie); assertThat( @@ -179,7 +179,7 @@ public class QueryComposerTest { .createQueryComposer(TestEntity.class) .where("name", Comparator.GTE, "bravo") .stream() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .collect(toImmutableList()))) .containsExactly(bravo, charlie); assertThat( @@ -189,7 +189,7 @@ public class QueryComposerTest { .createQueryComposer(TestEntity.class) .where("name", Comparator.LT, "charlie") .stream() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .collect(toImmutableList()))) .containsExactly(alpha, bravo); assertThat( @@ -199,7 +199,7 @@ public class QueryComposerTest { .createQueryComposer(TestEntity.class) .where("name", Comparator.LTE, "bravo") .stream() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .collect(toImmutableList()))) .containsExactly(alpha, bravo); } @@ -223,7 +223,7 @@ public class QueryComposerTest { tm().createQueryComposer(TestEntity.class) .where("val", Comparator.EQ, 2) .first() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .get())) .isEqualTo(bravo); } @@ -238,7 +238,7 @@ public class QueryComposerTest { .where("val", Comparator.GT, 1) .orderBy("val") .stream() - .map(QueryComposerTest::assertDetachedIfJpa) + .map(DatabaseHelper::assertDetachedFromEntityManager) .collect(toImmutableList()))) .containsExactly(bravo, alpha); } @@ -319,13 +319,6 @@ public class QueryComposerTest { .isEmpty(); } - private static T assertDetachedIfJpa(T entity) { - if (!tm().isOfy()) { - return DatabaseHelper.assertDetachedFromEntityManager(entity); - } - return entity; - } - @javax.persistence.Entity @Entity(name = "QueryComposerTestEntity") private static class TestEntity extends ImmutableObject { diff --git a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java index c2db90024..578781334 100644 --- a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java +++ b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java @@ -128,31 +128,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan transact(work); } - @Override - public T transactNew(Supplier work) { - return transact(work); - } - - @Override - public void transactNew(Runnable work) { - transact(work); - } - - @Override - public T transactNewReadOnly(Supplier work) { - return transact(work); - } - - @Override - public void transactNewReadOnly(Runnable work) { - transact(work); - } - - @Override - public T doTransactionless(Supplier work) { - return delegate.doTransactionless(work); - } - @Override public DateTime getTransactionTime() { return delegate.getTransactionTime(); @@ -285,16 +260,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan return delegate.createQueryComposer(entity); } - @Override - public void clearSessionCache() { - delegate.clearSessionCache(); - } - - @Override - public boolean isOfy() { - return delegate.isOfy(); - } - @Override public void assertDelete(VKey key) { delegate.assertDelete(key); diff --git a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java index 8dea3d218..c1d6e8ad3 100644 --- a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java @@ -111,22 +111,6 @@ public class TransactionManagerTest { assertEntityExists(theEntity); } - @Test - void transactNew_succeeds() { - assertEntityNotExist(theEntity); - tm().transactNew(() -> tm().insert(theEntity)); - assertEntityExists(theEntity); - } - - @Test - void transactNewReadOnly_succeeds() { - assertEntityNotExist(theEntity); - tm().transact(() -> tm().insert(theEntity)); - assertEntityExists(theEntity); - TestEntity persisted = tm().transactNewReadOnly(() -> tm().loadByKey(theEntity.key())); - assertThat(persisted).isEqualTo(theEntity); - } - @Test void saveNew_succeeds() { assertEntityNotExist(theEntity); diff --git a/core/src/test/java/google/registry/rde/EscrowTaskRunnerTest.java b/core/src/test/java/google/registry/rde/EscrowTaskRunnerTest.java index 7385c2be0..a4f0eb0d3 100644 --- a/core/src/test/java/google/registry/rde/EscrowTaskRunnerTest.java +++ b/core/src/test/java/google/registry/rde/EscrowTaskRunnerTest.java @@ -15,7 +15,6 @@ package google.registry.rde; import static com.google.common.truth.Truth.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadByKey; import static google.registry.testing.DatabaseHelper.persistResource; @@ -79,7 +78,6 @@ public class EscrowTaskRunnerTest { runner.lockRunAndRollForward( task, registry, standardSeconds(30), CursorType.RDE_STAGING, standardDays(1)); verify(task).runWithLock(DateTime.parse("2006-06-06TZ")); - tm().clearSessionCache(); Cursor cursor = loadByKey(Cursor.createScopedVKey(CursorType.RDE_STAGING, registry)); assertThat(cursor.getCursorTime()).isEqualTo(DateTime.parse("2006-06-07TZ")); } diff --git a/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java b/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java index c4d4168ae..9f25098cf 100644 --- a/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java +++ b/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java @@ -15,7 +15,6 @@ package google.registry.reporting.icann; import static com.google.common.truth.Truth.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTlds; import static google.registry.testing.DatabaseHelper.loadByKey; import static google.registry.testing.DatabaseHelper.persistResource; @@ -179,7 +178,6 @@ class IcannReportingUploadActionTest { when(mockReporter.send(PAYLOAD_SUCCESS, "tld-activity-200606.csv")).thenReturn(true); IcannReportingUploadAction action = createAction(); action.run(); - tm().clearSessionCache(); Cursor cursor = loadByKey(Cursor.createScopedVKey(CursorType.ICANN_UPLOAD_ACTIVITY, Registry.get("tld"))); assertThat(cursor.getCursorTime()).isEqualTo(DateTime.parse("2006-08-01TZ")); @@ -190,7 +188,6 @@ class IcannReportingUploadActionTest { clock.setTo(DateTime.parse("2006-5-01T00:30:00Z")); IcannReportingUploadAction action = createAction(); action.run(); - tm().clearSessionCache(); verifyNoMoreInteractions(mockReporter); verifyNoMoreInteractions(emailService); } @@ -238,7 +235,6 @@ class IcannReportingUploadActionTest { void testFailure_cursorIsNotAdvancedForward() throws Exception { runTest_nonRetryableException( new IOException("Your IP address 25.147.130.158 is not allowed to connect")); - tm().clearSessionCache(); Cursor cursor = loadByKey(Cursor.createScopedVKey(CursorType.ICANN_UPLOAD_ACTIVITY, Registry.get("tld"))); assertThat(cursor.getCursorTime()).isEqualTo(DateTime.parse("2006-07-01TZ")); @@ -249,7 +245,6 @@ class IcannReportingUploadActionTest { clock.setTo(DateTime.parse("2006-05-01T00:30:00Z")); IcannReportingUploadAction action = createAction(); action.run(); - tm().clearSessionCache(); Cursor cursor = loadByKey(Cursor.createScopedVKey(CursorType.ICANN_UPLOAD_ACTIVITY, Registry.get("foo"))); assertThat(cursor.getCursorTime()).isEqualTo(DateTime.parse("2006-07-01TZ")); diff --git a/core/src/test/java/google/registry/testing/DatabaseHelper.java b/core/src/test/java/google/registry/testing/DatabaseHelper.java index 7234ec313..4fedc0d33 100644 --- a/core/src/test/java/google/registry/testing/DatabaseHelper.java +++ b/core/src/test/java/google/registry/testing/DatabaseHelper.java @@ -32,7 +32,6 @@ import static google.registry.model.IdService.allocateId; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; import static google.registry.model.ImmutableObjectSubject.immutableObjectCorrespondence; import static google.registry.model.ResourceTransferUtils.createTransferResponse; -import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.model.tld.Registry.TldState.GENERAL_AVAILABILITY; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -319,7 +318,7 @@ public final class DatabaseHelper { final Domain persistedDomain = persistResource(domain); // Calls {@link LordnTaskUtils#enqueueDomainTask} wrapped in a transaction so that the // transaction time is set correctly. - tm().transactNew(() -> LordnTaskUtils.enqueueDomainTask(persistedDomain)); + tm().transact(() -> LordnTaskUtils.enqueueDomainTask(persistedDomain)); maybeAdvanceClock(); return persistedDomain; } @@ -399,7 +398,7 @@ public final class DatabaseHelper { toImmutableMap(Map.Entry::getKey, entry -> entry.getValue().getValue()))) .build(); // Since we used to persist a PremiumList to Datastore here, it is necessary to allocate an ID - // here to prevent breaking some of the hard-coded flow tests. IDs in tests are allocated in a + // here to prevent breaking some hard-coded flow tests. IDs in tests are allocated in a // strictly increasing sequence, if we don't pad out the ID here, we would have to renumber // hundreds of unit tests. allocateId(); @@ -990,11 +989,6 @@ public final class DatabaseHelper { .isNotInstanceOf(Buildable.Builder.class); tm().transact(() -> tm().put(resource)); maybeAdvanceClock(); - // Force the session cache to be cleared so that when we read the resource back, we read from - // Datastore and not from the session cache. This is needed to trigger Objectify's load process - // (unmarshalling entity protos to POJOs, nulling out empty collections, calling @OnLoad - // methods, etc.) which is bypassed for entities loaded from the session cache. - tm().clearSessionCache(); return tm().transact(() -> tm().loadByEntity(resource)); } @@ -1007,9 +1001,6 @@ public final class DatabaseHelper { } tm().transact(() -> resources.forEach(e -> tm().put(e))); maybeAdvanceClock(); - // Force the session to be cleared so that when we read it back, we read from Datastore - // and not from the transaction's session cache. - tm().clearSessionCache(); } /** @@ -1017,8 +1008,6 @@ public final class DatabaseHelper { * *

This was coded for testing RDE since its queries depend on the associated entries. * - *

Warning: If you call this multiple times in a single test, you need to inject Ofy's - * clock field and forward it by a millisecond between each subsequent call. * * @see #persistResource(ImmutableObject) */ @@ -1035,17 +1024,16 @@ public final class DatabaseHelper { .build()); }); maybeAdvanceClock(); - tm().clearSessionCache(); return tm().transact(() -> tm().loadByEntity(resource)); } - /** Returns all of the history entries that are parented off the given EppResource. */ + /** Returns all the history entries that are parented off the given EppResource. */ public static List getHistoryEntries(EppResource resource) { return HistoryEntryDao.loadHistoryObjectsForResource(resource.createVKey()); } /** - * Returns all of the history entries that are parented off the given EppResource, cast to the + * Returns all the history entries that are parented off the given EppResource, cast to the * corresponding subclass. */ public static List getHistoryEntries( @@ -1054,7 +1042,7 @@ public final class DatabaseHelper { } /** - * Returns all of the history entries that are parented off the given EppResource with the given + * Returns all the history entries that are parented off the given EppResource with the given * type. */ public static ImmutableList getHistoryEntriesOfType( @@ -1065,8 +1053,8 @@ public final class DatabaseHelper { } /** - * Returns all of the history entries that are parented off the given EppResource with the given - * type and cast to the corresponding subclass. + * Returns all the history entries that are parented off the given EppResource with the given type + * and cast to the corresponding subclass. */ public static ImmutableList getHistoryEntriesOfType( EppResource resource, final HistoryEntry.Type type, Class subclazz) { @@ -1161,16 +1149,10 @@ public final class DatabaseHelper { public static void insertSimpleResources(final Iterable resources) { tm().transact(() -> tm().putAll(ImmutableList.copyOf(resources))); maybeAdvanceClock(); - // Force the session to be cleared so that when we read it back, we read from Datastore - // and not from the transaction's session cache. - tm().clearSessionCache(); } public static void deleteResource(final Object resource) { tm().transact(() -> tm().delete(resource)); - // Force the session to be cleared so that when we read it back, we read from Datastore and - // not from the transaction's session cache. - tm().clearSessionCache(); } /** Force the create and update timestamps to get written into the resource. */ @@ -1201,14 +1183,11 @@ public final class DatabaseHelper { * Loads all entities from all classes stored in the database. * *

This is not performant (it requires initializing and detaching all Hibernate entities so - * that they can be used outside of the transaction in which they're loaded) and it should only be + * that they can be used outside the transaction in which they're loaded) and it should only be * used in situations where we need to verify that, for instance, a dry run flow hasn't affected * the database at all. */ public static List loadAllEntities() { - if (tm().isOfy()) { - return auditedOfy().load().list(); - } else { return jpaTm() .transact( () -> { @@ -1224,14 +1203,13 @@ public final class DatabaseHelper { } return result.build(); }); - } } /** * Loads (i.e. reloads) the specified entity from the DB. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. */ public static T loadByEntity(T entity) { return tm().transact(() -> tm().loadByEntity(entity)); @@ -1241,7 +1219,7 @@ public final class DatabaseHelper { * Loads the specified entity by its key from the DB. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. */ public static T loadByKey(VKey key) { return tm().transact(() -> tm().loadByKey(key)); @@ -1251,7 +1229,7 @@ public final class DatabaseHelper { * Loads the specified entity by its key from the DB or empty if it doesn't exist. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. */ public static Optional loadByKeyIfPresent(VKey key) { return tm().transact(() -> tm().loadByKeyIfPresent(key)); @@ -1261,17 +1239,17 @@ public final class DatabaseHelper { * Loads the specified entities by their keys from the DB. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. */ public static ImmutableCollection loadByKeys(Iterable> keys) { return tm().transact(() -> tm().loadByKeys(keys).values()); } /** - * Loads all of the entities of the specified type from the DB. + * Loads all the entities of the specified type from the DB. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. */ public static ImmutableList loadAllOf(Class clazz) { return tm().transact(() -> tm().loadAllOf(clazz)); @@ -1281,7 +1259,7 @@ public final class DatabaseHelper { * Loads the set of entities by their keys from the DB. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. * *

Nonexistent keys / entities are absent from the resulting map, but no {@link * NoSuchElementException} will be thrown. @@ -1295,7 +1273,7 @@ public final class DatabaseHelper { * Loads all given entities from the database if possible. * *

If the transaction manager is Cloud SQL, then this creates an inner wrapping transaction for - * convenience, so you don't need to wrap it in a transaction at the callsite. + * convenience, so you don't need to wrap it in a transaction at the call site. * *

Nonexistent entities are absent from the resulting list, but no {@link * NoSuchElementException} will be thrown. diff --git a/core/src/test/java/google/registry/tmch/LordnTaskUtilsTest.java b/core/src/test/java/google/registry/tmch/LordnTaskUtilsTest.java index efdefa1d9..9798bf830 100644 --- a/core/src/test/java/google/registry/tmch/LordnTaskUtilsTest.java +++ b/core/src/test/java/google/registry/tmch/LordnTaskUtilsTest.java @@ -102,6 +102,6 @@ public class LordnTaskUtilsTest { void test_enqueueDomainTask_throwsNpeOnNullDomain() { assertThrows( NullPointerException.class, - () -> tm().transactNew(() -> LordnTaskUtils.enqueueDomainTask(null))); + () -> tm().transact(() -> LordnTaskUtils.enqueueDomainTask(null))); } } diff --git a/core/src/test/java/google/registry/tools/CommandTestCase.java b/core/src/test/java/google/registry/tools/CommandTestCase.java index 1591c5e43..9d887c21b 100644 --- a/core/src/test/java/google/registry/tools/CommandTestCase.java +++ b/core/src/test/java/google/registry/tools/CommandTestCase.java @@ -100,9 +100,6 @@ public abstract class CommandTestCase { jcommander.parse(args); command.run(); } finally { - // Clear the session cache so that subsequent reads for verification purposes hit Datastore. - // This primarily matters for AutoTimestamp fields, which otherwise won't have updated values. - tm().clearSessionCache(); // Reset back to UNITTEST environment. RegistryToolEnvironment.UNITTEST.setup(systemPropertyExtension); } diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index 096f00afd..d43ce69c6 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -16,7 +16,6 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.CertificateSamples.SAMPLE_CERT; import static google.registry.testing.CertificateSamples.SAMPLE_CERT3; import static google.registry.testing.CertificateSamples.SAMPLE_CERT3_HASH; @@ -89,9 +88,6 @@ class CreateRegistrarCommandTest extends CommandTestCase "clientz"); DateTime after = fakeClock.nowUtc(); - // Clear the cache so that the CreateAutoTimestamp field gets reloaded. - tm().clearSessionCache(); - Optional registrarOptional = Registrar.loadByRegistrarId("clientz"); assertThat(registrarOptional).isPresent(); Registrar registrar = registrarOptional.get(); diff --git a/core/src/test/resources/google/registry/reporting/icann/registrar_operating_status_test.sql b/core/src/test/resources/google/registry/reporting/icann/registrar_operating_status_test.sql deleted file mode 100644 index 01e69ff2c..000000000 --- a/core/src/test/resources/google/registry/reporting/icann/registrar_operating_status_test.sql +++ /dev/null @@ -1,27 +0,0 @@ -#standardSQL - -- Copyright 2017 The Nomulus Authors. All Rights Reserved. - -- - -- Licensed under the Apache License, Version 2.0 (the "License"); - -- you may not use this file except in compliance with the License. - -- You may obtain a copy of the License at - -- - -- http://www.apache.org/licenses/LICENSE-2.0 - -- - -- Unless required by applicable law or agreed to in writing, software - -- distributed under the License is distributed on an "AS IS" BASIS, - -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - -- See the License for the specific language governing permissions and - -- limitations under the License. - - -- Query that counts the number of real registrars in system. - -SELECT - -- Applies to all TLDs, hence the 'null' magic value. - STRING(NULL) AS tld, - 'operational-registrars' AS metricName, - COUNT(registrarName) AS count -FROM - `domain-registry-alpha.latest_datastore_export.Registrar` -WHERE - (type = 'REAL' OR type = 'INTERNAL') -GROUP BY metricName