Skip to content

Commit

Permalink
[7.7][ML] Get ML filters size should default to 100 (#54207) (#54279)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dimitris-athanasiou authored Mar 26, 2020
1 parent 7491f7f commit 1e83b29
Show file tree
Hide file tree
Showing 9 changed files with 68 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,18 @@
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;
import org.elasticsearch.common.xcontent.StatusToXContentObject;
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<GetFiltersAction.Response> {

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Request> {


@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;
}
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugin/ml/qa/ml-with-security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -353,10 +351,6 @@ protected List<Forecast> 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<String> jobIds, String description) {
PutCalendarAction.Request request = new PutCalendarAction.Request(new Calendar(calendarId, jobIds, description));
return client().execute(PutCalendarAction.INSTANCE, request).actionGet();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<GetFiltersAction.Response>() {

@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
));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}
Original file line number Diff line number Diff line change
@@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 1e83b29

Please sign in to comment.