diff --git a/core/src/test/java/google/registry/cron/TldFanoutActionTest.java b/core/src/test/java/google/registry/cron/TldFanoutActionTest.java index 7d58986a3..16eef5f61 100644 --- a/core/src/test/java/google/registry/cron/TldFanoutActionTest.java +++ b/core/src/test/java/google/registry/cron/TldFanoutActionTest.java @@ -98,11 +98,7 @@ class TldFanoutActionTest { } private void assertTaskWithoutTld() { - cloudTasksHelper.assertTasksEnqueued( - QUEUE, - new TaskMatcher() - .url(ENDPOINT) - .header("content-type", "application/x-www-form-urlencoded")); + cloudTasksHelper.assertTasksEnqueued(QUEUE, new TaskMatcher().url(ENDPOINT)); } @Test diff --git a/core/src/test/java/google/registry/testing/CloudTasksHelper.java b/core/src/test/java/google/registry/testing/CloudTasksHelper.java index e2de09e5b..538432ee3 100644 --- a/core/src/test/java/google/registry/testing/CloudTasksHelper.java +++ b/core/src/test/java/google/registry/testing/CloudTasksHelper.java @@ -227,22 +227,20 @@ public class CloudTasksHelper implements Serializable { }); headers = headerBuilder.build(); ImmutableMultimap.Builder 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(); - } else if (method == HttpMethod.POST) { + paramBuilder.putAll(UriParameters.parse(uri.getQuery())); + } 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); - } - 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(); + paramBuilder.putAll( + UriParameters.parse( + task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8))); } + params = paramBuilder.build(); } public Map toMap() { diff --git a/util/src/main/java/google/registry/util/CloudTasksUtils.java b/util/src/main/java/google/registry/util/CloudTasksUtils.java index ebd97e226..19bca62ac 100644 --- a/util/src/main/java/google/registry/util/CloudTasksUtils.java +++ b/util/src/main/java/google/registry/util/CloudTasksUtils.java @@ -106,30 +106,34 @@ 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()); - Escaper escaper = UrlEscapers.urlPathSegmentEscaper(); - String encodedParams = - Joiner.on("&") - .join( - params.entries().stream() - .map( - entry -> - String.format( - "%s=%s", - escaper.escape(entry.getKey()), escaper.escape(entry.getValue()))) - .collect(toImmutableList())); - if (method == HttpMethod.GET) { - path = String.format("%s?%s", path, encodedParams); - } else if (method == HttpMethod.POST) { - 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)); + + if (!CollectionUtils.isNullOrEmpty(params)) { + Escaper escaper = UrlEscapers.urlPathSegmentEscaper(); + String encodedParams = + Joiner.on("&") + .join( + params.entries().stream() + .map( + entry -> + String.format( + "%s=%s", + escaper.escape(entry.getKey()), escaper.escape(entry.getValue()))) + .collect(toImmutableList())); + if (method == HttpMethod.GET) { + path = String.format("%s?%s", path, encodedParams); + } else { + requestBuilder + .putHeaders(HttpHeaders.CONTENT_TYPE, MediaType.FORM_DATA.toString()) + .setBody(ByteString.copyFrom(encodedParams, StandardCharsets.UTF_8)); + } } requestBuilder.setRelativeUri(path); return Task.newBuilder().setAppEngineHttpRequest(requestBuilder.build()).build(); diff --git a/util/src/main/java/google/registry/util/CollectionUtils.java b/util/src/main/java/google/registry/util/CollectionUtils.java index 2abb20a45..68bcfd6b1 100644 --- a/util/src/main/java/google/registry/util/CollectionUtils.java +++ b/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 Set nullToEmpty(@Nullable Set potentiallyNull) { return firstNonNull(potentiallyNull, ImmutableSet.of()); diff --git a/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java b/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java index cab58118b..6d74297c5 100644 --- a/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java +++ b/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() { diff --git a/util/src/test/java/google/registry/util/CollectionUtilsTest.java b/util/src/test/java/google/registry/util/CollectionUtilsTest.java index 9edb44352..f74f3ca3f 100644 --- a/util/src/test/java/google/registry/util/CollectionUtilsTest.java +++ b/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 map = ImmutableMap.of("ka", "va", "kb", "vb", "kc", "vc");