Merge pull request #11885 from MaineC/feature/filter-query-builder-refactoring

Isabel Drost-Fromm · Isabel Drost-Fromm · commit a0cccec86f94 · 2015-07-08T10:58:37.000+02:00
Refactors FilteredQueryBuilder. Relates to #10217
diff --git a/core/src/main/java/org/elasticsearch/index/query/FilteredQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/FilteredQueryBuilder.java
@@ -19,10 +19,15 @@
 
 package org.elasticsearch.index.query;
 
-import org.elasticsearch.common.Nullable;
+import org.apache.lucene.search.ConstantScoreQuery;
+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;
+import java.util.Objects;
 
 /**
  * A query that applies a filter to the results of another query.
@@ -31,36 +36,103 @@
 @Deprecated
 public class FilteredQueryBuilder extends AbstractQueryBuilder<FilteredQueryBuilder> {
 
+    /** Name of the query in the REST API. */
     public static final String NAME = "filtered";
-
+    /** The query to filter. */
     private final QueryBuilder queryBuilder;
-
+    /** The filter to apply to the query. */
     private final QueryBuilder filterBuilder;
 
     static final FilteredQueryBuilder PROTOTYPE = new FilteredQueryBuilder(null, null);
 
+    /**
+     * Returns a {@link MatchAllQueryBuilder} instance that will be used as
+     * default queryBuilder if none is supplied by the user. Feel free to
+     * set queryName and boost on that instance - it's always a new one.
+     * */
+    private static QueryBuilder generateDefaultQuery() {
+        return new MatchAllQueryBuilder();
+    }
+
+    /**
+     * A query that applies a filter to the results of a match_all query.
+     * @param filterBuilder The filter to apply on the query (Can be null)
+     * */
+    public FilteredQueryBuilder(QueryBuilder filterBuilder) {
+        this(generateDefaultQuery(), filterBuilder);
+    }
+
     /**
      * A query that applies a filter to the results of another query.
      *
-     * @param queryBuilder  The query to apply the filter to (Can be null)
+     * @param queryBuilder  The query to apply the filter to
      * @param filterBuilder The filter to apply on the query (Can be null)
      */
-    public FilteredQueryBuilder(@Nullable QueryBuilder queryBuilder, @Nullable QueryBuilder filterBuilder) {
-        this.queryBuilder = queryBuilder;
-        this.filterBuilder = filterBuilder;
+    public FilteredQueryBuilder(QueryBuilder queryBuilder, QueryBuilder filterBuilder) {
+        this.queryBuilder = (queryBuilder != null) ? queryBuilder : generateDefaultQuery();
+        this.filterBuilder = (filterBuilder != null) ? filterBuilder : EmptyQueryBuilder.PROTOTYPE;
+    }
+
+    /** Returns the query to apply the filter to. */
+    public QueryBuilder query() {
+        return queryBuilder;
+    }
+
+    /** Returns the filter to apply to the query results. */
+    public QueryBuilder filter() {
+        return filterBuilder;
     }
 
     @Override
-    protected void doXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject(NAME);
-        if (queryBuilder != null) {
-            builder.field("query");
-            queryBuilder.toXContent(builder, params);
+    protected boolean doEquals(FilteredQueryBuilder other) {
+        return Objects.equals(queryBuilder, other.queryBuilder) &&
+                Objects.equals(filterBuilder, other.filterBuilder);
+    }
+
+    @Override
+    public int doHashCode() {
+        return Objects.hash(queryBuilder, filterBuilder);
+    }
+
+    @Override
+    public Query doToQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
+        Query query = queryBuilder.toQuery(parseContext);
+        Query filter = filterBuilder.toQuery(parseContext);
+
+        if (query == null) {
+            // Most likely this query was generated from the JSON query DSL - it parsed to an EmptyQueryBuilder so we ignore
+            // the whole filtered query as there is nothing to filter on. See FilteredQueryParser for an example.
+            return null;
         }
-        if (filterBuilder != null) {
-            builder.field("filter");
-            filterBuilder.toXContent(builder, params);
+
+        if (filter == null || Queries.isConstantMatchAllQuery(filter)) {
+            // no filter, or match all filter
+            return query;
+        } else if (Queries.isConstantMatchAllQuery(query)) {
+            // if its a match_all query, use constant_score
+            return new ConstantScoreQuery(filter);
         }
+
+        // use a BooleanQuery
+        return Queries.filtered(query, filter);
+    }
+
+    @Override
+    public QueryValidationException validate() {
+        QueryValidationException validationException = null;
+        validationException = validateInnerQuery(queryBuilder, validationException);
+        validationException = validateInnerQuery(filterBuilder, validationException);
+        return validationException;
+
+    }
+
+    @Override
+    protected void doXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject(NAME);
+        builder.field("query");
+        queryBuilder.toXContent(builder, params);
+        builder.field("filter");
+        filterBuilder.toXContent(builder, params);
         printBoostAndQueryName(builder);
         builder.endObject();
     }
@@ -69,4 +141,18 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
     public String getName() {
         return NAME;
     }
+
+    @Override
+    public FilteredQueryBuilder doReadFrom(StreamInput in) throws IOException {
+        QueryBuilder query = in.readNamedWriteable();
+        QueryBuilder filter = in.readNamedWriteable();
+        FilteredQueryBuilder qb = new FilteredQueryBuilder(query, filter);
+        return qb;
+    }
+
+    @Override
+    public void doWriteTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(queryBuilder);
+        out.writeNamedWriteable(filterBuilder);
+    }
 }
