[REST] Missing scroll id now returns 404

A bad/non-existing scroll ID used to return a 200, however a 404 might be more useful.
Also, this PR returns the right Exception (SearchContextMissingException) in the Java API.

Additionally: Added StatusToXContent interface and RestStatusToXContentListener listener, so
the appropriate RestStatus can be returned

Closes #5729

rest-api-spec/test/scroll/11_clear.yaml

@@ -29,7 +29,6 @@
         scroll_id: $scroll_id1
 
   - do:
+      catch: missing
       scroll:
         scroll_id: $scroll_id1
-
-  - length: {hits.hits: 0}

src/main/java/org/elasticsearch/action/search/SearchResponse.java

@@ -23,10 +23,7 @@ import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.*;
-import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentBuilderString;
-import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.search.SearchHits;
 import org.elasticsearch.search.aggregations.Aggregations;
@@ -42,7 +39,7 @@ import static org.elasticsearch.search.internal.InternalSearchResponse.readInter
 /**
  * A response of a search request.
  */
-public class SearchResponse extends ActionResponse implements ToXContent {
+public class SearchResponse extends ActionResponse implements StatusToXContent {
 
     private InternalSearchResponse internalResponse;
 

src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java

@@ -21,6 +21,8 @@ package org.elasticsearch.action.search.type;
 
 import com.carrotsearch.hppc.IntArrayList;
 import org.apache.lucene.search.ScoreDoc;
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.search.*;
 import org.elasticsearch.cluster.ClusterService;
@@ -31,6 +33,7 @@ import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.AtomicArray;
+import org.elasticsearch.search.SearchContextMissingException;
 import org.elasticsearch.search.action.SearchServiceListener;
 import org.elasticsearch.search.action.SearchServiceTransportAction;
 import org.elasticsearch.search.controller.SearchPhaseController;
@@ -171,8 +174,13 @@ public class TransportSearchScrollQueryThenFetchAction extends AbstractComponent
 
                 @Override
                 public void onFailure(Throwable t) {
+                    Throwable cause = ExceptionsHelper.unwrapCause(t);
+                    if (cause instanceof SearchContextMissingException) {
+                        listener.onFailure(t);
+                    } else {
                         onQueryPhaseFailure(shardIndex, counter, searchId, t);
                     }
+                }
             });
         }
 

src/main/java/org/elasticsearch/common/xcontent/StatusToXContent.java

@@ -0,0 +1,32 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.common.xcontent;
+
+import org.elasticsearch.rest.RestStatus;
+
+/**
+ *
+ */
+public interface StatusToXContent extends ToXContent {
+
+    /**
+     * Returns the REST status to make sure it is returned correctly
+     */
+    RestStatus status();
+}

src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

@@ -33,6 +33,7 @@ import org.elasticsearch.rest.BaseRestHandler;
 import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.action.support.RestStatusToXContentListener;
 import org.elasticsearch.rest.action.support.RestToXContentListener;
 import org.elasticsearch.search.Scroll;
 import org.elasticsearch.search.builder.SearchSourceBuilder;
@@ -71,7 +72,7 @@ public class RestSearchAction extends BaseRestHandler {
         SearchRequest searchRequest;
         searchRequest = RestSearchAction.parseSearchRequest(request);
         searchRequest.listenerThreaded(false);
-        client.search(searchRequest, new RestToXContentListener<SearchResponse>(channel));
+        client.search(searchRequest, new RestStatusToXContentListener<SearchResponse>(channel));
     }
 
     public static SearchRequest parseSearchRequest(RestRequest request) {

src/main/java/org/elasticsearch/rest/action/search/RestSearchScrollAction.java

@@ -19,7 +19,6 @@
 
 package org.elasticsearch.rest.action.search;
 
-import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.search.SearchScrollRequest;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.inject.Inject;
@@ -29,7 +28,7 @@ import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.action.support.RestActions;
-import org.elasticsearch.rest.action.support.RestToXContentListener;
+import org.elasticsearch.rest.action.support.RestStatusToXContentListener;
 import org.elasticsearch.search.Scroll;
 
 import static org.elasticsearch.common.unit.TimeValue.parseTimeValue;
@@ -63,6 +62,7 @@ public class RestSearchScrollAction extends BaseRestHandler {
         if (scroll != null) {
             searchScrollRequest.scroll(new Scroll(parseTimeValue(scroll, null)));
         }
-        client.searchScroll(searchScrollRequest, new RestToXContentListener<SearchResponse>(channel));
+
+        client.searchScroll(searchScrollRequest, new RestStatusToXContentListener(channel));
     }
 }

