Merge "Avoid passing @Singleton to args4j"

java/com/google/gerrit/httpd/restapi/ParameterParser.java

@@ -43,6 +43,7 @@ import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
 import com.google.gson.JsonPrimitive;
 import com.google.inject.Inject;
+import com.google.inject.Singleton;
 import java.io.IOException;
 import java.io.StringWriter;
 import java.util.HashSet;
@@ -153,6 +154,12 @@ public class ParameterParser {
     this.parserFactory = pf;
   }
 
+  /**
+   * Parses query parameters ({@code in}) into annotated option fields of {@code param}.
+   *
+   * @return true if parsing was successful. Requesting help is considered failure and returns
+   *     false.
+   */
   <T> boolean parse(
       T param,
       DynamicOptions pluginOptions,
@@ -160,6 +167,10 @@ public class ParameterParser {
       HttpServletRequest req,
       HttpServletResponse res)
       throws IOException {
+    if (param.getClass().getAnnotation(Singleton.class) != null) {
+      // Command-line parsing mutates the object, so we can't have options on @Singleton.
+      return true;
+    }
     CmdLineParser clp = parserFactory.create(param);
     pluginOptions.setBean(param);
     pluginOptions.startLifecycleListeners();

javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.rest.binding;
 
 import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
 
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -48,6 +49,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
 import org.junit.Test;
+import org.kohsuke.args4j.Option;
 
 /**
  * Tests for checking plugin-provided REST API bindings directly under {@code /}.
@@ -192,8 +194,15 @@ public class PluginProvidedRootRestApiBindingsIT extends AbstractDaemonTest {
 
   @Singleton
   static class TestGet implements RestReadView<TestPluginResource> {
+
+    @Option(name = "--crash")
+    String crash;
+
     @Override
     public Response<String> apply(TestPluginResource resource) throws Exception {
+      if (!Strings.nullToEmpty(crash).isEmpty()) {
+        throw new IllegalStateException();
+      }
       return Response.ok("test");
     }
   }
@@ -204,4 +213,13 @@ public class PluginProvidedRootRestApiBindingsIT extends AbstractDaemonTest {
       RestApiCallHelper.execute(adminRestSession, TEST_CALLS.asList());
     }
   }
+
+  @Test
+  public void testOptionOnSingletonIsIgnored() throws Exception {
+    try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null)) {
+      RestApiCallHelper.execute(
+          adminRestSession,
+          RestCall.get("/plugins/" + PLUGIN_NAME + "/test-collection/1/detail?crash=xyz"));
+    }
+  }
 }