From 6466ad51f6f9cb6f6b19f4f78a6774f340ed5e98 Mon Sep 17 00:00:00 2001
From: nickfelt <nickfelt@google.com>
Date: Tue, 14 Jun 2016 13:18:47 -0700
Subject: [PATCH] Fix entry-overwriting bug in
 PremiumList.saveAndUpdateEntries()

The saveAndUpdateEntries() method was always saving entries under the current entity's revisionKey, and then always deleting them under the old entity's revisionKey - even if the revisionKeys were the same (i.e., we didn't change any entry content in the list, in which case the builder's build() method doesn't create a new revisionKey).

This would have caused the existing entries to be unnecessarily resaved, and (worse) then caused those same existing entries to all be deleted.

This fixes the bug by only resaving entries when the revisionKey has changed.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124873871
---
 .../model/registry/label/PremiumList.java     | 27 +++++++++------
 .../model/registry/label/PremiumListTest.java | 34 +++++++++++++++++++
 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/java/google/registry/model/registry/label/PremiumList.java b/java/google/registry/model/registry/label/PremiumList.java
index 6e715d02b..cb913253e 100644
--- a/java/google/registry/model/registry/label/PremiumList.java
+++ b/java/google/registry/model/registry/label/PremiumList.java
@@ -266,14 +266,21 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
    */
   public PremiumList saveAndUpdateEntries() {
     final Optional<PremiumList> oldPremiumList = get(name);
-    // Save the new child entities in a series of transactions.
-    for (final List<PremiumListEntry> batch
-        : partition(premiumListMap.values(), TRANSACTION_BATCH_SIZE)) {
-      ofy().transactNew(new VoidWork() {
-        @Override
-        public void vrun() {
-          ofy().save().entities(batch);
-        }});
+    // Only update entries if there's actually a new revision of the list to save (which there will
+    // be if the list content changes, vs just the description/metadata).
+    boolean entriesToUpdate =
+        !oldPremiumList.isPresent()
+            || !Objects.equals(oldPremiumList.get().revisionKey, this.revisionKey);
+    // If needed, save the new child entities in a series of transactions.
+    if (entriesToUpdate) {
+      for (final List<PremiumListEntry> batch
+          : partition(premiumListMap.values(), TRANSACTION_BATCH_SIZE)) {
+        ofy().transactNew(new VoidWork() {
+          @Override
+          public void vrun() {
+            ofy().save().entities(batch);
+          }});
+      }
     }
     // Save the new PremiumList itself.
     PremiumList updated = ofy().transactNew(new Work<PremiumList>() {
@@ -296,8 +303,8 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
         }});
     // Update the cache.
     PremiumList.cache.put(name, updated);
-    // Delete the entities under the old PremiumList, if any.
-    if (oldPremiumList.isPresent()) {
+    // If needed and there are any, delete the entities under the old PremiumList.
+    if (entriesToUpdate && oldPremiumList.isPresent()) {
       oldPremiumList.get().deleteEntries();
     }
     return updated;
diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java
index a84f5037f..695742409 100644
--- a/javatests/google/registry/model/registry/label/PremiumListTest.java
+++ b/javatests/google/registry/model/registry/label/PremiumListTest.java
@@ -179,6 +179,40 @@ public class PremiumListTest {
     assertThat(entry.label).isEqualTo("lol");
   }
 
+  @Test
+  public void test_saveAndUpdateEntries_twiceOnUnchangedList() throws Exception {
+    PremiumList pl =
+        new PremiumList.Builder()
+            .setName("pl")
+            .setPremiumListMapFromLines(ImmutableList.of("test,USD 1"))
+            .build()
+            .saveAndUpdateEntries();
+    Map<String, PremiumListEntry> entries = pl.getPremiumListEntries();
+    assertThat(entries.keySet()).containsExactly("test");
+    assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
+    // Save again with no changes, and clear the cache to force a re-load from datastore.
+    pl.saveAndUpdateEntries();
+    ofy().clearSessionCache();
+    assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
+  }
+
+  @Test
+  public void test_saveAndUpdateEntries_twiceOnListWithOnlyMetadataChanges() throws Exception {
+    PremiumList pl =
+        new PremiumList.Builder()
+            .setName("pl")
+            .setPremiumListMapFromLines(ImmutableList.of("test,USD 1"))
+            .build()
+            .saveAndUpdateEntries();
+    Map<String, PremiumListEntry> entries = pl.getPremiumListEntries();
+    assertThat(entries.keySet()).containsExactly("test");
+    assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
+    // Save again with description changed, and clear the cache to force a re-load from datastore.
+    pl.asBuilder().setDescription("foobar").build().saveAndUpdateEntries();
+    ofy().clearSessionCache();
+    assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
+  }
+
   @Test
   public void testDelete() throws Exception {
     persistPremiumList("gtld1", "trombone,USD 10");