Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deguice ActionFilters #26691

Merged
merged 1 commit into from
Sep 20, 2017
Merged

Deguice ActionFilters #26691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions core/src/main/java/org/elasticsearch/action/ActionModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@
import org.elasticsearch.usage.UsageService;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -341,7 +342,7 @@ public class ActionModule extends AbstractModule {
private final SettingsFilter settingsFilter;
private final List<ActionPlugin> actionPlugins;
private final Map<String, ActionHandler<?, ?>> actions;
private final List<Class<? extends ActionFilter>> actionFilters;
private final ActionFilters actionFilters;
private final AutoCreateIndex autoCreateIndex;
private final DestructiveOperations destructiveOperations;
private final RestController restController;
Expand Down Expand Up @@ -503,8 +504,9 @@ public <Request extends ActionRequest, Response extends ActionResponse> void reg
return unmodifiableMap(actions.getRegistry());
}

private List<Class<? extends ActionFilter>> setupActionFilters(List<ActionPlugin> actionPlugins) {
return unmodifiableList(actionPlugins.stream().flatMap(p -> p.getActionFilters().stream()).collect(Collectors.toList()));
private ActionFilters setupActionFilters(List<ActionPlugin> actionPlugins) {
return new ActionFilters(
Collections.unmodifiableSet(actionPlugins.stream().flatMap(p -> p.getActionFilters().stream()).collect(Collectors.toSet())));
}

public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster) {
Expand Down Expand Up @@ -649,11 +651,7 @@ public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster) {

@Override
protected void configure() {
Multibinder<ActionFilter> actionFilterMultibinder = Multibinder.newSetBinder(binder(), ActionFilter.class);
for (Class<? extends ActionFilter> actionFilter : actionFilters) {
actionFilterMultibinder.addBinding().to(actionFilter);
}
bind(ActionFilters.class).asEagerSingleton();
bind(ActionFilters.class).toInstance(actionFilters);
bind(DestructiveOperations.class).toInstance(destructiveOperations);

if (false == transportClient) {
Expand All @@ -676,6 +674,10 @@ protected void configure() {
}
}

public ActionFilters getActionFilters() {
return actionFilters;
}

public RestController getRestController() {
return restController;
}
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/java/org/elasticsearch/action/support/ActionFilters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

package org.elasticsearch.action.support;

import org.elasticsearch.common.inject.Inject;

import java.util.Arrays;
import java.util.Comparator;
import java.util.Set;
Expand All @@ -32,7 +30,6 @@ public class ActionFilters {

private final ActionFilter[] filters;

@Inject
public ActionFilters(Set<ActionFilter> actionFilters) {
this.filters = actionFilters.toArray(new ActionFilter[actionFilters.size()]);
Arrays.sort(filters, new Comparator<ActionFilter>() {
Expand Down
10 changes: 6 additions & 4 deletions core/src/main/java/org/elasticsearch/node/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,10 +361,6 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
CircuitBreakerService circuitBreakerService = createCircuitBreakerService(settingsModule.getSettings(),
settingsModule.getClusterSettings());
resourcesToClose.add(circuitBreakerService);
ActionModule actionModule = new ActionModule(false, settings, clusterModule.getIndexNameExpressionResolver(),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService);
modules.add(actionModule);
modules.add(new GatewayModule());


Expand Down Expand Up @@ -400,6 +396,12 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
scriptModule.getScriptService(), xContentRegistry, environment, nodeEnvironment,
namedWriteableRegistry).stream())
.collect(Collectors.toList());

ActionModule actionModule = new ActionModule(false, settings, clusterModule.getIndexNameExpressionResolver(),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService);
modules.add(actionModule);

final RestController restController = actionModule.getRestController();
final NetworkModule networkModule = new NetworkModule(settings, false, pluginsService.filterPlugins(NetworkPlugin.class),
threadPool, bigArrays, circuitBreakerService, namedWriteableRegistry, xContentRegistry, networkService, restController);
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/org/elasticsearch/plugins/ActionPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsFilter;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestHandler;

Expand Down Expand Up @@ -66,7 +65,7 @@ public interface ActionPlugin {
/**
* Action filters added by this plugin.
*/
default List<Class<? extends ActionFilter>> getActionFilters() {
default List<ActionFilter> getActionFilters() {
return Collections.emptyList();
}
/**
Expand Down
17 changes: 9 additions & 8 deletions core/src/test/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsAction;
import org.elasticsearch.action.admin.indices.stats.IndicesStatsAction;
import org.elasticsearch.action.support.ActionFilter;
Expand All @@ -35,7 +34,6 @@
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.env.NodeEnvironment;
Expand All @@ -48,9 +46,6 @@
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.InternalTestCluster;
import org.elasticsearch.test.transport.MockTransportService;
import org.elasticsearch.transport.ConnectionProfile;
import org.elasticsearch.transport.Transport;
import org.elasticsearch.transport.TransportException;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.transport.TransportRequestOptions;
import org.elasticsearch.transport.TransportService;
Expand Down Expand Up @@ -80,16 +75,22 @@
public class ClusterInfoServiceIT extends ESIntegTestCase {

public static class TestPlugin extends Plugin implements ActionPlugin {

private final BlockingActionFilter blockingActionFilter;

public TestPlugin(Settings settings) {
blockingActionFilter = new BlockingActionFilter(settings);
}

@Override
public List<Class<? extends ActionFilter>> getActionFilters() {
return singletonList(BlockingActionFilter.class);
public List<ActionFilter> getActionFilters() {
return singletonList(blockingActionFilter);
}
}

public static class BlockingActionFilter extends org.elasticsearch.action.support.ActionFilter.Simple {
private Set<String> blockedActions = emptySet();

@Inject
public BlockingActionFilter(Settings settings) {
super(settings);
}
Expand Down
28 changes: 24 additions & 4 deletions ...reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.index.reindex;

import org.apache.lucene.util.SetOnce;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.ActionListener;
Expand All @@ -29,23 +30,31 @@
import org.elasticsearch.action.support.ActionFilter;
import org.elasticsearch.action.support.ActionFilterChain;
import org.elasticsearch.action.support.WriteRequest.RefreshPolicy;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.network.NetworkModule;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.transport.TransportAddress;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.test.ESSingleNodeTestCase;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.Netty4Plugin;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.junit.Before;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -129,9 +138,21 @@ public void testReindexWithBadAuthentication() throws Exception {
* Plugin that demands authentication.
*/
public static class TestPlugin extends Plugin implements ActionPlugin {

private final SetOnce<ReindexFromRemoteWithAuthTests.TestFilter> testFilter = new SetOnce<>();

@Override
public Collection<Object> createComponents(Client client, ClusterService clusterService, ThreadPool threadPool,
ResourceWatcherService resourceWatcherService, ScriptService scriptService,
NamedXContentRegistry xContentRegistry, Environment environment,
NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
testFilter.set(new ReindexFromRemoteWithAuthTests.TestFilter(threadPool));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of createComponents is to create services. This creates an unenforcible ordering dependency between creating these services and calling getActionFilters(). getActionFilters should take whatever is necessary to creation action filters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getActionFilters is meant to be used by plugins, for which we don't know about the parameters that they require. We have the same situation for a lot of other methods (e.g. getRestHandlers). In case of incorrect ordering, our tests will blow up.

return Collections.emptyList();
}

@Override
public List<Class<? extends ActionFilter>> getActionFilters() {
return singletonList(ReindexFromRemoteWithAuthTests.TestFilter.class);
public List<ActionFilter> getActionFilters() {
return singletonList(testFilter.get());
}

@Override
Expand All @@ -153,7 +174,6 @@ public static class TestFilter implements ActionFilter {
private static final String EXAMPLE_HEADER = "Example-Header";
private final ThreadContext context;

@Inject
public TestFilter(ThreadPool threadPool) {
context = threadPool.getThreadContext();
}
Expand Down
31 changes: 17 additions & 14 deletions qa/smoke-test-http/src/test/java/org/elasticsearch/http/ContextAndHeaderTransportIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.http;

import org.apache.http.message.BasicHeader;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.admin.indices.refresh.RefreshRequest;
Expand All @@ -31,12 +32,14 @@
import org.elasticsearch.action.termvectors.MultiTermVectorsRequest;
import org.elasticsearch.client.Client;
import org.elasticsearch.client.Response;
import org.elasticsearch.common.inject.AbstractModule;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.inject.Module;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.network.NetworkModule;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.index.query.BoolQueryBuilder;
import org.elasticsearch.index.query.GeoShapeQueryBuilder;
import org.elasticsearch.index.query.MoreLikeThisQueryBuilder;
Expand All @@ -46,8 +49,10 @@
import org.elasticsearch.indices.TermsLookup;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.junit.After;
import org.junit.Before;

Expand Down Expand Up @@ -282,21 +287,20 @@ private Client transportClient() {

public static class ActionLoggingPlugin extends Plugin implements ActionPlugin {

@Override
public Collection<Module> createGuiceModules() {
return Collections.<Module>singletonList(new ActionLoggingModule());
}
private final SetOnce<LoggingFilter> loggingFilter = new SetOnce<>();

@Override
public List<Class<? extends ActionFilter>> getActionFilters() {
return singletonList(LoggingFilter.class);
public Collection<Object> createComponents(Client client, ClusterService clusterService, ThreadPool threadPool,
ResourceWatcherService resourceWatcherService, ScriptService scriptService,
NamedXContentRegistry xContentRegistry, Environment environment,
NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
loggingFilter.set(new LoggingFilter(clusterService.getSettings(), threadPool));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a sign that getActionFilters maybe should take ThreadPool as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is meant to be extended by plugins, it's unclear what parameters would be useful to them. I think it's best to keep the interface lean.

return Collections.emptyList();
}
}

public static class ActionLoggingModule extends AbstractModule {
@Override
protected void configure() {
bind(LoggingFilter.class).asEagerSingleton();
public List<ActionFilter> getActionFilters() {
return singletonList(loggingFilter.get());
}

}
Expand All @@ -305,7 +309,6 @@ public static class LoggingFilter extends ActionFilter.Simple {

private final ThreadPool threadPool;

@Inject
public LoggingFilter(Settings settings, ThreadPool pool) {
super(settings);
this.threadPool = pool;
Expand Down