Emit deprecation warning when TermsLookup contains a type (#53731)

TermsLookup in master no longer accepts a type parameter. We should emit
a deprecate warning in 7.x when a terms lookup requests includes type to prepare
users for its removal.

Relates to #41059

Author: romseygeek

rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/51_filter_with_types.yml

@@ -35,8 +35,11 @@ setup:
 ---
 "Filter aggs with terms lookup and ensure it's cached":
   # Because the filter agg rewrites the terms lookup in the rewrite phase the request can be cached
-
+  - skip:
+      features: allowed_warnings
   - do:
+      allowed_warnings:
+        - "Deprecated field [type] used, this field is unused and will be removed entirely"
       search:
         rest_total_hits_as_int: true
         size: 0

rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml

@@ -1,4 +1,6 @@
 "Ensure that we fetch the document only once":
+  - skip:
+      features: allowed_warnings
   - do:
       indices.create:
           index: search_index
@@ -31,11 +33,13 @@
       indices.refresh: {}
 
   - do:
+      allowed_warnings:
+        - "Deprecated field [type] used, this field is unused and will be removed entirely"
       catch: /no such index/
       search:
         rest_total_hits_as_int: true
         index: "search_index"
-        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type": "_doc", "id": "1", "path": "followers"} } } }
+        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
   - do:
       indices.create:
           index: lookup_index
@@ -55,10 +59,12 @@
       indices.refresh: {}
 
   - do:
+      allowed_warnings:
+        - "Deprecated field [type] used, this field is unused and will be removed entirely"
       search:
         rest_total_hits_as_int: true
         index: "search_index"
-        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type": "_doc", "id": "1", "path": "followers"} } } }
+        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
 
   - match: { _shards.total: 5 }
   - match: { _shards.successful: 5 }

rest-api-spec/src/main/resources/rest-api-spec/test/search/171_terms_query_with_types.yml

@@ -3,6 +3,7 @@
   - skip:
       version: " - 6.99.99"
       reason: index.max_terms_count setting has been added in 7.0.0
+      features: allowed_warnings
   - do:
       indices.create:
           include_type_name: true
@@ -46,6 +47,8 @@
           body: {"query" : {"terms" : {"user" : ["u1", "u2", "u3"]}}}
 
   - do:
+      allowed_warnings:
+        - "Deprecated field [type] used, this field is unused and will be removed entirely"
       search:
           rest_total_hits_as_int: true
           index: test_index

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

@@ -19,7 +19,6 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.logging.log4j.LogManager;
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.Query;
@@ -35,13 +34,12 @@
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.support.XContentMapValues;
 import org.elasticsearch.index.IndexSettings;
-import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.ConstantFieldType;
+import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.indices.TermsLookup;
 
 import java.io.IOException;
@@ -64,11 +62,6 @@
 public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
     public static final String NAME = "terms";
 
-    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(
-        LogManager.getLogger(TermsQueryBuilder.class));
-    static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Types are deprecated " +
-        "in [terms] lookup queries.";
-
     private final String fieldName;
     private final List<?> values;
     private final TermsLookup termsLookup;
@@ -402,15 +395,9 @@ public static TermsQueryBuilder fromXContent(XContentParser parser) throws IOExc
                     "followed by array of terms or a document lookup specification");
         }
 
-        TermsQueryBuilder builder = new TermsQueryBuilder(fieldName, values, termsLookup)
+        return new TermsQueryBuilder(fieldName, values, termsLookup)
             .boost(boost)
             .queryName(queryName);
-
-        if (builder.isTypeless() == false) {
-            deprecationLogger.deprecatedAndMaybeLog("terms_lookup_with_types", TYPES_DEPRECATION_MESSAGE);
-        }
-
-        return builder;
     }
 
     static List<Object> parseValues(XContentParser parser) throws IOException {

server/src/main/java/org/elasticsearch/indices/TermsLookup.java

@@ -21,10 +21,11 @@
 
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Nullable;
-import org.elasticsearch.common.ParsingException;
+import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -33,10 +34,14 @@
 import java.io.IOException;
 import java.util.Objects;
 
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+
 /**
  * Encapsulates the parameters needed to fetch terms.
  */
 public class TermsLookup implements Writeable, ToXContentFragment {
+
     private final String index;
     private @Nullable String type;
     private final String id;
@@ -142,48 +147,24 @@ public TermsLookup routing(String routing) {
         return this;
     }
 
+    private static final ConstructingObjectParser<TermsLookup, Void> PARSER = new ConstructingObjectParser<>("terms_lookup",
+        args -> {
+            String index = (String) args[0];
+            String type = (String) args[1];
+            String id = (String) args[2];
+            String path = (String) args[3];
+            return new TermsLookup(index, type, id, path);
+        });
+    static {
+        PARSER.declareString(constructorArg(), new ParseField("index"));
+        PARSER.declareString(optionalConstructorArg(), new ParseField("type").withAllDeprecated());
+        PARSER.declareString(constructorArg(), new ParseField("id"));
+        PARSER.declareString(constructorArg(), new ParseField("path"));
+        PARSER.declareString(TermsLookup::routing, new ParseField("routing"));
+    }
+
     public static TermsLookup parseTermsLookup(XContentParser parser) throws IOException {
-        String index = null;
-        String type = null;
-        String id = null;
-        String path = null;
-        String routing = null;
-        XContentParser.Token token;
-        String currentFieldName = "";
-        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
-            if (token == XContentParser.Token.FIELD_NAME) {
-                currentFieldName = parser.currentName();
-            } else if (token.isValue()) {
-                switch (currentFieldName) {
-                case "index":
-                    index = parser.text();
-                    break;
-                case "type":
-                    type = parser.text();
-                    break;
-                case "id":
-                    id = parser.text();
-                    break;
-                case "routing":
-                    routing = parser.textOrNull();
-                    break;
-                case "path":
-                    path = parser.text();
-                    break;
-                default:
-                    throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME +
-                        "] query does not support [" + currentFieldName + "] within lookup element");
-                }
-            } else {
-                throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME + "] unknown token ["
-                    + token + "] after [" + currentFieldName + "]");
-            }
-        }
-        if (type == null) {
-            return new TermsLookup(index, id, path).routing(routing);
-        } else {
-            return new TermsLookup(index, type, id, path).routing(routing);
-        }
+        return PARSER.parse(parser, null);
     }
 
     @Override

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

