Revert "Allow REST endpoints and SSH commands to accept and handle unknown options"

This reverts commit 35b37e19eb.

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

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

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

@@ -314,32 +314,26 @@ public class CmdLineParser {
 
   public void parseOptionMap(ListMultimap<String, String> params) throws CmdLineException {
     logger.atFinest().log("Command-line parameters: %s", params.keySet());
-    List<String> knownArgs = Lists.newArrayListWithCapacity(2 * params.size());
+    List<String> tmp = Lists.newArrayListWithCapacity(2 * params.size());
     for (String key : params.keySet()) {
       String name = makeOption(key);
 
-      if (isKnownOption(name)) {
+      if (isBooleanOption(name)) {
-        if (isBooleanOption(name)) {
+        boolean on = false;
-          boolean on = false;
+        for (String value : params.get(key)) {
-          for (String value : params.get(key)) {
+          on = toBoolean(key, value);
-            on = toBoolean(key, value);
+        }
-          }
+        if (on) {
-          if (on) {
+          tmp.add(name);
-            knownArgs.add(name);
-          }
-        } else {
-          for (String value : params.get(key)) {
-            knownArgs.add(name);
-            knownArgs.add(value);
-          }
         }
       } else {
         for (String value : params.get(key)) {
-          parser.handleUnknownOption(name, value);
+          tmp.add(name);
+          tmp.add(value);
         }
       }
     }
-    parser.parseArgument(knownArgs.toArray(new String[knownArgs.size()]));
+    parser.parseArgument(tmp.toArray(new String[tmp.size()]));
   }
 
   public void parseWithPrefix(String prefix, Object bean) {
@@ -365,10 +359,6 @@ public class CmdLineParser {
     return name;
   }
 
-  private boolean isKnownOption(String name) {
-    return findHandler(name) != null;
-  }
-
   @SuppressWarnings("rawtypes")
   private OptionHandler findHandler(String name) {
     if (options == null) {
@@ -436,8 +426,6 @@ public class CmdLineParser {
   }
 
   public class MyParser extends org.kohsuke.args4j.CmdLineParser {
-    private final Object bean;
-
     boolean help;
 
     @SuppressWarnings("rawtypes")
@@ -465,22 +453,11 @@ public class CmdLineParser {
 
     MyParser(Object bean) {
       super(bean, ParserProperties.defaults().withAtSyntax(false));
-      this.bean = bean;
       parseAdditionalOptions(bean, new HashSet<>());
       addOptionsWithMetRequirements();
       ensureOptionsInitialized();
     }
 
-    public void handleUnknownOption(String name, String value) throws CmdLineException {
-      if (bean instanceof UnknownOptionHandler
-          && ((UnknownOptionHandler) bean).accept(name, Strings.emptyToNull(value))) {
-        return;
-      }
-
-      // Parse argument to trigger a CmdLineException for the unknown option.
-      parseArgument(name, value);
-    }
-
     public int addOptionsWithMetRequirements() {
       int count = 0;
       for (Iterator<Map.Entry<String, QueuedOption>> it = queuedOptionsByName.entrySet().iterator();

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

@@ -1,42 +0,0 @@
-// Copyright (C) 2019 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.util.cli;
-
-import com.google.gerrit.common.Nullable;
-
-/**
- * Classes that define command-line options by using the {@link org.kohsuke.args4j.Option}
- * annotation can implement this class to accept and handle unknown options.
- *
- * <p>If a user specifies an unknown option and this unknown option doesn't get accepted, the
- * parsing of the command-line options fails and the user gets an error (this is the default
- * behavior if classes do not implement this interface).
- */
-public interface UnknownOptionHandler {
-  /**
-   * Whether an unknown option should be accepted.
-   *
-   * <p>If an unknown option is not accepted, the parsing of the command-line options fails and the
-   * user gets an error.
-   *
-   * <p>This method can be used to ignore unknown options (without failure for the user) or to
-   * handle them.
-   *
-   * @param name the name of an unknown option that was provided by the user
-   * @param value the value of the unknown option that was provided by the user
-   * @return whether this unknown options is accepted
-   */
-  boolean accept(String name, @Nullable String value);
-}

javatests/com/google/gerrit/acceptance/rest/BUILD

@@ -6,6 +6,5 @@ acceptance_tests(
     labels = ["rest"],
     deps = [
         "//java/com/google/gerrit/server/logging",
-        "//java/com/google/gerrit/util/cli",
     ],
 )

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

@@ -1,75 +0,0 @@
-// Copyright (C) 2019 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.acceptance.rest;
-
-import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.server.change.ChangeResource.CHANGE_KIND;
-import static org.apache.http.HttpStatus.SC_BAD_REQUEST;
-import static org.apache.http.HttpStatus.SC_OK;
-
-import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.RestResponse;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.extensions.restapi.Response;
-import com.google.gerrit.extensions.restapi.RestApiModule;
-import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.server.change.ChangeResource;
-import com.google.gerrit.util.cli.UnknownOptionHandler;
-import com.google.inject.Module;
-import org.junit.Test;
-
-public class UnknownOptionIT extends AbstractDaemonTest {
-  @Override
-  public Module createModule() {
-    return new RestApiModule() {
-      @Override
-      protected void configure() {
-        get(CHANGE_KIND, "test").to(MyChangeView.class);
-      }
-    };
-  }
-
-  @Test
-  public void unknownOptionIsRejectedIfRestEndpointDoesNotHandleUnknownOptions() throws Exception {
-    RestResponse response = adminRestSession.get("/accounts/self/detail?foo-bar");
-    assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
-  }
-
-  @Test
-  public void unknownOptionIsIgnoredIfRestEndpointAcceptsIt() throws Exception {
-    String changeId = createChange().getChangeId();
-    RestResponse response = adminRestSession.get("/changes/" + changeId + "/test?ignore-foo");
-    assertThat(response.getStatusCode()).isEqualTo(SC_OK);
-  }
-
-  @Test
-  public void unknownOptionCausesFailureIfRestEndpointDoesNotAcceptIt() throws Exception {
-    String changeId = createChange().getChangeId();
-    RestResponse response = adminRestSession.get("/changes/" + changeId + "/test?foo-bar");
-    assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
-  }
-
-  private static class MyChangeView implements RestReadView<ChangeResource>, UnknownOptionHandler {
-    @Override
-    public Response<String> apply(ChangeResource resource) {
-      return Response.ok("OK");
-    }
-
-    @Override
-    public boolean accept(String name, @Nullable String value) {
-      return name.startsWith("--ignore");
-    }
-  }
-}