From abb7d7841f0138024477d53f47fc175b5bdf8b08 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 11 Jan 2017 10:28:14 -0500 Subject: [PATCH 1/2] Remove SearchRequestParsers (#22538) It is empty now that we've moved all the parsing into `namedObject`. --- .../java/org/elasticsearch/node/Node.java | 5 +-- .../org/elasticsearch/plugins/Plugin.java | 4 +-- .../action/search/RestMultiSearchAction.java | 8 ++--- .../rest/action/search/RestSearchAction.java | 13 +++---- .../elasticsearch/search/SearchModule.java | 6 ---- .../search/SearchRequestParsers.java | 29 --------------- .../action/ExplainRequestTests.java | 6 +--- .../ShardValidateQueryRequestTests.java | 6 +--- .../search/MultiSearchRequestTests.java | 3 +- .../search/AbstractSearchTestCase.java | 2 -- .../mustache/RestSearchTemplateAction.java | 8 ++--- .../TransportSearchTemplateAction.java | 5 +-- .../AbstractBaseReindexRestHandler.java | 5 +-- .../AbstractBulkByQueryRestHandler.java | 8 ++--- .../reindex/RestDeleteByQueryAction.java | 5 ++- .../index/reindex/RestReindexAction.java | 35 ++++--------------- .../reindex/RestUpdateByQueryAction.java | 5 ++- .../index/reindex/RestReindexActionTests.java | 9 ++--- .../file/FileBasedDiscoveryPlugin.java | 2 -- 19 files changed, 31 insertions(+), 133 deletions(-) delete mode 100644 core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index 7033f2a9f3d..ebd12142917 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/core/src/main/java/org/elasticsearch/node/Node.java @@ -119,7 +119,6 @@ import org.elasticsearch.repositories.RepositoriesModule; import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.SearchModule; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.SearchService; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.snapshots.SnapshotShardsService; @@ -379,8 +378,7 @@ public class Node implements Closeable { Collection pluginComponents = pluginsService.filterPlugins(Plugin.class).stream() .flatMap(p -> p.createComponents(client, clusterService, threadPool, resourceWatcherService, - scriptModule.getScriptService(), searchModule.getSearchRequestParsers(), - xContentRegistry).stream()) + scriptModule.getScriptService(), xContentRegistry).stream()) .collect(Collectors.toList()); Collection>> customMetaDataUpgraders = pluginsService.filterPlugins(Plugin.class).stream() @@ -410,7 +408,6 @@ public class Node implements Closeable { final DiscoveryModule discoveryModule = new DiscoveryModule(this.settings, threadPool, transportService, namedWriteableRegistry, networkService, clusterService, pluginsService.filterPlugins(DiscoveryPlugin.class)); modules.add(b -> { - b.bind(SearchRequestParsers.class).toInstance(searchModule.getSearchRequestParsers()); b.bind(NamedXContentRegistry.class).toInstance(xContentRegistry); b.bind(PluginsService.class).toInstance(pluginsService); b.bind(Client.class).toInstance(client); diff --git a/core/src/main/java/org/elasticsearch/plugins/Plugin.java b/core/src/main/java/org/elasticsearch/plugins/Plugin.java index e7d97b0724e..87c5ef9a8c6 100644 --- a/core/src/main/java/org/elasticsearch/plugins/Plugin.java +++ b/core/src/main/java/org/elasticsearch/plugins/Plugin.java @@ -42,7 +42,6 @@ import org.elasticsearch.repositories.RepositoriesModule; import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.SearchModule; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.threadpool.ExecutorBuilder; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.watcher.ResourceWatcherService; @@ -103,11 +102,10 @@ public abstract class Plugin implements Closeable { * @param threadPool A service to allow retrieving an executor to run an async action * @param resourceWatcherService A service to watch for changes to node local files * @param scriptService A service to allow running scripts on the local node - * @param searchRequestParsers Parsers for search requests which may be used to templatize search requests */ public Collection createComponents(Client client, ClusterService clusterService, ThreadPool threadPool, ResourceWatcherService resourceWatcherService, ScriptService scriptService, - SearchRequestParsers searchRequestParsers, NamedXContentRegistry xContentRegistry) { + NamedXContentRegistry xContentRegistry) { return Collections.emptyList(); } diff --git a/core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java b/core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java index 073cc6bee8d..d265db94d9e 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java @@ -37,7 +37,6 @@ import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.builder.SearchSourceBuilder; import java.io.IOException; @@ -53,12 +52,10 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public class RestMultiSearchAction extends BaseRestHandler { private final boolean allowExplicitIndex; - private final SearchRequestParsers searchRequestParsers; @Inject - public RestMultiSearchAction(Settings settings, RestController controller, SearchRequestParsers searchRequestParsers) { + public RestMultiSearchAction(Settings settings, RestController controller) { super(settings); - this.searchRequestParsers = searchRequestParsers; controller.registerHandler(GET, "/_msearch", this); controller.registerHandler(POST, "/_msearch", this); @@ -72,7 +69,7 @@ public class RestMultiSearchAction extends BaseRestHandler { @Override public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { - MultiSearchRequest multiSearchRequest = parseRequest(request, allowExplicitIndex, searchRequestParsers, parseFieldMatcher); + MultiSearchRequest multiSearchRequest = parseRequest(request, allowExplicitIndex, parseFieldMatcher); return channel -> client.multiSearch(multiSearchRequest, new RestToXContentListener<>(channel)); } @@ -80,7 +77,6 @@ public class RestMultiSearchAction extends BaseRestHandler { * Parses a {@link RestRequest} body and returns a {@link MultiSearchRequest} */ public static MultiSearchRequest parseRequest(RestRequest restRequest, boolean allowExplicitIndex, - SearchRequestParsers searchRequestParsers, ParseFieldMatcher parseFieldMatcher) throws IOException { MultiSearchRequest multiRequest = new MultiSearchRequest(); if (restRequest.hasParam("max_concurrent_searches")) { diff --git a/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index fe4fe4a4f19..68669fe07eb 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -36,7 +36,6 @@ import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestActions; import org.elasticsearch.rest.action.RestStatusToXContentListener; import org.elasticsearch.search.Scroll; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.fetch.StoredFieldsContext; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -54,13 +53,9 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.search.suggest.SuggestBuilders.termSuggestion; public class RestSearchAction extends BaseRestHandler { - - private final SearchRequestParsers searchRequestParsers; - @Inject - public RestSearchAction(Settings settings, RestController controller, SearchRequestParsers searchRequestParsers) { + public RestSearchAction(Settings settings, RestController controller) { super(settings); - this.searchRequestParsers = searchRequestParsers; controller.registerHandler(GET, "/_search", this); controller.registerHandler(POST, "/_search", this); controller.registerHandler(GET, "/{index}/_search", this); @@ -73,7 +68,7 @@ public class RestSearchAction extends BaseRestHandler { public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { SearchRequest searchRequest = new SearchRequest(); request.withContentOrSourceParamParserOrNull(parser -> - parseSearchRequest(searchRequest, request, searchRequestParsers, parseFieldMatcher, parser)); + parseSearchRequest(searchRequest, request, parseFieldMatcher, parser)); return channel -> client.search(searchRequest, new RestStatusToXContentListener<>(channel)); } @@ -84,8 +79,8 @@ public class RestSearchAction extends BaseRestHandler { * @param requestContentParser body of the request to read. This method does not attempt to read the body from the {@code request} * parameter */ - public static void parseSearchRequest(SearchRequest searchRequest, RestRequest request, SearchRequestParsers searchRequestParsers, - ParseFieldMatcher parseFieldMatcher, XContentParser requestContentParser) throws IOException { + public static void parseSearchRequest(SearchRequest searchRequest, RestRequest request, ParseFieldMatcher parseFieldMatcher, + XContentParser requestContentParser) throws IOException { if (searchRequest.source() == null) { searchRequest.source(new SearchSourceBuilder()); diff --git a/core/src/main/java/org/elasticsearch/search/SearchModule.java b/core/src/main/java/org/elasticsearch/search/SearchModule.java index 951855820ad..048630dad20 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/core/src/main/java/org/elasticsearch/search/SearchModule.java @@ -276,7 +276,6 @@ public class SearchModule { private final Settings settings; private final List namedWriteables = new ArrayList<>(); private final List namedXContents = new ArrayList<>(); - private final SearchRequestParsers searchRequestParsers; public SearchModule(Settings settings, boolean transportClient, List plugins) { this.settings = settings; @@ -295,7 +294,6 @@ public class SearchModule { registerFetchSubPhases(plugins); registerSearchExts(plugins); registerShapes(); - searchRequestParsers = new SearchRequestParsers(); } public List getNamedWriteables() { @@ -306,10 +304,6 @@ public class SearchModule { return namedXContents; } - public SearchRequestParsers getSearchRequestParsers() { - return searchRequestParsers; - } - /** * Returns the {@link Highlighter} registry */ diff --git a/core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java b/core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java deleted file mode 100644 index 8a3186ad1da..00000000000 --- a/core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you 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 org.elasticsearch.search; - -/** - * A container for all parsers used to parse - * {@link org.elasticsearch.action.search.SearchRequest} objects from a rest request. - */ -public class SearchRequestParsers { - public SearchRequestParsers() { - } -} diff --git a/core/src/test/java/org/elasticsearch/action/ExplainRequestTests.java b/core/src/test/java/org/elasticsearch/action/ExplainRequestTests.java index 1bc895095ba..adab07f52a3 100644 --- a/core/src/test/java/org/elasticsearch/action/ExplainRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/ExplainRequestTests.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.search.SearchModule; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.test.ESTestCase; @@ -41,9 +40,8 @@ import java.util.Collections; import java.util.List; public class ExplainRequestTests extends ESTestCase { + private NamedWriteableRegistry namedWriteableRegistry; - protected NamedWriteableRegistry namedWriteableRegistry; - protected SearchRequestParsers searchRequestParsers; public void setUp() throws Exception { super.setUp(); IndicesModule indicesModule = new IndicesModule(Collections.emptyList()); @@ -52,10 +50,8 @@ public class ExplainRequestTests extends ESTestCase { entries.addAll(indicesModule.getNamedWriteables()); entries.addAll(searchModule.getNamedWriteables()); namedWriteableRegistry = new NamedWriteableRegistry(entries); - searchRequestParsers = searchModule.getSearchRequestParsers(); } - public void testSerialize() throws IOException { try (BytesStreamOutput output = new BytesStreamOutput()) { ExplainRequest request = new ExplainRequest("index", "type", "id"); diff --git a/core/src/test/java/org/elasticsearch/action/ShardValidateQueryRequestTests.java b/core/src/test/java/org/elasticsearch/action/ShardValidateQueryRequestTests.java index c1d18146a08..55bc37df18a 100644 --- a/core/src/test/java/org/elasticsearch/action/ShardValidateQueryRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/ShardValidateQueryRequestTests.java @@ -31,7 +31,6 @@ import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.search.SearchModule; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.test.ESTestCase; @@ -42,9 +41,8 @@ import java.util.Collections; import java.util.List; public class ShardValidateQueryRequestTests extends ESTestCase { - protected NamedWriteableRegistry namedWriteableRegistry; - protected SearchRequestParsers searchRequestParsers; + public void setUp() throws Exception { super.setUp(); IndicesModule indicesModule = new IndicesModule(Collections.emptyList()); @@ -53,10 +51,8 @@ public class ShardValidateQueryRequestTests extends ESTestCase { entries.addAll(indicesModule.getNamedWriteables()); entries.addAll(searchModule.getNamedWriteables()); namedWriteableRegistry = new NamedWriteableRegistry(entries); - searchRequestParsers = searchModule.getSearchRequestParsers(); } - public void testSerialize() throws IOException { try (BytesStreamOutput output = new BytesStreamOutput()) { ValidateQueryRequest validateQueryRequest = new ValidateQueryRequest("indices"); diff --git a/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java b/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java index a5bd3df4d63..c4eac65c212 100644 --- a/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java @@ -30,7 +30,6 @@ import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.search.RestMultiSearchAction; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.StreamsUtils; import org.elasticsearch.test.rest.FakeRestRequest; @@ -163,7 +162,7 @@ public class MultiSearchRequestTests extends ESTestCase { private MultiSearchRequest parseMultiSearchRequest(String sample) throws IOException { byte[] data = StreamsUtils.copyToBytesFromClasspath(sample); RestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()).withContent(new BytesArray(data)).build(); - return RestMultiSearchAction.parseRequest(restRequest, true, new SearchRequestParsers(), ParseFieldMatcher.EMPTY); + return RestMultiSearchAction.parseRequest(restRequest, true, ParseFieldMatcher.EMPTY); } @Override diff --git a/core/src/test/java/org/elasticsearch/search/AbstractSearchTestCase.java b/core/src/test/java/org/elasticsearch/search/AbstractSearchTestCase.java index 87c1fe66044..0163c98692e 100644 --- a/core/src/test/java/org/elasticsearch/search/AbstractSearchTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/AbstractSearchTestCase.java @@ -53,7 +53,6 @@ import java.util.function.Supplier; public abstract class AbstractSearchTestCase extends ESTestCase { protected NamedWriteableRegistry namedWriteableRegistry; - protected SearchRequestParsers searchRequestParsers; private TestSearchExtPlugin searchExtPlugin; private NamedXContentRegistry xContentRegistry; @@ -67,7 +66,6 @@ public abstract class AbstractSearchTestCase extends ESTestCase { entries.addAll(searchModule.getNamedWriteables()); namedWriteableRegistry = new NamedWriteableRegistry(entries); xContentRegistry = new NamedXContentRegistry(searchModule.getNamedXContents()); - searchRequestParsers = searchModule.getSearchRequestParsers(); } @Override diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/RestSearchTemplateAction.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/RestSearchTemplateAction.java index 192cdc6a463..f88ecc981a8 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/RestSearchTemplateAction.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/RestSearchTemplateAction.java @@ -36,7 +36,6 @@ import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestStatusToXContentListener; import org.elasticsearch.rest.action.search.RestSearchAction; import org.elasticsearch.script.ScriptType; -import org.elasticsearch.search.SearchRequestParsers; import java.io.IOException; @@ -75,12 +74,9 @@ public class RestSearchTemplateAction extends BaseRestHandler { }, new ParseField("inline", "template"), ObjectParser.ValueType.OBJECT_OR_STRING); } - private final SearchRequestParsers searchRequestParsers; - @Inject - public RestSearchTemplateAction(Settings settings, RestController controller, SearchRequestParsers searchRequestParsers) { + public RestSearchTemplateAction(Settings settings, RestController controller) { super(settings); - this.searchRequestParsers = searchRequestParsers; controller.registerHandler(GET, "/_search/template", this); controller.registerHandler(POST, "/_search/template", this); @@ -98,7 +94,7 @@ public class RestSearchTemplateAction extends BaseRestHandler { // Creates the search request with all required params SearchRequest searchRequest = new SearchRequest(); - RestSearchAction.parseSearchRequest(searchRequest, request, searchRequestParsers, parseFieldMatcher, null); + RestSearchAction.parseSearchRequest(searchRequest, request, parseFieldMatcher, null); // Creates the search template request SearchTemplateRequest searchTemplateRequest; diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java index 9d1071f62e5..683f4ccf02e 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java @@ -36,7 +36,6 @@ import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -51,19 +50,17 @@ public class TransportSearchTemplateAction extends HandledTransportAction > extends BaseRestHandler { - protected final SearchRequestParsers searchRequestParsers; private final A action; - protected AbstractBaseReindexRestHandler(Settings settings, SearchRequestParsers searchRequestParsers, A action) { + protected AbstractBaseReindexRestHandler(Settings settings, A action) { super(settings); - this.searchRequestParsers = searchRequestParsers; this.action = action; } diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java index 79bc0f381ef..1b6b8c74a78 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.search.RestSearchAction; -import org.elasticsearch.search.SearchRequestParsers; import java.io.IOException; import java.util.Map; @@ -43,8 +42,8 @@ public abstract class AbstractBulkByQueryRestHandler< Request extends AbstractBulkByScrollRequest, A extends GenericAction> extends AbstractBaseReindexRestHandler { - protected AbstractBulkByQueryRestHandler(Settings settings, SearchRequestParsers searchRequestParsers, A action) { - super(settings, searchRequestParsers, action); + protected AbstractBulkByQueryRestHandler(Settings settings, A action) { + super(settings, action); } protected void parseInternalRequest(Request internal, RestRequest restRequest, @@ -63,8 +62,7 @@ public abstract class AbstractBulkByQueryRestHandler< * the generated parser probably is a noop but we should do the accounting just in case. It doesn't hurt to close twice but it * really hurts not to close if by some miracle we have to. */ try { - RestSearchAction.parseSearchRequest(searchRequest, restRequest, searchRequestParsers, parseFieldMatcher, - searchRequestParser); + RestSearchAction.parseSearchRequest(searchRequest, restRequest, parseFieldMatcher, searchRequestParser); } finally { IOUtils.close(searchRequestParser); } diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryAction.java index 0e21024eb54..019ff9c1975 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryAction.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryAction.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.search.SearchRequestParsers; import java.io.IOException; import java.util.HashMap; @@ -38,8 +37,8 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public class RestDeleteByQueryAction extends AbstractBulkByQueryRestHandler { @Inject - public RestDeleteByQueryAction(Settings settings, RestController controller, SearchRequestParsers searchRequestParsers) { - super(settings, searchRequestParsers, DeleteByQueryAction.INSTANCE); + public RestDeleteByQueryAction(Settings settings, RestController controller) { + super(settings, DeleteByQueryAction.INSTANCE); controller.registerHandler(POST, "/{index}/_delete_by_query", this); controller.registerHandler(POST, "/{index}/{type}/_delete_by_query", this); } diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java index e5295b9e4cc..51477b76032 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java @@ -24,7 +24,6 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParseFieldMatcherSupplier; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; @@ -43,7 +42,6 @@ import org.elasticsearch.index.reindex.remote.RemoteInfo; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.script.Script; -import org.elasticsearch.search.SearchRequestParsers; import java.io.IOException; import java.util.List; @@ -61,11 +59,11 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; * Expose reindex over rest. */ public class RestReindexAction extends AbstractBaseReindexRestHandler { - static final ObjectParser PARSER = new ObjectParser<>("reindex"); + static final ObjectParser PARSER = new ObjectParser<>("reindex"); private static final Pattern HOST_PATTERN = Pattern.compile("(?[^:]+)://(?[^:]+):(?\\d+)"); static { - ObjectParser.Parser sourceParser = (parser, request, context) -> { + ObjectParser.Parser sourceParser = (parser, request, context) -> { // Funky hack to work around Search not having a proper ObjectParser and us wanting to extract query if using remote. Map source = parser.map(); String[] indices = extractStringArray(source, "index"); @@ -80,11 +78,11 @@ public class RestReindexAction extends AbstractBaseReindexRestHandler destParser = new ObjectParser<>("dest"); + ObjectParser destParser = new ObjectParser<>("dest"); destParser.declareString(IndexRequest::index, new ParseField("index")); destParser.declareString(IndexRequest::type, new ParseField("type")); destParser.declareString(IndexRequest::routing, new ParseField("routing")); @@ -101,8 +99,8 @@ public class RestReindexAction extends AbstractBaseReindexRestHandler map = (Map) query; return builder.map(map).bytes(); } - - static class ReindexParseContext implements ParseFieldMatcherSupplier { - private final SearchRequestParsers searchRequestParsers; - private final ParseFieldMatcher parseFieldMatcher; - - ReindexParseContext(SearchRequestParsers searchRequestParsers, ParseFieldMatcher parseFieldMatcher) { - this.searchRequestParsers = searchRequestParsers; - this.parseFieldMatcher = parseFieldMatcher; - } - - QueryParseContext queryParseContext(XContentParser parser) { - return new QueryParseContext(parser, parseFieldMatcher); - } - - @Override - public ParseFieldMatcher getParseFieldMatcher() { - return this.parseFieldMatcher; - } - } } diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java index 723c12f84cd..3c0f5d2772d 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java @@ -29,7 +29,6 @@ import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; -import org.elasticsearch.search.SearchRequestParsers; import java.io.IOException; import java.util.Collections; @@ -44,8 +43,8 @@ import static org.elasticsearch.script.Script.DEFAULT_SCRIPT_LANG; public class RestUpdateByQueryAction extends AbstractBulkByQueryRestHandler { @Inject - public RestUpdateByQueryAction(Settings settings, RestController controller, SearchRequestParsers searchRequestParsers) { - super(settings, searchRequestParsers, UpdateByQueryAction.INSTANCE); + public RestUpdateByQueryAction(Settings settings, RestController controller) { + super(settings, UpdateByQueryAction.INSTANCE); controller.registerHandler(POST, "/{index}/_update_by_query", this); controller.registerHandler(POST, "/{index}/{type}/_update_by_query", this); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java index 32cb2d5e9d3..abef7fb5902 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java @@ -21,16 +21,13 @@ package org.elasticsearch.index.reindex; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchRequest; -import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; -import org.elasticsearch.index.reindex.RestReindexAction.ReindexParseContext; import org.elasticsearch.index.reindex.remote.RemoteInfo; import org.elasticsearch.rest.RestController; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestRequest; @@ -124,16 +121,14 @@ public class RestReindexActionTests extends ESTestCase { } try (XContentParser p = createParser(JsonXContent.jsonXContent, request)) { ReindexRequest r = new ReindexRequest(new SearchRequest(), new IndexRequest()); - SearchRequestParsers searchParsers = new SearchRequestParsers(); - RestReindexAction.PARSER.parse(p, r, new ReindexParseContext(searchParsers, ParseFieldMatcher.STRICT)); + RestReindexAction.PARSER.parse(p, r, null); assertEquals("localhost", r.getRemoteInfo().getHost()); assertArrayEquals(new String[] {"source"}, r.getSearchRequest().indices()); } } public void testPipelineQueryParameterIsError() throws IOException { - SearchRequestParsers parsers = new SearchRequestParsers(); - RestReindexAction action = new RestReindexAction(Settings.EMPTY, mock(RestController.class), parsers); + RestReindexAction action = new RestReindexAction(Settings.EMPTY, mock(RestController.class)); FakeRestRequest.Builder request = new FakeRestRequest.Builder(xContentRegistry()); try (XContentBuilder body = JsonXContent.contentBuilder().prettyPrint()) { diff --git a/plugins/discovery-file/src/main/java/org/elasticsearch/discovery/file/FileBasedDiscoveryPlugin.java b/plugins/discovery-file/src/main/java/org/elasticsearch/discovery/file/FileBasedDiscoveryPlugin.java index 1250fb38a97..d809fd3fa88 100644 --- a/plugins/discovery-file/src/main/java/org/elasticsearch/discovery/file/FileBasedDiscoveryPlugin.java +++ b/plugins/discovery-file/src/main/java/org/elasticsearch/discovery/file/FileBasedDiscoveryPlugin.java @@ -35,7 +35,6 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.plugins.DiscoveryPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.ScriptService; -import org.elasticsearch.search.SearchRequestParsers; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.watcher.ResourceWatcherService; @@ -73,7 +72,6 @@ public class FileBasedDiscoveryPlugin extends Plugin implements DiscoveryPlugin ThreadPool threadPool, ResourceWatcherService resourceWatcherService, ScriptService scriptService, - SearchRequestParsers searchRequestParsers, NamedXContentRegistry xContentRegistry) { final int concurrentConnects = UnicastZenPing.DISCOVERY_ZEN_PING_UNICAST_CONCURRENT_CONNECTS_SETTING.get(settings); final ThreadFactory threadFactory = EsExecutors.daemonThreadFactory(settings, "[file_based_discovery_resolve]"); From 6810125a8b332c1d3ed83d15f30b1125a0ba6162 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 11 Jan 2017 17:02:36 +0100 Subject: [PATCH 2/2] Prevent open channel leaks if handshake times out or is interrupted (#22554) The low level TCP handshake can cause channel / connection leaks if it's interrupted since the caller doesn't close the channel / connection if the handshake was not successful. This commit fixes the channel leak and adds general test infrastructure to detect channel leaks in the future. --- .../elasticsearch/transport/TcpTransport.java | 14 ++++++++--- .../transport/MockTcpTransport.java | 23 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java index c2f0832b75e..f2b29706caf 100644 --- a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java +++ b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java @@ -477,8 +477,10 @@ public abstract class TcpTransport extends AbstractLifecycleComponent i @Override public final NodeChannels openConnection(DiscoveryNode node, ConnectionProfile connectionProfile) throws IOException { + boolean success = false; + NodeChannels nodeChannels = null; try { - NodeChannels nodeChannels = connectToChannels(node, connectionProfile); + nodeChannels = connectToChannels(node, connectionProfile); final Channel channel = nodeChannels.getChannels().get(0); // one channel is guaranteed by the connection profile final TimeValue connectTimeout = connectionProfile.getConnectTimeout() == null ? defaultConnectionProfile.getConnectTimeout() : @@ -487,13 +489,19 @@ public abstract class TcpTransport extends AbstractLifecycleComponent i connectTimeout : connectionProfile.getHandshakeTimeout(); final Version version = executeHandshake(node, channel, handshakeTimeout); transportServiceAdapter.onConnectionOpened(node); - return new NodeChannels(nodeChannels, version); // clone the channels - we now have the correct version + nodeChannels = new NodeChannels(nodeChannels, version);// clone the channels - we now have the correct version + success = true; + return nodeChannels; } catch (ConnectTransportException e) { throw e; } catch (Exception e) { // ConnectTransportExceptions are handled specifically on the caller end - we wrap the actual exception to ensure // only relevant exceptions are logged on the caller end.. this is the same as in connectToNode throw new ConnectTransportException(node, "general node connection failure", e); + } finally { + if (success == false) { + IOUtils.closeWhileHandlingException(nodeChannels); + } } } @@ -832,7 +840,7 @@ public abstract class TcpTransport extends AbstractLifecycleComponent i } @Override - protected final void doClose() { + protected void doClose() { } @Override diff --git a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java index 3b5b430f606..a08660bb388 100644 --- a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java +++ b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java @@ -49,6 +49,8 @@ import java.net.ServerSocket; import java.net.Socket; import java.net.SocketException; import java.net.SocketTimeoutException; +import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -74,6 +76,8 @@ public class MockTcpTransport extends TcpTransport */ public static final ConnectionProfile LIGHT_PROFILE; + private final Map openChannels = new IdentityHashMap<>(); + static { ConnectionProfile.Builder builder = new ConnectionProfile.Builder(); builder.addConnections(1, @@ -284,6 +288,9 @@ public class MockTcpTransport extends TcpTransport this.serverSocket = null; this.profile = profile; this.onClose = () -> onClose.accept(this); + synchronized (openChannels) { + openChannels.put(this, Boolean.TRUE); + } } /** @@ -353,12 +360,17 @@ public class MockTcpTransport extends TcpTransport @Override public void close() throws IOException { if (isOpen.compareAndSet(true, false)) { + final Boolean removedChannel; + synchronized (openChannels) { + removedChannel = openChannels.remove(this); + } //establish a happens-before edge between closing and accepting a new connection synchronized (this) { onChannelClosed(this); IOUtils.close(serverSocket, activeChannel, () -> IOUtils.close(workerChannels.keySet()), () -> cancellableThreads.cancel("channel closed"), onClose); } + assert removedChannel : "Channel was not removed or removed twice?"; } } } @@ -395,5 +407,16 @@ public class MockTcpTransport extends TcpTransport return mockVersion; } + @Override + protected void doClose() { + if (Thread.currentThread().isInterrupted() == false) { + // TCPTransport might be interrupted due to a timeout waiting for connections to be closed. + // in this case the thread is interrupted and we can't tell if we really missed something or if we are + // still closing connections. in such a case we don't assert the open channels + synchronized (openChannels) { + assert openChannels.isEmpty() : "there are still open channels: " + openChannels; + } + } + } }