@@ -57,6 +57,7 @@
 public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuilder> {
     private List<Object> randomTerms;
     private String termsPath;
+    private boolean maybeIncludeType = true;
 
     @Before
     public void randomTerms() {
@@ -100,10 +101,10 @@ protected TermsQueryBuilder doCreateTestQueryBuilder() {
 
     private TermsLookup randomTermsLookup() {
         // Randomly choose between a typeless terms lookup and one with an explicit type to make sure we are
+        TermsLookup lookup = maybeIncludeType && randomBoolean()
+            ? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
+            : new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
         // testing both cases.
-        TermsLookup lookup = randomBoolean()
-            ? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
-            : new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
         lookup.routing(randomBoolean() ? randomAlphaOfLength(10) : null);
         return lookup;
     }
@@ -149,7 +150,13 @@ protected void doAssertLuceneQuery(TermsQueryBuilder queryBuilder, Query query,
         }
     }
 
-    public void testEmtpyFieldName() {
+    @Override
+    public void testUnknownField() throws IOException {
+        maybeIncludeType = false;   // deprecation warnings will fail the parent test, so we disable types
+        super.testUnknownField();
+    }
+
+    public void testEmptyFieldName() {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new TermsQueryBuilder(null, "term"));
         assertEquals("field name cannot be null.", e.getMessage());
         e = expectThrows(IllegalArgumentException.class, () -> new TermsQueryBuilder("", "term"));
@@ -326,32 +333,36 @@ public void testTypeField() throws IOException {
         builder.doToQuery(createShardContext());
         assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE);
     }
-    
+
     public void testRewriteIndexQueryToMatchNone() throws IOException {
         TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", "also_does_not_exist");
         QueryShardContext queryShardContext = createShardContext();
         QueryBuilder rewritten = query.rewrite(queryShardContext);
         assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
-    }      
-    
+    }
+
     public void testRewriteIndexQueryToNotMatchNone() throws IOException {
         // At least one name is good
         TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", getIndex().getName());
         QueryShardContext queryShardContext = createShardContext();
         QueryBuilder rewritten = query.rewrite(queryShardContext);
         assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
-    }      
-    
+    }
+
     @Override
     protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
-        QueryBuilder query = super.parseQuery(parser);
-        assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));
+        try {
+            QueryBuilder query = super.parseQuery(parser);
+            assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));
+
+            TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
+            if (termsQuery.isTypeless() == false) {
+                assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely");
+            }
+            return query;
+        } finally {
 
-        TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
-        if (termsQuery.isTypeless() == false) {
-            assertWarnings(TermsQueryBuilder.TYPES_DEPRECATION_MESSAGE);
         }
-        return query;
     }
 
 }

server/src/test/java/org/elasticsearch/indices/TermsLookupTests.java

@@ -22,6 +22,8 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
@@ -105,6 +107,32 @@ public void testSerializationWithTypes() throws IOException {
         }
     }
 
+    public void testXContentParsingWithType() throws IOException {
+        XContentParser parser = createParser(JsonXContent.jsonXContent,
+            "{ \"index\" : \"index\", \"id\" : \"id\", \"type\" : \"type\", \"path\" : \"path\", \"routing\" : \"routing\" }");
+
+        TermsLookup tl = TermsLookup.parseTermsLookup(parser);
+        assertEquals("index", tl.index());
+        assertEquals("type", tl.type());
+        assertEquals("id", tl.id());
+        assertEquals("path", tl.path());
+        assertEquals("routing", tl.routing());
+
+        assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely");
+    }
+
+    public void testXContentParsing() throws IOException {
+        XContentParser parser = createParser(JsonXContent.jsonXContent,
+            "{ \"index\" : \"index\", \"id\" : \"id\", \"path\" : \"path\", \"routing\" : \"routing\" }");
+
+        TermsLookup tl = TermsLookup.parseTermsLookup(parser);
+        assertEquals("index", tl.index());
+        assertNull(tl.type());
+        assertEquals("id", tl.id());
+        assertEquals("path", tl.path());
+        assertEquals("routing", tl.routing());
+    }
+
     public static TermsLookup randomTermsLookup() {
         return new TermsLookup(
             randomAlphaOfLength(10),