Ignore unknown plugin options in REST endpoints that support dynamic options

Some REST endpoints support dynamic options so that plugins can define
additional options for the REST endpoint. If callers start using the
plugin options, their requests will start failing as soon as the plugin
is disabled or uninstalled. This is unfortunate since plugin options are
mostly used to retrieve additional data which is optional. With this
change REST endpoints that support dynamic options now ignore unknown
plugin options, so that requests that use plugin options can still
succeed when a plugin is disabled or uninstalled.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ibf25127ce99bd858c7c02c38a07f58bd3e926908

java/com/google/gerrit/server/DynamicOptions.java

@@ -14,9 +14,12 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.server.plugins.DelegatingClassLoader;
 import com.google.gerrit.util.cli.CmdLineParser;
+import com.google.gerrit.util.cli.UnknownOptionHandler;
 import com.google.inject.Injector;
 import com.google.inject.Module;
 import com.google.inject.Provider;
@@ -30,6 +33,8 @@ import java.util.WeakHashMap;
 
 /** Helper class to define and parse options from plugins on ssh and RestAPI commands. */
 public class DynamicOptions {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   /**
    * To provide additional options, bind a DynamicBean. For example:
    *
@@ -151,7 +156,7 @@ public class DynamicOptions {
    * }
    * </pre>
    */
-  public interface BeanReceiver {
+  public interface BeanReceiver extends UnknownOptionHandler {
     void setDynamicBean(String plugin, DynamicBean dynamicBean);
 
     /**
@@ -164,6 +169,17 @@ public class DynamicOptions {
     default Class<? extends BeanReceiver> getExportedBeanReceiver() {
       return getClass();
     }
+
+    @Override
+    default boolean accept(String name, @Nullable String value) {
+      // Ignore unknown plugin options, so that callers who set a plugin option do not fail if the
+      // plugin is disabled.
+      boolean isPluginOption = UnknownOptionHandler.isPluginOption(name);
+      if (isPluginOption) {
+        logger.atFine().log("Unknown plugin option %s is ignored.", name);
+      }
+      return isPluginOption;
+    }
   }
 
   public interface BeanProvider {

java/com/google/gerrit/util/cli/UnknownOptionHandler.java

@@ -25,6 +25,22 @@ import com.google.gerrit.common.Nullable;
  * behavior if classes do not implement this interface).
  */
 public interface UnknownOptionHandler {
+  /**
+   * Checks whether the given option name matches the naming pattern of options that are defined by
+   * plugins that were defined by registering a {@link
+   * com.google.gerrit.server.DynamicOptions.DynamicBean}.
+   *
+   * @param optionName name of the option
+   * @return {@code true} if it's a plugin option, otherwise {@code false}
+   */
+  public static boolean isPluginOption(String optionName) {
+    // Options from plugins have the following name pattern: '--<plugin-name>--<option-name>'
+    if (optionName.startsWith("--")) {
+      optionName = optionName.substring(2);
+    }
+    return optionName.contains("--");
+  }
+
   /**
    * Whether an unknown option should be accepted.
    *

javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java

@@ -61,6 +61,29 @@ public class UnknownOptionIT extends AbstractDaemonTest {
     assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
   }
 
+  @Test
+  public void unknownPluginOptionIsIgnoredIfRestEndpointSupportsDynamicOptions() throws Exception {
+    String changeId = createChange().getChangeId();
+    RestResponse response = adminRestSession.get("/changes/" + changeId + "?foo--bar");
+    assertThat(response.getStatusCode()).isEqualTo(SC_OK);
+  }
+
+  @Test
+  public void unknownNonPluginOptionCausesFailureIfRestEndpointSupportsDynamicOptions()
+      throws Exception {
+    String changeId = createChange().getChangeId();
+    RestResponse response = adminRestSession.get("/changes/" + changeId + "?foo-bar");
+    assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
+  }
+
+  @Test
+  public void unknownPluginOptionCausesFailureIfRestEndpointDoesNotSupportDynamicOptions()
+      throws Exception {
+    String changeId = createChange().getChangeId();
+    RestResponse response = adminRestSession.get("/changes/" + changeId + "/comments?foo--bar");
+    assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
+  }
+
   private static class MyChangeView implements RestReadView<ChangeResource>, UnknownOptionHandler {
     @Override
     public Response<String> apply(ChangeResource resource) {