Remove compareTo() from DelegationSignerData

The only reason why it existed was so that we could get tests to print information in a consistent order and there are other ways of doing that. By removing compareTo we can use the properties of the extended ImmutableObject properly and properly implement the RFC https://tools.ietf.org/html/rfc5910#page-18

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=240170488

java/google/registry/model/domain/DomainBase.java

@@ -27,7 +27,6 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.CollectionUtils.forceEmptyToNull;
 import static google.registry.util.CollectionUtils.nullToEmpty;
 import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
-import static google.registry.util.CollectionUtils.nullToEmptyImmutableSortedCopy;
 import static google.registry.util.CollectionUtils.union;
 import static google.registry.util.DateTimeUtils.earliestOf;
 import static google.registry.util.DateTimeUtils.isBeforeOrAt;
@@ -254,8 +253,8 @@ public class DomainBase extends EppResource
     return fullyQualifiedDomainName;
   }
 
-  public ImmutableSortedSet<DelegationSignerData> getDsData() {
+  public ImmutableSet<DelegationSignerData> getDsData() {
-    return nullToEmptyImmutableSortedCopy(dsData);
+    return nullToEmptyImmutableCopy(dsData);
   }
 
   public LaunchNotice getLaunchNotice() {

java/google/registry/model/domain/secdns/DelegationSignerData.java

@@ -30,8 +30,7 @@ import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter;
  */
 @Embed
 @XmlType(name = "dsData")
-public class DelegationSignerData
+public class DelegationSignerData extends ImmutableObject {
-    extends ImmutableObject implements Comparable<DelegationSignerData> {
 
   /** The identifier for this particular key in the domain. */
   int keyTag;
@@ -85,11 +84,6 @@ public class DelegationSignerData
     return instance;
   }
 
-  @Override
-  public int compareTo(DelegationSignerData other) {
-    return Integer.compare(getKeyTag(), other.getKeyTag());
-  }
-
   /**
    * Returns the presentation format of this DS record.
    *

javatests/google/registry/flows/domain/DomainUpdateFlowTest.java

@@ -97,7 +97,13 @@ import org.junit.Test;
 public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, DomainBase> {
 
   private static final DelegationSignerData SOME_DSDATA =
-      DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2});
+      DelegationSignerData.create(1, 2, 3, base16().decode("0123"));
+  private static final ImmutableMap<String, String> OTHER_DSDATA_TEMPLATE_MAP =
+      ImmutableMap.of(
+          "KEY_TAG", "12346",
+          "ALG", "3",
+          "DIGEST_TYPE", "1",
+          "DIGEST", "38EC35D5B3A34B44C39B");
 
   ContactResource sh8013Contact;
   ContactResource mak21Contact;
@@ -432,7 +438,16 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
       ImmutableSet<DelegationSignerData> originalDsData,
       ImmutableSet<DelegationSignerData> expectedDsData)
       throws Exception {
-    setEppInput(xmlFilename);
+    doSecDnsSuccessfulTest(xmlFilename, originalDsData, expectedDsData, OTHER_DSDATA_TEMPLATE_MAP);
+  }
+
+  private void doSecDnsSuccessfulTest(
+      String xmlFilename,
+      ImmutableSet<DelegationSignerData> originalDsData,
+      ImmutableSet<DelegationSignerData> expectedDsData,
+      ImmutableMap<String, String> substitutions)
+      throws Exception {
+    setEppInput(xmlFilename, substitutions);
     persistResource(
         newDomainBase(getUniqueIdFromCommand()).asBuilder().setDsData(originalDsData).build());
     assertTransactionalFlow(true);
@@ -453,7 +468,9 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
         "domain_update_dsdata_add.xml",
         null,
         ImmutableSet.of(
-            DelegationSignerData.create(12346, 3, 1, base16().decode("38EC35D5B3A34B44C39B"))));
+            DelegationSignerData.create(12346, 3, 1, base16().decode("38EC35D5B3A34B44C39B"))),
+        ImmutableMap.of(
+            "KEY_TAG", "12346", "ALG", "3", "DIGEST_TYPE", "1", "DIGEST", "38EC35D5B3A34B44C39B"));
   }
 
   @Test
@@ -463,7 +480,66 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
         ImmutableSet.of(SOME_DSDATA),
         ImmutableSet.of(
             SOME_DSDATA,
-            DelegationSignerData.create(12346, 3, 1, base16().decode("38EC35D5B3A34B44C39B"))));
+            DelegationSignerData.create(12346, 3, 1, base16().decode("38EC35D5B3A34B44C39B"))),
+        ImmutableMap.of(
+            "KEY_TAG", "12346", "ALG", "3", "DIGEST_TYPE", "1", "DIGEST", "38EC35D5B3A34B44C39B"));
+  }
+
+  @Test
+  public void testSuccess_secDnsAddSameDoesNothing() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableMap.of("KEY_TAG", "1", "ALG", "2", "DIGEST_TYPE", "3", "DIGEST", "0123"));
+  }
+
+  @Test
+  public void testSuccess_secDnsAddOnlyKeyTagRemainsSame() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(
+            SOME_DSDATA, DelegationSignerData.create(1, 8, 4, base16().decode("4567"))),
+        ImmutableMap.of("KEY_TAG", "1", "ALG", "8", "DIGEST_TYPE", "4", "DIGEST", "4567"));
+  }
+
+  // Changing any of the four fields in DelegationSignerData should result in a new object
+  @Test
+  public void testSuccess_secDnsAddOnlyChangeKeyTag() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(
+            SOME_DSDATA, DelegationSignerData.create(12346, 2, 3, base16().decode("0123"))),
+        ImmutableMap.of("KEY_TAG", "12346", "ALG", "2", "DIGEST_TYPE", "3", "DIGEST", "0123"));
+  }
+
+  @Test
+  public void testSuccess_secDnsAddOnlyChangeAlgorithm() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(SOME_DSDATA, DelegationSignerData.create(1, 8, 3, base16().decode("0123"))),
+        ImmutableMap.of("KEY_TAG", "1", "ALG", "8", "DIGEST_TYPE", "3", "DIGEST", "0123"));
+  }
+
+  @Test
+  public void testSuccess_secDnsAddOnlyChangeDigestType() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(SOME_DSDATA, DelegationSignerData.create(1, 2, 4, base16().decode("0123"))),
+        ImmutableMap.of("KEY_TAG", "1", "ALG", "2", "DIGEST_TYPE", "4", "DIGEST", "0123"));
+  }
+
+  @Test
+  public void testSuccess_secDnsAddOnlyChangeDigest() throws Exception {
+    doSecDnsSuccessfulTest(
+        "domain_update_dsdata_add.xml",
+        ImmutableSet.of(SOME_DSDATA),
+        ImmutableSet.of(SOME_DSDATA, DelegationSignerData.create(1, 2, 3, base16().decode("4567"))),
+        ImmutableMap.of("KEY_TAG", "1", "ALG", "2", "DIGEST_TYPE", "3", "DIGEST", "4567"));
   }
 
   @Test
@@ -699,7 +775,7 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
       builder.add(DelegationSignerData.create(i, 2, 3, new byte[] {0, 1, 2}));
     }
 
-    setEppInput("domain_update_dsdata_add.xml");
+    setEppInput("domain_update_dsdata_add.xml", OTHER_DSDATA_TEMPLATE_MAP);
     persistResource(
         newDomainBase(getUniqueIdFromCommand()).asBuilder().setDsData(builder.build()).build());
     EppException thrown = assertThrows(TooManyDsRecordsException.class, this::runFlow);

javatests/google/registry/flows/domain/testdata/domain_update_dsdata_add.xml

@@ -13,10 +13,10 @@
        xmlns:secDNS="urn:ietf:params:xml:ns:secDNS-1.1">
         <secDNS:add>
           <secDNS:dsData>
-            <secDNS:keyTag>12346</secDNS:keyTag>
+            <secDNS:keyTag>%KEY_TAG%</secDNS:keyTag>
-            <secDNS:alg>3</secDNS:alg>
+            <secDNS:alg>%ALG%</secDNS:alg>
-            <secDNS:digestType>1</secDNS:digestType>
+            <secDNS:digestType>%DIGEST_TYPE%</secDNS:digestType>
-            <secDNS:digest>38EC35D5B3A34B44C39B</secDNS:digest>
+            <secDNS:digest>%DIGEST%</secDNS:digest>
           </secDNS:dsData>
         </secDNS:add>
       </secDNS:update>

javatests/google/registry/tools/GenerateDnsReportCommandTest.java

@@ -30,6 +30,7 @@ import com.beust.jcommander.ParameterException;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
 import com.google.common.net.InetAddresses;
 import com.googlecode.objectify.Key;
 import google.registry.model.domain.DomainBase;
@@ -42,6 +43,7 @@ import java.io.Reader;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.List;
 import org.joda.time.DateTime;
 import org.json.simple.JSONValue;
 import org.json.simple.parser.ParseException;
@@ -72,22 +74,34 @@ public class GenerateDnsReportCommandTest extends CommandTestCase<GenerateDnsRep
   private HostResource nameserver4;
   private DomainBase domain1;
 
+  private static final ImmutableList<?> DS_DATA_OUTPUT = ImmutableList.of(
+      ImmutableMap.of(
+          "keyTag", 12345L,
+          "algorithm", 3L,
+          "digestType", 1L,
+          "digest", "49FD46E6C4B45C55D4AC"),
+      ImmutableMap.of(
+          "keyTag", 56789L,
+          "algorithm", 2L,
+          "digestType", 4L,
+          "digest", "69FD46E6C4A45C55D4AC"));
+
+  private static final List<?> DS_DATA_OUTPUT_REVERSED = Lists.reverse(DS_DATA_OUTPUT);
+
   private static final ImmutableMap<String, ?> DOMAIN1_OUTPUT = ImmutableMap.of(
       "domain", "example.xn--q9jyb4c",
       "nameservers", ImmutableList.of(
           "ns1.example.xn--q9jyb4c",
           "ns2.example.xn--q9jyb4c"),
-      "dsData", ImmutableList.of(
+      "dsData", DS_DATA_OUTPUT);
-          ImmutableMap.of(
+
-              "keyTag", 12345L,
+  // We can't guarantee inner ordering
-              "algorithm", 3L,
+  private static final ImmutableMap<String, ?> DOMAIN1_OUTPUT_ALT = ImmutableMap.of(
-              "digestType", 1L,
+      "domain", "example.xn--q9jyb4c",
-              "digest", "49FD46E6C4B45C55D4AC"),
+      "nameservers", ImmutableList.of(
-          ImmutableMap.of(
+          "ns1.example.xn--q9jyb4c",
-              "keyTag", 56789L,
+          "ns2.example.xn--q9jyb4c"),
-              "algorithm", 2L,
+      "dsData", DS_DATA_OUTPUT_REVERSED);
-              "digestType", 4L,
-              "digest", "69FD46E6C4A45C55D4AC")));
 
   private static final ImmutableMap<String, ?> DOMAIN2_OUTPUT = ImmutableMap.of(
       "domain", "foobar.xn--q9jyb4c",
@@ -146,60 +160,63 @@ public class GenerateDnsReportCommandTest extends CommandTestCase<GenerateDnsRep
   @Test
   public void testSuccess() throws Exception {
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson()).isEqualTo(
+    Iterable<?> output = (Iterable<?>) getOutputAsJson();
-        ImmutableList.of(DOMAIN1_OUTPUT, DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+    assertThat(output).containsAnyOf(DOMAIN1_OUTPUT, DOMAIN1_OUTPUT_ALT);
+    assertThat(output).containsAllOf(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipDeletedDomain() throws Exception {
     persistResource(domain1.asBuilder().setDeletionTime(now).build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    assertThat((Iterable<?>) getOutputAsJson())
-        .isEqualTo(ImmutableList.of(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+        .containsExactly(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipDeletedNameserver() throws Exception {
-    persistResource(
+    persistResource(nameserver1.asBuilder().setDeletionTime(now).build());
-        nameserver1.asBuilder().setDeletionTime(now).build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    Iterable<?> output = (Iterable<?>) getOutputAsJson();
-        .isEqualTo(ImmutableList.of(DOMAIN1_OUTPUT, DOMAIN2_OUTPUT, NAMESERVER2_OUTPUT));
+    assertThat(output).containsAnyOf(DOMAIN1_OUTPUT, DOMAIN1_OUTPUT_ALT);
+    assertThat(output).containsAllOf(DOMAIN2_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipClientHoldDomain() throws Exception {
     persistResource(domain1.asBuilder().addStatusValue(StatusValue.CLIENT_HOLD).build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    assertThat((Iterable<?>) getOutputAsJson())
-        .isEqualTo(ImmutableList.of(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+        .containsExactly(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipServerHoldDomain() throws Exception {
     persistResource(domain1.asBuilder().addStatusValue(StatusValue.SERVER_HOLD).build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    assertThat((Iterable<?>) getOutputAsJson())
-        .isEqualTo(ImmutableList.of(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+        .containsExactly(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipPendingDeleteDomain() throws Exception {
-    persistResource(domain1.asBuilder()
+    persistResource(
-        .addStatusValue(StatusValue.PENDING_DELETE)
+        domain1
-        .setDeletionTime(now.plusDays(30))
+            .asBuilder()
-        .build());
+            .addStatusValue(StatusValue.PENDING_DELETE)
+            .setDeletionTime(now.plusDays(30))
+            .build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    assertThat((Iterable<?>) getOutputAsJson())
-        .isEqualTo(ImmutableList.of(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+        .containsExactly(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test
   public void testSuccess_skipDomainsWithoutNameservers() throws Exception {
     persistResource(domain1.asBuilder().setNameservers(ImmutableSet.of()).build());
     runCommand("--output=" + output, "--tld=xn--q9jyb4c");
-    assertThat(getOutputAsJson())
+    assertThat((Iterable<?>) getOutputAsJson())
-        .isEqualTo(ImmutableList.of(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT));
+        .containsExactly(DOMAIN2_OUTPUT, NAMESERVER1_OUTPUT, NAMESERVER2_OUTPUT);
   }
 
   @Test