diff --git a/core/src/main/java/google/registry/model/domain/DomainContent.java b/core/src/main/java/google/registry/model/domain/DomainContent.java index fb3748613..a038d433f 100644 --- a/core/src/main/java/google/registry/model/domain/DomainContent.java +++ b/core/src/main/java/google/registry/model/domain/DomainContent.java @@ -1035,17 +1035,20 @@ public class DomainContent extends EppResource public B setGracePeriods(ImmutableSet gracePeriods) { getInstance().gracePeriods = gracePeriods; + getInstance().resetUpdateTimestamp(); return thisCastToDerived(); } public B addGracePeriod(GracePeriod gracePeriod) { getInstance().gracePeriods = union(getInstance().getGracePeriods(), gracePeriod); + getInstance().resetUpdateTimestamp(); return thisCastToDerived(); } public B removeGracePeriod(GracePeriod gracePeriod) { getInstance().gracePeriods = CollectionUtils.difference(getInstance().getGracePeriods(), gracePeriod); + getInstance().resetUpdateTimestamp(); return thisCastToDerived(); } diff --git a/core/src/main/java/google/registry/model/ofy/CommitLoggedWork.java b/core/src/main/java/google/registry/model/ofy/CommitLoggedWork.java index d3efc043a..e3a1e1319 100644 --- a/core/src/main/java/google/registry/model/ofy/CommitLoggedWork.java +++ b/core/src/main/java/google/registry/model/ofy/CommitLoggedWork.java @@ -41,7 +41,7 @@ import org.joda.time.DateTime; /** Wrapper for {@link Supplier} that associates a time with each attempt. */ @DeleteAfterMigration -class CommitLoggedWork implements Runnable { +public class CommitLoggedWork implements Runnable { private final Supplier work; private final Clock clock; 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 75cf0dd33..b85ceceff 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -46,6 +46,7 @@ import static google.registry.util.X509Utils.loadCertificate; import static java.util.Comparator.comparing; import static java.util.function.Predicate.isEqual; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; @@ -824,6 +825,7 @@ public class Registrar extends ImmutableObject public Builder setAllowedTlds(Set allowedTlds) { getInstance().allowedTlds = ImmutableSortedSet.copyOf(assertTldsExist(allowedTlds)); + getInstance().lastUpdateTime = UpdateAutoTimestamp.create(null); return this; } @@ -991,6 +993,16 @@ public class Registrar extends ImmutableObject return this; } + /** + * This lets tests set the update timestamp in cases where setting fields resets the timestamp + * and breaks the verification that an object has not been updated since it was copied. + */ + @VisibleForTesting + public Builder setLastUpdateTime(DateTime timestamp) { + getInstance().lastUpdateTime = UpdateAutoTimestamp.create(timestamp); + return this; + } + /** Build the registrar, nullifying empty fields. */ @Override public Registrar build() { 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 6463a90d5..24c39d537 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -30,6 +30,7 @@ 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 com.googlecode.objectify.Key; import google.registry.model.ImmutableObject; import google.registry.model.common.DatabaseMigrationStateSchedule; import google.registry.model.common.DatabaseMigrationStateSchedule.ReplayDirection; @@ -604,6 +605,13 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { managedEntity = getEntityManager().merge(entity); } getEntityManager().remove(managedEntity); + + // We check shouldReplicate() in TransactionInfo.addDelete(), but we have to check it here as + // well prior to attempting to create a datastore key because a non-replicated entity may not + // have one. + if (shouldReplicate(entity.getClass())) { + transactionInfo.get().addDelete(VKey.from(Key.create(entity))); + } return managedEntity; } @@ -827,6 +835,12 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { replaySqlToDatastoreOverrideForTest.set(Optional.empty()); } + /** Returns true if the entity class should be replicated from SQL to datastore. */ + private static boolean shouldReplicate(Class entityClass) { + return !NonReplicatedEntity.class.isAssignableFrom(entityClass) + && !SqlOnlyEntity.class.isAssignableFrom(entityClass); + } + private static class TransactionInfo { ReadOnlyCheckingEntityManager entityManager; boolean inTransaction = false; @@ -883,12 +897,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } } - /** Returns true if the entity class should be replicated from SQL to datastore. */ - private boolean shouldReplicate(Class entityClass) { - return !NonReplicatedEntity.class.isAssignableFrom(entityClass) - && !SqlOnlyEntity.class.isAssignableFrom(entityClass); - } - private void recordTransaction() { if (contentsBuilder != null) { Transaction persistedTxn = contentsBuilder.build(); 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 4b962e14e..33d4cc4c5 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 @@ -453,6 +453,13 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA Map diffs = DiffUtils.deepDiff( originalRegistrar.toDiffableFieldMap(), updatedRegistrar.toDiffableFieldMap(), true); + + // It's expected that the update timestamp will be changed, as it gets reset whenever we change + // nested collections. If it's the only change, just return the original registrar. + if (diffs.keySet().equals(ImmutableSet.of("lastUpdateTime"))) { + return originalRegistrar; + } + throw new ForbiddenException( String.format("Unauthorized: only %s can change fields %s", allowedRole, diffs.keySet())); } diff --git a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java index 97744b53c..3738c7558 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -553,6 +553,7 @@ class DomainCreateFlowTest extends ResourceFlowTestCase> ALL_JPA_ENTITIES = + public static final ImmutableSet> ALL_JPA_ENTITIES = PersistenceXmlUtility.getManagedClasses().stream() .filter(e -> !IGNORE_ENTITIES.contains(e.getSimpleName())) .filter(e -> e.isAnnotationPresent(Entity.class)) 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 7e04ca37c..f79c4fc91 100644 --- a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java @@ -30,6 +30,7 @@ import com.googlecode.objectify.annotation.Id; import google.registry.model.ImmutableObject; import google.registry.model.ofy.DatastoreTransactionManager; import google.registry.model.ofy.Ofy; +import google.registry.model.replay.NonReplicatedEntity; import google.registry.persistence.VKey; import google.registry.persistence.transaction.TransactionManagerFactory.ReadOnlyModeException; import google.registry.testing.AppEngineExtension; @@ -448,7 +449,7 @@ public class TransactionManagerTest { @Entity(name = "TxnMgrTestEntity") @javax.persistence.Entity(name = "TestEntity") - private static class TestEntity extends TestEntityBase { + private static class TestEntity extends TestEntityBase implements NonReplicatedEntity { private String data; diff --git a/core/src/test/java/google/registry/testing/ReplayExtension.java b/core/src/test/java/google/registry/testing/ReplayExtension.java index af090a1fa..53aeb8cbb 100644 --- a/core/src/test/java/google/registry/testing/ReplayExtension.java +++ b/core/src/test/java/google/registry/testing/ReplayExtension.java @@ -14,17 +14,25 @@ package google.registry.testing; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; +import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; +import static org.junit.Assert.fail; import com.google.common.base.Ascii; import com.google.common.base.Suppliers; 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 com.googlecode.objectify.Key; +import google.registry.model.EntityClasses; import google.registry.model.ImmutableObject; import google.registry.model.domain.DomainBase; import google.registry.model.ofy.CommitLogBucket; @@ -34,6 +42,7 @@ import google.registry.model.replay.DatastoreEntity; import google.registry.model.replay.ReplicateToDatastoreAction; import google.registry.model.replay.SqlEntity; import google.registry.persistence.VKey; +import google.registry.persistence.transaction.JpaEntityCoverageExtension; import google.registry.persistence.transaction.JpaTransactionManagerImpl; import google.registry.persistence.transaction.Transaction; import google.registry.persistence.transaction.Transaction.Delete; @@ -42,10 +51,15 @@ import google.registry.persistence.transaction.Transaction.Update; import google.registry.persistence.transaction.TransactionEntity; import google.registry.util.RequestStatusChecker; import java.io.IOException; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Optional; import javax.annotation.Nullable; +import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.AfterEachCallback; import org.junit.jupiter.api.extension.BeforeEachCallback; import org.junit.jupiter.api.extension.ExtensionContext; @@ -58,30 +72,74 @@ import org.mockito.Mockito; * that extension are also replayed. If AppEngineExtension is not used, * JpaTransactionManagerExtension must be, and this extension should be ordered _after_ * JpaTransactionManagerExtension so that writes to SQL work. - * - *

If the "compare" flag is set in the constructor, this will also compare all touched objects in - * both databases after performing the replay. */ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private static ImmutableSet NON_REPLICATED_TYPES = + ImmutableSet.of( + "PremiumList", + "PremiumListRevision", + "PremiumListEntry", + "ReservedList", + "RdeRevision", + "ServerSecret", + "SignedMarkRevocationList", + "ClaimsListShard", + "TmchCrl", + "EppResourceIndex", + "ForeignKeyIndex", + "ForeignKeyHostIndex", + "ForeignKeyContactIndex", + "ForeignKeyDomainIndex"); + + // Entity classes to be ignored during the final database comparison. Note that this is just a + // mash-up of Datastore and SQL classes, and used for filtering both sets. We could split them + // out, but there is plenty of overlap and no name collisions so it doesn't matter very much. + private static ImmutableSet IGNORED_ENTITIES = + Streams.concat( + ImmutableSet.of( + // These entities *should* be comparable, but this isn't working yet so exclude + // them so we can tackle them independently. + "GracePeriod", + "GracePeriodHistory", + "HistoryEntry", + "DomainHistory", + "ContactHistory", + "HostHistory", + "DomainDsDataHistory", + "DelegationSignerData", + "DomainTransactionRecord", + + // These entities are legitimately not comparable. + "ClaimsEntry", + "ClaimsList", + "CommitLogBucket", + "CommitLogManifest", + "CommitLogMutation", + "PremiumEntry", + "ReservedListEntry") + .stream(), + NON_REPLICATED_TYPES.stream()) + .collect(toImmutableSet()); + FakeClock clock; - boolean compare; boolean replayed = false; boolean inOfyContext; InjectExtension injectExtension = new InjectExtension(); @Nullable ReplicateToDatastoreAction sqlToDsReplicator; List expectedUpdates = new ArrayList<>(); boolean enableDomainTimestampChecks; + boolean enableDatabaseCompare = true; - private ReplayExtension( - FakeClock clock, boolean compare, @Nullable ReplicateToDatastoreAction sqlToDsReplicator) { + private ReplayExtension(FakeClock clock, @Nullable ReplicateToDatastoreAction sqlToDsReplicator) { this.clock = clock; - this.compare = compare; this.sqlToDsReplicator = sqlToDsReplicator; } public static ReplayExtension createWithCompare(FakeClock clock) { - return new ReplayExtension(clock, true, null); + return new ReplayExtension(clock, null); } // This allows us to disable the replay tests from an environment variable in specific @@ -104,7 +162,6 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { if (replayTestsEnabled()) { return new ReplayExtension( clock, - true, new ReplicateToDatastoreAction( clock, Mockito.mock(RequestStatusChecker.class), new FakeResponse())); } else { @@ -138,6 +195,11 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { @Override public void beforeEach(ExtensionContext context) { + Optional elem = context.getTestMethod(); + if (elem.isPresent() && elem.get().isAnnotationPresent(NoDatabaseCompare.class)) { + enableDatabaseCompare = false; + } + // Use a single bucket to expose timestamp inversion problems. This typically happens when // a test with this extension rolls back the fake clock in the setup method, creating inverted // timestamp with the canned data preloaded by AppengineExtension. The solution is to move @@ -170,23 +232,6 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { } } - private static ImmutableSet NON_REPLICATED_TYPES = - ImmutableSet.of( - "PremiumList", - "PremiumListRevision", - "PremiumListEntry", - "ReservedList", - "RdeRevision", - "ServerSecret", - "SignedMarkRevocationList", - "ClaimsListShard", - "TmchCrl", - "EppResourceIndex", - "ForeignKeyIndex", - "ForeignKeyHostIndex", - "ForeignKeyContactIndex", - "ForeignKeyDomainIndex"); - public void replay() { if (!replayed) { if (inOfyContext) { @@ -211,34 +256,32 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { ImmutableMap, Object> changes = ReplayQueue.replay(); // Compare JPA to OFY, if requested. - if (compare) { - for (ImmutableMap.Entry, Object> entry : changes.entrySet()) { - // Don't verify non-replicated types. - if (NON_REPLICATED_TYPES.contains(entry.getKey().getKind())) { - continue; - } - - // Since the object may have changed in datastore by the time we're doing the replay, we - // have to compare the current value in SQL (which we just mutated) against the value that - // we originally would have persisted (that being the object in the entry). - VKey vkey = VKey.from(entry.getKey()); - jpaTm() - .transact( - () -> { - Optional jpaValue = jpaTm().loadByKeyIfPresent(vkey); - if (entry.getValue().equals(TransactionInfo.Delete.SENTINEL)) { - assertThat(jpaValue.isPresent()).isFalse(); - } else { - ImmutableObject immutJpaObject = (ImmutableObject) jpaValue.get(); - assertAboutImmutableObjects().that(immutJpaObject).hasCorrectHashValue(); - assertAboutImmutableObjects() - .that(immutJpaObject) - .isEqualAcrossDatabases( - (ImmutableObject) - ((DatastoreEntity) entry.getValue()).toSqlEntity().get()); - } - }); + for (ImmutableMap.Entry, Object> entry : changes.entrySet()) { + // Don't verify non-replicated types. + if (NON_REPLICATED_TYPES.contains(entry.getKey().getKind())) { + continue; } + + // Since the object may have changed in datastore by the time we're doing the replay, we + // have to compare the current value in SQL (which we just mutated) against the value that + // we originally would have persisted (that being the object in the entry). + VKey vkey = VKey.from(entry.getKey()); + jpaTm() + .transact( + () -> { + Optional jpaValue = jpaTm().loadByKeyIfPresent(vkey); + if (entry.getValue().equals(TransactionInfo.Delete.SENTINEL)) { + assertThat(jpaValue.isPresent()).isFalse(); + } else { + ImmutableObject immutJpaObject = (ImmutableObject) jpaValue.get(); + assertAboutImmutableObjects().that(immutJpaObject).hasCorrectHashValue(); + assertAboutImmutableObjects() + .that(immutJpaObject) + .isEqualAcrossDatabases( + (ImmutableObject) + ((DatastoreEntity) entry.getValue()).toSqlEntity().get()); + } + }); } } @@ -252,12 +295,15 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { transactionBatch = sqlToDsReplicator.getTransactionBatchAtSnapshot(); for (TransactionEntity txn : transactionBatch) { ReplicateToDatastoreAction.applyTransaction(txn); - if (compare) { - ofyTm().transact(() -> compareSqlTransaction(txn)); - } + ofyTm().transact(() -> compareSqlTransaction(txn)); clock.advanceOneMilli(); } } while (!transactionBatch.isEmpty()); + + // Now that everything has been replayed, compare the databases. + if (enableDatabaseCompare) { + compareDatabases(); + } } /** Verifies that the replaying the SQL transaction created the same entities in Datastore. */ @@ -305,4 +351,84 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { } } } + + /** Compares the final state of both databases after replay is complete. */ + private void compareDatabases() { + boolean gotDiffs = false; + + // Build a map containing all of the SQL entities indexed by their key. + HashMap sqlEntities = new HashMap<>(); + for (Class cls : JpaEntityCoverageExtension.ALL_JPA_ENTITIES) { + if (IGNORED_ENTITIES.contains(cls.getSimpleName())) { + continue; + } + + jpaTm() + .transact( + () -> jpaTm().loadAllOfStream(cls).forEach(e -> sqlEntities.put(getSqlKey(e), e))); + } + + for (Class cls : EntityClasses.ALL_CLASSES) { + if (IGNORED_ENTITIES.contains(cls.getSimpleName())) { + continue; + } + + for (ImmutableObject entity : auditedOfy().load().type(cls).list()) { + // Find the entity in SQL and verify that it's the same. + Key ofyKey = Key.create(entity); + Object sqlKey = VKey.from(ofyKey).getSqlKey(); + ImmutableObject sqlEntity = (ImmutableObject) sqlEntities.get(sqlKey); + Optional expectedSqlEntity = ((DatastoreEntity) entity).toSqlEntity(); + if (expectedSqlEntity.isPresent()) { + // Check for null just so we get a better error message. + if (sqlEntity == null) { + logger.atSevere().log("Entity %s is in Datastore but not in SQL.", ofyKey); + gotDiffs = true; + } else { + try { + assertAboutImmutableObjects() + .that((ImmutableObject) expectedSqlEntity.get()) + .isEqualAcrossDatabases(sqlEntity); + } catch (AssertionError e) { + // Show the message but swallow the stack trace (we'll get that from the fail() at + // the end of the comparison). + logger.atSevere().log("For entity %s: %s", ofyKey, e.getMessage()); + gotDiffs = true; + } + } + } else { + logger.atInfo().log("Datastore entity has no sql representation for %s", ofyKey); + } + sqlEntities.remove(sqlKey); + } + } + + // Report any objects in the SQL set that we didn't remove while iterating over the Datastore + // objects. + if (!sqlEntities.isEmpty()) { + for (Object item : sqlEntities.values()) { + logger.atSevere().log( + "Entity of %s found in SQL but not in datastore: %s", item.getClass().getName(), item); + } + gotDiffs = true; + } + + if (gotDiffs) { + fail("There were differences between the final SQL and Datastore contents."); + } + } + + private static Object getSqlKey(Object entity) { + return jpaTm() + .getEntityManager() + .getEntityManagerFactory() + .getPersistenceUnitUtil() + .getIdentifier(entity); + } + + /** Annotation to use for test methods where we don't want to do a database comparison yet. */ + @Target({METHOD}) + @Retention(RUNTIME) + @TestTemplate + public @interface NoDatabaseCompare {} } diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 639697b18..0e8b86a20 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -267,9 +267,16 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { Map response = action.handleJsonRequest( ImmutableMap.of( - "op", "update", - "id", CLIENT_ID, - "args", setter.apply(registrar.asBuilder(), newValue).build().toJsonMap())); + "op", + "update", + "id", + CLIENT_ID, + "args", + setter + .apply(registrar.asBuilder(), newValue) + .setLastUpdateTime(registrar.getLastUpdateTime()) + .build() + .toJsonMap())); Registrar updatedRegistrar = loadRegistrar(CLIENT_ID); persistResource(registrar); @@ -320,7 +327,11 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { "id", CLIENT_ID, "args", - setter.apply(registrar.asBuilder(), newValue).build().toJsonMap())); + setter + .apply(registrar.asBuilder(), newValue) + .setLastUpdateTime(registrar.getLastUpdateTime()) + .build() + .toJsonMap())); Registrar updatedRegistrar = loadRegistrar(CLIENT_ID); persistResource(registrar);