Remove the mechanism to compare objects across database (#1822)

The migration is done.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/google/nomulus/1822)
<!-- Reviewable:end -->

core/src/main/java/google/registry/beam/common/RegistryPipelineOptions.java

@@ -16,6 +16,7 @@ package google.registry.beam.common;
 
 import google.registry.beam.common.RegistryJpaIO.Write;
 import google.registry.config.RegistryEnvironment;
+import google.registry.model.annotations.DeleteAfterMigration;
 import google.registry.persistence.PersistenceModule.JpaTransactionManagerType;
 import google.registry.persistence.PersistenceModule.TransactionIsolationLevel;
 import java.util.Objects;
@@ -65,6 +66,17 @@ public interface RegistryPipelineOptions extends GcpOptions {
 
   void setSqlWriteShards(int maxConcurrentSqlWriters);
 
+  @DeleteAfterMigration
+  @Description(
+      "Whether to use self allocated primary IDs when building entities. This should only be used"
+          + " when the IDs are not significant and the resulting entities are not persisted back to"
+          + " the database. Use with caution as self allocated IDs are not unique across workers,"
+          + " and persisting entities with these IDs can be dangerous.")
+  @Default.Boolean(false)
+  boolean getUseSelfAllocatedId();
+
+  void setUseSelfAllocatedId(boolean useSelfAllocatedId);
+
   static RegistryPipelineComponent toRegistryPipelineComponent(RegistryPipelineOptions options) {
     return DaggerRegistryPipelineComponent.builder()
         .isolationOverride(options.getIsolationOverride())

core/src/main/java/google/registry/beam/common/RegistryPipelineWorkerInitializer.java

@@ -22,6 +22,8 @@ import dagger.Lazy;
 import google.registry.config.RegistryEnvironment;
 import google.registry.config.SystemPropertySetter;
 import google.registry.model.AppEngineEnvironment;
+import google.registry.model.IdService;
+import google.registry.model.IdService.SelfAllocatedIdSupplier;
 import google.registry.persistence.transaction.JpaTransactionManager;
 import google.registry.persistence.transaction.TransactionManagerFactory;
 import org.apache.beam.sdk.harness.JvmInitializer;
@@ -65,12 +67,20 @@ public class RegistryPipelineWorkerInitializer implements JvmInitializer {
         transactionManagerLazy = registryPipelineComponent.getJpaTransactionManager();
     }
     TransactionManagerFactory.setJpaTmOnBeamWorker(transactionManagerLazy::get);
-    // Masquerade all threads as App Engine threads so we can create Ofy keys in the pipeline. Also
+    // Masquerade all threads as App Engine threads, so we can create Ofy keys in the pipeline. Also
     // loads all ofy entities.
     new AppEngineEnvironment("s~" + registryPipelineComponent.getProjectId())
         .setEnvironmentForAllThreads();
-    // Set the system property so that we can call IdService.allocateId() without access to
-    // datastore.
     SystemPropertySetter.PRODUCTION_IMPL.setProperty(PROPERTY, "true");
+    // Use self-allocated IDs if requested. Note that this inevitably results in duplicate IDs from
+    // multiple workers, which can also collide with existing IDs in the database. So they cannot be
+    // dependent upon for comparison or anything significant. The resulting entities can never be
+    // persisted back into the database. This is a stop-gap measure that should only be used when
+    // you need to create Buildables in Beam, but do not have control over how the IDs are
+    // allocated, and you don't care about the generated IDs as long
+    // as you can build the entities.
+    if (registryOptions.getUseSelfAllocatedId()) {
+      IdService.setIdSupplier(SelfAllocatedIdSupplier.getInstance());
+    }
   }
 }

core/src/main/java/google/registry/beam/rde/RdePipeline.java

@@ -128,7 +128,7 @@ import org.joda.time.DateTime;
  * <h2>{@link EppResource}</h2>
  *
  * All EPP resources are loaded from the corresponding {@link HistoryEntry}, which has the resource
- * embedded. In general we find most recent history entry before watermark and filter out the ones
+ * embedded. In general, we find most recent history entry before watermark and filter out the ones
  * that are soft-deleted by watermark. The history is emitted as pairs of (resource repo ID: history
  * revision ID) from the SQL query.
  *
@@ -164,7 +164,7 @@ import org.joda.time.DateTime;
  *
  * The (pending deposit: deposit fragment) pairs from different resources are combined and grouped
  * by pending deposit. For each pending deposit, all the relevant deposit fragments are written into
- * a encrypted file stored on GCS. The filename is uniquely determined by the Beam job ID so there
+ * an encrypted file stored on GCS. The filename is uniquely determined by the Beam job ID so there
  * is no need to lock the GCS write operation to prevent stomping. The cursor for staging the
  * pending deposit is then rolled forward, and the next action is enqueued. The latter two
  * operations are performed in a transaction so the cursor is rolled back if enqueueing failed.
