Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Add ML filter update API #31437

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import org.elasticsearch.xpack.core.ml.action.StopDatafeedAction;
import org.elasticsearch.xpack.core.ml.action.UpdateCalendarJobAction;
import org.elasticsearch.xpack.core.ml.action.UpdateDatafeedAction;
import org.elasticsearch.xpack.core.ml.action.UpdateFilterAction;
import org.elasticsearch.xpack.core.ml.action.UpdateJobAction;
import org.elasticsearch.xpack.core.ml.action.UpdateModelSnapshotAction;
import org.elasticsearch.xpack.core.ml.action.UpdateProcessAction;
Expand Down Expand Up @@ -220,6 +221,7 @@ public List<Action> getClientActions() {
OpenJobAction.INSTANCE,
GetFiltersAction.INSTANCE,
PutFilterAction.INSTANCE,
UpdateFilterAction.INSTANCE,
DeleteFilterAction.INSTANCE,
KillProcessAction.INSTANCE,
GetBucketsAction.INSTANCE,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
/*
* 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.core.ml.action;

import org.elasticsearch.action.Action;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestBuilder;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.client.ElasticsearchClient;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.xpack.core.ml.job.config.MlFilter;
import org.elasticsearch.xpack.core.ml.job.messages.Messages;
import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Objects;
import java.util.SortedSet;
import java.util.TreeSet;


public class UpdateFilterAction extends Action<PutFilterAction.Response> {

public static final UpdateFilterAction INSTANCE = new UpdateFilterAction();
public static final String NAME = "cluster:admin/xpack/ml/filters/update";

private UpdateFilterAction() {
super(NAME);
}

@Override
public PutFilterAction.Response newResponse() {
return new PutFilterAction.Response();
}

public static class Request extends ActionRequest implements ToXContentObject {

public static final ParseField ADD_ITEMS = new ParseField("add_items");
public static final ParseField REMOVE_ITEMS = new ParseField("remove_items");

private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>(NAME, Request::new);

static {
PARSER.declareString((request, filterId) -> request.filterId = filterId, MlFilter.ID);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd that it doesn't have a setter. Can you use the ConstructingObjectParser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we follow this pattern in a few request objects where there is a field that will also be in the URI. While it seems odd, it saves us from having to relax the null check in the constructor. At the end, it makes the java API cleaner at the cost of having that funky lambda in the parser.

PARSER.declareStringOrNull(Request::setDescription, MlFilter.DESCRIPTION);
PARSER.declareStringArray(Request::setAddItems, ADD_ITEMS);
PARSER.declareStringArray(Request::setRemoveItems, REMOVE_ITEMS);
}

public static Request parseRequest(String filterId, XContentParser parser) {
Request request = PARSER.apply(parser, null);
if (request.filterId == null) {
request.filterId = filterId;
} else if (!Strings.isNullOrEmpty(filterId) && !filterId.equals(request.filterId)) {
// If we have both URI and body filter ID, they must be identical
throw new IllegalArgumentException(Messages.getMessage(Messages.INCONSISTENT_ID, MlFilter.ID.getPreferredName(),
request.filterId, filterId));
}
return request;
}

private String filterId;
@Nullable
private String description;
private SortedSet<String> addItems = Collections.emptySortedSet();
private SortedSet<String> removeItems = Collections.emptySortedSet();

public Request() {
}

public Request(String filterId) {
this.filterId = ExceptionsHelper.requireNonNull(filterId, MlFilter.ID.getPreferredName());
}

public String getFilterId() {
return filterId;
}

public String getDescription() {
return description;
}

public void setDescription(String description) {
this.description = description;
}

public SortedSet<String> getAddItems() {
return addItems;
}

public void setAddItems(Collection<String> addItems) {
this.addItems = new TreeSet<>(ExceptionsHelper.requireNonNull(addItems, ADD_ITEMS.getPreferredName()));
}

public SortedSet<String> getRemoveItems() {
return removeItems;
}

public void setRemoveItems(Collection<String> removeItems) {
this.removeItems = new TreeSet<>(ExceptionsHelper.requireNonNull(removeItems, REMOVE_ITEMS.getPreferredName()));
}

public boolean isNoop() {
return description == null && addItems.isEmpty() && removeItems.isEmpty();
}

@Override
public ActionRequestValidationException validate() {
return null;
}

@Override
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
filterId = in.readString();
description = in.readOptionalString();
addItems = new TreeSet<>(Arrays.asList(in.readStringArray()));
removeItems = new TreeSet<>(Arrays.asList(in.readStringArray()));
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeString(filterId);
out.writeOptionalString(description);
out.writeStringArray(addItems.toArray(new String[addItems.size()]));
out.writeStringArray(removeItems.toArray(new String[removeItems.size()]));
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field(MlFilter.ID.getPreferredName(), filterId);
if (description != null) {
builder.field(MlFilter.DESCRIPTION.getPreferredName(), description);
}
if (addItems.isEmpty() == false) {
builder.field(ADD_ITEMS.getPreferredName(), addItems);
}
if (removeItems.isEmpty() == false) {
builder.field(REMOVE_ITEMS.getPreferredName(), removeItems);
}
builder.endObject();
return builder;
}

@Override
public int hashCode() {
return Objects.hash(filterId, description, addItems, removeItems);
}

@Override
public boolean equals(Object obj) {
if (obj == null) {
return false;
}
if (getClass() != obj.getClass()) {
return false;
}
Request other = (Request) obj;
return Objects.equals(filterId, other.filterId)
&& Objects.equals(description, other.description)
&& Objects.equals(addItems, other.addItems)
&& Objects.equals(removeItems, other.removeItems);
}
}

public static class RequestBuilder extends ActionRequestBuilder<Request, PutFilterAction.Response> {

public RequestBuilder(ElasticsearchClient client) {
super(client, INSTANCE, new Request());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie
private final String description;
private final SortedSet<String> items;

public MlFilter(String id, String description, SortedSet<String> items) {
private MlFilter(String id, String description, SortedSet<String> items) {
this.id = Objects.requireNonNull(id, ID.getPreferredName() + " must not be null");
this.description = description;
this.items = Objects.requireNonNull(items, ITEMS.getPreferredName() + " must not be null");
Expand All @@ -69,8 +69,7 @@ public MlFilter(StreamInput in) throws IOException {
} else {
description = null;
}
items = new TreeSet<>();
items.addAll(Arrays.asList(in.readStringArray()));
items = new TreeSet<>(Arrays.asList(in.readStringArray()));
}

@Override
Expand Down Expand Up @@ -163,9 +162,13 @@ public Builder setDescription(String description) {
return this;
}

public Builder setItems(SortedSet<String> items) {
this.items = items;
return this;
}

public Builder setItems(List<String> items) {
this.items = new TreeSet<>();
this.items.addAll(items);
this.items = new TreeSet<>(items);
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public final class Messages {
public static final String DATAFEED_FREQUENCY_MUST_BE_MULTIPLE_OF_AGGREGATIONS_INTERVAL =
"Datafeed frequency [{0}] must be a multiple of the aggregation interval [{1}]";

public static final String FILTER_NOT_FOUND = "No filter with id [{0}] exists";

public static final String INCONSISTENT_ID =
"Inconsistent {0}; ''{1}'' specified in the body differs from ''{2}'' specified as a URL argument";
public static final String INVALID_ID = "Invalid {0}; ''{1}'' can contain lowercase alphanumeric (a-z and 0-9), hyphens or " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentParser.Token;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.xpack.core.ml.job.config.Job;
import org.elasticsearch.xpack.core.ml.utils.time.TimeUtils;

Expand Down Expand Up @@ -345,7 +345,7 @@ public static String v54DocumentId(String jobId, String snapshotId) {

public static ModelSnapshot fromJson(BytesReference bytesReference) {
try (InputStream stream = bytesReference.streamInput();
XContentParser parser = XContentFactory.xContent(XContentHelper.xContentType(bytesReference))
XContentParser parser = XContentFactory.xContent(XContentType.JSON)
.createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, stream)) {
return LENIENT_PARSER.apply(parser, null).build();
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ public static ElasticsearchException serverError(String msg, Throwable cause) {
return new ElasticsearchException(msg, cause);
}

public static ElasticsearchStatusException conflictStatusException(String msg, Throwable cause, Object... args) {
return new ElasticsearchStatusException(msg, RestStatus.CONFLICT, cause, args);
}

public static ElasticsearchStatusException conflictStatusException(String msg, Object... args) {
return new ElasticsearchStatusException(msg, RestStatus.CONFLICT, args);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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.core.ml.action;

import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractStreamableXContentTestCase;
import org.elasticsearch.xpack.core.ml.action.UpdateFilterAction.Request;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;

public class UpdateFilterActionRequestTests extends AbstractStreamableXContentTestCase<Request> {

private String filterId = randomAlphaOfLength(20);

@Override
protected Request createTestInstance() {
UpdateFilterAction.Request request = new UpdateFilterAction.Request(filterId);
if (randomBoolean()) {
request.setDescription(randomAlphaOfLength(20));
}
if (randomBoolean()) {
request.setAddItems(generateRandomStrings());
}
if (randomBoolean()) {
request.setRemoveItems(generateRandomStrings());
}
return request;
}

private static Collection<String> generateRandomStrings() {
int size = randomIntBetween(0, 10);
List<String> strings = new ArrayList<>(size);
for (int i = 0; i < size; ++i) {
strings.add(randomAlphaOfLength(20));
}
return strings;
}

@Override
protected boolean supportsUnknownFields() {
return false;
}

@Override
protected Request createBlankInstance() {
return new Request();
}

@Override
protected Request doParseInstance(XContentParser parser) {
return Request.parseRequest(filterId, parser);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.test.AbstractSerializingTestCase;

import java.io.IOException;
import java.util.SortedSet;
import java.util.TreeSet;

import static org.hamcrest.Matchers.contains;
Expand Down Expand Up @@ -43,7 +44,7 @@ public static MlFilter createRandom(String filterId) {
for (int i = 0; i < size; i++) {
items.add(randomAlphaOfLengthBetween(1, 20));
}
return new MlFilter(filterId, description, items);
return MlFilter.builder(filterId).setDescription(description).setItems(items).build();
}

@Override
Expand All @@ -57,13 +58,13 @@ protected MlFilter doParseInstance(XContentParser parser) {
}

public void testNullId() {
NullPointerException ex = expectThrows(NullPointerException.class, () -> new MlFilter(null, "", new TreeSet<>()));
NullPointerException ex = expectThrows(NullPointerException.class, () -> MlFilter.builder(null).build());
assertEquals(MlFilter.ID.getPreferredName() + " must not be null", ex.getMessage());
}

public void testNullItems() {
NullPointerException ex =
expectThrows(NullPointerException.class, () -> new MlFilter(randomAlphaOfLengthBetween(1, 20), "", null));
NullPointerException ex = expectThrows(NullPointerException.class,
() -> MlFilter.builder(randomAlphaOfLength(20)).setItems((SortedSet<String>) null).build());
assertEquals(MlFilter.ITEMS.getPreferredName() + " must not be null", ex.getMessage());
}

Expand Down
Loading