Skip to content

Commit

Permalink
6.x Backport: Terms query validate bug (#30319)
Browse files Browse the repository at this point in the history
* 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

* Backport terms-query-validate-bug changes to 6.x
  • Loading branch information
Paul Sanwald authored May 3, 2018
1 parent b4d1dd9 commit 9b01a40
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ public String getExplanation() {

@Override
public void readFrom(StreamInput in) throws IOException {
index = in.readString();
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
index = in.readOptionalString();
} else {
index = in.readString();
}
if (in.getVersion().onOrAfter(Version.V_5_4_0)) {
shard = in.readInt();
} else {
Expand All @@ -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_6_4_0)) {
out.writeOptionalString(index);
} else {
out.writeString(index);
}
if (out.getVersion().onOrAfter(Version.V_5_4_0)) {
out.writeInt(shard);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<ValidateQueryRequest, ValidateQueryResponse, ShardValidateQueryRequest, ShardValidateQueryResponse> {

Expand All @@ -71,7 +75,39 @@ public TransportValidateQueryAction(Settings settings, ThreadPool threadPool, Cl
@Override
protected void doExecute(Task task, ValidateQueryRequest request, ActionListener<ValidateQueryResponse> listener) {
request.nowInMillis = System.currentTimeMillis();
super.doExecute(task, request, listener);
LongSupplier timeProvider = () -> request.nowInMillis;
ActionListener<org.elasticsearch.index.query.QueryBuilder> rewriteListener = ActionListener.wrap(rewrittenQuery -> {
request.query(rewrittenQuery);
super.doExecute(task, request, listener);
},
ex -> {
if (ex instanceof IndexNotFoundException ||
ex instanceof IndexClosedException) {
listener.onFailure(ex);
}
List<QueryExplanation> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -111,7 +112,7 @@ static <T extends Rewriteable<T>> void rewriteAndFetch(T original, QueryRewriteC
}
}
rewriteResponse.onResponse(builder);
} catch (IOException ex) {
} catch (IOException|IllegalArgumentException|ParsingException ex) {
rewriteResponse.onFailure(ex);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}

0 comments on commit 9b01a40

Please sign in to comment.