Fail queries with scroll that explicitely set request_cache (#27342)

Queries that create a scroll context cannot use the cache.
They modify the search context during their execution so using the cache
can lead to duplicate result for the next scroll query.

This change fails the entire request if the request_cache option is explictely set
on a query that creates a scroll context (`scroll=1m`) and make sure internally that we never
use the cache for these queries when the option is not explicitely used.
For 6.x a deprecation log will be printed instead of failing the entire request and the request_cache hint
will be ignored (forced to false).

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

@@ -28,6 +28,8 @@
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.search.Scroll;
@@ -55,6 +57,7 @@
  * @see SearchResponse
  */
 public final class SearchRequest extends ActionRequest implements IndicesRequest.Replaceable {
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(SearchRequest.class));
 
     private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));
 
@@ -120,6 +123,10 @@ public ActionRequestValidationException validate() {
             validationException =
                 addValidationError("using [from] is not allowed in a scroll context", validationException);
         }
+        if (requestCache != null && requestCache && scroll() != null) {
+            DEPRECATION_LOGGER.deprecated("Explicitly set [request_cache] for a scroll query is deprecated and will return a 400 " +
+                "error in future versions");
+        }
         return validationException;
     }
 
@@ -385,7 +392,7 @@ public String getDescription() {
                 sb.append("], ");
                 sb.append("search_type[").append(searchType).append("], ");
                 if (source != null) {
-                    
+
                     sb.append("source[").append(source.toString(FORMAT_PARAMS)).append("]");
                 } else {
                     sb.append("source[]");

core/src/main/java/org/elasticsearch/indices/IndicesService.java

@@ -1072,6 +1072,12 @@ public void close() {
      * Can the shard request be cached at all?
      */
     public boolean canCache(ShardSearchRequest request, SearchContext context) {
+        // Queries that create a scroll context cannot use the cache.
+        // They modify the search context during their execution so using the cache
+        // may invalidate the scroll for the next query.
+        if (request.scroll() != null) {
+            return false;
+        }
 
         // We cannot cache with DFS because results depend not only on the content of the index but also
         // on the overridden statistics. So if you ran two queries on the same index with different stats
@@ -1080,6 +1086,7 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) {
         if (SearchType.QUERY_THEN_FETCH != context.searchType()) {
             return false;
         }
+
         IndexSettings settings = context.indexShard().indexSettings();
         // if not explicitly set in the request, use the index setting, if not, use the request
         if (request.requestCache() == null) {

docs/reference/migration/migrate_6_0/search.asciidoc

@@ -182,4 +182,12 @@ The parameter was already ignored in these situations, now in addition an error
 
 The maximum number of results (`from` + `size`) that is allowed to be retrieved
 via inner hits and top hits has been limited to 100. The limit can be controlled
-via the `index.max_inner_result_window` index setting.
+via the `index.max_inner_result_window` index setting.
+
+==== Scroll queries that use the request_cache are deprecated
+
+Setting `request_cache:true` on a query that creates a scroll ('scroll=1m`)
+ is deprecated and the request will not use the cache internally.
+In future versions we will return a `400 - Bad request` instead of just ignoring
+the hint.
+Scroll queries are not meant to be cached.

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

@@ -197,3 +197,23 @@
       clear_scroll:
         scroll_id: $scroll_id
 
+---
+"Scroll cannot used the request cache":
+  - skip:
+      version: " - 6.0.99"
+      reason:  the deprecation appears in 6.1.0
+      features: "warnings"
+
+  - do:
+      indices.create:
+        index: test_scroll
+  - do:
+      warnings:
+        - 'Explicitly set [request_cache] for a scroll query is deprecated and will return a 400 error in future versions'
+      search:
+        index: test_scroll
+        scroll: 1m
+        request_cache: true
+        body:
+          query:
+            match_all: {}