[ML] Add description to ML filters (#31330)

dimitris-athanasiou · dimitris-athanasiou · commit 363ec05df40d · 2018-06-15T16:28:43.000+01:00
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).
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
@@ -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,34 +45,48 @@ private static ObjectParser<Builder, Void> 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<String> items;
 
-    public MlFilter(String id, List<String> items) {
+    public MlFilter(String id, String description, List<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");
     }
 
     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()]));
     }
 
     @Override
     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<String> 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<String> 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<String> 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);
         }
     }
 }
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
@@ -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<Ge
     @Override
     protected Response createTestInstance() {
         final QueryPage<MlFilter> 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
@@ -8,24 +8,15 @@
 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<Request> {
 
     private final String filterId = randomAlphaOfLengthBetween(1, 20);
 
     @Override
     protected Request createTestInstance() {
-        int size = randomInt(10);
-        List<String> 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
@@ -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<String> 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
@@ -385,8 +385,8 @@ public void testGetAutodetectParams() throws Exception {
         indexScheduledEvents(events);
 
         List<MlFilter> 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
@@ -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
@@ -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
@@ -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
@@ -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<MlFilter> 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
@@ -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
@@ -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<String> 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.

Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,7 @@ setup:`
`32`	`32`	`filter_id: filter-foo2`
`33`	`33`	`body: >`
`34`	`34`	`{`
	`35`	`+ "description": "This filter has a description",`
`35`	`36`	`"items": ["123", "lmnop"]`
`36`	`37`	`}`
`37`	`38`
`@@ -76,6 +77,7 @@ setup:`
`76`	`77`	`- match:`
`77`	`78`	`filters.1:`
`78`	`79`	`filter_id: "filter-foo2"`
	`80`	`+ description: "This filter has a description"`
`79`	`81`	`items: ["123", "lmnop"]`
`80`	`82`
`81`	`83`	`- do:`