@@ -698,8 +698,8 @@ public class RdePipeline implements Serializable {
   }
 
   /**
-   * Encodes the pending deposit set in an URL safe string that is sent to the pipeline worker by
-   * the pipeline launcher as a pipeline option.
+   * Encodes the pending deposit set in a URL safe string that is sent to the pipeline worker by the
+   * pipeline launcher as a pipeline option.
    */
   public static String encodePendingDeposits(ImmutableSet<PendingDeposit> pendingDeposits)
       throws IOException {
@@ -715,6 +715,12 @@ public class RdePipeline implements Serializable {
     PipelineOptionsFactory.register(RdePipelineOptions.class);
     RdePipelineOptions options =
         PipelineOptionsFactory.fromArgs(args).withValidation().as(RdePipelineOptions.class);
+    // We need to self allocate the IDs because the pipeline creates EPP resources from history
+    // entries and projects them to watermark. These buildable entities would otherwise request an
+    // ID from datastore, which Beam does not have access to. The IDs are not included in the
+    // deposits or are these entities persisted back to the database, so it is OK to use a self
+    // allocated ID to get around the limitations of beam.
+    options.setUseSelfAllocatedId(true);
     RegistryPipelineOptions.validateRegistryPipelineOptions(options);
     options.setIsolationOverride(TransactionIsolationLevel.TRANSACTION_READ_COMMITTED);
     DaggerRdePipeline_RdePipelineComponent.builder().options(options).build().rdePipeline().run();

core/src/main/java/google/registry/model/IdService.java

@@ -17,58 +17,114 @@ package google.registry.model;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.appengine.api.datastore.DatastoreServiceFactory;
-import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.FluentLogger;
 import google.registry.beam.common.RegistryPipelineWorkerInitializer;
 import google.registry.config.RegistryEnvironment;
 import google.registry.model.annotations.DeleteAfterMigration;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
 
 /**
- * Allocates a globally unique {@link Long} number to use as an Ofy {@code @Id}.
+ * Allocates a {@link long} to use as a {@code @Id}, (part) of the primary SQL key for an entity.
  *
- * <p>In non-test, non-beam environments the Id is generated by Datastore, otherwise it's from an
- * atomic long number that's incremented every time this method is called.
+ * <p>Normally, the ID is globally unique and allocated by Datastore. It is possible to override
+ * this behavior by providing an ID supplier, such as in unit tests, where a self-allocated ID based
+ * on a monotonically increasing atomic {@link long} is used. Such an ID supplier can also be used
+ * in other scenarios, such as in a Beam pipeline to get around the limitation of Beam's inability
+ * to use GAE SDK to access Datastore. The override should be used with great care lest it results
+ * in irreversible data corruption.
+ *
+ * @see #setIdSupplier(Supplier)
  */
 @DeleteAfterMigration
 public final class IdService {
 
-  /**
-   * A placeholder String passed into DatastoreService.allocateIds that ensures that all ids are
-   * initialized from the same id pool.
-   */
-  private static final String APP_WIDE_ALLOCATION_KIND = "common";
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  private IdService() {}
+
+  private static Supplier<Long> idSupplier =
+      RegistryEnvironment.UNITTEST.equals(RegistryEnvironment.get())
+          ? SelfAllocatedIdSupplier.getInstance()
+          : DatastoreIdSupplier.getInstance();
 
   /**
-   * Counts of used ids for use in unit tests or Beam.
+   * Provides a {@link Supplier} of ID that overrides the default.
    *
-   * <p>Note that one should only use self-allocate Ids in Beam for entities whose Ids are not
-   * important and are not persisted back to the database, i. e. nowhere the uniqueness of the ID is
-   * required.
+   * <p>Currently, the only use case for an override is in the Beam pipeline, where access to
+   * Datastore is not possible through the App Engine API. As such, the setter explicitly checks if
+   * the runtime is Beam.
+   *
+   * <p>Because the provided supplier is not guaranteed to be globally unique and compatible with
+   * existing IDs in the database, one should proceed with great care. It is safe to use an
+   * arbitrary supplier when the resulting IDs are not significant and not persisted back to the
+   * database, i.e. the IDs are only required by the {@link Buildable} contract but are not used in
+   * any meaningful way. One example is the RDE pipeline where we project EPP resource entities from
+   * history entries to watermark time, which are then marshalled into XML elements in the RDE
+   * deposits, where the IDs are omitted.
    */
-  private static final AtomicLong nextSelfAllocatedId = new AtomicLong(1); // ids cannot be zero
-
-  private static final boolean isSelfAllocated() {
-    return RegistryEnvironment.UNITTEST.equals(RegistryEnvironment.get())
-        || "true".equals(System.getProperty(RegistryPipelineWorkerInitializer.PROPERTY, "false"));
+  public static void setIdSupplier(Supplier<Long> idSupplier) {
+    checkState(
+        "true".equals(System.getProperty(RegistryPipelineWorkerInitializer.PROPERTY, "false")),
+        "Can only set ID supplier in a Beam pipeline");
+    logger.atWarning().log("Using ID supplier override!");
+    IdService.idSupplier = idSupplier;
   }
 
   /** Allocates an id. */
-  // TODO(b/201547855): Find a way to allocate a unique ID without datastore.
   public static long allocateId() {
-    return isSelfAllocated()
-        ? nextSelfAllocatedId.getAndIncrement()
-        : DatastoreServiceFactory.getDatastoreService()
-            .allocateIds(APP_WIDE_ALLOCATION_KIND, 1)
-            .iterator()
-            .next()
-            .getId();
+    return idSupplier.get();
   }
 
-  /** Resets the global self-allocated id counter (i.e. sets the next id to 1). */
-  @VisibleForTesting
-  public static void resetSelfAllocatedId() {
-    checkState(
-        isSelfAllocated(), "Can only call resetSelfAllocatedId() in unit tests or Beam pipelines");
-    nextSelfAllocatedId.set(1); // ids cannot be zero
+  // TODO(b/201547855): Find a way to allocate a unique ID without datastore.
+  private static class DatastoreIdSupplier implements Supplier<Long> {
+
+    private static final DatastoreIdSupplier INSTANCE = new DatastoreIdSupplier();
+
+    /**
+     * A placeholder String passed into {@code DatastoreService.allocateIds} that ensures that all
+     * IDs are initialized from the same ID pool.
+     */
+    private static final String APP_WIDE_ALLOCATION_KIND = "common";
+
+    public static DatastoreIdSupplier getInstance() {
+      return INSTANCE;
+    }
+
+    @Override
+    public Long get() {
+      return DatastoreServiceFactory.getDatastoreService()
+          .allocateIds(APP_WIDE_ALLOCATION_KIND, 1)
+          .iterator()
+          .next()
+          .getId();
+    }
+  }
+
+  /**
+   * An ID supplier that allocates an ID from a monotonically increasing atomic {@link long}.
+   *
+   * <p>The generated IDs are only unique within the same JVM. It is not suitable for production use
+   * unless in cases the IDs are not significant.
+   */
+  public static class SelfAllocatedIdSupplier implements Supplier<Long> {
+
+    private static final SelfAllocatedIdSupplier INSTANCE = new SelfAllocatedIdSupplier();
+
+    /** Counts of used ids for self allocating IDs. */
+    private static final AtomicLong nextSelfAllocatedId = new AtomicLong(1); // ids cannot be zero
+
+    public static SelfAllocatedIdSupplier getInstance() {
+      return INSTANCE;
+    }
+
+    @Override
+    public Long get() {
+      return nextSelfAllocatedId.getAndIncrement();
+    }
+
+    public void reset() {
+      nextSelfAllocatedId.set(1);
+    }
   }
 }

core/src/main/java/google/registry/model/ImmutableObject.java

@@ -14,17 +14,13 @@
 
 package google.registry.model;
 
-import static com.google.common.collect.Iterables.transform;
 import static com.google.common.collect.Maps.transformValues;
-import static google.registry.model.ofy.ObjectifyService.auditedOfy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static java.lang.annotation.ElementType.FIELD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 import static java.util.stream.Collectors.toCollection;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.Joiner;
-import com.google.common.collect.Maps;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Ignore;
 import google.registry.persistence.VKey;
@@ -56,15 +52,6 @@ public abstract class ImmutableObject implements Cloneable {
   @Target(FIELD)
   public @interface DoNotHydrate {}
 
-  /**
-   * Indicates that the field should be ignored when comparing an object in the datastore to the
-   * corresponding object in Cloud SQL.
-   */
-  @Documented
-  @Retention(RUNTIME)
-  @Target(FIELD)
-  public @interface DoNotCompare {}
-
   /**
    * Indicates that the field stores a null value to indicate an empty set. This is also used in
    * object comparison.
@@ -105,7 +92,7 @@ public abstract class ImmutableObject implements Cloneable {
    */
   protected Map<Field, Object> getSignificantFields() {
     // Can't use streams or ImmutableMap because we can have null values.
-    Map<Field, Object> result = new LinkedHashMap();
+    Map<Field, Object> result = new LinkedHashMap<>();
     for (Map.Entry<Field, Object> entry : ModelUtils.getFieldValues(this).entrySet()) {
       if (!entry.getKey().isAnnotationPresent(Insignificant.class)) {
         result.put(entry.getKey(), entry.getValue());
@@ -190,15 +177,15 @@ public abstract class ImmutableObject implements Cloneable {
   /** Helper function to recursively hydrate an ImmutableObject. */
   private static Object hydrate(Object value) {
     if (value instanceof Key) {
-      if (tm().isOfy()) {
-        return hydrate(auditedOfy().load().key((Key<?>) value).now());
-      }
       return value;
-    } else if (value instanceof Map) {
+    }
+    if (value instanceof Map) {
       return transformValues((Map<?, ?>) value, ImmutableObject::hydrate);
-    } else if (value instanceof Collection) {
-      return transform((Collection<?>) value, ImmutableObject::hydrate);
-    } else if (value instanceof ImmutableObject) {
+    }
+    if (value instanceof Collection) {
+      return ((Collection<?>) value).stream().map(ImmutableObject::hydrate);
+    }
+    if (value instanceof ImmutableObject) {
       return ((ImmutableObject) value).toHydratedString();
     }
     return value;
@@ -220,7 +207,7 @@ public abstract class ImmutableObject implements Cloneable {
       }
       return result;
     } else if (o instanceof Map) {
-      return Maps.transformValues((Map<?, ?>) o, ImmutableObject::toMapRecursive);
+      return transformValues((Map<?, ?>) o, ImmutableObject::toMapRecursive);
     } else if (o instanceof Set) {
       return ((Set<?>) o)
           .stream()

core/src/main/java/google/registry/model/contact/ContactHistory.java

@@ -60,7 +60,7 @@ public class ContactHistory extends HistoryEntry implements UnsafeSerializable {
 
   // Store ContactBase instead of Contact so we don't pick up its @Id
   // Nullable for the sake of pre-Registry-3.0 history objects
-  @DoNotCompare @Nullable ContactBase contactBase;
+  @Nullable ContactBase contactBase;
 
   @Id
   @Access(AccessType.PROPERTY)

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

@@ -83,7 +83,7 @@ public class DomainHistory extends HistoryEntry {
 
   // Store DomainBase instead of Domain so we don't pick up its @Id
   // Nullable for the sake of pre-Registry-3.0 history objects
-  @DoNotCompare @Nullable DomainBase domainBase;
+  @Nullable DomainBase domainBase;
 
   @Id
   @Access(AccessType.PROPERTY)
@@ -102,7 +102,6 @@ public class DomainHistory extends HistoryEntry {
   // We could have reused domainBase.nsHosts here, but Hibernate throws a weird exception after
   // we change to use a composite primary key.
   // TODO(b/166776754): Investigate if we can reuse domainBase.nsHosts for storing host keys.
-  @DoNotCompare
   @ElementCollection
   @JoinTable(
       name = "DomainHistoryHost",
@@ -116,7 +115,6 @@ public class DomainHistory extends HistoryEntry {
   @Column(name = "host_repo_id")
   Set<VKey<Host>> nsHosts;
 
-  @DoNotCompare
   @OneToMany(
       cascade = {CascadeType.ALL},
       fetch = FetchType.EAGER,
@@ -137,7 +135,6 @@ public class DomainHistory extends HistoryEntry {
   @Ignore
   Set<DomainDsDataHistory> dsDataHistories = new HashSet<>();
 
-  @DoNotCompare
   @OneToMany(
       cascade = {CascadeType.ALL},
       fetch = FetchType.EAGER,

core/src/main/java/google/registry/model/host/HostHistory.java

@@ -61,7 +61,7 @@ public class HostHistory extends HistoryEntry implements UnsafeSerializable {
 
   // Store HostBase instead of Host so we don't pick up its @Id
   // Nullable for the sake of pre-Registry-3.0 history objects
-  @DoNotCompare @Nullable HostBase hostBase;
+  @Nullable HostBase hostBase;
 
   @Id
   @Access(AccessType.PROPERTY)

core/src/test/java/google/registry/model/ImmutableObjectSubject.java

@@ -18,26 +18,19 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertAbout;
 import static com.google.common.truth.Truth.assertThat;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Sets;
 import com.google.common.truth.Correspondence;
 import com.google.common.truth.Correspondence.BinaryPredicate;
 import com.google.common.truth.FailureMetadata;
 import com.google.common.truth.SimpleSubjectBuilder;
 import com.google.common.truth.Subject;
-import java.lang.annotation.Annotation;
 import java.lang.reflect.Field;
 import java.util.Arrays;
-import java.util.Collection;
-import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collector;
 import javax.annotation.Nullable;
 
 /** Truth subject for asserting things about ImmutableObjects that are not built in. */
@@ -45,7 +38,7 @@ public final class ImmutableObjectSubject extends Subject {
 
   @Nullable private final ImmutableObject actual;
 
-  protected ImmutableObjectSubject(
+  private ImmutableObjectSubject(
       FailureMetadata failureMetadata, @Nullable ImmutableObject actual) {
     super(failureMetadata, actual);
     this.actual = actual;
@@ -73,261 +66,6 @@ public final class ImmutableObjectSubject extends Subject {
     }
   }
 
-  /**
-   * Checks that {@code expected} has the same contents as {@code actual} except for fields that are
-   * marked with {@link ImmutableObject.DoNotCompare}.
-   *
-   * <p>This is used to verify that entities stored in both cloud SQL and Datastore are identical.
-   */
-  public void isEqualAcrossDatabases(@Nullable ImmutableObject expected) {
-    ComparisonResult result =
-        checkObjectAcrossDatabases(
-            actual, expected, actual != null ? actual.getClass().getName() : "null");
-    if (result.isFailure()) {
-      throw new AssertionError(result.getMessage());
-    }
-  }
-
-  // The following "check" methods implement a recursive check of immutable object equality across
-  // databases.  All of them function in both assertive and predicate modes: if "path" is
-  // provided (not null) then they throw AssertionError's with detailed error messages.  If
-  // it is null, they return true for equal objects and false for inequal ones.
-  //
-  // The reason for this dual-mode behavior is that all of these methods can either be used in the
-  // context of a test assertion (in which case we want a detailed error message describing exactly
-  // the location in a complex object where a difference was discovered) or in the context of a
-  // membership check in a set (in which case we don't care about the specific location of the first
-  // difference, we just want to be able to determine if the object "is equal to" another object as
-  // efficiently as possible -- see checkSetAcrossDatabase()).
-
-  @VisibleForTesting
-  static ComparisonResult checkObjectAcrossDatabases(
-      @Nullable Object actual, @Nullable Object expected, @Nullable String path) {
-    if (Objects.equals(actual, expected)) {
-      return ComparisonResult.createSuccess();
-    }
-
-    // They're different, do a more detailed comparison.
-
-    // Check for null first (we can assume both variables are not null at this point).
-    if (actual == null) {
-      return ComparisonResult.createFailure(path, "expected ", expected, "got null.");
-    } else if (expected == null) {
-      return ComparisonResult.createFailure(path, "expected null, got ", actual);
-
-      // For immutable objects, we have to recurse since the contained
-      // object could have DoNotCompare fields, too.
-    } else if (expected instanceof ImmutableObject) {
-      // We only verify that actual is an ImmutableObject so we get a good error message instead
-      // of a context-less ClassCastException.
-      if (!(actual instanceof ImmutableObject)) {
-        return ComparisonResult.createFailure(path, actual, " is not an immutable object.");
-      }
-
-      return checkImmutableAcrossDatabases(
-          (ImmutableObject) actual, (ImmutableObject) expected, path);
-    } else if (expected instanceof Map) {
-      if (!(actual instanceof Map)) {
-        return ComparisonResult.createFailure(path, actual, " is not a Map.");
-      }
-
-      // This would likely be more efficient if we could assume that keys can be compared across
-      // databases using .equals(), however we cannot guarantee key equality so the simplest and
-      // most correct way to accomplish this is by reusing the set comparison.
-      return checkSetAcrossDatabases(
-          ((Map<?, ?>) actual).entrySet(), ((Map<?, ?>) expected).entrySet(), path, "Map");
-    } else if (expected instanceof Set) {
-      if (!(actual instanceof Set)) {
-        return ComparisonResult.createFailure(path, actual, " is not a Set.");
-      }
-
-      return checkSetAcrossDatabases((Set<?>) actual, (Set<?>) expected, path, "Set");
-    } else if (expected instanceof Collection) {
-      if (!(actual instanceof Collection)) {
-        return ComparisonResult.createFailure(path, actual, " is not a Collection.");
-      }
-
-      return checkListAcrossDatabases((Collection<?>) actual, (Collection<?>) expected, path);
-      // Give Map.Entry special treatment to facilitate the use of Set comparison for verification
-      // of Map.
-    } else if (expected instanceof Map.Entry) {
-      if (!(actual instanceof Map.Entry)) {
-        return ComparisonResult.createFailure(path, actual, " is not a Map.Entry.");
-      }
-
-      // Check both the key and value.  We can always ignore the path here, this should only be
-      // called from within a set comparison.
-      ComparisonResult result;
-      if ((result =
-              checkObjectAcrossDatabases(
-                  ((Map.Entry<?, ?>) actual).getKey(), ((Map.Entry<?, ?>) expected).getKey(), null))
-          .isFailure()) {
-        return result;
-      }
-      if ((result =
-              checkObjectAcrossDatabases(
-                  ((Map.Entry<?, ?>) actual).getValue(),
-                  ((Map.Entry<?, ?>) expected).getValue(),
-                  null))
-          .isFailure()) {
-        return result;
-      }
-    } else {
-      // Since we know that the objects are not equal and since any other types can not be expected
-      // to contain DoNotCompare elements, this condition is always a failure.
-      return ComparisonResult.createFailure(path, actual, " is not equal to ", expected);
-    }
-
-    return ComparisonResult.createSuccess();
-  }
-
-  private static ComparisonResult checkSetAcrossDatabases(
-      Set<?> actual, Set<?> expected, String path, String type) {
-    // Unfortunately, we can't just check to see whether one set "contains" all of the elements of
-    // the other, as the cross database checks don't require strict equality.  Instead we have to do
-    // an N^2 comparison to search for an equivalent element.
-
-    // Objects in expected that aren't in actual.  We use "identity sets" here and below because we
-    // want to keep track of the _objects themselves_ rather than rely upon any overridable notion
-    // of equality.
-    Set<Object> missing = path != null ? Sets.newIdentityHashSet() : null;
-
-    // Objects from actual that have matching elements in expected.
-    Set<Object> found = Sets.newIdentityHashSet();
-
-    // Build missing and found.
-    for (Object expectedElem : expected) {
-      boolean gotMatch = false;
-      for (Object actualElem : actual) {
-        if (!checkObjectAcrossDatabases(actualElem, expectedElem, null).isFailure()) {
-          gotMatch = true;
-
-          // Add the element to the set of expected elements that were "found" in actual.  If the
-          // element matches multiple elements in "expected," we have a basic problem with this
-          // kind of set that we'll want to know about.
-          if (!found.add(actualElem)) {
-            return ComparisonResult.createFailure(
-                path, "element ", actualElem, " matches multiple elements in ", expected);
-          }
-
-          break;
-        }
-      }
-
-      if (!gotMatch) {
-        if (path == null) {
-          return ComparisonResult.createFailure();
-        }
-        missing.add(expectedElem);
-      }
-    }
-
-    if (path != null) {
-      // Provide a detailed message consisting of any missing or unexpected items.
-
-      // Build a set of all objects in actual that don't have counterparts in expected.
-      Set<Object> unexpected =
-          actual.stream()
-              .filter(actualElem -> !found.contains(actualElem))
-              .collect(
-                  Collector.of(
-                      Sets::newIdentityHashSet,
-                      Set::add,
-                      (result, values) -> {
-                        result.addAll(values);
-                        return result;
-                      }));
-
-      if (!missing.isEmpty() || !unexpected.isEmpty()) {
-        String message = type + " does not contain the expected contents.";
-        if (!missing.isEmpty()) {
-          message += "  It is missing: " + formatItems(missing.iterator());
-        }
-
-        if (!unexpected.isEmpty()) {
-          message += "  It contains additional elements: " + formatItems(unexpected.iterator());
-        }
-
-        return ComparisonResult.createFailure(path, message);
-      }
-
-      // We just need to check if there were any objects in "actual" that were not in "expected"
-      // (where "found" is a proxy for "expected").
-    } else if (!found.containsAll(actual)) {
-      return ComparisonResult.createFailure();
-    }
-
-    return ComparisonResult.createSuccess();
-  }
-
-  private static ComparisonResult checkListAcrossDatabases(
-      Collection<?> actual, Collection<?> expected, @Nullable String path) {
-    Iterator<?> actualIter = actual.iterator();
-    Iterator<?> expectedIter = expected.iterator();
-    int index = 0;
-    while (actualIter.hasNext() && expectedIter.hasNext()) {
-      Object actualItem = actualIter.next();
-      Object expectedItem = expectedIter.next();
-      ComparisonResult result =
-          checkObjectAcrossDatabases(
-              actualItem, expectedItem, path != null ? path + "[" + index + "]" : null);
-      if (result.isFailure()) {
-        return result;
-      }
-      ++index;
-    }
-
-    if (actualIter.hasNext()) {
-      return ComparisonResult.createFailure(
-          path, "has additional items: ", formatItems(actualIter));
-    }
-
-    if (expectedIter.hasNext()) {
-      return ComparisonResult.createFailure(path, "missing items: ", formatItems(expectedIter));
-    }
-
-    return ComparisonResult.createSuccess();
-  }
-
-  /** Recursive helper for isEqualAcrossDatabases. */
-  private static ComparisonResult checkImmutableAcrossDatabases(
-      ImmutableObject actual, ImmutableObject expected, String path) {
-    Map<Field, Object> actualFields = filterFields(actual, ImmutableObject.DoNotCompare.class);
-    Map<Field, Object> expectedFields = filterFields(expected, ImmutableObject.DoNotCompare.class);
-
-    for (Map.Entry<Field, Object> entry : expectedFields.entrySet()) {
-      if (!actualFields.containsKey(entry.getKey())) {
-        return ComparisonResult.createFailure(path, "is missing field ", entry.getKey().getName());
-      }
-
-      // Verify that the field values are the same.
-      Object expectedFieldValue = entry.getValue();
-      Object actualFieldValue = actualFields.get(entry.getKey());
-      ComparisonResult result =
-          checkObjectAcrossDatabases(
-              actualFieldValue,
-              expectedFieldValue,
-              path != null ? path + "." + entry.getKey().getName() : null);
-      if (result.isFailure()) {
-        return result;
-      }
-    }
-
-    // Check for fields in actual that are not in expected.
-    for (Map.Entry<Field, Object> entry : actualFields.entrySet()) {
-      if (!expectedFields.containsKey(entry.getKey())) {
-        return ComparisonResult.createFailure(
-            path, "has additional field ", entry.getKey().getName());
-      }
-    }
-
-    return ComparisonResult.createSuccess();
-  }
-
-  private static String formatItems(Iterator<?> iter) {
-    return Joiner.on(", ").join(iter);
-  }
-
   /** Encapsulates success/failure result in recursive comparison with optional error string. */
   static class ComparisonResult {
     boolean succeeded;
@@ -412,7 +150,6 @@ public final class ImmutableObjectSubject extends Subject {
     // don't use ImmutableMap or a stream->collect model since we can have nulls
     Map<Field, Object> result = new LinkedHashMap<>();
     for (Map.Entry<Field, Object> entry : originalFields.entrySet()) {
-      // TODO(b/203685960): filter by @DoNotCompare instead.
       if (entry.getKey().isAnnotationPresent(ImmutableObject.Insignificant.class)) {
         continue;
       }
@@ -422,28 +159,4 @@ public final class ImmutableObjectSubject extends Subject {
     }
     return result;
   }
-
-  /** Filter out fields with the given annotation. */
-  private static Map<Field, Object> filterFields(
-      ImmutableObject original, Class<? extends Annotation> annotation) {
-    Map<Field, Object> originalFields = ModelUtils.getFieldValues(original);
-    // don't use ImmutableMap or a stream->collect model since we can have nulls
-    Map<Field, Object> result = new LinkedHashMap<>();
-    for (Map.Entry<Field, Object> entry : originalFields.entrySet()) {
-      // TODO(b/203685960): filter by @DoNotCompare instead.
-      if (!entry.getKey().isAnnotationPresent(annotation)
-          && !entry.getKey().isAnnotationPresent(ImmutableObject.Insignificant.class)) {
-
-        // Perform any necessary substitutions.
-        if (entry.getKey().isAnnotationPresent(ImmutableObject.EmptySetToNull.class)
-            && entry.getValue() != null
-            && ((Set<?>) entry.getValue()).isEmpty()) {
-          result.put(entry.getKey(), null);
-        } else {
-          result.put(entry.getKey(), entry.getValue());
-        }
-      }
-    }
-    return result;
-  }
 }

core/src/test/java/google/registry/model/ImmutableObjectSubjectTest.java

@@ -14,380 +14,17 @@
 
 package google.registry.model;
 
-import static com.google.common.truth.Truth.assertThat;
-import static google.registry.model.ImmutableObjectSubject.ComparisonResult;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
-import static google.registry.model.ImmutableObjectSubject.checkObjectAcrossDatabases;
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 import org.junit.jupiter.api.Test;
 
 public class ImmutableObjectSubjectTest {
 
-  // Unique id to assign to the "ignored" field so that it always gets a unique value.
-  private static int uniqueId = 0;
-
-  @Test
-  void testCrossDatabase_nulls() {
-    assertAboutImmutableObjects().that(null).isEqualAcrossDatabases(null);
-    assertAboutImmutableObjects()
-        .that(makeTestAtom(null))
-        .isEqualAcrossDatabases(makeTestAtom(null));
-
-    assertThat(checkObjectAcrossDatabases(null, makeTestAtom("foo"), null).isFailure()).isTrue();
-    assertThat(checkObjectAcrossDatabases(null, makeTestAtom("foo"), null).isFailure()).isTrue();
-  }
-
-  @Test
-  void testCrossDatabase_equalObjects() {
-    TestImmutableObject actual = makeTestObj();
-    assertAboutImmutableObjects().that(actual).isEqualAcrossDatabases(actual);
-    assertAboutImmutableObjects().that(actual).isEqualAcrossDatabases(makeTestObj());
-    assertThat(checkObjectAcrossDatabases(makeTestObj(), makeTestObj(), null).isFailure())
-        .isFalse();
-  }
-
-  @Test
-  void testCrossDatabase_simpleFieldFailure() {
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withStringField("bar")));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$TestImmutableObject.stringField:");
-    assertThat(
-            checkObjectAcrossDatabases(makeTestObj(), makeTestObj().withStringField(null), null)
-                .isFailure())
-        .isTrue();
-  }
-
-  @Test
-  void testCrossDatabase_nestedImmutableFailure() {
-    // Repeat the null checks to verify that the attribute path is preserved.
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withNested(null)));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$TestImmutableObject.nested:"
-                + " expected null, got TestImmutableObject");
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj().withNested(null))
-                    .isEqualAcrossDatabases(makeTestObj()));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$TestImmutableObject.nested:"
-                + " expected TestImmutableObject");
-    assertThat(e).hasMessageThat().contains("got null.");
-
-    // Test with a field difference.
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj().withNested(makeTestObj().withNested(null))));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.nested.stringField:");
-    assertThat(
-            checkObjectAcrossDatabases(makeTestObj(), makeTestObj().withNested(null), null)
-                .isFailure())
-        .isTrue();
-  }
-
-  @Test
-  void testCrossDatabase_listFailure() {
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withList(null)));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$" + "TestImmutableObject.list:");
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj().withList(ImmutableList.of(makeTestAtom("wack")))));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.list[0].stringField:");
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj()
-                            .withList(
-                                ImmutableList.of(
-                                    makeTestAtom("baz"),
-                                    makeTestAtom("bot"),
-                                    makeTestAtom("boq")))));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.list: missing items");
-    // Make sure multiple additional items get formatted nicely.
-    assertThat(e).hasMessageThat().contains("}, TestImmutableObject");
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withList(ImmutableList.of())));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.list: has additional items");
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(),
-                    makeTestObj()
-                        .withList(ImmutableList.of(makeTestAtom("baz"), makeTestAtom("gauze"))),
-                    null)
-                .isFailure())
-        .isTrue();
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(), makeTestObj().withList(ImmutableList.of()), null)
-                .isFailure())
-        .isTrue();
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(),
-                    makeTestObj().withList(ImmutableList.of(makeTestAtom("gauze"))),
-                    null)
-                .isFailure())
-        .isTrue();
-  }
-
-  @Test
-  void testCrossDatabase_setFailure() {
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withSet(null)));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.set: expected null, got ");
-
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj().withSet(ImmutableSet.of(makeTestAtom("jim")))));
-    assertThat(e)
-        .hasMessageThat()
-        .containsMatch(
-            Pattern.compile(
-                "Set does not contain the expected contents.  "
-                    + "It is missing: .*jim.*  It contains additional elements: .*bob",
-                Pattern.DOTALL));
-
-    // Trickery here to verify that multiple items that both match existing items in the set trigger
-    // an error: we can add two of the same items because equality for purposes of the set includes
-    // the DoNotCompare field.
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj()
-                            .withSet(ImmutableSet.of(makeTestAtom("bob"), makeTestAtom("bob")))));
-    assertThat(e)
-        .hasMessageThat()
-        .containsMatch(
-            Pattern.compile(
-                "At google.registry.model.ImmutableObjectSubjectTest\\$TestImmutableObject.set: "
-                    + "element .*bob.* matches multiple elements in .*bob.*bob",
-                Pattern.DOTALL));
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(
-                        makeTestObj()
-                            .withSet(ImmutableSet.of(makeTestAtom("bob"), makeTestAtom("bob"))))
-                    .isEqualAcrossDatabases(makeTestObj()));
-    assertThat(e)
-        .hasMessageThat()
-        .containsMatch(
-            Pattern.compile(
-                "At google.registry.model.ImmutableObjectSubjectTest\\$TestImmutableObject.set: "
-                    + "Set does not contain the expected contents.  It contains additional "
-                    + "elements: .*bob",
-                Pattern.DOTALL));
-
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(),
-                    makeTestObj()
-                        .withSet(ImmutableSet.of(makeTestAtom("bob"), makeTestAtom("robert"))),
-                    null)
-                .isFailure())
-        .isTrue();
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(), makeTestObj().withSet(ImmutableSet.of()), null)
-                .isFailure())
-        .isTrue();
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(),
-                    makeTestObj()
-                        .withSet(ImmutableSet.of(makeTestAtom("bob"), makeTestAtom("bob"))),
-                    null)
-                .isFailure())
-        .isTrue();
-    // We don't test the case where actual's set contains multiple items matching the single item in
-    // the expected set: that path is the same as the "additional contents" path.
-  }
-
-  @Test
-  void testCrossDatabase_mapFailure() {
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(makeTestObj().withMap(null)));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$"
-                + "TestImmutableObject.map: expected null, got ");
-
-    e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(makeTestObj())
-                    .isEqualAcrossDatabases(
-                        makeTestObj()
-                            .withMap(ImmutableMap.of(makeTestAtom("difk"), makeTestAtom("difv")))));
-    assertThat(e)
-        .hasMessageThat()
-        .containsMatch(
-            Pattern.compile(
-                "Map does not contain the expected contents.  "
-                    + "It is missing: .*difk.*difv.*  It contains additional elements: .*key.*val",
-                Pattern.DOTALL));
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(),
-                    makeTestObj()
-                        .withMap(
-                            ImmutableMap.of(
-                                makeTestAtom("key"), makeTestAtom("val"),
-                                makeTestAtom("otherk"), makeTestAtom("otherv"))),
-                    null)
-                .isFailure())
-        .isTrue();
-    assertThat(
-            checkObjectAcrossDatabases(
-                    makeTestObj(), makeTestObj().withMap(ImmutableMap.of()), null)
-                .isFailure())
-        .isTrue();
-  }
-
-  @Test
-  void testCrossDatabase_typeChecks() {
-    ComparisonResult result = checkObjectAcrossDatabases("blech", makeTestObj(), "xxx");
-    assertThat(result.getMessage()).isEqualTo("At xxx: blech is not an immutable object.");
-    assertThat(result.isFailure()).isTrue();
-    assertThat(checkObjectAcrossDatabases("blech", makeTestObj(), null).isFailure()).isTrue();
-
-    result = checkObjectAcrossDatabases("blech", ImmutableMap.of(), "xxx");
-    assertThat(result.getMessage()).isEqualTo("At xxx: blech is not a Map.");
-    assertThat(result.isFailure()).isTrue();
-    assertThat(checkObjectAcrossDatabases("blech", ImmutableMap.of(), null).isFailure()).isTrue();
-
-    result = checkObjectAcrossDatabases("blech", ImmutableList.of(), "xxx");
-    assertThat(result.getMessage()).isEqualTo("At xxx: blech is not a Collection.");
-    assertThat(result.isFailure()).isTrue();
-    assertThat(checkObjectAcrossDatabases("blech", ImmutableList.of(), null).isFailure()).isTrue();
-
-    for (ImmutableMap.Entry<String, String> entry : ImmutableMap.of("foo", "bar").entrySet()) {
-      result = checkObjectAcrossDatabases("blech", entry, "xxx");
-      assertThat(result.getMessage()).isEqualTo("At xxx: blech is not a Map.Entry.");
-      assertThat(result.isFailure()).isTrue();
-      assertThat(checkObjectAcrossDatabases("blech", entry, "xxx").isFailure()).isTrue();
-    }
-  }
-
-  @Test
-  void testCrossDatabase_checkAdditionalFields() {
-    AssertionError e =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                assertAboutImmutableObjects()
-                    .that(DerivedImmutableObject.create())
-                    .isEqualAcrossDatabases(makeTestAtom(null)));
-    assertThat(e)
-        .hasMessageThat()
-        .contains(
-            "At google.registry.model.ImmutableObjectSubjectTest$DerivedImmutableObject: "
-                + "has additional field extraField");
-
-    assertThat(
-            checkObjectAcrossDatabases(DerivedImmutableObject.create(), makeTestAtom(null), null)
-                .isFailure())
-        .isTrue();
-  }
-
   @Test
   void testHasCorrectHashValue() {
     TestImmutableObject object = makeTestObj();
@@ -421,8 +58,6 @@ public class ImmutableObjectSubjectTest {
     ImmutableSet<TestImmutableObject> set;
     ImmutableMap<TestImmutableObject, TestImmutableObject> map;
 
-    @ImmutableObject.DoNotCompare int ignored;
-
     static TestImmutableObject create(
         String stringField,
         TestImmutableObject nested,
@@ -435,7 +70,6 @@ public class ImmutableObjectSubjectTest {
       instance.list = list;
       instance.set = set;
       instance.map = map;
-      instance.ignored = ++uniqueId;
       return instance;
     }
 

core/src/test/java/google/registry/testing/AppEngineExtension.java

@@ -40,7 +40,7 @@ import com.google.common.collect.Sets;
 import com.google.common.io.Files;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.ObjectifyFilter;
-import google.registry.model.IdService;
+import google.registry.model.IdService.SelfAllocatedIdSupplier;
 import google.registry.model.ofy.ObjectifyService;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.Registrar.State;
@@ -441,7 +441,7 @@ public final class AppEngineExtension implements BeforeEachCallback, AfterEachCa
 
     ObjectifyService.initOfy();
     // Reset id allocation in ObjectifyService so that ids are deterministic in tests.
-    IdService.resetSelfAllocatedId();
+    SelfAllocatedIdSupplier.getInstance().reset();
     this.ofyTestEntities.forEach(AppEngineExtension::register);
   }