From 3ca9bb6aeb40dc445d45d5df0cb6952f38005c5e Mon Sep 17 00:00:00 2001 From: mcilwain Date: Fri, 24 Feb 2017 10:32:23 -0800 Subject: [PATCH] Read from bloom filter for premium pricing checks This also cleans up the PremiumList API so that it only has one method for checking premium prices, which is by TLD, rather than two. I will be refactoring a lot of the static methods currently residing in the PremiumList class into a separate utils class, but I don't want to include too many changes in this one CL. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=148475345 --- .../registry/config/RegistryConfig.java | 7 + .../config/RegistryConfigSettings.java | 1 + .../registry/config/files/default-config.yaml | 8 + .../config/files/nomulus-config-unittest.yaml | 1 + .../StaticPremiumListPricingEngine.java | 11 +- .../registry/model/registry/Registry.java | 1 + .../registry/label/BaseDomainLabelList.java | 4 +- .../model/registry/label/PremiumList.java | 303 +++++++++++------- .../CreateOrUpdatePremiumListCommand.java | 9 +- .../tools/DeletePremiumListCommand.java | 5 +- .../tools/server/CreatePremiumListAction.java | 19 +- .../tools/server/UpdatePremiumListAction.java | 27 +- .../model/registry/label/PremiumListTest.java | 137 ++++---- .../registry/testing/DatastoreHelper.java | 42 ++- .../tools/DeletePremiumListCommandTest.java | 2 +- .../server/CreatePremiumListActionTest.java | 14 +- .../server/ListPremiumListsActionTest.java | 5 - .../server/UpdatePremiumListActionTest.java | 14 +- 18 files changed, 328 insertions(+), 282 deletions(-) diff --git a/java/google/registry/config/RegistryConfig.java b/java/google/registry/config/RegistryConfig.java index 5c09bd72e..2a17ca4ab 100644 --- a/java/google/registry/config/RegistryConfig.java +++ b/java/google/registry/config/RegistryConfig.java @@ -1135,6 +1135,13 @@ public final class RegistryConfig { return Duration.standardSeconds(CONFIG_SETTINGS.get().caching.singletonCachePersistSeconds); } + /** + * Returns the maximum number of premium list entries across all TLDs to keep in in-memory cache. + */ + public static int getStaticPremiumListMaxCachedEntries() { + return CONFIG_SETTINGS.get().caching.staticPremiumListMaxCachedEntries; + } + /** Returns the email address that outgoing emails from the app are sent from. */ public static String getGSuiteOutgoingEmailAddress() { return CONFIG_SETTINGS.get().gSuite.outgoingEmailAddress; diff --git a/java/google/registry/config/RegistryConfigSettings.java b/java/google/registry/config/RegistryConfigSettings.java index 0c62df131..54f1eb7f5 100644 --- a/java/google/registry/config/RegistryConfigSettings.java +++ b/java/google/registry/config/RegistryConfigSettings.java @@ -91,6 +91,7 @@ public class RegistryConfigSettings { public int singletonCacheRefreshSeconds; public int domainLabelCachingSeconds; public int singletonCachePersistSeconds; + public int staticPremiumListMaxCachedEntries; } /** Configuration for Registry Data Escrow (RDE). */ diff --git a/java/google/registry/config/files/default-config.yaml b/java/google/registry/config/files/default-config.yaml index 0509489c0..6f605fed3 100644 --- a/java/google/registry/config/files/default-config.yaml +++ b/java/google/registry/config/files/default-config.yaml @@ -115,6 +115,14 @@ caching: # Length of time that a long-lived singleton in persist mode should be cached. singletonCachePersistSeconds: 31557600 # This is one year. + # Maximum total number of static premium list entry entities to cache in + # memory, across all premium lists for all TLDs. Tuning this up will use more + # memory (and might require using larger App Engine instances). Note that + # premium list entries that are absent are cached in addition to ones that are + # present, so the total cache size is not bounded by the total number of + # premium price entries that exist. + staticPremiumListMaxCachedEntries: 200000 + rde: # URL prefix of ICANN's server to upload RDE reports to. Nomulus adds /TLD/ID # to the end of this to construct the full URL. diff --git a/java/google/registry/config/files/nomulus-config-unittest.yaml b/java/google/registry/config/files/nomulus-config-unittest.yaml index dbd78efe0..34cb4ad11 100644 --- a/java/google/registry/config/files/nomulus-config-unittest.yaml +++ b/java/google/registry/config/files/nomulus-config-unittest.yaml @@ -17,6 +17,7 @@ caching: singletonCacheRefreshSeconds: 0 domainLabelCachingSeconds: 0 singletonCachePersistSeconds: 0 + staticPremiumListMaxCachedEntries: 50 braintree: merchantAccountIdsMap: diff --git a/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java b/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java index 103012d2f..158633845 100644 --- a/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java +++ b/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java @@ -15,9 +15,9 @@ package google.registry.model.pricing; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.emptyToNull; import static google.registry.model.registry.Registry.TldState.SUNRISE; +import static google.registry.model.registry.label.PremiumList.getPremiumPrice; import static google.registry.model.registry.label.ReservationType.NAME_COLLISION; import static google.registry.model.registry.label.ReservedList.getReservation; import static google.registry.util.DomainNameUtils.getTldFromDomainName; @@ -26,7 +26,6 @@ import com.google.common.base.Joiner; import com.google.common.base.Optional; import com.google.common.net.InternetDomainName; import google.registry.model.registry.Registry; -import google.registry.model.registry.label.PremiumList; import javax.inject.Inject; import org.joda.money.Money; import org.joda.time.DateTime; @@ -44,13 +43,7 @@ public final class StaticPremiumListPricingEngine implements PremiumPricingEngin String tld = getTldFromDomainName(fullyQualifiedDomainName); String label = InternetDomainName.from(fullyQualifiedDomainName).parts().get(0); Registry registry = Registry.get(checkNotNull(tld, "tld")); - Optional premiumPrice = Optional.absent(); - if (registry.getPremiumList() != null) { - String listName = registry.getPremiumList().getName(); - Optional premiumList = PremiumList.get(listName); - checkState(premiumList.isPresent(), "Could not load premium list: %s", listName); - premiumPrice = premiumList.get().getPremiumPrice(label); - } + Optional premiumPrice = getPremiumPrice(label, registry); boolean isNameCollisionInSunrise = registry.getTldState(priceTime).equals(SUNRISE) && getReservation(label, tld) == NAME_COLLISION; diff --git a/java/google/registry/model/registry/Registry.java b/java/google/registry/model/registry/Registry.java index 3638077ea..ce1b21316 100644 --- a/java/google/registry/model/registry/Registry.java +++ b/java/google/registry/model/registry/Registry.java @@ -484,6 +484,7 @@ public class Registry extends ImmutableObject implements Buildable { return anchorTenantAddGracePeriodLength; } + @Nullable public Key getPremiumList() { return premiumList; } diff --git a/java/google/registry/model/registry/label/BaseDomainLabelList.java b/java/google/registry/model/registry/label/BaseDomainLabelList.java index 7f5c34bb4..f506d0c10 100644 --- a/java/google/registry/model/registry/label/BaseDomainLabelList.java +++ b/java/google/registry/model/registry/label/BaseDomainLabelList.java @@ -19,7 +19,6 @@ import static com.google.common.base.Strings.isNullOrEmpty; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.registry.Registries.getTlds; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.cache.CacheLoader.InvalidCacheLoadException; import com.google.common.cache.LoadingCache; @@ -83,8 +82,7 @@ public abstract class BaseDomainLabelList, R extends Dom * * @param lines the CSV file, line by line */ - @VisibleForTesting - protected ImmutableMap parse(Iterable lines) { + public ImmutableMap parse(Iterable lines) { Map labelsToEntries = new HashMap<>(); Multiset duplicateLabels = HashMultiset.create(); for (String line : lines) { diff --git a/java/google/registry/model/registry/label/PremiumList.java b/java/google/registry/model/registry/label/PremiumList.java index 3414337c6..e87e443a3 100644 --- a/java/google/registry/model/registry/label/PremiumList.java +++ b/java/google/registry/model/registry/label/PremiumList.java @@ -13,23 +13,20 @@ // limitations under the License. package google.registry.model.registry.label; - -import static com.google.appengine.api.datastore.DatastoreServiceFactory.getDatastoreService; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Iterables.partition; import static com.google.common.hash.Funnels.unencodedCharsFunnel; import static google.registry.config.RegistryConfig.getDomainLabelListCacheDuration; +import static google.registry.config.RegistryConfig.getSingletonCachePersistDuration; +import static google.registry.config.RegistryConfig.getStaticPremiumListMaxCachedEntries; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.ofy.ObjectifyService.allocateId; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.Ofy.RECOMMENDED_MEMCACHE_EXPIRATION; -import static google.registry.util.CollectionUtils.nullToEmpty; -import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import static java.util.concurrent.TimeUnit.MILLISECONDS; -import com.google.appengine.api.datastore.EntityNotFoundException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Optional; @@ -38,8 +35,9 @@ import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.CacheLoader.InvalidCacheLoadException; import com.google.common.cache.LoadingCache; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; +import com.google.common.collect.ImmutableSet; import com.google.common.hash.BloomFilter; import com.google.common.util.concurrent.UncheckedExecutionException; import com.googlecode.objectify.Key; @@ -48,8 +46,6 @@ import com.googlecode.objectify.Work; import com.googlecode.objectify.annotation.Cache; import com.googlecode.objectify.annotation.Entity; import com.googlecode.objectify.annotation.Id; -import com.googlecode.objectify.annotation.Ignore; -import com.googlecode.objectify.annotation.OnLoad; import com.googlecode.objectify.annotation.Parent; import com.googlecode.objectify.cmd.Query; import google.registry.model.Buildable; @@ -81,13 +77,6 @@ public final class PremiumList extends BaseDomainLabelList revisionKey; - /** The revision to be saved along with this entity. */ - @Ignore - PremiumListRevision revision; - - @Ignore - Map premiumListMap; - /** Virtual parent entity for premium list entry entities associated with a single revision. */ @ReportedOn @Entity @@ -120,7 +109,8 @@ public final class PremiumList extends BaseDomainLabelList premiumLabels) { + @VisibleForTesting + public static PremiumListRevision create(PremiumList parent, Set premiumLabels) { PremiumListRevision revision = new PremiumListRevision(); revision.parent = Key.create(parent); revision.revisionId = allocateId(); @@ -143,7 +133,13 @@ public final class PremiumList extends BaseDomainLabelList cache = + /** + * In-memory cache for premium lists. + * + *

