Skip to content

Commit

Permalink
Fixed validate query parsing issues
Browse files Browse the repository at this point in the history 
Made sure that a match_all query is used when no query is specified and ensure no NPE is thrown either.
Also used the same code path as the search api to ensure that alias filters are taken into account, same for type filters.

Closes elastic#6111 Closes elastic#6112 Closes elastic#6116
  • Loading branch information
@javanna
javanna committed May 12, 2014
1 parent 6678da8 commit 6e317cb
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 38 deletions.
9 changes: 9 additions & 0 deletions rest-api-spec/test/indices.validate_query/10_basic.yaml
View file
Expand Up @@ -25,3 +25,12 @@

- is_false: valid

- do:
indices.validate_query:
explain: true

- is_true: valid
- match: {_shards.failed: 0}
- match: {explanations.0.index: 'testing'}
- match: {explanations.0.explanation: 'ConstantScore(*:*)'}

48 changes: 26 additions & 22 deletions ...a/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java
View file
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.index.query.IndexQueryParserService;
import org.elasticsearch.index.query.ParsedQuery;
import org.elasticsearch.index.query.QueryParsingException;
import org.elasticsearch.index.service.IndexService;
import org.elasticsearch.index.shard.service.IndexShard;
Expand Down Expand Up @@ -182,30 +181,35 @@ protected ShardValidateQueryResponse shardOperation(ShardValidateQueryRequest re
boolean valid;
String explanation = null;
String error = null;
if (request.source().length() == 0) {

DefaultSearchContext searchContext = new DefaultSearchContext(0,
new ShardSearchRequest().types(request.types()).nowInMillis(request.nowInMillis())
.filteringAliases(request.filteringAliases()),
null, indexShard.acquireSearcher("validate_query"), indexService, indexShard,
scriptService, cacheRecycler, pageCacheRecycler, bigArrays
);
SearchContext.setCurrent(searchContext);
try {
if (request.source() != null && request.source().length() > 0) {
searchContext.parsedQuery(queryParserService.parseQuery(request.source()));
}
searchContext.preProcess();

valid = true;
} else {
SearchContext.setCurrent(new DefaultSearchContext(0,
new ShardSearchRequest().types(request.types()).nowInMillis(request.nowInMillis()),
null, indexShard.acquireSearcher("validate_query"), indexService, indexShard,
scriptService, cacheRecycler, pageCacheRecycler, bigArrays));
try {
ParsedQuery parsedQuery = queryParserService.parseQuery(request.source());
valid = true;
if (request.explain()) {
explanation = parsedQuery.query().toString();
}
} catch (QueryParsingException e) {
valid = false;
error = e.getDetailedMessage();
} catch (AssertionError e) {
valid = false;
error = e.getMessage();
} finally {
SearchContext.current().close();
SearchContext.removeCurrent();
if (request.explain()) {
explanation = searchContext.query().toString();
}
} catch (QueryParsingException e) {
valid = false;
error = e.getDetailedMessage();
} catch (AssertionError e) {
valid = false;
error = e.getMessage();
} finally {
SearchContext.current().close();
SearchContext.removeCurrent();
}

return new ShardValidateQueryResponse(request.index(), request.shardId(), valid, explanation, error);
}
}
66 changes: 50 additions & 16 deletions src/test/java/org/elasticsearch/validate/SimpleValidateQueryTests.java
View file
Expand Up @@ -19,6 +19,7 @@
package org.elasticsearch.validate;

import com.google.common.base.Charsets;
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.common.geo.GeoDistance;
Expand All @@ -28,6 +29,7 @@
import org.elasticsearch.index.query.FilterBuilders;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.indices.IndexMissingException;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope;
import org.hamcrest.Matcher;
Expand Down Expand Up @@ -107,27 +109,27 @@ public void explainValidateQuery() throws Exception {
assertThat(response.getQueryExplanation().get(0).getError(), containsString("Failed to parse"));
assertThat(response.getQueryExplanation().get(0).getExplanation(), nullValue());

assertExplanation(QueryBuilders.queryString("_id:1"), equalTo("ConstantScore(_uid:type1#1)"));
assertExplanation(QueryBuilders.queryString("_id:1"), equalTo("filtered(ConstantScore(_uid:type1#1))->cache(_type:type1)"));

assertExplanation(QueryBuilders.idsQuery("type1").addIds("1").addIds("2"),
equalTo("ConstantScore(_uid:type1#1 _uid:type1#2)"));
equalTo("filtered(ConstantScore(_uid:type1#1 _uid:type1#2))->cache(_type:type1)"));

assertExplanation(QueryBuilders.queryString("foo"), equalTo("_all:foo"));
assertExplanation(QueryBuilders.queryString("foo"), equalTo("filtered(_all:foo)->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.termQuery("foo", "1"),
FilterBuilders.orFilter(
FilterBuilders.termFilter("bar", "2"),
FilterBuilders.termFilter("baz", "3")
)
), equalTo("filtered(foo:1)->cache(bar:[2 TO 2]) cache(baz:3)"));
), equalTo("filtered(filtered(foo:1)->cache(bar:[2 TO 2]) cache(baz:3))->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.termQuery("foo", "1"),
FilterBuilders.orFilter(
FilterBuilders.termFilter("bar", "2")
)
), equalTo("filtered(foo:1)->cache(bar:[2 TO 2])"));
), equalTo("filtered(filtered(foo:1)->cache(bar:[2 TO 2]))->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.matchAllQuery(),
Expand All @@ -136,55 +138,55 @@ public void explainValidateQuery() throws Exception {
.addPoint(30, -80)
.addPoint(20, -90)
.addPoint(40, -70) // closing polygon
), equalTo("ConstantScore(GeoPolygonFilter(pin.location, [[40.0, -70.0], [30.0, -80.0], [20.0, -90.0], [40.0, -70.0]]))"));
), equalTo("filtered(ConstantScore(GeoPolygonFilter(pin.location, [[40.0, -70.0], [30.0, -80.0], [20.0, -90.0], [40.0, -70.0]])))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.geoBoundingBoxFilter("pin.location")
.topLeft(40, -80)
.bottomRight(20, -70)
), equalTo("ConstantScore(GeoBoundingBoxFilter(pin.location, [40.0, -80.0], [20.0, -70.0]))"));
), equalTo("filtered(ConstantScore(GeoBoundingBoxFilter(pin.location, [40.0, -80.0], [20.0, -70.0])))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.geoDistanceFilter("pin.location")
.lat(10).lon(20).distance(15, DistanceUnit.DEFAULT).geoDistance(GeoDistance.PLANE)
), equalTo("ConstantScore(GeoDistanceFilter(pin.location, PLANE, 15.0, 10.0, 20.0))"));
), equalTo("filtered(ConstantScore(GeoDistanceFilter(pin.location, PLANE, 15.0, 10.0, 20.0)))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.geoDistanceFilter("pin.location")
.lat(10).lon(20).distance(15, DistanceUnit.DEFAULT).geoDistance(GeoDistance.PLANE)
), equalTo("ConstantScore(GeoDistanceFilter(pin.location, PLANE, 15.0, 10.0, 20.0))"));
), equalTo("filtered(ConstantScore(GeoDistanceFilter(pin.location, PLANE, 15.0, 10.0, 20.0)))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.geoDistanceRangeFilter("pin.location")
.lat(10).lon(20).from("15m").to("25m").geoDistance(GeoDistance.PLANE)
), equalTo("ConstantScore(GeoDistanceRangeFilter(pin.location, PLANE, [15.0 - 25.0], 10.0, 20.0))"));
), equalTo("filtered(ConstantScore(GeoDistanceRangeFilter(pin.location, PLANE, [15.0 - 25.0], 10.0, 20.0)))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.geoDistanceRangeFilter("pin.location")
.lat(10).lon(20).from("15miles").to("25miles").geoDistance(GeoDistance.PLANE)
), equalTo("ConstantScore(GeoDistanceRangeFilter(pin.location, PLANE, [" + DistanceUnit.DEFAULT.convert(15.0, DistanceUnit.MILES) + " - " + DistanceUnit.DEFAULT.convert(25.0, DistanceUnit.MILES) + "], 10.0, 20.0))"));
), equalTo("filtered(ConstantScore(GeoDistanceRangeFilter(pin.location, PLANE, [" + DistanceUnit.DEFAULT.convert(15.0, DistanceUnit.MILES) + " - " + DistanceUnit.DEFAULT.convert(25.0, DistanceUnit.MILES) + "], 10.0, 20.0)))->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.termQuery("foo", "1"),
FilterBuilders.andFilter(
FilterBuilders.termFilter("bar", "2"),
FilterBuilders.termFilter("baz", "3")
)
), equalTo("filtered(foo:1)->+cache(bar:[2 TO 2]) +cache(baz:3)"));
), equalTo("filtered(filtered(foo:1)->+cache(bar:[2 TO 2]) +cache(baz:3))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.termsFilter("foo", "1", "2", "3")),
equalTo("ConstantScore(cache(foo:1 foo:2 foo:3))"));
equalTo("filtered(ConstantScore(cache(foo:1 foo:2 foo:3)))->cache(_type:type1)"));

