diff --git a/core/src/main/java/google/registry/model/contact/ContactHistory.java b/core/src/main/java/google/registry/model/contact/ContactHistory.java index 56e0e830e..e9f4e11bc 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java @@ -60,7 +60,9 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { @Id @Access(AccessType.PROPERTY) public String getContactRepoId() { - return parent.getName(); + // We need to handle null case here because Hibernate sometimes accesses this method before + // parent gets initialized + return parent == null ? null : parent.getName(); } /** This method is private because it is only used by Hibernate. */ diff --git a/core/src/main/java/google/registry/model/domain/DomainHistory.java b/core/src/main/java/google/registry/model/domain/DomainHistory.java index 228937834..a773de541 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -75,7 +75,9 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { @Id @Access(AccessType.PROPERTY) public String getDomainRepoId() { - return parent.getName(); + // We need to handle null case here because Hibernate sometimes accesses this method before + // parent gets initialized + return parent == null ? null : parent.getName(); } /** This method is private because it is only used by Hibernate. */ @@ -127,14 +129,24 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { * *

This will be empty for any DomainHistory/HistoryEntry generated before this field was added, * mid-2017, as well as any action that does not generate billable events (e.g. updates). + * + *

This method is dedicated for Hibernate, external caller should use {@link + * #getDomainTransactionRecords()}. */ @Access(AccessType.PROPERTY) @OneToMany(cascade = {CascadeType.ALL}) @JoinColumn(name = "historyRevisionId", referencedColumnName = "historyRevisionId") @JoinColumn(name = "domainRepoId", referencedColumnName = "domainRepoId") - @Override - public Set getDomainTransactionRecords() { - return super.getDomainTransactionRecords(); + @SuppressWarnings("unused") + private Set getInternalDomainTransactionRecords() { + return domainTransactionRecords; + } + + /** Sets the domain transaction records. This method is dedicated for Hibernate. */ + @SuppressWarnings("unused") + private void setInternalDomainTransactionRecords( + Set domainTransactionRecords) { + this.domainTransactionRecords = domainTransactionRecords; } @Id diff --git a/core/src/main/java/google/registry/model/host/HostHistory.java b/core/src/main/java/google/registry/model/host/HostHistory.java index 078020037..3c52666f1 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/core/src/main/java/google/registry/model/host/HostHistory.java @@ -61,7 +61,9 @@ public class HostHistory extends HistoryEntry implements SqlEntity { @Id @Access(AccessType.PROPERTY) public String getHostRepoId() { - return parent.getName(); + // We need to handle null case here because Hibernate sometimes accesses this method before + // parent gets initialized + return parent == null ? null : parent.getName(); } /** This method is private because it is only used by Hibernate. */ diff --git a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java b/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java index 8dc474293..db404f762 100644 --- a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java +++ b/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java @@ -23,7 +23,9 @@ import com.google.common.base.Functions; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Streams; import com.googlecode.objectify.Key; +import com.googlecode.objectify.Result; import google.registry.model.contact.ContactHistory; import google.registry.model.host.HostHistory; import google.registry.model.reporting.HistoryEntry; @@ -102,12 +104,22 @@ public class DatastoreTransactionManager implements TransactionManager { @Override public void insert(Object entity) { - saveEntity(entity); + put(entity); } @Override public void insertAll(ImmutableCollection entities) { - getOfy().save().entities(entities); + putAll(entities); + } + + @Override + public void insertWithoutBackup(Object entity) { + putWithoutBackup(entity); + } + + @Override + public void insertAllWithoutBackup(ImmutableCollection entities) { + putAllWithoutBackup(entities); } @Override @@ -117,17 +129,37 @@ public class DatastoreTransactionManager implements TransactionManager { @Override public void putAll(ImmutableCollection entities) { - getOfy().save().entities(entities); + syncIfTransactionless(getOfy().save().entities(entities)); + } + + @Override + public void putWithoutBackup(Object entity) { + syncIfTransactionless(getOfy().saveWithoutBackup().entities(entity)); + } + + @Override + public void putAllWithoutBackup(ImmutableCollection entities) { + syncIfTransactionless(getOfy().saveWithoutBackup().entities(entities)); } @Override public void update(Object entity) { - saveEntity(entity); + put(entity); } @Override public void updateAll(ImmutableCollection entities) { - getOfy().save().entities(entities); + putAll(entities); + } + + @Override + public void updateWithoutBackup(Object entity) { + putWithoutBackup(entity); + } + + @Override + public void updateAllWithoutBackup(ImmutableCollection entities) { + putAllWithoutBackup(entities); } @Override @@ -158,6 +190,11 @@ public class DatastoreTransactionManager implements TransactionManager { return result; } + @Override + public T load(T entity) { + return ofy().load().entity(entity).now(); + } + @Override public ImmutableMap, T> load(Iterable> keys) { // Keep track of the Key -> VKey mapping so we can translate them back. @@ -175,13 +212,17 @@ public class DatastoreTransactionManager implements TransactionManager { @Override public ImmutableList loadAll(Class clazz) { - // We can do a ofy().load().type(clazz), but this doesn't work in a transaction. - throw new UnsupportedOperationException("Not available in the Datastore transaction manager"); + return ImmutableList.copyOf(getOfy().load().type(clazz)); + } + + @Override + public ImmutableList loadAll(Iterable entities) { + return ImmutableList.copyOf(getOfy().load().entities(entities).values()); } @Override public void delete(VKey key) { - getOfy().delete().key(key.getOfyKey()).now(); + syncIfTransactionless(getOfy().delete().key(key.getOfyKey())); } @Override @@ -192,7 +233,35 @@ public class DatastoreTransactionManager implements TransactionManager { StreamSupport.stream(vKeys.spliterator(), false) .map(VKey::getOfyKey) .collect(toImmutableList()); - getOfy().delete().keys(list).now(); + syncIfTransactionless(getOfy().delete().keys(list)); + } + + @Override + public void delete(Object entity) { + syncIfTransactionless(getOfy().delete().entity(entity)); + } + + @Override + public void deleteWithoutBackup(VKey key) { + syncIfTransactionless(getOfy().deleteWithoutBackup().key(key.getOfyKey())); + } + + @Override + public void deleteWithoutBackup(Iterable> keys) { + syncIfTransactionless( + getOfy() + .deleteWithoutBackup() + .keys(Streams.stream(keys).map(VKey::getOfyKey).collect(toImmutableList()))); + } + + @Override + public void deleteWithoutBackup(Object entity) { + syncIfTransactionless(getOfy().deleteWithoutBackup().entity(entity)); + } + + @Override + public void clearSessionCache() { + getOfy().clearSessionCache(); } /** @@ -209,7 +278,7 @@ public class DatastoreTransactionManager implements TransactionManager { if (entity instanceof HistoryEntry) { entity = ((HistoryEntry) entity).asHistoryEntry(); } - getOfy().save().entity(entity); + syncIfTransactionless(getOfy().save().entity(entity)); } @SuppressWarnings("unchecked") @@ -227,4 +296,19 @@ public class DatastoreTransactionManager implements TransactionManager { private T loadNullable(VKey key) { return toChildHistoryEntryIfPossible(getOfy().load().key(key.getOfyKey()).now()); } + + /** + * Executes the given {@link Result} instance synchronously if not in a transaction. + * + *

The {@link Result} instance contains a task that will be executed by Objectify + * asynchronously. If it is in a transaction, we don't need to execute the task immediately + * because it is guaranteed to be done by the end of the transaction. However, if it is not in a + * transaction, we need to execute it in case the following code expects that happens before + * themselves. + */ + private void syncIfTransactionless(Result result) { + if (!inTransaction()) { + result.now(); + } + } } diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index cd5730661..b9f57bbb2 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -32,6 +32,7 @@ import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registry.Registries.assertTldsExist; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import static google.registry.util.CollectionUtils.nullToEmptyImmutableSortedCopy; import static google.registry.util.PasswordUtils.SALT_SUPPLIER; @@ -704,10 +705,18 @@ public class Registrar extends ImmutableObject return new Builder(clone(this)); } + /** Creates a {@link VKey} for this instance. */ public VKey createVKey() { return VKey.create(Registrar.class, clientIdentifier, Key.create(this)); } + /** Creates a {@link VKey} for the given {@code registrarId}. */ + public static VKey createVKey(String registrarId) { + checkArgumentNotNull(registrarId, "registrarId must be specified"); + return VKey.create( + Registrar.class, registrarId, Key.create(getCrossTldKey(), Registrar.class, registrarId)); + } + /** A builder for constructing {@link Registrar}, since it is immutable. */ public static class Builder extends Buildable.Builder { public Builder() {} @@ -991,8 +1000,7 @@ public class Registrar extends ImmutableObject /** Loads and returns a registrar entity by its client id directly from Datastore. */ public static Optional loadByClientId(String clientId) { checkArgument(!Strings.isNullOrEmpty(clientId), "clientId must be specified"); - return Optional.ofNullable( - ofy().load().type(Registrar.class).parent(getCrossTldKey()).id(clientId).now()); + return transactIfJpaTm(() -> tm().maybeLoad(createVKey(clientId))); } /** diff --git a/core/src/main/java/google/registry/model/registry/Registries.java b/core/src/main/java/google/registry/model/registry/Registries.java index 7953b5b14..599225428 100644 --- a/core/src/main/java/google/registry/model/registry/Registries.java +++ b/core/src/main/java/google/registry/model/registry/Registries.java @@ -25,6 +25,7 @@ import static google.registry.model.CacheUtils.memoizeWithShortExpiration; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.ofyOrJpaTm; import static google.registry.util.CollectionUtils.entriesToImmutableMap; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; @@ -59,15 +60,21 @@ public final class Registries { tm().doTransactionless( () -> { ImmutableSet tlds = - ofy() - .load() - .type(Registry.class) - .ancestor(getCrossTldKey()) - .keys() - .list() - .stream() - .map(Key::getName) - .collect(toImmutableSet()); + ofyOrJpaTm( + () -> + ofy() + .load() + .type(Registry.class) + .ancestor(getCrossTldKey()) + .keys() + .list() + .stream() + .map(Key::getName) + .collect(toImmutableSet()), + () -> + tm().loadAll(Registry.class).stream() + .map(Registry::getTldStr) + .collect(toImmutableSet())); return Registry.getAll(tlds).stream() .map(e -> Maps.immutableEntry(e.getTldStr(), e.getTldType())) .collect(entriesToImmutableMap()); diff --git a/core/src/main/java/google/registry/model/registry/Registry.java b/core/src/main/java/google/registry/model/registry/Registry.java index 41805cffd..1df3ce0bc 100644 --- a/core/src/main/java/google/registry/model/registry/Registry.java +++ b/core/src/main/java/google/registry/model/registry/Registry.java @@ -22,7 +22,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Maps.toMap; import static google.registry.config.RegistryConfig.getSingletonCacheRefreshDuration; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; -import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import static google.registry.util.DateTimeUtils.END_OF_TIME; @@ -61,6 +60,7 @@ import google.registry.model.domain.fee.BaseFee.FeeType; import google.registry.model.domain.fee.Fee; import google.registry.model.registry.label.PremiumList; import google.registry.model.registry.label.ReservedList; +import google.registry.persistence.VKey; import google.registry.schema.replay.DatastoreAndSqlEntity; import google.registry.util.Idn; import java.util.Map; @@ -267,28 +267,24 @@ public class Registry extends ImmutableObject implements Buildable, DatastoreAnd public Optional load(final String tld) { // Enter a transaction-less context briefly; we don't want to enroll every TLD in // a transaction that might be wrapping this call. - return Optional.ofNullable( - tm().doTransactionless( - () -> - ofy() - .load() - .key(Key.create(getCrossTldKey(), Registry.class, tld)) - .now())); + return tm().doTransactionless(() -> tm().maybeLoad(createVKey(tld))); } @Override public Map> loadAll(Iterable tlds) { - ImmutableMap> keysMap = - toMap( - ImmutableSet.copyOf(tlds), - tld -> Key.create(getCrossTldKey(), Registry.class, tld)); - Map, Registry> entities = - tm().doTransactionless(() -> ofy().load().keys(keysMap.values())); + ImmutableMap> keysMap = + toMap(ImmutableSet.copyOf(tlds), Registry::createVKey); + Map, Registry> entities = + tm().doTransactionless(() -> tm().load(keysMap.values())); return Maps.transformEntries( keysMap, (k, v) -> Optional.ofNullable(entities.getOrDefault(v, null))); } }); + public static VKey createVKey(String tld) { + return VKey.create(Registry.class, tld, Key.create(getCrossTldKey(), Registry.class, tld)); + } + /** * The name of the pricing engine that this TLD uses. * @@ -386,7 +382,7 @@ public class Registry extends ImmutableObject implements Buildable, DatastoreAnd CreateAutoTimestamp creationTime = CreateAutoTimestamp.create(null); /** The set of reserved lists that are applicable to this registry. */ - @Column(name = "reserved_list_names", nullable = false) + @Column(name = "reserved_list_names") Set> reservedLists; /** Retrieves an ImmutableSet of all ReservedLists associated with this tld. */ diff --git a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java index 6e9d3c156..f8a95da33 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java +++ b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java @@ -193,7 +193,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor * transaction counts (such as contact or host mutations). */ @Transient // domain-specific - Set domainTransactionRecords; + protected Set domainTransactionRecords; public long getId() { // For some reason, Hibernate throws NPE during some initialization phase if we don't deal with @@ -273,7 +273,8 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor /** This method exists solely to satisfy Hibernate. Use the {@link Builder} instead. */ @SuppressWarnings("UnusedMethod") private void setDomainTransactionRecords(Set domainTransactionRecords) { - this.domainTransactionRecords = ImmutableSet.copyOf(domainTransactionRecords); + this.domainTransactionRecords = + domainTransactionRecords == null ? null : ImmutableSet.copyOf(domainTransactionRecords); } public static VKey createVKey(Key key) { 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 457b4fd28..ce5e3af8e 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -15,6 +15,7 @@ package google.registry.persistence.transaction; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; @@ -25,6 +26,7 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import google.registry.config.RegistryConfig; import google.registry.persistence.JpaRetries; @@ -238,6 +240,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { entities.forEach(this::insert); } + @Override + public void insertWithoutBackup(Object entity) { + insert(entity); + } + + @Override + public void insertAllWithoutBackup(ImmutableCollection entities) { + insertAll(entities); + } + @Override public void put(Object entity) { checkArgumentNotNull(entity, "entity must be specified"); @@ -253,6 +265,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { entities.forEach(this::put); } + @Override + public void putWithoutBackup(Object entity) { + put(entity); + } + + @Override + public void putAllWithoutBackup(ImmutableCollection entities) { + putAll(entities); + } + @Override public void update(Object entity) { checkArgumentNotNull(entity, "entity must be specified"); @@ -269,6 +291,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { entities.forEach(this::update); } + @Override + public void updateWithoutBackup(Object entity) { + update(entity); + } + + @Override + public void updateAllWithoutBackup(ImmutableCollection entities) { + updateAll(entities); + } + @Override public boolean exists(VKey key) { checkArgumentNotNull(key, "key must be specified"); @@ -315,6 +347,14 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { return result; } + @Override + public T load(T entity) { + checkArgumentNotNull(entity, "entity must be specified"); + assertInTransaction(); + return (T) + load(VKey.createSql(entity.getClass(), emf.getPersistenceUnitUtil().getIdentifier(entity))); + } + @Override public ImmutableMap, T> load(Iterable> keys) { checkArgumentNotNull(keys, "keys must be specified"); @@ -342,6 +382,11 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { .getResultList()); } + @Override + public ImmutableList loadAll(Iterable entities) { + return Streams.stream(entities).map(this::load).collect(toImmutableList()); + } + private int internalDelete(VKey key) { checkArgumentNotNull(key, "key must be specified"); assertInTransaction(); @@ -366,6 +411,37 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { vKeys.forEach(this::internalDelete); } + @Override + public void delete(Object entity) { + checkArgumentNotNull(entity, "entity must be specified"); + assertInTransaction(); + Object managedEntity = entity; + if (!getEntityManager().contains(entity)) { + managedEntity = getEntityManager().merge(entity); + } + getEntityManager().remove(managedEntity); + } + + @Override + public void deleteWithoutBackup(VKey key) { + delete(key); + } + + @Override + public void deleteWithoutBackup(Iterable> keys) { + delete(keys); + } + + @Override + public void deleteWithoutBackup(Object entity) { + delete(entity); + } + + @Override + public void clearSessionCache() { + // This is an intended no-op method as there is no session cache in Postgresql. + } + @Override public void assertDelete(VKey key) { if (internalDelete(key) != 1) { 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 82f747c44..edbc1b0b5 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -91,18 +91,90 @@ public interface TransactionManager { /** Persists all new entities in the database, throws exception if any entity already exists. */ void insertAll(ImmutableCollection entities); + /** + * Persists a new entity in the database without writing any backup if the underlying database is + * Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void insertWithoutBackup(Object entity); + + /** + * Persists all new entities in the database without writing any backup if the underlying database + * is Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void insertAllWithoutBackup(ImmutableCollection entities); + /** Persists a new entity or update the existing entity in the database. */ void put(Object entity); /** Persists all new entities or update the existing entities in the database. */ void putAll(ImmutableCollection entities); + /** + * Persists a new entity or update the existing entity in the database without writing any backup + * if the underlying database is Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void putWithoutBackup(Object entity); + + /** + * Persists all new entities or update the existing entities in the database without writing any + * backup if the underlying database is Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void putAllWithoutBackup(ImmutableCollection entities); + /** Updates an entity in the database, throws exception if the entity does not exist. */ void update(Object entity); /** Updates all entities in the database, throws exception if any entity does not exist. */ void updateAll(ImmutableCollection entities); + /** + * Updates an entity in the database without writing any backup if the underlying database is + * Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void updateWithoutBackup(Object entity); + + /** + * Updates all entities in the database without writing any backup if the underlying database is + * Datastore. + * + *

This method is for the sake of keeping a single code path when replacing ofy() with tm() in + * the application code. When the method is invoked with Datastore, it won't write the commit log + * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have + * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in + * SQL. + */ + void updateAllWithoutBackup(ImmutableCollection entities); + /** Returns whether the given entity with same ID exists. */ boolean exists(Object entity); @@ -115,6 +187,11 @@ public interface TransactionManager { /** Loads the entity by its id, throws NoSuchElementException if it doesn't exist. */ T load(VKey key); + /** + * Loads the given entity from the database, throws NoSuchElementException if it doesn't exist. + */ + T load(T entity); + /** * Loads the set of entities by their key id. * @@ -125,9 +202,38 @@ public interface TransactionManager { /** Loads all entities of the given type, returns empty if there is no such entity. */ ImmutableList loadAll(Class clazz); + /** + * Loads all given entities from the database, throws NoSuchElementException if it doesn't exist. + */ + ImmutableList loadAll(Iterable entities); + /** Deletes the entity by its id. */ void delete(VKey key); /** Deletes the set of entities by their key id. */ void delete(Iterable> keys); + + /** Deletes the given entity from the database. */ + void delete(Object entity); + + /** + * Deletes the entity by its id without writing any backup if the underlying database is + * Datastore. + */ + void deleteWithoutBackup(VKey key); + + /** + * Deletes the set of entities by their key id without writing any backup if the underlying + * database is Datastore. + */ + void deleteWithoutBackup(Iterable> keys); + + /** + * Deletes the given entity from the database without writing any backup if the underlying + * database is Datastore. + */ + void deleteWithoutBackup(Object entity); + + /** Clears the session cache if the underlying database is Datastore, otherwise it is a no-op. */ + void clearSessionCache(); } diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManagerUtil.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManagerUtil.java new file mode 100644 index 000000000..1028505b3 --- /dev/null +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManagerUtil.java @@ -0,0 +1,110 @@ +// Copyright 2020 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. + +package google.registry.persistence.transaction; + +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; + +import google.registry.model.ofy.DatastoreTransactionManager; +import java.util.function.Supplier; + +/** Utility class that provides supplementary methods for {@link TransactionManager}. */ +public class TransactionManagerUtil { + + /** + * Returns the result of the given {@link Supplier}. + * + *

If {@link TransactionManagerFactory#tm()} returns a {@link JpaTransactionManager} instance, + * the {@link Supplier} is executed in a transaction. + */ + public static T transactIfJpaTm(Supplier supplier) { + if (tm() instanceof JpaTransactionManager) { + return tm().transact(supplier); + } else { + return supplier.get(); + } + } + + /** + * Executes the given {@link Runnable}. + * + *

If {@link TransactionManagerFactory#tm()} returns a {@link JpaTransactionManager} instance, + * the {@link Runnable} is executed in a transaction. + */ + public static void transactIfJpaTm(Runnable runnable) { + transactIfJpaTm( + () -> { + runnable.run(); + return null; + }); + } + + /** + * Executes either {@code ofyRunnable} if {@link TransactionManagerFactory#tm()} returns a {@link + * JpaTransactionManager} instance, or {@code jpaRunnable} if {@link + * TransactionManagerFactory#tm()} returns a {@link DatastoreTransactionManager} instance. + */ + public static void ofyOrJpaTm(Runnable ofyRunnable, Runnable jpaRunnable) { + ofyOrJpaTm( + () -> { + ofyRunnable.run(); + return null; + }, + () -> { + jpaRunnable.run(); + return null; + }); + } + + /** + * Returns the result from either {@code ofySupplier} if {@link TransactionManagerFactory#tm()} + * returns a {@link JpaTransactionManager} instance, or {@code jpaSupplier} if {@link + * TransactionManagerFactory#tm()} returns a {@link DatastoreTransactionManager} instance. + */ + public static T ofyOrJpaTm(Supplier ofySupplier, Supplier jpaSupplier) { + if (tm() instanceof DatastoreTransactionManager) { + return ofySupplier.get(); + } else if (tm() instanceof JpaTransactionManager) { + return jpaSupplier.get(); + } else { + throw new IllegalStateException( + "Expected tm() to be DatastoreTransactionManager or JpaTransactionManager, but got " + + tm().getClass()); + } + } + + /** + * Executes the given {@link Runnable} if {@link TransactionManagerFactory#tm()} returns a {@link + * DatastoreTransactionManager} instance, otherwise does nothing. + */ + public static void ofyTmOrDoNothing(Runnable ofyRunnable) { + if (tm() instanceof DatastoreTransactionManager) { + ofyRunnable.run(); + } + } + + /** + * Returns the result from the given {@link Supplier} if {@link TransactionManagerFactory#tm()} + * returns a {@link DatastoreTransactionManager} instance, otherwise returns null. + */ + public static T ofyTmOrDoNothing(Supplier ofySupplier) { + if (tm() instanceof DatastoreTransactionManager) { + return ofySupplier.get(); + } else { + return null; + } + } + + private TransactionManagerUtil() {} +} diff --git a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java index ffa96ce92..0ab3c58b0 100644 --- a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java +++ b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java @@ -480,8 +480,7 @@ public class ExpandRecurringBillingEventsActionTest .setSyntheticCreationTime(testTime) .build()); } - assertBillingEventsForResource( - domain, Iterables.toArray(expectedEvents, BillingEvent.class)); + assertBillingEventsForResource(domain, Iterables.toArray(expectedEvents, BillingEvent.class)); assertCursorAt(testTime); } diff --git a/core/src/test/java/google/registry/model/billing/BillingEventTest.java b/core/src/test/java/google/registry/model/billing/BillingEventTest.java index 59d2d29c5..4d3241ed4 100644 --- a/core/src/test/java/google/registry/model/billing/BillingEventTest.java +++ b/core/src/test/java/google/registry/model/billing/BillingEventTest.java @@ -17,11 +17,12 @@ package google.registry.model.billing; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.domain.token.AllocationToken.TokenType.UNLIMITED_USE; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.ofyTmOrDoNothing; +import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistActiveDomain; import static google.registry.testing.DatastoreHelper.persistResource; -import static google.registry.testing.SqlHelper.saveRegistrar; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static org.joda.money.CurrencyUnit.USD; import static org.joda.time.DateTimeZone.UTC; @@ -39,13 +40,17 @@ import google.registry.model.domain.rgp.GracePeriodStatus; import google.registry.model.domain.token.AllocationToken; import google.registry.model.domain.token.AllocationToken.TokenStatus; import google.registry.model.reporting.HistoryEntry; +import google.registry.persistence.VKey; +import google.registry.testing.DualDatabaseTest; +import google.registry.testing.TestOfyAndSql; +import google.registry.testing.TestOfyOnly; import google.registry.util.DateTimeUtils; import org.joda.money.Money; import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; /** Unit tests for {@link BillingEvent}. */ +@DualDatabaseTest public class BillingEventTest extends EntityTestCase { private final DateTime now = DateTime.now(UTC); @@ -67,16 +72,26 @@ public class BillingEventTest extends EntityTestCase { void setUp() { createTld("tld"); domain = persistActiveDomain("foo.tld"); - historyEntry = persistResource( - new HistoryEntry.Builder() - .setParent(domain) - .setModificationTime(now) - .build()); - historyEntry2 = persistResource( - new HistoryEntry.Builder() - .setParent(domain) - .setModificationTime(now.plusDays(1)) - .build()); + historyEntry = + persistResource( + new HistoryEntry.Builder() + .setParent(domain) + .setModificationTime(now) + .setRequestedByRegistrar(false) + .setType(HistoryEntry.Type.DOMAIN_CREATE) + .setXmlBytes(new byte[0]) + .build() + .toChildHistoryEntity()); + historyEntry2 = + persistResource( + new HistoryEntry.Builder() + .setParent(domain) + .setModificationTime(now.plusDays(1)) + .setRequestedByRegistrar(false) + .setType(HistoryEntry.Type.DOMAIN_CREATE) + .setXmlBytes(new byte[0]) + .build() + .toChildHistoryEntity()); AllocationToken allocationToken = persistResource( @@ -149,74 +164,38 @@ public class BillingEventTest extends EntityTestCase { .setEventTime(now.plusDays(1)) .setBillingTime(now.plusYears(1).plusDays(45)) .setRecurringEventKey(recurring.createVKey()))); - modification = persistResource(commonInit( - new BillingEvent.Modification.Builder() - .setParent(historyEntry2) - .setReason(Reason.CREATE) - .setCost(Money.of(USD, 1)) - .setDescription("Something happened") - .setEventTime(now.plusDays(1)) - .setEventKey(Key.create(oneTime)))); + modification = + ofyTmOrDoNothing( + () -> + persistResource( + commonInit( + new BillingEvent.Modification.Builder() + .setParent(historyEntry2) + .setReason(Reason.CREATE) + .setCost(Money.of(USD, 1)) + .setDescription("Something happened") + .setEventTime(now.plusDays(1)) + .setEventKey(Key.create(oneTime))))); } private > E commonInit(B builder) { - return builder - .setClientId("a registrar") - .setTargetId("foo.tld") - .build(); + return builder.setClientId("TheRegistrar").setTargetId("foo.tld").build(); } - private void saveNewBillingEvent(BillingEvent billingEvent) { - jpaTm().transact(() -> jpaTm().insert(billingEvent)); - } - - @Test - void testCloudSqlPersistence_OneTime() { - saveRegistrar("a registrar"); - saveNewBillingEvent(oneTime); - - BillingEvent.OneTime persisted = jpaTm().transact(() -> jpaTm().load(oneTime.createVKey())); - // TODO(b/168325240): Remove this fix after VKeyConverter generates symmetric key for - // AllocationToken. - BillingEvent.OneTime fixed = - persisted.asBuilder().setAllocationToken(oneTime.getAllocationToken().get()).build(); - assertThat(fixed).isEqualTo(oneTime); - } - - @Test - void testCloudSqlPersistence_Cancellation() { - saveRegistrar("a registrar"); - saveNewBillingEvent(oneTime); - saveNewBillingEvent(cancellationOneTime); - - BillingEvent.Cancellation persisted = - jpaTm().transact(() -> jpaTm().load(cancellationOneTime.createVKey())); - // TODO(b/168537779): Remove this fix after VKey can be reconstructed correctly. - BillingEvent.Cancellation fixed = - persisted.asBuilder().setOneTimeEventKey(oneTime.createVKey()).build(); - assertThat(fixed).isEqualTo(cancellationOneTime); - } - - @Test - void testCloudSqlPersistence_Recurring() { - saveRegistrar("a registrar"); - saveNewBillingEvent(recurring); - - BillingEvent.Recurring persisted = jpaTm().transact(() -> jpaTm().load(recurring.createVKey())); - assertThat(persisted).isEqualTo(recurring); - } - - @Test + @TestOfyAndSql void testPersistence() { - assertThat(ofy().load().entity(oneTime).now()).isEqualTo(oneTime); - assertThat(ofy().load().entity(oneTimeSynthetic).now()).isEqualTo(oneTimeSynthetic); - assertThat(ofy().load().entity(recurring).now()).isEqualTo(recurring); - assertThat(ofy().load().entity(cancellationOneTime).now()).isEqualTo(cancellationOneTime); - assertThat(ofy().load().entity(cancellationRecurring).now()).isEqualTo(cancellationRecurring); - assertThat(ofy().load().entity(modification).now()).isEqualTo(modification); + assertThat(transactIfJpaTm(() -> tm().load(oneTime))).isEqualTo(oneTime); + assertThat(transactIfJpaTm(() -> tm().load(oneTimeSynthetic))).isEqualTo(oneTimeSynthetic); + assertThat(transactIfJpaTm(() -> tm().load(recurring))).isEqualTo(recurring); + assertThat(transactIfJpaTm(() -> tm().load(cancellationOneTime))) + .isEqualTo(cancellationOneTime); + assertThat(transactIfJpaTm(() -> tm().load(cancellationRecurring))) + .isEqualTo(cancellationRecurring); + + ofyTmOrDoNothing(() -> assertThat(tm().load(modification)).isEqualTo(modification)); } - @Test + @TestOfyOnly void testParenting() { // Note that these are all tested separately because BillingEvent is an abstract base class that // lacks the @Entity annotation, and thus we cannot call .type(BillingEvent.class) @@ -238,19 +217,14 @@ public class BillingEventTest extends EntityTestCase { .containsExactly(modification); } - @Test + @TestOfyAndSql void testCancellationMatching() { - Key recurringKey = - ofy() - .load() - .entity(oneTimeSynthetic) - .now() - .getCancellationMatchingBillingEvent() - .getOfyKey(); - assertThat(ofy().load().key(recurringKey).now()).isEqualTo(recurring); + VKey recurringKey = + transactIfJpaTm(() -> tm().load(oneTimeSynthetic).getCancellationMatchingBillingEvent()); + assertThat(transactIfJpaTm(() -> tm().load(recurringKey))).isEqualTo(recurring); } - @Test + @TestOfyOnly void testIndexing() throws Exception { verifyIndexing( oneTime, @@ -272,7 +246,7 @@ public class BillingEventTest extends EntityTestCase { verifyIndexing(modification, "clientId", "eventTime"); } - @Test + @TestOfyAndSql void testFailure_syntheticFlagWithoutCreationTime() { IllegalStateException thrown = assertThrows( @@ -288,7 +262,7 @@ public class BillingEventTest extends EntityTestCase { .contains("Synthetic creation time must be set if and only if the SYNTHETIC flag is set."); } - @Test + @TestOfyAndSql void testFailure_syntheticCreationTimeWithoutFlag() { IllegalStateException thrown = assertThrows( @@ -299,7 +273,7 @@ public class BillingEventTest extends EntityTestCase { .contains("Synthetic creation time must be set if and only if the SYNTHETIC flag is set"); } - @Test + @TestOfyAndSql void testFailure_syntheticFlagWithoutCancellationMatchingKey() { IllegalStateException thrown = assertThrows( @@ -317,7 +291,7 @@ public class BillingEventTest extends EntityTestCase { + "if and only if the SYNTHETIC flag is set"); } - @Test + @TestOfyAndSql void testFailure_cancellationMatchingKeyWithoutFlag() { IllegalStateException thrown = assertThrows( @@ -334,7 +308,7 @@ public class BillingEventTest extends EntityTestCase { + "if and only if the SYNTHETIC flag is set"); } - @Test + @TestOfyAndSql void testSuccess_cancellation_forGracePeriod_withOneTime() { BillingEvent.Cancellation newCancellation = BillingEvent.Cancellation.forGracePeriod( @@ -343,10 +317,15 @@ public class BillingEventTest extends EntityTestCase { "foo.tld"); // Set ID to be the same to ignore for the purposes of comparison. newCancellation = newCancellation.asBuilder().setId(cancellationOneTime.getId()).build(); - assertThat(newCancellation).isEqualTo(cancellationOneTime); + + // TODO(b/168537779): Remove setRecurringEventKey after symmetric VKey can be reconstructed + // correctly. + assertThat(newCancellation) + .isEqualTo( + cancellationOneTime.asBuilder().setOneTimeEventKey(oneTime.createVKey()).build()); } - @Test + @TestOfyAndSql void testSuccess_cancellation_forGracePeriod_withRecurring() { BillingEvent.Cancellation newCancellation = BillingEvent.Cancellation.forGracePeriod( @@ -354,16 +333,21 @@ public class BillingEventTest extends EntityTestCase { GracePeriodStatus.AUTO_RENEW, domain.getRepoId(), now.plusYears(1).plusDays(45), - "a registrar", + "TheRegistrar", recurring.createVKey()), historyEntry2, "foo.tld"); // Set ID to be the same to ignore for the purposes of comparison. newCancellation = newCancellation.asBuilder().setId(cancellationRecurring.getId()).build(); - assertThat(newCancellation).isEqualTo(cancellationRecurring); + + // TODO(b/168537779): Remove setRecurringEventKey after symmetric VKey can be reconstructed + // correctly. + assertThat(newCancellation) + .isEqualTo( + cancellationRecurring.asBuilder().setRecurringEventKey(recurring.createVKey()).build()); } - @Test + @TestOfyAndSql void testFailure_cancellation_forGracePeriodWithoutBillingEvent() { IllegalArgumentException thrown = assertThrows( @@ -380,7 +364,7 @@ public class BillingEventTest extends EntityTestCase { assertThat(thrown).hasMessageThat().contains("grace period without billing event"); } - @Test + @TestOfyAndSql void testFailure_cancellationWithNoBillingEvent() { IllegalStateException thrown = assertThrows( @@ -394,7 +378,7 @@ public class BillingEventTest extends EntityTestCase { assertThat(thrown).hasMessageThat().contains("exactly one billing event"); } - @Test + @TestOfyAndSql void testFailure_cancellationWithBothBillingEvents() { IllegalStateException thrown = assertThrows( @@ -408,7 +392,7 @@ public class BillingEventTest extends EntityTestCase { assertThat(thrown).hasMessageThat().contains("exactly one billing event"); } - @Test + @TestOfyAndSql void testDeadCodeThatDeletedScrapCommandsReference() { assertThat(recurring.getParentKey()).isEqualTo(Key.create(historyEntry)); new BillingEvent.OneTime.Builder().setParent(Key.create(historyEntry)); diff --git a/core/src/test/java/google/registry/model/rde/RdeRevisionTest.java b/core/src/test/java/google/registry/model/rde/RdeRevisionTest.java index f9e27548b..a1ab776f9 100644 --- a/core/src/test/java/google/registry/model/rde/RdeRevisionTest.java +++ b/core/src/test/java/google/registry/model/rde/RdeRevisionTest.java @@ -23,9 +23,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import google.registry.model.EntityTestCase; import google.registry.testing.DualDatabaseTest; +import google.registry.testing.TestOfyAndSql; import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.TestTemplate; /** Unit tests for {@link RdeRevision}. */ @DualDatabaseTest @@ -40,20 +40,20 @@ public class RdeRevisionTest extends EntityTestCase { fakeClock.setTo(DateTime.parse("1984-12-18TZ")); } - @TestTemplate + @TestOfyAndSql void testGetNextRevision_objectDoesntExist_returnsZero() { tm().transact( () -> assertThat(getNextRevision("torment", fakeClock.nowUtc(), FULL)).isEqualTo(0)); } - @TestTemplate + @TestOfyAndSql void testGetNextRevision_objectExistsAtZero_returnsOne() { save("sorrow", fakeClock.nowUtc(), FULL, 0); tm().transact( () -> assertThat(getNextRevision("sorrow", fakeClock.nowUtc(), FULL)).isEqualTo(1)); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_objectDoesntExist_newRevisionIsZero_nextRevIsOne() { tm().transact(() -> saveRevision("despondency", fakeClock.nowUtc(), FULL, 0)); tm().transact( @@ -61,7 +61,7 @@ public class RdeRevisionTest extends EntityTestCase { assertThat(getNextRevision("despondency", fakeClock.nowUtc(), FULL)).isEqualTo(1)); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_objectDoesntExist_newRevisionIsOne_throwsVe() { IllegalArgumentException thrown = assertThrows( @@ -74,7 +74,7 @@ public class RdeRevisionTest extends EntityTestCase { + "when trying to save new revision 1"); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_objectExistsAtZero_newRevisionIsZero_throwsVe() { save("melancholy", fakeClock.nowUtc(), FULL, 0); IllegalArgumentException thrown = @@ -84,7 +84,7 @@ public class RdeRevisionTest extends EntityTestCase { assertThat(thrown).hasMessageThat().contains("object already created"); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_objectExistsAtZero_newRevisionIsOne_nextRevIsTwo() { DateTime startOfDay = fakeClock.nowUtc().withTimeAtStartOfDay(); save("melancholy", startOfDay, FULL, 0); @@ -93,7 +93,7 @@ public class RdeRevisionTest extends EntityTestCase { tm().transact(() -> assertThat(getNextRevision("melancholy", startOfDay, FULL)).isEqualTo(2)); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_objectExistsAtZero_newRevisionIsTwo_throwsVe() { save("melancholy", fakeClock.nowUtc(), FULL, 0); IllegalArgumentException thrown = @@ -105,7 +105,7 @@ public class RdeRevisionTest extends EntityTestCase { .contains("RDE revision object should be at revision 1 but was"); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_negativeRevision_throwsIae() { IllegalArgumentException thrown = assertThrows( @@ -114,7 +114,7 @@ public class RdeRevisionTest extends EntityTestCase { assertThat(thrown).hasMessageThat().contains("Negative revision"); } - @TestTemplate + @TestOfyAndSql void testSaveRevision_callerNotInTransaction_throwsIse() { IllegalStateException thrown = assertThrows( 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 4fe27c0a9..c3898a9da 100644 --- a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java @@ -33,6 +33,8 @@ import google.registry.testing.AppEngineExtension; import google.registry.testing.DualDatabaseTest; import google.registry.testing.FakeClock; import google.registry.testing.InjectExtension; +import google.registry.testing.TestOfyAndSql; +import google.registry.testing.TestOfyOnly; import java.util.List; import java.util.NoSuchElementException; import java.util.Set; @@ -40,7 +42,6 @@ import java.util.stream.Stream; import javax.persistence.Embeddable; import javax.persistence.MappedSuperclass; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.RegisterExtension; /** @@ -78,28 +79,28 @@ public class TransactionManagerTest { fakeClock.advanceOneMilli(); } - @TestTemplate + @TestOfyAndSql void inTransaction_returnsCorrespondingResult() { assertThat(tm().inTransaction()).isFalse(); tm().transact(() -> assertThat(tm().inTransaction()).isTrue()); assertThat(tm().inTransaction()).isFalse(); } - @TestTemplate + @TestOfyAndSql void assertInTransaction_throwsExceptionWhenNotInTransaction() { assertThrows(IllegalStateException.class, () -> tm().assertInTransaction()); tm().transact(() -> tm().assertInTransaction()); assertThrows(IllegalStateException.class, () -> tm().assertInTransaction()); } - @TestTemplate + @TestOfyAndSql void getTransactionTime_throwsExceptionWhenNotInTransaction() { assertThrows(IllegalStateException.class, () -> tm().getTransactionTime()); tm().transact(() -> assertThat(tm().getTransactionTime()).isEqualTo(fakeClock.nowUtc())); assertThrows(IllegalStateException.class, () -> tm().getTransactionTime()); } - @TestTemplate + @TestOfyAndSql void transact_hasNoEffectWithPartialSuccess() { assertEntityNotExist(theEntity); assertThrows( @@ -113,21 +114,21 @@ public class TransactionManagerTest { assertEntityNotExist(theEntity); } - @TestTemplate + @TestOfyAndSql void transact_reusesExistingTransaction() { assertEntityNotExist(theEntity); tm().transact(() -> tm().transact(() -> tm().insert(theEntity))); assertEntityExists(theEntity); } - @TestTemplate + @TestOfyAndSql void transactNew_succeeds() { assertEntityNotExist(theEntity); tm().transactNew(() -> tm().insert(theEntity)); assertEntityExists(theEntity); } - @TestTemplate + @TestOfyAndSql void transactNewReadOnly_succeeds() { assertEntityNotExist(theEntity); tm().transact(() -> tm().insert(theEntity)); @@ -136,7 +137,7 @@ public class TransactionManagerTest { assertThat(persisted).isEqualTo(theEntity); } - @TestTemplate + @TestOfyAndSql void transactNewReadOnly_throwsWhenWritingEntity() { assertEntityNotExist(theEntity); assertThrows( @@ -144,7 +145,7 @@ public class TransactionManagerTest { assertEntityNotExist(theEntity); } - @TestTemplate + @TestOfyAndSql void saveNew_succeeds() { assertEntityNotExist(theEntity); tm().transact(() -> tm().insert(theEntity)); @@ -152,14 +153,14 @@ public class TransactionManagerTest { assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity); } - @TestTemplate + @TestOfyAndSql void saveAllNew_succeeds() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); assertAllEntitiesExist(moreEntities); } - @TestTemplate + @TestOfyAndSql void saveNewOrUpdate_persistsNewEntity() { assertEntityNotExist(theEntity); tm().transact(() -> tm().put(theEntity)); @@ -167,7 +168,7 @@ public class TransactionManagerTest { assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity); } - @TestTemplate + @TestOfyAndSql void saveNewOrUpdate_updatesExistingEntity() { tm().transact(() -> tm().insert(theEntity)); TestEntity persisted = tm().transact(() -> tm().load(theEntity.key())); @@ -179,14 +180,14 @@ public class TransactionManagerTest { assertThat(persisted.data).isEqualTo("bar"); } - @TestTemplate + @TestOfyAndSql void saveNewOrUpdateAll_succeeds() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().putAll(moreEntities)); assertAllEntitiesExist(moreEntities); } - @TestTemplate + @TestOfyAndSql void update_succeeds() { tm().transact(() -> tm().insert(theEntity)); TestEntity persisted = @@ -202,7 +203,7 @@ public class TransactionManagerTest { assertThat(persisted.data).isEqualTo("bar"); } - @TestTemplate + @TestOfyAndSql void load_succeeds() { assertEntityNotExist(theEntity); tm().transact(() -> tm().insert(theEntity)); @@ -211,14 +212,14 @@ public class TransactionManagerTest { assertThat(persisted.data).isEqualTo("foo"); } - @TestTemplate + @TestOfyAndSql void load_throwsOnMissingElement() { assertEntityNotExist(theEntity); assertThrows( NoSuchElementException.class, () -> tm().transact(() -> tm().load(theEntity.key()))); } - @TestTemplate + @TestOfyAndSql void maybeLoad_succeeds() { assertEntityNotExist(theEntity); tm().transact(() -> tm().insert(theEntity)); @@ -227,13 +228,13 @@ public class TransactionManagerTest { assertThat(persisted.data).isEqualTo("foo"); } - @TestTemplate + @TestOfyAndSql void maybeLoad_nonExistentObject() { assertEntityNotExist(theEntity); assertThat(tm().transact(() -> tm().maybeLoad(theEntity.key())).isPresent()).isFalse(); } - @TestTemplate + @TestOfyAndSql void delete_succeeds() { tm().transact(() -> tm().insert(theEntity)); assertEntityExists(theEntity); @@ -242,14 +243,14 @@ public class TransactionManagerTest { assertEntityNotExist(theEntity); } - @TestTemplate + @TestOfyAndSql void delete_doNothingWhenEntityNotExist() { assertEntityNotExist(theEntity); tm().transact(() -> tm().delete(theEntity.key())); assertEntityNotExist(theEntity); } - @TestTemplate + @TestOfyAndSql void delete_succeedsForEntitySet() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); @@ -261,7 +262,7 @@ public class TransactionManagerTest { assertAllEntitiesNotExist(moreEntities); } - @TestTemplate + @TestOfyAndSql void delete_ignoreNonExistentEntity() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); @@ -276,7 +277,16 @@ public class TransactionManagerTest { assertAllEntitiesNotExist(moreEntities); } - @TestTemplate + @TestOfyAndSql + void delete_deletesTheGivenEntity() { + tm().transact(() -> tm().insert(theEntity)); + assertEntityExists(theEntity); + fakeClock.advanceOneMilli(); + tm().transact(() -> tm().delete(theEntity)); + assertEntityNotExist(theEntity); + } + + @TestOfyAndSql void load_multi() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); @@ -286,7 +296,7 @@ public class TransactionManagerTest { .isEqualTo(Maps.uniqueIndex(moreEntities, TestEntity::key)); } - @TestTemplate + @TestOfyAndSql void load_multiWithDuplicateKeys() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); @@ -298,7 +308,7 @@ public class TransactionManagerTest { .isEqualTo(Maps.uniqueIndex(moreEntities, TestEntity::key)); } - @TestTemplate + @TestOfyAndSql void load_multiMissingKeys() { assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().insertAll(moreEntities)); @@ -310,6 +320,14 @@ public class TransactionManagerTest { .isEqualTo(Maps.uniqueIndex(moreEntities, TestEntity::key)); } + @TestOfyOnly + void loadAllForOfyTm_throwsExceptionInTransaction() { + assertAllEntitiesNotExist(moreEntities); + tm().transact(() -> tm().insertAll(moreEntities)); + assertThrows( + IllegalArgumentException.class, () -> tm().transact(() -> tm().loadAll(TestEntity.class))); + } + private static void assertEntityExists(TestEntity entity) { assertThat(tm().transact(() -> tm().exists(entity))).isTrue(); } diff --git a/core/src/test/java/google/registry/server/Fixture.java b/core/src/test/java/google/registry/server/Fixture.java index de3c09fa5..e10629105 100644 --- a/core/src/test/java/google/registry/server/Fixture.java +++ b/core/src/test/java/google/registry/server/Fixture.java @@ -61,7 +61,7 @@ public enum Fixture { createTlds("xn--q9jyb4c", "example"); // Used for OT&E TLDs - persistPremiumList("default_sandbox_list"); + persistPremiumList("default_sandbox_list", "sandbox,USD 1000"); try { OteStatsTestHelper.setupCompleteOte("otefinished"); diff --git a/core/src/test/java/google/registry/testing/AbstractEppResourceSubject.java b/core/src/test/java/google/registry/testing/AbstractEppResourceSubject.java index eb5b0278c..07bf67230 100644 --- a/core/src/test/java/google/registry/testing/AbstractEppResourceSubject.java +++ b/core/src/test/java/google/registry/testing/AbstractEppResourceSubject.java @@ -49,7 +49,7 @@ abstract class AbstractEppResourceSubject< this.actual = subject; } - private List getHistoryEntries() { + private List getHistoryEntries() { return DatastoreHelper.getHistoryEntries(actual); } @@ -120,7 +120,7 @@ abstract class AbstractEppResourceSubject< // TODO(weiminyu): Remove after next Truth update @SuppressWarnings("UnnecessaryParentheses") public Which hasHistoryEntryAtIndex(int index) { - List historyEntries = getHistoryEntries(); + List historyEntries = getHistoryEntries(); check("getHistoryEntries().size()").that(historyEntries.size()).isAtLeast(index + 1); return new Which<>( check("getHistoryEntries(%s)", index) diff --git a/core/src/test/java/google/registry/testing/AppEngineExtension.java b/core/src/test/java/google/registry/testing/AppEngineExtension.java index 0f2d5f190..54f90cee1 100644 --- a/core/src/test/java/google/registry/testing/AppEngineExtension.java +++ b/core/src/test/java/google/registry/testing/AppEngineExtension.java @@ -17,6 +17,7 @@ package google.registry.testing; import static com.google.common.base.Preconditions.checkState; import static com.google.common.io.Files.asCharSink; import static com.google.common.truth.Truth.assertWithMessage; +import static google.registry.persistence.transaction.TransactionManagerUtil.ofyOrJpaTm; import static google.registry.testing.DatastoreHelper.persistSimpleResources; import static google.registry.testing.DualDatabaseTestInvocationContextProvider.injectTmForDualDatabaseTest; import static google.registry.testing.DualDatabaseTestInvocationContextProvider.restoreTmAfterDualDatabaseTest; @@ -384,6 +385,17 @@ public final class AppEngineExtension implements BeforeEachCallback, AfterEachCa if (isWithDatastoreAndCloudSql()) { injectTmForDualDatabaseTest(context); } + ofyOrJpaTm( + () -> { + if (withDatastore && !withoutCannedData) { + loadInitialData(); + } + }, + () -> { + if (withCloudSql && !withJpaUnitTest && !withoutCannedData) { + loadInitialData(); + } + }); } /** @@ -458,9 +470,6 @@ public final class AppEngineExtension implements BeforeEachCallback, AfterEachCa ObjectifyService.initOfy(); // Reset id allocation in ObjectifyService so that ids are deterministic in tests. ObjectifyService.resetNextTestId(); - if (!withoutCannedData) { - loadInitialData(); - } this.ofyTestEntities.forEach(AppEngineExtension::register); } } diff --git a/core/src/test/java/google/registry/testing/DatastoreHelper.java b/core/src/test/java/google/registry/testing/DatastoreHelper.java index c91ecb37f..2e55673af 100644 --- a/core/src/test/java/google/registry/testing/DatastoreHelper.java +++ b/core/src/test/java/google/registry/testing/DatastoreHelper.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Suppliers.memoize; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.Iterables.toArray; import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; @@ -33,6 +34,9 @@ import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registry.Registry.TldState.GENERAL_AVAILABILITY; import static google.registry.model.registry.label.PremiumListUtils.parentPremiumListEntriesOnRevision; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.ofyOrJpaTm; +import static google.registry.persistence.transaction.TransactionManagerUtil.ofyTmOrDoNothing; +import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm; import static google.registry.pricing.PricingEngineProxy.getDomainRenewCost; import static google.registry.util.CollectionUtils.difference; import static google.registry.util.CollectionUtils.union; @@ -44,6 +48,7 @@ import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import static google.registry.util.ResourceUtils.readResourceUtf8; import static java.util.Arrays.asList; import static org.joda.money.CurrencyUnit.USD; +import static org.junit.jupiter.api.Assertions.fail; import com.google.common.base.Ascii; import com.google.common.base.Splitter; @@ -60,22 +65,27 @@ import google.registry.dns.writer.VoidDnsWriter; import google.registry.model.Buildable; import google.registry.model.EppResource; import google.registry.model.EppResource.ForeignKeyedEppResource; -import google.registry.model.ImmutableObject; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Flag; import google.registry.model.billing.BillingEvent.Reason; import google.registry.model.contact.ContactAuthInfo; +import google.registry.model.contact.ContactBase; +import google.registry.model.contact.ContactHistory; import google.registry.model.contact.ContactResource; import google.registry.model.domain.DesignatedContact; import google.registry.model.domain.DesignatedContact.Type; import google.registry.model.domain.DomainAuthInfo; import google.registry.model.domain.DomainBase; +import google.registry.model.domain.DomainContent; +import google.registry.model.domain.DomainHistory; import google.registry.model.domain.GracePeriod; import google.registry.model.domain.rgp.GracePeriodStatus; import google.registry.model.domain.token.AllocationToken; import google.registry.model.eppcommon.AuthInfo.PasswordAuth; import google.registry.model.eppcommon.StatusValue; import google.registry.model.eppcommon.Trid; +import google.registry.model.host.HostBase; +import google.registry.model.host.HostHistory; import google.registry.model.host.HostResource; import google.registry.model.index.EppResourceIndex; import google.registry.model.index.EppResourceIndexBucket; @@ -101,10 +111,14 @@ import google.registry.persistence.VKey; import google.registry.tmch.LordnTaskUtils; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Set; import javax.annotation.Nullable; +import org.joda.money.CurrencyUnit; import org.joda.money.Money; import org.joda.time.DateTime; +import org.joda.time.DateTimeComparator; +import org.joda.time.DateTimeZone; /** Static utils for setting up test resources. */ public class DatastoreHelper { @@ -325,18 +339,40 @@ public class DatastoreHelper { * the requirement to have monotonically increasing timestamps. */ public static PremiumList persistPremiumList(String listName, String... lines) { - PremiumList premiumList = new PremiumList.Builder().setName(listName).build(); - ImmutableMap entries = premiumList.parse(asList(lines)); + checkState(lines.length != 0, "Must provide at least one premium entry"); + PremiumList partialPremiumList = new PremiumList.Builder().setName(listName).build(); + ImmutableMap entries = partialPremiumList.parse(asList(lines)); + CurrencyUnit currencyUnit = + entries.entrySet().iterator().next().getValue().getValue().getCurrencyUnit(); + PremiumList premiumList = + partialPremiumList + .asBuilder() + .setCreationTime(DateTime.now(DateTimeZone.UTC)) + .setCurrency(currencyUnit) + .setLabelsToPrices( + entries.entrySet().stream() + .collect( + toImmutableMap( + Map.Entry::getKey, entry -> entry.getValue().getValue().getAmount()))) + .build(); PremiumListRevision revision = PremiumListRevision.create(premiumList, entries.keySet()); - ofy() - .saveWithoutBackup() - .entities(premiumList.asBuilder().setRevision(Key.create(revision)).build(), revision) - .now(); - ofy() - .saveWithoutBackup() - .entities(parentPremiumListEntriesOnRevision(entries.values(), Key.create(revision))) - .now(); - return ofy().load().entity(premiumList).now(); + + ofyOrJpaTm( + () -> { + tm().putAllWithoutBackup( + ImmutableList.of( + premiumList.asBuilder().setRevision(Key.create(revision)).build(), revision)); + tm().putAllWithoutBackup( + parentPremiumListEntriesOnRevision(entries.values(), Key.create(revision))); + }, + () -> tm().transact(() -> tm().insert(premiumList))); + // The above premiumList is in the session cache and it is different from the corresponding + // entity stored in Datastore because it has some @Ignore fields set dedicated for SQL. This + // breaks the assumption we have in our application code, see + // PremiumListUtils.savePremiumListAndEntries(). Clearing the session cache can help make sure + // we always get the same list. + tm().clearSessionCache(); + return transactIfJpaTm(() -> tm().load(premiumList)); } /** Creates and persists a tld. */ @@ -672,17 +708,28 @@ public class DatastoreHelper { } private static Iterable getBillingEvents() { - return Iterables.concat( - ofy().load().type(BillingEvent.OneTime.class), - ofy().load().type(BillingEvent.Recurring.class), - ofy().load().type(BillingEvent.Cancellation.class)); + return transactIfJpaTm( + () -> + Iterables.concat( + tm().loadAll(BillingEvent.OneTime.class), + tm().loadAll(BillingEvent.Recurring.class), + tm().loadAll(BillingEvent.Cancellation.class))); } private static Iterable getBillingEvents(EppResource resource) { - return Iterables.concat( - ofy().load().type(BillingEvent.OneTime.class).ancestor(resource), - ofy().load().type(BillingEvent.Recurring.class).ancestor(resource), - ofy().load().type(BillingEvent.Cancellation.class).ancestor(resource)); + return transactIfJpaTm( + () -> + Iterables.concat( + tm().loadAll(BillingEvent.OneTime.class).stream() + .filter(oneTime -> oneTime.getDomainRepoId().equals(resource.getRepoId())) + .collect(toImmutableList()), + tm().loadAll(BillingEvent.Recurring.class).stream() + .filter(recurring -> recurring.getDomainRepoId().equals(resource.getRepoId())) + .collect(toImmutableList()), + tm().loadAll(BillingEvent.Cancellation.class).stream() + .filter( + cancellation -> cancellation.getDomainRepoId().equals(resource.getRepoId())) + .collect(toImmutableList()))); } /** Assert that the actual billing event matches the expected one, ignoring IDs. */ @@ -751,41 +798,63 @@ public class DatastoreHelper { assertPollMessagesEqual(getPollMessages(), Arrays.asList(expected)); } - public static void assertPollMessagesForResource(EppResource resource, PollMessage... expected) { - assertPollMessagesEqual(getPollMessages(resource), Arrays.asList(expected)); + public static void assertPollMessagesForResource(DomainContent domain, PollMessage... expected) { + assertPollMessagesEqual(getPollMessages(domain), Arrays.asList(expected)); } public static ImmutableList getPollMessages() { - return ImmutableList.copyOf(ofy().load().type(PollMessage.class)); + return ImmutableList.copyOf(transactIfJpaTm(() -> tm().loadAll(PollMessage.class))); } public static ImmutableList getPollMessages(String clientId) { - return ImmutableList.copyOf(ofy().load().type(PollMessage.class).filter("clientId", clientId)); + return transactIfJpaTm( + () -> + tm().loadAll(PollMessage.class).stream() + .filter(pollMessage -> pollMessage.getClientId().equals(clientId)) + .collect(toImmutableList())); } - public static ImmutableList getPollMessages(EppResource resource) { - return ImmutableList.copyOf(ofy().load().type(PollMessage.class).ancestor(resource)); + public static ImmutableList getPollMessages(DomainContent domain) { + return transactIfJpaTm( + () -> + tm().loadAll(PollMessage.class).stream() + .filter( + pollMessage -> + pollMessage.getParentKey().getParent().getName().equals(domain.getRepoId())) + .collect(toImmutableList())); } public static ImmutableList getPollMessages(String clientId, DateTime now) { - return ImmutableList.copyOf( - ofy() - .load() - .type(PollMessage.class) - .filter("clientId", clientId) - .filter("eventTime <=", now.toDate())); + return transactIfJpaTm( + () -> + tm().loadAll(PollMessage.class).stream() + .filter(pollMessage -> pollMessage.getClientId().equals(clientId)) + .filter( + pollMessage -> + pollMessage.getEventTime().isEqual(now) + || pollMessage.getEventTime().isBefore(now)) + .collect(toImmutableList())); } /** Gets all PollMessages associated with the given EppResource. */ public static ImmutableList getPollMessages( EppResource resource, String clientId, DateTime now) { - return ImmutableList.copyOf( - ofy() - .load() - .type(PollMessage.class) - .ancestor(resource) - .filter("clientId", clientId) - .filter("eventTime <=", now.toDate())); + return transactIfJpaTm( + () -> + tm().loadAll(PollMessage.class).stream() + .filter( + pollMessage -> + pollMessage + .getParentKey() + .getParent() + .getName() + .equals(resource.getRepoId())) + .filter(pollMessage -> pollMessage.getClientId().equals(clientId)) + .filter( + pollMessage -> + pollMessage.getEventTime().isEqual(now) + || pollMessage.getEventTime().isBefore(now)) + .collect(toImmutableList())); } public static PollMessage getOnlyPollMessage(String clientId) { @@ -808,19 +877,15 @@ public class DatastoreHelper { } public static PollMessage getOnlyPollMessage( - EppResource resource, - String clientId, - DateTime now, - Class subType) { - return getPollMessages(resource, clientId, now) - .stream() + DomainContent domain, String clientId, DateTime now, Class subType) { + return getPollMessages(domain, clientId, now).stream() .filter(subType::isInstance) .map(subType::cast) .collect(onlyElement()); } public static void assertAllocationTokens(AllocationToken... expectedTokens) { - assertThat(ofy().load().type(AllocationToken.class).list()) + assertThat(transactIfJpaTm(() -> tm().loadAll(AllocationToken.class))) .comparingElementsUsing(immutableObjectCorrespondence("updateTimestamp", "creationTime")) .containsExactlyElementsIn(expectedTokens); } @@ -861,13 +926,17 @@ public class DatastoreHelper { } private static void saveResource(R resource, boolean wantBackup) { - Saver saver = wantBackup ? ofy().save() : ofy().saveWithoutBackup(); - saver.entity(resource); - if (resource instanceof EppResource) { - EppResource eppResource = (EppResource) resource; - persistEppResourceExtras( - eppResource, EppResourceIndex.create(Key.create(eppResource)), saver); - } + ofyOrJpaTm( + () -> { + Saver saver = wantBackup ? ofy().save() : ofy().saveWithoutBackup(); + saver.entity(resource); + if (resource instanceof EppResource) { + EppResource eppResource = (EppResource) resource; + persistEppResourceExtras( + eppResource, EppResourceIndex.create(Key.create(eppResource)), saver); + } + }, + () -> tm().put(resource)); } private static void persistEppResourceExtras( @@ -890,23 +959,25 @@ public class DatastoreHelper { // 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. - ofy().clearSessionCache(); - return ofy().load().entity(resource).now(); + tm().clearSessionCache(); + return transactIfJpaTm(() -> tm().load(resource)); } /** Persists an EPP resource with the {@link EppResourceIndex} always going into bucket one. */ public static R persistEppResourceInFirstBucket(final R resource) { final EppResourceIndex eppResourceIndex = EppResourceIndex.create(Key.create(EppResourceIndexBucket.class, 1), Key.create(resource)); - tm() - .transact( - () -> { - Saver saver = ofy().save(); - saver.entity(resource); - persistEppResourceExtras(resource, eppResourceIndex, saver); - }); - ofy().clearSessionCache(); - return ofy().load().entity(resource).now(); + tm().transact( + () -> + ofyOrJpaTm( + () -> { + Saver saver = ofy().save(); + saver.entity(resource); + persistEppResourceExtras(resource, eppResourceIndex, saver); + }, + () -> tm().put(resource))); + tm().clearSessionCache(); + return transactIfJpaTm(() -> tm().load(resource)); } public static void persistResources(final Iterable resources) { @@ -925,9 +996,9 @@ public class DatastoreHelper { } // 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. - ofy().clearSessionCache(); + tm().clearSessionCache(); for (R resource : resources) { - ofy().load().entity(resource).now(); + transactIfJpaTm(() -> tm().load(resource)); } } @@ -943,31 +1014,57 @@ public class DatastoreHelper { */ public static R persistEppResource(final R resource) { checkState(!tm().inTransaction()); - tm() - .transact( + tm().transact( () -> { - ofy() - .save() - .entities( - resource, + tm().put(resource); + tm().put( new HistoryEntry.Builder() .setParent(resource) .setType(getHistoryEntryType(resource)) .setModificationTime(tm().getTransactionTime()) - .build()); - ofy().save().entity(ForeignKeyIndex.create(resource, resource.getDeletionTime())); + .build() + .toChildHistoryEntity()); + ofyTmOrDoNothing( + () -> tm().put(ForeignKeyIndex.create(resource, resource.getDeletionTime()))); }); - ofy().clearSessionCache(); - return ofy().load().entity(resource).safe(); + tm().clearSessionCache(); + return transactIfJpaTm(() -> tm().load(resource)); } /** Returns all of the history entries that are parented off the given EppResource. */ - public static List getHistoryEntries(EppResource resource) { - return ofy().load() - .type(HistoryEntry.class) - .ancestor(resource) - .order("modificationTime") - .list(); + public static List getHistoryEntries(EppResource resource) { + return ofyOrJpaTm( + () -> + ofy() + .load() + .type(HistoryEntry.class) + .ancestor(resource) + .order("modificationTime") + .list(), + () -> + tm().transact( + () -> { + ImmutableList unsorted = null; + if (resource instanceof ContactBase) { + unsorted = tm().loadAll(ContactHistory.class); + } else if (resource instanceof HostBase) { + unsorted = tm().loadAll(HostHistory.class); + } else if (resource instanceof DomainContent) { + unsorted = tm().loadAll(DomainHistory.class); + } else { + fail("Expected an EppResource instance, but got " + resource.getClass()); + } + ImmutableList filtered = + unsorted.stream() + .filter( + historyEntry -> + historyEntry + .getParent() + .getName() + .equals(resource.getRepoId())) + .collect(toImmutableList()); + return ImmutableList.sortedCopyOf(DateTimeComparator.getInstance(), filtered); + })); } /** @@ -1009,9 +1106,13 @@ public class DatastoreHelper { } public static PollMessage getOnlyPollMessageForHistoryEntry(HistoryEntry historyEntry) { - return Iterables.getOnlyElement(ofy().load() - .type(PollMessage.class) - .ancestor(historyEntry)); + return Iterables.getOnlyElement( + transactIfJpaTm( + () -> + tm().loadAll(PollMessage.class).stream() + .filter( + pollMessage -> pollMessage.getParentKey().equals(Key.create(historyEntry))) + .collect(toImmutableList()))); } public static HistoryEntry createHistoryEntryForEppResource( @@ -1029,23 +1130,30 @@ public class DatastoreHelper { * ForeignKeyedEppResources. */ public static ImmutableList persistSimpleResources(final Iterable resources) { - tm().transact(() -> ofy().saveWithoutBackup().entities(resources)); + tm().transact(() -> tm().putAllWithoutBackup(ImmutableList.copyOf(resources))); // 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. - ofy().clearSessionCache(); - return ImmutableList.copyOf(ofy().load().entities(resources).values()); + tm().clearSessionCache(); + return transactIfJpaTm(() -> tm().loadAll(resources)); } public static void deleteResource(final Object resource) { - ofy().deleteWithoutBackup().entity(resource).now(); + transactIfJpaTm(() -> tm().deleteWithoutBackup(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. - ofy().clearSessionCache(); + tm().clearSessionCache(); } /** Force the create and update timestamps to get written into the resource. **/ public static R cloneAndSetAutoTimestamps(final R resource) { - return tm().transact(() -> ofy().load().fromEntity(ofy().save().toEntity(resource))); + return tm().transact( + () -> + ofyOrJpaTm( + () -> ofy().load().fromEntity(ofy().save().toEntity(resource)), + () -> { + tm().put(resource); + return tm().load(resource); + })); } /** Returns the entire map of {@link PremiumListEntry}s for the given {@link PremiumList}. */ diff --git a/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProvider.java b/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProvider.java index c7db28812..fecde15f4 100644 --- a/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProvider.java +++ b/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProvider.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import google.registry.persistence.transaction.TransactionManager; import google.registry.persistence.transaction.TransactionManagerFactory; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.util.List; import java.util.function.Supplier; import java.util.stream.Stream; @@ -52,9 +53,21 @@ class DualDatabaseTestInvocationContextProvider implements TestTemplateInvocatio @Override public Stream provideTestTemplateInvocationContexts( ExtensionContext context) { - return Stream.of( - createInvocationContext("Test Datastore", TransactionManagerFactory::ofyTm), - createInvocationContext("Test PostgreSQL", TransactionManagerFactory::jpaTm)); + TestTemplateInvocationContext ofyContext = + createInvocationContext("Test Datastore", TransactionManagerFactory::ofyTm); + TestTemplateInvocationContext sqlContext = + createInvocationContext("Test PostgreSQL", TransactionManagerFactory::jpaTm); + Method testMethod = context.getTestMethod().orElseThrow(IllegalStateException::new); + if (testMethod.isAnnotationPresent(TestOfyAndSql.class)) { + return Stream.of(ofyContext, sqlContext); + } else if (testMethod.isAnnotationPresent(TestOfyOnly.class)) { + return Stream.of(ofyContext); + } else if (testMethod.isAnnotationPresent(TestSqlOnly.class)) { + return Stream.of(sqlContext); + } else { + throw new IllegalStateException( + "Test method must be annotated with @TestOfyAndSql, @TestOfyOnly or @TestSqlOnly"); + } } private TestTemplateInvocationContext createInvocationContext( @@ -104,6 +117,18 @@ class DualDatabaseTestInvocationContextProvider implements TestTemplateInvocatio static void injectTmForDualDatabaseTest(ExtensionContext context) { if (isDualDatabaseTest(context)) { + context + .getTestMethod() + .ifPresent( + testMethod -> { + if (!testMethod.isAnnotationPresent(TestOfyAndSql.class) + && !testMethod.isAnnotationPresent(TestOfyOnly.class) + && !testMethod.isAnnotationPresent(TestSqlOnly.class)) { + throw new IllegalStateException( + "Test method must be annotated with @TestOfyAndSql, @TestOfyOnly or" + + " @TestSqlOnly"); + } + }); context.getStore(NAMESPACE).put(ORIGINAL_TM_KEY, tm()); Supplier tmSupplier = (Supplier) diff --git a/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProviderTest.java b/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProviderTest.java index ef71f8853..3e037a507 100644 --- a/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProviderTest.java +++ b/core/src/test/java/google/registry/testing/DualDatabaseTestInvocationContextProviderTest.java @@ -19,37 +19,67 @@ import static google.registry.persistence.transaction.TransactionManagerFactory. import google.registry.model.ofy.DatastoreTransactionManager; import google.registry.persistence.transaction.JpaTransactionManager; +import google.registry.persistence.transaction.TransactionManager; import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.RegisterExtension; /** - * Test to verify that {@link DualDatabaseTestInvocationContextProvider} extension executes {@link - * TestTemplate} test twice with different databases. + * Test to verify that {@link DualDatabaseTestInvocationContextProvider} extension executes tests + * with corresponding {@link TransactionManager}. */ @DualDatabaseTest public class DualDatabaseTestInvocationContextProviderTest { - private static int datastoreTestCounter = 0; - private static int postgresqlTestCounter = 0; + private static int testBothDbsOfyCounter = 0; + private static int testBothDbsSqlCounter = 0; + private static int testOfyOnlyOfyCounter = 0; + private static int testOfyOnlySqlCounter = 0; + private static int testSqlOnlyOfyCounter = 0; + private static int testSqlOnlySqlCounter = 0; @RegisterExtension public final AppEngineExtension appEngine = AppEngineExtension.builder().withDatastoreAndCloudSql().build(); - @TestTemplate - void testToUseTransactionManager() { + @TestOfyAndSql + void testToVerifyBothOfyAndSqlTmAreUsed() { if (tm() instanceof DatastoreTransactionManager) { - datastoreTestCounter++; + testBothDbsOfyCounter++; } if (tm() instanceof JpaTransactionManager) { - postgresqlTestCounter++; + testBothDbsSqlCounter++; + } + } + + @TestOfyOnly + void testToVerifyOnlyOfyTmIsUsed() { + if (tm() instanceof DatastoreTransactionManager) { + testOfyOnlyOfyCounter++; + } + if (tm() instanceof JpaTransactionManager) { + testOfyOnlySqlCounter++; + } + } + + @TestSqlOnly + void testToVerifyOnlySqlTmIsUsed() { + if (tm() instanceof DatastoreTransactionManager) { + testSqlOnlyOfyCounter++; + } + if (tm() instanceof JpaTransactionManager) { + testSqlOnlySqlCounter++; } } @AfterAll static void assertEachTransactionManagerIsUsed() { - assertThat(datastoreTestCounter).isEqualTo(1); - assertThat(postgresqlTestCounter).isEqualTo(1); + assertThat(testBothDbsOfyCounter).isEqualTo(1); + assertThat(testBothDbsSqlCounter).isEqualTo(1); + + assertThat(testOfyOnlyOfyCounter).isEqualTo(1); + assertThat(testOfyOnlySqlCounter).isEqualTo(0); + + assertThat(testSqlOnlyOfyCounter).isEqualTo(0); + assertThat(testSqlOnlySqlCounter).isEqualTo(1); } } diff --git a/core/src/test/java/google/registry/testing/TestOfyAndSql.java b/core/src/test/java/google/registry/testing/TestOfyAndSql.java new file mode 100644 index 000000000..713ce789d --- /dev/null +++ b/core/src/test/java/google/registry/testing/TestOfyAndSql.java @@ -0,0 +1,31 @@ +// Copyright 2020 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. + +package google.registry.testing; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import org.junit.jupiter.api.TestTemplate; + +/** + * Annotation to indicate a test method will be executed twice with Datastore and Postgresql + * respectively. + */ +@Target({METHOD}) +@Retention(RUNTIME) +@TestTemplate +public @interface TestOfyAndSql {} diff --git a/core/src/test/java/google/registry/testing/TestOfyOnly.java b/core/src/test/java/google/registry/testing/TestOfyOnly.java new file mode 100644 index 000000000..7a74cc5d3 --- /dev/null +++ b/core/src/test/java/google/registry/testing/TestOfyOnly.java @@ -0,0 +1,28 @@ +// Copyright 2020 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. + +package google.registry.testing; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import org.junit.jupiter.api.TestTemplate; + +/** Annotation to indicate a test method will be executed only with Datastore. */ +@Target({METHOD}) +@Retention(RUNTIME) +@TestTemplate +public @interface TestOfyOnly {} diff --git a/core/src/test/java/google/registry/testing/TestSqlOnly.java b/core/src/test/java/google/registry/testing/TestSqlOnly.java new file mode 100644 index 000000000..fa6861732 --- /dev/null +++ b/core/src/test/java/google/registry/testing/TestSqlOnly.java @@ -0,0 +1,28 @@ +// Copyright 2020 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. + +package google.registry.testing; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import org.junit.jupiter.api.TestTemplate; + +/** Annotation to indicate a test method will be executed only with Postgresql. */ +@Target({METHOD}) +@Retention(RUNTIME) +@TestTemplate +public @interface TestSqlOnly {} diff --git a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html index 2e4b1fe7f..c1a55fe28 100644 --- a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html @@ -261,11 +261,11 @@ td.section { generated on - 2020-10-26 15:49:09.291097 + 2020-10-26 18:19:17.415062 last flyway file - V67__grace_period_history_ids.sql + V68__make_reserved_list_nullable_in_registry.sql @@ -284,7 +284,7 @@ td.section { generated on - 2020-10-26 15:49:09.291097 + 2020-10-26 18:19:17.415062 diff --git a/db/src/main/resources/sql/er_diagram/full_er_diagram.html b/db/src/main/resources/sql/er_diagram/full_er_diagram.html index 12a697db3..403a48622 100644 --- a/db/src/main/resources/sql/er_diagram/full_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/full_er_diagram.html @@ -261,11 +261,11 @@ td.section { generated on - 2020-10-26 15:49:07.386754 + 2020-10-26 18:19:15.617359 last flyway file - V67__grace_period_history_ids.sql + V68__make_reserved_list_nullable_in_registry.sql @@ -284,7 +284,7 @@ td.section { generated on - 2020-10-26 15:49:07.386754 + 2020-10-26 18:19:15.617359 @@ -5600,7 +5600,7 @@ td.section { - _text not null + _text restore_billing_cost_amount @@ -11740,7 +11740,7 @@ td.section { reserved_list_names - _text not null + _text diff --git a/db/src/main/resources/sql/flyway.txt b/db/src/main/resources/sql/flyway.txt index 290df0ff6..ef6b1d762 100644 --- a/db/src/main/resources/sql/flyway.txt +++ b/db/src/main/resources/sql/flyway.txt @@ -65,3 +65,4 @@ V64__transfer_history_columns.sql V65__local_date_date_type.sql V66__create_rde_revision.sql V67__grace_period_history_ids.sql +V68__make_reserved_list_nullable_in_registry.sql diff --git a/db/src/main/resources/sql/flyway/V68__make_reserved_list_nullable_in_registry.sql b/db/src/main/resources/sql/flyway/V68__make_reserved_list_nullable_in_registry.sql new file mode 100644 index 000000000..9d14808d1 --- /dev/null +++ b/db/src/main/resources/sql/flyway/V68__make_reserved_list_nullable_in_registry.sql @@ -0,0 +1,15 @@ +-- Copyright 2020 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. + +ALTER TABLE "Tld" ALTER COLUMN reserved_list_names DROP NOT NULL; diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index d46cd81f5..4b6771eff 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -646,7 +646,7 @@ registry_lock_or_unlock_cost_currency text, renew_billing_cost_transitions hstore not null, renew_grace_period_length interval not null, - reserved_list_names text[] not null, + reserved_list_names text[], restore_billing_cost_amount numeric(19, 2), restore_billing_cost_currency text, roid_suffix text, diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index aa4def939..7e2286bcd 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/db/src/main/resources/sql/schema/nomulus.golden.sql @@ -904,7 +904,7 @@ CREATE TABLE public."Tld" ( registry_lock_or_unlock_cost_currency text, renew_billing_cost_transitions public.hstore NOT NULL, renew_grace_period_length interval NOT NULL, - reserved_list_names text[] NOT NULL, + reserved_list_names text[], restore_billing_cost_amount numeric(19,2), restore_billing_cost_currency text, roid_suffix text,