This is cached for a shorter duration because we need to periodically reload this entity to + * check if a new revision has been published, and if so, then use that. + */ + private static final LoadingCache cachePremiumLists = CacheBuilder.newBuilder() .expireAfterWrite(getDomainLabelListCacheDuration().getMillis(), MILLISECONDS) .build(new CacheLoader() { @@ -161,53 +157,122 @@ public final class PremiumList extends BaseDomainLabelListThis is cached for a long duration (essentially indefinitely) because a given + * {@link PremiumListRevision} is immutable and cannot ever be changed once created, so its cache + * need not ever expire. */ - public static Optional getPremiumPrice(String label, String tld) { - Registry registry = Registry.get(checkNotNull(tld, "tld")); + private static final LoadingCache, PremiumListRevision> + cachePremiumListRevisions = + CacheBuilder.newBuilder() + .expireAfterWrite(getSingletonCachePersistDuration().getMillis(), MILLISECONDS) + .build( + new CacheLoader, PremiumListRevision>() { + @Override + public PremiumListRevision load(final Key revisionKey) { + return ofy() + .doTransactionless( + new Work() { + @Override + public PremiumListRevision run() { + return ofy().load().key(revisionKey).now(); + }}); + }}); + + /** + * In-memory cache for {@link PremiumListEntry}s for a given label and {@link PremiumListRevision} + * + *

Because the PremiumList itself makes up part of the PremiumListRevision's key, this is + * specific to a given premium list. Premium list entries might not be present, as indicated by + * the Optional wrapper, and we want to cache that as well. + * + *

This is cached for a long duration (essentially indefinitely) because a given {@link + * PremiumListRevision} and its child {@link PremiumListEntry}s are immutable and cannot ever be + * changed once created, so the cache need not ever expire. + * + *

A maximum size is set here on the cache because it can potentially grow too big to fit in + * memory if there are a very large number of premium list entries in the system. The least- + * accessed entries will be evicted first. + */ + @VisibleForTesting + static final LoadingCache, Optional> + cachePremiumListEntries = + CacheBuilder.newBuilder() + .expireAfterWrite(getSingletonCachePersistDuration().getMillis(), MILLISECONDS) + .maximumSize(getStaticPremiumListMaxCachedEntries()) + .build( + new CacheLoader, Optional>() { + @Override + public Optional load(final Key entryKey) { + return ofy() + .doTransactionless( + new Work>() { + @Override + public Optional run() { + return Optional.fromNullable(ofy().load().key(entryKey).now()); + }}); + }}); + + /** + * Returns the premium price for the specified label and registry, or absent if the label is not + * premium. + */ + public static Optional getPremiumPrice(String label, Registry registry) { + // If the registry has no configured premium list, then no labels are premium. if (registry.getPremiumList() == null) { return Optional. absent(); } String listName = registry.getPremiumList().getName(); - Optional premiumList = get(listName); - if (!premiumList.isPresent()) { - throw new IllegalStateException("Could not load premium list named " + listName); - } - return premiumList.get().getPremiumPrice(label); - } - - @OnLoad - private void onLoad() { - if (revisionKey != null) { - revision = ofy().load().key(revisionKey).now(); - } - - // TODO(b/32383610): Don't load up the premium list entries. + Optional optionalPremiumList = get(listName); + checkState(optionalPremiumList.isPresent(), "Could not load premium list '%s'", listName); + PremiumList premiumList = optionalPremiumList.get(); + PremiumListRevision revision; try { - ImmutableMap.Builder entriesMap = new ImmutableMap.Builder<>(); - if (revisionKey != null) { - for (PremiumListEntry entry : loadEntriesForCurrentRevision()) { - entriesMap.put(entry.getLabel(), entry); - } + revision = cachePremiumListRevisions.get(premiumList.getRevisionKey()); + } catch (InvalidCacheLoadException | ExecutionException e) { + throw new RuntimeException( + "Could not load premium list revision " + premiumList.getRevisionKey(), e); + } + checkState( + revision.probablePremiumLabels != null, + "Probable premium labels bloom filter is null on revision '%s'", + premiumList.getRevisionKey()); + + if (revision.probablePremiumLabels.mightContain(label)) { + Key entryKey = + Key.create(premiumList.getRevisionKey(), PremiumListEntry.class, label); + try { + Optional entry = cachePremiumListEntries.get(entryKey); + return (entry.isPresent()) ? Optional.of(entry.get().getValue()) : Optional.absent(); + } catch (InvalidCacheLoadException | ExecutionException e) { + throw new RuntimeException("Could not load premium list entry " + entryKey, e); } - premiumListMap = entriesMap.build(); - } catch (Exception e) { - throw new RuntimeException("Could not retrieve entries for premium list " + name, e); + } else { + return Optional.absent(); } } /** - * Gets the premium price for the specified label in the current PremiumList, or returns - * Optional.absent if there is no premium price. + * Loads and returns the entire premium list map. + * + *

This load operation is quite expensive for large premium lists because each premium list + * entry is a separate Datastore entity, and loading them this way bypasses the in-memory caches. + * Do not use this method if all you need to do is check the price of a small number of labels! */ - public Optional getPremiumPrice(String label) { - return Optional.fromNullable( - premiumListMap.containsKey(label) ? premiumListMap.get(label).getValue() : null); - } - - public Map getPremiumListEntries() { - return nullToEmptyImmutableCopy(premiumListMap); + @VisibleForTesting + public Map loadPremiumListEntries() { + try { + ImmutableMap.Builder entriesMap = new ImmutableMap.Builder<>(); + if (revisionKey != null) { + for (PremiumListEntry entry : queryEntriesForCurrentRevision()) { + entriesMap.put(entry.getLabel(), entry); + } + } + return entriesMap.build(); + } catch (Exception e) { + throw new RuntimeException("Could not retrieve entries for premium list " + name, e); + } } @VisibleForTesting @@ -215,15 +280,10 @@ public final class PremiumList extends BaseDomainLabelList get(String name) { try { - return Optional.of(cache.get(name)); + return Optional.of(cachePremiumLists.get(name)); } catch (InvalidCacheLoadException e) { return Optional. absent(); } catch (ExecutionException e) { @@ -231,18 +291,9 @@ public final class PremiumList extends BaseDomainLabelList premiumListLines) { + return saveWithEntries(premiumList, premiumList.parse(premiumListLines)); + } + + /** Re-parents the given {@link PremiumListEntry}s on the given {@link PremiumListRevision}. */ + public static ImmutableSet parentEntriesOnRevision( + Iterable entries, final Key revisionKey) { + return FluentIterable.from(firstNonNull(entries, ImmutableSet.of())) + .transform( + new Function() { + @Override + public PremiumListEntry apply(PremiumListEntry entry) { + return entry.asBuilder().setParent(revisionKey).build(); + } + }) + .toSet(); + } + /** - * Persists a PremiumList object to Datastore. + * Persists a new or updated PremiumList object and its descendant entities to Datastore. * - *

The flow here is: save the new premium list entries parented on that revision entity, + *

The flow here is: save the new premium list entries parented on that revision entity, * save/update the PremiumList, and then delete the old premium list entries associated with the * old revision. + * + *

This is the only valid way to save these kinds of entities! */ - public PremiumList saveAndUpdateEntries() { - final Optional oldPremiumList = get(name); - // 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 batch - : partition(premiumListMap.values(), TRANSACTION_BATCH_SIZE)) { - ofy().transactNew(new VoidWork() { - @Override - public void vrun() { - ofy().save().entities(batch); - }}); - } + public static PremiumList saveWithEntries( + final PremiumList premiumList, ImmutableMap premiumListEntries) { + final Optional oldPremiumList = get(premiumList.getName()); + + // Create the new revision (with its bloom filter) and parent the entries on it. + final PremiumListRevision newRevision = + PremiumListRevision.create(premiumList, premiumListEntries.keySet()); + final Key newRevisionKey = Key.create(newRevision); + ImmutableSet parentedEntries = + parentEntriesOnRevision( + firstNonNull(premiumListEntries.values(), ImmutableSet.of()), newRevisionKey); + + // Save the new child entities in a series of transactions. + for (final List batch : partition(parentedEntries, TRANSACTION_BATCH_SIZE)) { + ofy().transactNew(new VoidWork() { + @Override + public void vrun() { + ofy().save().entities(batch); + }}); } // Save the new PremiumList and revision itself. @@ -342,23 +416,27 @@ public final class PremiumList extends BaseDomainLabelList> batch : partition( - loadEntriesForCurrentRevision().keys(), + queryEntriesForCurrentRevision().keys(), TRANSACTION_BATCH_SIZE)) { ofy().transactNew(new VoidWork() { @Override @@ -400,7 +478,7 @@ public final class PremiumList extends BaseDomainLabelList loadEntriesForCurrentRevision() { + private Query queryEntriesForCurrentRevision() { return ofy().load().type(PremiumListEntry.class).ancestor(revisionKey); } @@ -418,34 +496,13 @@ public final class PremiumList extends BaseDomainLabelList premiumListMap) { - entriesWereUpdated = true; - getInstance().premiumListMap = premiumListMap; + public Builder setRevision(Key revision) { + getInstance().revisionKey = revision; return this; } - /** Updates the premiumListMap from input lines. */ - public Builder setPremiumListMapFromLines(Iterable lines) { - return setPremiumListMap(getInstance().parse(lines)); - } - @Override public PremiumList build() { - final PremiumList instance = getInstance(); - if (instance.revisionKey == null || entriesWereUpdated) { - instance.revision = PremiumListRevision.create(instance, instance.premiumListMap.keySet()); - instance.revisionKey = Key.create(instance.revision); - } - // When we build an instance, make sure all entries are parented on its revisionKey. - instance.premiumListMap = Maps.transformValues( - nullToEmpty(instance.premiumListMap), - new Function() { - @Override - public PremiumListEntry apply(PremiumListEntry entry) { - return entry.asBuilder().setParent(instance.revisionKey).build(); - }}); return super.build(); } } diff --git a/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java b/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java index 0b36598b8..9388811c6 100644 --- a/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java +++ b/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java @@ -75,11 +75,8 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand protected void init() throws Exception { name = isNullOrEmpty(name) ? convertFilePathToName(inputFile) : name; List lines = Files.readAllLines(inputFile, UTF_8); - // Try constructing the premium list locally to check up front for validation errors. - new PremiumList.Builder() - .setName(name) - .setPremiumListMapFromLines(lines) - .build(); + // Try constructing and parsing the premium list locally to check up front for validation errors + new PremiumList.Builder().setName(name).build().parse(lines); inputLineCount = lines.size(); } @@ -108,7 +105,7 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand getCommandPath(), params.build(), MediaType.FORM_DATA, - requestBody.getBytes()); + requestBody.getBytes(UTF_8)); return extractServerResponse(response); } diff --git a/java/google/registry/tools/DeletePremiumListCommand.java b/java/google/registry/tools/DeletePremiumListCommand.java index 0d9be97ba..93a33a8d8 100644 --- a/java/google/registry/tools/DeletePremiumListCommand.java +++ b/java/google/registry/tools/DeletePremiumListCommand.java @@ -62,9 +62,6 @@ final class DeletePremiumListCommand extends ConfirmingCommand implements Remote @Override protected String execute() throws Exception { premiumList.delete(); - return String.format( - "Deleted premium list %s with %d entries.\n", - premiumList.getName(), - premiumList.getPremiumListEntries().size()); + return String.format("Deleted premium list '%s'.\n", premiumList.getName()); } } diff --git a/java/google/registry/tools/server/CreatePremiumListAction.java b/java/google/registry/tools/server/CreatePremiumListAction.java index 6299b6516..cb2a4aa13 100644 --- a/java/google/registry/tools/server/CreatePremiumListAction.java +++ b/java/google/registry/tools/server/CreatePremiumListAction.java @@ -16,6 +16,7 @@ package google.registry.tools.server; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.model.registry.Registries.assertTldExists; +import static google.registry.model.registry.label.PremiumList.saveWithEntries; import static google.registry.request.Action.Method.POST; import com.google.common.base.Splitter; @@ -52,16 +53,14 @@ public class CreatePremiumListAction extends CreateOrUpdatePremiumListAction { logger.infofmt("Got the following input data: %s", inputData); List inputDataPreProcessed = Splitter.on('\n').omitEmptyStrings().splitToList(inputData); - PremiumList premiumList = new PremiumList.Builder() - .setName(name) - .setPremiumListMapFromLines(inputDataPreProcessed) - .build(); - premiumList.saveAndUpdateEntries(); + PremiumList premiumList = new PremiumList.Builder().setName(name).build(); + saveWithEntries(premiumList, inputDataPreProcessed); - logger.infofmt("Saved premium list %s with entries %s", - premiumList.getName(), - premiumList.getPremiumListEntries()); - - response.setPayload(ImmutableMap.of("status", "success")); + String message = + String.format( + "Saved premium list %s with %d entries", + premiumList.getName(), inputDataPreProcessed.size()); + logger.info(message); + response.setPayload(ImmutableMap.of("status", "success", "message", message)); } } diff --git a/java/google/registry/tools/server/UpdatePremiumListAction.java b/java/google/registry/tools/server/UpdatePremiumListAction.java index a1cd5e648..f3830ff02 100644 --- a/java/google/registry/tools/server/UpdatePremiumListAction.java +++ b/java/google/registry/tools/server/UpdatePremiumListAction.java @@ -15,6 +15,7 @@ package google.registry.tools.server; import static com.google.common.base.Preconditions.checkArgument; +import static google.registry.model.registry.label.PremiumList.saveWithEntries; import static google.registry.request.Action.Method.POST; import com.google.common.base.Optional; @@ -38,9 +39,9 @@ public class UpdatePremiumListAction extends CreateOrUpdatePremiumListAction { @Override protected void savePremiumList() { - Optional existingName = PremiumList.get(name); + Optional existingPremiumList = PremiumList.get(name); checkArgument( - existingName.isPresent(), + existingPremiumList.isPresent(), "Could not update premium list %s because it doesn't exist.", name); @@ -48,21 +49,13 @@ public class UpdatePremiumListAction extends CreateOrUpdatePremiumListAction { logger.infofmt("Got the following input data: %s", inputData); List inputDataPreProcessed = Splitter.on('\n').omitEmptyStrings().splitToList(inputData); - PremiumList premiumList = existingName.get().asBuilder() - .setPremiumListMapFromLines(inputDataPreProcessed) - .build(); - premiumList.saveAndUpdateEntries(); + PremiumList newPremiumList = saveWithEntries(existingPremiumList.get(), inputDataPreProcessed); - logger.infofmt("Updated premium list %s with entries %s", - premiumList.getName(), - premiumList.getPremiumListEntries()); - - String message = String.format( - "Saved premium list %s with %d entries.\n", - premiumList.getName(), - premiumList.getPremiumListEntries().size()); - response.setPayload(ImmutableMap.of( - "status", "success", - "message", message)); + String message = + String.format( + "Updated premium list %s with %d entries.", + newPremiumList.getName(), inputDataPreProcessed.size()); + logger.info(message); + response.setPayload(ImmutableMap.of("status", "success", "message", message)); } } diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java index 6c58ccb13..f3f90fb4d 100644 --- a/javatests/google/registry/model/registry/label/PremiumListTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListTest.java @@ -17,7 +17,9 @@ package google.registry.model.registry.label; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.registry.label.PremiumList.cachePremiumListEntries; import static google.registry.model.registry.label.PremiumList.getPremiumPrice; +import static google.registry.model.registry.label.PremiumList.saveWithEntries; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistPremiumList; import static google.registry.testing.DatastoreHelper.persistReservedList; @@ -34,7 +36,6 @@ import google.registry.testing.AppEngineRule; import google.registry.testing.ExceptionRule; import java.util.Map; import org.joda.money.Money; -import org.joda.time.DateTime; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -75,54 +76,49 @@ public class PremiumListTest { .setPremiumPricingEngine(StaticPremiumListPricingEngine.NAME) .build()); assertThat(Registry.get("ghost").getPremiumList()).isNull(); - assertThat(getPremiumPrice("blah", "ghost")).isAbsent(); + assertThat(getPremiumPrice("blah", Registry.get("ghost"))).isAbsent(); } @Test public void testGetPremiumPrice_throwsExceptionWhenNonExistentPremiumListConfigured() throws Exception { PremiumList.get("tld").get().delete(); - thrown.expect(IllegalStateException.class, "Could not load premium list named tld"); - getPremiumPrice("blah", "tld"); + thrown.expect(IllegalStateException.class, "Could not load premium list 'tld'"); + getPremiumPrice("blah", Registry.get("tld")); } @Test public void testSave_largeNumberOfEntries_succeeds() throws Exception { PremiumList premiumList = persistHumongousPremiumList("tld", 2500); - assertThat(premiumList.getPremiumListEntries()).hasSize(2500); - assertThat(premiumList.getPremiumPrice("7")).hasValue(Money.parse("USD 100")); + assertThat(premiumList.loadPremiumListEntries()).hasSize(2500); + assertThat(getPremiumPrice("7", Registry.get("tld"))).hasValue(Money.parse("USD 100")); } @Test public void testSave_updateTime_isUpdatedOnEverySave() throws Exception { - PremiumList pl = new PremiumList.Builder() - .setName("tld3") - .setPremiumListMapFromLines(ImmutableList.of("slime,USD 10")) - .build() - .saveAndUpdateEntries(); - PremiumList newPl = new PremiumList.Builder() - .setName(pl.getName()) - .setPremiumListMapFromLines(ImmutableList.of("mutants,USD 20")) - .build() - .saveAndUpdateEntries(); + PremiumList pl = + saveWithEntries( + new PremiumList.Builder().setName("tld3").build(), ImmutableList.of("slime,USD 10")); + PremiumList newPl = + saveWithEntries( + new PremiumList.Builder().setName(pl.getName()).build(), + ImmutableList.of("mutants,USD 20")); assertThat(newPl.getLastUpdateTime()).isGreaterThan(pl.getLastUpdateTime()); } @Test public void testSave_creationTime_onlyUpdatedOnFirstCreation() throws Exception { PremiumList pl = persistPremiumList("tld3", "sludge,JPY 1000"); - DateTime creationTime = pl.creationTime; - pl = pl.asBuilder() - .setPremiumListMapFromLines(ImmutableList.of("sleighbells,CHF 2000")) - .build(); - assertThat(pl.creationTime).isEqualTo(creationTime); + PremiumList newPl = saveWithEntries(pl, ImmutableList.of("sleighbells,CHF 2000")); + assertThat(newPl.creationTime).isEqualTo(pl.creationTime); } @Test public void testSave_removedPremiumListEntries_areNoLongerInDatastore() throws Exception { + Registry registry = Registry.get("tld"); PremiumList pl = persistPremiumList("tld", "genius,USD 10", "dolt,JPY 1000"); - assertThat(getPremiumPrice("genius", "tld")).hasValue(Money.parse("USD 10")); - assertThat(getPremiumPrice("dolt", "tld")).hasValue(Money.parse("JPY 1000")); + assertThat(getPremiumPrice("genius", registry)).hasValue(Money.parse("USD 10")); + assertThat(getPremiumPrice("dolt", registry)).hasValue(Money.parse("JPY 1000")); assertThat(ofy() .load() .type(PremiumListEntry.class) @@ -131,13 +127,10 @@ public class PremiumListTest { .now() .price) .isEqualTo(Money.parse("JPY 1000")); - PremiumList pl2 = pl.asBuilder() - .setPremiumListMapFromLines(ImmutableList.of("genius,USD 10", "savant,USD 90")) - .build() - .saveAndUpdateEntries(); - assertThat(getPremiumPrice("genius", "tld")).hasValue(Money.parse("USD 10")); - assertThat(getPremiumPrice("savant", "tld")).hasValue(Money.parse("USD 90")); - assertThat(getPremiumPrice("dolt", "tld")).isAbsent(); + PremiumList pl2 = saveWithEntries(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)).isAbsent(); assertThat(ofy() .load() .type(PremiumListEntry.class) @@ -156,59 +149,46 @@ public class PremiumListTest { @Test public void testGetPremiumPrice_allLabelsAreNonPremium_whenNotInList() throws Exception { - assertThat(getPremiumPrice("blah", "tld")).isAbsent(); - assertThat(getPremiumPrice("slinge", "tld")).isAbsent(); + assertThat(getPremiumPrice("blah", Registry.get("tld"))).isAbsent(); + assertThat(getPremiumPrice("slinge", Registry.get("tld"))).isAbsent(); } @Test public void testSave_simple() throws Exception { - PremiumList pl = persistPremiumList("tld2", "lol , USD 999 # yupper rooni "); + PremiumList pl = + saveWithEntries( + new PremiumList.Builder().setName("tld2").build(), + ImmutableList.of("lol , USD 999 # yupper rooni ")); createTld("tld"); persistResource(Registry.get("tld").asBuilder().setPremiumList(pl).build()); - assertThat(pl.getPremiumPrice("lol")).hasValue(Money.parse("USD 999")); - assertThat(getPremiumPrice("lol", "tld")).hasValue(Money.parse("USD 999")); - assertThat(getPremiumPrice("lol ", "tld")).isAbsent(); - Map entries = PremiumList.get("tld2").get().getPremiumListEntries(); + assertThat(getPremiumPrice("lol", Registry.get("tld"))).hasValue(Money.parse("USD 999")); + assertThat(getPremiumPrice("lol ", Registry.get("tld"))).isAbsent(); + Map entries = + PremiumList.get("tld2").get().loadPremiumListEntries(); assertThat(entries.keySet()).containsExactly("lol"); assertThat(entries).doesNotContainKey("lol "); - PremiumListEntry entry = entries.values().iterator().next(); + PremiumListEntry entry = entries.get("lol"); assertThat(entry.comment).isEqualTo("yupper rooni"); assertThat(entry.price).isEqualTo(Money.parse("USD 999")); assertThat(entry.label).isEqualTo("lol"); } @Test - public void test_saveAndUpdateEntries_twiceOnUnchangedList() throws Exception { + public void test_saveAndUpdateEntriesTwice() throws Exception { PremiumList pl = - new PremiumList.Builder() - .setName("pl") - .setPremiumListMapFromLines(ImmutableList.of("test,USD 1")) - .build() - .saveAndUpdateEntries(); - Map entries = pl.getPremiumListEntries(); + saveWithEntries( + new PremiumList.Builder().setName("pl").build(), ImmutableList.of("test,USD 1")); + Map entries = pl.loadPremiumListEntries(); assertThat(entries.keySet()).containsExactly("test"); - assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries); + assertThat(PremiumList.get("pl").get().loadPremiumListEntries()).isEqualTo(entries); // Save again with no changes, and clear the cache to force a re-load from Datastore. - pl.saveAndUpdateEntries(); + PremiumList resaved = saveWithEntries(pl, ImmutableList.of("test,USD 1")); 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 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); + Map entriesReloaded = + PremiumList.get("pl").get().loadPremiumListEntries(); + assertThat(entriesReloaded).hasSize(1); + assertThat(entriesReloaded).containsKey("test"); + assertThat(entriesReloaded.get("test").parent).isEqualTo(resaved.getRevisionKey()); } @Test @@ -253,22 +233,33 @@ public class PremiumListTest { } @Test - public void testAsBuilder_updatingEntitiesreplacesRevisionKey() throws Exception { + public void testGetPremiumPrice_comesFromBloomFilter() throws Exception { PremiumList pl = PremiumList.get("tld").get(); - assertThat(pl.asBuilder() - .setPremiumListMapFromLines(ImmutableList.of("qux,USD 123")) - .build() - .getRevisionKey()) - .isNotEqualTo(pl.getRevisionKey()); + PremiumListEntry entry = + persistResource( + new PremiumListEntry.Builder() + .setParent(pl.getRevisionKey()) + .setLabel("missingno") + .setPrice(Money.parse("USD 1000")) + .build()); + // "missingno" shouldn't be in the bloom filter, thus it should return not premium without + // attempting to load the entity that is actually present. + assertThat(getPremiumPrice("missingno", Registry.get("tld"))).isAbsent(); + // However, if we manually query the cache to force an entity load, it should be found. + assertThat( + cachePremiumListEntries.get( + Key.create(pl.getRevisionKey(), PremiumListEntry.class, "missingno"))) + .hasValue(entry); } @Test public void testProbablePremiumLabels() throws Exception { PremiumList pl = PremiumList.get("tld").get(); - assertThat(pl.getRevision().probablePremiumLabels.mightContain("notpremium")).isFalse(); + PremiumListRevision revision = ofy().load().key(pl.getRevisionKey()).now(); + assertThat(revision.probablePremiumLabels.mightContain("notpremium")).isFalse(); for (String label : ImmutableList.of("rich", "lol", "johnny-be-goode", "icann")) { assertWithMessage(label + " should be a probable premium") - .that(pl.getRevision().probablePremiumLabels.mightContain(label)) + .that(revision.probablePremiumLabels.mightContain(label)) .isTrue(); } } diff --git a/javatests/google/registry/testing/DatastoreHelper.java b/javatests/google/registry/testing/DatastoreHelper.java index b6b7b3ad6..f758b6924 100644 --- a/javatests/google/registry/testing/DatastoreHelper.java +++ b/javatests/google/registry/testing/DatastoreHelper.java @@ -27,6 +27,7 @@ import static google.registry.model.EppResourceUtils.createDomainRepoId; import static google.registry.model.EppResourceUtils.createRepoId; import static google.registry.model.domain.launch.ApplicationStatus.VALIDATED; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.registry.label.PremiumList.parentEntriesOnRevision; import static google.registry.pricing.PricingEngineProxy.getDomainRenewCost; import static google.registry.util.CollectionUtils.difference; import static google.registry.util.CollectionUtils.union; @@ -35,16 +36,17 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME; import static google.registry.util.DomainNameUtils.ACE_PREFIX_REGEX; import static google.registry.util.DomainNameUtils.getTldFromDomainName; import static google.registry.util.ResourceUtils.readResourceUtf8; +import static java.util.Arrays.asList; import static org.joda.money.CurrencyUnit.USD; import com.google.common.base.Ascii; import com.google.common.base.Function; -import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.base.Splitter; import com.google.common.base.Supplier; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Iterables; @@ -83,6 +85,8 @@ import google.registry.model.registrar.Registrar; import google.registry.model.registry.Registry; import google.registry.model.registry.Registry.TldState; import google.registry.model.registry.label.PremiumList; +import google.registry.model.registry.label.PremiumList.PremiumListEntry; +import google.registry.model.registry.label.PremiumList.PremiumListRevision; import google.registry.model.registry.label.ReservedList; import google.registry.model.reporting.HistoryEntry; import google.registry.model.smd.EncodedSignedMark; @@ -343,23 +347,27 @@ public class DatastoreHelper { .build()); } + /** + * Persists a premium list and its child entities directly without writing commit logs. + * + *

Avoiding commit logs is important because a simple default premium list is persisted for + * each TLD that is created in tests, and clocks would need to be mocked using an auto- + * incrementing FakeClock for all tests in order to persist the commit logs properly because of + * the requirement to have monotonically increasing timestamps. + */ public static PremiumList persistPremiumList(String listName, String... lines) { - Optional existing = PremiumList.get(listName); - return persistPremiumList( - (existing.isPresent() ? existing.get().asBuilder() : new PremiumList.Builder()) - .setName(listName) - .setPremiumListMapFromLines(ImmutableList.copyOf(lines)) - .build()); - } - - private static PremiumList persistPremiumList(PremiumList premiumList) { - // Persist the list and its child entities directly, rather than using its helper method, so - // that we can avoid writing commit logs. This would cause issues since many tests replace the - // clock in Ofy with a non-advancing FakeClock, and commit logs currently require - // monotonically increasing timestamps. - ofy().saveWithoutBackup().entities(premiumList, premiumList.getRevision()).now(); - ofy().saveWithoutBackup().entities(premiumList.getPremiumListEntries().values()).now(); - return premiumList; + PremiumList premiumList = new PremiumList.Builder().setName(listName).build(); + ImmutableMap entries = premiumList.parse(asList(lines)); + PremiumListRevision revision = PremiumListRevision.create(premiumList, entries.keySet()); + ofy() + .saveWithoutBackup() + .entities(premiumList.asBuilder().setRevision(Key.create(revision)).build(), revision) + .now(); + ofy() + .saveWithoutBackup() + .entities(parentEntriesOnRevision(entries.values(), Key.create(revision))) + .now(); + return ofy().load().entity(premiumList).now(); } /** Creates and persists a tld. */ diff --git a/javatests/google/registry/tools/DeletePremiumListCommandTest.java b/javatests/google/registry/tools/DeletePremiumListCommandTest.java index a41a3f997..bd496d8ac 100644 --- a/javatests/google/registry/tools/DeletePremiumListCommandTest.java +++ b/javatests/google/registry/tools/DeletePremiumListCommandTest.java @@ -32,7 +32,7 @@ public class DeletePremiumListCommandTest extends CommandTestCase