From 0c0d45db99cf37812ab9fdd83b1df565d2577bf8 Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Tue, 16 Jul 2019 14:58:42 +0200
Subject: [PATCH] Revert "Ignore unknown plugin options in REST endpoints that
 support dynamic options"

This reverts commit 6210be243d6334be5cafc85a20df66a3bc667cb5.

We are currently discussing alternative solutions [1] and want to start
from scratch once we decided which one we like most.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/231004/2//COMMIT_MSG

Change-Id: I906eac5eef2e7163a608227d4822984474bdf413
---
 .../google/gerrit/server/DynamicOptions.java  | 18 +--------------
 .../gerrit/util/cli/UnknownOptionHandler.java | 16 -------------
 .../acceptance/rest/UnknownOptionIT.java      | 23 -------------------
 3 files changed, 1 insertion(+), 56 deletions(-)

diff --git a/java/com/google/gerrit/server/DynamicOptions.java b/java/com/google/gerrit/server/DynamicOptions.java
index 90bb50be0f..44d34935db 100644
--- a/java/com/google/gerrit/server/DynamicOptions.java
+++ b/java/com/google/gerrit/server/DynamicOptions.java
@@ -14,12 +14,9 @@
 
 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;
@@ -33,8 +30,6 @@ 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:
    *
@@ -156,7 +151,7 @@ public class DynamicOptions {
    * }
    * </pre>
    */
-  public interface BeanReceiver extends UnknownOptionHandler {
+  public interface BeanReceiver {
     void setDynamicBean(String plugin, DynamicBean dynamicBean);
 
     /**
@@ -169,17 +164,6 @@ 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 {
diff --git a/java/com/google/gerrit/util/cli/UnknownOptionHandler.java b/java/com/google/gerrit/util/cli/UnknownOptionHandler.java
index 8d47765a8b..af096f9500 100644
--- a/java/com/google/gerrit/util/cli/UnknownOptionHandler.java
+++ b/java/com/google/gerrit/util/cli/UnknownOptionHandler.java
@@ -25,22 +25,6 @@ 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.
    *
diff --git a/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java b/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java
index 99c727e740..f15334bd8c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java
@@ -61,29 +61,6 @@ 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) {