From d356881664dc7ce60f46aaed4d910f87be3cfea9 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Mon, 5 May 2014 10:35:46 +0200 Subject: [PATCH] [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 | 3 +- .../action/search/SearchResponse.java | 7 +-- ...sportSearchScrollQueryThenFetchAction.java | 10 +++- .../common/xcontent/StatusToXContent.java | 32 +++++++++++ .../rest/action/search/RestSearchAction.java | 3 +- .../action/search/RestSearchScrollAction.java | 6 +-- .../support/RestStatusToXContentListener.java | 48 +++++++++++++++++ .../search/SearchContextMissingException.java | 6 +++ .../search/scroll/SearchScrollTests.java | 53 +++++++++---------- 9 files changed, 129 insertions(+), 39 deletions(-) create mode 100644 src/main/java/org/elasticsearch/common/xcontent/StatusToXContent.java create mode 100644 src/main/java/org/elasticsearch/rest/action/support/RestStatusToXContentListener.java diff --git a/rest-api-spec/test/scroll/11_clear.yaml b/rest-api-spec/test/scroll/11_clear.yaml index 12cf760b418..6cdfb288198 100644 --- a/rest-api-spec/test/scroll/11_clear.yaml +++ b/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} diff --git a/src/main/java/org/elasticsearch/action/search/SearchResponse.java b/src/main/java/org/elasticsearch/action/search/SearchResponse.java index 08ad7a236b3..6584052468f 100644 --- a/src/main/java/org/elasticsearch/action/search/SearchResponse.java +++ b/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.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentBuilderString; -import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.*; 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; diff --git a/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java b/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java index 91a76d4901b..632c836e412 100644 --- a/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java +++ b/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,7 +174,12 @@ public class TransportSearchScrollQueryThenFetchAction extends AbstractComponent @Override public void onFailure(Throwable t) { - onQueryPhaseFailure(shardIndex, counter, searchId, t); + Throwable cause = ExceptionsHelper.unwrapCause(t); + if (cause instanceof SearchContextMissingException) { + listener.onFailure(t); + } else { + onQueryPhaseFailure(shardIndex, counter, searchId, t); + } } }); } diff --git a/src/main/java/org/elasticsearch/common/xcontent/StatusToXContent.java b/src/main/java/org/elasticsearch/common/xcontent/StatusToXContent.java new file mode 100644 index 00000000000..d181ffc21d1 --- /dev/null +++ b/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(); +} diff --git a/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index 7df3726a915..d6789cc5e4c 100644 --- a/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/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(channel)); + client.search(searchRequest, new RestStatusToXContentListener(channel)); } public static SearchRequest parseSearchRequest(RestRequest request) { diff --git a/src/main/java/org/elasticsearch/rest/action/search/RestSearchScrollAction.java b/src/main/java/org/elasticsearch/rest/action/search/RestSearchScrollAction.java index 8d2e001e6b9..ed9786a3582 100644 --- a/src/main/java/org/elasticsearch/rest/action/search/RestSearchScrollAction.java +++ b/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(channel)); + + client.searchScroll(searchScrollRequest, new RestStatusToXContentListener(channel)); } } diff --git a/src/main/java/org/elasticsearch/rest/action/support/RestStatusToXContentListener.java b/src/main/java/org/elasticsearch/rest/action/support/RestStatusToXContentListener.java new file mode 100644 index 00000000000..6bd9a900c8e --- /dev/null +++ b/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 extends RestResponseListener { + + 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); + } + +} diff --git a/src/main/java/org/elasticsearch/search/SearchContextMissingException.java b/src/main/java/org/elasticsearch/search/SearchContextMissingException.java index b778985b405..044cddfde30 100644 --- a/src/main/java/org/elasticsearch/search/SearchContextMissingException.java +++ b/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; + } } diff --git a/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java b/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java index 33a3f9104ae..7e2415e9632 100644 --- a/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java +++ b/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()) - .setScroll(TimeValue.timeValueMinutes(2)) - .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)); + assertThrows(client().prepareSearchScroll(searchResponse1.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class); + assertThrows(client().prepareSearchScroll(searchResponse2.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class); } @Test @@ -404,19 +397,8 @@ public class SearchScrollTests extends ElasticsearchIntegrationTest { .execute().actionGet(); assertThat(clearResponse.isSucceeded(), equalTo(true)); - searchResponse1 = client().prepareSearchScroll(searchResponse1.getScrollId()) - .setScroll(TimeValue.timeValueMinutes(2)) - .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)); + assertThrows(cluster().transportClient().prepareSearchScroll(searchResponse1.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class); + assertThrows(cluster().transportClient().prepareSearchScroll(searchResponse2.getScrollId()).setScroll(TimeValue.timeValueMinutes(2)), SearchContextMissingException.class); } @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)); + } + } }