Cleanup code and always use strict parsing on query tests

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

@@ -99,6 +99,13 @@ public class HasParentQueryBuilder extends AbstractQueryBuilder<HasParentQueryBu
         return score;
     }
 
+    /**
+     * Returns the parents type name
+     */
+    public String type() {
+        return type;
+    }
+
     /**
      *  Returns inner hit definition in the scope of this query and reusing the defined type and query.
      */

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

@@ -78,7 +78,7 @@ public class NotQueryParser extends BaseQueryParser<NotQueryBuilder> {
         }
 
         if (!queryFound) {
-            throw new QueryParsingException(parseContext, "filter is required when using `not` query");
+            throw new QueryParsingException(parseContext, "query is required when using `not` query");
         }
 
         NotQueryBuilder notQueryBuilder = new NotQueryBuilder(query);

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

@@ -39,10 +39,7 @@ import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.indices.cache.query.terms.TermsLookup;
 
 import java.io.IOException;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Objects;
 
 /**
  * A filter for a field based on several terms matching on any of them.
@@ -51,16 +48,29 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
 
     public static final String NAME = "terms";
 
-    static final TermsQueryBuilder PROTOTYPE = new TermsQueryBuilder(null);
+    static final TermsQueryBuilder PROTOTYPE = new TermsQueryBuilder("");
 
     public static final boolean DEFAULT_DISABLE_COORD = false;
 
     private final String fieldName;
-    private List<Object> values;
+    private final List<Object> values;
+    @Deprecated
     private String minimumShouldMatch;
+    @Deprecated
     private boolean disableCoord = DEFAULT_DISABLE_COORD;
     private TermsLookup termsLookup;
 
+    TermsQueryBuilder(String fieldName, List<Object> values, String minimumShouldMatch, boolean disableCoord, TermsLookup termsLookup) {
+        this.fieldName = fieldName;
+        if (values == null && termsLookup == null) {
+            throw new IllegalArgumentException("No value specified for terms query");
+        }
+        this.values = values;
+        this.disableCoord = disableCoord;
+        this.minimumShouldMatch = minimumShouldMatch;
+        this.termsLookup = termsLookup;
+    }
+
     /**
      * A filter for a field based on several terms matching on any of them.
      *
@@ -128,6 +138,7 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
      */
     public TermsQueryBuilder(String fieldName) {
         this.fieldName = fieldName;
+        this.values = null;
     }
 
     /**
@@ -176,7 +187,7 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
         return this;
     }
 
-    public boolean disableCoord() {
+    boolean disableCoord() {
         return this.disableCoord;
     }
 
@@ -308,7 +319,9 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
         if (minimumShouldMatch != null) {
             builder.field("minimum_should_match", minimumShouldMatch);
         }
-        builder.field("disable_coord", disableCoord);
+        if (disableCoord != DEFAULT_DISABLE_COORD) {
+            builder.field("disable_coord", disableCoord);
+        }
         printBoostAndQueryName(builder);
         builder.endObject();
     }
@@ -391,14 +404,15 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
     @SuppressWarnings("unchecked")
     @Override
     protected TermsQueryBuilder doReadFrom(StreamInput in) throws IOException {
-        TermsQueryBuilder termsQueryBuilder = new TermsQueryBuilder(in.readString());
+        String field = in.readString();
+        TermsLookup lookup = null;
         if (in.readBoolean()) {
-            termsQueryBuilder.termsLookup = TermsLookup.readTermsLookupFrom(in);
+            lookup = TermsLookup.readTermsLookupFrom(in);
         }
-        termsQueryBuilder.values = ((List<Object>) in.readGenericValue());
+        List<Object> values = (List<Object>) in.readGenericValue();
-        termsQueryBuilder.minimumShouldMatch = in.readOptionalString();
+        String minimumShouldMatch = in.readOptionalString();
-        termsQueryBuilder.disableCoord = in.readBoolean();
+        boolean disableCoord = in.readBoolean();
-        return termsQueryBuilder;
+        return new TermsQueryBuilder(field, values, minimumShouldMatch, disableCoord, lookup);
     }
 
     @Override

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

@@ -104,16 +104,7 @@ public class TermsQueryParser extends BaseQueryParser {
         if (fieldName == null) {
             throw new QueryParsingException(parseContext, "terms query requires a field name, followed by array of terms or a document lookup specification");
         }
-        TermsQueryBuilder termsQueryBuilder;
+        return new TermsQueryBuilder(fieldName, values, minShouldMatch, disableCoord, termsLookup)
-        if (values == null) {
-            termsQueryBuilder = new TermsQueryBuilder(fieldName);
-        } else {
-            termsQueryBuilder = new TermsQueryBuilder(fieldName, values);
-        }
-        return termsQueryBuilder
-                .disableCoord(disableCoord)
-                .minimumShouldMatch(minShouldMatch)
-                .termsLookup(termsLookup)
                 .boost(boost)
                 .queryName(queryName);
     }

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

@@ -254,7 +254,7 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
         QB testQuery = createTestQueryBuilder();
         assertParsedQuery(testQuery.toString(), testQuery);
         for (Map.Entry<String, QB> alternateVersion : getAlternateVersions().entrySet()) {
-            assertParsedQuery(alternateVersion.getKey(), alternateVersion.getValue(), ParseFieldMatcher.EMPTY);
+            assertParsedQuery(alternateVersion.getKey(), alternateVersion.getValue());
         }
     }
 
@@ -270,7 +270,7 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
      * Parses the query provided as string argument and compares it with the expected result provided as argument as a {@link QueryBuilder}
      */
     protected void assertParsedQuery(String queryAsString, QueryBuilder<?> expectedQuery) throws IOException {
-        assertParsedQuery(queryAsString, expectedQuery, getDefaultParseFieldMatcher());
+        assertParsedQuery(queryAsString, expectedQuery, ParseFieldMatcher.STRICT);
     }
 
     protected void assertParsedQuery(String queryAsString, QueryBuilder<?> expectedQuery, ParseFieldMatcher matcher) throws IOException {
@@ -281,7 +281,7 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
     }
 
     protected QueryBuilder<?> parseQuery(String queryAsString) throws IOException {
-        return parseQuery(queryAsString, getDefaultParseFieldMatcher());
+        return parseQuery(queryAsString, ParseFieldMatcher.STRICT);
     }
 
     protected QueryBuilder<?> parseQuery(String queryAsString, ParseFieldMatcher matcher) throws IOException {
@@ -292,16 +292,6 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
         return context.parseInnerQueryBuilder();
     }
 
