Browse Source

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

`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
Adrien Grand 6 years ago
parent
commit
62f0895424

+ 2 - 3
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>
         }

+ 18 - 9
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 class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
         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 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
                     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 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
             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 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
         // 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 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
         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 + "]");

+ 42 - 15
modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java

@@ -29,6 +29,7 @@ import org.elasticsearch.common.bytes.BytesArray;
 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 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
         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 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
         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 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
     @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 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
     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 class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
         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 class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
         });
         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 class PercolateQueryBuilderTests extends AbstractQueryTestCase<PercolateQ
         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 + "\"}}"));

+ 44 - 0
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  }

+ 96 - 0
modules/percolator/src/test/resources/rest-api-spec/test/11_basic_with_types.yml

@@ -0,0 +1,96 @@
+---
+"Test percolator basics via rest":
+
+  - do:
+      indices.create:
+        include_type_name: true
+        index: queries_index
+        body:
+          mappings:
+            queries_type:
+              properties:
+                query:
+                  type: percolator
+                foo:
+                  type: keyword
+
+  - do:
+      indices.create:
+        include_type_name: true
+        index: documents_index
+        body:
+          mappings:
+            documents_type:
+              properties:
+                foo:
+                  type: keyword
+
+  - do:
+      index:
+        index: queries_index
+        type: queries_type
+        id:   test_percolator
+        body:
+          query:
+            match_all: {}
+
+  - do:
+      index:
+        index: documents_index
+        type: documents_type
+        id: some_id
+        body:
+          foo: bar
+
+  - do:
+        indices.refresh: {}
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body:
+          - query:
+              percolate:
+                field: query
+                document:
+                  document_type: queries_type
+                  foo: bar
+  - match:  { hits.total:     1  }
+
+  - do:
+      msearch:
+        rest_total_hits_as_int: true
+        body:
+          - index: queries_index
+          - query:
+              percolate:
+                field: query
+                document_type: queries_type
+                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
+                type: documents_type
+                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
+                type: documents_type
+                id: some_id
+  - match:  { responses.0.hits.total:     1  }