Skip to content

Commit ec5e540

Browse files
jasontedorjasontedor
authored
Fix routing with leading or trailing whitespace
The problem here is that splitting was using a method that intentionally trims whitespace (the method is really meant to be used for splitting parameters where whitespace should be trimmed like list settings). However, for routing values whitespace should not be trimmed because we allow routing with leading and trailing spaces. This commit switches the parsing of these routing values to a method that does not trim whitespace. Relates #27712
1 parent 64dd21a commit ec5e540

File tree

5 files changed

+120
-14
lines changed

5 files changed

+120
-14
lines changed

‎buildSrc/src/main/resources/checkstyle_suppressions.xml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,6 @@
658658
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]recovery[/\\]RelocationIT.java" checks="LineLength" />
659659
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]recovery[/\\]TruncatedRecoveryIT.java" checks="LineLength" />
660660
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]rest[/\\]BytesRestResponseTests.java" checks="LineLength" />
661-
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]AliasResolveRoutingIT.java" checks="LineLength" />
662661
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]AliasRoutingIT.java" checks="LineLength" />
663662
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]routing[/\\]SimpleRoutingIT.java" checks="LineLength" />
664663
<suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]script[/\\]FileScriptTests.java" checks="LineLength" />

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.elasticsearch.common.compress.CompressedXContent;
2828
import org.elasticsearch.common.io.stream.StreamInput;
2929
import org.elasticsearch.common.io.stream.StreamOutput;
30+
import org.elasticsearch.common.util.set.Sets;
3031
import org.elasticsearch.common.xcontent.ToXContent;
3132
import org.elasticsearch.common.xcontent.XContentBuilder;
3233
import org.elasticsearch.common.xcontent.XContentFactory;
@@ -58,7 +59,7 @@ private AliasMetaData(String alias, CompressedXContent filter, String indexRouti
5859
this.indexRouting = indexRouting;
5960
this.searchRouting = searchRouting;
6061
if (searchRouting != null) {
61-
searchRoutingValues = Collections.unmodifiableSet(Strings.splitStringByCommaToSet(searchRouting));
62+
searchRoutingValues = Collections.unmodifiableSet(Sets.newHashSet(Strings.splitStringByCommaToArray(searchRouting)));
6263
} else {
6364
searchRoutingValues = emptySet();
6465
}
@@ -186,7 +187,7 @@ public AliasMetaData(StreamInput in) throws IOException {
186187
}
187188
if (in.readBoolean()) {
188189
searchRouting = in.readString();
189-
searchRoutingValues = Collections.unmodifiableSet(Strings.splitStringByCommaToSet(searchRouting));
190+
searchRoutingValues = Collections.unmodifiableSet(Sets.newHashSet(Strings.splitStringByCommaToArray(searchRouting)));
190191
} else {
191192
searchRouting = null;
192193
searchRoutingValues = emptySet();

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.elasticsearch.common.joda.FormatDateTimeFormatter;
3232
import org.elasticsearch.common.regex.Regex;
3333
import org.elasticsearch.common.settings.Settings;
34+
import org.elasticsearch.common.util.set.Sets;
3435
import org.elasticsearch.index.Index;
3536
import org.elasticsearch.index.IndexNotFoundException;
3637
import org.elasticsearch.indices.IndexClosedException;
@@ -358,6 +359,7 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
358359
resolvedExpressions = expressionResolver.resolve(context, resolvedExpressions);
359360
}
360361

362+
// TODO: it appears that this can never be true?
361363
if (isAllIndices(resolvedExpressions)) {
362364
return resolveSearchRoutingAllIndices(state.metaData(), routing);
363365
}
@@ -367,7 +369,7 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
367369
// List of indices that don't require any routing
368370
Set<String> norouting = new HashSet<>();
369371
if (routing != null) {
370-
paramRouting = Strings.splitStringByCommaToSet(routing);
372+
paramRouting = Sets.newHashSet(Strings.splitStringByCommaToArray(routing));
371373
}
372374

373375
for (String expression : resolvedExpressions) {
@@ -442,9 +444,9 @@ public Map<String, Set<String>> resolveSearchRouting(ClusterState state, @Nullab
442444
/**
443445
* Sets the same routing for all indices
444446
*/
445-
private Map<String, Set<String>> resolveSearchRoutingAllIndices(MetaData metaData, String routing) {
447+
public Map<String, Set<String>> resolveSearchRoutingAllIndices(MetaData metaData, String routing) {
446448
if (routing != null) {
447-
Set<String> r = Strings.splitStringByCommaToSet(routing);
449+
Set<String> r = Sets.newHashSet(Strings.splitStringByCommaToArray(routing));
448450
Map<String, Set<String>> routings = new HashMap<>();
449451
String[] concreteIndices = metaData.getConcreteAllIndices();
450452
for (String index : concreteIndices) {

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.cluster.metadata;
21+
22+
import org.elasticsearch.common.io.stream.BytesStreamOutput;
23+
import org.elasticsearch.common.io.stream.StreamInput;
24+
import org.elasticsearch.common.util.set.Sets;
25+
import org.elasticsearch.common.xcontent.XContent;
26+
import org.elasticsearch.common.xcontent.XContentHelper;
27+
import org.elasticsearch.test.ESTestCase;
28+
29+
import java.io.IOException;
30+
31+
import static org.hamcrest.Matchers.equalTo;
32+
33+
public class AliasMetaDataTests extends ESTestCase {
34+
35+
public void testSerialization() throws IOException {
36+
final AliasMetaData before =
37+
AliasMetaData
38+
.builder("alias")
39+
.filter("{ \"term\": \"foo\"}")
40+
.indexRouting("indexRouting")
41+
.routing("routing")
42+
.searchRouting("trim,tw , ltw , lw")
43+
.build();
44+
45+
assertThat(before.searchRoutingValues(), equalTo(Sets.newHashSet("trim", "tw ", " ltw ", " lw")));
46+
47+
final BytesStreamOutput out = new BytesStreamOutput();
48+
before.writeTo(out);
49+
50+
final StreamInput in = out.bytes().streamInput();
51+
final AliasMetaData after = new AliasMetaData(in);
52+
53+
assertThat(after, equalTo(before));
54+
}
55+
}

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

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,20 @@
2626
import org.elasticsearch.common.Priority;
2727
import org.elasticsearch.test.ESIntegTestCase;
2828

29+
import java.util.Collections;
2930
import java.util.HashMap;
3031
import java.util.Map;
3132
import java.util.Set;
3233
import java.util.concurrent.ExecutionException;
3334

3435
import static org.elasticsearch.common.util.set.Sets.newHashSet;
35-
import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
3636
import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery;
3737
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
3838
import static org.hamcrest.Matchers.equalTo;
3939
import static org.hamcrest.Matchers.nullValue;
4040

4141
public class AliasResolveRoutingIT extends ESIntegTestCase {
4242

43-
4443
// see https://github.com/elastic/elasticsearch/issues/13278
4544
public void testSearchClosedWildcardIndex() throws ExecutionException, InterruptedException {
4645
createIndex("test-0");
@@ -52,10 +51,17 @@ public void testSearchClosedWildcardIndex() throws ExecutionException, Interrupt
5251
client().prepareIndex("test-0", "type1", "2").setSource("field1", "quick brown"),
5352
client().prepareIndex("test-0", "type1", "3").setSource("field1", "quick"));
5453
refresh("test-*");
55-
assertHitCount(client().prepareSearch().setIndices("alias-*").setIndicesOptions(IndicesOptions.lenientExpandOpen()).setQuery(queryStringQuery("quick")).get(), 3L);
54+
assertHitCount(
55+
client()
56+
.prepareSearch()
57+
.setIndices("alias-*")
58+
.setIndicesOptions(IndicesOptions.lenientExpandOpen())
59+
.setQuery(queryStringQuery("quick"))
60+
.get(),
61+
3L);
5662
}
5763

58-
public void testResolveIndexRouting() throws Exception {
64+
public void testResolveIndexRouting() {
5965
createIndex("test1");
6066
createIndex("test2");
6167
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();
@@ -97,9 +103,10 @@ public void testResolveIndexRouting() throws Exception {
97103
}
98104
}
99105

100-
public void testResolveSearchRouting() throws Exception {
106+
public void testResolveSearchRouting() {
101107
createIndex("test1");
102108
createIndex("test2");
109+
createIndex("test3");
103110
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();
104111

105112
client().admin().indices().prepareAliases()
@@ -108,7 +115,10 @@ public void testResolveSearchRouting() throws Exception {
108115
.addAliasAction(AliasActions.add().index("test2").alias("alias20").routing("0"))
109116
.addAliasAction(AliasActions.add().index("test2").alias("alias21").routing("1"))
110117
.addAliasAction(AliasActions.add().index("test1").alias("alias0").routing("0"))
111-
.addAliasAction(AliasActions.add().index("test2").alias("alias0").routing("0")).get();
118+
.addAliasAction(AliasActions.add().index("test2").alias("alias0").routing("0"))
119+
.addAliasAction(AliasActions.add().index("test3").alias("alias3tw").routing("tw "))
120+
.addAliasAction(AliasActions.add().index("test3").alias("alias3ltw").routing(" ltw "))
121+
.addAliasAction(AliasActions.add().index("test3").alias("alias3lw").routing(" lw")).get();
112122

113123
ClusterState state = clusterService().state();
114124
IndexNameExpressionResolver indexNameExpressionResolver = internalCluster().getInstance(IndexNameExpressionResolver.class);
@@ -118,7 +128,9 @@ public void testResolveSearchRouting() throws Exception {
118128
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, "alias10"), equalTo(newMap("test1", newSet("0"))));
119129
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "0", "alias10"), equalTo(newMap("test1", newSet("0"))));
120130
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "1", "alias10"), nullValue());
121-
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, "alias0"), equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
131+
assertThat(
132+
indexNameExpressionResolver.resolveSearchRouting(state, null, "alias0"),
133+
equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
122134

123135
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, null, new String[]{"alias10", "alias20"}),
124136
equalTo(newMap("test1", newSet("0"), "test2", newSet("0"))));
@@ -143,13 +155,42 @@ public void testResolveSearchRouting() throws Exception {
143155
equalTo(newMap("test1", newSet("0"), "test2", newSet("1"))));
144156
assertThat(indexNameExpressionResolver.resolveSearchRouting(state, "0,1,2", new String[]{"test1", "alias10", "alias21"}),
145157
equalTo(newMap("test1", newSet("0", "1", "2"), "test2", newSet("1"))));
158+
159+
assertThat(
160+
indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "test1"),
161+
equalTo(newMap("test1", newSet("tw ", " ltw ", " lw"))));
162+
assertThat(
163+
indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3tw"),
164+
equalTo(newMap("test3", newSet("tw "))));
165+
assertThat(
166+
indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3ltw"),
167+
equalTo(newMap("test3", newSet(" ltw "))));
168+
assertThat(
169+
indexNameExpressionResolver.resolveSearchRouting(state, "tw , ltw , lw", "alias3lw"),
170+
equalTo(newMap("test3", newSet(" lw"))));
171+
assertThat(
172+
indexNameExpressionResolver.resolveSearchRouting(state, "0,tw , ltw , lw", "test1", "alias3ltw"),
173+
equalTo(newMap("test1", newSet("0", "tw ", " ltw ", " lw"), "test3", newSet(" ltw "))));
174+
175+
assertThat(
176+
indexNameExpressionResolver.resolveSearchRouting(state, "0,1,2,tw , ltw , lw", (String[])null),
177+
equalTo(newMap(
178+
"test1", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
179+
"test2", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
180+
"test3", newSet("0", "1", "2", "tw ", " ltw ", " lw"))));
181+
182+
assertThat(
183+
indexNameExpressionResolver.resolveSearchRoutingAllIndices(state.metaData(), "0,1,2,tw , ltw , lw"),
184+
equalTo(newMap(
185+
"test1", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
186+
"test2", newSet("0", "1", "2", "tw ", " ltw ", " lw"),
187+
"test3", newSet("0", "1", "2", "tw ", " ltw ", " lw"))));
146188
}
147189

148190
private <T> Set<T> newSet(T... elements) {
149191
return newHashSet(elements);
150192
}
151193

152-
153194
private <K, V> Map<K, V> newMap(K key, V value) {
154195
Map<K, V> r = new HashMap<>();
155196
r.put(key, value);
@@ -163,4 +204,12 @@ private <K, V> Map<K, V> newMap(K key1, V value1, K key2, V value2) {
163204
return r;
164205
}
165206

207+
private <K, V> Map<K, V> newMap(K key1, V value1, K key2, V value2, K key3, V value3) {
208+
Map<K, V> r = new HashMap<>();
209+
r.put(key1, value1);
210+
r.put(key2, value2);
211+
r.put(key3, value3);
212+
return r;
213+
}
214+
166215
}

0 commit comments

Comments
 (0)