From 4dcf96c37c3a358279aeac4c84689063b2f8a268 Mon Sep 17 00:00:00 2001
From: Dimitris Athanasiou <dimitris@elastic.co>
Date: Thu, 14 Jun 2018 15:46:17 +0100
Subject: [PATCH] [ML] Hold ML filter items in sorted set

Filter items should be unique. They should also
be sorted to make them easier to read plus save
sorting in the autodetect process.
---
 .../xpack/core/ml/job/config/MlFilter.java    | 21 +++++++++++--------
 .../core/ml/job/config/MlFilterTests.java     | 19 ++++++++++++-----
 .../rest-api-spec/test/ml/filter_crud.yml     |  2 +-
 3 files changed, 27 insertions(+), 15 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 991f421265ea8..b11dfd476515c 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
@@ -17,11 +17,12 @@
 import org.elasticsearch.xpack.core.ml.MlMetaIndex;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
+import java.util.SortedSet;
+import java.util.TreeSet;
 
 public class MlFilter implements ToXContentObject, Writeable {
 
@@ -53,9 +54,9 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie
 
     private final String id;
     private final String description;
-    private final List<String> items;
+    private final SortedSet<String> items;
 
-    public MlFilter(String id, String description, List<String> items) {
+    public 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");
@@ -68,7 +69,8 @@ public MlFilter(StreamInput in) throws IOException {
         } else {
             description = null;
         }
-        items = Arrays.asList(in.readStringArray());
+        items = new TreeSet<>();
+        items.addAll(Arrays.asList(in.readStringArray()));
     }
 
     @Override
@@ -103,8 +105,8 @@ public String getDescription() {
         return description;
     }
 
-    public List<String> getItems() {
-        return new ArrayList<>(items);
+    public SortedSet<String> getItems() {
+        return Collections.unmodifiableSortedSet(items);
     }
 
     @Override
@@ -142,7 +144,7 @@ public static class Builder {
 
         private String id;
         private String description;
-        private List<String> items = Collections.emptyList();
+        private SortedSet<String> items = new TreeSet<>();
 
         private Builder() {}
 
@@ -162,12 +164,13 @@ public Builder setDescription(String description) {
         }
 
         public Builder setItems(List<String> items) {
-            this.items = items;
+            this.items = new TreeSet<>();
+            this.items.addAll(items);
             return this;
         }
 
         public Builder setItems(String... items) {
-            this.items = Arrays.asList(items);
+            setItems(Arrays.asList(items));
             return this;
         }
 
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 78d87b82839a2..9ac6683f004c5 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
@@ -11,10 +11,9 @@
 import org.elasticsearch.test.AbstractSerializingTestCase;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
+import java.util.TreeSet;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
@@ -40,7 +39,7 @@ public static MlFilter createRandom(String filterId) {
         }
 
         int size = randomInt(10);
-        List<String> items = new ArrayList<>(size);
+        TreeSet<String> items = new TreeSet<>();
         for (int i = 0; i < size; i++) {
             items.add(randomAlphaOfLengthBetween(1, 20));
         }
@@ -58,7 +57,7 @@ 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, "", new TreeSet<>()));
         assertEquals(MlFilter.ID.getPreferredName() + " must not be null", ex.getMessage());
     }
 
@@ -88,4 +87,14 @@ public void testLenientParser() throws IOException {
             MlFilter.LENIENT_PARSER.apply(parser, null);
         }
     }
+
+    public void testItemsAreSorted() {
+        MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build();
+        assertThat(filter.getItems(), contains("a", "b", "c"));
+    }
+
+    public void testGetItemsReturnsUnmodifiable() {
+        MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build();
+        expectThrows(UnsupportedOperationException.class, () -> filter.getItems().add("x"));
+    }
 }
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 a1f7eee0dcc3d..2b7b86673e0d0 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
@@ -22,7 +22,7 @@ setup:
         filter_id: filter-foo
         body:  >
           {
-            "items": ["abc", "xyz"]
+            "items": ["xyz", "abc"]
           }
 
   - do: