Fix routing with leading or trailing whitespace

buildSrc/src/main/resources/checkstyle_suppressions.xml

@@ -658,7 +658,6 @@
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]recovery[/\\]RelocationIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]recovery[/\\]TruncatedRecoveryIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]rest[/\\]BytesRestResponseTests.java" checks="LineLength" />
-  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]AliasResolveRoutingIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]AliasRoutingIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]SimpleRoutingIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]script[/\\]FileScriptTests.java" checks="LineLength" />

core/src/main/java/org/elasticsearch/cluster/metadata/AliasMetaData.java

@@ -27,6 +27,7 @@
 import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
@@ -58,7 +59,7 @@ private AliasMetaData(String alias, CompressedXContent filter, String indexRouti
         this.indexRouting = indexRouting;
         this.searchRouting = searchRouting;
         if (searchRouting != null) {
-            searchRoutingValues = Collections.unmodifiableSet(Strings.splitStringByCommaToSet(searchRouting));
+            searchRoutingValues = Collections.unmodifiableSet(Sets.newHashSet(Strings.splitStringByCommaToArray(searchRouting)));
         } else {
             searchRoutingValues = emptySet();
         }
@@ -186,7 +187,7 @@ public AliasMetaData(StreamInput in) throws IOException {
         }
         if (in.readBoolean()) {
             searchRouting = in.readString();
-            searchRoutingValues = Collections.unmodifiableSet(Strings.splitStringByCommaToSet(searchRouting));
+            searchRoutingValues = Collections.unmodifiableSet(Sets.newHashSet(Strings.splitStringByCommaToArray(searchRouting)));
         } else {
             searchRouting = null;
             searchRoutingValues = emptySet();

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

@@ -31,6 +31,7 @@
 import org.elasticsearch.common.joda.FormatDateTimeFormatter;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.indices.IndexClosedException;
@@ -358,6 +359,7 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
             resolvedExpressions = expressionResolver.resolve(context, resolvedExpressions);
         }
 
+        // TODO: it appears that this can never be true?
         if (isAllIndices(resolvedExpressions)) {
             return resolveSearchRoutingAllIndices(state.metaData(), routing);
         }
@@ -367,7 +369,7 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
         // List of indices that don't require any routing
         Set<String> norouting = new HashSet<>();
         if (routing != null) {
-            paramRouting = Strings.splitStringByCommaToSet(routing);
+            paramRouting = Sets.newHashSet(Strings.splitStringByCommaToArray(routing));
         }
 
         for (String expression : resolvedExpressions) {
@@ -442,9 +444,9 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
     /**
      * Sets the same routing for all indices
      */
-    private Map<String, Set<String>> resolveSearchRoutingAllIndices(MetaData metaData, String routing) {
+    Map<String, Set<String>> resolveSearchRoutingAllIndices(MetaData metaData, String routing) {
         if (routing != null) {
-            Set<String> r = Strings.splitStringByCommaToSet(routing);
+            Set<String> r = Sets.newHashSet(Strings.splitStringByCommaToArray(routing));
             Map<String, Set<String>> routings = new HashMap<>();
             String[] concreteIndices = metaData.getConcreteAllIndices();
             for (String index : concreteIndices) {

core/src/test/java/org/elasticsearch/cluster/metadata/AliasMetaDataTests.java

@@ -0,0 +1,55 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cluster.metadata;
+
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.util.set.Sets;
+import org.elasticsearch.common.xcontent.XContent;
+import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.test.ESTestCase;
+
+import java.io.IOException;
+
+import static org.hamcrest.Matchers.equalTo;
+
+public class AliasMetaDataTests extends ESTestCase {
+
+    public void testSerialization() throws IOException {
+        final AliasMetaData before =
+                AliasMetaData
+                        .builder("alias")
+                        .filter("{ \"term\": \"foo\"}")
+                        .indexRouting("indexRouting")
+                        .routing("routing")
+                        .searchRouting("trim,tw , ltw , lw")
+                        .build();
+
+        assertThat(before.searchRoutingValues(), equalTo(Sets.newHashSet("trim", "tw ", " ltw ", " lw")));
+
+        final BytesStreamOutput out = new BytesStreamOutput();
+        before.writeTo(out);
+
+        final StreamInput in = out.bytes().streamInput();
+        final AliasMetaData after = new AliasMetaData(in);
+
+        assertThat(after, equalTo(before));
+    }
+}

core/src/test/java/org/elasticsearch/routing/AliasResolveRoutingIT.java

@@ -32,15 +32,13 @@
 import java.util.concurrent.ExecutionException;
 
 import static org.elasticsearch.common.util.set.Sets.newHashSet;
-import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
 import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 
 public class AliasResolveRoutingIT extends ESIntegTestCase {
 
-
     // see https://github.com/elastic/elasticsearch/issues/13278
     public void testSearchClosedWildcardIndex() throws ExecutionException, InterruptedException {
         createIndex("test-0");
@@ -52,10 +50,17 @@ public void testSearchClosedWildcardIndex() throws ExecutionException, Interrupt
             client().prepareIndex("test-0", "type1", "2").setSource("field1", "quick brown"),
             client().prepareIndex("test-0", "type1", "3").setSource("field1", "quick"));
         refresh("test-*");
-        assertHitCount(client().prepareSearch().setIndices("alias-*").setIndicesOptions(IndicesOptions.lenientExpandOpen()).setQuery(queryStringQuery("quick")).get(), 3L);
+        assertHitCount(
+                client()
+                        .prepareSearch()
+                        .setIndices("alias-*")
+                        .setIndicesOptions(IndicesOptions.lenientExpandOpen())
+                        .setQuery(queryStringQuery("quick"))
+                        .get(),
+                3L);
     }
 
-    public void testResolveIndexRouting() throws Exception {
+    public void testResolveIndexRouting() {
         createIndex("test1");
         createIndex("test2");
         client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();
@@ -97,9 +102,10 @@ public void testResolveIndexRouting() throws Exception {
         }
     }
 
-    public void testResolveSearchRouting() throws Exception {
+    public void testResolveSearchRouting() {
         createIndex("test1");
         createIndex("test2");
+        createIndex("test3");
         client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();
 
         client().admin().indices().prepareAliases()
@@ -108,7 +114,10 @@ public void testResolveSearchRouting() throws Exception {
                 .addAliasAction(AliasActions.add().index("test2").alias("alias20").routing("0"))
                 .addAliasAction(AliasActions.add().index("test2").alias("alias21").routing("1"))
                 .addAliasAction(AliasActions.add().index("test1").alias("alias0").routing("0"))
-                .addAliasAction(AliasActions.add().index("test2").alias("alias0").routing("0")).get();
+                .addAliasAction(AliasActions.add().index("test2").alias("alias0").routing("0"))
+                .addAliasAction(AliasActions.add().index("test3").alias("alias3tw").routing("tw "))
+                .addAliasAction(AliasActions.add().index("test3").alias("alias3ltw").routing(" ltw "))
+                .addAliasAction(AliasActions.add().index("test3").alias("alias3lw").routing(" lw")).get();
 
         ClusterState state = clusterService().state();
         IndexNameExpressionResolver indexNameExpressionResolver = internalCluster().getInstance(IndexNameExpressionResolver.class);
@@ -118,7 +127,9 @@ public void testResolveSearchRouting() throws Exception {
         assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, "alias10"), equalTo(newMap("test1", newSet("0"))));
         assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "0", "alias10"), equalTo(newMap("test1", newSet("0"))));
         assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "1", "alias10"), nullValue());
-        assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, "alias0"), equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, null, "alias0"),
+                equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
 
         assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, new String[]{"alias10", "alias20"}),
                 equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
@@ -143,13 +154,35 @@ public void testResolveSearchRouting() throws Exception {
                 equalTo(newMap("test1", newSet("0"), "test2", newSet("1"))));
         assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "0,1,2", new String[]{"test1", "alias10", "alias21"}),
                 equalTo(newMap("test1", newSet("0", "1", "2"), "test2", newSet("1"))));