diff --git a/core/src/main/java/org/elasticsearch/index/query/FilteredQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/FilteredQueryParser.java
@@ -19,20 +19,17 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.search.BooleanQuery;
-import org.apache.lucene.search.ConstantScoreQuery;
-import org.apache.lucene.search.Query;
 import org.elasticsearch.common.inject.Inject;
-import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.xcontent.XContentParser;
 
 import java.io.IOException;
 
 /**
- *
+ * @deprecated Use {@link BoolQueryParser} instead.
  */
+
 @Deprecated
-public class FilteredQueryParser extends BaseQueryParserTemp {
+public class FilteredQueryParser extends BaseQueryParser {
 
     @Inject
     public FilteredQueryParser() {
@@ -44,12 +41,11 @@ public String[] names() {
     }
 
     @Override
-    public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
+    public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
         XContentParser parser = parseContext.parser();
 
-        Query query = Queries.newMatchAllQuery();
-        Query filter = null;
-        boolean filterFound = false;
+        QueryBuilder query = null;
+        QueryBuilder filter = null;
         float boost = AbstractQueryBuilder.DEFAULT_BOOST;
         String queryName = null;
 
@@ -63,10 +59,9 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
                 // skip
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if ("query".equals(currentFieldName)) {
-                    query = parseContext.parseInnerQuery();
+                    query = parseContext.parseInnerQueryBuilder();
                 } else if ("filter".equals(currentFieldName)) {
-                    filterFound = true;
-                    filter = parseContext.parseInnerFilter();
+                    filter = parseContext.parseInnerFilterToQueryBuilder();
                 } else {
                     throw new QueryParsingException(parseContext, "[filtered] query does not support [" + currentFieldName + "]");
                 }
@@ -83,44 +78,15 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
             }
         }
 
-        // parsed internally, but returned null during parsing...
-        if (query == null) {
-            return null;
-        }
-
-        if (filter == null) {
-            if (!filterFound) {
-                // we allow for null filter, so it makes compositions on the client side to be simpler
-                return query;
-            } else {
-                // even if the filter is not found, and its null, we should simply ignore it, and go
-                // by the query
-                return query;
-            }
-        }
-        if (Queries.isConstantMatchAllQuery(filter)) {
-            // this is an instance of match all filter, just execute the query
-            return query;
-        }
-
-        // if its a match_all query, use constant_score
-        if (Queries.isConstantMatchAllQuery(query)) {
-            Query q = new ConstantScoreQuery(filter);
-            q.setBoost(boost);
-            return q;
-        }
-
-        BooleanQuery filteredQuery = Queries.filtered(query, filter);
-
-        filteredQuery.setBoost(boost);
-        if (queryName != null) {
-            parseContext.addNamedQuery(queryName, filteredQuery);
-        }
-        return filteredQuery;
+        FilteredQueryBuilder qb = new FilteredQueryBuilder(query, filter);
+        qb.boost(boost);
+        qb.queryName(queryName);
+        return qb;
     }
 
     @Override
     public FilteredQueryBuilder getBuilderPrototype() {
         return FilteredQueryBuilder.PROTOTYPE;
     }
+
 }
