From cc981fa3771a590f50e058aad9fe8f8fed8a63b1 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 26 Mar 2020 17:51:43 +0200 Subject: [PATCH] [7.x][ML] Get ML filters size should default to 100 (#54207) (#54278) When get filters is called without setting the `size` paramter only up to 10 filters are returned. However, 100 filters should be returned. This commit fixes this and adds an integ test to guard it. It seems this was accidentally broken in #39976. Closes #54206 Backport of #54207 --- .../core/ml/action/GetFiltersAction.java | 29 +++------------ .../action/GetFiltersActionRequestTests.java | 15 ++++---- .../ml/qa/ml-with-security/build.gradle | 1 - .../MlNativeAutodetectIntegTestCase.java | 6 ---- .../ml/integration/MlNativeIntegTestCase.java | 11 ++++++ .../autodetect/AutodetectProcessManager.java | 20 +++-------- .../ml/rest/filter/RestGetFiltersAction.java | 8 ++--- .../xpack/ml/integration/MlFiltersIT.java | 36 +++++++++++++++++++ .../rest-api-spec/test/ml/filter_crud.yml | 22 ------------ 9 files changed, 68 insertions(+), 80 deletions(-) create mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/MlFiltersIT.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetFiltersAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetFiltersAction.java index de22800832dfc..a304704914470 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetFiltersAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetFiltersAction.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.core.ml.action; import org.elasticsearch.action.ActionRequestBuilder; -import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.ActionType; import org.elasticsearch.client.ElasticsearchClient; import org.elasticsearch.common.io.stream.StreamInput; @@ -14,14 +13,11 @@ import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.core.action.AbstractGetResourcesRequest; import org.elasticsearch.xpack.core.action.AbstractGetResourcesResponse; -import org.elasticsearch.xpack.core.action.util.PageParams; import org.elasticsearch.xpack.core.action.util.QueryPage; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import java.io.IOException; -import static org.elasticsearch.action.ValidateActions.addValidationError; - public class GetFiltersAction extends ActionType { @@ -35,31 +31,16 @@ private GetFiltersAction() { public static class Request extends AbstractGetResourcesRequest { public Request() { - // Put our own defaults for backwards compatibility - super(null, null, true); - } - - public Request(StreamInput in) throws IOException { - super(in); + setAllowNoResources(true); } - public void setFilterId(String filterId) { + public Request(String filterId) { setResourceId(filterId); + setAllowNoResources(true); } - public String getFilterId() { - return getResourceId(); - } - - @Override - public ActionRequestValidationException validate() { - ActionRequestValidationException validationException = null; - if (getPageParams() != null && getResourceId() != null) { - validationException = addValidationError("Params [" + PageParams.FROM.getPreferredName() + - ", " + PageParams.SIZE.getPreferredName() + "] are incompatible with [" - + MlFilter.ID.getPreferredName() + "]", validationException); - } - return validationException; + public Request(StreamInput in) throws IOException { + super(in); } @Override diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionRequestTests.java index a4419a60fca20..71602af1539dd 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionRequestTests.java @@ -7,23 +7,22 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.test.AbstractWireSerializingTestCase; -import org.elasticsearch.xpack.core.ml.action.GetFiltersAction.Request; import org.elasticsearch.xpack.core.action.util.PageParams; +import org.elasticsearch.xpack.core.ml.action.GetFiltersAction.Request; public class GetFiltersActionRequestTests extends AbstractWireSerializingTestCase { @Override protected Request createTestInstance() { + if (randomBoolean()) { + return new Request(randomAlphaOfLength(10)); + } Request request = new Request(); if (randomBoolean()) { - request.setFilterId(randomAlphaOfLengthBetween(1, 20)); - } else { - if (randomBoolean()) { - int from = randomInt(10000); - int size = randomInt(10000); - request.setPageParams(new PageParams(from, size)); - } + int from = randomInt(10000); + int size = randomInt(10000); + request.setPageParams(new PageParams(from, size)); } return request; } diff --git a/x-pack/plugin/ml/qa/ml-with-security/build.gradle b/x-pack/plugin/ml/qa/ml-with-security/build.gradle index 1146db74510bb..bdbe35ed8b885 100644 --- a/x-pack/plugin/ml/qa/ml-with-security/build.gradle +++ b/x-pack/plugin/ml/qa/ml-with-security/build.gradle @@ -127,7 +127,6 @@ integTest.runner { 'ml/filter_crud/Test create filter api with mismatching body ID', 'ml/filter_crud/Test create filter given invalid filter_id', 'ml/filter_crud/Test get filter API with bad ID', - 'ml/filter_crud/Test invalid param combinations', 'ml/filter_crud/Test non-existing filter', 'ml/filter_crud/Test update filter given remove item is not present', 'ml/filter_crud/Test get all filter given index exists but no mapping for filter_id', diff --git a/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java b/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java index 2d41cf6409700..8a3c4236b7950 100644 --- a/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java +++ b/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java @@ -39,7 +39,6 @@ import org.elasticsearch.xpack.core.ml.action.PostDataAction; import org.elasticsearch.xpack.core.ml.action.PutCalendarAction; import org.elasticsearch.xpack.core.ml.action.PutDatafeedAction; -import org.elasticsearch.xpack.core.ml.action.PutFilterAction; import org.elasticsearch.xpack.core.ml.action.PutJobAction; import org.elasticsearch.xpack.core.ml.action.RevertModelSnapshotAction; import org.elasticsearch.xpack.core.ml.action.StartDatafeedAction; @@ -53,7 +52,6 @@ import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.JobUpdate; -import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndex; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCounts; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSnapshot; @@ -353,10 +351,6 @@ protected List getForecasts(String jobId, ForecastRequestStats forecas return forecasts; } - protected PutFilterAction.Response putMlFilter(MlFilter filter) { - return client().execute(PutFilterAction.INSTANCE, new PutFilterAction.Request(filter)).actionGet(); - } - protected PutCalendarAction.Response putCalendar(String calendarId, List jobIds, String description) { PutCalendarAction.Request request = new PutCalendarAction.Request(new Calendar(calendarId, jobIds, description)); return client().execute(PutCalendarAction.INSTANCE, request).actionGet(); diff --git a/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeIntegTestCase.java b/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeIntegTestCase.java index 8cfc4b16124db..ce14160132cd3 100644 --- a/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeIntegTestCase.java +++ b/x-pack/plugin/ml/qa/native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeIntegTestCase.java @@ -31,12 +31,15 @@ import org.elasticsearch.xpack.core.ml.MlMetadata; import org.elasticsearch.xpack.core.ml.MlTasks; import org.elasticsearch.xpack.core.ml.action.DeleteExpiredDataAction; +import org.elasticsearch.xpack.core.ml.action.GetFiltersAction; import org.elasticsearch.xpack.core.ml.action.OpenJobAction; +import org.elasticsearch.xpack.core.ml.action.PutFilterAction; import org.elasticsearch.xpack.core.ml.action.StartDataFrameAnalyticsAction; import org.elasticsearch.xpack.core.ml.action.StartDatafeedAction; import org.elasticsearch.xpack.core.ml.datafeed.DatafeedState; import org.elasticsearch.xpack.core.ml.dataframe.DataFrameAnalyticsTaskState; import org.elasticsearch.xpack.core.ml.job.config.JobTaskState; +import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.security.SecurityField; import org.elasticsearch.xpack.core.security.authc.TokenMetaData; import org.elasticsearch.xpack.ilm.IndexLifecycle; @@ -136,6 +139,14 @@ protected DeleteExpiredDataAction.Response deleteExpiredData() throws Exception return response; } + protected PutFilterAction.Response putMlFilter(MlFilter filter) { + return client().execute(PutFilterAction.INSTANCE, new PutFilterAction.Request(filter)).actionGet(); + } + + protected GetFiltersAction.Response getMlFilters() { + return client().execute(GetFiltersAction.INSTANCE, new GetFiltersAction.Request()).actionGet(); + } + @Override protected void ensureClusterStateConsistency() throws IOException { if (cluster() != null && cluster().size() > 0) { diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManager.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManager.java index 4845b67bae3f2..205a4292194a7 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManager.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManager.java @@ -357,21 +357,11 @@ public void onFailure(Exception e) { if (updateParams.getFilter() == null) { filterListener.onResponse(null); } else { - GetFiltersAction.Request getFilterRequest = new GetFiltersAction.Request(); - getFilterRequest.setFilterId(updateParams.getFilter().getId()); - executeAsyncWithOrigin(client, ML_ORIGIN, GetFiltersAction.INSTANCE, getFilterRequest, - new ActionListener() { - - @Override - public void onResponse(GetFiltersAction.Response response) { - filterListener.onResponse(response.getFilters().results().get(0)); - } - - @Override - public void onFailure(Exception e) { - handler.accept(e); - } - }); + GetFiltersAction.Request getFilterRequest = new GetFiltersAction.Request(updateParams.getFilter().getId()); + executeAsyncWithOrigin(client, ML_ORIGIN, GetFiltersAction.INSTANCE, getFilterRequest, ActionListener.wrap( + getFilterResponse -> filterListener.onResponse(getFilterResponse.getFilters().results().get(0)), + handler::accept + )); } } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/filter/RestGetFiltersAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/filter/RestGetFiltersAction.java index 2752adfdd5fd3..048de3bbde7f6 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/filter/RestGetFiltersAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/filter/RestGetFiltersAction.java @@ -47,17 +47,17 @@ public String getName() { @Override protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException { - GetFiltersAction.Request getListRequest = new GetFiltersAction.Request(); + GetFiltersAction.Request request = new GetFiltersAction.Request(); String filterId = restRequest.param(MlFilter.ID.getPreferredName()); if (!Strings.isNullOrEmpty(filterId)) { - getListRequest.setFilterId(filterId); + request.setResourceId(filterId); } if (restRequest.hasParam(PageParams.FROM.getPreferredName()) || restRequest.hasParam(PageParams.SIZE.getPreferredName())) { - getListRequest.setPageParams( + request.setPageParams( new PageParams(restRequest.paramAsInt(PageParams.FROM.getPreferredName(), PageParams.DEFAULT_FROM), restRequest.paramAsInt(PageParams.SIZE.getPreferredName(), PageParams.DEFAULT_SIZE))); } - return channel -> client.execute(GetFiltersAction.INSTANCE, getListRequest, new RestStatusToXContentListener<>(channel)); + return channel -> client.execute(GetFiltersAction.INSTANCE, request, new RestStatusToXContentListener<>(channel)); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/MlFiltersIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/MlFiltersIT.java new file mode 100644 index 0000000000000..9cbfbddb851e7 --- /dev/null +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/MlFiltersIT.java @@ -0,0 +1,36 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.ml.integration; + +import org.elasticsearch.xpack.core.ml.action.GetFiltersAction; +import org.elasticsearch.xpack.core.ml.action.PutFilterAction; +import org.elasticsearch.xpack.core.ml.job.config.MlFilter; +import org.elasticsearch.xpack.ml.MlSingleNodeTestCase; +import org.junit.Before; + +import static org.hamcrest.Matchers.equalTo; + +public class MlFiltersIT extends MlSingleNodeTestCase { + + @Before + public void beforeTests() throws Exception { + waitForMlTemplates(); + } + + public void testGetFilters_ShouldReturnUpTo100ByDefault() { + int filtersCount = randomIntBetween(11, 100); + for (int i = 0; i < filtersCount; i++) { + PutFilterAction.Request putFilterRequest = new PutFilterAction.Request( + MlFilter.builder("filter-" + i).setItems("item-" + i).build()); + client().execute(PutFilterAction.INSTANCE, putFilterRequest).actionGet(); + } + + GetFiltersAction.Response filters = client().execute(GetFiltersAction.INSTANCE, new GetFiltersAction.Request()).actionGet(); + assertThat((int) filters.getFilters().count(), equalTo(filtersCount)); + assertThat(filters.getFilters().results().size(), equalTo(filtersCount)); + } +} diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml index d886ec1fb7ff6..3ff3fe5b34bfd 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml @@ -129,28 +129,6 @@ setup: description: "This filter has a description" items: ["123", "lmnop"] ---- -"Test invalid param combinations": - - - do: - catch: bad_request - ml.get_filters: - filter_id: "filter-foo" - from: 0 - - - do: - catch: bad_request - ml.get_filters: - filter_id: "filter-foo" - size: 1 - - - do: - catch: bad_request - ml.get_filters: - filter_id: "filter-foo" - from: 0 - size: 1 - --- "Test create filter given invalid filter_id": - do: