From 633eb3179a45e4f87fee924b20b18f2c109fbb8f Mon Sep 17 00:00:00 2001
From: guyben <guyben@google.com>
Date: Tue, 19 Dec 2017 01:53:58 -0800
Subject: [PATCH] Skip RRS update if existing records are equal to desired
 records

This is done first and formost to stop "empty" commits that cause errors in
publishDnsUpdates. The reason being that the Cloud DNS api fails when there are
no updates at all in a change.

Allowing this is a requirement for the writer to be idempotent - if we delete a
domain, then run the writer to delete it again - we'll get 0 additions and 0
deletions which fails.

This isn't theoretical either - we've seen it happen, causing a
publishDnsUpdates to fail over and over again.

While fixing this, we also remove all RRS that are common between additions and
deletions. This is just an optimization and shouldn't affect behavior.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=179525218
---
 .../dns/writer/clouddns/CloudDnsWriter.java   | 19 ++++++++++++++++++-
 .../writer/clouddns/CloudDnsWriterTest.java   |  6 ++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java b/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
index b9979085f..52664ff53 100644
--- a/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
+++ b/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
@@ -28,6 +28,7 @@ import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.google.common.net.InternetDomainName;
 import com.google.common.util.concurrent.RateLimiter;
 import google.registry.config.RegistryConfig.Config;
@@ -379,7 +380,23 @@ public class CloudDnsWriter extends BaseDnsWriter {
    */
   private void updateResourceRecords(
       ImmutableSet<ResourceRecordSet> additions, ImmutableSet<ResourceRecordSet> deletions) {
-    Change change = new Change().setAdditions(additions.asList()).setDeletions(deletions.asList());
+    // Find records that are both in additions and deletions, so we can remove them from both before
+    // requesting the change. This is mostly for optimization reasons - not doing so doesn't affect
+    // the result.
+    ImmutableSet<ResourceRecordSet> intersection =
+        Sets.intersection(additions, deletions).immutableCopy();
+    logger.infofmt(
+        "There are %s common items out of the %s items in 'additions' and %s items in 'deletions'",
+        intersection.size(), additions.size(), deletions.size());
+    // Exit early if we have nothing to update - dnsConnection doesn't work on empty changes
+    if (additions.equals(deletions)) {
+      logger.infofmt("Returning early because additions is the same as deletions");
+      return;
+    }
+    Change change =
+        new Change()
+            .setAdditions(ImmutableList.copyOf(Sets.difference(additions, intersection)))
+            .setDeletions(ImmutableList.copyOf(Sets.difference(deletions, intersection)));
 
     rateLimiter.acquire();
     try {
diff --git a/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java b/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
index 494b94f81..eeae299ba 100644
--- a/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
+++ b/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
@@ -490,4 +490,10 @@ public class CloudDnsWriterTest {
     writer.commit();
     assertThat(zoneNameCaptor.getValue()).isEqualTo("triple-secret-tld");
   }
+
+  @Test
+  public void testEmptyCommit() {
+    writer.commit();
+    verify(dnsConnection, times(0)).changes();
+  }
 }