From 65aaeccfc63a75a2faa47206692c0624acf952d1 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Fri, 19 May 2017 15:30:41 -0700 Subject: [PATCH] Fix TaskQueueHelper param matching for pull queue tasks This fixes TaskQueueHelper methods and MatchableTaskInfo so that the .param() matching works for pull queues, by parsing the payload for URL-encoded parameters more liberally. As such, it updates all the places where formerly we were hacking around this by manually constructing the expected payloads and using TaskMatcher.payload() instead. It also adds a TaskQueueHelper.assertTasksEnqueued() overload that accepts an Iterable so that you can cleanly assert that a queue contains the same tasks that were returned via a previous call to getQueueInfo("queue").getTaskInfo(). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=156604901 --- .../RefreshDnsOnHostRenameActionTest.java | 4 +- javatests/google/registry/dns/BUILD | 1 + .../google/registry/dns/DnsQueueTest.java | 16 +++++-- .../registry/dns/ReadDnsQueueActionTest.java | 17 ++++--- .../registry/flows/ResourceFlowTestCase.java | 15 ++---- .../flows/host/HostUpdateFlowTest.java | 2 +- .../registry/testing/TaskQueueHelper.java | 47 +++++++++++++++++-- 7 files changed, 71 insertions(+), 31 deletions(-) diff --git a/javatests/google/registry/batch/RefreshDnsOnHostRenameActionTest.java b/javatests/google/registry/batch/RefreshDnsOnHostRenameActionTest.java index 156691532..d362d1280 100644 --- a/javatests/google/registry/batch/RefreshDnsOnHostRenameActionTest.java +++ b/javatests/google/registry/batch/RefreshDnsOnHostRenameActionTest.java @@ -165,7 +165,7 @@ public class RefreshDnsOnHostRenameActionTest assertTasksEnqueued( QUEUE_ASYNC_HOST_RENAME, new TaskMatcher() - .payload("hostKey=" + Key.create(host).getString()) - .etaDelta(standardHours(23), standardHours(25))); + .etaDelta(standardHours(23), standardHours(25)) + .param("hostKey", Key.create(host).getString())); } } diff --git a/javatests/google/registry/dns/BUILD b/javatests/google/registry/dns/BUILD index 16dccf1f4..2bf814257 100644 --- a/javatests/google/registry/dns/BUILD +++ b/javatests/google/registry/dns/BUILD @@ -24,6 +24,7 @@ java_library( "//javatests/google/registry/testing", "//third_party/java/objectify:objectify-v4_1", "@com_google_appengine_api_1_0_sdk//:testonly", + "@com_google_appengine_api_stubs", "@com_google_code_findbugs_jsr305", "@com_google_dagger", "@com_google_guava", diff --git a/javatests/google/registry/dns/DnsQueueTest.java b/javatests/google/registry/dns/DnsQueueTest.java index 74925c288..0b10f191d 100644 --- a/javatests/google/registry/dns/DnsQueueTest.java +++ b/javatests/google/registry/dns/DnsQueueTest.java @@ -52,8 +52,12 @@ public class DnsQueueTest { public void test_addHostRefreshTask_success() throws Exception { createTld("tld"); dnsQueue.addHostRefreshTask("octopus.tld"); - assertTasksEnqueued("dns-pull", - new TaskMatcher().payload("Target-Type=HOST&Target-Name=octopus.tld&tld=tld")); + assertTasksEnqueued( + "dns-pull", + new TaskMatcher() + .param("Target-Type", "HOST") + .param("Target-Name", "octopus.tld") + .param("tld", "tld")); } @Test @@ -71,8 +75,12 @@ public class DnsQueueTest { public void test_addDomainRefreshTask_success() throws Exception { createTld("tld"); dnsQueue.addDomainRefreshTask("octopus.tld"); - assertTasksEnqueued("dns-pull", - new TaskMatcher().payload("Target-Type=DOMAIN&Target-Name=octopus.tld&tld=tld")); + assertTasksEnqueued( + "dns-pull", + new TaskMatcher() + .param("Target-Type", "DOMAIN") + .param("Target-Name", "octopus.tld") + .param("tld", "tld")); } @Test diff --git a/javatests/google/registry/dns/ReadDnsQueueActionTest.java b/javatests/google/registry/dns/ReadDnsQueueActionTest.java index dc9ae77f6..250efe75d 100644 --- a/javatests/google/registry/dns/ReadDnsQueueActionTest.java +++ b/javatests/google/registry/dns/ReadDnsQueueActionTest.java @@ -20,7 +20,6 @@ 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.DnsConstants.DNS_TARGET_NAME_PARAM; import static google.registry.dns.DnsConstants.DNS_TARGET_TYPE_PARAM; -import static google.registry.request.RequestParameters.PARAM_TLD; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued; @@ -30,6 +29,7 @@ import static java.util.Arrays.asList; 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.appengine.api.taskqueue.dev.QueueStateInfo.TaskStateInfo; import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Optional; @@ -38,9 +38,9 @@ import com.google.common.net.InternetDomainName; import google.registry.dns.DnsConstants.TargetType; import google.registry.model.registry.Registry; import google.registry.model.registry.Registry.TldType; -import google.registry.request.RequestParameters; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; +import google.registry.testing.TaskQueueHelper; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.util.Retrier; import google.registry.util.TaskEnqueuer; @@ -109,7 +109,7 @@ public class ReadDnsQueueActionTest { .param(DNS_TARGET_TYPE_PARAM, type.toString()) .param(DNS_TARGET_NAME_PARAM, name); String tld = InternetDomainName.from(name).parts().reverse().get(0); - return options.param(PARAM_TLD, tld); + return options.param("tld", tld); } private void assertTldsEnqueuedInPushQueue(String... tlds) throws Exception { @@ -120,7 +120,7 @@ public class ReadDnsQueueActionTest { public TaskMatcher apply(String tld) { return new TaskMatcher() .url(PublishDnsUpdatesAction.PATH) - .param(RequestParameters.PARAM_TLD, tld) + .param("tld", tld) .header("content-type", "application/x-www-form-urlencoded"); }})); } @@ -154,13 +154,12 @@ public class ReadDnsQueueActionTest { dnsQueue.addDomainRefreshTask("domain.com"); dnsQueue.addDomainRefreshTask("domain.net"); dnsQueue.addDomainRefreshTask("domain.example"); + List preexistingTasks = + TaskQueueHelper.getQueueInfo(DNS_PULL_QUEUE_NAME).getTaskInfo(); run(true); - assertTasksEnqueued( - DNS_PULL_QUEUE_NAME, - new TaskMatcher().payload("Target-Type=DOMAIN&Target-Name=domain.com&tld=com"), - new TaskMatcher().payload("Target-Type=DOMAIN&Target-Name=domain.net&tld=net"), - new TaskMatcher().payload("Target-Type=DOMAIN&Target-Name=domain.example&tld=example")); assertTldsEnqueuedInPushQueue("com", "net", "example"); + // Check that keepTasks was honored and the pull queue tasks are still present in the queue. + assertTasksEnqueued(DNS_PULL_QUEUE_NAME, preexistingTasks); } @Test diff --git a/javatests/google/registry/flows/ResourceFlowTestCase.java b/javatests/google/registry/flows/ResourceFlowTestCase.java index ca0790bca..33b034db4 100644 --- a/javatests/google/registry/flows/ResourceFlowTestCase.java +++ b/javatests/google/registry/flows/ResourceFlowTestCase.java @@ -158,20 +158,15 @@ public abstract class ResourceFlowTestCase void assertAsyncDeletionTaskEnqueued( T resource, String requestingClientId, Trid trid, boolean isSuperuser) throws Exception { - String expectedPayload = - String.format( - "resourceKey=%s&requestingClientId=%s&clientTransactionId=%s&" - + "serverTransactionId=%s&isSuperuser=%s", - Key.create(resource).getString(), - requestingClientId, - trid.getClientTransactionId(), - trid.getServerTransactionId(), - Boolean.toString(isSuperuser)); assertTasksEnqueued( "async-delete-pull", new TaskMatcher() .etaDelta(Duration.standardSeconds(75), Duration.standardSeconds(105)) // expected: 90 - .payload(expectedPayload)); + .param("resourceKey", Key.create(resource).getString()) + .param("requestingClientId", requestingClientId) + .param("clientTransactionId", trid.getClientTransactionId()) + .param("serverTransactionId", trid.getServerTransactionId()) + .param("isSuperuser", Boolean.toString(isSuperuser))); } diff --git a/javatests/google/registry/flows/host/HostUpdateFlowTest.java b/javatests/google/registry/flows/host/HostUpdateFlowTest.java index 9f7d1a2a9..f0a6b9102 100644 --- a/javatests/google/registry/flows/host/HostUpdateFlowTest.java +++ b/javatests/google/registry/flows/host/HostUpdateFlowTest.java @@ -192,7 +192,7 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase { - MatchableTaskInfo expected = new MatchableTaskInfo(); + private final MatchableTaskInfo expected; + + public TaskMatcher() { + expected = new MatchableTaskInfo(); + } + + /** + * Constructor to create a TaskMatcher that should exactly match an existing TaskStateInfo. + * + * This is useful for checking that a pre-existing task as returned by TaskStateInfo is still + * in the queue; we can't just directly compare the lists of TaskStateInfos because they have + * no equals() override and there's no guarantee that reference equality is sufficient. + */ + private TaskMatcher(TaskStateInfo taskStateInfo) { + expected = new MatchableTaskInfo(taskStateInfo); + } public TaskMatcher taskName(String taskName) { expected.taskName = taskName; @@ -77,6 +93,10 @@ public class TaskQueueHelper { return this; } + /** + * Sets the HTTP method to match against. WARNING: due to b/38459667, pull queue tasks will + * report "POST" as their method. + */ public TaskMatcher method(String method) { expected.method = method; return this; @@ -197,6 +217,15 @@ public class TaskQueueHelper { assertTasksEnqueuedWithProperty(queueName, nameGetter, expectedTaskNames); } + public static void assertTasksEnqueued(String queueName, Iterable taskStateInfos) + throws Exception { + ImmutableList.Builder taskMatchers = ImmutableList.builder(); + for (TaskStateInfo taskStateInfo : taskStateInfos) { + taskMatchers.add(new TaskMatcher(taskStateInfo)); + } + assertTasksEnqueued(queueName, taskMatchers.build()); + } + /** * Ensures that the only tasks in the named queue are exactly those that match the expected * matchers. @@ -210,7 +239,7 @@ public class TaskQueueHelper { * Ensures that the only tasks in the named queue are exactly those that match the expected * matchers. */ - public static void assertTasksEnqueued(String queueName, List taskMatchers) + public static void assertTasksEnqueued(String queueName, Collection taskMatchers) throws Exception { QueueStateInfo qsi = getQueueInfo(queueName); assertThat(qsi.getTaskInfo()).hasSize(taskMatchers.size()); @@ -322,8 +351,16 @@ public class TaskQueueHelper { if (query != null) { inputParams.putAll(UriParameters.parse(query)); } - if (headers.containsEntry( - Ascii.toLowerCase(HttpHeaders.CONTENT_TYPE), MediaType.FORM_DATA.toString())) { + boolean hasFormDataContentType = + headers.containsEntry( + Ascii.toLowerCase(HttpHeaders.CONTENT_TYPE), MediaType.FORM_DATA.toString()); + // Try decoding the body as a parameter map if it either has the "x-www-form-urlencoded" + // content type, or if it's a POST or PULL task (in which case parameters should be encoded + // into the body automatically upon being enqueued). Note that pull queue tasks also report + // "POST" as their method (which is misleading - see b/38459667) so we just check for "POST". + if (hasFormDataContentType || "POST".equals(this.method)) { + // Note that UriParameters.parse() does not throw an IAE on a bad query string (e.g. one + // where parameters are not properly URL-encoded); it always does a best-effort parse. inputParams.putAll(UriParameters.parse(info.getBody())); } this.params = inputParams.build();