From b8d7d9da29e7b19da6fee7f5e86207c6adf7ae35 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Thu, 17 May 2018 12:30:52 -0700 Subject: [PATCH] Don't delete the old premium list yet after updating to a new one The issue is that the premium list cache is configured to persist for 60 minutes. So after updating the list, checks/creates for up to the next 60 minutes could still be referring to the old list. That's fine and dandy, unless you delete the old premium list immediately (*bad*), which makes all domains appear to now be non-premium for as long as the cache lasts. The reason deleting the premium list entries makes names appear as non-premium is that a load-by-key existence check with the domain label itself is used to determine if a name is premium. I also removed a misleading cache update statement, which doesn't do what it appears to be doing (it appears to fix this issue) because cache is instance-level, and so even if the premium list were updated from the frontend instance only one of 100 instances would have its cache updated. But it's updated from the tools service anyway, so it's guaranteed to not be a shared cache with any instance serving EPP traffic. On a sidenote, I introduced this bug on 2014-10-27 in [] The domain label list refactor was my Noogler project. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=197033604 --- .../model/registry/label/PremiumListUtils.java | 6 ++---- .../registry/label/PremiumListUtilsTest.java | 15 +-------------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/java/google/registry/model/registry/label/PremiumListUtils.java b/java/google/registry/model/registry/label/PremiumListUtils.java index bc05508cd..46506fb86 100644 --- a/java/google/registry/model/registry/label/PremiumListUtils.java +++ b/java/google/registry/model/registry/label/PremiumListUtils.java @@ -180,10 +180,8 @@ public final class PremiumListUtils { ofy().save().entities(newList, newRevision); return newList; }); - // Update the cache. - cachePremiumLists.put(premiumList.getName(), updated); - // Delete the entities under the old PremiumList. - oldPremiumList.ifPresent(PremiumListUtils::deleteRevisionAndEntriesOfPremiumList); + // TODO(b/79888775): Enqueue the oldPremiumList for deletion after at least + // RegistryConfig.getDomainLabelListCacheDuration() has elapsed. return updated; } diff --git a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java index 214a37a9f..d49deb1d4 100644 --- a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java @@ -244,20 +244,7 @@ public class PremiumListUtilsTest { assertThat(getPremiumPrice("genius", registry)).hasValue(Money.parse("USD 10")); assertThat(getPremiumPrice("savant", registry)).hasValue(Money.parse("USD 90")); assertThat(getPremiumPrice("dolt", registry)).isEmpty(); - assertThat(ofy() - .load() - .type(PremiumListEntry.class) - .parent(pl.getRevisionKey()) - .id("dolt") - .now()) - .isNull(); - assertThat(ofy() - .load() - .type(PremiumListEntry.class) - .parent(pl2.getRevisionKey()) - .id("dolt") - .now()) - .isNull(); + // TODO(b/79888775): Assert that the old premium list is enqueued for later deletion. assertThat(premiumListChecks) .hasValueForLabels(4, "tld", "tld", UNCACHED_POSITIVE.toString()) .and()