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<TaskStateInfo> 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

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()));
   }
 }

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",

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

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<TaskStateInfo> 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

javatests/google/registry/flows/ResourceFlowTestCase.java

@@ -158,20 +158,15 @@ public abstract class ResourceFlowTestCase<F extends Flow, R extends EppResource
   /** Asserts the presence of a single enqueued async contact or host deletion */
   protected static <T extends EppResource> 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)));
   }
 
 

javatests/google/registry/flows/host/HostUpdateFlowTest.java

@@ -192,7 +192,7 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase<HostUpdateFlow, Hos
     // Task enqueued to change the NS record of the referencing domain via mapreduce.
     assertTasksEnqueued(
         QUEUE_ASYNC_HOST_RENAME,
-        new TaskMatcher().payload("hostKey=" + Key.create(renamedHost).getString()));
+        new TaskMatcher().param("hostKey", Key.create(renamedHost).getString()));
   }
 
   @Test

javatests/google/registry/testing/TaskQueueHelper.java

@@ -36,6 +36,7 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableMultiset;
 import com.google.common.collect.Iterables;
@@ -47,10 +48,10 @@ import google.registry.dns.DnsConstants;
 import google.registry.model.ImmutableObject;
 import java.net.URI;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedList;
-import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Objects;
@@ -65,7 +66,22 @@ public class TaskQueueHelper {
    */
   public static class TaskMatcher implements Predicate<TaskStateInfo> {
 
-    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<TaskStateInfo> taskStateInfos)
+      throws Exception {
+    ImmutableList.Builder<TaskMatcher> 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<TaskMatcher> taskMatchers)
+  public static void assertTasksEnqueued(String queueName, Collection<TaskMatcher> 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();