From 682b499f4640820ae78d29e9c35d5ead8ab4ed98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 17 Jun 2015 19:55:25 +0200 Subject: [PATCH] Query refactoring: NotQueryBuilder 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 Closes #11823 --- .../index/query/NotQueryBuilder.java | 82 ++++++++++++++-- .../index/query/NotQueryParser.java | 26 ++--- .../index/query/NotQueryBuilderTest.java | 95 +++++++++++++++++++ 3 files changed, 180 insertions(+), 23 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/index/query/NotQueryBuilderTest.java diff --git a/core/src/main/java/org/elasticsearch/index/query/NotQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/NotQueryBuilder.java index 40fc1a6f0d8..f7e446419ad 100644 --- a/core/src/main/java/org/elasticsearch/index/query/NotQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/NotQueryBuilder.java @@ -19,6 +19,10 @@ package org.elasticsearch.index.query; +import org.apache.lucene.search.Query; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; @@ -35,24 +39,34 @@ public class NotQueryBuilder extends AbstractQueryBuilder { private String queryName; - static final NotQueryBuilder PROTOTYPE = new NotQueryBuilder(); + static final NotQueryBuilder PROTOTYPE = new NotQueryBuilder(null); public NotQueryBuilder(QueryBuilder filter) { - this.filter = Objects.requireNonNull(filter); + this.filter = filter; } /** - * private constructor for internal use + * @return the filter added to "not". */ - private NotQueryBuilder() { - this.filter = null; + public QueryBuilder filter() { + return this.filter; } + /** + * Sets the filter name for the filter that can be used when searching for matched_filters per hit. + */ public NotQueryBuilder 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); @@ -64,8 +78,64 @@ public class NotQueryBuilder extends AbstractQueryBuilder { builder.endObject(); } + @Override + public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException { + if (filter == null) { + return null; + } + + Query luceneQuery = filter.toQuery(parseContext); + if (luceneQuery == null) { + return null; + } + + Query notQuery = Queries.not(luceneQuery); + if (queryName != null) { + parseContext.addNamedQuery(queryName, notQuery); + } + return notQuery; + } + + @Override + public QueryValidationException validate() { + // nothing to validate. + return null; + } + + @Override + public int hashCode() { + return Objects.hash(filter, queryName); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + NotQueryBuilder other = (NotQueryBuilder) obj; + return Objects.equals(filter, other.filter) && + Objects.equals(queryName, other.queryName); + } + + @Override + public NotQueryBuilder readFrom(StreamInput in) throws IOException { + QueryBuilder queryBuilder = in.readNamedWriteable(); + NotQueryBuilder notQueryBuilder = new NotQueryBuilder(queryBuilder); + notQueryBuilder.queryName = in.readOptionalString(); + return notQueryBuilder; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeNamedWriteable(filter); + out.writeOptionalString(queryName); + } + @Override public String getName() { return NAME; } -} \ No newline at end of file +} diff --git a/core/src/main/java/org/elasticsearch/index/query/NotQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/NotQueryParser.java index c33b947fa43..adddc4c70b1 100644 --- a/core/src/main/java/org/elasticsearch/index/query/NotQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/NotQueryParser.java @@ -19,10 +19,8 @@ package org.elasticsearch.index.query; -import org.apache.lucene.search.Query; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.inject.Inject; -import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.xcontent.XContentParser; import java.io.IOException; @@ -30,7 +28,7 @@ import java.io.IOException; /** * */ -public class NotQueryParser extends BaseQueryParserTemp { +public class NotQueryParser extends BaseQueryParser { private static final ParseField QUERY_FIELD = new ParseField("filter", "query"); @@ -44,10 +42,10 @@ public class NotQueryParser extends BaseQueryParserTemp { } @Override - public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException { + public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException { XContentParser parser = parseContext.parser(); - Query query = null; + QueryBuilder query = null; boolean queryFound = false; String queryName = null; @@ -60,17 +58,17 @@ public class NotQueryParser extends BaseQueryParserTemp { // skip } else if (token == XContentParser.Token.START_OBJECT) { if (QUERY_FIELD.match(currentFieldName)) { - query = parseContext.parseInnerFilter(); + query = parseContext.parseInnerFilterToQueryBuilder(); queryFound = true; } else { queryFound = true; // its the filter, and the name is the field - query = parseContext.parseInnerFilter(currentFieldName); + query = parseContext.parseInnerFilterToQueryBuilder(currentFieldName); } } else if (token == XContentParser.Token.START_ARRAY) { queryFound = true; // its the filter, and the name is the field - query = parseContext.parseInnerFilter(currentFieldName); + query = parseContext.parseInnerFilterToQueryBuilder(currentFieldName); } else if (token.isValue()) { if ("_name".equals(currentFieldName)) { queryName = parser.text(); @@ -84,15 +82,9 @@ public class NotQueryParser extends BaseQueryParserTemp { throw new QueryParsingException(parseContext, "filter is required when using `not` query"); } - if (query == null) { - return null; - } - - Query notQuery = Queries.not(query); - if (queryName != null) { - parseContext.addNamedQuery(queryName, notQuery); - } - return notQuery; + NotQueryBuilder notQueryBuilder = new NotQueryBuilder(query); + notQueryBuilder.queryName(queryName); + return notQueryBuilder; } @Override diff --git a/core/src/test/java/org/elasticsearch/index/query/NotQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/NotQueryBuilderTest.java new file mode 100644 index 00000000000..379edc4980d --- /dev/null +++ b/core/src/test/java/org/elasticsearch/index/query/NotQueryBuilderTest.java @@ -0,0 +1,95 @@ +/* + * 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.Query; +import org.elasticsearch.common.lucene.search.Queries; +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; + +public class NotQueryBuilderTest extends BaseQueryTestCase { + + @Override + protected Query createExpectedQuery(NotQueryBuilder queryBuilder, QueryParseContext context) throws QueryParsingException, IOException { + if (queryBuilder.filter() == null) { + return null; + } + return Queries.not(queryBuilder.filter().toQuery(context)); + } + + /** + * @return a NotQueryBuilder with random limit between 0 and 20 + */ + @Override + protected NotQueryBuilder createTestQueryBuilder() { + NotQueryBuilder query = new NotQueryBuilder(RandomQueryBuilder.create(random())); + if (randomBoolean()) { + query.queryName(randomAsciiOfLengthBetween(1, 10)); + } + return query; + } + + @Override + protected void assertLuceneQuery(NotQueryBuilder 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 query exist + */ + @Test + public void testNoInnerQuerry() throws QueryParsingException, IOException { + NotQueryBuilder notQuery = new NotQueryBuilder(null); + assertNull(notQuery.toQuery(createContext())); + } + + /** + * Test corner case where inner queries returns null. + * This is legal, e.g. here {@link ConstantScoreQueryBuilder} can wrap a filter + * element that itself parses to null, e.g. + * '{ "constant_score" : "filter {} }' + */ + @Test + public void testInnerQuery() throws QueryParsingException, IOException { + NotQueryBuilder notQuery = new NotQueryBuilder(new ConstantScoreQueryBuilder(null)); + assertNull(notQuery.toQuery(createContext())); + } + + /** + * @throws IOException + */ + @Test(expected=QueryParsingException.class) + public void testMissingFilterSection() throws IOException { + QueryParseContext context = createContext(); + String queryString = "{ \"not\" : {}"; + XContentParser parser = XContentFactory.xContent(queryString).createParser(queryString); + context.reset(parser); + assertQueryHeader(parser, NotQueryBuilder.PROTOTYPE.getName()); + context.indexQueryParserService().queryParser(NotQueryBuilder.PROTOTYPE.getName()).fromXContent(context); + } +}