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

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<NotQueryBuilder> {
 
     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() {
+    public QueryBuilder filter() {
-        this.filter = null;
+        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,6 +78,62 @@ public class NotQueryBuilder extends AbstractQueryBuilder<NotQueryBuilder> {
         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;

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) {
+        NotQueryBuilder notQueryBuilder = new NotQueryBuilder(query);
-            return null;
+        notQueryBuilder.queryName(queryName);
-        }
+        return notQueryBuilder;
-
-        Query notQuery = Queries.not(query);
-        if (queryName != null) {
-            parseContext.addNamedQuery(queryName, notQuery);
-        }
-        return notQuery;
     }
 
     @Override

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<NotQueryBuilder> {
+
+    @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 <tt>null</tt>, 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);
+    }
+}