Query refactoring: QueryFilterBuilder and Parser

core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java

@@ -19,9 +19,14 @@
 
 package org.elasticsearch.index.query;
 
+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.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Objects;
 
 /**
  * A filter that simply wraps a query. Same as the {@link QueryFilterBuilder} except that it allows also to
@@ -30,24 +35,105 @@
  *             query as a filter directly.
  */
 @Deprecated
-public class FQueryFilterBuilder extends QueryFilterBuilder {
+public class FQueryFilterBuilder extends AbstractQueryBuilder<FQueryFilterBuilder> {
 
     public static final String NAME = "fquery";
 
     static final FQueryFilterBuilder PROTOTYPE = new FQueryFilterBuilder(null);
 
+    private String queryName;
+
+    private final QueryBuilder queryBuilder;
+
     /**
      * A filter that simply wraps a query.
      *
      * @param queryBuilder The query to wrap as a filter
      */
     public FQueryFilterBuilder(QueryBuilder queryBuilder) {
-        super(queryBuilder);
+        this.queryBuilder = queryBuilder;
+    }
+
+    /**
+     * @return the query builder that is wrapped by this {@link FQueryFilterBuilder}
+     */
+    public QueryBuilder innerQuery() {
+        return this.queryBuilder;
+    }
+
+    /**
+     * Sets the query name for the filter that can be used when searching for matched_filters per hit.
+     */
+    public FQueryFilterBuilder queryName(String queryName) {
+        this.queryName = queryName;
+        return this;
+    }
+
+    /**
+     * @return the query name for the filter that can be used when searching for matched_filters per hit
+     */
+    public String queryName() {
+        return this.queryName;
     }
 
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
-        buildFQuery(builder, params);
+        builder.startObject(FQueryFilterBuilder.NAME);
+        builder.field("query");
+        queryBuilder.toXContent(builder, params);
+        if (queryName != null) {
+            builder.field("_name", queryName);
+        }
+        builder.endObject();
+    }
+
+    @Override
+    public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
+        // inner query builder can potentially be `null`, in that case we ignore it
+        if (this.queryBuilder == null) {
+            return null;
+        }
+        Query innerQuery = this.queryBuilder.toQuery(parseContext);
+        if (innerQuery == null) {
+            return null;
+        }
+        Query query = new ConstantScoreQuery(innerQuery);
+        if (queryName != null) {
+            parseContext.addNamedQuery(queryName, query);
+        }
+        return query;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(queryBuilder, queryName);
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null || getClass() != obj.getClass()) {
+            return false;
+        }
+        FQueryFilterBuilder other = (FQueryFilterBuilder) obj;
+        return Objects.equals(queryBuilder, other.queryBuilder) &&
+                Objects.equals(queryName, other.queryName);
+    }
+
+    @Override
+    public FQueryFilterBuilder readFrom(StreamInput in) throws IOException {
+        QueryBuilder innerQueryBuilder = in.readNamedWriteable();
+        FQueryFilterBuilder fquery = new FQueryFilterBuilder(innerQueryBuilder);
+        fquery.queryName = in.readOptionalString();
+        return fquery;
+    }
+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(this.queryBuilder);
+        out.writeOptionalString(queryName);
     }
 
     @Override

core/src/main/java/org/elasticsearch/index/query/FQueryFilterParser.java

@@ -19,8 +19,6 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.search.ConstantScoreQuery;
-import org.apache.lucene.search.Query;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.xcontent.XContentParser;
 
@@ -31,7 +29,7 @@
  * associate a name with the query filter.
  */
 @Deprecated
