Merge pull request elastic#12182 from cbuescher/feature/query-refacto…

…ring-spanmultiterm

Query refactoring: SpanMultiTermQueryBuilder and Parser

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

@@ -29,7 +29,7 @@
  * Doesn't support conversion to {@link org.elasticsearch.common.xcontent.XContent} via {@link #doXContent(XContentBuilder, Params)}.
  */
 //norelease to be removed once all queries support separate fromXContent and toQuery methods. Make AbstractQueryBuilder#toQuery final as well then.
-public class QueryWrappingQueryBuilder extends AbstractQueryBuilder<QueryWrappingQueryBuilder> {
+public class QueryWrappingQueryBuilder extends AbstractQueryBuilder<QueryWrappingQueryBuilder> implements MultiTermQueryBuilder<QueryWrappingQueryBuilder> {
 
     private Query query;
 

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

@@ -18,18 +18,38 @@
  */
 package org.elasticsearch.index.query;
 
+import org.apache.lucene.search.MultiTermQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper;
+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;
 
+/**
+ * Query that allows wraping a {@link MultiTermQueryBuilder} (one of wildcard, fuzzy, prefix, term, range or regexp query)
+ * as a {@link SpanQueryBuilder} so it can be nested.
+ */
 public class SpanMultiTermQueryBuilder extends AbstractQueryBuilder<SpanMultiTermQueryBuilder> implements SpanQueryBuilder<SpanMultiTermQueryBuilder> {
 
     public static final String NAME = "span_multi";
-    private MultiTermQueryBuilder multiTermQueryBuilder;
-    static final SpanMultiTermQueryBuilder PROTOTYPE = new SpanMultiTermQueryBuilder(null);
+    private final MultiTermQueryBuilder multiTermQueryBuilder;
+    static final SpanMultiTermQueryBuilder PROTOTYPE = new SpanMultiTermQueryBuilder();
 
     public SpanMultiTermQueryBuilder(MultiTermQueryBuilder multiTermQueryBuilder) {
-        this.multiTermQueryBuilder = multiTermQueryBuilder;
+        this.multiTermQueryBuilder = Objects.requireNonNull(multiTermQueryBuilder);
+    }
+
+    /**
+     * only used for prototype
+     */
+    private SpanMultiTermQueryBuilder() {
+        this.multiTermQueryBuilder = null;
+    }
+
+    public MultiTermQueryBuilder multiTermQueryBuilder() {
+        return this.multiTermQueryBuilder;
     }
 
     @Override
@@ -41,6 +61,42 @@ protected void doXContent(XContentBuilder builder, Params params)
         builder.endObject();
     }
 
+    @Override
+    protected Query doToQuery(QueryParseContext parseContext) throws IOException {
+        Query subQuery = multiTermQueryBuilder.toQuery(parseContext);
+        if (subQuery instanceof MultiTermQuery == false) {
+            throw new UnsupportedOperationException("unsupported inner query, should be " + MultiTermQuery.class.getName() +" but was "
+                    + subQuery.getClass().getName());
+        }
+        return new SpanMultiTermQueryWrapper<>((MultiTermQuery) subQuery);
+    }
+
+    @Override
+    public QueryValidationException validate() {
+        return validateInnerQuery(multiTermQueryBuilder, null);
+    }
+
+    @Override
+    protected SpanMultiTermQueryBuilder doReadFrom(StreamInput in) throws IOException {
+        MultiTermQueryBuilder multiTermBuilder = in.readNamedWriteable();
+        return new SpanMultiTermQueryBuilder(multiTermBuilder);
+    }
+
+    @Override
+    protected void doWriteTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(multiTermQueryBuilder);
+    }
+
+    @Override
+    protected int doHashCode() {
+        return Objects.hash(multiTermQueryBuilder);
+    }
+
+    @Override
+    protected boolean doEquals(SpanMultiTermQueryBuilder other) {
+        return Objects.equals(multiTermQueryBuilder, other.multiTermQueryBuilder);
+    }
+
     @Override
     public String getName() {
         return NAME;

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

@@ -18,9 +18,6 @@
  */
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.search.MultiTermQuery;
-import org.apache.lucene.search.Query;
-import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -31,7 +28,7 @@
 /**
  *
  */
-public class SpanMultiTermQueryParser extends BaseQueryParserTemp {
+public class SpanMultiTermQueryParser extends BaseQueryParser {
 
     public static final String MATCH_NAME = "match";
 
@@ -45,7 +42,7 @@ public String[] names() {
     }
 
     @Override
-    public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
+    public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
         XContentParser parser = parseContext.parser();
 
         Token token = parser.nextToken();
@@ -58,13 +55,13 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
             throw new QueryParsingException(parseContext, "spanMultiTerm must have [" + MATCH_NAME + "] multi term query clause");
         }
 
-        Query subQuery = parseContext.parseInnerQuery();
-        if (!(subQuery instanceof MultiTermQuery)) {
+        QueryBuilder subQuery = parseContext.parseInnerQueryBuilder();
+        if (subQuery instanceof MultiTermQueryBuilder == false) {
             throw new QueryParsingException(parseContext, "spanMultiTerm [" + MATCH_NAME + "] must be of type multi term query");
         }
 
         parser.nextToken();
-        return new SpanMultiTermQueryWrapper<>((MultiTermQuery) subQuery);
+        return new SpanMultiTermQueryBuilder((MultiTermQueryBuilder) subQuery);
     }
 
     @Override

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

@@ -20,6 +20,7 @@
 package org.elasticsearch.index.query;
 
 import com.carrotsearch.randomizedtesting.generators.RandomInts;
+import com.carrotsearch.randomizedtesting.generators.RandomStrings;
 
 import java.util.Random;
 
@@ -36,20 +37,37 @@ public class RandomQueryBuilder {
      * @return a random {@link QueryBuilder}
      */
     public static QueryBuilder createQuery(Random r) {
-        switch (RandomInts.randomIntBetween(r, 0, 3)) {
+        switch (RandomInts.randomIntBetween(r, 0, 4)) {
             case 0:
                 return new MatchAllQueryBuilderTest().createTestQueryBuilder();
             case 1:
                 return new TermQueryBuilderTest().createTestQueryBuilder();
             case 2:
                 return new IdsQueryBuilderTest().createTestQueryBuilder();
             case 3:
+                return createMultiTermQuery(r);
+            case 4:
                 return EmptyQueryBuilder.PROTOTYPE;
             default:
                 throw new UnsupportedOperationException();
         }
     }
 
+    /**
+     * Create a new multi term query of a random type
+     * @param r random seed
+     * @return a random {@link MultiTermQueryBuilder}
+     */
+    public static MultiTermQueryBuilder createMultiTermQuery(Random r) {
+        // for now, only use String Rangequeries for MultiTerm test, numeric and date makes little sense
+        // see issue #12123 for discussion
+        // Prefix / Fuzzy / RegEx / Wildcard can go here later once refactored and they have random query generators
+        RangeQueryBuilder query = new RangeQueryBuilder(BaseQueryTestCase.STRING_FIELD_NAME);
+        query.from("a" + RandomStrings.randomAsciiOfLengthBetween(r, 1, 10));
+        query.to("z" + RandomStrings.randomAsciiOfLengthBetween(r, 1, 10));
+        return query;
+    }
+
     /**
      * Create a new invalid query of a random type
      * @param r random seed

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

@@ -45,32 +45,42 @@ public class RangeQueryBuilderTest extends BaseQueryTestCase<RangeQueryBuilder>
     protected RangeQueryBuilder doCreateTestQueryBuilder() {
         RangeQueryBuilder query;
         // switch between numeric and date ranges
-        if (randomBoolean()) {
-            if (randomBoolean()) {
-                // use mapped integer field for numeric range queries
-                query = new RangeQueryBuilder(INT_FIELD_NAME);
-                query.from(randomIntBetween(1, 100));
-                query.to(randomIntBetween(101, 200));
-            } else {
-                // use unmapped field for numeric range queries
-                query = new RangeQueryBuilder(randomAsciiOfLengthBetween(1, 10));
-                query.from(0.0 - randomDouble());
-                query.to(randomDouble());
-            }
-        } else {
-            // use mapped date field, using date string representation
-            query = new RangeQueryBuilder(DATE_FIELD_NAME);
-            query.from(new DateTime(System.currentTimeMillis() - randomIntBetween(0, 1000000), DateTimeZone.UTC).toString());
-            query.to(new DateTime(System.currentTimeMillis() + randomIntBetween(0, 1000000), DateTimeZone.UTC).toString());
-            // Create timestamp option only then we have a date mapper, otherwise we could trigger exception.
-            if (createContext().mapperService().smartNameFieldType(DATE_FIELD_NAME) != null) {
+        switch (randomIntBetween(0, 2)) {
+            case 0:
                 if (randomBoolean()) {
-                    query.timeZone(TIMEZONE_IDS.get(randomIntBetween(0, TIMEZONE_IDS.size() - 1)));
+                    // use mapped integer field for numeric range queries
+                    query = new RangeQueryBuilder(INT_FIELD_NAME);
+                    query.from(randomIntBetween(1, 100));
+                    query.to(randomIntBetween(101, 200));
+                } else {
+                    // use unmapped field for numeric range queries
+                    query = new RangeQueryBuilder(randomAsciiOfLengthBetween(1, 10));
+                    query.from(0.0 - randomDouble());
+                    query.to(randomDouble());
                 }
-                if (randomBoolean()) {
-                    query.format("yyyy-MM-dd'T'HH:mm:ss.SSSZZ");
+                break;
+            case 1:
+                // use mapped date field, using date string representation
+                query = new RangeQueryBuilder(DATE_FIELD_NAME);
+                query.from(new DateTime(System.currentTimeMillis() - randomIntBetween(0, 1000000), DateTimeZone.UTC).toString());
+                query.to(new DateTime(System.currentTimeMillis() + randomIntBetween(0, 1000000), DateTimeZone.UTC).toString());
+                // Create timestamp option only then we have a date mapper,
+                // otherwise we could trigger exception.
+                if (createContext().mapperService().smartNameFieldType(DATE_FIELD_NAME) != null) {
+                    if (randomBoolean()) {
+                        query.timeZone(TIMEZONE_IDS.get(randomIntBetween(0, TIMEZONE_IDS.size() - 1)));
+                    }
+                    if (randomBoolean()) {
+                        query.format("yyyy-MM-dd'T'HH:mm:ss.SSSZZ");
+                    }
                 }
-            }
+                break;
+            case 2:
+            default:
+                query = new RangeQueryBuilder(STRING_FIELD_NAME);
+                query.from("a" + randomAsciiOfLengthBetween(1, 10));
+                query.to("z" + randomAsciiOfLengthBetween(1, 10));
+                break;
         }
         query.includeLower(randomBoolean()).includeUpper(randomBoolean());
         if (randomBoolean()) {

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

@@ -0,0 +1,82 @@
+/*
+ * 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.MultiTermQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper;
+import org.junit.Test;
+
+import java.io.IOException;
+
+public class SpanMultiTermQueryBuilderTest extends BaseQueryTestCase<SpanMultiTermQueryBuilder> {
+
+    @Override
+    protected Query doCreateExpectedQuery(SpanMultiTermQueryBuilder testQueryBuilder, QueryParseContext context) throws IOException {
+        Query multiTermQuery = testQueryBuilder.multiTermQueryBuilder().toQuery(context);
+        return new SpanMultiTermQueryWrapper<>((MultiTermQuery) multiTermQuery);
+    }
+
+    @Override
+    protected SpanMultiTermQueryBuilder doCreateTestQueryBuilder() {
+        MultiTermQueryBuilder multiTermQueryBuilder = RandomQueryBuilder.createMultiTermQuery(random());
+        return new SpanMultiTermQueryBuilder(multiTermQueryBuilder);
+    }
+
+    @Test
+    public void testValidate() {
+        int totalExpectedErrors = 0;
+        MultiTermQueryBuilder multiTermQueryBuilder;
+        if (randomBoolean()) {
+            multiTermQueryBuilder = new RangeQueryBuilder("");
+            totalExpectedErrors++;
+        } else {
+            multiTermQueryBuilder = new RangeQueryBuilder("field");
+        }
+        SpanMultiTermQueryBuilder queryBuilder = new SpanMultiTermQueryBuilder(multiTermQueryBuilder);
+        assertValidate(queryBuilder, totalExpectedErrors);
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void testInnerQueryNull() {
+        new SpanMultiTermQueryBuilder(null);
+    }
+
+    /**
+     * test checks that we throw an {@link UnsupportedOperationException} if the query wrapped
+     * by {@link SpanMultiTermQueryBuilder} does not generate a lucene {@link MultiTermQuery}.
+     * This is currently the case for {@link RangeQueryBuilder} when the target field is mapped
+     * to a date.
+     */
+    @Test
+    public void testUnsupportedInnerQueryType() throws IOException {
+        QueryParseContext parseContext = createContext();
+        // test makes only sense if date field is mapped
+        if (parseContext.fieldMapper(DATE_FIELD_NAME) != null) {
+            try {
+                RangeQueryBuilder query = new RangeQueryBuilder(DATE_FIELD_NAME);
+                new SpanMultiTermQueryBuilder(query).toQuery(createContext());
+                fail("Exception expected, range query on date fields should not generate a lucene " + MultiTermQuery.class.getName());
+            } catch (UnsupportedOperationException e) {
+                assert(e.getMessage().contains("unsupported inner query, should be " + MultiTermQuery.class.getName()));
+            }
+        }
+    }
+}