From 187432890a87b90d56e67cd915d6b24fa2cee55f Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Wed, 23 Mar 2022 13:52:31 -0400 Subject: [PATCH] Ignore trivial differences when comparing DB (#1567) * Ignore trivial differences when comparing DB Some data difference are due to entity model differences and also harmless. We should igore them when comparing Datastore and SQL. This PR ignores the following diffs: - null vs empty collection - the empty string in Address.stree field, which is a list --- .../beam/comparedb/ValidateSqlUtils.java | 50 ++++++++++++++++++- .../beam/comparedb/ValidateSqlUtilsTest.java | 33 ++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/google/registry/beam/comparedb/ValidateSqlUtils.java b/core/src/main/java/google/registry/beam/comparedb/ValidateSqlUtils.java index db5cd45d7..71ec8bd38 100644 --- a/core/src/main/java/google/registry/beam/comparedb/ValidateSqlUtils.java +++ b/core/src/main/java/google/registry/beam/comparedb/ValidateSqlUtils.java @@ -20,6 +20,8 @@ import static google.registry.persistence.transaction.TransactionManagerFactory. import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; +import com.google.common.collect.Maps.EntryTransformer; import google.registry.beam.initsql.Transforms; import google.registry.config.RegistryEnvironment; import google.registry.model.EppResource; @@ -37,19 +39,24 @@ import google.registry.model.registrar.Registrar; import google.registry.model.replay.SqlEntity; import google.registry.model.reporting.HistoryEntry; import google.registry.util.DiffUtils; +import java.io.Serializable; import java.lang.reflect.Field; import java.math.BigInteger; +import java.util.Collection; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import org.apache.beam.sdk.metrics.Counter; import org.apache.beam.sdk.metrics.Metrics; import org.apache.beam.sdk.transforms.DoFn; import org.apache.beam.sdk.values.KV; import org.apache.beam.sdk.values.TupleTag; +import org.checkerframework.checker.nullness.qual.Nullable; /** Helpers for use by {@link ValidateDatabasePipeline}. */ @DeleteAfterMigration @@ -194,13 +201,52 @@ final class ValidateSqlUtils { return; } - if (!Objects.equals(entity0, entity1)) { + Map fields0 = + Maps.transformEntries( + ((ImmutableObject) entity0).toDiffableFieldMap(), new DiffableFieldNormalizer()); + Map fields1 = + Maps.transformEntries( + ((ImmutableObject) entity1).toDiffableFieldMap(), new DiffableFieldNormalizer()); + if (!Objects.equals(fields0, fields1)) { unequalCounters.get(counterKey).inc(); - out.output(unEqualEntityLog(kv.getKey(), entities.get(0), entities.get(1))); + out.output(kv.getKey() + " " + DiffUtils.prettyPrintEntityDeepDiff(fields0, fields1)); } } } + /** + * Normalizes trivial differences between objects persisted in Datastore and SQL. + * + *

This class works on a map generated by {@link ImmutableObject#toDiffableFieldMap}, and + * performs the following changes: + * + *

    + *
  • If a value is an empty {@link Collection}, it is converted to null + *
  • For each {@link google.registry.model.eppcommon.Address} object, empty strings are + * removed from its {@code string} field, which is a {@link List}. + *
+ */ + static class DiffableFieldNormalizer + implements EntryTransformer, Serializable { + + @Override + public Object transformEntry(String key, @Nullable Object value) { + if (value instanceof Collection && ((Collection) value).isEmpty()) { + return null; + } + if (key.equals("street") && value instanceof List) { + return ((List) value) + .stream().filter(v -> v != null && !Objects.equals("", v)).collect(Collectors.toList()); + } + // Short-term hack: LinkedHashMap represents a child ImmutableObject instance. + if (value instanceof LinkedHashMap + && ((LinkedHashMap) value).keySet().stream().anyMatch(e -> e instanceof String)) { + return Maps.transformEntries((Map) value, this); + } + return value; + } + } + static SqlEntity normalizeEntity(SqlEntity sqlEntity) { if (sqlEntity instanceof EppResource) { return normalizeEppResource(sqlEntity); diff --git a/core/src/test/java/google/registry/beam/comparedb/ValidateSqlUtilsTest.java b/core/src/test/java/google/registry/beam/comparedb/ValidateSqlUtilsTest.java index c45af8d68..131daf64e 100644 --- a/core/src/test/java/google/registry/beam/comparedb/ValidateSqlUtilsTest.java +++ b/core/src/test/java/google/registry/beam/comparedb/ValidateSqlUtilsTest.java @@ -19,11 +19,19 @@ import static google.registry.beam.comparedb.ValidateSqlUtils.getMedianIdForHist import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static org.joda.time.DateTimeZone.UTC; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; import com.google.common.truth.Truth; +import google.registry.beam.comparedb.ValidateSqlUtils.DiffableFieldNormalizer; import google.registry.model.bulkquery.TestSetupHelper; +import google.registry.model.contact.ContactAddress; +import google.registry.model.contact.ContactResource; +import google.registry.model.contact.PostalInfo; import google.registry.model.domain.DomainHistory; import google.registry.testing.AppEngineExtension; import google.registry.testing.FakeClock; +import google.registry.util.DiffUtils; +import java.util.Map; import org.joda.time.DateTime; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -60,4 +68,29 @@ class ValidateSqlUtilsTest { assertThat(getMedianIdForHistoryTable("DomainHistory")) .hasValue(setupHelper.domainHistory.getId()); } + + @Test + void diffableFieldNormalizer() { + ContactResource contactResource = + new ContactResource.Builder() + .setLocalizedPostalInfo( + new PostalInfo.Builder() + .setType(PostalInfo.Type.LOCALIZED) + .setAddress( + new ContactAddress.Builder() + .setStreet(ImmutableList.of("111 8th Ave", "")) + .setCity("New York") + .setState("NY") + .setZip("10011") + .setCountryCode("US") + .build()) + .build()) + .build(); + Map origMap = contactResource.toDiffableFieldMap(); + Map trimmedMap = Maps.transformEntries(origMap, new DiffableFieldNormalizer()); + // In the trimmed map, localizedPostalInfo.address.street only has one element in the list, + // thus the output: 'null -> ' + Truth.assertThat(DiffUtils.prettyPrintEntityDeepDiff(trimmedMap, origMap)) + .isEqualTo("localizedPostalInfo.address.street.1: null -> \n"); + } }