assertExplanation(QueryBuilders.constantScoreQuery(FilterBuilders.notFilter(FilterBuilders.termFilter("foo", "bar"))),
equalTo("ConstantScore(NotFilter(cache(foo:bar)))"));
equalTo("filtered(ConstantScore(NotFilter(cache(foo:bar))))->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.termQuery("foo", "1"),
FilterBuilders.hasChildFilter(
"child-type",
QueryBuilders.matchQuery("foo", "1")
)
), equalTo("filtered(foo:1)->CustomQueryWrappingFilter(child_filter[child-type/type1](filtered(foo:1)->cache(_type:child-type)))"));
), equalTo("filtered(filtered(foo:1)->CustomQueryWrappingFilter(child_filter[child-type/type1](filtered(foo:1)->cache(_type:child-type))))->cache(_type:type1)"));

assertExplanation(QueryBuilders.filteredQuery(
QueryBuilders.termQuery("foo", "1"),
FilterBuilders.scriptFilter("true")
), equalTo("filtered(foo:1)->ScriptFilter(true)"));
), equalTo("filtered(filtered(foo:1)->ScriptFilter(true))->cache(_type:type1)"));

}

Expand Down Expand Up @@ -253,6 +255,38 @@ public void explainDateRangeInQueryString() {
assertThat(response.isValid(), equalTo(true));
}