-    /**
-     * Returns the default {@link ParseFieldMatcher} used for parsing non-alternative XContent representations.
-     * The default is {@link ParseFieldMatcher#STRICT}.
-     * Note: Queries returned from {@link #getAlternateVersions()} are always parsed with {@link ParseFieldMatcher#EMPTY} as they might
-     * not be backwards compatible.
-     */
-    protected ParseFieldMatcher getDefaultParseFieldMatcher() {
-        return ParseFieldMatcher.STRICT;
-    }
-
     /**
      * Test creates the {@link Query} from the {@link QueryBuilder} under test and delegates the
      * assertions being made on the result to the implementing subclass.
@@ -384,6 +374,13 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
     @Test
     public void testSerialization() throws IOException {
         QB testQuery = createTestQueryBuilder();
+        assertSerialization(testQuery);
+    }
+
+    /**
+     * Serialize the given query builder and asserts that both are equal
+     */
+    protected QB assertSerialization(QB testQuery) throws IOException {
         try (BytesStreamOutput output = new BytesStreamOutput()) {
             testQuery.writeTo(output);
             try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) {
@@ -392,6 +389,7 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
                 assertEquals(deserializedQuery, testQuery);
                 assertEquals(deserializedQuery.hashCode(), testQuery.hashCode());
                 assertNotSame(deserializedQuery, testQuery);
+                return (QB) deserializedQuery;
             }
         }
     }

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

@@ -160,37 +160,35 @@ public class HasParentQueryBuilderTests extends AbstractQueryTestCase<HasParentQ
         builder.field("type", "foo"); // deprecated
         builder.endObject();
         builder.endObject();
