From 9d2b1e7572715146db5c6b24753c8282347586e1 Mon Sep 17 00:00:00 2001 From: guyben Date: Thu, 31 May 2018 18:08:02 -0700 Subject: [PATCH] Consolidate all Set parameter parsing Currently, we have two different ways to parse a "set" parameter: key=value1&key=value2&key=value3... and keys=value1,value2,value3 This is error prone for several reasons: - different parts of the code must be "synchronized" to use the same style (the place that creates the request, and the place that parses the request) - for the key=value1&key=value2, we often use the same key name for the single value and the set value. This can result in subtle bugs where part of the code will successfully read the key assuming there's only one key (and will get the first key=value1, ignoring the rest) Here we transition everything to the keys=value1,value2,value3 method. This one was chosen because: - it's shorter - it's more intuitive for users - the key name is plural, differentiating it from the singular key=value that other requests might need ----------------------------------- To make sure there are not "transition issues", we will continue to support (with warnings) the key=value1&key=value2 parameter parsing until we're sure we haven't forgotten to update any part of the code. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198810681 --- .../batch/DeleteProberDataAction.java | 4 +- java/google/registry/cron/CronModule.java | 36 +++--- .../google/registry/cron/TldFanoutAction.java | 28 ++--- java/google/registry/dns/DnsModule.java | 15 +-- .../registry/dns/PublishDnsUpdatesAction.java | 14 +-- .../registry/dns/ReadDnsQueueAction.java | 53 +++++---- .../env/alpha/default/WEB-INF/cron.xml | 10 +- .../env/crash/default/WEB-INF/cron.xml | 10 +- .../env/production/default/WEB-INF/cron.xml | 10 +- .../env/sandbox/default/WEB-INF/cron.xml | 10 +- .../module/backend/BackendModule.java | 5 +- java/google/registry/rde/RdeModule.java | 5 +- .../google/registry/rde/RdeStagingAction.java | 4 +- .../registry/request/RequestParameters.java | 97 ++++++++++++---- .../tools/GenerateEscrowDepositCommand.java | 27 +++-- .../tools/server/ListDomainsAction.java | 3 +- .../server/RefreshDnsForAllDomainsAction.java | 3 +- .../tools/server/ToolsServerModule.java | 12 +- .../registry/dns/ReadDnsQueueActionTest.java | 107 +++++++++++------- .../GenerateEscrowDepositCommandTest.java | 18 ++- 20 files changed, 290 insertions(+), 181 deletions(-) diff --git a/java/google/registry/batch/DeleteProberDataAction.java b/java/google/registry/batch/DeleteProberDataAction.java index a7f9fad09..91bf9c320 100644 --- a/java/google/registry/batch/DeleteProberDataAction.java +++ b/java/google/registry/batch/DeleteProberDataAction.java @@ -23,7 +23,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registry.Registries.getTldsOfType; import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_DELETE; import static google.registry.request.Action.Method.POST; -import static google.registry.request.RequestParameters.PARAM_TLD; +import static google.registry.request.RequestParameters.PARAM_TLDS; import static org.joda.time.DateTimeZone.UTC; import com.google.appengine.tools.mapreduce.Mapper; @@ -76,7 +76,7 @@ public class DeleteProberDataAction implements Runnable { @Inject @Parameter(PARAM_DRY_RUN) boolean isDryRun; /** List of TLDs to work on. If empty - will work on all TLDs that end with .test. */ - @Inject @Parameter(PARAM_TLD) ImmutableSet tlds; + @Inject @Parameter(PARAM_TLDS) ImmutableSet tlds; @Inject @Config("registryAdminClientId") String registryAdminClientId; @Inject MapreduceRunner mrRunner; @Inject Response response; diff --git a/java/google/registry/cron/CronModule.java b/java/google/registry/cron/CronModule.java index d2dbd09e1..3927b04ba 100644 --- a/java/google/registry/cron/CronModule.java +++ b/java/google/registry/cron/CronModule.java @@ -30,45 +30,53 @@ import javax.servlet.http.HttpServletRequest; @Module public final class CronModule { + public static final String ENDPOINT_PARAM = "endpoint"; + public static final String QUEUE_PARAM = "queue"; + public static final String FOR_EACH_REAL_TLD_PARAM = "forEachRealTld"; + public static final String FOR_EACH_TEST_TLD_PARAM = "forEachTestTld"; + public static final String RUN_IN_EMPTY_PARAM = "runInEmpty"; + public static final String EXCLUDE_PARAM = "exclude"; + public static final String JITTER_SECONDS_PARAM = "jitterSeconds"; + @Provides - @Parameter("endpoint") + @Parameter(ENDPOINT_PARAM) static String provideEndpoint(HttpServletRequest req) { - return extractRequiredParameter(req, "endpoint"); + return extractRequiredParameter(req, ENDPOINT_PARAM); } @Provides - @Parameter("exclude") + @Parameter(EXCLUDE_PARAM) static ImmutableSet provideExcludes(HttpServletRequest req) { - return extractSetOfParameters(req, "exclude"); + return extractSetOfParameters(req, EXCLUDE_PARAM); } @Provides - @Parameter("queue") + @Parameter(QUEUE_PARAM) static String provideQueue(HttpServletRequest req) { - return extractRequiredParameter(req, "queue"); + return extractRequiredParameter(req, QUEUE_PARAM); } @Provides - @Parameter("runInEmpty") + @Parameter(RUN_IN_EMPTY_PARAM) static boolean provideRunInEmpty(HttpServletRequest req) { - return extractBooleanParameter(req, "runInEmpty"); + return extractBooleanParameter(req, RUN_IN_EMPTY_PARAM); } @Provides - @Parameter("forEachRealTld") + @Parameter(FOR_EACH_REAL_TLD_PARAM) static boolean provideForEachRealTld(HttpServletRequest req) { - return extractBooleanParameter(req, "forEachRealTld"); + return extractBooleanParameter(req, FOR_EACH_REAL_TLD_PARAM); } @Provides - @Parameter("forEachTestTld") + @Parameter(FOR_EACH_TEST_TLD_PARAM) static boolean provideForEachTestTld(HttpServletRequest req) { - return extractBooleanParameter(req, "forEachTestTld"); + return extractBooleanParameter(req, FOR_EACH_TEST_TLD_PARAM); } @Provides - @Parameter("jitterSeconds") + @Parameter(JITTER_SECONDS_PARAM) static Optional provideJitterSeconds(HttpServletRequest req) { - return extractOptionalIntParameter(req, "jitterSeconds"); + return extractOptionalIntParameter(req, JITTER_SECONDS_PARAM); } } diff --git a/java/google/registry/cron/TldFanoutAction.java b/java/google/registry/cron/TldFanoutAction.java index 7f50aaaf2..3677a8585 100644 --- a/java/google/registry/cron/TldFanoutAction.java +++ b/java/google/registry/cron/TldFanoutAction.java @@ -24,6 +24,13 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.getFirst; import static com.google.common.collect.Multimaps.filterKeys; import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; +import static google.registry.cron.CronModule.ENDPOINT_PARAM; +import static google.registry.cron.CronModule.EXCLUDE_PARAM; +import static google.registry.cron.CronModule.FOR_EACH_REAL_TLD_PARAM; +import static google.registry.cron.CronModule.FOR_EACH_TEST_TLD_PARAM; +import static google.registry.cron.CronModule.JITTER_SECONDS_PARAM; +import static google.registry.cron.CronModule.QUEUE_PARAM; +import static google.registry.cron.CronModule.RUN_IN_EMPTY_PARAM; import static google.registry.model.registry.Registries.getTldsOfType; import static google.registry.model.registry.Registry.TldType.REAL; import static google.registry.model.registry.Registry.TldType.TEST; @@ -80,14 +87,6 @@ import javax.inject.Inject; ) public final class TldFanoutAction implements Runnable { - private static final String ENDPOINT_PARAM = "endpoint"; - private static final String QUEUE_PARAM = "queue"; - private static final String FOR_EACH_REAL_TLD_PARAM = "forEachRealTld"; - private static final String FOR_EACH_TEST_TLD_PARAM = "forEachTestTld"; - private static final String RUN_IN_EMPTY_PARAM = "runInEmpty"; - private static final String EXCLUDE_PARAM = "exclude"; - private static final String JITTER_SECONDS_PARAM = "jitterSeconds"; - /** A set of control params to TldFanoutAction that aren't passed down to the executing action. */ private static final ImmutableSet CONTROL_PARAMS = ImmutableSet.of( @@ -127,12 +126,13 @@ public final class TldFanoutAction implements Runnable { !(runInEmpty && !excludes.isEmpty()), "Can't specify 'exclude' with 'runInEmpty'"); ImmutableSet tlds = - Streams.concat( - runInEmpty ? Stream.of("") : Stream.of(), - forEachRealTld ? getTldsOfType(REAL).stream() : Stream.of(), - forEachTestTld ? getTldsOfType(TEST).stream() : Stream.of()) - .filter(not(in(excludes))) - .collect(toImmutableSet()); + runInEmpty + ? ImmutableSet.of("") + : Streams.concat( + forEachRealTld ? getTldsOfType(REAL).stream() : Stream.of(), + forEachTestTld ? getTldsOfType(TEST).stream() : Stream.of()) + .filter(not(in(excludes))) + .collect(toImmutableSet()); Multimap flowThruParams = filterKeys(params, not(in(CONTROL_PARAMS))); Queue taskQueue = getQueue(queue); StringBuilder outputPayload = diff --git a/java/google/registry/dns/DnsModule.java b/java/google/registry/dns/DnsModule.java index 6489dd3c1..4a06206c2 100644 --- a/java/google/registry/dns/DnsModule.java +++ b/java/google/registry/dns/DnsModule.java @@ -16,13 +16,6 @@ package google.registry.dns; import static google.registry.dns.DnsConstants.DNS_PUBLISH_PUSH_QUEUE_NAME; import static google.registry.dns.DnsConstants.DNS_PULL_QUEUE_NAME; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_DNS_WRITER; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_DOMAINS; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_HOSTS; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_LOCK_INDEX; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_NUM_PUBLISH_LOCKS; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_PUBLISH_TASK_ENQUEUED; -import static google.registry.dns.PublishDnsUpdatesAction.PARAM_REFRESH_REQUEST_CREATED; import static google.registry.request.RequestParameters.extractEnumParameter; import static google.registry.request.RequestParameters.extractIntParameter; import static google.registry.request.RequestParameters.extractRequiredParameter; @@ -48,6 +41,14 @@ import org.joda.time.DateTime; @Module public abstract class DnsModule { + public static final String PARAM_DNS_WRITER = "dnsWriter"; + public static final String PARAM_LOCK_INDEX = "lockIndex"; + public static final String PARAM_NUM_PUBLISH_LOCKS = "numPublishLocks"; + public static final String PARAM_DOMAINS = "domains"; + public static final String PARAM_HOSTS = "hosts"; + public static final String PARAM_PUBLISH_TASK_ENQUEUED = "enqueued"; + public static final String PARAM_REFRESH_REQUEST_CREATED = "itemsCreated"; + @Binds @DnsWriterZone abstract String provideZoneName(@Parameter(RequestParameters.PARAM_TLD) String tld); diff --git a/java/google/registry/dns/PublishDnsUpdatesAction.java b/java/google/registry/dns/PublishDnsUpdatesAction.java index 08c89fbec..0141b74aa 100644 --- a/java/google/registry/dns/PublishDnsUpdatesAction.java +++ b/java/google/registry/dns/PublishDnsUpdatesAction.java @@ -14,6 +14,13 @@ package google.registry.dns; +import static google.registry.dns.DnsModule.PARAM_DNS_WRITER; +import static google.registry.dns.DnsModule.PARAM_DOMAINS; +import static google.registry.dns.DnsModule.PARAM_HOSTS; +import static google.registry.dns.DnsModule.PARAM_LOCK_INDEX; +import static google.registry.dns.DnsModule.PARAM_NUM_PUBLISH_LOCKS; +import static google.registry.dns.DnsModule.PARAM_PUBLISH_TASK_ENQUEUED; +import static google.registry.dns.DnsModule.PARAM_REFRESH_REQUEST_CREATED; import static google.registry.request.Action.Method.POST; import static google.registry.request.RequestParameters.PARAM_TLD; import static google.registry.util.CollectionUtils.nullToEmpty; @@ -49,13 +56,6 @@ import org.joda.time.Duration; public final class PublishDnsUpdatesAction implements Runnable, Callable { public static final String PATH = "/_dr/task/publishDnsUpdates"; - public static final String PARAM_DNS_WRITER = "dnsWriter"; - public static final String PARAM_LOCK_INDEX = "lockIndex"; - public static final String PARAM_NUM_PUBLISH_LOCKS = "numPublishLocks"; - public static final String PARAM_DOMAINS = "domains"; - public static final String PARAM_HOSTS = "hosts"; - public static final String PARAM_PUBLISH_TASK_ENQUEUED = "enqueued"; - public static final String PARAM_REFRESH_REQUEST_CREATED = "itemsCreated"; public static final String LOCK_NAME = "DNS updates"; private static final FluentLogger logger = FluentLogger.forEnclosingClass(); diff --git a/java/google/registry/dns/ReadDnsQueueAction.java b/java/google/registry/dns/ReadDnsQueueAction.java index 21a8e6e94..d6bfec63c 100644 --- a/java/google/registry/dns/ReadDnsQueueAction.java +++ b/java/google/registry/dns/ReadDnsQueueAction.java @@ -14,13 +14,20 @@ package google.registry.dns; -import static com.google.appengine.api.taskqueue.TaskOptions.Builder.withUrl; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static com.google.common.collect.Sets.difference; import static google.registry.dns.DnsConstants.DNS_PUBLISH_PUSH_QUEUE_NAME; import static google.registry.dns.DnsConstants.DNS_TARGET_CREATE_TIME_PARAM; import static google.registry.dns.DnsConstants.DNS_TARGET_NAME_PARAM; import static google.registry.dns.DnsConstants.DNS_TARGET_TYPE_PARAM; +import static google.registry.dns.DnsModule.PARAM_DNS_WRITER; +import static google.registry.dns.DnsModule.PARAM_DOMAINS; +import static google.registry.dns.DnsModule.PARAM_HOSTS; +import static google.registry.dns.DnsModule.PARAM_LOCK_INDEX; +import static google.registry.dns.DnsModule.PARAM_NUM_PUBLISH_LOCKS; +import static google.registry.dns.DnsModule.PARAM_PUBLISH_TASK_ENQUEUED; +import static google.registry.dns.DnsModule.PARAM_REFRESH_REQUEST_CREATED; +import static google.registry.request.RequestParameters.PARAM_TLD; import static google.registry.util.DomainNameUtils.getSecondLevelDomain; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.SECONDS; @@ -44,7 +51,6 @@ import google.registry.model.registry.Registries; import google.registry.model.registry.Registry; import google.registry.request.Action; import google.registry.request.Parameter; -import google.registry.request.RequestParameters; import google.registry.request.auth.Auth; import google.registry.util.Clock; import google.registry.util.TaskQueueUtils; @@ -55,6 +61,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Random; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.inject.Named; import org.joda.time.DateTime; @@ -265,7 +272,7 @@ public final class ReadDnsQueueAction implements Runnable { try { Map params = ImmutableMap.copyOf(task.extractParams()); DateTime creationTime = DateTime.parse(params.get(DNS_TARGET_CREATE_TIME_PARAM)); - String tld = params.get(RequestParameters.PARAM_TLD); + String tld = params.get(PARAM_TLD); if (tld == null) { logger.atSevere().log( "Discarding invalid DNS refresh request %s; no TLD specified.", task); @@ -353,31 +360,33 @@ public final class ReadDnsQueueAction implements Runnable { DateTime earliestCreateTime = chunk.stream().map(RefreshItem::creationTime).min(Comparator.naturalOrder()).get(); for (String dnsWriter : Registry.get(tld).getDnsWriters()) { - TaskOptions options = - withUrl(PublishDnsUpdatesAction.PATH) + taskQueueUtils.enqueue( + dnsPublishPushQueue, + TaskOptions.Builder.withUrl(PublishDnsUpdatesAction.PATH) .countdownMillis( jitterSeconds .map(seconds -> random.nextInt((int) SECONDS.toMillis(seconds))) .orElse(0)) - .param(RequestParameters.PARAM_TLD, tld) - .param(PublishDnsUpdatesAction.PARAM_DNS_WRITER, dnsWriter) - .param(PublishDnsUpdatesAction.PARAM_LOCK_INDEX, Integer.toString(lockIndex)) + .param(PARAM_TLD, tld) + .param(PARAM_DNS_WRITER, dnsWriter) + .param(PARAM_LOCK_INDEX, Integer.toString(lockIndex)) + .param(PARAM_NUM_PUBLISH_LOCKS, Integer.toString(numPublishLocks)) + .param(PARAM_PUBLISH_TASK_ENQUEUED, clock.nowUtc().toString()) + .param(PARAM_REFRESH_REQUEST_CREATED, earliestCreateTime.toString()) .param( - PublishDnsUpdatesAction.PARAM_NUM_PUBLISH_LOCKS, - Integer.toString(numPublishLocks)) + PARAM_DOMAINS, + chunk + .stream() + .filter(item -> item.type() == TargetType.DOMAIN) + .map(RefreshItem::name) + .collect(Collectors.joining(","))) .param( - PublishDnsUpdatesAction.PARAM_PUBLISH_TASK_ENQUEUED, clock.nowUtc().toString()) - .param( - PublishDnsUpdatesAction.PARAM_REFRESH_REQUEST_CREATED, - earliestCreateTime.toString()); - for (RefreshItem refreshItem : chunk) { - options.param( - (refreshItem.type() == TargetType.HOST) - ? PublishDnsUpdatesAction.PARAM_HOSTS - : PublishDnsUpdatesAction.PARAM_DOMAINS, - refreshItem.name()); - } - taskQueueUtils.enqueue(dnsPublishPushQueue, options); + PARAM_HOSTS, + chunk + .stream() + .filter(item -> item.type() == TargetType.HOST) + .map(RefreshItem::name) + .collect(Collectors.joining(",")))); } } } diff --git a/java/google/registry/env/alpha/default/WEB-INF/cron.xml b/java/google/registry/env/alpha/default/WEB-INF/cron.xml index f833af46f..62d31c406 100644 --- a/java/google/registry/env/alpha/default/WEB-INF/cron.xml +++ b/java/google/registry/env/alpha/default/WEB-INF/cron.xml @@ -4,11 +4,11 @@ diff --git a/java/google/registry/env/crash/default/WEB-INF/cron.xml b/java/google/registry/env/crash/default/WEB-INF/cron.xml index 730f04024..cab6f5fef 100644 --- a/java/google/registry/env/crash/default/WEB-INF/cron.xml +++ b/java/google/registry/env/crash/default/WEB-INF/cron.xml @@ -4,11 +4,11 @@ diff --git a/java/google/registry/env/production/default/WEB-INF/cron.xml b/java/google/registry/env/production/default/WEB-INF/cron.xml index 14cda880d..c1db58a70 100644 --- a/java/google/registry/env/production/default/WEB-INF/cron.xml +++ b/java/google/registry/env/production/default/WEB-INF/cron.xml @@ -4,11 +4,11 @@ diff --git a/java/google/registry/env/sandbox/default/WEB-INF/cron.xml b/java/google/registry/env/sandbox/default/WEB-INF/cron.xml index c36dfc566..ac5412605 100644 --- a/java/google/registry/env/sandbox/default/WEB-INF/cron.xml +++ b/java/google/registry/env/sandbox/default/WEB-INF/cron.xml @@ -4,11 +4,11 @@ diff --git a/java/google/registry/module/backend/BackendModule.java b/java/google/registry/module/backend/BackendModule.java index c617c7d86..a84a7fa1c 100644 --- a/java/google/registry/module/backend/BackendModule.java +++ b/java/google/registry/module/backend/BackendModule.java @@ -43,9 +43,10 @@ public class BackendModule { } @Provides - @Parameter(RequestParameters.PARAM_TLD) + @Parameter(RequestParameters.PARAM_TLDS) static ImmutableSet provideTlds(HttpServletRequest req) { - ImmutableSet tlds = extractSetOfParameters(req, RequestParameters.PARAM_TLD); + ImmutableSet tlds = + extractSetOfParameters(req, RequestParameters.PARAM_TLDS, RequestParameters.PARAM_TLD); assertTldsExist(tlds); return tlds; } diff --git a/java/google/registry/rde/RdeModule.java b/java/google/registry/rde/RdeModule.java index ac22b7f12..9bfaba82e 100644 --- a/java/google/registry/rde/RdeModule.java +++ b/java/google/registry/rde/RdeModule.java @@ -41,6 +41,7 @@ import org.joda.time.DateTime; public final class RdeModule { public static final String PARAM_WATERMARK = "watermark"; + public static final String PARAM_WATERMARKS = "watermarks"; public static final String PARAM_MANUAL = "manual"; public static final String PARAM_DIRECTORY = "directory"; public static final String PARAM_MODE = "mode"; @@ -54,9 +55,9 @@ public final class RdeModule { } @Provides - @Parameter(PARAM_WATERMARK) + @Parameter(PARAM_WATERMARKS) static ImmutableSet provideWatermarks(HttpServletRequest req) { - return extractSetOfDatetimeParameters(req, PARAM_WATERMARK); + return extractSetOfDatetimeParameters(req, PARAM_WATERMARKS, PARAM_WATERMARK); } @Provides diff --git a/java/google/registry/rde/RdeStagingAction.java b/java/google/registry/rde/RdeStagingAction.java index 046ab5a2a..fed0bc3a2 100644 --- a/java/google/registry/rde/RdeStagingAction.java +++ b/java/google/registry/rde/RdeStagingAction.java @@ -205,8 +205,8 @@ public final class RdeStagingAction implements Runnable { @Inject @Parameter(RdeModule.PARAM_MANUAL) boolean manual; @Inject @Parameter(RdeModule.PARAM_DIRECTORY) Optional directory; @Inject @Parameter(RdeModule.PARAM_MODE) ImmutableSet modeStrings; - @Inject @Parameter(RequestParameters.PARAM_TLD) ImmutableSet tlds; - @Inject @Parameter(RdeModule.PARAM_WATERMARK) ImmutableSet watermarks; + @Inject @Parameter(RequestParameters.PARAM_TLDS) ImmutableSet tlds; + @Inject @Parameter(RdeModule.PARAM_WATERMARKS) ImmutableSet watermarks; @Inject @Parameter(RdeModule.PARAM_REVISION) Optional revision; @Inject @Parameter(RdeModule.PARAM_LENIENT) boolean lenient; diff --git a/java/google/registry/request/RequestParameters.java b/java/google/registry/request/RequestParameters.java index e24b4317a..23b5b5412 100644 --- a/java/google/registry/request/RequestParameters.java +++ b/java/google/registry/request/RequestParameters.java @@ -14,12 +14,16 @@ package google.registry.request; +import static com.google.common.base.Predicates.not; import static com.google.common.base.Strings.emptyToNull; import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.base.Strings.nullToEmpty; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.base.Ascii; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.FluentLogger; import google.registry.request.HttpException.BadRequestException; import java.util.Optional; import javax.annotation.Nullable; @@ -29,8 +33,12 @@ import org.joda.time.DateTime; /** Utilities for extracting parameters from HTTP requests. */ public final class RequestParameters { - /** The standardized request parameter name used by any action that takes a tld parameter. */ + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + /** The standardized request parameter name used by any action taking a tld parameter. */ public static final String PARAM_TLD = "tld"; + /** The standardized request parameter name used by any action taking multiple tld parameters. */ + public static final String PARAM_TLDS = "tlds"; /** * Returns first GET or POST parameter associated with {@code name}. @@ -90,10 +98,67 @@ public final class RequestParameters { } } - /** Returns all GET or POST parameters associated with {@code name}. */ - public static ImmutableSet extractSetOfParameters(HttpServletRequest req, String name) { + /** + * Returns all GET or POST parameters associated with {@code name} (or {@code nameLegacy}). + * + *

While transitioning from "param=key1¶m=key2" to "params=key1,key2" style of set inputing + * - we will be accepting all options (with a warning for "wrong" uses). + * + *

TODO(b/78226288): remove transition code (including "legacyName") once there are no more + * warnings. + * + * @param req the request that has the parameter + * @param name the name of the parameter, should be in plural form (e.g. tlds=com,net) + * @param legacyName the legacy, singular form, name. Used only while transitioning from the + * tld=com&tld=net form, in case we forgot to fix some reference. Null if there was no + * singular form to transition from. + */ + public static ImmutableSet extractSetOfParameters( + HttpServletRequest req, String name, @Nullable String legacyName) { String[] parameters = req.getParameterValues(name); - return parameters == null ? ImmutableSet.of() : ImmutableSet.copyOf(parameters); + if (legacyName != null) { + String[] legacyParameters = req.getParameterValues(legacyName); + if (legacyParameters != null) { + if (parameters == null) { + logger.atWarning().log( + "Bad 'set of parameters' input! Used legacy name %s instead of new name %s", + legacyName, name); + parameters = legacyParameters; + } else { + logger.atSevere().log( + "Bad 'set of parameters' input! Used both legacy name %s and new name %s! " + + "Ignoring lagacy name.", + legacyName, name); + } + } + } + if (parameters == null) { + return ImmutableSet.of(); + } + if (parameters.length > 1) { + logger.atWarning().log( + "Bad 'set of parameters' input! " + + "Received multiple values instead of single comma-delimited value for parameter %s", + name); + return ImmutableSet.copyOf(parameters); + } + if (parameters[0].isEmpty()) { + return ImmutableSet.of(); + } + return ImmutableSet.copyOf(Splitter.on(',').split(parameters[0])); + } + + /** + * Returns all GET or POST parameters associated with {@code name}. + * + *

While transitioning from "param=key1¶m=key2" to "params=key1,key2" style of set inputing + * - we will be accepting all options (with a warning for "wrong" uses). + * + *

TODO(b/78226288): remove transition code (including "legacyName") once there are no more + * warnings. + */ + public static ImmutableSet extractSetOfParameters(HttpServletRequest req, String name) { + return extractSetOfParameters(req, name, null); } /** @@ -195,22 +260,16 @@ public final class RequestParameters { * @throws BadRequestException if one of the parameter values is not a valid {@link DateTime}. */ public static ImmutableSet extractSetOfDatetimeParameters( - HttpServletRequest req, String name) { - String[] stringParams = req.getParameterValues(name); - if (stringParams == null) { - return ImmutableSet.of(); + HttpServletRequest req, String name, @Nullable String legacyName) { + try { + return extractSetOfParameters(req, name, legacyName) + .stream() + .filter(not(String::isEmpty)) + .map(DateTime::parse) + .collect(toImmutableSet()); + } catch (IllegalArgumentException e) { + throw new BadRequestException("Bad ISO 8601 timestamp: " + name); } - ImmutableSet.Builder datesBuilder = new ImmutableSet.Builder<>(); - for (String stringParam : stringParams) { - try { - if (!isNullOrEmpty(stringParam)) { - datesBuilder.add(DateTime.parse(stringParam)); - } - } catch (IllegalArgumentException e) { - throw new BadRequestException("Bad ISO 8601 timestamp: " + name); - } - } - return datesBuilder.build(); } private static boolean equalsFalse(@Nullable String value) { diff --git a/java/google/registry/tools/GenerateEscrowDepositCommand.java b/java/google/registry/tools/GenerateEscrowDepositCommand.java index b65a5e0fa..244a68f35 100644 --- a/java/google/registry/tools/GenerateEscrowDepositCommand.java +++ b/java/google/registry/tools/GenerateEscrowDepositCommand.java @@ -16,6 +16,12 @@ package google.registry.tools; import static com.google.appengine.api.taskqueue.TaskOptions.Builder.withUrl; import static google.registry.model.registry.Registries.assertTldsExist; +import static google.registry.rde.RdeModule.PARAM_DIRECTORY; +import static google.registry.rde.RdeModule.PARAM_MANUAL; +import static google.registry.rde.RdeModule.PARAM_MODE; +import static google.registry.rde.RdeModule.PARAM_REVISION; +import static google.registry.rde.RdeModule.PARAM_WATERMARKS; +import static google.registry.request.RequestParameters.PARAM_TLDS; import com.beust.jcommander.Parameter; import com.beust.jcommander.ParameterException; @@ -24,12 +30,11 @@ import com.google.appengine.api.modules.ModulesService; import com.google.appengine.api.taskqueue.Queue; import com.google.appengine.api.taskqueue.TaskOptions; import google.registry.model.rde.RdeMode; -import google.registry.rde.RdeModule; import google.registry.rde.RdeStagingAction; -import google.registry.request.RequestParameters; import google.registry.tools.Command.RemoteApiCommand; import google.registry.tools.params.DateTimeParameter; import java.util.List; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.inject.Named; import org.joda.time.DateTime; @@ -101,17 +106,15 @@ final class GenerateEscrowDepositCommand implements RemoteApiCommand { TaskOptions opts = withUrl(RdeStagingAction.PATH) .header("Host", hostname) - .param(RdeModule.PARAM_MANUAL, String.valueOf(true)) - .param(RdeModule.PARAM_MODE, mode.toString()) - .param(RdeModule.PARAM_DIRECTORY, outdir); - for (String tld : tlds) { - opts = opts.param(RequestParameters.PARAM_TLD, tld); - } - for (DateTime watermark : watermarks) { - opts = opts.param(RdeModule.PARAM_WATERMARK, watermark.toString()); - } + .param(PARAM_MANUAL, String.valueOf(true)) + .param(PARAM_MODE, mode.toString()) + .param(PARAM_DIRECTORY, outdir) + .param(PARAM_TLDS, tlds.stream().collect(Collectors.joining(","))) + .param( + PARAM_WATERMARKS, + watermarks.stream().map(DateTime::toString).collect(Collectors.joining(","))); if (revision != null) { - opts = opts.param(RdeModule.PARAM_REVISION, String.valueOf(revision)); + opts = opts.param(PARAM_REVISION, String.valueOf(revision)); } queue.add(opts); } diff --git a/java/google/registry/tools/server/ListDomainsAction.java b/java/google/registry/tools/server/ListDomainsAction.java index bd14fb1b0..bf64ef60e 100644 --- a/java/google/registry/tools/server/ListDomainsAction.java +++ b/java/google/registry/tools/server/ListDomainsAction.java @@ -20,6 +20,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registry.Registries.assertTldsExist; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.POST; +import static google.registry.request.RequestParameters.PARAM_TLDS; import static java.util.Comparator.comparing; import com.google.common.collect.ImmutableSet; @@ -45,7 +46,7 @@ public final class ListDomainsAction extends ListObjectsAction { private static final int MAX_NUM_SUBQUERIES = 30; public static final String PATH = "/_dr/admin/list/domains"; - @Inject @Parameter("tlds") ImmutableSet tlds; + @Inject @Parameter(PARAM_TLDS) ImmutableSet tlds; @Inject @Parameter("limit") int limit; @Inject Clock clock; @Inject ListDomainsAction() {} diff --git a/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java b/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java index cfd7a8e73..d9479dfe0 100644 --- a/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java +++ b/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java @@ -17,6 +17,7 @@ package google.registry.tools.server; import static google.registry.mapreduce.inputs.EppResourceInputs.createEntityInput; import static google.registry.model.EppResourceUtils.isActive; import static google.registry.model.registry.Registries.assertTldsExist; +import static google.registry.request.RequestParameters.PARAM_TLDS; import static google.registry.util.PipelineUtils.createJobPath; import com.google.appengine.tools.mapreduce.Mapper; @@ -57,7 +58,7 @@ public class RefreshDnsForAllDomainsAction implements Runnable { @Inject MapreduceRunner mrRunner; @Inject Response response; - @Inject @Parameter("tlds") ImmutableSet tlds; + @Inject @Parameter(PARAM_TLDS) ImmutableSet tlds; @Inject RefreshDnsForAllDomainsAction() {} @Override diff --git a/java/google/registry/tools/server/ToolsServerModule.java b/java/google/registry/tools/server/ToolsServerModule.java index 9809d8af5..25762e533 100644 --- a/java/google/registry/tools/server/ToolsServerModule.java +++ b/java/google/registry/tools/server/ToolsServerModule.java @@ -19,12 +19,13 @@ import static google.registry.request.RequestParameters.extractBooleanParameter; import static google.registry.request.RequestParameters.extractIntParameter; import static google.registry.request.RequestParameters.extractOptionalParameter; import static google.registry.request.RequestParameters.extractRequiredParameter; +import static google.registry.request.RequestParameters.extractSetOfParameters; -import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; import dagger.Module; import dagger.Provides; import google.registry.request.Parameter; +import google.registry.request.RequestParameters; import java.util.Optional; import javax.servlet.http.HttpServletRequest; @@ -79,16 +80,15 @@ public class ToolsServerModule { } @Provides - @Parameter("tld") + @Parameter(RequestParameters.PARAM_TLD) static String provideTld(HttpServletRequest req) { - return extractRequiredParameter(req, "tld"); + return extractRequiredParameter(req, RequestParameters.PARAM_TLD); } @Provides - @Parameter("tlds") + @Parameter(RequestParameters.PARAM_TLDS) static ImmutableSet provideTlds(HttpServletRequest req) { - String tldsString = extractRequiredParameter(req, "tlds"); - return ImmutableSet.copyOf(Splitter.on(',').split(tldsString)); + return extractSetOfParameters(req, RequestParameters.PARAM_TLDS, RequestParameters.PARAM_TLD); } @Provides diff --git a/javatests/google/registry/dns/ReadDnsQueueActionTest.java b/javatests/google/registry/dns/ReadDnsQueueActionTest.java index 44a6f4b0d..ed6237879 100644 --- a/javatests/google/registry/dns/ReadDnsQueueActionTest.java +++ b/javatests/google/registry/dns/ReadDnsQueueActionTest.java @@ -17,6 +17,7 @@ package google.registry.dns; import static com.google.appengine.api.taskqueue.QueueFactory.getQueue; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Lists.transform; +import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.dns.DnsConstants.DNS_PUBLISH_PUSH_QUEUE_NAME; @@ -35,6 +36,7 @@ import com.google.appengine.api.taskqueue.QueueFactory; import com.google.appengine.api.taskqueue.TaskOptions; import com.google.appengine.api.taskqueue.TaskOptions.Method; import com.google.common.base.Joiner; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; @@ -48,10 +50,9 @@ import google.registry.testing.FakeClock; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.util.Retrier; import google.registry.util.TaskQueueUtils; -import java.util.ArrayList; -import java.util.List; import java.util.Map.Entry; import java.util.Optional; +import java.util.stream.Collectors; import java.util.stream.IntStream; import org.joda.time.DateTime; import org.joda.time.Duration; @@ -218,7 +219,8 @@ public class ReadDnsQueueActionTest { assertThat( queuedParams .stream() - .flatMap(params -> params.get("domains").stream())) + .map(params -> params.get("domains").stream().collect(onlyElement())) + .flatMap(values -> Splitter.on(',').splitToList(values).stream())) .containsExactlyElementsIn(domains); } @@ -249,19 +251,17 @@ public class ReadDnsQueueActionTest { run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); - assertThat(getQueuedParams(DNS_PUBLISH_PUSH_QUEUE_NAME)) + assertThat(getQueuedParams(DNS_PUBLISH_PUSH_QUEUE_NAME)).hasSize(1); + assertThat(getQueuedParams(DNS_PUBLISH_PUSH_QUEUE_NAME).get(0)) .containsExactly( - new ImmutableMultimap.Builder() - .put("enqueued", "3000-02-05T01:00:00.000Z") - .put("itemsCreated", "3000-02-03T00:00:00.000Z") - .put("tld", "com") - .put("dnsWriter", "comWriter") - .put("domains", "domain1.com") - .put("domains", "domain2.com") - .put("domains", "domain3.com") - .put("lockIndex", "1") - .put("numPublishLocks", "1") - .build()); + "enqueued", "3000-02-05T01:00:00.000Z", + "itemsCreated", "3000-02-03T00:00:00.000Z", + "tld", "com", + "dnsWriter", "comWriter", + "domains", "domain1.com,domain2.com,domain3.com", + "hosts", "", + "lockIndex", "1", + "numPublishLocks", "1"); } @Test @@ -411,44 +411,32 @@ public class ReadDnsQueueActionTest { new TaskMatcher().url(PublishDnsUpdatesAction.PATH).param("hosts", "ns1.domain.com")); } + private static String makeCommaSeparatedRange(int from, int to, String format) { + return IntStream.range(from, to) + .mapToObj(i -> String.format(format, i)) + .collect(Collectors.joining(",")); + } + @Test public void testSuccess_manyDomainsAndHosts() throws Exception { - List expectedTasks = new ArrayList<>(); - for (String tld : ImmutableList.of("com", "net")) { - int refreshItemsInTask = 0; - TaskMatcher task = null; + for (int i = 0; i < 150; i++) { // 0: domain; 1: host 1; 2: host 2 for (int thingType = 0; thingType < 3; thingType++) { - for (int i = 0; i < 150; i++) { + for (String tld : ImmutableList.of("com", "net")) { String domainName = String.format("domain%04d.%s", i, tld); - // If we don't have an existing task into which to dump new refreshes, create one. - if (task == null) { - task = new TaskMatcher().url(PublishDnsUpdatesAction.PATH); - expectedTasks.add(task); - refreshItemsInTask = 0; - } switch (thingType) { case 1: getQueue(DNS_PULL_QUEUE_NAME) .add(createRefreshTask("ns1." + domainName, TargetType.HOST)); - task.param("hosts", "ns1." + domainName); break; case 2: getQueue(DNS_PULL_QUEUE_NAME) .add(createRefreshTask("ns2." + domainName, TargetType.HOST)); - task.param("hosts", "ns2." + domainName); break; default: dnsQueue.addDomainRefreshTask(domainName); - task.param("domains", domainName); break; } - // If this task is now full up, wash our hands of it, so that we'll start a new one the - // next time through the loop. - refreshItemsInTask++; - if (refreshItemsInTask >= TEST_TLD_UPDATE_BATCH_SIZE) { - task = null; - } } } } @@ -456,7 +444,48 @@ public class ReadDnsQueueActionTest { run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); - assertTasksEnqueued(DNS_PUBLISH_PUSH_QUEUE_NAME, expectedTasks); + assertTasksEnqueued( + DNS_PUBLISH_PUSH_QUEUE_NAME, + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", makeCommaSeparatedRange(0, 100, "domain%04d.com")) + .param("hosts", ""), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", makeCommaSeparatedRange(100, 150, "domain%04d.com")) + .param("hosts", makeCommaSeparatedRange(0, 50, "ns1.domain%04d.com")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(50, 150, "ns1.domain%04d.com")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(0, 100, "ns2.domain%04d.com")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(100, 150, "ns2.domain%04d.com")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", makeCommaSeparatedRange(0, 100, "domain%04d.net")) + .param("hosts", ""), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", makeCommaSeparatedRange(100, 150, "domain%04d.net")) + .param("hosts", makeCommaSeparatedRange(0, 50, "ns1.domain%04d.net")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(50, 150, "ns1.domain%04d.net")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(0, 100, "ns2.domain%04d.net")), + new TaskMatcher() + .url(PublishDnsUpdatesAction.PATH) + .param("domains", "") + .param("hosts", makeCommaSeparatedRange(100, 150, "ns2.domain%04d.net"))); } @Test @@ -481,8 +510,7 @@ public class ReadDnsQueueActionTest { .param("itemsCreated", "3000-01-01T00:00:00.000Z") .param("enqueued", "3000-01-01T01:00:00.000Z") .param("domains", "hello.multilock.uk") - .param("hosts", "ns1.abc.hello.multilock.uk") - .param("hosts", "ns2.hello.multilock.uk") + .param("hosts", "ns1.abc.hello.multilock.uk,ns2.hello.multilock.uk") .header("content-type", "application/x-www-form-urlencoded"), new TaskMatcher() .url(PublishDnsUpdatesAction.PATH) @@ -491,8 +519,7 @@ public class ReadDnsQueueActionTest { .param("itemsCreated", "3000-01-01T00:00:00.000Z") .param("enqueued", "3000-01-01T01:00:00.000Z") .param("domains", "another.multilock.uk") - .param("hosts", "ns3.def.another.multilock.uk") - .param("hosts", "ns4.another.multilock.uk") + .param("hosts", "ns3.def.another.multilock.uk,ns4.another.multilock.uk") .header("content-type", "application/x-www-form-urlencoded")); } } diff --git a/javatests/google/registry/tools/GenerateEscrowDepositCommandTest.java b/javatests/google/registry/tools/GenerateEscrowDepositCommandTest.java index 13b102aec..123fca0e7 100644 --- a/javatests/google/registry/tools/GenerateEscrowDepositCommandTest.java +++ b/javatests/google/registry/tools/GenerateEscrowDepositCommandTest.java @@ -193,8 +193,8 @@ public class GenerateEscrowDepositCommandTest .url("/_dr/task/rdeStaging") .header("Host", "1.backend.test.localhost") .param("mode", "THIN") - .param("watermark", "2017-01-01T00:00:00.000Z") - .param("tld", "tld") + .param("watermarks", "2017-01-01T00:00:00.000Z") + .param("tlds", "tld") .param("directory", "test") .param("manual", "true") .param("revision", "42")); @@ -209,8 +209,8 @@ public class GenerateEscrowDepositCommandTest .url("/_dr/task/rdeStaging") .header("Host", "1.backend.test.localhost") .param("mode", "THIN") - .param("watermark", "2017-01-01T00:00:00.000Z") - .param("tld", "tld") + .param("watermarks", "2017-01-01T00:00:00.000Z") + .param("tlds", "tld") .param("directory", "test") .param("manual", "true")); } @@ -224,8 +224,8 @@ public class GenerateEscrowDepositCommandTest .url("/_dr/task/rdeStaging") .header("Host", "1.backend.test.localhost") .param("mode", "FULL") - .param("watermark", "2017-01-01T00:00:00.000Z") - .param("tld", "tld") + .param("watermarks", "2017-01-01T00:00:00.000Z") + .param("tlds", "tld") .param("directory", "test") .param("manual", "true") .param("revision", "42")); @@ -245,10 +245,8 @@ public class GenerateEscrowDepositCommandTest .url("/_dr/task/rdeStaging") .header("Host", "1.backend.test.localhost") .param("mode", "THIN") - .param("watermark", "2017-01-01T00:00:00.000Z") - .param("watermark", "2017-01-02T00:00:00.000Z") - .param("tld", "tld") - .param("tld", "anothertld") + .param("watermarks", "2017-01-01T00:00:00.000Z,2017-01-02T00:00:00.000Z") + .param("tlds", "tld,anothertld") .param("directory", "test") .param("manual", "true") .param("revision", "42"));