From c1ee50f238c07cd5c6cf5a237438ca0afc39c582 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 12 Jan 2017 17:46:39 +0000 Subject: [PATCH] Return 200 on GET requests for all resources when none exists (elastic/elasticsearch#694) When a user makes a GET request to retrieve all resources of a type (e.g. anomaly_detectors) and none exists, the response should be an empty array with 200 status code. This commit fixes this issue for: * anomaly_detectors and _stats * schedulers and _stats * lists * buckets All other GETs work fine already. Original commit: elastic/x-pack-elasticsearch@4daaa91aa4ec37671a7f97f19cb3dc3983bf1b36 --- .../xpack/ml/action/GetBucketsAction.java | 10 ++----- .../xpack/ml/action/GetJobsAction.java | 10 ++----- .../xpack/ml/action/GetJobsStatsAction.java | 14 +++++----- .../xpack/ml/action/GetListAction.java | 27 +++++++------------ .../xpack/ml/action/GetSchedulersAction.java | 10 ++----- .../ml/action/GetSchedulersStatsAction.java | 10 ++----- .../xpack/ml/job/manager/JobManager.java | 2 +- .../xpack/ml/rest/job/RestGetJobsAction.java | 4 +-- .../ml/rest/job/RestGetJobsStatsAction.java | 4 +-- .../ml/rest/results/RestGetBucketsAction.java | 4 +-- .../schedulers/RestGetSchedulersAction.java | 4 +-- .../RestGetSchedulersStatsAction.java | 4 +-- .../xpack/ml/integration/MlJobIT.java | 2 +- .../rest-api-spec/test/get_lists.yaml | 19 +++++++++++++ .../rest-api-spec/test/jobs_crud.yaml | 16 +++++++++++ .../rest-api-spec/test/schedulers_crud.yaml | 16 +++++++++++ 16 files changed, 87 insertions(+), 69 deletions(-) create mode 100644 elasticsearch/src/test/resources/rest-api-spec/test/get_lists.yaml diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetBucketsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetBucketsAction.java index 51b93ff91f3..6f3e1462407 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetBucketsAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetBucketsAction.java @@ -22,12 +22,11 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.StatusToXContentObject; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.rest.RestStatus; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.ml.job.Job; @@ -326,7 +325,7 @@ public class GetBucketsAction extends Action buckets; @@ -361,11 +360,6 @@ public class GetBucketsAction extends Action jobs; @@ -146,11 +145,6 @@ public class GetJobsAction extends Action listener) { logger.debug("Get stats for job '{}'", request.getJobId()); QueryPage jobs = jobManager.getJob(request.getJobId(), clusterService.state()); + if (jobs.count() == 0) { + listener.onResponse(new GetJobsStatsAction.Response(new QueryPage<>(Collections.emptyList(), 0, Job.RESULTS_FIELD))); + } MlMetadata mlMetadata = clusterService.state().metaData().custom(MlMetadata.TYPE); AtomicInteger counter = new AtomicInteger(0); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetListAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetListAction.java index e1fdb3eda2d..44a74f4165b 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetListAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetListAction.java @@ -183,7 +183,7 @@ public class GetListAction extends Action responseBody; - if (response.getHits().hits().length > 0) { - List docs = new ArrayList<>(response.getHits().hits().length); - for (SearchHit hit : response.getHits().getHits()) { - BytesReference docSource = hit.sourceRef(); - XContentParser parser = - XContentFactory.xContent(docSource).createParser(NamedXContentRegistry.EMPTY, docSource); - docs.add(ListDocument.PARSER.apply(parser, () -> parseFieldMatcher)); - } - - responseBody = new QueryPage<>(docs, docs.size(), ListDocument.RESULTS_FIELD); - - Response listResponse = new Response(responseBody); - listener.onResponse(listResponse); - } else { - this.onFailure(QueryPage.emptyQueryPage(ListDocument.RESULTS_FIELD)); + List docs = new ArrayList<>(); + for (SearchHit hit : response.getHits().getHits()) { + BytesReference docSource = hit.sourceRef(); + XContentParser parser = + XContentFactory.xContent(docSource).createParser(NamedXContentRegistry.EMPTY, docSource); + docs.add(ListDocument.PARSER.apply(parser, () -> parseFieldMatcher)); } + Response listResponse = new Response(new QueryPage<>(docs, docs.size(), ListDocument.RESULTS_FIELD)); + listener.onResponse(listResponse); + } catch (Exception e) { this.onFailure(e); } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetSchedulersAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetSchedulersAction.java index ca871d12432..74f383295cd 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetSchedulersAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/action/GetSchedulersAction.java @@ -23,10 +23,9 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.StatusToXContentObject; +import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.rest.RestStatus; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.ml.job.metadata.MlMetadata; @@ -119,7 +118,7 @@ public class GetSchedulersAction extends Action schedulers; @@ -145,11 +144,6 @@ public class GetSchedulersAction extends Action transportGetJobAction.execute(request, new RestStatusToXContentListener<>(channel)); + return channel -> transportGetJobAction.execute(request, new RestToXContentListener<>(channel)); } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/job/RestGetJobsStatsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/job/RestGetJobsStatsAction.java index 14bebbaa436..24e2bc58bc5 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/job/RestGetJobsStatsAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/job/RestGetJobsStatsAction.java @@ -11,7 +11,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.action.RestStatusToXContentListener; +import org.elasticsearch.rest.action.RestToXContentListener; import org.elasticsearch.xpack.ml.MlPlugin; import org.elasticsearch.xpack.ml.action.GetJobsStatsAction; import org.elasticsearch.xpack.ml.job.Job; @@ -35,6 +35,6 @@ public class RestGetJobsStatsAction extends BaseRestHandler { @Override protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException { GetJobsStatsAction.Request request = new GetJobsStatsAction.Request(restRequest.param(Job.ID.getPreferredName())); - return channel -> transportGetJobsStatsAction.execute(request, new RestStatusToXContentListener<>(channel)); + return channel -> transportGetJobsStatsAction.execute(request, new RestToXContentListener<>(channel)); } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/results/RestGetBucketsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/results/RestGetBucketsAction.java index c3cfd6c2b4c..9f3ebaeeedf 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/results/RestGetBucketsAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/results/RestGetBucketsAction.java @@ -12,7 +12,7 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.action.RestStatusToXContentListener; +import org.elasticsearch.rest.action.RestToXContentListener; import org.elasticsearch.xpack.ml.MlPlugin; import org.elasticsearch.xpack.ml.action.GetBucketsAction; import org.elasticsearch.xpack.ml.job.Job; @@ -90,6 +90,6 @@ public class RestGetBucketsAction extends BaseRestHandler { request.setIncludeInterim(restRequest.paramAsBoolean(GetBucketsAction.Request.INCLUDE_INTERIM.getPreferredName(), false)); } - return channel -> transportAction.execute(request, new RestStatusToXContentListener<>(channel)); + return channel -> transportAction.execute(request, new RestToXContentListener<>(channel)); } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersAction.java index bb23e30d0a1..b90ec776d5d 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersAction.java @@ -11,7 +11,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.action.RestStatusToXContentListener; +import org.elasticsearch.rest.action.RestToXContentListener; import org.elasticsearch.xpack.ml.MlPlugin; import org.elasticsearch.xpack.ml.action.GetSchedulersAction; import org.elasticsearch.xpack.ml.scheduler.SchedulerConfig; @@ -35,6 +35,6 @@ public class RestGetSchedulersAction extends BaseRestHandler { @Override protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException { GetSchedulersAction.Request request = new GetSchedulersAction.Request(restRequest.param(SchedulerConfig.ID.getPreferredName())); - return channel -> transportGetSchedulersAction.execute(request, new RestStatusToXContentListener<>(channel)); + return channel -> transportGetSchedulersAction.execute(request, new RestToXContentListener<>(channel)); } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersStatsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersStatsAction.java index 89b6cc78592..49d26304c5b 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersStatsAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ml/rest/schedulers/RestGetSchedulersStatsAction.java @@ -11,7 +11,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.action.RestStatusToXContentListener; +import org.elasticsearch.rest.action.RestToXContentListener; import org.elasticsearch.xpack.ml.MlPlugin; import org.elasticsearch.xpack.ml.action.GetSchedulersStatsAction; import org.elasticsearch.xpack.ml.scheduler.SchedulerConfig; @@ -36,6 +36,6 @@ public class RestGetSchedulersStatsAction extends BaseRestHandler { protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException { GetSchedulersStatsAction.Request request = new GetSchedulersStatsAction.Request( restRequest.param(SchedulerConfig.ID.getPreferredName())); - return channel -> transportGetSchedulersStatsAction.execute(request, new RestStatusToXContentListener<>(channel)); + return channel -> transportGetSchedulersStatsAction.execute(request, new RestToXContentListener<>(channel)); } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/ml/integration/MlJobIT.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/ml/integration/MlJobIT.java index 83c8656d3bb..0be54a5263c 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/ml/integration/MlJobIT.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/ml/integration/MlJobIT.java @@ -55,7 +55,7 @@ public class MlJobIT extends ESRestTestCase { () -> client().performRequest("get", MlPlugin.BASE_PATH + "anomaly_detectors/non-existing-job/_stats")); assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(404)); - assertThat(e.getMessage(), containsString("Could not find requested jobs")); + assertThat(e.getMessage(), containsString("No known job with id 'non-existing-job'")); } public void testGetJob_GivenJobExists() throws Exception { diff --git a/elasticsearch/src/test/resources/rest-api-spec/test/get_lists.yaml b/elasticsearch/src/test/resources/rest-api-spec/test/get_lists.yaml new file mode 100644 index 00000000000..e599ad7798a --- /dev/null +++ b/elasticsearch/src/test/resources/rest-api-spec/test/get_lists.yaml @@ -0,0 +1,19 @@ +--- +"Test get all lists given no list exists": + + - do: + xpack.ml.put_list: + body: > + { + "id": "foo", + "items": ["abc", "xyz"] + } + + - do: + xpack.ml.delete_list: + list_id: "foo" + + - do: + xpack.ml.get_lists: {} + - match: { count: 0 } + - match: { lists: [] } diff --git a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml index d140ebc7918..6fd7b973575 100644 --- a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml +++ b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml @@ -1,5 +1,21 @@ +--- +"Test get all jobs and stats given no job exists": + + - do: + xpack.ml.get_jobs: + job_id: "_all" + - match: { count: 0 } + - match: { jobs: [] } + + - do: + xpack.ml.get_jobs_stats: + job_id: "_all" + - match: { count: 0 } + - match: { jobs: [] } + --- "Test job crud apis": + - do: xpack.ml.put_job: job_id: farequote diff --git a/elasticsearch/src/test/resources/rest-api-spec/test/schedulers_crud.yaml b/elasticsearch/src/test/resources/rest-api-spec/test/schedulers_crud.yaml index f39d64c3761..40f1fd0c1c3 100644 --- a/elasticsearch/src/test/resources/rest-api-spec/test/schedulers_crud.yaml +++ b/elasticsearch/src/test/resources/rest-api-spec/test/schedulers_crud.yaml @@ -31,6 +31,22 @@ setup: "time_format":"epoch" } } + +--- +"Test get all schedulers and stats given no scheduler exists": + + - do: + xpack.ml.get_schedulers: + scheduler_id: "_all" + - match: { count: 0 } + - match: { schedulers: [] } + + - do: + xpack.ml.get_schedulers_stats: + scheduler_id: "_all" + - match: { count: 0 } + - match: { schedulers: [] } + --- "Test put scheduler referring to missing job_id": - do: