Check for diffs in ConfigureTldCommand (#2146)

* Check for diffs in ConfigureTldCommand

* undo override

* Add handling for ordering sets

* Fix comments

* fix formatting

* fix test

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

@@ -14,6 +14,7 @@
 package google.registry.model;
 
 import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
+import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
 import static com.google.common.collect.Ordering.natural;
 
 import com.fasterxml.jackson.core.JsonGenerator;
@@ -29,6 +30,7 @@ import com.fasterxml.jackson.databind.module.SimpleModule;
 import com.fasterxml.jackson.databind.ser.std.StdSerializer;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 import com.fasterxml.jackson.dataformat.yaml.YAMLGenerator.Feature;
+import com.google.common.collect.ImmutableSortedSet;
 import google.registry.model.common.TimedTransitionProperty;
 import google.registry.model.domain.token.AllocationToken;
 import google.registry.model.tld.Tld.TldState;
@@ -39,6 +41,7 @@ import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.SortedMap;
 import org.joda.money.CurrencyUnit;
 import org.joda.money.Money;
@@ -65,6 +68,57 @@ public class EntityYamlUtils {
     return mapper;
   }
 
+  /**
+   * A custom serializer for String Set to sort the order and make YAML generation deterministic.
+   */
+  public static class SortedSetSerializer extends StdSerializer<Set<String>> {
+    public SortedSetSerializer() {
+      this(null);
+    }
+
+    public SortedSetSerializer(Class<Set<String>> t) {
+      super(t);
+    }
+
+    @Override
+    public void serialize(Set<String> value, JsonGenerator g, SerializerProvider provider)
+        throws IOException {
+      ImmutableSortedSet<String> sorted =
+          value.stream()
+              .collect(toImmutableSortedSet(String::compareTo)); // sort the entries into a new set
+      g.writeStartArray();
+      for (String entry : sorted) {
+        g.writeString(entry);
+      }
+      g.writeEndArray();
+    }
+  }
+
+  /** A custom serializer for Enum Set to sort the order and make YAML generation deterministic. */
+  public static class SortedEnumSetSerializer extends StdSerializer<Set<Enum>> {
+    public SortedEnumSetSerializer() {
+      this(null);
+    }
+
+    public SortedEnumSetSerializer(Class<Set<Enum>> t) {
+      super(t);
+    }
+
+    @Override
+    public void serialize(Set<Enum> value, JsonGenerator g, SerializerProvider provider)
+        throws IOException {
+      ImmutableSortedSet<String> sorted =
+          value.stream()
+              .map(Enum::name)
+              .collect(toImmutableSortedSet(String::compareTo)); // sort the entries into a new set
+      g.writeStartArray();
+      for (String entry : sorted) {
+        g.writeString(entry);
+      }
+      g.writeEndArray();
+    }
+  }
+
   /** A custom JSON serializer for {@link Money}. */
   public static class MoneySerializer extends StdSerializer<Money> {
 

core/src/main/java/google/registry/model/tld/Tld.java

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Maps.toMap;
 import static google.registry.config.RegistryConfig.getSingletonCacheRefreshDuration;
+import static google.registry.model.EntityYamlUtils.createObjectMapper;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
@@ -28,6 +29,8 @@ import static org.joda.money.CurrencyUnit.USD;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 import com.github.benmanes.caffeine.cache.CacheLoader;
@@ -50,6 +53,8 @@ import google.registry.model.EntityYamlUtils.CurrencyDeserializer;
 import google.registry.model.EntityYamlUtils.CurrencySerializer;
 import google.registry.model.EntityYamlUtils.OptionalDurationSerializer;
 import google.registry.model.EntityYamlUtils.OptionalStringSerializer;
+import google.registry.model.EntityYamlUtils.SortedEnumSetSerializer;
+import google.registry.model.EntityYamlUtils.SortedSetSerializer;
 import google.registry.model.EntityYamlUtils.TimedTransitionPropertyMoneyDeserializer;
 import google.registry.model.EntityYamlUtils.TimedTransitionPropertyTldStateDeserializer;
 import google.registry.model.EntityYamlUtils.TokenVKeyListDeserializer;
@@ -124,6 +129,20 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl
   public static final Money DEFAULT_SERVER_STATUS_CHANGE_BILLING_COST = Money.of(USD, 20);
   public static final Money DEFAULT_REGISTRY_LOCK_OR_UNLOCK_BILLING_COST = Money.of(USD, 0);
 
+  public boolean equalYaml(Tld tldToCompare) {
+    if (this == tldToCompare) {
+      return true;
+    }
+    ObjectMapper mapper = createObjectMapper();
+    try {
+      String thisYaml = mapper.writeValueAsString(this);
+      String otherYaml = mapper.writeValueAsString(tldToCompare);
+      return thisYaml.equals(otherYaml);
+    } catch (JsonProcessingException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
   /** The type of TLD, which determines things like backups and escrow policy. */
   public enum TldType {
     /**
@@ -255,6 +274,7 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl
    * <p>All entries of this list must be valid keys for the map of {@code DnsWriter}s injected by
    * {@code @Inject Map<String, DnsWriter>}
    */
+  @JsonSerialize(using = SortedSetSerializer.class)
   @Column(nullable = false)
   Set<String> dnsWriters;
 
@@ -354,6 +374,7 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl
   CreateAutoTimestamp creationTime = CreateAutoTimestamp.create(null);
 
   /** The set of reserved list names that are applicable to this tld. */
+  @JsonSerialize(using = SortedSetSerializer.class)
   @Column(name = "reserved_list_names")
   Set<String> reservedListNames;
 
@@ -493,10 +514,14 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl
   DateTime claimsPeriodEnd = END_OF_TIME;
 
   /** An allowlist of clients allowed to be used on domains on this TLD (ignored if empty). */
-  @Nullable Set<String> allowedRegistrantContactIds;
+  @Nullable
+  @JsonSerialize(using = SortedSetSerializer.class)
+  Set<String> allowedRegistrantContactIds;
 
   /** An allowlist of hosts allowed to be used on domains on this TLD (ignored if empty). */
-  @Nullable Set<String> allowedFullyQualifiedHostNames;
+  @Nullable
+  @JsonSerialize(using = SortedSetSerializer.class)
+  Set<String> allowedFullyQualifiedHostNames;
 
   /**
    * Indicates when the TLD is being modified using locally modified files to override the source
@@ -521,6 +546,7 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl
   List<VKey<AllocationToken>> defaultPromoTokens;
 
   /** A set of allowed {@link IdnTableEnum}s for this TLD, or empty if we should use the default. */
+  @JsonSerialize(using = SortedEnumSetSerializer.class)
   Set<IdnTableEnum> idnTables;
 
   public String getTldStr() {

core/src/main/java/google/registry/tools/ConfigureTldCommand.java

@@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Sets.SetView;
+import com.google.common.flogger.FluentLogger;
 import google.registry.model.tld.Tld;
 import google.registry.model.tld.label.PremiumList;
 import google.registry.model.tld.label.PremiumListDao;
@@ -53,6 +54,8 @@ import org.yaml.snakeyaml.Yaml;
 @Parameters(separators = " =", commandDescription = "Create or update TLD using YAML")
 public class ConfigureTldCommand extends MutatingCommand {
 
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   @Parameter(
       names = {"-i", "--input"},
       description = "Filename of TLD YAML file.",
@@ -66,6 +69,9 @@ public class ConfigureTldCommand extends MutatingCommand {
   @Named("dnsWriterNames")
   Set<String> validDnsWriterNames;
 
+  /** Indicates if the passed in file contains new changes to the TLD */
+  boolean newDiff = false;
+
   // TODO(sarahbot@): Add a breakglass setting to this tool to indicate when a TLD has been modified
   // outside of source control
 
@@ -80,12 +86,25 @@ public class ConfigureTldCommand extends MutatingCommand {
     checkForMissingFields(tldData);
     Tld oldTld = getTlds().contains(name) ? Tld.get(name) : null;
     Tld newTld = mapper.readValue(inputFile.toFile(), Tld.class);
+    if (oldTld != null && oldTld.equalYaml(newTld)) {
+      return;
+    }
+    newDiff = true;
     checkPremiumList(newTld);
     checkDnsWriters(newTld);
     checkCurrency(newTld);
     stageEntityChange(oldTld, newTld);
   }
 
+  @Override
+  protected boolean dontRunCommand() {
+    if (!newDiff) {
+      logger.atInfo().log("TLD YAML file contains no new changes");
+      return true;
+    }
+    return false;
+  }
+
   private void checkName(String name, Map<String, Object> tldData) {
     checkArgument(CharMatcher.ascii().matchesAllOf(name), "A TLD name must be in plain ASCII");
     checkArgument(!Character.isDigit(name.charAt(0)), "TLDs cannot begin with a number");

core/src/test/java/google/registry/tools/ConfigureTldCommandTest.java

@@ -14,14 +14,18 @@
 package google.registry.tools;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.EntityYamlUtils.createObjectMapper;
 import static google.registry.model.domain.token.AllocationToken.TokenType.DEFAULT_PROMO;
 import static google.registry.testing.DatabaseHelper.createTld;
 import static google.registry.testing.DatabaseHelper.persistPremiumList;
 import static google.registry.testing.DatabaseHelper.persistResource;
+import static google.registry.testing.LogsSubject.assertAboutLogs;
 import static google.registry.testing.TestDataHelper.loadFile;
 import static google.registry.tldconfig.idn.IdnTableEnum.EXTENDED_LATIN;
 import static google.registry.tldconfig.idn.IdnTableEnum.JA;
+import static google.registry.tldconfig.idn.IdnTableEnum.UNCONFUSABLE_LATIN;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.logging.Level.INFO;
 import static org.joda.money.CurrencyUnit.JPY;
 import static org.joda.money.CurrencyUnit.USD;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -34,12 +38,13 @@ import com.google.common.base.Ascii;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.Files;
-import google.registry.model.EntityYamlUtils;
+import com.google.common.testing.TestLogHandler;
 import google.registry.model.domain.token.AllocationToken;
 import google.registry.model.tld.Tld;
 import google.registry.model.tld.label.PremiumList;
 import google.registry.model.tld.label.PremiumListDao;
 import java.io.File;
+import java.util.logging.Logger;
 import org.joda.money.Money;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
@@ -50,7 +55,9 @@ import org.testcontainers.shaded.com.google.common.collect.ImmutableMap;
 public class ConfigureTldCommandTest extends CommandTestCase<ConfigureTldCommand> {
 
   PremiumList premiumList;
-  ObjectMapper objectMapper = EntityYamlUtils.createObjectMapper();
+  ObjectMapper objectMapper = createObjectMapper();
+  private final TestLogHandler logHandler = new TestLogHandler();
+  private final Logger logger = Logger.getLogger(ConfigureTldCommand.class.getCanonicalName());
 
   @BeforeEach
   void beforeEach() {
@@ -89,6 +96,25 @@ public class ConfigureTldCommandTest extends CommandTestCase<ConfigureTldCommand
     testTldConfiguredSuccessfully(updatedTld, "tld.yaml");
   }
 
+  @Test
+  void testSuccess_noDiff() throws Exception {
+    logger.addHandler(logHandler);
+    Tld tld = createTld("idns");
+    tld =
+        persistResource(
+            tld.asBuilder()
+                .setIdnTables(ImmutableSet.of(JA, UNCONFUSABLE_LATIN, EXTENDED_LATIN))
+                .setAllowedFullyQualifiedHostNames(
+                    ImmutableSet.of("zeta", "alpha", "gamma", "beta"))
+                .build());
+    File tldFile = tmpDir.resolve("idns.yaml").toFile();
+    Files.asCharSink(tldFile, UTF_8).write(loadFile(getClass(), "idns.yaml"));
+    runCommandForced("--input=" + tldFile);
+    assertAboutLogs()
+        .that(logHandler)
+        .hasLogAtLevelWithMessage(INFO, "TLD YAML file contains no new changes");
+  }
+
   @Test
   void testSuccess_outOfOrderFieldsOnCreate() throws Exception {
     File tldFile = tmpDir.resolve("outoforderfields.yaml").toFile();

core/src/test/resources/google/registry/model/tld/tld.yaml

@@ -18,8 +18,8 @@ dnsDsTtl: null
 dnsNsTtl: null
 dnsPaused: false
 dnsWriters:
-- "baz"
 - "bang"
+- "baz"
 driveFolderId: null
 eapFeeSchedule:
   "1970-01-01T00:00:00.000Z":
@@ -33,8 +33,8 @@ eapFeeSchedule:
     amount: 0.00
 escrowEnabled: false
 idnTables:
-- "JA"
 - "EXTENDED_LATIN"
+- "JA"
 invoicingEnabled: false
 lordnUsername: null
 numDnsPublishLocks: 1

core/src/test/resources/google/registry/tools/idns.yaml

@@ -0,0 +1,62 @@
+addGracePeriodLength: 432000000
+allowedFullyQualifiedHostNames:
+- "beta"
+- "zeta"
+- "alpha"
+- "gamma"
+allowedRegistrantContactIds: []
+anchorTenantAddGracePeriodLength: 2592000000
+autoRenewGracePeriodLength: 3888000000
+automaticTransferLength: 432000000
+claimsPeriodEnd: "294247-01-10T04:00:54.775Z"
+createBillingCost:
+  currency: "USD"
+  amount: 13.00
+creationTime: "2022-09-01T00:00:00.000Z"
+currency: "USD"
+defaultPromoTokens: []
+dnsAPlusAaaaTtl: null
+dnsDsTtl: null
+dnsNsTtl: null
+dnsPaused: false
+dnsWriters:
+- "VoidDnsWriter"
+driveFolderId: null
+eapFeeSchedule:
+  "1970-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: 0.00
+escrowEnabled: false
+idnTables:
+- "EXTENDED_LATIN"
+- "JA"
+- "UNCONFUSABLE_LATIN"
+invoicingEnabled: false
+lordnUsername: null
+numDnsPublishLocks: 1
+pendingDeleteLength: 432000000
+premiumListName: "idns"
+pricingEngineClassName: "google.registry.model.pricing.StaticPremiumListPricingEngine"
+redemptionGracePeriodLength: 2592000000
+registryLockOrUnlockBillingCost:
+  currency: "USD"
+  amount: 0.00
+renewBillingCostTransitions:
+  "1970-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: 11.00
+renewGracePeriodLength: 432000000
+reservedListNames: []
+restoreBillingCost:
+  currency: "USD"
+  amount: 17.00
+roidSuffix: "IDNS"
+serverStatusChangeBillingCost:
+  currency: "USD"
+  amount: 19.00
+tldStateTransitions:
+  "1970-01-01T00:00:00.000Z": "GENERAL_AVAILABILITY"
+tldStr: "idns"
+tldType: "REAL"
+tldUnicode: "idns"
+transferGracePeriodLength: 432000000