From 4a49077638763e55ced118fa8e51452b836fbba3 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 1 May 2018 13:38:22 -0700 Subject: [PATCH 1/2] Fix failure for validate API on a terms query (#29483) * WIP commit to try calling rewrite on coordinating node during TransportSearchAction * Use re-written query instead of using the original query * fix incorrect/unused imports and wildcarding * add error handling for cases where an exception is thrown * correct exception handling such that integration tests pass successfully * fix additional case covered by IndicesOptionsIntegrationIT. * add integration test case that verifies queries are now valid * add optional value for index * address review comments: catch superclass of XContentParseException fixes #29483 --- .../validate/query/QueryExplanation.java | 12 +++++- .../query/TransportValidateQueryAction.java | 38 ++++++++++++++++++- .../index/query/Rewriteable.java | 3 +- .../validate/SimpleValidateQueryIT.java | 19 ++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java index df9c12c95f4c9..1438daf29fd15 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java @@ -75,7 +75,11 @@ public String getExplanation() { @Override public void readFrom(StreamInput in) throws IOException { - index = in.readString(); + if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + index = in.readOptionalString(); + } else { + index = in.readString(); + } if (in.getVersion().onOrAfter(Version.V_5_4_0)) { shard = in.readInt(); } else { @@ -88,7 +92,11 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeString(index); + if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + out.writeOptionalString(index); + } else { + out.writeString(index); + } if (out.getVersion().onOrAfter(Version.V_5_4_0)) { out.writeInt(shard); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 0513a37e4fe0e..5be321734b5db 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -38,8 +38,11 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.query.ParsedQuery; import org.elasticsearch.index.query.QueryShardException; +import org.elasticsearch.index.query.Rewriteable; +import org.elasticsearch.indices.IndexClosedException; import org.elasticsearch.search.SearchService; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.search.internal.SearchContext; @@ -54,6 +57,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReferenceArray; +import java.util.function.LongSupplier; public class TransportValidateQueryAction extends TransportBroadcastAction { @@ -71,7 +75,39 @@ public TransportValidateQueryAction(Settings settings, ThreadPool threadPool, Cl @Override protected void doExecute(Task task, ValidateQueryRequest request, ActionListener listener) { request.nowInMillis = System.currentTimeMillis(); - super.doExecute(task, request, listener); + LongSupplier timeProvider = () -> request.nowInMillis; + ActionListener rewriteListener = ActionListener.wrap(rewrittenQuery -> { + request.query(rewrittenQuery); + super.doExecute(task, request, listener); + }, + ex -> { + if (ex instanceof IndexNotFoundException || + ex instanceof IndexClosedException) { + listener.onFailure(ex); + } + List explanations = new ArrayList<>(); + explanations.add(new QueryExplanation(null, + QueryExplanation.RANDOM_SHARD, + false, + null, + ex.getMessage())); + listener.onResponse( + new ValidateQueryResponse( + false, + explanations, + // totalShards is documented as "the total shards this request ran against", + // which is 0 since the failure is happening on the coordinating node. + 0, + 0 , + 0, + null)); + }); + if (request.query() == null) { + rewriteListener.onResponse(request.query()); + } else { + Rewriteable.rewriteAndFetch(request.query(), searchService.getRewriteContext(timeProvider), + rewriteListener); + } } @Override diff --git a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java index 492130527e8d4..ba8d6b84d5374 100644 --- a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java +++ b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java @@ -19,6 +19,7 @@ package org.elasticsearch.index.query; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.common.ParsingException; import java.io.IOException; import java.util.ArrayList; @@ -111,7 +112,7 @@ static > void rewriteAndFetch(T original, QueryRewriteC } } rewriteResponse.onResponse(builder); - } catch (IOException ex) { + } catch (IOException|IllegalArgumentException|ParsingException ex) { rewriteResponse.onFailure(ex); } } diff --git a/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java b/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java index a87f428fec51e..66fdf81744410 100644 --- a/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java +++ b/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java @@ -29,6 +29,8 @@ import org.elasticsearch.index.query.MoreLikeThisQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.index.query.TermsQueryBuilder; +import org.elasticsearch.indices.TermsLookup; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.ESIntegTestCase.Scope; @@ -330,4 +332,21 @@ private static void assertExplanations(QueryBuilder queryBuilder, assertThat(response.isValid(), equalTo(true)); } } + + public void testExplainTermsQueryWithLookup() throws Exception { + client().admin().indices().prepareCreate("twitter") + .addMapping("_doc", "user", "type=integer", "followers", "type=integer") + .setSettings(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 2).put("index.number_of_routing_shards", 2)).get(); + client().prepareIndex("twitter", "_doc", "1") + .setSource("followers", new int[] {1, 2, 3}).get(); + refresh(); + + TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "_doc", "1", "followers")); + ValidateQueryResponse response = client().admin().indices().prepareValidateQuery("twitter") + .setTypes("_doc") + .setQuery(termsLookupQuery) + .setExplain(true) + .execute().actionGet(); + assertThat(response.isValid(), is(true)); + } } From 0841b443ad0eedc902e328a3d6577106adac0f4b Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 1 May 2018 15:42:13 -0700 Subject: [PATCH 2/2] Backport terms-query-validate-bug changes to 6.x --- .../action/admin/indices/validate/query/QueryExplanation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java index 1438daf29fd15..780bf037f0e28 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java @@ -75,7 +75,7 @@ public String getExplanation() { @Override public void readFrom(StreamInput in) throws IOException { - if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + if (in.getVersion().onOrAfter(Version.V_6_4_0)) { index = in.readOptionalString(); } else { index = in.readString(); @@ -92,7 +92,7 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + if (out.getVersion().onOrAfter(Version.V_6_4_0)) { out.writeOptionalString(index); } else { out.writeString(index);