Add support for empty or null params for createTask() (#1448)

* Add support for null or empty params

* Add Null or empty check in CollectionUtils

* Remove content type header for empty params in POST request

core/src/test/java/google/registry/cron/TldFanoutActionTest.java

@@ -98,11 +98,7 @@ class TldFanoutActionTest {
   }
 
   private void assertTaskWithoutTld() {
-    cloudTasksHelper.assertTasksEnqueued(
+    cloudTasksHelper.assertTasksEnqueued(QUEUE, new TaskMatcher().url(ENDPOINT));
-        QUEUE,
-        new TaskMatcher()
-            .url(ENDPOINT)
-            .header("content-type", "application/x-www-form-urlencoded"));
   }
 
   @Test

core/src/test/java/google/registry/testing/CloudTasksHelper.java

@@ -227,23 +227,21 @@ public class CloudTasksHelper implements Serializable {
               });
       headers = headerBuilder.build();
       ImmutableMultimap.Builder<String, String> paramBuilder = new ImmutableMultimap.Builder<>();
-      String query = null;
+      // 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.
       if (method == HttpMethod.GET) {
-        query = uri.getQuery();
+        paramBuilder.putAll(UriParameters.parse(uri.getQuery()));
-      } else if (method == HttpMethod.POST) {
+      } else if (method == HttpMethod.POST && !task.getAppEngineHttpRequest().getBody().isEmpty()) {
         assertThat(
                 headers.containsEntry(
                     Ascii.toLowerCase(HttpHeaders.CONTENT_TYPE), MediaType.FORM_DATA.toString()))
             .isTrue();
-        query = task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8);
+        paramBuilder.putAll(
+            UriParameters.parse(
+                task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)));
       }
-      if (query != null) {
-        // 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.
-        paramBuilder.putAll(UriParameters.parse(query));
       params = paramBuilder.build();
     }
-    }
 
     public Map<String, Object> toMap() {
       Map<String, Object> builder = new HashMap<>();

util/src/main/java/google/registry/util/CloudTasksUtils.java

@@ -106,10 +106,16 @@ public class CloudTasksUtils implements Serializable {
     checkArgument(
         path != null && !path.isEmpty() && path.charAt(0) == '/',
         "The path must start with a '/'.");
+    checkArgument(
+        method.equals(HttpMethod.GET) || method.equals(HttpMethod.POST),
+        "HTTP method %s is used. Only GET and POST are allowed.",
+        method);
     AppEngineHttpRequest.Builder requestBuilder =
         AppEngineHttpRequest.newBuilder()
             .setHttpMethod(method)
             .setAppEngineRouting(AppEngineRouting.newBuilder().setService(service).build());
+
+    if (!CollectionUtils.isNullOrEmpty(params)) {
       Escaper escaper = UrlEscapers.urlPathSegmentEscaper();
       String encodedParams =
           Joiner.on("&")
@@ -123,13 +129,11 @@ public class CloudTasksUtils implements Serializable {
                       .collect(toImmutableList()));
       if (method == HttpMethod.GET) {
         path = String.format("%s?%s", path, encodedParams);
-    } else if (method == HttpMethod.POST) {
+      } else {
         requestBuilder
             .putHeaders(HttpHeaders.CONTENT_TYPE, MediaType.FORM_DATA.toString())
             .setBody(ByteString.copyFrom(encodedParams, StandardCharsets.UTF_8));
-    } else {
+      }
-      throw new IllegalArgumentException(
-          String.format("HTTP method %s is used. Only GET and POST are allowed.", method));
     }
     requestBuilder.setRelativeUri(path);
     return Task.newBuilder().setAppEngineHttpRequest(requestBuilder.build()).build();

util/src/main/java/google/registry/util/CollectionUtils.java

@@ -49,6 +49,11 @@ public class CollectionUtils {
     return potentiallyNull == null || potentiallyNull.isEmpty();
   }
 
+  /** Checks if a Multimap is null or empty. */
+  public static boolean isNullOrEmpty(@Nullable Multimap<?, ?> potentiallyNull) {
+    return potentiallyNull == null || potentiallyNull.isEmpty();
+  }
+
   /** Turns a null set into an empty set. JAXB leaves lots of null sets lying around. */
   public static <T> Set<T> nullToEmpty(@Nullable Set<T> potentiallyNull) {
     return firstNonNull(potentiallyNull, ImmutableSet.of());

util/src/test/java/google/registry/util/CloudTasksUtilsTest.java

@@ -25,6 +25,7 @@ import static org.mockito.Mockito.when;
 import com.google.cloud.tasks.v2.HttpMethod;
 import com.google.cloud.tasks.v2.Task;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.LinkedListMultimap;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeSleeper;
@@ -80,6 +81,48 @@ public class CloudTasksUtilsTest {
     assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
   }
 
+  @Test
+  void testSuccess_createGetTasks_withNullParams() {
+    Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", null);
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createPostTasks_withNullParams() {
+    Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", null);
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)).isEmpty();
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createGetTasks_withEmptyParams() {
+    Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", ImmutableMultimap.of());
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createPostTasks_withEmptyParams() {
+    Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", ImmutableMultimap.of());
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)).isEmpty();
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
   @SuppressWarnings("ProtoTimestampGetSecondsGetNano")
   @Test
   void testSuccess_createGetTasks_withJitterSeconds() {

util/src/test/java/google/registry/util/CollectionUtilsTest.java

@@ -22,6 +22,8 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.Multimap;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
 
@@ -43,6 +45,12 @@ class CollectionUtilsTest {
     assertThat(convertedMap).isEmpty();
   }
 
+  @Test
+  void testNullOrEmptyMultimap() {
+    assertThat(CollectionUtils.isNullOrEmpty((Multimap<?, ?>) null)).isTrue();
+    assertThat(CollectionUtils.isNullOrEmpty(ImmutableMultimap.of())).isTrue();
+  }
+
   @Test
   void testPartitionMap() {
     Map<String, String> map = ImmutableMap.of("ka", "va", "kb", "vb", "kc", "vc");