Merge changes Id139d43f,I906eac5e
* changes:
Revert "Allow REST endpoints and SSH commands to accept and handle unknown options"
Revert "Ignore unknown plugin options in REST endpoints that support dynamic options"
diff --git a/java/com/google/gerrit/server/DynamicOptions.java b/java/com/google/gerrit/server/DynamicOptions.java
index 90bb50b..44d3493 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 @@
/** 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 @@
* }
* </pre>
*/
- public interface BeanReceiver extends UnknownOptionHandler {
+ public interface BeanReceiver {
void setDynamicBean(String plugin, DynamicBean dynamicBean);
/**
@@ -169,17 +164,6 @@
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/CmdLineParser.java b/java/com/google/gerrit/util/cli/CmdLineParser.java
index d8b9b58..1c430fc 100644
--- a/java/com/google/gerrit/util/cli/CmdLineParser.java
+++ b/java/com/google/gerrit/util/cli/CmdLineParser.java
@@ -314,32 +314,26 @@
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)) {
- 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 @@
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 MyParser extends org.kohsuke.args4j.CmdLineParser {
- private final Object bean;
-
boolean help;
@SuppressWarnings("rawtypes")
@@ -465,22 +453,11 @@
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();
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 8d47765..0000000
--- a/java/com/google/gerrit/util/cli/UnknownOptionHandler.java
+++ /dev/null
@@ -1,58 +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 {
- /**
- * 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.
- *
- * <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);
-}
diff --git a/javatests/com/google/gerrit/acceptance/rest/BUILD b/javatests/com/google/gerrit/acceptance/rest/BUILD
index c766934..84887da 100644
--- a/javatests/com/google/gerrit/acceptance/rest/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/BUILD
@@ -6,6 +6,5 @@
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 99c727e..0000000
--- a/javatests/com/google/gerrit/acceptance/rest/UnknownOptionIT.java
+++ /dev/null
@@ -1,98 +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);
- }
-
- @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) {
- return Response.ok("OK");
- }
-
- @Override
- public boolean accept(String name, @Nullable String value) {
- return name.startsWith("--ignore");
- }
- }
-}