-        String queryAsString = builder.string();
-        QueryShardContext shardContext = createShardContext();
-        QueryParseContext context = shardContext.parseContext();
-        XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(queryAsString);
-        context.reset(parser);
-        context.parseFieldMatcher(ParseFieldMatcher.STRICT);
         try {
-            context.parseInnerQueryBuilder();
+            parseQuery(builder.string());
             fail("type is deprecated");
         } catch (IllegalArgumentException ex) {
             assertEquals("Deprecated field [type] used, expected [parent_type] instead", ex.getMessage());
         }
 
+        HasParentQueryBuilder queryBuilder = (HasParentQueryBuilder) parseQuery(builder.string(), ParseFieldMatcher.EMPTY);
+        assertEquals("foo", queryBuilder.type());
+
+        boolean score = randomBoolean();
         String key = RandomPicks.randomFrom(random(), Arrays.asList("score_mode", "scoreMode", "score_type", "scoreType"));
         builder = XContentFactory.jsonBuilder().prettyPrint();
         builder.startObject();
         builder.startObject("has_parent");
         builder.field("query");
         EmptyQueryBuilder.PROTOTYPE.toXContent(builder, ToXContent.EMPTY_PARAMS);
-        builder.field(key, "score");
+        builder.field(key, score ? "score": "none");
+        builder.field("parent_type", "foo");
         builder.endObject();
         builder.endObject();
-        queryAsString = builder.string();
-        parser = XContentFactory.xContent(XContentType.JSON).createParser(queryAsString);
-        context.reset(parser);
-        context.parseFieldMatcher(ParseFieldMatcher.STRICT);
         try {
-            context.parseInnerQueryBuilder();
+            parseQuery(builder.string());
             fail(key + " is deprecated");
         } catch (IllegalArgumentException ex) {
             assertEquals("Deprecated field [" + key + "] used, replaced by [score]", ex.getMessage());
         }
+
+        queryBuilder = (HasParentQueryBuilder) parseQuery(builder.string(), ParseFieldMatcher.EMPTY);
+        assertEquals(score, queryBuilder.score());
     }
 }

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

