Do full database comparison during replay tests (#1524)

* Fix entity delete replication, compare db @ replay

Replay tests currently only verify that the contents of a transaction are
can be successfully replicated to the other database.  They do not verify that
the contents of both databases are equivalent.  As a result, we miss any
changes omitted from the transaction (as was the case with entity deletions).

This change adds a final database comparison to ReplayExtension so we can
safely say that the databases are in the same state.

This comparison is introduced in part as a unit test for the one-line fix for
replication of an "entity delete" operation (where we delete using an entity
object instead of the object's key) which so far has only affected PollMessage
deletion.  The fix is also included in this commit within
JpaTransactionManagerImpl.

* Exclude tests and entities with failing comparisons

* Get all tests to pass and fix more timestamp

Fix most of the unit tests that were broken by this change.

- Fix timestamp updates after grace period changes in DomainContent and for
  TLD changes in Registrar.
- Reenable full database comparison for most DomainCreateFlowTest's.
- Make some test entities NonReplicated so they don't break when used with
  jpaTm().delete()
- Diable checking of a few more entity types that are failing comparisons.
- Add some formatting fixes.

* Remove unnecessary "NoDatabaseCompare"

I turns out that after other fixes/elisions we no longer need these for
any tests in DomainCreateFlowTest.

* Changes for review

* Remove old "compare" flag.

* Reformatted.

core/src/main/java/google/registry/model/domain/DomainContent.java

@@ -1035,17 +1035,20 @@ public class DomainContent extends EppResource
 
     public B setGracePeriods(ImmutableSet<GracePeriod> 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();
     }
 

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<R> implements Runnable {
+public class CommitLoggedWork<R> implements Runnable {
 
   private final Supplier<R> work;
   private final Clock clock;

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<String> 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() {

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();

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()));
   }