Clarify when to use cache (or not) when loading premium lists

You don't want to use the cache when loading them for the purposes of updating
them, but you definitely do still want to use the cache when checking the
price of individual domains.

In [] the cache clearing of premium lists on update was removed. This
is a good thing in aggregate because the cache is per-instance and thus
misleading, but it also caused us to not be able to update the same premium
list twice within an hour because the second update would hit a "PremiumList
was concurrently edited" exception, owing to first loading the stale version
from the cache for the purposes of updating it. Now we bypass the cache for
that purpose.

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

java/google/registry/model/registry/label/PremiumList.java

@@ -138,19 +138,15 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
           .build(
               new CacheLoader<String, PremiumList>() {
                 @Override
-                public PremiumList load(final String listName) {
+                public PremiumList load(final String name) {
-                  return ofy()
+                  return ofy().doTransactionless(() -> loadPremiumList(name));
-                      .doTransactionless(
-                          () ->
-                              ofy()
-                                  .load()
-                                  .type(PremiumList.class)
-                                  .parent(getCrossTldKey())
-                                  .id(listName)
-                                  .now());
                 }
               });
 
+  private static PremiumList loadPremiumList(String name) {
+    return ofy().load().type(PremiumList.class).parent(getCrossTldKey()).id(name).now();
+  }
+
   /**
    * In-memory cache for {@link PremiumListRevision}s, used for retrieving Bloom filters quickly.
    *
@@ -211,8 +207,8 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
     return revisionKey;
   }
 
-  /** Returns the PremiumList with the specified name. */
+  /** Returns the PremiumList with the specified name, from cache. */
-  public static Optional<PremiumList> get(String name) {
+  public static Optional<PremiumList> getCached(String name) {
     try {
       return Optional.of(cachePremiumLists.get(name));
     } catch (InvalidCacheLoadException e) {
@@ -222,6 +218,11 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
     }
   }
 
+  /** Returns the PremiumList with the specified name, uncached. */
+  public static Optional<PremiumList> getUncached(String name) {
+    return Optional.ofNullable(loadPremiumList(name));
+  }
+
   /**
    * A premium list entry entity, persisted to Datastore.  Each instance represents the price of a
    * single label on a given TLD.

java/google/registry/model/registry/label/PremiumListUtils.java

@@ -76,7 +76,7 @@ public final class PremiumListUtils {
     }
     DateTime startTime = DateTime.now(UTC);
     String listName = registry.getPremiumList().getName();
-    Optional<PremiumList> optionalPremiumList = PremiumList.get(listName);
+    Optional<PremiumList> optionalPremiumList = PremiumList.getCached(listName);
     checkState(optionalPremiumList.isPresent(), "Could not load premium list '%s'", listName);
     PremiumList premiumList = optionalPremiumList.get();
     PremiumListRevision revision;
@@ -141,7 +141,7 @@ public final class PremiumListUtils {
   public static PremiumList savePremiumListAndEntries(
       final PremiumList premiumList,
       ImmutableMap<String, PremiumListEntry> premiumListEntries) {
-    final Optional<PremiumList> oldPremiumList = PremiumList.get(premiumList.getName());
+    final Optional<PremiumList> oldPremiumList = PremiumList.getUncached(premiumList.getName());
 
     // Create the new revision (with its Bloom filter) and parent the entries on it.
     final PremiumListRevision newRevision =
@@ -151,13 +151,8 @@ public final class PremiumListUtils {
         parentPremiumListEntriesOnRevision(premiumListEntries.values(), newRevisionKey);
 
     // Save the new child entities in a series of transactions.
-    for (final List<PremiumListEntry> batch :
+    for (final List<PremiumListEntry> batch : partition(parentedEntries, TRANSACTION_BATCH_SIZE)) {
-        partition(parentedEntries, TRANSACTION_BATCH_SIZE)) {
+      ofy().transactNew(() -> ofy().save().entities(batch));
-      ofy().transactNew(new VoidWork() {
-        @Override
-        public void vrun() {
-          ofy().save().entities(batch);
-        }});
     }
 
     // Save the new PremiumList and revision itself.

java/google/registry/tools/CreateOrUpdateTldCommand.java

@@ -362,8 +362,9 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
 
       if (premiumListName != null) {
         if (premiumListName.isPresent()) {
-          Optional<PremiumList> premiumList = PremiumList.get(premiumListName.get());
+          Optional<PremiumList> premiumList = PremiumList.getUncached(premiumListName.get());
-          checkArgument(premiumList.isPresent(),
+          checkArgument(
+              premiumList.isPresent(),
               String.format("The premium list '%s' doesn't exist", premiumListName.get()));
           builder.setPremiumList(premiumList.get());
         } else {

java/google/registry/tools/DeletePremiumListCommand.java

@@ -48,7 +48,7 @@ final class DeletePremiumListCommand extends ConfirmingCommand implements Remote
         doesPremiumListExist(name),
         "Cannot delete the premium list %s because it doesn't exist.",
         name);
-    premiumList = PremiumList.get(name).get();
+    premiumList = PremiumList.getUncached(name).get();
     ImmutableSet<String> tldsUsedOn = premiumList.getReferencingTlds();
     checkArgument(
         tldsUsedOn.isEmpty(),

java/google/registry/tools/server/UpdatePremiumListAction.java

@@ -44,7 +44,7 @@ public class UpdatePremiumListAction extends CreateOrUpdatePremiumListAction {
 
   @Override
   protected void savePremiumList() {
-    Optional<PremiumList> existingPremiumList = PremiumList.get(name);
+    Optional<PremiumList> existingPremiumList = PremiumList.getUncached(name);
     checkArgument(
         existingPremiumList.isPresent(),
         "Could not update premium list %s because it doesn't exist.",

javatests/google/registry/model/registry/RegistryTest.java

@@ -226,7 +226,7 @@ public class RegistryTest extends EntityTestCase {
     Registry registry = Registry.get("tld").asBuilder().setPremiumList(pl2).build();
     Key<PremiumList> plKey = registry.getPremiumList();
     assertThat(plKey).isNotNull();
-    PremiumList stored = PremiumList.get(plKey.getName()).get();
+    PremiumList stored = PremiumList.getUncached(plKey.getName()).get();
     assertThat(stored.getName()).isEqualTo("tld2");
   }
 

javatests/google/registry/model/registry/label/PremiumListTest.java

@@ -70,7 +70,7 @@ public class PremiumListTest {
 
   @Test
   public void testProbablePremiumLabels() throws Exception {
-    PremiumList pl = PremiumList.get("tld").get();
+    PremiumList pl = PremiumList.getUncached("tld").get();
     PremiumListRevision revision = ofy().load().key(pl.getRevisionKey()).now();
     assertThat(revision.getProbablePremiumLabels().mightContain("notpremium")).isFalse();
     for (String label : ImmutableList.of("rich", "lol", "johnny-be-goode", "icann")) {
@@ -86,7 +86,7 @@ public class PremiumListTest {
         assertThrows(
             IllegalStateException.class,
             () ->
-                PremiumList.get("tld")
+                PremiumList.getUncached("tld")
                     .get()
                     .parse(
                         ImmutableList.of(

javatests/google/registry/model/registry/label/PremiumListUtilsTest.java

@@ -110,7 +110,7 @@ public class PremiumListUtilsTest {
   @Test
   public void testGetPremiumPrice_throwsExceptionWhenNonExistentPremiumListConfigured()
       throws Exception {
-    deletePremiumList(PremiumList.get("tld").get());
+    deletePremiumList(PremiumList.getUncached("tld").get());
     IllegalStateException thrown =
         assertThrows(
             IllegalStateException.class, () -> getPremiumPrice("blah", Registry.get("tld")));
@@ -152,7 +152,7 @@ public class PremiumListUtilsTest {
 
   @Test
   public void testGetPremiumPrice_comesFromBloomFilter() throws Exception {
-    PremiumList pl = PremiumList.get("tld").get();
+    PremiumList pl = PremiumList.getCached("tld").get();
     PremiumListEntry entry =
         persistResource(
             new PremiumListEntry.Builder()
@@ -201,7 +201,7 @@ public class PremiumListUtilsTest {
                     .delete()
                     .keys(
                         Key.create(
-                            PremiumList.get("tld").get().getRevisionKey(),
+                            PremiumList.getCached("tld").get().getRevisionKey(),
                             PremiumListEntry.class,
                             "rich"));
               }
@@ -239,8 +239,7 @@ public class PremiumListUtilsTest {
             .now()
             .price)
         .isEqualTo(Money.parse("JPY 1000"));
-    PremiumList pl2 =
+    savePremiumListAndEntries(pl, ImmutableList.of("genius,USD 10", "savant,USD 90"));
-        savePremiumListAndEntries(pl, ImmutableList.of("genius,USD 10", "savant,USD 90"));
     assertThat(getPremiumPrice("genius", registry)).hasValue(Money.parse("USD 10"));
     assertThat(getPremiumPrice("savant", registry)).hasValue(Money.parse("USD 90"));
     assertThat(getPremiumPrice("dolt", registry)).isEmpty();
@@ -277,7 +276,7 @@ public class PremiumListUtilsTest {
     assertThat(getPremiumPrice("lol", Registry.get("tld"))).hasValue(Money.parse("USD 999"));
     assertThat(getPremiumPrice("lol ", Registry.get("tld"))).isEmpty();
     ImmutableMap<String, PremiumListEntry> entries =
-        loadPremiumListEntries(PremiumList.get("tld2").get());
+        loadPremiumListEntries(PremiumList.getUncached("tld2").get());
     assertThat(entries.keySet()).containsExactly("lol");
     assertThat(entries).doesNotContainKey("lol ");
     PremiumListEntry entry = entries.get("lol");
@@ -305,12 +304,12 @@ public class PremiumListUtilsTest {
             new PremiumList.Builder().setName("pl").build(), ImmutableList.of("test,USD 1"));
     ImmutableMap<String, PremiumListEntry> entries = loadPremiumListEntries(pl);
     assertThat(entries.keySet()).containsExactly("test");
-    assertThat(loadPremiumListEntries(PremiumList.get("pl").get())).isEqualTo(entries);
+    assertThat(loadPremiumListEntries(PremiumList.getUncached("pl").get())).isEqualTo(entries);
     // Save again with no changes, and clear the cache to force a re-load from Datastore.
     PremiumList resaved = savePremiumListAndEntries(pl, ImmutableList.of("test,USD 1"));
     ofy().clearSessionCache();
     Map<String, PremiumListEntry> entriesReloaded =
-        loadPremiumListEntries(PremiumList.get("pl").get());
+        loadPremiumListEntries(PremiumList.getUncached("pl").get());
     assertThat(entriesReloaded).hasSize(1);
     assertThat(entriesReloaded).containsKey("test");
     assertThat(entriesReloaded.get("test").parent).isEqualTo(resaved.getRevisionKey());
@@ -319,18 +318,18 @@ public class PremiumListUtilsTest {
   @Test
   public void testDelete() throws Exception {
     persistPremiumList("gtld1", "trombone,USD 10");
-    assertThat(PremiumList.get("gtld1")).isPresent();
+    assertThat(PremiumList.getUncached("gtld1")).isPresent();
-    Key<PremiumListRevision> parent = PremiumList.get("gtld1").get().getRevisionKey();
+    Key<PremiumListRevision> parent = PremiumList.getUncached("gtld1").get().getRevisionKey();
-    deletePremiumList(PremiumList.get("gtld1").get());
+    deletePremiumList(PremiumList.getUncached("gtld1").get());
-    assertThat(PremiumList.get("gtld1")).isEmpty();
+    assertThat(PremiumList.getUncached("gtld1")).isEmpty();
     assertThat(ofy().load().type(PremiumListEntry.class).ancestor(parent).list()).isEmpty();
   }
 
   @Test
   public void testDelete_largeNumberOfEntries_succeeds() {
     persistHumongousPremiumList("ginormous", 2500);
-    deletePremiumList(PremiumList.get("ginormous").get());
+    deletePremiumList(PremiumList.getUncached("ginormous").get());
-    assertThat(PremiumList.get("ginormous")).isEmpty();
+    assertThat(PremiumList.getUncached("ginormous")).isEmpty();
   }
 
   /** Persists a premium list with a specified number of nonsense entries. */

javatests/google/registry/tools/DeletePremiumListCommandTest.java

@@ -36,7 +36,7 @@ public class DeletePremiumListCommandTest extends CommandTestCase<DeletePremiumL
     PremiumList premiumList = persistPremiumList("xn--q9jyb4c", "blah,USD 100");
     assertThat(loadPremiumListEntries(premiumList)).hasSize(1);
     runCommand("--force", "--name=xn--q9jyb4c");
-    assertThat(PremiumList.get("xn--q9jyb4c")).isEmpty();
+    assertThat(PremiumList.getUncached("xn--q9jyb4c")).isEmpty();
 
     // Ensure that the Datastore premium list entry entities were deleted correctly.
     assertThat(ofy().load()
@@ -64,7 +64,7 @@ public class DeletePremiumListCommandTest extends CommandTestCase<DeletePremiumL
         assertThrows(
             IllegalArgumentException.class,
             () -> runCommandForced("--name=" + premiumList.getName()));
-    assertThat(PremiumList.get(premiumList.getName())).isPresent();
+    assertThat(PremiumList.getUncached(premiumList.getName())).isPresent();
     assertThat(thrown)
         .hasMessageThat()
         .isEqualTo("Cannot delete premium list because it is used on these tld(s): xn--q9jyb4c");

javatests/google/registry/tools/server/CreatePremiumListActionTest.java

@@ -46,7 +46,7 @@ public class CreatePremiumListActionTest {
   @Before
   public void init() throws Exception {
     createTlds("foo", "xn--q9jyb4c", "how");
-    deletePremiumList(PremiumList.get("foo").get());
+    deletePremiumList(PremiumList.getUncached("foo").get());
     action = new CreatePremiumListAction();
     response = new FakeJsonResponse();
     action.response = response;
@@ -78,7 +78,7 @@ public class CreatePremiumListActionTest {
     action.override = true;
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_OK);
-    assertThat(loadPremiumListEntries(PremiumList.get("zanzibar").get())).hasSize(1);
+    assertThat(loadPremiumListEntries(PremiumList.getUncached("zanzibar").get())).hasSize(1);
   }
 
   @Test
@@ -87,7 +87,7 @@ public class CreatePremiumListActionTest {
     action.inputData = "rich,USD 25\nricher,USD 1000\n";
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_OK);
-    assertThat(loadPremiumListEntries(PremiumList.get("foo").get())).hasSize(2);
+    assertThat(loadPremiumListEntries(PremiumList.getUncached("foo").get())).hasSize(2);
     assertThat(getPremiumPrice("rich", Registry.get("foo"))).hasValue(Money.parse("USD 25"));
     assertThat(getPremiumPrice("diamond", Registry.get("foo"))).isEmpty();
   }

javatests/google/registry/tools/server/UpdatePremiumListActionTest.java

@@ -80,7 +80,7 @@ public class UpdatePremiumListActionTest {
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_OK);
     Registry registry = Registry.get("foo");
-    assertThat(loadPremiumListEntries(PremiumList.get("foo").get())).hasSize(3);
+    assertThat(loadPremiumListEntries(PremiumList.getUncached("foo").get())).hasSize(3);
     assertThat(getPremiumPrice("rich", registry)).hasValue(Money.parse("USD 75"));
     assertThat(getPremiumPrice("richer", registry)).hasValue(Money.parse("USD 5000"));
     assertThat(getPremiumPrice("poor", registry)).hasValue(Money.parse("USD 0.99"));