Merge pull request #11817 from cbuescher/feature/query-refactoring-or

Query refactoring: OrQueryBuilder and Parser

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

@@ -80,11 +80,10 @@ public class AndQueryBuilder extends AbstractQueryBuilder<AndQueryBuilder> {
     /**
      * @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);
@@ -108,7 +107,11 @@ public class AndQueryBuilder extends AbstractQueryBuilder<AndQueryBuilder> {
 
         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);
@@ -116,6 +119,12 @@ public class AndQueryBuilder extends AbstractQueryBuilder<AndQueryBuilder> {
         return query;
     }
 
+    @Override
+    public QueryValidationException validate() {
+        // nothing to validate.
+        return null;
+    }
+
     @Override
     public String getName() {
         return NAME;
@@ -136,7 +145,7 @@ public class AndQueryBuilder extends AbstractQueryBuilder<AndQueryBuilder> {
         }
         AndQueryBuilder other = (AndQueryBuilder) obj;
         return Objects.equals(filters, other.filters) &&
-                Objects.equals(queryName, other.queryName);
+               Objects.equals(queryName, other.queryName);
     }
 
     @Override

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;
     }
 

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<OrQueryBuilder> {
         return this;
     }
 
+    /**
+     * @return the list of filters added to "or".
+     */
+    public List<QueryBuilder> 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,71 @@ public class OrQueryBuilder extends AbstractQueryBuilder<OrQueryBuilder> {
         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 innerQuery = f.toQuery(parseContext);
+            // ignore queries that are null
+            if (innerQuery != null) {
+                query.add(innerQuery, 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;
     }
+
+    @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<QueryBuilder> 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);
+    }
 }

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<Query> queries = newArrayList();
+        ArrayList<QueryBuilder> 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()) {
+        OrQueryBuilder orQuery = new OrQueryBuilder();
-            return null;
+        for (QueryBuilder query : queries) {
+            orQuery.add(query);
         }
-
+        orQuery.queryName(queryName);
-        BooleanQuery query = new BooleanQuery();
+        return orQuery;
-        for (Query f : queries) {
-            query.add(f, Occur.SHOULD);
-        }
-        if (queryName != null) {
-            parseContext.addNamedQuery(queryName, query);
-        }
-        return query;
     }
 
     @Override

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

@@ -40,7 +40,10 @@ public class AndQueryBuilderTest extends BaseQueryTestCase<AndQueryBuilder> {
         }
         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;
     }

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

@@ -0,0 +1,94 @@
+/*
+ * 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<OrQueryBuilder> {
+
+    @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 innerQuery = subQuery.toQuery(context);
+            // ignore queries that are null
+            if (innerQuery != null) {
+                query.add(innerQuery, 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);
+    }
+}