Address code review comments

google · Sep 20, 2024 · 9d0ef27 · 9d0ef27
1 parent 6e33abb
commit 9d0ef27
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 8 deletions.
diff --git a/core/src/main/java/google/registry/batch/CloudTasksUtils.java b/core/src/main/java/google/registry/batch/CloudTasksUtils.java
@@ -28,6 +28,7 @@
 import com.google.cloud.tasks.v2.Task;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Streams;
 import com.google.common.escape.Escaper;
@@ -219,6 +220,13 @@ public Task createTask(
         "Action class %s is not annotated with @Action",
         actionClazz.getSimpleName());
     String path = action.path();
+    ImmutableSet<Method> allowedMethods = ImmutableSet.copyOf(action.method());
+    checkArgument(
+        allowedMethods.contains(method),
+        "Method %s is not allowed for action %s. Allowed methods are %s",
+        method,
+        actionClazz.getSimpleName(),
+        allowedMethods);
     Service service =
         RegistryEnvironment.isOnJetty() ? Action.ServiceGetter.get(action) : action.service();
     return createTask(path, method, service, params);

diff --git a/core/src/main/java/google/registry/tools/ServiceConnection.java b/core/src/main/java/google/registry/tools/ServiceConnection.java
@@ -14,6 +14,7 @@
 
 package google.registry.tools;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.base.Verify.verify;
@@ -59,17 +60,19 @@ public class ServiceConnection {
   /** Pattern to heuristically extract title tag contents in HTML responses. */
   protected static final Pattern HTML_TITLE_TAG_PATTERN = Pattern.compile("<title>(.*?)</title>");
 
-  protected final Service service;
-  protected final boolean useCanary;
-  protected final HttpRequestFactory requestFactory;
+  private final Service service;
+  private final boolean useCanary;
+  private final HttpRequestFactory requestFactory;
 
   @Inject
   ServiceConnection(@Config("useGke") boolean useGke, HttpRequestFactory requestFactory) {
     this(useGke ? GkeService.BACKEND : GaeService.TOOLS, requestFactory, false);
   }
 
-  protected ServiceConnection(
-      Service service, HttpRequestFactory requestFactory, boolean useCanary) {
+  private ServiceConnection(Service service, HttpRequestFactory requestFactory, boolean useCanary) {
+    // Currently, only GAE supports connecting to canary.
+    // TODO (jianglai): decide how to implement canary for GKE.
+    checkArgument(useCanary == false || service instanceof GaeService, "Canary is only for GAE");
     this.service = service;
     this.requestFactory = requestFactory;
     this.useCanary = useCanary;
@@ -132,8 +135,7 @@ url, new ByteArrayContent(contentType.toString(), payload))
   @VisibleForTesting
   URL getServer() {
     URL url = service.getServiceUrl();
-    // Currently only GAE supports connecting to canary.
-    if (service instanceof GaeService && useCanary) {
+    if (useCanary) {
       verify(!isNullOrEmpty(url.getHost()), "Null host in url");
       url =
           makeUrl(

diff --git a/core/src/test/java/google/registry/batch/CloudTasksUtilsTest.java b/core/src/test/java/google/registry/batch/CloudTasksUtilsTest.java
@@ -158,6 +158,13 @@ void testFailure_createTasks_notAnAction() {
         () -> cloudTasksUtils.createTask(NotAnAction.class, GET, params));
   }
 
+  @Test
+  void testFailure_methodNotAllowed() {
+    assertThrows(
+        IllegalArgumentException.class,
+        () -> cloudTasksUtils.createTask(OtherAction.class, POST, params));
+  }
+
   @Test
   void testSuccess_createPostTasks() {
     Task task = cloudTasksUtils.createTask(TheAction.class, POST, params);
@@ -276,7 +283,7 @@ void testSuccess_createTasks_withZeroDelay() {
       service = GaeService.BACKEND,
       gkeService = GkeService.BACKEND,
       path = "/the/path",
-      method = {GET},
+      method = {GET, POST},
       auth = Auth.AUTH_ADMIN)
   private static class TheAction implements Runnable {