src/main/java/org/elasticsearch/rest/action/support/RestStatusToXContentListener.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.rest.action.support;
+
+import org.elasticsearch.common.xcontent.StatusToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.rest.BytesRestResponse;
+import org.elasticsearch.rest.RestChannel;
+import org.elasticsearch.rest.RestResponse;
+
+/**
+ *
+ */
+public class RestStatusToXContentListener<Response extends StatusToXContent> extends RestResponseListener<Response> {
+
+    public RestStatusToXContentListener(RestChannel channel) {
+        super(channel);
+    }
+
+    @Override
+    public final RestResponse buildResponse(Response response) throws Exception {
+        return buildResponse(response, channel.newBuilder());
+    }
+
+    public final RestResponse buildResponse(Response response, XContentBuilder builder) throws Exception {
+        builder.startObject();
+        response.toXContent(builder, channel.request());
+        builder.endObject();
+        return new BytesRestResponse(response.status(), builder);
+    }
+
+}

src/main/java/org/elasticsearch/search/SearchContextMissingException.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.search;
 
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.rest.RestStatus;
 
 /**
  *
@@ -36,4 +37,9 @@ public class SearchContextMissingException extends ElasticsearchException {
     public long id() {
         return this.id;
     }
+
+    @Override
+    public RestStatus status() {
+        return RestStatus.NOT_FOUND;
+    }
 }

src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.search.scroll;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.action.search.ClearScrollResponse;
 import org.elasticsearch.action.search.SearchRequestBuilder;
@@ -29,6 +30,8 @@ import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException;
 import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.search.SearchContextMissingException;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ElasticsearchIntegrationTest;
@@ -39,6 +42,7 @@ import java.util.Map;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.*;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows;
 import static org.hamcrest.Matchers.*;
 
 /**
@@ -287,19 +291,8 @@ public class SearchScrollTests extends ElasticsearchIntegrationTest {
                 .execute().actionGet();
         assertThat(clearResponse.isSucceeded(), equalTo(true));
 
-        searchResponse1 = client().prepareSearchScroll(searchResponse1.getScrollId())
+        assertThrows(client().prepareSearchScroll(searchResponse1.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class);
-                .setScroll(TimeValue.timeValueMinutes(2))
+        assertThrows(client().prepareSearchScroll(searchResponse2.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class);
-                .execute().actionGet();
-
-        searchResponse2 = client().prepareSearchScroll(searchResponse2.getScrollId())
-                .setScroll(TimeValue.timeValueMinutes(2))
-                .execute().actionGet();
-
-        assertThat(searchResponse1.getHits().getTotalHits(), equalTo(0l));
-        assertThat(searchResponse1.getHits().hits().length, equalTo(0));
-
-        assertThat(searchResponse2.getHits().getTotalHits(), equalTo(0l));
-        assertThat(searchResponse2.getHits().hits().length, equalTo(0));
     }
 
     @Test
@@ -404,19 +397,8 @@ public class SearchScrollTests extends ElasticsearchIntegrationTest {
                 .execute().actionGet();
         assertThat(clearResponse.isSucceeded(), equalTo(true));
 
-        searchResponse1 = client().prepareSearchScroll(searchResponse1.getScrollId())
+        assertThrows(cluster().transportClient().prepareSearchScroll(searchResponse1.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class);
-                .setScroll(TimeValue.timeValueMinutes(2))
+        assertThrows(cluster().transportClient().prepareSearchScroll(searchResponse2.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class);
-                .execute().actionGet();
-
-        searchResponse2 = client().prepareSearchScroll(searchResponse2.getScrollId())
-                .setScroll(TimeValue.timeValueMinutes(2))
-                .execute().actionGet();
-
-        assertThat(searchResponse1.getHits().getTotalHits(), equalTo(0l));
-        assertThat(searchResponse1.getHits().hits().length, equalTo(0));
-
-        assertThat(searchResponse2.getHits().getTotalHits(), equalTo(0l));
-        assertThat(searchResponse2.getHits().hits().length, equalTo(0));
     }
 
     @Test
@@ -447,7 +429,24 @@ public class SearchScrollTests extends ElasticsearchIntegrationTest {
                 }
             }
         }
-
     }
 
+    @Test
+    public void testThatNonExistingScrollIdReturnsCorrectException() throws Exception {
+        client().prepareIndex("index", "type", "1").setSource("field", "value").execute().get();
+        refresh();
+
+        try {
+            SearchResponse searchResponse = client().prepareSearch("index").setSize(1).setScroll("1m").get();
+            assertThat(searchResponse.getScrollId(), is(notNullValue()));
+
+            ClearScrollResponse clearScrollResponse = client().prepareClearScroll().addScrollId(searchResponse.getScrollId()).get();
+            assertThat(clearScrollResponse.isSucceeded(), is(true));
+
+            cluster().transportClient().prepareSearchScroll(searchResponse.getScrollId()).get();
+            fail("Expected exception to happen due to non-existing scroll id");
+        } catch (ElasticsearchException e) {
+            assertThat(e.status(), is(RestStatus.NOT_FOUND));
+        }
+    }
 }