Make the type parameter optional when percolating existing documents. (#39987) (#39989)

jpountz · web-flow · commit 9731ba4338f7 · 2019-03-13T15:04:41.000+01:00
`document_type` is the type to use for parsing the document to percolate, which is already optional and deprecated. However `percotale` queries also have the ability to percolate existing documents, identified by an index, an id and a type. This change makes the latter optional and deprecated. Closes #39963
diff --git a/docs/reference/query-dsl/percolate-query.asciidoc b/docs/reference/query-dsl/percolate-query.asciidoc
@@ -134,7 +134,7 @@ The following parameters are required when percolating a document:
          This is an optional parameter.
 `document`:: The source of the document being percolated.
 `documents`:: Like the `document` parameter, but accepts multiple documents via a json array.
-`document_type`:: The type / mapping of the document being percolated. This setting is deprecated and only required for indices created before 6.0
+`document_type`:: The type / mapping of the document being percolated. This parameter is deprecated and will be removed in Elasticsearch 8.0.
 
 Instead of specifying the source of the document being percolated, the source can also be retrieved from an already
 stored document. The `percolate` query will then internally execute a get request to fetch that document.
@@ -143,7 +143,7 @@ In that case the `document` parameter can be substituted with the following para
 
 [horizontal]
 `index`:: The index the document resides in. This is a required parameter.
-`type`:: The type of the document to fetch. This is a required parameter.
+`type`:: The type of the document to fetch. This parameter is deprecated and will be removed in Elasticsearch 8.0.
 `id`:: The id of the document to fetch. This is a required parameter.
 `routing`:: Optionally, routing to be used to fetch document to percolate.
 `preference`:: Optionally, preference to be used to fetch document to percolate.
@@ -323,7 +323,6 @@ GET /my-index/_search
         "percolate" : {
             "field": "query",
             "index" : "my-index",
-            "type" : "_doc",
             "id" : "2",
             "version" : 1 <1>
         }
diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
@@ -96,6 +96,10 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
     public static final String NAME = "percolate";
 
     private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(ParseField.class));
+    static final String DOCUMENT_TYPE_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [percolate] queries. " +
+            "The [document_type] should no longer be specified.";
+    static final String TYPE_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [percolate] queries. " +
+            "The [type] of the indexed document should no longer be specified.";
 
     static final ParseField DOCUMENT_FIELD = new ParseField("document");
     static final ParseField DOCUMENTS_FIELD = new ParseField("documents");
@@ -117,6 +121,7 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
     private final XContentType documentXContentType;
 
     private final String indexedDocumentIndex;
+    @Deprecated
     private final String indexedDocumentType;
     private final String indexedDocumentId;
     private final String indexedDocumentRouting;
@@ -220,9 +225,6 @@ public PercolateQueryBuilder(String field, String documentType, String indexedDo
         if (indexedDocumentIndex == null) {
             throw new IllegalArgumentException("[index] is a required argument");
         }
-        if (indexedDocumentType == null) {
-            throw new IllegalArgumentException("[type] is a required argument");
-        }
         if (indexedDocumentId == null) {
             throw new IllegalArgumentException("[id] is a required argument");
         }
@@ -521,7 +523,13 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) {
                     XContentHelper.xContentType(source));
             }
         }
-        GetRequest getRequest = new GetRequest(indexedDocumentIndex, indexedDocumentType, indexedDocumentId);
+        GetRequest getRequest;
+        if (indexedDocumentType != null) {
+            deprecationLogger.deprecatedAndMaybeLog("percolate_with_type", TYPE_DEPRECATION_MESSAGE);
+            getRequest = new GetRequest(indexedDocumentIndex, indexedDocumentType, indexedDocumentId);
+        } else {
+            getRequest = new GetRequest(indexedDocumentIndex, indexedDocumentId);
+        }
         getRequest.preference("_local");
         getRequest.routing(indexedDocumentRouting);
         getRequest.preference(indexedDocumentPreference);