diff --git a/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java b/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java
@@ -218,8 +218,8 @@ public void testFromXContent() throws IOException {
 
         QueryBuilder newQuery = queryParserService.queryParser(testQuery.getName()).fromXContent(context);
         assertNotSame(newQuery, testQuery);
-        assertEquals("Queries should be equal", testQuery, newQuery);
-        assertEquals("Queries should have equal hashcodes", testQuery.hashCode(), newQuery.hashCode());
+        assertEquals(testQuery, newQuery);
+        assertEquals(testQuery.hashCode(), newQuery.hashCode());
     }
 
     /**
diff --git a/core/src/test/java/org/elasticsearch/index/query/FilteredQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/FilteredQueryBuilderTest.java
@@ -0,0 +1,105 @@
+/*
+ * 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.ConstantScoreQuery;
+import org.apache.lucene.search.Query;
+import org.elasticsearch.common.lucene.search.Queries;
+import org.junit.Test;
+
+import java.io.IOException;
+
+@SuppressWarnings("deprecation")
+public class FilteredQueryBuilderTest extends BaseQueryTestCase<FilteredQueryBuilder> {
+
+    @Override
+    protected FilteredQueryBuilder doCreateTestQueryBuilder() {
+        QueryBuilder queryBuilder = RandomQueryBuilder.createQuery(random());
+        QueryBuilder filterBuilder = RandomQueryBuilder.createQuery(random());
+
+        FilteredQueryBuilder query = new FilteredQueryBuilder(queryBuilder, filterBuilder);
+        return query;
+    }
+
+    @Override
+    protected Query doCreateExpectedQuery(FilteredQueryBuilder qb, QueryParseContext context) throws IOException {
+        Query query = qb.query().toQuery(context);
+        Query filter = qb.filter().toQuery(context);
+
+        if (query == null) {
+            return null;
+        }
+
+        Query result;
+        if (filter == null || Queries.isConstantMatchAllQuery(filter)) {
+            result = qb.query().toQuery(context);
+        } else if (Queries.isConstantMatchAllQuery(query)) {
+            result = new ConstantScoreQuery(filter);
+        } else {
+            result = Queries.filtered(qb.query().toQuery(context), filter);
+        }
+        result.setBoost(qb.boost());
+        return result;
+    }
+
+    @Test
+    public void testValidation() {
+        QueryBuilder valid = RandomQueryBuilder.createQuery(random());
+        QueryBuilder invalid = RandomQueryBuilder.createInvalidQuery(random());
+
+        // invalid cases
+        FilteredQueryBuilder qb = new FilteredQueryBuilder(invalid);
+        QueryValidationException result = qb.validate();
+        assertNotNull(result);
+        assertEquals(1, result.validationErrors().size());
+
+        qb = new FilteredQueryBuilder(valid, invalid);
+        result = qb.validate();
+        assertNotNull(result);
+        assertEquals(1, result.validationErrors().size());
+
+        qb = new FilteredQueryBuilder(invalid, valid);
+        result = qb.validate();
+        assertNotNull(result);
+        assertEquals(1, result.validationErrors().size());
+
+        qb = new FilteredQueryBuilder(invalid, invalid);
+        result = qb.validate();
+        assertNotNull(result);
+        assertEquals(2, result.validationErrors().size());
+
+        // valid cases
+        qb = new FilteredQueryBuilder(valid);
+        assertNull(qb.validate());
+
+        qb = new FilteredQueryBuilder(null);
+        assertNull(qb.validate());
+
+        qb = new FilteredQueryBuilder(null, valid);
+        assertNull(qb.validate());
+
+        qb = new FilteredQueryBuilder(valid, null);
+        assertNull(qb.validate());
+
+        qb = new FilteredQueryBuilder(valid, valid);
+        assertNull(qb.validate());
+    }
+
+}

Original file line number	Diff line number	Diff line change
`@@ -218,8 +218,8 @@ public void testFromXContent() throws IOException {`
`218`	`218`
`219`	`219`	`QueryBuilder newQuery = queryParserService.queryParser(testQuery.getName()).fromXContent(context);`
`220`	`220`	`assertNotSame(newQuery, testQuery);`
`221`		`- assertEquals("Queries should be equal", testQuery, newQuery);`
`222`		`- assertEquals("Queries should have equal hashcodes", testQuery.hashCode(), newQuery.hashCode());`
	`221`	`+ assertEquals(testQuery, newQuery);`
	`222`	`+ assertEquals(testQuery.hashCode(), newQuery.hashCode());`
`223`	`223`	`}`
`224`	`224`
`225`	`225`	`/**`