From 83982cfcb7cda848e709236ec67fc59b1633e8e6 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 16 Jul 2019 15:15:48 +0200 Subject: [PATCH] Revert "Allow REST endpoints and SSH commands to accept and handle unknown options" This reverts commit 35b37e19eb10f63852707556a2e9b84f137d2a91. 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 Change-Id: Id139d43f6dc627e02bbc2484c70d281f19e27d14 --- .../google/gerrit/util/cli/CmdLineParser.java | 45 +++-------- .../gerrit/util/cli/UnknownOptionHandler.java | 42 ----------- .../com/google/gerrit/acceptance/rest/BUILD | 1 - .../acceptance/rest/UnknownOptionIT.java | 75 ------------------- 4 files changed, 11 insertions(+), 152 deletions(-) delete mode 100644 java/com/google/gerrit/util/cli/UnknownOptionHandler.java delete mode 100644 javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java diff --git a/java/com/google/gerrit/util/cli/CmdLineParser.java b/java/com/google/gerrit/util/cli/CmdLineParser.java index d8b9b58236..1c430fc965 100644 --- a/java/com/google/gerrit/util/cli/CmdLineParser.java +++ b/java/com/google/gerrit/util/cli/CmdLineParser.java @@ -314,32 +314,26 @@ public class CmdLineParser { public void parseOptionMap(ListMultimap params) throws CmdLineException { logger.atFinest().log("Command-line parameters: %s", params.keySet()); - List knownArgs = Lists.newArrayListWithCapacity(2 * params.size()); + List tmp = Lists.newArrayListWithCapacity(2 * params.size()); for (String key : params.keySet()) { String name = makeOption(key); - if (isKnownOption(name)) { - if (isBooleanOption(name)) { - boolean on = false; - for (String value : params.get(key)) { - on = toBoolean(key, value); - } - if (on) { - knownArgs.add(name); - } - } else { - for (String value : params.get(key)) { - knownArgs.add(name); - knownArgs.add(value); - } + if (isBooleanOption(name)) { + boolean on = false; + for (String value : params.get(key)) { + on = toBoolean(key, value); + } + if (on) { + tmp.add(name); } } 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> it = queuedOptionsByName.entrySet().iterator(); diff --git a/java/com/google/gerrit/util/cli/UnknownOptionHandler.java b/java/com/google/gerrit/util/cli/UnknownOptionHandler.java deleted file mode 100644 index af096f9500..0000000000 --- a/java/com/google/gerrit/util/cli/UnknownOptionHandler.java +++ /dev/null @@ -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. - * - *

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. - * - *

If an unknown option is not accepted, the parsing of the command-line options fails and the - * user gets an error. - * - *

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); -} diff --git a/javatests/com/google/gerrit/acceptance/rest/BUILD b/javatests/com/google/gerrit/acceptance/rest/BUILD index c7669347df..84887dadb3 100644 --- a/javatests/com/google/gerrit/acceptance/rest/BUILD +++ b/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", ], ) diff --git a/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java b/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java deleted file mode 100644 index f15334bd8c..0000000000 --- a/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java +++ /dev/null @@ -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, UnknownOptionHandler { - @Override - public Response apply(ChangeResource resource) { - return Response.ok("OK"); - } - - @Override - public boolean accept(String name, @Nullable String value) { - return name.startsWith("--ignore"); - } - } -}