@@ -533,13 +541,14 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) {
             client.get(getRequest, ActionListener.wrap(getResponse -> {
                 if (getResponse.isExists() == false) {
                     throw new ResourceNotFoundException(
-                        "indexed document [{}/{}/{}] couldn't be found", indexedDocumentIndex, indexedDocumentType, indexedDocumentId
+                        "indexed document [{}{}/{}] couldn't be found", indexedDocumentIndex,
+                        indexedDocumentType == null ? "" : "/" + indexedDocumentType, indexedDocumentId
                     );
                 }
                 if(getResponse.isSourceEmpty()) {
                     throw new IllegalArgumentException(
-                        "indexed document [" + indexedDocumentIndex + "/" + indexedDocumentType + "/" + indexedDocumentId
-                            + "] source disabled"
+                        "indexed document [" + indexedDocumentIndex + (indexedDocumentType == null ? "" : "/" + indexedDocumentType) +
+                        "/" + indexedDocumentId + "] source disabled"
                     );
                 }
                 documentSupplier.set(getResponse.getSourceAsBytesRef());
@@ -554,7 +563,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
         // Call nowInMillis() so that this query becomes un-cacheable since we
         // can't be sure that it doesn't use now or scripts
         context.nowInMillis();
-        if (indexedDocumentIndex != null || indexedDocumentType != null || indexedDocumentId != null || documentSupplier != null) {
+        if (indexedDocumentIndex != null || indexedDocumentId != null || documentSupplier != null) {
             throw new IllegalStateException("query builder must be rewritten first");
         }
 
@@ -577,7 +586,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
         final MapperService mapperService = context.getMapperService();
         String type = mapperService.documentMapper().type();
         if (documentType != null) {
-            deprecationLogger.deprecated("[document_type] parameter has been deprecated because types have been deprecated");
+            deprecationLogger.deprecatedAndMaybeLog("percolate_with_document_type", DOCUMENT_TYPE_DEPRECATION_MESSAGE);
             if (documentType.equals(type) == false) {
                 throw new IllegalArgumentException("specified document_type [" + documentType +
                     "] is not equal to the actual type [" + type + "]");
diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.lucene.uid.Versions;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentType;
@@ -69,7 +70,6 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
     private static String docType;
 
     private String indexedDocumentIndex;
-    private String indexedDocumentType;
     private String indexedDocumentId;
     private String indexedDocumentRouting;
     private String indexedDocumentPreference;
@@ -88,7 +88,7 @@ protected void initializeAdditionalMappings(MapperService mapperService) throws
         queryField = randomAlphaOfLength(4);
         aliasField = randomAlphaOfLength(4);
 
-        String docType = "_doc";
+        docType = "_doc";
         mapperService.merge(docType, new CompressedXContent(Strings.toString(PutMappingRequest.buildFromSimplifiedDef(docType,
                 queryField, "type=percolator", aliasField, "type=alias,path=" + queryField
         ))), MapperService.MergeReason.MAPPING_UPDATE);
@@ -117,15 +117,14 @@ private PercolateQueryBuilder doCreateTestQueryBuilder(boolean indexedDocument)
         PercolateQueryBuilder queryBuilder;
         if (indexedDocument) {
             indexedDocumentIndex = randomAlphaOfLength(4);
-            indexedDocumentType = "doc";
             indexedDocumentId = randomAlphaOfLength(4);
             indexedDocumentRouting = randomAlphaOfLength(4);
             indexedDocumentPreference = randomAlphaOfLength(4);
             indexedDocumentVersion = (long) randomIntBetween(0, Integer.MAX_VALUE);
-            queryBuilder = new PercolateQueryBuilder(queryField, docType, indexedDocumentIndex, indexedDocumentType, indexedDocumentId,
+            queryBuilder = new PercolateQueryBuilder(queryField, null, indexedDocumentIndex, null, indexedDocumentId,
                     indexedDocumentRouting, indexedDocumentPreference, indexedDocumentVersion);
         } else {
-            queryBuilder = new PercolateQueryBuilder(queryField, docType, documentSource, XContentType.JSON);
+            queryBuilder = new PercolateQueryBuilder(queryField, null, documentSource, XContentType.JSON);
         }
         if (randomBoolean()) {
             queryBuilder.setName(randomAlphaOfLength(4));
@@ -146,19 +145,19 @@ protected String[] shuffleProtectedFields() {
     @Override
     protected GetResponse executeGet(GetRequest getRequest) {
         assertThat(getRequest.index(), Matchers.equalTo(indexedDocumentIndex));
-        assertThat(getRequest.type(), Matchers.equalTo(indexedDocumentType));
+        assertThat(getRequest.type(), Matchers.equalTo(MapperService.SINGLE_MAPPING_NAME));
         assertThat(getRequest.id(), Matchers.equalTo(indexedDocumentId));
         assertThat(getRequest.routing(), Matchers.equalTo(indexedDocumentRouting));
         assertThat(getRequest.preference(), Matchers.equalTo(indexedDocumentPreference));
         assertThat(getRequest.version(), Matchers.equalTo(indexedDocumentVersion));
         if (indexedDocumentExists) {
             return new GetResponse(
-                    new GetResult(indexedDocumentIndex, indexedDocumentType, indexedDocumentId, 0, 1, 0L, true,
+                    new GetResult(indexedDocumentIndex, MapperService.SINGLE_MAPPING_NAME, indexedDocumentId, 0, 1, 0L, true,
                             documentSource.iterator().next(), Collections.emptyMap())
             );
         } else {
             return new GetResponse(
-                    new GetResult(indexedDocumentIndex, indexedDocumentType, indexedDocumentId, UNASSIGNED_SEQ_NO, 0, -1,
+                    new GetResult(indexedDocumentIndex, MapperService.SINGLE_MAPPING_NAME, indexedDocumentId, UNASSIGNED_SEQ_NO, 0, -1,
                         false, null, Collections.emptyMap())
             );
         }
@@ -168,7 +167,7 @@ protected GetResponse executeGet(GetRequest getRequest) {
     protected void doAssertLuceneQuery(PercolateQueryBuilder queryBuilder, Query query, SearchContext context) throws IOException {
         assertThat(query, Matchers.instanceOf(PercolateQuery.class));
         PercolateQuery percolateQuery = (PercolateQuery) query;
-        assertThat(docType, Matchers.equalTo(queryBuilder.getDocumentType()));
+        assertNull(queryBuilder.getDocumentType());
         assertThat(percolateQuery.getDocuments(), Matchers.equalTo(documentSource));
     }
 
@@ -188,7 +187,7 @@ public void testIndexedDocumentDoesNotExist() throws IOException {
         PercolateQueryBuilder pqb = doCreateTestQueryBuilder(true);
         ResourceNotFoundException e = expectThrows(ResourceNotFoundException.class, () -> rewriteAndFetch(pqb,
             createShardContext()));
-        String expectedString = "indexed document [" + indexedDocumentIndex + "/" + indexedDocumentType + "/" +
+        String expectedString = "indexed document [" + indexedDocumentIndex + "/" +
                 indexedDocumentId +  "] couldn't be found";
         assertThat(e.getMessage() , equalTo(expectedString));
     }
@@ -220,11 +219,6 @@ public void testRequiredParameters() {
         });
         assertThat(e.getMessage(), equalTo("[index] is a required argument"));
 
-        e = expectThrows(IllegalArgumentException.class, () -> {
-            new PercolateQueryBuilder("_field", "_document_type", "_index", null, "_id", null, null, null);
-        });
-        assertThat(e.getMessage(), equalTo("[type] is a required argument"));
-
         e = expectThrows(IllegalArgumentException.class, () -> {
             new PercolateQueryBuilder("_field", "_document_type", "_index", "_type", null, null, null, null);
         });
@@ -237,6 +231,39 @@ public void testFromJsonNoDocumentType() throws IOException {
         queryBuilder.toQuery(queryShardContext);
     }
 
+    public void testFromJsonWithDocumentType() throws IOException {
+        QueryShardContext queryShardContext = createShardContext();
+        QueryBuilder queryBuilder = parseQuery("{\"percolate\" : { \"document\": {}, \"document_type\":\"" + docType  + "\", \"field\":\"" +
+                queryField + "\"}}");
+        queryBuilder.toQuery(queryShardContext);
+        assertWarnings(PercolateQueryBuilder.DOCUMENT_TYPE_DEPRECATION_MESSAGE);
+    }
+
+    public void testFromJsonNoType() throws IOException {
+        indexedDocumentIndex = randomAlphaOfLength(4);
+        indexedDocumentId = randomAlphaOfLength(4);
+        indexedDocumentVersion = Versions.MATCH_ANY;
+        documentSource = Collections.singletonList(randomSource(new HashSet<>()));
+
+        QueryShardContext queryShardContext = createShardContext();
+        QueryBuilder queryBuilder =  parseQuery("{\"percolate\" : { \"index\": \"" + indexedDocumentIndex + "\", \"id\": \"" +
+                indexedDocumentId + "\", \"field\":\"" + queryField + "\"}}");
+        rewriteAndFetch(queryBuilder, queryShardContext).toQuery(queryShardContext);
+    }
+
+    public void testFromJsonWithType() throws IOException {
+        indexedDocumentIndex = randomAlphaOfLength(4);
+        indexedDocumentId = randomAlphaOfLength(4);
+        indexedDocumentVersion = Versions.MATCH_ANY;
+        documentSource = Collections.singletonList(randomSource(new HashSet<>()));
+
+        QueryShardContext queryShardContext = createShardContext();
+        QueryBuilder queryBuilder =  parseQuery("{\"percolate\" : { \"index\": \"" + indexedDocumentIndex +
+                "\", \"type\": \"_doc\", \"id\": \"" + indexedDocumentId + "\", \"field\":\"" + queryField + "\"}}");
+        rewriteAndFetch(queryBuilder, queryShardContext).toQuery(queryShardContext);
+        assertWarnings(PercolateQueryBuilder.TYPE_DEPRECATION_MESSAGE);
+    }
+
     public void testBothDocumentAndDocumentsSpecified() throws IOException {
         expectThrows(IllegalArgumentException.class,
             () -> parseQuery("{\"percolate\" : { \"document\": {}, \"documents\": [{}, {}], \"field\":\"" + queryField + "\"}}"));
diff --git a/modules/percolator/src/test/resources/rest-api-spec/test/10_basic.yml b/modules/percolator/src/test/resources/rest-api-spec/test/10_basic.yml
@@ -1,5 +1,10 @@
 ---
 "Test percolator basics via rest":
+
+  - skip:
+      version: " - 6.99.99"
+      reason: types are required in requests before 7.0.0
+
   - do:
       indices.create:
         index: queries_index
@@ -11,6 +16,15 @@
               foo:
                 type: keyword
 
+  - do:
+      indices.create:
+        index: documents_index
+        body:
+          mappings:
+            properties:
+              foo:
+                type: keyword
+
   - do:
       index:
         index: queries_index
@@ -19,6 +33,13 @@
           query:
             match_all: {}
 
+  - do:
+      index:
+        index: documents_index
+        id: some_id
+        body:
+          foo: bar
+
   - do:
         indices.refresh: {}
 
@@ -44,3 +65,26 @@
                 document:
                   foo: bar
   - match:  { responses.0.hits.total:     1  }
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body:
+          - query:
+              percolate:
+                field: query
+                index: documents_index
+                id: some_id 
+  - match:  { hits.total:     1  }
+
+  - do:
+      msearch:
+        rest_total_hits_as_int: true
+        body:
+          - index: queries_index
+          - query:
+              percolate:
+                field: query
+                index: documents_index
+                id: some_id
+  - match:  { responses.0.hits.total:     1  }
diff --git a/modules/percolator/src/test/resources/rest-api-spec/test/11_basic_with_types.yml b/modules/percolator/src/test/resources/rest-api-spec/test/11_basic_with_types.yml
