From 363ec05df40d0fda6d9fbe5f67f060760542f0a9 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 14 Jun 2018 16:52:32 +0100 Subject: [PATCH] [ML] Add description to ML filters (#31330) This adds a `description` to ML filters in order to allow users to describe their filters in a human readable form which is also editable (filter updates to be added shortly). --- .../xpack/core/ml/job/config/MlFilter.java | 47 ++++++++++++++++--- .../action/GetFiltersActionResponseTests.java | 5 +- .../action/PutFilterActionRequestTests.java | 14 +----- .../core/ml/job/config/MlFilterTests.java | 19 ++++++-- .../xpack/ml/integration/JobProviderIT.java | 4 +- .../xpack/ml/job/JobManagerTests.java | 2 +- .../ControlMsgToProcessWriterTests.java | 4 +- .../writer/FieldConfigWriterTests.java | 4 +- .../writer/MlFilterWriterTests.java | 5 +- .../rest-api-spec/test/ml/filter_crud.yml | 2 + .../ml/integration/DetectionRulesIT.java | 6 +-- 11 files changed, 75 insertions(+), 37 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java index de6ee3d509c69..991f421265ea8 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.core.ml.job.config; +import org.elasticsearch.Version; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; @@ -30,6 +31,7 @@ public class MlFilter implements ToXContentObject, Writeable { public static final ParseField TYPE = new ParseField("type"); public static final ParseField ID = new ParseField("filter_id"); + public static final ParseField DESCRIPTION = new ParseField("description"); public static final ParseField ITEMS = new ParseField("items"); // For QueryPage @@ -43,27 +45,38 @@ private static ObjectParser createParser(boolean ignoreUnknownFie parser.declareString((builder, s) -> {}, TYPE); parser.declareString(Builder::setId, ID); + parser.declareStringOrNull(Builder::setDescription, DESCRIPTION); parser.declareStringArray(Builder::setItems, ITEMS); return parser; } private final String id; + private final String description; private final List items; - public MlFilter(String id, List items) { + public MlFilter(String id, String description, List 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"); } public MlFilter(StreamInput in) throws IOException { id = in.readString(); + if (in.getVersion().onOrAfter(Version.V_6_4_0)) { + description = in.readOptionalString(); + } else { + description = null; + } items = Arrays.asList(in.readStringArray()); } @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(id); + if (out.getVersion().onOrAfter(Version.V_6_4_0)) { + out.writeOptionalString(description); + } out.writeStringArray(items.toArray(new String[items.size()])); } @@ -71,6 +84,9 @@ public void writeTo(StreamOutput out) throws IOException { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(ID.getPreferredName(), id); + if (description != null) { + builder.field(DESCRIPTION.getPreferredName(), description); + } builder.field(ITEMS.getPreferredName(), items); if (params.paramAsBoolean(MlMetaIndex.INCLUDE_TYPE_KEY, false)) { builder.field(TYPE.getPreferredName(), FILTER_TYPE); @@ -83,6 +99,10 @@ public String getId() { return id; } + public String getDescription() { + return description; + } + public List getItems() { return new ArrayList<>(items); } @@ -98,12 +118,12 @@ public boolean equals(Object obj) { } MlFilter other = (MlFilter) obj; - return id.equals(other.id) && items.equals(other.items); + return id.equals(other.id) && Objects.equals(description, other.description) && items.equals(other.items); } @Override public int hashCode() { - return Objects.hash(id, items); + return Objects.hash(id, description, items); } public String documentId() { @@ -114,30 +134,45 @@ public static String documentId(String filterId) { return DOCUMENT_ID_PREFIX + filterId; } + public static Builder builder(String filterId) { + return new Builder().setId(filterId); + } + public static class Builder { private String id; + private String description; private List items = Collections.emptyList(); + private Builder() {} + public Builder setId(String id) { this.id = id; return this; } - private Builder() {} - @Nullable public String getId() { return id; } + public Builder setDescription(String description) { + this.description = description; + return this; + } + public Builder setItems(List items) { this.items = items; return this; } + public Builder setItems(String... items) { + this.items = Arrays.asList(items); + return this; + } + public MlFilter build() { - return new MlFilter(id, items); + return new MlFilter(id, description, items); } } } \ No newline at end of file diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionResponseTests.java index c8465c87587e9..7bda0f6e7de76 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/GetFiltersActionResponseTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.xpack.core.ml.action.GetFiltersAction.Response; import org.elasticsearch.xpack.core.ml.action.util.QueryPage; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; +import org.elasticsearch.xpack.core.ml.job.config.MlFilterTests; import java.util.Collections; @@ -17,9 +18,7 @@ public class GetFiltersActionResponseTests extends AbstractStreamableTestCase result; - - MlFilter doc = new MlFilter( - randomAlphaOfLengthBetween(1, 20), Collections.singletonList(randomAlphaOfLengthBetween(1, 20))); + MlFilter doc = MlFilterTests.createRandom(); result = new QueryPage<>(Collections.singletonList(doc), 1, MlFilter.RESULTS_FIELD); return new Response(result); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java index 21845922470f0..dfc3f5f37f40c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java @@ -8,10 +8,7 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractStreamableXContentTestCase; import org.elasticsearch.xpack.core.ml.action.PutFilterAction.Request; -import org.elasticsearch.xpack.core.ml.job.config.MlFilter; - -import java.util.ArrayList; -import java.util.List; +import org.elasticsearch.xpack.core.ml.job.config.MlFilterTests; public class PutFilterActionRequestTests extends AbstractStreamableXContentTestCase { @@ -19,13 +16,7 @@ public class PutFilterActionRequestTests extends AbstractStreamableXContentTestC @Override protected Request createTestInstance() { - int size = randomInt(10); - List items = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - items.add(randomAlphaOfLengthBetween(1, 20)); - } - MlFilter filter = new MlFilter(filterId, items); - return new PutFilterAction.Request(filter); + return new PutFilterAction.Request(MlFilterTests.createRandom(filterId)); } @Override @@ -42,5 +33,4 @@ protected Request createBlankInstance() { protected Request doParseInstance(XContentParser parser) { return PutFilterAction.Request.parseRequest(filterId, parser); } - } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java index 1b61e3ec9a43d..78d87b82839a2 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java @@ -26,12 +26,25 @@ public static MlFilter createTestFilter() { @Override protected MlFilter createTestInstance() { + return createRandom(); + } + + public static MlFilter createRandom() { + return createRandom(randomAlphaOfLengthBetween(1, 20)); + } + + public static MlFilter createRandom(String filterId) { + String description = null; + if (randomBoolean()) { + description = randomAlphaOfLength(20); + } + int size = randomInt(10); List items = new ArrayList<>(size); for (int i = 0; i < size; i++) { items.add(randomAlphaOfLengthBetween(1, 20)); } - return new MlFilter(randomAlphaOfLengthBetween(1, 20), items); + return new MlFilter(filterId, description, items); } @Override @@ -45,13 +58,13 @@ protected MlFilter doParseInstance(XContentParser parser) { } public void testNullId() { - NullPointerException ex = expectThrows(NullPointerException.class, () -> new MlFilter(null, Collections.emptyList())); + NullPointerException ex = expectThrows(NullPointerException.class, () -> new MlFilter(null, "", Collections.emptyList())); assertEquals(MlFilter.ID.getPreferredName() + " must not be null", ex.getMessage()); } public void testNullItems() { NullPointerException ex = - expectThrows(NullPointerException.class, () -> new MlFilter(randomAlphaOfLengthBetween(1, 20), null)); + expectThrows(NullPointerException.class, () -> new MlFilter(randomAlphaOfLengthBetween(1, 20), "", null)); assertEquals(MlFilter.ITEMS.getPreferredName() + " must not be null", ex.getMessage()); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index 7e0dc453f07ee..856b930ac49b5 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -385,8 +385,8 @@ public void testGetAutodetectParams() throws Exception { indexScheduledEvents(events); List filters = new ArrayList<>(); - filters.add(new MlFilter("fruit", Arrays.asList("apple", "pear"))); - filters.add(new MlFilter("tea", Arrays.asList("green", "builders"))); + filters.add(MlFilter.builder("fruit").setItems("apple", "pear").build()); + filters.add(MlFilter.builder("tea").setItems("green", "builders").build()); indexFilters(filters); DataCounts earliestCounts = DataCountsTests.createTestInstance(jobId); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java index 454f941d6c8b0..42b0a56f49a82 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java @@ -210,7 +210,7 @@ public void testUpdateProcessOnFilterChanged() { JobManager jobManager = createJobManager(); - MlFilter filter = new MlFilter("foo_filter", Arrays.asList("a", "b")); + MlFilter filter = MlFilter.builder("foo_filter").setItems("a", "b").build(); jobManager.updateProcessOnFilterChanged(filter); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java index 8c32a5bb40d46..3d08f5a1c25fb 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java @@ -207,8 +207,8 @@ public void testWriteUpdateDetectorRulesMessage() throws IOException { public void testWriteUpdateFiltersMessage() throws IOException { ControlMsgToProcessWriter writer = new ControlMsgToProcessWriter(lengthEncodedWriter, 2); - MlFilter filter1 = new MlFilter("filter_1", Arrays.asList("a")); - MlFilter filter2 = new MlFilter("filter_2", Arrays.asList("b", "c")); + MlFilter filter1 = MlFilter.builder("filter_1").setItems("a").build(); + MlFilter filter2 = MlFilter.builder("filter_2").setItems("b", "c").build(); writer.writeUpdateFiltersMessage(Arrays.asList(filter1, filter2)); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java index bf08d09bf090c..d26dbb203c84e 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java @@ -220,8 +220,8 @@ public void testWrite_GivenFilters() throws IOException { AnalysisConfig.Builder builder = new AnalysisConfig.Builder(Collections.singletonList(d)); analysisConfig = builder.build(); - filters.add(new MlFilter("filter_1", Arrays.asList("a", "b"))); - filters.add(new MlFilter("filter_2", Arrays.asList("c", "d"))); + filters.add(MlFilter.builder("filter_1").setItems("a", "b").build()); + filters.add(MlFilter.builder("filter_2").setItems("c", "d").build()); writer = mock(OutputStreamWriter.class); createFieldConfigWriter().write(); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/MlFilterWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/MlFilterWriterTests.java index f22f7d85090be..12ceb12f46223 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/MlFilterWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/MlFilterWriterTests.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -28,8 +27,8 @@ public void testWrite_GivenEmpty() throws IOException { public void testWrite() throws IOException { List filters = new ArrayList<>(); - filters.add(new MlFilter("filter_1", Arrays.asList("a", "b"))); - filters.add(new MlFilter("filter_2", Arrays.asList("c", "d"))); + filters.add(MlFilter.builder("filter_1").setItems("a", "b").build()); + filters.add(MlFilter.builder("filter_2").setItems("c", "d").build()); StringBuilder buffer = new StringBuilder(); new MlFilterWriter(filters, buffer).write(); 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 d3165260f4b95..a1f7eee0dcc3d 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 @@ -32,6 +32,7 @@ setup: filter_id: filter-foo2 body: > { + "description": "This filter has a description", "items": ["123", "lmnop"] } @@ -76,6 +77,7 @@ setup: - match: filters.1: filter_id: "filter-foo2" + description: "This filter has a description" items: ["123", "lmnop"] - do: diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java index aa53d6255cb8e..b99170546df3b 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java @@ -120,7 +120,7 @@ public void testCondition() throws Exception { } public void testScope() throws Exception { - MlFilter safeIps = new MlFilter("safe_ips", Arrays.asList("111.111.111.111", "222.222.222.222")); + MlFilter safeIps = MlFilter.builder("safe_ips").setItems("111.111.111.111", "222.222.222.222").build(); assertThat(putMlFilter(safeIps), is(true)); DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")).build(); @@ -178,7 +178,7 @@ public void testScope() throws Exception { assertThat(records.get(0).getOverFieldValue(), equalTo("333.333.333.333")); // Now let's update the filter - MlFilter updatedFilter = new MlFilter(safeIps.getId(), Collections.singletonList("333.333.333.333")); + MlFilter updatedFilter = MlFilter.builder(safeIps.getId()).setItems("333.333.333.333").build(); assertThat(putMlFilter(updatedFilter), is(true)); // Wait until the notification that the process was updated is indexed @@ -229,7 +229,7 @@ public void testScope() throws Exception { public void testScopeAndCondition() throws IOException { // We have 2 IPs and they're both safe-listed. List ips = Arrays.asList("111.111.111.111", "222.222.222.222"); - MlFilter safeIps = new MlFilter("safe_ips", ips); + MlFilter safeIps = MlFilter.builder("safe_ips").setItems(ips).build(); assertThat(putMlFilter(safeIps), is(true)); // Ignore if ip in safe list AND actual < 10.