From 63785e51496e57cbcd27b945ba7f1332b0fb1d17 Mon Sep 17 00:00:00 2001 From: guyben Date: Wed, 21 Mar 2018 13:07:55 -0700 Subject: [PATCH] Remove empty TLD parameter when fanning out without TLDs TldFanoutAction fans out a given endpoint to all TLDs (either TEST, REAL, or both). However, it is also used to delegate a single endpoint request that we want set in a specific queue (so we can control retries). We do that by setting the TLD list to "runInEmpty" rather than "forEachRealTld" or "forEachTestTld". Currently, using "runInEmpty" would still specify a TLD - but that TLD would be the empty string. This is a bug: it sets the TLD parameter to a bad value. It worked only because none of the endpoints called with "runInEmpty" were using the TLD parameter. However, this will (and does) break if either (a) the endpoint accepts an optional TLD parameter (like deleteProberData does), or (b) the given endpoint already has a TLD parameter in it (we want to run the endpoint with a single TLD, but still use the "fanout" to set the right queue). This CL fixes several things: - if runInEmpty is given, no TLD parameter is added - 'runInEmpty' is now mutually exclusive with 'forEach*Tld' and 'excludes' - we do some sanity checks and added logging - removed the buggy and unused "':tld' in path is replaced by TLD" - in the cron.xml, removed documentation for :tld and the broken :registrar Note that none of the endpoints that were used with runInEmpty fanout had the TLD parameter prior to deleteProberData ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=189954585 --- .../google/registry/cron/TldFanoutAction.java | 58 +++++---- .../env/alpha/default/WEB-INF/cron.xml | 4 +- .../env/production/default/WEB-INF/cron.xml | 4 +- .../env/sandbox/default/WEB-INF/cron.xml | 4 +- .../registry/cron/TldFanoutActionTest.java | 110 +++++++++++------- 5 files changed, 111 insertions(+), 69 deletions(-) diff --git a/java/google/registry/cron/TldFanoutAction.java b/java/google/registry/cron/TldFanoutAction.java index f1fd453df..2a601493e 100644 --- a/java/google/registry/cron/TldFanoutAction.java +++ b/java/google/registry/cron/TldFanoutAction.java @@ -16,13 +16,13 @@ package google.registry.cron; import static com.google.appengine.api.taskqueue.QueueFactory.getQueue; import static com.google.appengine.api.taskqueue.TaskOptions.Builder.withUrl; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Predicates.in; import static com.google.common.base.Predicates.not; import static com.google.common.base.Strings.nullToEmpty; 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.collect.Sets.difference; import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; import static google.registry.model.registry.Registries.getTldsOfType; import static google.registry.model.registry.Registry.TldType.REAL; @@ -42,10 +42,11 @@ import google.registry.request.ParameterMap; import google.registry.request.RequestParameters; import google.registry.request.Response; import google.registry.request.auth.Auth; +import google.registry.util.FormattingLogger; import google.registry.util.TaskEnqueuer; import java.util.Optional; import java.util.Random; -import java.util.Set; +import java.util.stream.Stream; import javax.inject.Inject; /** @@ -58,10 +59,10 @@ import javax.inject.Inject; *
  • {@code queue} (Required) Name of the App Engine push queue to which this task should be sent. *
  • {@code forEachRealTld} Launch the task in each real TLD namespace. *
  • {@code forEachTestTld} Launch the task in each test TLD namespace. - *
  • {@code runInEmpty} Launch the task in the empty namespace. + *
  • {@code runInEmpty} Launch the task once, without the TLD argument. *
  • {@code exclude} TLDs to exclude. *
  • {@code jitterSeconds} Randomly delay each task by up to this many seconds. - *
  • Any other parameters specified will be passed through as POST parameters to the called task. + *
  • Any other parameters specified will be passed through as POST parameters to the called task. * * *

    Patharg Reference

    @@ -98,9 +99,10 @@ public final class TldFanoutAction implements Runnable { EXCLUDE_PARAM, JITTER_SECONDS_PARAM); - private static final String TLD_PATHARG = ":tld"; private static final Random random = new Random(); + private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); + @Inject TaskEnqueuer taskEnqueuer; @Inject Response response; @Inject @Parameter(ENDPOINT_PARAM) String endpoint; @@ -115,28 +117,40 @@ public final class TldFanoutAction implements Runnable { @Override public void run() { - Set tlds = - difference( - Streams.concat( - Streams.stream(runInEmpty ? ImmutableSet.of("") : ImmutableSet.of()), - Streams.stream( - forEachRealTld ? getTldsOfType(REAL) : ImmutableSet.of()), - Streams.stream( - forEachTestTld ? getTldsOfType(TEST) : ImmutableSet.of())) - .collect(toImmutableSet()), - excludes); + checkArgument( + !(runInEmpty && (forEachTestTld || forEachRealTld)), + "runInEmpty and forEach*Tld are mutually exclusive"); + checkArgument( + runInEmpty || forEachTestTld || forEachRealTld, + "At least one of runInEmpty, forEachTestTld, forEachRealTld must be given"); + checkArgument( + !(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(tld -> !excludes.contains(tld)) + .collect(toImmutableSet()); Multimap flowThruParams = filterKeys(params, not(in(CONTROL_PARAMS))); Queue taskQueue = getQueue(queue); StringBuilder outputPayload = new StringBuilder( String.format("OK: Launched the following %d tasks in queue %s\n", tlds.size(), queue)); + logger.infofmt("Launching %d tasks in queue %s", tlds.size(), queue); + if (tlds.isEmpty()) { + logger.warning("No TLDs to fan-out!"); + } for (String tld : tlds) { TaskOptions taskOptions = createTaskOptions(tld, flowThruParams); TaskHandle taskHandle = taskEnqueuer.enqueue(taskQueue, taskOptions); outputPayload.append( String.format( - "- Task: %s, tld: %s, endpoint: %s\n", + "- Task: '%s', tld: '%s', endpoint: '%s'\n", taskHandle.getName(), tld, taskOptions.getUrl())); + logger.infofmt("Task: '%s', tld: '%s', endpoint: '%s'", + taskHandle.getName(), tld, taskOptions.getUrl()); } response.setContentType(PLAIN_TEXT_UTF_8); response.setPayload(outputPayload.toString()); @@ -144,12 +158,14 @@ public final class TldFanoutAction implements Runnable { private TaskOptions createTaskOptions(String tld, Multimap params) { TaskOptions options = - withUrl(endpoint.replace(TLD_PATHARG, String.valueOf(tld))) + withUrl(endpoint) .countdownMillis( - jitterSeconds.isPresent() - ? random.nextInt((int) SECONDS.toMillis(jitterSeconds.get())) - : 0); - options.param(RequestParameters.PARAM_TLD, tld); + jitterSeconds + .map(seconds -> random.nextInt((int) SECONDS.toMillis(seconds))) + .orElse(0)); + if (!tld.isEmpty()) { + options.param(RequestParameters.PARAM_TLD, tld); + } for (String param : params.keySet()) { // TaskOptions.param() does not accept null values. options.param(param, nullToEmpty((getFirst(params.get(param), null)))); 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 30686205a..bb0589566 100644 --- a/java/google/registry/env/alpha/default/WEB-INF/cron.xml +++ b/java/google/registry/env/alpha/default/WEB-INF/cron.xml @@ -5,9 +5,7 @@ /cron/fanout params: queue= endpoint= // URL Path of servlet, which may contain placeholders: - // :tld - Replaced with the TLD, e.g. foo, soy - // :registrar - Replaced with registrar clientId - runInEmpty // Run in the empty namespace + runInEmpty // Run once, with no tld parameter forEachRealTld // Run for tlds with getTldType() == TldType.REAL forEachTestTld // Run for tlds with getTldType() == TldType.TEST exclude=TLD1[&exclude=TLD2] // exclude something otherwise included 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 9352939a2..8189f7089 100644 --- a/java/google/registry/env/production/default/WEB-INF/cron.xml +++ b/java/google/registry/env/production/default/WEB-INF/cron.xml @@ -5,9 +5,7 @@ /cron/fanout params: queue= endpoint= // URL Path of servlet, which may contain placeholders: - // :tld - Replaced with the TLD, e.g. foo, soy - // :registrar - Replaced with registrar clientId - runInEmpty // Run in the empty namespace + runInEmpty // Run once, with no tld parameter forEachRealTld // Run for tlds with getTldType() == TldType.REAL forEachTestTld // Run for tlds with getTldType() == TldType.TEST exclude=TLD1[&exclude=TLD2] // exclude something otherwise included 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 62ef3a97d..b410c933d 100644 --- a/java/google/registry/env/sandbox/default/WEB-INF/cron.xml +++ b/java/google/registry/env/sandbox/default/WEB-INF/cron.xml @@ -5,9 +5,7 @@ /cron/fanout params: queue= endpoint= // URL Path of servlet, which may contain placeholders: - // :tld - Replaced with the TLD, e.g. foo, soy - // :registrar - Replaced with registrar clientId - runInEmpty // Run in the empty namespace + runInEmpty // Run once, with no tld parameter forEachRealTld // Run for tlds with getTldType() == TldType.REAL forEachTestTld // Run for tlds with getTldType() == TldType.TEST exclude=TLD1[&exclude=TLD2] // exclude something otherwise included diff --git a/javatests/google/registry/cron/TldFanoutActionTest.java b/javatests/google/registry/cron/TldFanoutActionTest.java index 31ead3030..265456cc4 100644 --- a/javatests/google/registry/cron/TldFanoutActionTest.java +++ b/javatests/google/registry/cron/TldFanoutActionTest.java @@ -14,14 +14,14 @@ package google.registry.cron; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Iterables.getLast; -import static com.google.common.collect.Lists.transform; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.persistResource; +import static google.registry.testing.JUnitBackports.assertThrows; import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued; import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued; -import static java.util.Arrays.asList; import com.google.appengine.api.taskqueue.dev.QueueStateInfo.TaskStateInfo; import com.google.appengine.tools.development.testing.LocalTaskQueueTestConfig; @@ -38,6 +38,7 @@ import google.registry.util.Retrier; import google.registry.util.TaskEnqueuer; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -101,24 +102,21 @@ public class TldFanoutActionTest { private static void assertTasks(String... tasks) throws Exception { assertTasksEnqueued( QUEUE, - transform( - asList(tasks), - (String namespace) -> + Stream.of(tasks).map( + namespace -> new TaskMatcher() .url(ENDPOINT) .header("content-type", "application/x-www-form-urlencoded") - .param("tld", namespace))); + .param("tld", namespace)) + .collect(toImmutableList())); } - @Test - public void testSuccess_pathargTld() throws Exception { - run(getParamsMap( - "forEachRealTld", "", - "endpoint", "/the/servlet/:tld")); - assertTasksEnqueued(QUEUE, - new TaskMatcher().url("/the/servlet/com"), - new TaskMatcher().url("/the/servlet/net"), - new TaskMatcher().url("/the/servlet/org")); + private static void assertTaskWithoutTld() throws Exception { + assertTasksEnqueued( + QUEUE, + new TaskMatcher() + .url(ENDPOINT) + .header("content-type", "application/x-www-form-urlencoded")); } @Test @@ -128,15 +126,14 @@ public class TldFanoutActionTest { } @Test - public void testSuccess_noTlds() throws Exception { - run(getParamsMap()); - assertNoTasksEnqueued(QUEUE); + public void testFailure_noTlds() throws Exception { + assertThrows(IllegalArgumentException.class, () -> run(getParamsMap())); } @Test public void testSuccess_runInEmpty() throws Exception { run(getParamsMap("runInEmpty", "")); - assertTasks(""); + assertTaskWithoutTld(); } @Test @@ -151,12 +148,6 @@ public class TldFanoutActionTest { assertTasks("example"); } - @Test - public void testSuccess_runInEmptyAndRunInRealTld() throws Exception { - run(getParamsMap("runInEmpty", "", "forEachRealTld", "")); - assertTasks("", "com", "net", "org"); - } - @Test public void testSuccess_forEachTestTldAndForEachRealTld() throws Exception { run(getParamsMap( @@ -165,16 +156,10 @@ public class TldFanoutActionTest { assertTasks("com", "net", "org", "example"); } - @Test - public void testSuccess_runInEmptyAndForEachTestTld() throws Exception { - run(getParamsMap("runInEmpty", "", "forEachTestTld", "")); - assertTasks("", "example"); - } - @Test public void testSuccess_runEverywhere() throws Exception { - run(getParamsMap("runInEmpty", "", "forEachTestTld", "", "forEachRealTld", "")); - assertTasks("", "com", "net", "org", "example"); + run(getParamsMap("forEachTestTld", "", "forEachRealTld", "")); + assertTasks("com", "net", "org", "example"); } @Test @@ -196,11 +181,43 @@ public class TldFanoutActionTest { @Test public void testSuccess_excludeNonexistentTlds() throws Exception { run(getParamsMap( - "runInEmpty", "", "forEachTestTld", "", "forEachRealTld", "", "exclude", "foo")); - assertTasks("", "com", "net", "org", "example"); + assertTasks("com", "net", "org", "example"); + } + + @Test + public void testFailure_runInEmptyAndTest() throws Exception { + assertThrows( + IllegalArgumentException.class, + () -> + run( + getParamsMap( + "runInEmpty", "", + "forEachTestTld", ""))); + } + + @Test + public void testFailure_runInEmptyAndReal() throws Exception { + assertThrows( + IllegalArgumentException.class, + () -> + run( + getParamsMap( + "runInEmpty", "", + "forEachRealTld", ""))); + } + + @Test + public void testFailure_runInEmptyAndExclude() throws Exception { + assertThrows( + IllegalArgumentException.class, + () -> + run( + getParamsMap( + "runInEmpty", "", + "exclude", "foo"))); } @Test @@ -212,7 +229,7 @@ public class TldFanoutActionTest { @Test public void testSuccess_returnHttpResponse() throws Exception { - run(getParamsMap("forEachRealTld", "", "endpoint", "/the/servlet/:tld")); + run(getParamsMap("forEachRealTld", "", "endpoint", "/the/servlet")); List taskList = LocalTaskQueueTestConfig.getLocalTaskQueue().getQueueStateInfo().get(QUEUE).getTaskInfo(); @@ -220,12 +237,27 @@ public class TldFanoutActionTest { assertThat(taskList).hasSize(3); String expectedResponse = String.format( "OK: Launched the following 3 tasks in queue the-queue\n" - + "- Task: %s, tld: com, endpoint: /the/servlet/com\n" - + "- Task: %s, tld: net, endpoint: /the/servlet/net\n" - + "- Task: %s, tld: org, endpoint: /the/servlet/org\n", + + "- Task: '%s', tld: 'com', endpoint: '/the/servlet'\n" + + "- Task: '%s', tld: 'net', endpoint: '/the/servlet'\n" + + "- Task: '%s', tld: 'org', endpoint: '/the/servlet'\n", taskList.get(0).getTaskName(), taskList.get(1).getTaskName(), taskList.get(2).getTaskName()); assertThat(response.getPayload()).isEqualTo(expectedResponse); } + + @Test + public void testSuccess_returnHttpResponse_runInEmpty() throws Exception { + run(getParamsMap("runInEmpty", "", "endpoint", "/the/servlet")); + + List taskList = + LocalTaskQueueTestConfig.getLocalTaskQueue().getQueueStateInfo().get(QUEUE).getTaskInfo(); + + assertThat(taskList).hasSize(1); + String expectedResponse = String.format( + "OK: Launched the following 1 tasks in queue the-queue\n" + + "- Task: '%s', tld: '', endpoint: '/the/servlet'\n", + taskList.get(0).getTaskName()); + assertThat(response.getPayload()).isEqualTo(expectedResponse); + } }