diff --git a/docs/operational-procedures/premium-list-management.md b/docs/operational-procedures/premium-list-management.md index 08a404124..33b33971d 100644 --- a/docs/operational-procedures/premium-list-management.md +++ b/docs/operational-procedures/premium-list-management.md @@ -7,8 +7,9 @@ prices of domain labels (i.e. the part of the domain name without the TLD) by checking for their presence on a list of prices in Datastore. `nomulus` is used to load and update these lists from flat text files. The format of this list is simple: It is a newline-delimited CSV text file with each line containing the -label and its price (including currency specifier in ISO-4217 format). As an -example: +label and its price (including currency specifier in ISO-4217 format). Any +individual label may not appear more than once in the file. Here's an example of +the formatting: ``` premium,USD 100 diff --git a/docs/operational-procedures/reserved-list-management.md b/docs/operational-procedures/reserved-list-management.md index d27d14d5e..1cf9608f8 100644 --- a/docs/operational-procedures/reserved-list-management.md +++ b/docs/operational-procedures/reserved-list-management.md @@ -6,7 +6,7 @@ for various reasons, usually because of potential abuse. ## Reserved list file format Reserved lists are handled in a similar way to [premium -lists](./premium-list-management.md), except that rather than each label having +lists](./premium-list-management.md), except that instead of each label having a price, it has a reservation type. The valid values for reservation types are: * **`UNRESERVED`** - The default value for any label that isn't reserved. @@ -29,11 +29,11 @@ a price, it has a reservation type. The valid values for reservation types are: specified. The reservation types are listed in order of increasing precedence, so if a -label is included on the same list multiple times, or on different lists that -are applied to a single TLD, whichever reservation type is later in the list -takes precedence. E.g. a label being fully blocked in one list always supersedes -it being allowed in sunrise from another list. In general `FULLY_BLOCKED` is by -far the most widely used reservation type for typical TLD use cases. +label is included on different lists that are applied to a single TLD, whichever +reservation type is later in the list takes precedence. E.g. a label being fully +blocked in one list always supersedes it being allowed in sunrise from another +list. In general `FULLY_BLOCKED` is by far the most widely used reservation type +for typical TLD use cases. Here's an example of a small reserved list. Note that `RESERVED_FOR_ANCHOR_TENANT` is the only reservation type that has a third entry diff --git a/java/google/registry/model/registry/label/BaseDomainLabelList.java b/java/google/registry/model/registry/label/BaseDomainLabelList.java index e822c3c33..7f5c34bb4 100644 --- a/java/google/registry/model/registry/label/BaseDomainLabelList.java +++ b/java/google/registry/model/registry/label/BaseDomainLabelList.java @@ -23,10 +23,11 @@ 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; +import com.google.common.collect.HashMultiset; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Ordering; +import com.google.common.collect.Multiset; import com.google.common.util.concurrent.UncheckedExecutionException; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Id; @@ -85,16 +86,26 @@ public abstract class BaseDomainLabelList, R extends Dom @VisibleForTesting protected ImmutableMap parse(Iterable lines) { Map labelsToEntries = new HashMap<>(); + Multiset duplicateLabels = HashMultiset.create(); for (String line : lines) { R entry = createFromLine(line); if (entry == null) { continue; } String label = entry.getLabel(); - // Adds the label to the list of all labels if it (a) doesn't already exist, or (b) already - // exists, but the new value has higher priority (as determined by sort order). - labelsToEntries.put( - label, Ordering.natural().nullsFirst().max(labelsToEntries.get(label), entry)); + // Check if the label was already processed for this list (which is an error), and if so, + // accumulate it so that a list of all duplicates can be thrown. + if (labelsToEntries.containsKey(label)) { + duplicateLabels.add(label, duplicateLabels.contains(label) ? 1 : 2); + } else { + labelsToEntries.put(label, entry); + } + } + if (!duplicateLabels.isEmpty()) { + throw new IllegalStateException( + String.format( + "List '%s' cannot contain duplicate labels. Dupes (with counts) were: %s", + name, duplicateLabels)); } return ImmutableMap.copyOf(labelsToEntries); } diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java index bbb1948da..6c58ccb13 100644 --- a/javatests/google/registry/model/registry/label/PremiumListTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListTest.java @@ -273,6 +273,18 @@ public class PremiumListTest { } } + @Test + public void testParse_cannotIncludeDuplicateLabels() { + thrown.expect( + IllegalStateException.class, + "List 'tld' cannot contain duplicate labels. Dupes (with counts) were: [lol x 2]"); + PremiumList.get("tld") + .get() + .parse( + ImmutableList.of( + "lol,USD 100", "rofl,USD 90", "paper,USD 80", "wood,USD 70", "lol,USD 200")); + } + /** Persists a premium list with a specified number of nonsense entries. */ private PremiumList persistHumongousPremiumList(String name, int size) { String[] entries = new String[size]; diff --git a/javatests/google/registry/model/registry/label/ReservedListTest.java b/javatests/google/registry/model/registry/label/ReservedListTest.java index 968ca30f6..3940819ff 100644 --- a/javatests/google/registry/model/registry/label/ReservedListTest.java +++ b/javatests/google/registry/model/registry/label/ReservedListTest.java @@ -291,6 +291,21 @@ public class ReservedListTest { .build()); } + @Test + public void testParse_cannotIncludeDuplicateLabels() { + ReservedList rl = new ReservedList.Builder().setName("blah").build(); + thrown.expect( + IllegalStateException.class, + "List 'blah' cannot contain duplicate labels. Dupes (with counts) were: [lol x 2]"); + rl.parse( + ImmutableList.of( + "lol,FULLY_BLOCKED", + "rofl,FULLY_BLOCKED", + "paper,FULLY_BLOCKED", + "wood,FULLY_BLOCKED", + "lol,FULLY_BLOCKED")); + } + /** Gets the name of a reserved list. */ public static final Function, String> GET_NAME_FUNCTION = new Function, String>() { diff --git a/javatests/google/registry/testing/default_premium_list_testdata.csv b/javatests/google/registry/testing/default_premium_list_testdata.csv index d29ca7f40..edd206e01 100644 --- a/javatests/google/registry/testing/default_premium_list_testdata.csv +++ b/javatests/google/registry/testing/default_premium_list_testdata.csv @@ -10,4 +10,3 @@ palladium,USD 877 aluminum,USD 11 copper,USD 15 brass,USD 20 -platinum,USD 80000 #this should be ignored as a cheaper duplicate