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