From 11db3170cd373674e7ed6499e1ad4cfd1012687c Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 12 May 2015 15:59:57 +0200 Subject: [PATCH] Query DSL: Fix `bool` parsing. In #10985 I introduced a bug that should clauses are parsed as filters while must_not clauses should be parsed as filters. --- .../index/query/BoolQueryParser.java | 6 +- .../query/SimpleIndexQueryParserTests.java | 106 +++++++++++++++++- 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java b/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java index 6f64586afdb..25d176c0219 100644 --- a/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java +++ b/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java @@ -77,12 +77,12 @@ public class BoolQueryParser implements QueryParser { clauses.add(new BooleanClause(query, BooleanClause.Occur.MUST)); } } else if ("must_not".equals(currentFieldName) || "mustNot".equals(currentFieldName)) { - Query query = parseContext.parseInnerQuery(); + Query query = parseContext.parseInnerFilter(); if (query != null) { clauses.add(new BooleanClause(query, BooleanClause.Occur.MUST_NOT)); } } else if ("should".equals(currentFieldName)) { - Query query = parseContext.parseInnerFilter(); + Query query = parseContext.parseInnerQuery(); if (query != null) { clauses.add(new BooleanClause(query, BooleanClause.Occur.SHOULD)); if (parseContext.isFilter() && minimumShouldMatch == null) { @@ -102,7 +102,7 @@ public class BoolQueryParser implements QueryParser { } } else if ("must_not".equals(currentFieldName) || "mustNot".equals(currentFieldName)) { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - Query query = parseContext.parseInnerQuery(); + Query query = parseContext.parseInnerFilter(); if (query != null) { clauses.add(new BooleanClause(query, BooleanClause.Occur.MUST_NOT)); } diff --git a/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java b/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java index c1095e38554..4c31ee897bd 100644 --- a/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java +++ b/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java @@ -73,6 +73,7 @@ import org.elasticsearch.action.termvectors.TermVectorsResponse; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.compress.CompressedString; +import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.lucene.search.MoreLikeThisQuery; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.lucene.search.function.BoostScoreFunction; @@ -82,9 +83,12 @@ import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.unit.Fuzziness; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.index.AbstractIndexComponent; +import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.ParsedDocument; @@ -134,7 +138,6 @@ import static org.elasticsearch.index.query.QueryBuilders.spanTermQuery; import static org.elasticsearch.index.query.QueryBuilders.spanWithinQuery; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.elasticsearch.index.query.QueryBuilders.termsQuery; -import static org.elasticsearch.index.query.QueryBuilders.termsQuery; import static org.elasticsearch.index.query.QueryBuilders.wildcardQuery; import static org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders.factorFunction; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertBooleanSubQuery; @@ -155,9 +158,54 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { private IndexQueryParserService queryParser; + private static class DummyQuery extends Query { + + public boolean isFilter; + + @Override + public String toString(String field) { + return getClass().getSimpleName(); + } + + } + + public static class DummyQueryParser extends AbstractIndexComponent implements QueryParser { + + @Inject + public DummyQueryParser(Index index, Settings indexSettings) { + super(index, indexSettings); + } + + @Override + public String[] names() { + return new String[] {"dummy"}; + } + + @Override + public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException { + assertEquals(XContentParser.Token.END_OBJECT, parseContext.parser().nextToken()); + DummyQuery query = new DummyQuery(); + query.isFilter = parseContext.isFilter(); + return query; + } + + } + + private static class DummyQueryBuilder extends BaseQueryBuilder { + @Override + protected void doXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject("dummy").endObject(); + } + } + + private static DummyQueryBuilder dummyQuery() { + return new DummyQueryBuilder(); + } + @Before public void setup() throws IOException { Settings settings = ImmutableSettings.settingsBuilder() + .put("index.queryparser.query.dummy.type", DummyQueryParser.class) .put("index.cache.filter.type", "none") .put("name", "SimpleIndexQueryParserTests") .build(); @@ -2509,4 +2557,60 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { q = csq.getQuery(); assertThat(q, instanceOf(TermsQuery.class)); } + + public void testConstantScoreParsesFilter() throws Exception { + IndexQueryParserService queryParser = queryParser(); + Query q = queryParser.parse(constantScoreQuery(dummyQuery())).query(); + Query inner = ((ConstantScoreQuery) q).getQuery(); + assertThat(inner, instanceOf(DummyQuery.class)); + assertEquals(true, ((DummyQuery) inner).isFilter); + } + + public void testBooleanParsesFilter() throws Exception { + IndexQueryParserService queryParser = queryParser(); + // single clause, serialized as inner object + Query q = queryParser.parse(boolQuery().should(dummyQuery()).must(dummyQuery()).mustNot(dummyQuery())).query(); + assertThat(q, instanceOf(BooleanQuery.class)); + BooleanQuery bq = (BooleanQuery) q; + assertEquals(3, bq.clauses().size()); + for (BooleanClause clause : bq.clauses()) { + DummyQuery dummy = (DummyQuery) clause.getQuery(); + switch (clause.getOccur()) { + case FILTER: + case MUST_NOT: + assertEquals(true, dummy.isFilter); + break; + case MUST: + case SHOULD: + assertEquals(false, dummy.isFilter); + break; + default: + throw new AssertionError(); + } + } + + // multiple clauses, serialized as inner arrays + q = queryParser.parse(boolQuery() + .should(dummyQuery()).should(dummyQuery()) + .must(dummyQuery()).must(dummyQuery()) + .mustNot(dummyQuery()).mustNot(dummyQuery())).query(); + assertThat(q, instanceOf(BooleanQuery.class)); + bq = (BooleanQuery) q; + assertEquals(6, bq.clauses().size()); + for (BooleanClause clause : bq.clauses()) { + DummyQuery dummy = (DummyQuery) clause.getQuery(); + switch (clause.getOccur()) { + case FILTER: + case MUST_NOT: + assertEquals(true, dummy.isFilter); + break; + case MUST: + case SHOULD: + assertEquals(false, dummy.isFilter); + break; + default: + throw new AssertionError(); + } + } + } }