-public class FQueryFilterParser extends BaseQueryParserTemp {
+public class FQueryFilterParser extends BaseQueryParser {
 
     @Inject
     public FQueryFilterParser() {
@@ -43,10 +41,10 @@ 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 = null;
+        QueryBuilder wrappedQuery = null;
         boolean queryFound = false;
 
         String queryName = null;
@@ -60,7 +58,7 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if ("query".equals(currentFieldName)) {
                     queryFound = true;
-                    query = parseContext.parseInnerQuery();
+                    wrappedQuery = parseContext.parseInnerQueryBuilder();
                 } else {
                     throw new QueryParsingException(parseContext, "[fquery] query does not support [" + currentFieldName + "]");
                 }
@@ -75,14 +73,12 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
         if (!queryFound) {
             throw new QueryParsingException(parseContext, "[fquery] requires 'query' element");
         }
-        if (query == null) {
+        if (wrappedQuery == null) {
             return null;
         }
-        query = new ConstantScoreQuery(query);
-        if (queryName != null) {
-            parseContext.addNamedQuery(queryName, query);
-        }
-        return query;
+        FQueryFilterBuilder queryBuilder = new FQueryFilterBuilder(wrappedQuery);
+        queryBuilder.queryName(queryName);
+        return queryBuilder;
     }
 
     @Override

core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java

@@ -19,9 +19,14 @@
 
 package org.elasticsearch.index.query;
 
+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.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Objects;
 
 /**
  * A filter that simply wraps a query.
@@ -35,8 +40,6 @@ public class QueryFilterBuilder extends AbstractQueryBuilder<QueryFilterBuilder>
 
     private final QueryBuilder queryBuilder;
 
-    private String queryName;
-
     static final QueryFilterBuilder PROTOTYPE = new QueryFilterBuilder(null);
 
     /**
@@ -49,32 +52,57 @@ public QueryFilterBuilder(QueryBuilder queryBuilder) {
     }
 
     /**
-     * Sets the query name for the filter that can be used when searching for matched_filters per hit.
+     * @return the query builder that is wrapped by this {@link QueryFilterBuilder}
      */
-    public QueryFilterBuilder queryName(String queryName) {
-        this.queryName = queryName;
-        return this;
+    public QueryBuilder innerQuery() {
+        return this.queryBuilder;
     }
 
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
-        if (queryName == null) {
-            builder.field(NAME);
-            queryBuilder.toXContent(builder, params);
-        } else {
-            //fallback fo fquery when needed, for bw comp
-            buildFQuery(builder, params);
+        builder.field(NAME);
+        queryBuilder.toXContent(builder, params);
+    }
+
+    @Override
+    public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
+        // inner query builder can potentially be `null`, in that case we ignore it
+        if (this.queryBuilder == null) {
+            return null;
         }
+        Query innerQuery = this.queryBuilder.toQuery(parseContext);
+        if (innerQuery == null) {
+            return null;
+        }
+        return new ConstantScoreQuery(innerQuery);
     }
 
-    protected void buildFQuery(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject(FQueryFilterBuilder.NAME);
-        builder.field("query");
-        queryBuilder.toXContent(builder, params);
-        if (queryName != null) {
-            builder.field("_name", queryName);
+    @Override
+    public int hashCode() {
+        return Objects.hash(queryBuilder);
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null || getClass() != obj.getClass()) {
+            return false;
         }
-        builder.endObject();
+        QueryFilterBuilder other = (QueryFilterBuilder) obj;
+        return Objects.equals(queryBuilder, other.queryBuilder);
+    }
+
+    @Override
+    public QueryFilterBuilder readFrom(StreamInput in) throws IOException {
+        QueryBuilder innerQueryBuilder = in.readNamedWriteable();
+        return new QueryFilterBuilder(innerQueryBuilder);
+    }
+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(this.queryBuilder);
     }
 
     @Override

core/src/main/java/org/elasticsearch/index/query/QueryFilterParser.java

@@ -19,14 +19,12 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.search.ConstantScoreQuery;
-import org.apache.lucene.search.Query;
 import org.elasticsearch.common.inject.Inject;
 
 import java.io.IOException;
 
 @Deprecated
-public class QueryFilterParser extends BaseQueryParserTemp {
+public class QueryFilterParser extends BaseQueryParser {
 
     @Inject
     public QueryFilterParser() {
@@ -38,8 +36,8 @@ public String[] names() {
     }
 
     @Override
-    public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
-        return new ConstantScoreQuery(parseContext.parseInnerQuery());
+    public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
+        return new QueryFilterBuilder(parseContext.parseInnerQueryBuilder());
     }
 
     @Override

core/src/test/java/org/elasticsearch/index/query/FQueryFilterBuilderTest.java

@@ -0,0 +1,84 @@
+/*
+ * 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.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 FQueryFilterBuilderTest extends BaseQueryTestCase<FQueryFilterBuilder> {
+
+    @Override
+    protected Query createExpectedQuery(FQueryFilterBuilder queryBuilder, QueryParseContext context) throws QueryParsingException, IOException {
+        return new ConstantScoreQuery(queryBuilder.innerQuery().toQuery(context));
+    }
+
+    /**
+     * @return a AndQueryBuilder with random limit between 0 and 20
+     */
+    @Override
+    protected FQueryFilterBuilder createTestQueryBuilder() {
+        QueryBuilder innerQuery = RandomQueryBuilder.create(random());
+        FQueryFilterBuilder testQuery = new FQueryFilterBuilder(innerQuery);
+        testQuery.queryName(randomAsciiOfLengthBetween(1, 10));
+        return testQuery;
+    }
+
+    @Override
+    protected void assertLuceneQuery(FQueryFilterBuilder queryBuilder, Query query, QueryParseContext context) {
+        Query namedQuery = context.copyNamedFilters().get(queryBuilder.queryName());
+        assertThat(namedQuery, equalTo(query));
+    }
+
+    /**
+     * test corner case where no inner query exist
+     */
+    @Test
+    public void testNoInnerQuery() throws QueryParsingException, IOException {
+        FQueryFilterBuilder queryFilterQuery = new FQueryFilterBuilder(null);
+        assertNull(queryFilterQuery.toQuery(createContext()));
+    }
+
+    /**
+     * test wrapping an inner filter that returns null also returns <tt>null</null> to pass on upwards
+     */
+    @Test
+    public void testInnerQueryReturnsNull() throws IOException {
+        QueryParseContext context = createContext();
+
+        // create inner filter
+        String queryString = "{ \"constant_score\" : { \"filter\" : {} }";
+        XContentParser parser = XContentFactory.xContent(queryString).createParser(queryString);
+        context.reset(parser);
+        assertQueryHeader(parser, ConstantScoreQueryBuilder.PROTOTYPE.getName());
+        QueryBuilder innerQuery = context.indexQueryParserService().queryParser(ConstantScoreQueryBuilder.PROTOTYPE.getName()).fromXContent(context);
+
+        // check that when wrapping this filter, toQuery() returns null
+        FQueryFilterBuilder queryFilterQuery = new FQueryFilterBuilder(innerQuery);
+        assertNull(queryFilterQuery.toQuery(createContext()));
+    }
+}