@Test(expected = IndexMissingException.class)
public void validateEmptyCluster() {
client().admin().indices().prepareValidateQuery().get();
}

@Test
public void explainNoQuery() {
createIndex("test");
ensureGreen();

ValidateQueryResponse validateQueryResponse = client().admin().indices().prepareValidateQuery().setExplain(true).get();
assertThat(validateQueryResponse.isValid(), equalTo(true));
assertThat(validateQueryResponse.getQueryExplanation().size(), equalTo(1));
assertThat(validateQueryResponse.getQueryExplanation().get(0).getIndex(), equalTo("test"));
assertThat(validateQueryResponse.getQueryExplanation().get(0).getExplanation(), equalTo("ConstantScore(*:*)"));
}

@Test
public void explainFilteredAlias() {
assertAcked(prepareCreate("test")
.addMapping("test", "field", "type=string")
.addAlias(new Alias("alias").filter(FilterBuilders.termFilter("field", "value1"))));
ensureGreen();

ValidateQueryResponse validateQueryResponse = client().admin().indices().prepareValidateQuery("alias")
.setQuery(QueryBuilders.matchAllQuery()).setExplain(true).get();
assertThat(validateQueryResponse.isValid(), equalTo(true));
assertThat(validateQueryResponse.getQueryExplanation().size(), equalTo(1));
assertThat(validateQueryResponse.getQueryExplanation().get(0).getIndex(), equalTo("test"));
assertThat(validateQueryResponse.getQueryExplanation().get(0).getExplanation(), containsString("field:value1"));
}

private void assertExplanation(QueryBuilder queryBuilder, Matcher<String> matcher) {
ValidateQueryResponse response = client().admin().indices().prepareValidateQuery("test")
.setTypes("type1")
Expand Down

0 comments on commit 6e317cb

Please sign in to comment.