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