From 6f8c0ce9667840cfd5fcc291088f395f00e23dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 17 Jun 2015 18:59:44 +0200 Subject: [PATCH 1/2] Query refactoring: OrQueryBuilder and Parser Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to #10217 --- .../index/query/AndQueryBuilder.java | 11 ++- .../index/query/AndQueryParser.java | 1 - .../index/query/OrQueryBuilder.java | 85 +++++++++++++++++- .../index/query/OrQueryParser.java | 34 +++---- .../index/query/OrQueryBuilderTest.java | 90 +++++++++++++++++++ 5 files changed, 193 insertions(+), 28 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java diff --git a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java index 0868a8f149b..d309efa6e10 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java @@ -80,11 +80,10 @@ public class AndQueryBuilder extends AbstractQueryBuilder { /** * @return the query name. */ - public Object queryName() { + public String queryName() { return this.queryName; } - @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); @@ -116,6 +115,12 @@ public class AndQueryBuilder extends AbstractQueryBuilder { return query; } + @Override + public QueryValidationException validate() { + // nothing to validate. + return null; + } + @Override public String getName() { return NAME; @@ -136,7 +141,7 @@ public class AndQueryBuilder extends AbstractQueryBuilder { } AndQueryBuilder other = (AndQueryBuilder) obj; return Objects.equals(filters, other.filters) && - Objects.equals(queryName, other.queryName); + Objects.equals(queryName, other.queryName); } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java index 274d0bd1ae3..033169cb5de 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java @@ -103,7 +103,6 @@ public class AndQueryParser extends BaseQueryParser { andQuery.add(query); } andQuery.queryName(queryName); - andQuery.validate(); return andQuery; } diff --git a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java index ba6f45b2370..fc26745cb8f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java @@ -21,11 +21,18 @@ package org.elasticsearch.index.query; import com.google.common.collect.Lists; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.BooleanClause.Occur; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.List; +import java.util.Objects; /** * A filter that matches documents matching boolean combinations of other filters. @@ -54,11 +61,28 @@ public class OrQueryBuilder extends AbstractQueryBuilder { return this; } + /** + * @return the list of filters added to "or". + */ + public List filters() { + return this.filters; + } + + /** + * Sets the filter name for the filter that can be used when searching for matched_filters per hit. + */ public OrQueryBuilder queryName(String queryName) { this.queryName = queryName; return this; } + /** + * @return the query name. + */ + public String queryName() { + return this.queryName; + } + @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); @@ -73,8 +97,67 @@ public class OrQueryBuilder extends AbstractQueryBuilder { builder.endObject(); } + @Override + public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException { + if (filters.isEmpty()) { + // no filters provided, this should be ignored upstream + return null; + } + + BooleanQuery query = new BooleanQuery(); + for (QueryBuilder f : filters) { + query.add(f.toQuery(parseContext), Occur.SHOULD); + } + if (queryName != null) { + parseContext.addNamedQuery(queryName, query); + } + return query; + } + + @Override + public QueryValidationException validate() { + // nothing to validate. + return null; + } + @Override public String getName() { return NAME; } -} \ No newline at end of file + + @Override + public int hashCode() { + return Objects.hash(filters, queryName); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + OrQueryBuilder other = (OrQueryBuilder) obj; + return Objects.equals(filters, other.filters) && + Objects.equals(queryName, other.queryName); + } + + @Override + public OrQueryBuilder readFrom(StreamInput in) throws IOException { + OrQueryBuilder orQueryBuilder = new OrQueryBuilder(); + List queryBuilders = in.readNamedWritableList(); + for (QueryBuilder queryBuilder : queryBuilders) { + orQueryBuilder.add(queryBuilder); + } + orQueryBuilder.queryName = in.readOptionalString(); + return orQueryBuilder; + + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeNamedWritableList(this.filters); + out.writeOptionalString(queryName); + } +} diff --git a/core/src/main/java/org/elasticsearch/index/query/OrQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/OrQueryParser.java index e328ac81742..7004a259c86 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryParser.java @@ -19,9 +19,6 @@ package org.elasticsearch.index.query; -import org.apache.lucene.search.BooleanClause.Occur; -import org.apache.lucene.search.BooleanQuery; -import org.apache.lucene.search.Query; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.xcontent.XContentParser; @@ -30,11 +27,8 @@ import java.util.ArrayList; import static com.google.common.collect.Lists.newArrayList; -/** - * - */ @Deprecated -public class OrQueryParser extends BaseQueryParserTemp { +public class OrQueryParser extends BaseQueryParser { @Inject public OrQueryParser() { @@ -46,10 +40,10 @@ public class OrQueryParser extends BaseQueryParserTemp { } @Override - public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException { + public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException { XContentParser parser = parseContext.parser(); - ArrayList queries = newArrayList(); + ArrayList queries = newArrayList(); boolean queriesFound = false; String queryName = null; @@ -58,7 +52,7 @@ public class OrQueryParser extends BaseQueryParserTemp { if (token == XContentParser.Token.START_ARRAY) { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { queriesFound = true; - Query filter = parseContext.parseInnerFilter(); + QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); if (filter != null) { queries.add(filter); } @@ -71,7 +65,7 @@ public class OrQueryParser extends BaseQueryParserTemp { if ("filters".equals(currentFieldName)) { queriesFound = true; while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - Query filter = parseContext.parseInnerFilter(); + QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); if (filter != null) { queries.add(filter); } @@ -79,7 +73,7 @@ public class OrQueryParser extends BaseQueryParserTemp { } else { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { queriesFound = true; - Query filter = parseContext.parseInnerFilter(); + QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); if (filter != null) { queries.add(filter); } @@ -99,18 +93,12 @@ public class OrQueryParser extends BaseQueryParserTemp { throw new QueryParsingException(parseContext, "[or] query requires 'filters' to be set on it'"); } - if (queries.isEmpty()) { - return null; + OrQueryBuilder orQuery = new OrQueryBuilder(); + for (QueryBuilder query : queries) { + orQuery.add(query); } - - BooleanQuery query = new BooleanQuery(); - for (Query f : queries) { - query.add(f, Occur.SHOULD); - } - if (queryName != null) { - parseContext.addNamedQuery(queryName, query); - } - return query; + orQuery.queryName(queryName); + return orQuery; } @Override diff --git a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java new file mode 100644 index 00000000000..8169cbd37ad --- /dev/null +++ b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java @@ -0,0 +1,90 @@ +/* + * 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.index.query; + +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.BooleanClause.Occur; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.junit.Test; + +import java.io.IOException; + +import static org.hamcrest.Matchers.equalTo; + +@SuppressWarnings("deprecation") +public class OrQueryBuilderTest extends BaseQueryTestCase { + + @Override + protected Query createExpectedQuery(OrQueryBuilder queryBuilder, QueryParseContext context) throws QueryParsingException, IOException { + if (queryBuilder.filters().isEmpty()) { + return null; + } + BooleanQuery query = new BooleanQuery(); + for (QueryBuilder subQuery : queryBuilder.filters()) { + query.add(subQuery.toQuery(context), Occur.SHOULD); + } + return query; + } + + /** + * @return an OrQueryBuilder with random limit between 0 and 20 + */ + @Override + protected OrQueryBuilder createTestQueryBuilder() { + OrQueryBuilder query = new OrQueryBuilder(); + int subQueries = randomIntBetween(1, 5); + for (int i = 0; i < subQueries; i++ ) { + query.add(RandomQueryBuilder.create(random())); + } + if (randomBoolean()) { + query.queryName(randomAsciiOfLengthBetween(1, 10)); + } + return query; + } + + @Override + protected void assertLuceneQuery(OrQueryBuilder queryBuilder, Query query, QueryParseContext context) { + if (queryBuilder.queryName() != null) { + Query namedQuery = context.copyNamedFilters().get(queryBuilder.queryName()); + assertThat(namedQuery, equalTo(query)); + } + } + + /** + * test corner case where no inner queries exist + */ + @Test + public void testNoInnerQueries() throws QueryParsingException, IOException { + OrQueryBuilder orQuery = new OrQueryBuilder(); + assertNull(orQuery.toQuery(createContext())); + } + + @Test(expected=QueryParsingException.class) + public void testMissingFiltersSection() throws IOException { + QueryParseContext context = createContext(); + String queryString = "{ \"or\" : {}"; + XContentParser parser = XContentFactory.xContent(queryString).createParser(queryString); + context.reset(parser); + assertQueryHeader(parser, OrQueryBuilder.PROTOTYPE.getName()); + context.indexQueryParserService().queryParser(OrQueryBuilder.PROTOTYPE.getName()).fromXContent(context); + } +} From f2573da77b73eaf031f236d5a6af0be4b5f431ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 23 Jun 2015 11:45:00 +0200 Subject: [PATCH 2/2] Added null checks when adding inner queries --- .../java/org/elasticsearch/index/query/AndQueryBuilder.java | 6 +++++- .../java/org/elasticsearch/index/query/OrQueryBuilder.java | 6 +++++- .../org/elasticsearch/index/query/AndQueryBuilderTest.java | 5 ++++- .../org/elasticsearch/index/query/OrQueryBuilderTest.java | 6 +++++- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java index d309efa6e10..920e20f2be3 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java @@ -107,7 +107,11 @@ public class AndQueryBuilder extends AbstractQueryBuilder { BooleanQuery query = new BooleanQuery(); for (QueryBuilder f : filters) { - query.add(f.toQuery(parseContext), Occur.MUST); + Query innerQuery = f.toQuery(parseContext); + // ignore queries that are null + if (innerQuery != null) { + query.add(innerQuery, Occur.MUST); + } } if (queryName != null) { parseContext.addNamedQuery(queryName, query); diff --git a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java index fc26745cb8f..27d6298b01d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java @@ -106,7 +106,11 @@ public class OrQueryBuilder extends AbstractQueryBuilder { BooleanQuery query = new BooleanQuery(); for (QueryBuilder f : filters) { - query.add(f.toQuery(parseContext), Occur.SHOULD); + Query innerQuery = f.toQuery(parseContext); + // ignore queries that are null + if (innerQuery != null) { + query.add(innerQuery, Occur.SHOULD); + } } if (queryName != null) { parseContext.addNamedQuery(queryName, query); diff --git a/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java index 98185b124a7..9c175bd5600 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java @@ -40,7 +40,10 @@ public class AndQueryBuilderTest extends BaseQueryTestCase { } BooleanQuery query = new BooleanQuery(); for (QueryBuilder subQuery : queryBuilder.filters()) { - query.add(subQuery.toQuery(context), Occur.MUST); + Query innerQuery = subQuery.toQuery(context); + if (innerQuery != null) { + query.add(innerQuery, Occur.MUST); + } } return query; } diff --git a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java index 8169cbd37ad..db9b7ba1b27 100644 --- a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java @@ -40,7 +40,11 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { } BooleanQuery query = new BooleanQuery(); for (QueryBuilder subQuery : queryBuilder.filters()) { - query.add(subQuery.toQuery(context), Occur.SHOULD); + Query innerQuery = subQuery.toQuery(context); + // ignore queries that are null + if (innerQuery != null) { + query.add(innerQuery, Occur.SHOULD); + } } return query; }