+
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "test1"),
+                equalTo(newMap("test1", newSet("tw ", " ltw ", " lw"))));
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3tw"),
+                equalTo(newMap("test3", newSet("tw "))));
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3ltw"),
+                equalTo(newMap("test3", newSet(" ltw "))));
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3lw"),
+                equalTo(newMap("test3", newSet(" lw"))));
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "0,tw , ltw , lw", "test1", "alias3ltw"),
+                equalTo(newMap("test1", newSet("0", "tw ", " ltw ", " lw"), "test3", newSet(" ltw "))));
+
+        assertThat(
+                indexNameExpressionResolver.resolveSearchRouting(state, "0,1,2,tw , ltw , lw", (String[])null),
+                equalTo(newMap(
+                        "test1", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
+                        "test2", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
+                        "test3", newSet("0", "1", "2", "tw ", " ltw ", " lw"))));
     }
 
     private <T> Set<T> newSet(T... elements) {
         return newHashSet(elements);
     }
 
-
     private <K, V> Map<K, V> newMap(K key, V value) {
         Map<K, V> r = new HashMap<>();
         r.put(key, value);
@@ -163,4 +196,12 @@ private <K, V> Map<K, V> newMap(K key1, V value1, K key2, V value2) {
         return r;
     }
 
+    private <K, V> Map<K, V> newMap(K key1, V value1, K key2, V value2, K key3, V value3) {
+        Map<K, V> r = new HashMap<>();
+        r.put(key1, value1);
+        r.put(key2, value2);
+        r.put(key3, value3);
+        return r;
+    }
+
 }