Revert "Ignore unknown plugin options in REST endpoints that support dynamic options"
This reverts commit 6210be243d.
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
This commit is contained in:
@@ -14,12 +14,9 @@
|
|||||||
|
|
||||||
package com.google.gerrit.server;
|
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.extensions.registration.DynamicMap;
|
||||||
import com.google.gerrit.server.plugins.DelegatingClassLoader;
|
import com.google.gerrit.server.plugins.DelegatingClassLoader;
|
||||||
import com.google.gerrit.util.cli.CmdLineParser;
|
import com.google.gerrit.util.cli.CmdLineParser;
|
||||||
import com.google.gerrit.util.cli.UnknownOptionHandler;
|
|
||||||
import com.google.inject.Injector;
|
import com.google.inject.Injector;
|
||||||
import com.google.inject.Module;
|
import com.google.inject.Module;
|
||||||
import com.google.inject.Provider;
|
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. */
|
/** Helper class to define and parse options from plugins on ssh and RestAPI commands. */
|
||||||
public class DynamicOptions {
|
public class DynamicOptions {
|
||||||
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* To provide additional options, bind a DynamicBean. For example:
|
* To provide additional options, bind a DynamicBean. For example:
|
||||||
*
|
*
|
||||||
@@ -156,7 +151,7 @@ public class DynamicOptions {
|
|||||||
* }
|
* }
|
||||||
* </pre>
|
* </pre>
|
||||||
*/
|
*/
|
||||||
public interface BeanReceiver extends UnknownOptionHandler {
|
public interface BeanReceiver {
|
||||||
void setDynamicBean(String plugin, DynamicBean dynamicBean);
|
void setDynamicBean(String plugin, DynamicBean dynamicBean);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -169,17 +164,6 @@ public class DynamicOptions {
|
|||||||
default Class<? extends BeanReceiver> getExportedBeanReceiver() {
|
default Class<? extends BeanReceiver> getExportedBeanReceiver() {
|
||||||
return getClass();
|
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 {
|
public interface BeanProvider {
|
||||||
|
|||||||
@@ -25,22 +25,6 @@ import com.google.gerrit.common.Nullable;
|
|||||||
* behavior if classes do not implement this interface).
|
* behavior if classes do not implement this interface).
|
||||||
*/
|
*/
|
||||||
public interface UnknownOptionHandler {
|
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.
|
* Whether an unknown option should be accepted.
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -61,29 +61,6 @@ public class UnknownOptionIT extends AbstractDaemonTest {
|
|||||||
assertThat(response.getStatusCode()).isEqualTo(SC_BAD_REQUEST);
|
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 {
|
private static class MyChangeView implements RestReadView<ChangeResource>, UnknownOptionHandler {
|
||||||
@Override
|
@Override
|
||||||
public Response<String> apply(ChangeResource resource) {
|
public Response<String> apply(ChangeResource resource) {
|
||||||
|
|||||||
Reference in New Issue
Block a user