From 9e6f99face455528ce5e2955af2852272f8a2a54 Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Fri, 15 Jan 2021 14:20:55 -0500 Subject: [PATCH] Add object comparison to replay tests (#925) * Add object comparison to replay tests Allow optional object comparison in the replay test extension and enable it for the DomainCreateFlow test. To faciliate this, add two new field annotations to ImmutableObject: DoNotCompare, to be used for fields that are not relevant to either database, and Insignificant, to be used for fields that are mutated after they have been accessed and therefore violate immutability (there is currently only one of these, however we might discover more in the course of adding more comparisons to the replay test. * Revert commented out premium price error log * Added static create methods for ReplayExtension --- .../google/registry/model/EppResource.java | 2 +- .../registry/model/ImmutableObject.java | 43 +++++++++- .../registry/model/domain/DomainContent.java | 8 +- .../registry/model/domain/DomainHistory.java | 5 +- .../registry/model/ofy/ReplayQueue.java | 81 +++++++++++++++++-- .../registry/model/ofy/TransactionInfo.java | 43 ++-------- .../registry/model/poll/PollMessage.java | 14 +++- .../reporting/DomainTransactionRecord.java | 2 + .../model/reporting/HistoryEntry.java | 1 + .../flows/domain/DomainCreateFlowTest.java | 2 +- .../flows/domain/DomainDeleteFlowTest.java | 2 +- .../flows/domain/DomainRenewFlowTest.java | 2 +- .../flows/domain/DomainUpdateFlowTest.java | 2 +- .../model/ImmutableObjectSubject.java | 44 ++++++++++ .../registry/testing/ReplayExtension.java | 69 +++++++++++++++- 15 files changed, 262 insertions(+), 58 deletions(-) diff --git a/core/src/main/java/google/registry/model/EppResource.java b/core/src/main/java/google/registry/model/EppResource.java index 586d601f8..067536ccd 100644 --- a/core/src/main/java/google/registry/model/EppResource.java +++ b/core/src/main/java/google/registry/model/EppResource.java @@ -148,7 +148,7 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { * * @see google.registry.model.translators.CommitLogRevisionsTranslatorFactory */ - @Transient + @Transient @DoNotCompare ImmutableSortedMap> revisions = ImmutableSortedMap.of(); public String getRepoId() { diff --git a/core/src/main/java/google/registry/model/ImmutableObject.java b/core/src/main/java/google/registry/model/ImmutableObject.java index 31407dec7..e830f3af8 100644 --- a/core/src/main/java/google/registry/model/ImmutableObject.java +++ b/core/src/main/java/google/registry/model/ImmutableObject.java @@ -54,9 +54,37 @@ public abstract class ImmutableObject implements Cloneable { @Target(FIELD) public @interface DoNotHydrate {} - @Ignore - @XmlTransient - Integer hashCode; + /** + * 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. + */ + @Documented + @Retention(RUNTIME) + @Target(FIELD) + public @interface EmptySetToNull {} + + /** + * Indicates that the field does not take part in the immutability contract. + * + *

Certain fields currently get modified by hibernate and there is nothing we can do about it. + * As well as violating immutability, this breaks hashing and equality comparisons, so we mark + * these fields with this annotation to exclude them from most operations. + */ + @Documented + @Retention(RUNTIME) + @Target(FIELD) + public @interface Insignificant {} + + @Ignore @XmlTransient protected Integer hashCode; private boolean equalsImmutableObject(ImmutableObject other) { return getClass().equals(other.getClass()) @@ -71,7 +99,14 @@ public abstract class ImmutableObject implements Cloneable { *

Isolated into a method so that derived classes can override it. */ protected Map getSignificantFields() { - return ModelUtils.getFieldValues(this); + // Can't use streams or ImmutableMap because we can have null values. + Map result = new LinkedHashMap(); + for (Map.Entry entry : ModelUtils.getFieldValues(this).entrySet()) { + if (!entry.getKey().isAnnotationPresent(Insignificant.class)) { + result.put(entry.getKey(), entry.getValue()); + } + } + return result; } @Override 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 bafc897cc..0d05e7eac 100644 --- a/core/src/main/java/google/registry/model/domain/DomainContent.java +++ b/core/src/main/java/google/registry/model/domain/DomainContent.java @@ -50,6 +50,7 @@ import com.googlecode.objectify.condition.IfNull; import google.registry.flows.ResourceFlowUtils; import google.registry.model.EppResource; import google.registry.model.EppResource.ResourceWithTransferData; +import google.registry.model.ImmutableObject.EmptySetToNull; import google.registry.model.billing.BillingEvent; import google.registry.model.common.EntityGroupRoot; import google.registry.model.contact.ContactResource; @@ -132,7 +133,7 @@ public class DomainContent extends EppResource @Index String tld; /** References to hosts that are the nameservers for the domain. */ - @Index @Transient Set> nsHosts; + @EmptySetToNull @Index @Transient Set> nsHosts; /** * The union of the contacts visible via {@link #getContacts} and {@link #getRegistrant}. @@ -319,6 +320,11 @@ public class DomainContent extends EppResource autorenewPollMessageHistoryId = getHistoryId(autorenewPollMessage); autorenewBillingEventHistoryId = getHistoryId(autorenewBillingEvent); deletePollMessageHistoryId = getHistoryId(deletePollMessage); + + // Fix PollMessage VKeys. + autorenewPollMessage = PollMessage.Autorenew.convertVKey(autorenewPollMessage); + deletePollMessage = PollMessage.OneTime.convertVKey(deletePollMessage); + dsData = nullToEmptyImmutableCopy(dsData).stream() .map(dsData -> dsData.cloneWithDomainRepoId(getRepoId())) 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 00d207ec9..98f6e3860 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -104,6 +104,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { "domain_history_history_revision_id,domain_history_domain_repo_id,host_repo_id", unique = true), }) + @ImmutableObject.EmptySetToNull @Column(name = "host_repo_id") Set> nsHosts; @@ -180,7 +181,9 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { * #getDomainTransactionRecords()}. */ @Access(AccessType.PROPERTY) - @OneToMany(cascade = {CascadeType.ALL}) + @OneToMany( + cascade = {CascadeType.ALL}, + fetch = FetchType.EAGER) @JoinColumn(name = "historyRevisionId", referencedColumnName = "historyRevisionId") @JoinColumn(name = "domainRepoId", referencedColumnName = "domainRepoId") @SuppressWarnings("unused") diff --git a/core/src/main/java/google/registry/model/ofy/ReplayQueue.java b/core/src/main/java/google/registry/model/ofy/ReplayQueue.java index 75874dafa..a488daf18 100644 --- a/core/src/main/java/google/registry/model/ofy/ReplayQueue.java +++ b/core/src/main/java/google/registry/model/ofy/ReplayQueue.java @@ -14,7 +14,18 @@ package google.registry.model.ofy; +import static google.registry.model.ofy.EntityWritePriorities.getEntityPriority; +import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; + +import com.google.common.collect.ImmutableMap; +import com.googlecode.objectify.Key; import google.registry.config.RegistryEnvironment; +import google.registry.model.UpdateAutoTimestamp; +import google.registry.persistence.VKey; +import google.registry.schema.replay.DatastoreEntity; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.ConcurrentLinkedQueue; /** @@ -24,23 +35,79 @@ import java.util.concurrent.ConcurrentLinkedQueue; */ public class ReplayQueue { - static ConcurrentLinkedQueue queue = - new ConcurrentLinkedQueue(); + static ConcurrentLinkedQueue, Object>> queue = + new ConcurrentLinkedQueue, Object>>(); static void addInTests(TransactionInfo info) { if (RegistryEnvironment.get() == RegistryEnvironment.UNITTEST) { - queue.add(info); + // Transform the entities to be persisted to the set of values as they were actually + // persisted. + ImmutableMap.Builder, Object> builder = new ImmutableMap.Builder, Object>(); + for (ImmutableMap.Entry, Object> entry : info.getChanges().entrySet()) { + if (entry.getValue().equals(TransactionInfo.Delete.SENTINEL)) { + builder.put(entry.getKey(), entry.getValue()); + } else { + // The value is an entity object that has not yet been persisted, and thus some of the + // special transformations that we do (notably the auto-timestamp transformations) have + // not been applied. Converting the object to an entity and then back again performs + // those transformations so that we persist the same values to SQL that we have in + // Datastore. + builder.put(entry.getKey(), ofy().toPojo(ofy().toEntity(entry.getValue()))); + } + } + queue.add(builder.build()); } } - public static void replay() { - TransactionInfo info; - while ((info = queue.poll()) != null) { - info.saveToJpa(); + /** Replay all transactions, return the set of keys that were replayed. */ + public static ImmutableMap, Object> replay() { + // We can't use an ImmutableMap.Builder here, we need to be able to overwrite existing values + // and the builder doesn't support that. + Map, Object> result = new HashMap, Object>(); + ImmutableMap, Object> changes; + while ((changes = queue.poll()) != null) { + saveToJpa(changes); + result.putAll(changes); } + + return ImmutableMap.copyOf(result); } public static void clear() { queue.clear(); } + + /** Returns the priority of the entity type in the map entry. */ + private static int getPriority(ImmutableMap.Entry, Object> entry) { + return getEntityPriority( + entry.getKey().getKind(), entry.getValue().equals(TransactionInfo.Delete.SENTINEL)); + } + + private static int compareByPriority( + ImmutableMap.Entry, Object> a, ImmutableMap.Entry, Object> b) { + return getPriority(a) - getPriority(b); + } + + private static void saveToJpa(ImmutableMap, Object> changes) { + try (UpdateAutoTimestamp.DisableAutoUpdateResource disabler = + UpdateAutoTimestamp.disableAutoUpdate()) { + // Sort the changes into an order that will work for insertion into the database. + jpaTm() + .transact( + () -> { + changes.entrySet().stream() + .sorted(ReplayQueue::compareByPriority) + .forEach( + entry -> { + if (entry.getValue().equals(TransactionInfo.Delete.SENTINEL)) { + jpaTm().delete(VKey.from(entry.getKey())); + } else { + ((DatastoreEntity) entry.getValue()) + .toSqlEntity() + .ifPresent(jpaTm()::put); + } + }); + }); + } + } } diff --git a/core/src/main/java/google/registry/model/ofy/TransactionInfo.java b/core/src/main/java/google/registry/model/ofy/TransactionInfo.java index 54ed4559d..d488b4a51 100644 --- a/core/src/main/java/google/registry/model/ofy/TransactionInfo.java +++ b/core/src/main/java/google/registry/model/ofy/TransactionInfo.java @@ -20,24 +20,20 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Maps.filterValues; import static com.google.common.collect.Maps.toMap; import static google.registry.model.ofy.CommitLogBucket.getArbitraryBucketId; -import static google.registry.model.ofy.EntityWritePriorities.getEntityPriority; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.googlecode.objectify.Key; -import google.registry.persistence.VKey; -import google.registry.schema.replay.DatastoreEntity; import java.util.Map; import org.joda.time.DateTime; /** Metadata for an {@link Ofy} transaction that saves commit logs. */ -class TransactionInfo { +public class TransactionInfo { @VisibleForTesting - enum Delete { + public enum Delete { SENTINEL } @@ -87,6 +83,10 @@ class TransactionInfo { return ImmutableSet.copyOf(changesBuilder.build().keySet()); } + ImmutableMap, Object> getChanges() { + return changesBuilder.build(); + } + ImmutableSet> getDeletes() { return ImmutableSet.copyOf( filterValues(changesBuilder.build(), Delete.SENTINEL::equals).keySet()); @@ -100,35 +100,4 @@ class TransactionInfo { .filter(not(Delete.SENTINEL::equals)) .collect(toImmutableSet()); } - - /** Returns the weight of the entity type in the map entry. */ - @VisibleForTesting - static int getWeight(ImmutableMap.Entry, Object> entry) { - return getEntityPriority(entry.getKey().getKind(), entry.getValue().equals(Delete.SENTINEL)); - } - - private static int compareByWeight( - ImmutableMap.Entry, Object> a, ImmutableMap.Entry, Object> b) { - return getWeight(a) - getWeight(b); - } - - void saveToJpa() { - // Sort the changes into an order that will work for insertion into the database. - jpaTm() - .transact( - () -> { - changesBuilder.build().entrySet().stream() - .sorted(TransactionInfo::compareByWeight) - .forEach( - entry -> { - if (entry.getValue().equals(Delete.SENTINEL)) { - jpaTm().delete(VKey.from(entry.getKey())); - } else { - ((DatastoreEntity) entry.getValue()) - .toSqlEntity() - .ifPresent(jpaTm()::put); - } - }); - }); - } } diff --git a/core/src/main/java/google/registry/model/poll/PollMessage.java b/core/src/main/java/google/registry/model/poll/PollMessage.java index 577a6043a..40d1a19a1 100644 --- a/core/src/main/java/google/registry/model/poll/PollMessage.java +++ b/core/src/main/java/google/registry/model/poll/PollMessage.java @@ -52,6 +52,7 @@ import google.registry.persistence.WithLongVKey; import google.registry.schema.replay.DatastoreAndSqlEntity; import java.util.List; import java.util.Optional; +import javax.annotation.Nullable; import javax.persistence.AttributeOverride; import javax.persistence.AttributeOverrides; import javax.persistence.Column; @@ -185,6 +186,7 @@ public abstract class PollMessage extends ImmutableObject @Override public abstract VKey createVKey(); + /** Static VKey factory method for use by VKeyTranslatorFactory. */ public static VKey createVKey(Key key) { return VKey.create(PollMessage.class, key.getId(), key); } @@ -289,7 +291,7 @@ public abstract class PollMessage extends ImmutableObject @Transient List contactTransferResponses; - @Transient + @Transient @ImmutableObject.DoNotCompare List domainPendingActionNotificationResponses; @Transient List domainTransferResponses; @@ -355,6 +357,11 @@ public abstract class PollMessage extends ImmutableObject return VKey.create(OneTime.class, getId(), Key.create(this)); } + /** Converts an unspecialized VKey<PollMessage> to a VKey of the derived class. */ + public static @Nullable VKey convertVKey(@Nullable VKey key) { + return key == null ? null : VKey.create(OneTime.class, key.getSqlKey(), key.getOfyKey()); + } + @Override public Builder asBuilder() { return new Builder(clone(this)); @@ -456,6 +463,11 @@ public abstract class PollMessage extends ImmutableObject return VKey.create(Autorenew.class, getId(), Key.create(this)); } + /** Converts an unspecialized VKey<PollMessage> to a VKey of the derived class. */ + public static @Nullable VKey convertVKey(VKey key) { + return key == null ? null : VKey.create(Autorenew.class, key.getSqlKey(), key.getOfyKey()); + } + @Override public ImmutableList getResponseData() { // Note that the event time is when the auto-renew occured, so the expiration time in the diff --git a/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java b/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java index 74b2f3adf..4894ef6d9 100644 --- a/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java +++ b/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java @@ -49,7 +49,9 @@ public class DomainTransactionRecord extends ImmutableObject @Id @Ignore + @ImmutableObject.DoNotCompare @GeneratedValue(strategy = GenerationType.IDENTITY) + @ImmutableObject.Insignificant Long id; /** The TLD this record operates on. */ 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 ed3113b9b..9fe12542f 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java +++ b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java @@ -198,6 +198,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor * transaction counts (such as contact or host mutations). */ @Transient // domain-specific + @ImmutableObject.EmptySetToNull protected Set domainTransactionRecords; public long getId() { 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 5c5334e68..bb50dd786 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -184,7 +184,7 @@ class DomainCreateFlowTest extends ResourceFlowTestCaseThis is used to verify that entities stored in both cloud SQL and Datastore are identical. + */ + public void isEqualAcrossDatabases(@Nullable ImmutableObject expected) { + if (actual == null) { + assertThat(expected).isNull(); + } else { + assertThat(expected).isNotNull(); + } + if (actual != null) { + Map actualFields = filterFields(actual, ImmutableObject.DoNotCompare.class); + Map expectedFields = + filterFields(expected, ImmutableObject.DoNotCompare.class); + assertThat(actualFields).containsExactlyEntriesIn(expectedFields); + } + } + public static Correspondence immutableObjectCorrespondence( String... ignoredFields) { return Correspondence.from( @@ -99,4 +121,26 @@ public final class ImmutableObjectSubject extends Subject { } return result; } + + /** Filter out fields with the given annotation. */ + public static Map filterFields( + ImmutableObject original, Class annotation) { + Map originalFields = ModelUtils.getFieldValues(original); + // don't use ImmutableMap or a stream->collect model since we can have nulls + Map result = new LinkedHashMap<>(); + for (Map.Entry entry : originalFields.entrySet()) { + if (!entry.getKey().isAnnotationPresent(annotation)) { + + // 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; + } } diff --git a/core/src/test/java/google/registry/testing/ReplayExtension.java b/core/src/test/java/google/registry/testing/ReplayExtension.java index f2c42dbd5..8ad8c9544 100644 --- a/core/src/test/java/google/registry/testing/ReplayExtension.java +++ b/core/src/test/java/google/registry/testing/ReplayExtension.java @@ -14,7 +14,19 @@ package google.registry.testing; +import static com.google.common.truth.Truth.assertThat; +import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.googlecode.objectify.Key; +import google.registry.model.ImmutableObject; import google.registry.model.ofy.ReplayQueue; +import google.registry.model.ofy.TransactionInfo; +import google.registry.persistence.VKey; +import java.util.Optional; import org.junit.jupiter.api.extension.AfterEachCallback; import org.junit.jupiter.api.extension.BeforeEachCallback; import org.junit.jupiter.api.extension.ExtensionContext; @@ -26,13 +38,26 @@ import org.junit.jupiter.api.extension.ExtensionContext; * 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 { FakeClock clock; + boolean compare; - public ReplayExtension(FakeClock clock) { + private ReplayExtension(FakeClock clock, boolean compare) { this.clock = clock; + this.compare = compare; + } + + public static ReplayExtension createWithCompare(FakeClock clock) { + return new ReplayExtension(clock, true); + } + + public static ReplayExtension createWithoutCompare(FakeClock clock) { + return new ReplayExtension(clock, false); } @Override @@ -51,8 +76,48 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { replayToSql(); } + private static ImmutableSet NON_REPLICATED_TYPES = + ImmutableSet.of( + "PremiumList", + "PremiumListRevision", + "PremiumListEntry", + "ReservedList", + "RdeRevision", + "KmsSecretRevision", + "ServerSecret", + "SignedMarkRevocationList", + "ClaimsListShard", + "TmchCrl", + "EppResourceIndex", + "ForeignKeyIndex", + "ForeignKeyHostIndex", + "ForeignKeyContactIndex", + "ForeignKeyDomainIndex"); + public void replayToSql() { DatabaseHelper.setAlwaysSaveWithBackup(false); - ReplayQueue.replay(); + 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; + } + + VKey vkey = VKey.from(entry.getKey()); + Optional ofyValue = ofyTm().transact(() -> ofyTm().loadByKeyIfPresent(vkey)); + Optional jpaValue = jpaTm().transact(() -> jpaTm().loadByKeyIfPresent(vkey)); + if (entry.getValue().equals(TransactionInfo.Delete.SENTINEL)) { + assertThat(jpaValue.isPresent()).isFalse(); + assertThat(ofyValue.isPresent()).isFalse(); + } else { + assertAboutImmutableObjects() + .that((ImmutableObject) jpaValue.get()) + .isEqualAcrossDatabases((ImmutableObject) ofyValue.get()); + } + } + } } }