Add POST /_search/clear_scroll endpoint and deprecate delete scroll endpoint

core/src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java

@@ -37,6 +37,14 @@ public class ClearScrollRequest extends ActionRequest {
 
     private List<String> scrollIds;
 
+    public ClearScrollRequest() {
+
+    }
+
+    public ClearScrollRequest(List<String> scrollIds) {
+        this.scrollIds = scrollIds;
+    }
+
     public List<String> getScrollIds() {
         return scrollIds;
     }

core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java

@@ -20,7 +20,6 @@
 package org.elasticsearch.rest.action.search;
 
 import org.elasticsearch.action.search.ClearScrollRequest;
-import org.elasticsearch.action.search.ClearScrollResponse;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesReference;
@@ -37,47 +36,61 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 
 import static org.elasticsearch.rest.RestRequest.Method.DELETE;
+import static org.elasticsearch.rest.RestRequest.Method.POST;
 
 public class RestClearScrollAction extends BaseRestHandler {
 
     @Inject
     public RestClearScrollAction(Settings settings, RestController controller) {
         super(settings);
-
+        //the DELETE endpoints are both deprecated
         controller.registerHandler(DELETE, "/_search/scroll", this);
         controller.registerHandler(DELETE, "/_search/scroll/{scroll_id}", this);
+        controller.registerHandler(POST, "/_search/clear_scroll", this);
+        //the POST endpoint that accepts scroll_id in the url is deprecated too
+        controller.registerHandler(POST, "/_search/clear_scroll/{scroll_id}", this);
     }
 
     @Override
     public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-        String scrollIds = request.param("scroll_id");
-        ClearScrollRequest clearRequest = new ClearScrollRequest();
-        clearRequest.setScrollIds(Arrays.asList(splitScrollIds(scrollIds)));
+        if (request.method() == DELETE) {
+            deprecationLogger.deprecated("Deprecated [DELETE /_search/scroll] endpoint used, expected [POST /_search/scroll] instead");
+        }
+        if (request.method() == POST && request.hasParam("scroll_id")) {
+            deprecationLogger.deprecated("Deprecated [POST /_search/clear_scroll/{scroll_id}] endpoint used, " +
+                    "expected [POST /_search/clear_scroll] instead. The scroll_id should rather be provided as part of the request body");
+        }
+        final ClearScrollRequest clearRequest;
         if (RestActions.hasBodyContent(request)) {
+            //consume the scroll_id param, it will get ignored though (bw comp)
+            request.param("scroll_id");
             XContentType type = RestActions.guessBodyContentType(request);
-           if (type == null) {
-               scrollIds = RestActions.getRestContent(request).utf8ToString();
-               clearRequest.setScrollIds(Arrays.asList(splitScrollIds(scrollIds)));
-           } else {
-               // NOTE: if rest request with xcontent body has request parameters, these parameters does not override xcontent value
-               clearRequest.setScrollIds(null);
-               buildFromContent(RestActions.getRestContent(request), clearRequest);
-           }
+            if (type == null) {
+                String scrollIds = RestActions.getRestContent(request).utf8ToString();
+                clearRequest = new ClearScrollRequest(splitScrollIds(scrollIds));
+            } else {
+                clearRequest = parseClearScrollRequest(RestActions.getRestContent(request));
+            }
+        } else {
+            clearRequest = new ClearScrollRequest(splitScrollIds(request.param("scroll_id")));
         }
 
-        return channel -> client.clearScroll(clearRequest, new RestStatusToXContentListener<ClearScrollResponse>(channel));
+        return channel -> client.clearScroll(clearRequest, new RestStatusToXContentListener<>(channel));
     }
 
-    public static String[] splitScrollIds(String scrollIds) {
+    private static List<String> splitScrollIds(String scrollIds) {
         if (scrollIds == null) {
-            return Strings.EMPTY_ARRAY;
+            return Collections.emptyList();
         }
-        return Strings.splitStringByCommaToArray(scrollIds);
+        return Arrays.asList(Strings.splitStringByCommaToArray(scrollIds));
     }
 
-    public static void buildFromContent(BytesReference content, ClearScrollRequest clearScrollRequest) {
+    public static ClearScrollRequest parseClearScrollRequest(BytesReference content) {
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
         try (XContentParser parser = XContentHelper.createParser(content)) {
             if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
                 throw new IllegalArgumentException("Malformed content, must start with an object");
@@ -103,6 +116,6 @@ public static void buildFromContent(BytesReference content, ClearScrollRequest c
         } catch (IOException e) {
             throw new IllegalArgumentException("Failed to parse request body", e);
         }
+        return clearScrollRequest;
     }
-
 }

core/src/test/java/org/elasticsearch/search/scroll/SearchScrollIT.java

@@ -545,16 +545,13 @@ public void testParseClearScrollRequest() throws Exception {
         BytesReference content = XContentFactory.jsonBuilder().startObject()
             .array("scroll_id", "value_1", "value_2")
             .endObject().bytes();
-        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
-        RestClearScrollAction.buildFromContent(content, clearScrollRequest);
+        ClearScrollRequest clearScrollRequest = RestClearScrollAction.parseClearScrollRequest(content);
         assertThat(clearScrollRequest.scrollIds(), contains("value_1", "value_2"));
     }
 
     public void testParseClearScrollRequestWithInvalidJsonThrowsException() throws Exception {
-        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
-
         try {
-            RestClearScrollAction.buildFromContent(new BytesArray("{invalid_json}"), clearScrollRequest);
+            RestClearScrollAction.parseClearScrollRequest(new BytesArray("{invalid_json}"));
             fail("expected parseContent failure");
         } catch (Exception e) {
             assertThat(e, instanceOf(IllegalArgumentException.class));
@@ -567,10 +564,8 @@ public void testParseClearScrollRequestWithUnknownParamThrowsException() throws
             .array("scroll_id", "value_1", "value_2")
             .field("unknown", "keyword")
             .endObject().bytes();
-        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
-
         try {
-            RestClearScrollAction.buildFromContent(invalidContent, clearScrollRequest);
+            RestClearScrollAction.parseClearScrollRequest(invalidContent);
             fail("expected parseContent failure");
         } catch (Exception e) {
             assertThat(e, instanceOf(IllegalArgumentException.class));

docs/reference/search/request/scroll.asciidoc

@@ -134,15 +134,15 @@ GET /_nodes/stats/indices/search
 
 ==== Clear scroll API
 
-Search context are automatically removed when the `scroll` timeout has been
+Search contexts are automatically removed when the `scroll` timeout has been
 exceeded. However keeping scrolls open has a cost, as discussed in the
 <<scroll-search-context,previous section>> so scrolls should be explicitly
-cleared as soon as the scroll is not being used anymore using the
-`clear-scroll` API:
+cleared as soon as they are not being used anymore using the `clear-scroll`
+API:
 
 [source,js]
 ---------------------------------------
-DELETE /_search/scroll
+POST /_search/clear_scroll
 {
     "scroll_id" : ["DXF1ZXJ5QW5kRmV0Y2gBAAAAAAAAAD4WYm9laVYtZndUQlNsdDcwakFMNjU1QQ=="]
 }
@@ -154,7 +154,7 @@ Multiple scroll IDs can be passed as array:
 
 [source,js]
 ---------------------------------------
-DELETE /_search/scroll
+POST /_search/clear_scroll
 {
     "scroll_id" : [
       "DXF1ZXJ5QW5kRmV0Y2gBAAAAAAAAAD4WYm9laVYtZndUQlNsdDcwakFMNjU1QQ==",
@@ -165,23 +165,24 @@ DELETE /_search/scroll
 // CONSOLE
 // TEST[catch:missing]
 
-All search contexts can be cleared with the `_all` parameter:
+The recommended way to provide the `scroll_id` is by placing it in the request
+body using the `POST` verb against the `clear_scroll` endpoint added[5.2.0].
+All of the other ways to provide the `scroll_id`, either as a url parameter or
+as part of the request body using the `DELETE /_search/scroll` endpoint, are
+ deprecated deprecated[5.2.0].
 
-[source,js]
----------------------------------------
-DELETE /_search/scroll/_all
----------------------------------------
-// CONSOLE
-
-The `scroll_id` can also be passed as a query string parameter or in the request body.
-Multiple scroll IDs can be passed as comma separated values:
+All search contexts can be cleared with the special `_all` scroll id:
 
 [source,js]
 ---------------------------------------
-DELETE /_search/scroll/DXF1ZXJ5QW5kRmV0Y2gBAAAAAAAAAD4WYm9laVYtZndUQlNsdDcwakFMNjU1QQ==,DnF1ZXJ5VGhlbkZldGNoBQAAAAAAAAABFmtSWWRRWUJrU2o2ZExpSGJCVmQxYUEAAAAAAAAAAxZrUllkUVlCa1NqNmRMaUhiQlZkMWFBAAAAAAAAAAIWa1JZZFFZQmtTajZkTGlIYkJWZDFhQQAAAAAAAAAFFmtSWWRRWUJrU2o2ZExpSGJCVmQxYUEAAAAAAAAABBZrUllkUVlCa1NqNmRMaUhiQlZkMWFB
+POST /_search/clear_scroll
+{
+    "scroll_id" : [
+      "_all"
+    ]
+}
 ---------------------------------------
 // CONSOLE
-// TEST[catch:missing]
 
 [[sliced-scroll]]
 ==== Sliced Scroll

rest-api-spec/src/main/resources/rest-api-spec/api/clear_scroll.json

@@ -1,10 +1,10 @@
 {
   "clear_scroll": {
     "documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/5.x/search-request-scroll.html",
-    "methods": ["DELETE"],
+    "methods": ["POST"],
     "url": {
-      "path": "/_search/scroll/{scroll_id}",
-      "paths": ["/_search/scroll/{scroll_id}", "/_search/scroll"],
+      "path": "/_search/clear_scroll/{scroll_id}",
+      "paths": ["/_search/clear_scroll/{scroll_id}", "/_search/clear_scroll"],
       "parts": {
         "scroll_id": {
           "type" : "list",
@@ -14,7 +14,7 @@
       "params": {}
     },
     "body": {
-      "description": "A comma-separated list of scroll IDs to clear if none was specified via the scroll_id parameter"
+      "description": "A comma-separated list of scroll IDs to clear, has the precedence over the scroll_id parameter"
     }
   }
 }

rest-api-spec/src/main/resources/rest-api-spec/api/delete_scroll.json

@@ -0,0 +1,20 @@
+{
+  "delete_scroll": {
+    "documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/5.x/search-request-scroll.html",
+    "methods": ["DELETE"],
+    "url": {
+      "path": "/_search/scroll/{scroll_id}",
+      "paths": ["/_search/scroll/{scroll_id}", "/_search/scroll"],
+      "parts": {
+        "scroll_id": {
+          "type" : "list",
+          "description" : "A comma-separated list of scroll IDs to clear"
+        }
+      },
+      "params": {}
+    },
+    "body": {
+      "description": "A comma-separated list of scroll IDs to clear, has the precedence over the scroll_id parameter"
+    }
+  }
+}

rest-api-spec/src/main/resources/rest-api-spec/test/scroll/10_basic.yaml

@@ -63,7 +63,7 @@
 
   - do:
       clear_scroll:
-        scroll_id: $scroll_id
+        body: { "scroll_id": [ "$scroll_id" ]}
 
 ---
 "Body params override query string":
@@ -123,5 +123,5 @@
 
   - do:
       clear_scroll:
-        scroll_id: $scroll_id
+        body: { "scroll_id": [ "$scroll_id" ]}
 

rest-api-spec/src/main/resources/rest-api-spec/test/scroll/11_clear.yaml

@@ -25,54 +25,19 @@
 
   - do:
       clear_scroll:
-        scroll_id: $scroll_id1
+        body: { "scroll_id": [ "$scroll_id1" ]}
 
   - do:
       catch: missing
       scroll:
         scroll_id: $scroll_id1
 
   - do:
-        catch: missing
-        clear_scroll:
-          scroll_id: $scroll_id1
-
----
-"Body params override query string":
-  - do:
-      indices.create:
-          index:  test_scroll
-  - do:
-      index:
-          index:  test_scroll
-          type:   test
-          id:     42
-          body:   { foo: bar }
-
-  - do:
-      indices.refresh: {}
-
-  - do:
-      search:
-        index: test_scroll
-        scroll: 1m
-        body:
-          query:
-            match_all: {}
-
-  - set: {_scroll_id: scroll_id1}
-
-  - do:
+      catch: missing
       clear_scroll:
-        scroll_id: "invalid_scroll_id"
         body: { "scroll_id": [ "$scroll_id1" ]}
 
   - do:
       catch: missing
-      scroll:
-        scroll_id: $scroll_id1
-
-  - do:
-        catch: missing
-        clear_scroll:
-          scroll_id: $scroll_id1
+      clear_scroll:
+        body: { "scroll_id": [ "$scroll_id1" ]}

rest-api-spec/src/main/resources/rest-api-spec/test/scroll/12_slices.yaml

@@ -30,8 +30,8 @@
   - set: {_scroll_id: scroll_id}
 
   - do:
-        clear_scroll:
-          scroll_id: $scroll_id
+      clear_scroll:
+        body: { "scroll_id": [ "$scroll_id" ]}
 
   - do:
         catch:  /query_phase_execution_exception.*The number of slices.*index.max_slices_per_scroll/
@@ -67,6 +67,6 @@
   - set: {_scroll_id: scroll_id}
 
   - do:
-        clear_scroll:
-          scroll_id: $scroll_id
+      clear_scroll:
+        body: { "scroll_id": [ "$scroll_id" ]}