Query refactoring: QueryFilterBuilder and Parser

Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings. In
this case this also includes FQueryFilterParser, since both queries are
closely related.

Relates to #10217
Closes #11729

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 @@ import java.io.IOException;
  *             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 @@ import java.io.IOException;
  * 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 class FQueryFilterParser 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 wrappedQuery = null;
         boolean queryFound = false;
 
         String queryName = null;
@@ -60,7 +58,7 @@ public class FQueryFilterParser extends BaseQueryParserTemp {
             } 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 class FQueryFilterParser extends BaseQueryParserTemp {
         if (!queryFound) {
             throw new QueryParsingException(parseContext, "[fquery] requires 'query' element");
         }
-        if (query == null) {
+        if (wrappedQuery == null) {
             return null;
         }
-        query = new ConstantScoreQuery(query);
+        FQueryFilterBuilder queryBuilder = new FQueryFilterBuilder(wrappedQuery);
-        if (queryName != null) {
+        queryBuilder.queryName(queryName);
-            parseContext.addNamedQuery(queryName, query);
+        return queryBuilder;
-        }
-        return query;
     }
 
     @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 class QueryFilterBuilder extends AbstractQueryBuilder<QueryFilterBuilder>
     }
 
     /**
-     * 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) {
+    public QueryBuilder innerQuery() {
-        this.queryName = queryName;
+        return this.queryBuilder;
-        return this;
     }
 
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
-        if (queryName == null) {
+        builder.field(NAME);
-            builder.field(NAME);
+        queryBuilder.toXContent(builder, params);
-            queryBuilder.toXContent(builder, params);
-        } else {
-            //fallback fo fquery when needed, for bw comp
-            buildFQuery(builder, params);
-        }
     }
 
-    protected void buildFQuery(XContentBuilder builder, Params params) throws IOException {
+    @Override
-        builder.startObject(FQueryFilterBuilder.NAME);
+    public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
-        builder.field("query");
+        // inner query builder can potentially be `null`, in that case we ignore it
-        queryBuilder.toXContent(builder, params);
+        if (this.queryBuilder == null) {
-        if (queryName != null) {
+            return null;
-            builder.field("_name", queryName);
         }
-        builder.endObject();
+        Query innerQuery = this.queryBuilder.toQuery(parseContext);
+        if (innerQuery == null) {
+            return null;
+        }
+        return new ConstantScoreQuery(innerQuery);
+    }
+
+    @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;
+        }
+        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 class QueryFilterParser extends BaseQueryParserTemp {
     }
 
     @Override
-    public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
+    public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
-        return new ConstantScoreQuery(parseContext.parseInnerQuery());
+        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()));
+    }
+}

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

@@ -0,0 +1,75 @@
+/*
+ * 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;
+
+@SuppressWarnings("deprecation")
+public class QueryFilterBuilderTest extends BaseQueryTestCase<QueryFilterBuilder> {
+
+    @Override
+    protected Query createExpectedQuery(QueryFilterBuilder 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 QueryFilterBuilder createTestQueryBuilder() {
+        QueryBuilder innerQuery = RandomQueryBuilder.create(random());
+        QueryFilterBuilder testQuery = new QueryFilterBuilder(innerQuery);
+        return testQuery;
+    }
+
+    /**
+     * test corner case where no inner query exist
+     */
+    @Test
+    public void testNoInnerQuery() throws QueryParsingException, IOException {
+        QueryFilterBuilder queryFilterQuery = new QueryFilterBuilder(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
+        QueryFilterBuilder queryFilterQuery = new QueryFilterBuilder(innerQuery);
+        assertNull(queryFilterQuery.toQuery(createContext()));
+    }
+}

docs/reference/migration/migrate_query_refactoring.asciidoc

@@ -0,0 +1,13 @@
+[[breaking-changes query-refactoring]]
+== Breaking changes on the query-refactoring branch
+
+This section discusses changes that are breaking to the current rest or java-api
+on the query-refactoring feature branch.
+
+=== Java-API
+
+==== QueryFilterBuilder
+
+Removed the setter `queryName(String queryName)` since this field is not supported
+in this type of query. Use `FQueryFilterBuilder.queryName(String queryName)` instead 
+when in need to wrap a named query as a filter.