@@ -23,6 +23,7 @@ import org.apache.lucene.search.BooleanClause;
 import org.apache.lucene.search.BooleanQuery;
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Query;
+import org.elasticsearch.common.ParseFieldMatcher;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -69,15 +70,6 @@ public class NotQueryBuilderTests extends AbstractQueryTestCase<NotQueryBuilder>
     @Override
     protected Map<String, NotQueryBuilder> getAlternateVersions() {
         Map<String, NotQueryBuilder> alternateVersions = new HashMap<>();
-
-        NotQueryBuilder testQuery1 = new NotQueryBuilder(createTestQueryBuilder().innerQuery());
-        String contentString1 = "{\n" +
-                "    \"not\" : {\n" +
-                "        \"filter\" : " + testQuery1.innerQuery().toString() + "\n" +
-                "    }\n" +
-                "}";
-        alternateVersions.put(contentString1, testQuery1);
-
         QueryBuilder innerQuery = createTestQueryBuilder().innerQuery();
         //not doesn't support empty query when query/filter element is not specified
         if (innerQuery != EmptyQueryBuilder.PROTOTYPE) {
@@ -90,6 +82,24 @@ public class NotQueryBuilderTests extends AbstractQueryTestCase<NotQueryBuilder>
         return alternateVersions;
     }
 
+
+    public void testDeprecatedXContent() throws IOException {
+        String deprecatedJson = "{\n" +
+                "    \"not\" : {\n" +
+                "        \"filter\" : " + EmptyQueryBuilder.PROTOTYPE.toString() + "\n" +
+                "    }\n" +
+                "}";
+        try {
+            parseQuery(deprecatedJson);
+            fail("filter is deprecated");
+        } catch (IllegalArgumentException ex) {
+            assertEquals("Deprecated field [filter] used, expected [query] instead", ex.getMessage());
+        }
+
+        NotQueryBuilder queryBuilder = (NotQueryBuilder) parseQuery(deprecatedJson, ParseFieldMatcher.EMPTY);
+        assertEquals(EmptyQueryBuilder.PROTOTYPE, queryBuilder.innerQuery());
+    }
+
     @Test
     public void testValidate() {
         QueryBuilder innerQuery = null;

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

@@ -35,7 +35,7 @@ import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.Matchers.*;
 
-public class QueryStringQueryBuilderTests extends BaseQueryTestCase<QueryStringQueryBuilder> {
+public class QueryStringQueryBuilderTests extends AbstractQueryTestCase<QueryStringQueryBuilder> {
 
     @Override
     protected QueryStringQueryBuilder doCreateTestQueryBuilder() {

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

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.index.query;
 
+import com.carrotsearch.randomizedtesting.generators.RandomPicks;
 import org.apache.lucene.index.Term;
 import org.apache.lucene.search.BooleanClause;
 import org.apache.lucene.search.BooleanQuery;
@@ -34,6 +35,7 @@ import org.junit.Test;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
 
@@ -49,11 +51,6 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
         queryParserService().setTermsLookupFetchService(termsLookupFetchService);
     }
 
-    @Override
-    protected ParseFieldMatcher getDefaultParseFieldMatcher() {
-        return ParseFieldMatcher.EMPTY;
-    }
-
     @Override
     protected TermsQueryBuilder doCreateTestQueryBuilder() {
         TermsQueryBuilder query;
@@ -71,12 +68,6 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
             query = new TermsQueryBuilder(randomBoolean() ? randomAsciiOfLengthBetween(1,10) : STRING_FIELD_NAME);
             query.termsLookup(randomTermsLookup());
         }
-        if (randomBoolean()) {
-            query.minimumShouldMatch(randomInt(100) + "%");
-        }
-        if (randomBoolean()) {
-            query.disableCoord(randomBoolean());
-        }
         return query;
     }
 
@@ -207,6 +198,50 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
         assertThat(termsQueryBuilder.validate().validationErrors().size(), is(1));
     }
 
+    public void testDeprecatedXContent() throws IOException {
+        String query = "{\n" +
+                "  \"terms\": {\n" +
+                "    \"field\": [\n" +
+                "      \"blue\",\n" +
+                "      \"pill\"\n" +
+                "    ],\n" +
+                "    \"disable_coord\": true\n" +
+                "  }\n" +
+                "}";
+        try {
+            parseQuery(query);
+            fail("disable_coord is deprecated");
+        } catch (IllegalArgumentException ex) {
+            assertEquals("Deprecated field [disable_coord] used, replaced by [Use [bool] query instead]", ex.getMessage());
+        }
+
+        TermsQueryBuilder queryBuilder = (TermsQueryBuilder) parseQuery(query, ParseFieldMatcher.EMPTY);
+        TermsQueryBuilder copy = assertSerialization(queryBuilder);
+        assertTrue(queryBuilder.disableCoord());
+        assertTrue(copy.disableCoord());
+
+        String randomMinShouldMatch = RandomPicks.randomFrom(random(), Arrays.asList("min_match", "min_should_match", "minimum_should_match"));
+        query = "{\n" +
+                "  \"terms\": {\n" +
+                "    \"field\": [\n" +
+                "      \"blue\",\n" +
+                "      \"pill\"\n" +
+                "    ],\n" +
+                "    \"" + randomMinShouldMatch +"\": \"42%\"\n" +
+                "  }\n" +
+                "}";
+        try {
+            parseQuery(query);
+            fail(randomMinShouldMatch + " is deprecated");
+        } catch (IllegalArgumentException ex) {
+            assertEquals("Deprecated field [" + randomMinShouldMatch + "] used, replaced by [Use [bool] query instead]", ex.getMessage());
+        }
+        queryBuilder = (TermsQueryBuilder) parseQuery(query, ParseFieldMatcher.EMPTY);
+        copy = assertSerialization(queryBuilder);
+        assertEquals("42%", queryBuilder.minimumShouldMatch());
+        assertEquals("42%", copy.minimumShouldMatch());
+    }
+
     private static class MockTermsLookupFetchService extends TermsLookupFetchService {
 
         private List<Object> randomTerms = new ArrayList<>();