Do full database comparison during replay tests (#1524)

* Fix entity delete replication, compare db @ replay

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

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

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

* Exclude tests and entities with failing comparisons

* Get all tests to pass and fix more timestamp

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

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

* Remove unnecessary "NoDatabaseCompare"

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

* Changes for review

* Remove old "compare" flag.

* Reformatted.
This commit is contained in:
Michael Muller 2022-02-22 10:49:57 -05:00 committed by GitHub
parent d355da362f
commit bb7d0b6b89
12 changed files with 244 additions and 70 deletions

View file

@ -1035,17 +1035,20 @@ public class DomainContent extends EppResource
public B setGracePeriods(ImmutableSet<GracePeriod> gracePeriods) {
getInstance().gracePeriods = gracePeriods;
getInstance().resetUpdateTimestamp();
return thisCastToDerived();
}
public B addGracePeriod(GracePeriod gracePeriod) {
getInstance().gracePeriods = union(getInstance().getGracePeriods(), gracePeriod);
getInstance().resetUpdateTimestamp();
return thisCastToDerived();
}
public B removeGracePeriod(GracePeriod gracePeriod) {
getInstance().gracePeriods =
CollectionUtils.difference(getInstance().getGracePeriods(), gracePeriod);
getInstance().resetUpdateTimestamp();
return thisCastToDerived();
}

View file

@ -41,7 +41,7 @@ import org.joda.time.DateTime;
/** Wrapper for {@link Supplier} that associates a time with each attempt. */
@DeleteAfterMigration
class CommitLoggedWork<R> implements Runnable {
public class CommitLoggedWork<R> implements Runnable {
private final Supplier<R> work;
private final Clock clock;

View file

@ -46,6 +46,7 @@ import static google.registry.util.X509Utils.loadCertificate;
import static java.util.Comparator.comparing;
import static java.util.function.Predicate.isEqual;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableList;
@ -824,6 +825,7 @@ public class Registrar extends ImmutableObject
public Builder setAllowedTlds(Set<String> allowedTlds) {
getInstance().allowedTlds = ImmutableSortedSet.copyOf(assertTldsExist(allowedTlds));
getInstance().lastUpdateTime = UpdateAutoTimestamp.create(null);
return this;
}
@ -991,6 +993,16 @@ public class Registrar extends ImmutableObject
return this;
}
/**
* This lets tests set the update timestamp in cases where setting fields resets the timestamp
* and breaks the verification that an object has not been updated since it was copied.
*/
@VisibleForTesting
public Builder setLastUpdateTime(DateTime timestamp) {
getInstance().lastUpdateTime = UpdateAutoTimestamp.create(timestamp);
return this;
}
/** Build the registrar, nullifying empty fields. */
@Override
public Registrar build() {

View file

@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.googlecode.objectify.Key;
import google.registry.model.ImmutableObject;
import google.registry.model.common.DatabaseMigrationStateSchedule;
import google.registry.model.common.DatabaseMigrationStateSchedule.ReplayDirection;
@ -604,6 +605,13 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
managedEntity = getEntityManager().merge(entity);
}
getEntityManager().remove(managedEntity);
// We check shouldReplicate() in TransactionInfo.addDelete(), but we have to check it here as
// well prior to attempting to create a datastore key because a non-replicated entity may not
// have one.
if (shouldReplicate(entity.getClass())) {
transactionInfo.get().addDelete(VKey.from(Key.create(entity)));
}
return managedEntity;
}
@ -827,6 +835,12 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
replaySqlToDatastoreOverrideForTest.set(Optional.empty());
}
/** Returns true if the entity class should be replicated from SQL to datastore. */
private static boolean shouldReplicate(Class<?> entityClass) {
return !NonReplicatedEntity.class.isAssignableFrom(entityClass)
&& !SqlOnlyEntity.class.isAssignableFrom(entityClass);
}
private static class TransactionInfo {
ReadOnlyCheckingEntityManager entityManager;
boolean inTransaction = false;
@ -883,12 +897,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
}
}
/** Returns true if the entity class should be replicated from SQL to datastore. */
private boolean shouldReplicate(Class<?> entityClass) {
return !NonReplicatedEntity.class.isAssignableFrom(entityClass)
&& !SqlOnlyEntity.class.isAssignableFrom(entityClass);
}
private void recordTransaction() {
if (contentsBuilder != null) {
Transaction persistedTxn = contentsBuilder.build();

View file

@ -453,6 +453,13 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
Map<?, ?> diffs =
DiffUtils.deepDiff(
originalRegistrar.toDiffableFieldMap(), updatedRegistrar.toDiffableFieldMap(), true);
// It's expected that the update timestamp will be changed, as it gets reset whenever we change
// nested collections. If it's the only change, just return the original registrar.
if (diffs.keySet().equals(ImmutableSet.of("lastUpdateTime"))) {
return originalRegistrar;
}
throw new ForbiddenException(
String.format("Unauthorized: only %s can change fields %s", allowedRole, diffs.keySet()));
}

View file

@ -553,6 +553,7 @@ class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow, Domain
.hasValue(HistoryEntry.createVKey(Key.create(historyEntry)));
}
// DomainTransactionRecord is not propagated.
@TestOfyAndSql
void testSuccess_validAllocationToken_multiUse() throws Exception {
setEppInput(

View file

@ -109,6 +109,7 @@ import google.registry.persistence.VKey;
import google.registry.testing.DatabaseHelper;
import google.registry.testing.DualDatabaseTest;
import google.registry.testing.ReplayExtension;
import google.registry.testing.ReplayExtension.NoDatabaseCompare;
import google.registry.testing.TestOfyAndSql;
import google.registry.testing.TestOfyOnly;
import java.util.Optional;
@ -954,6 +955,7 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
assertThat(thrown).hasMessageThat().contains("(sh8013)");
}
@NoDatabaseCompare
@TestOfyAndSql
void testFailure_addingDuplicateContact() throws Exception {
persistReferencedEntities();
@ -1281,6 +1283,8 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
assertAboutEppExceptions().that(thrown).marshalsToXml();
}
// Contacts mismatch.
@NoDatabaseCompare
@TestOfyAndSql
void testFailure_sameContactAddedAndRemoved() throws Exception {
setEppInput("domain_update_add_remove_same_contact.xml");

View file

@ -22,6 +22,7 @@ import static google.registry.testing.DatabaseHelper.insertInDb;
import com.google.common.collect.ImmutableSet;
import google.registry.model.ImmutableObject;
import google.registry.model.replay.NonReplicatedEntity;
import google.registry.persistence.transaction.JpaTestExtensions;
import google.registry.persistence.transaction.JpaTestExtensions.JpaUnitTestExtension;
import java.lang.reflect.Method;
@ -168,7 +169,7 @@ class EntityCallbacksListenerTest {
}
@Entity(name = "TestEntity")
private static class TestEntity extends ParentEntity {
private static class TestEntity extends ParentEntity implements NonReplicatedEntity {
@Id String name = "id";
int nonTransientField = 0;

View file

@ -49,7 +49,7 @@ public class JpaEntityCoverageExtension implements BeforeEachCallback, AfterEach
// TransactionEntity is trivial; its persistence is tested in TransactionTest.
"TransactionEntity");
private static final ImmutableSet<Class<?>> ALL_JPA_ENTITIES =
public static final ImmutableSet<Class<?>> ALL_JPA_ENTITIES =
PersistenceXmlUtility.getManagedClasses().stream()
.filter(e -> !IGNORE_ENTITIES.contains(e.getSimpleName()))
.filter(e -> e.isAnnotationPresent(Entity.class))

View file

@ -30,6 +30,7 @@ import com.googlecode.objectify.annotation.Id;
import google.registry.model.ImmutableObject;
import google.registry.model.ofy.DatastoreTransactionManager;
import google.registry.model.ofy.Ofy;
import google.registry.model.replay.NonReplicatedEntity;
import google.registry.persistence.VKey;
import google.registry.persistence.transaction.TransactionManagerFactory.ReadOnlyModeException;
import google.registry.testing.AppEngineExtension;
@ -448,7 +449,7 @@ public class TransactionManagerTest {
@Entity(name = "TxnMgrTestEntity")
@javax.persistence.Entity(name = "TestEntity")
private static class TestEntity extends TestEntityBase {
private static class TestEntity extends TestEntityBase implements NonReplicatedEntity {
private String data;

View file

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

View file

@ -267,9 +267,16 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
Map<String, Object> response =
action.handleJsonRequest(
ImmutableMap.of(
"op", "update",
"id", CLIENT_ID,
"args", setter.apply(registrar.asBuilder(), newValue).build().toJsonMap()));
"op",
"update",
"id",
CLIENT_ID,
"args",
setter
.apply(registrar.asBuilder(), newValue)
.setLastUpdateTime(registrar.getLastUpdateTime())
.build()
.toJsonMap()));
Registrar updatedRegistrar = loadRegistrar(CLIENT_ID);
persistResource(registrar);
@ -320,7 +327,11 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
"id",
CLIENT_ID,
"args",
setter.apply(registrar.asBuilder(), newValue).build().toJsonMap()));
setter
.apply(registrar.asBuilder(), newValue)
.setLastUpdateTime(registrar.getLastUpdateTime())
.build()
.toJsonMap()));
Registrar updatedRegistrar = loadRegistrar(CLIENT_ID);
persistResource(registrar);