Skip to content

Only negate index expression on all indices with preceding wildcard #20898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
javanna merged 8 commits into from
Oct 18, 2016
Merged

Only negate index expression on all indices with preceding wildcard #20898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d54c70
Only negate index expression on all indices with preceding wildcard
dakrone Oct 12, 2016
a7f4af5
Fix WildcardExpressionResolverTests
dakrone Oct 12, 2016
5a2df4e
Assert that non-existent negated indices are correctly non-expanded
dakrone Oct 14, 2016
7453fc5
Add more tests for corner cases
dakrone Oct 14, 2016
e4d5ee2
Add additional test cases
dakrone Oct 16, 2016
12b957b
Add more additional tests
dakrone Oct 17, 2016
5f186a9
Check expression with Regex.isSimpleMatchPattern and only check when …
dakrone Oct 17, 2016
6982a9c
No need to check for expandWildcardsOpen and expandWildcardsClosed
dakrone Oct 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ public List<String> resolve(Context context, List<String> expressions) {

private Set<String> innerResolve(Context context, List<String> expressions, IndicesOptions options, MetaData metaData) {
Set<String> result = null;
boolean wildcardSeen = false;
for (int i = 0; i < expressions.size(); i++) {
String expression = expressions.get(i);
if (aliasOrIndexExists(metaData, expression)) {
Copy link
Member

@javanna javanna Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you understand this if block? I suspect it made sense before your change, to make it possible to refer to existing indices or aliases that started with - rather than treating the name as a negation. That said, I can't quite follow why it makes sense in some cases for result to be null. This was already there, not your doing but I wonder if it's related and may be cleaned up.

Copy link
Member Author

@dakrone dakrone Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it doesn't initialize the result set like the other null checks do, but I do think all of this code should be cleaned up after this (I hesitate to touch more code as this is supposed to be backported all the way back to 2.4)

Expand All @@ -598,13 +599,14 @@ private Set<String> innerResolve(Context context, List<String> expressions, Indi
}
expression = expression.substring(1);
} else if (expression.charAt(0) == '-') {
// if its the first, fill it with all the indices...
if (i == 0) {
List<String> concreteIndices = resolveEmptyOrTrivialWildcard(options, metaData, false);
result = new HashSet<>(concreteIndices);
// if there is a negation without a wildcard being previously seen, add it verbatim,
// otherwise return the expression
if (wildcardSeen) {
add = false;
expression = expression.substring(1);
} else {
add = true;
}
add = false;
expression = expression.substring(1);
}
if (result == null) {
// add all the previous ones...
Expand Down Expand Up @@ -634,6 +636,10 @@ private Set<String> innerResolve(Context context, List<String> expressions, Indi
if (!noIndicesAllowedOrMatches(options, matches)) {
throw infe(expression);
}

if (Regex.isSimpleMatchPattern(expression)) {
wildcardSeen = true;
Copy link
Member

@javanna javanna Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should set this to true only if options.expandWildcardsOpen || options.expandWildcardsClosed . Otherwise wildcard expressions don't get expanded...

Copy link
Member Author

@dakrone dakrone Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I've changed the if to check these

}
}
return result;
}
Expand Down
60 changes: 59 additions & 1 deletion core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public void testIndexOptionsWildcardExpansion() {
assertEquals(1, results.length);
assertEquals("bar", results[0]);

results = indexNameExpressionResolver.concreteIndexNames(context, "-foo*");
results = indexNameExpressionResolver.concreteIndexNames(context, "*", "-foo*");
assertEquals(1, results.length);
assertEquals("bar", results[0]);

Expand Down Expand Up @@ -585,6 +585,64 @@ public void testConcreteIndicesWildcardExpansion() {
assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "testX*")), equalTo(newHashSet("testXXX", "testXXY", "testXYY")));
}

public void testConcreteIndicesWildcardWithNegation() {
MetaData.Builder mdBuilder = MetaData.builder()
.put(indexBuilder("testXXX").state(State.OPEN))
.put(indexBuilder("testXXY").state(State.OPEN))
.put(indexBuilder("testXYY").state(State.OPEN))
.put(indexBuilder("-testXYZ").state(State.OPEN))
.put(indexBuilder("-testXZZ").state(State.OPEN))
.put(indexBuilder("-testYYY").state(State.OPEN))
.put(indexBuilder("testYYY").state(State.OPEN))
.put(indexBuilder("testYYX").state(State.OPEN));
ClusterState state = ClusterState.builder(new ClusterName("_name")).metaData(mdBuilder).build();

IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state,
IndicesOptions.fromOptions(true, true, true, true));
assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "testX*")),
equalTo(newHashSet("testXXX", "testXXY", "testXYY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "test*", "-testX*")),
equalTo(newHashSet("testYYY", "testYYX")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "-testX*")),
equalTo(newHashSet("-testXYZ", "-testXZZ")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "testXXY", "-testX*")),
equalTo(newHashSet("testXXY", "-testXYZ", "-testXZZ")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "*", "--testX*")),
equalTo(newHashSet("testXXX", "testXXY", "testXYY", "testYYX", "testYYY", "-testYYY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "-testXXX", "test*")),
equalTo(newHashSet("testYYX", "testXXX", "testXYY", "testYYY", "testXXY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "test*", "-testXXX")),
equalTo(newHashSet("testYYX", "testXYY", "testYYY", "testXXY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "+testXXX", "+testXXY", "+testYYY", "-testYYY")),
equalTo(newHashSet("testXXX", "testXXY", "testYYY", "-testYYY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "testYYY", "testYYX", "testX*", "-testXXX")),
equalTo(newHashSet("testYYY", "testYYX", "testXXY", "testXYY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "-testXXX", "*testY*", "-testYYY")),
equalTo(newHashSet("testYYX", "testYYY", "-testYYY")));

String[] indexNames = indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.lenientExpandOpen(), "-doesnotexist");
assertEquals(0, indexNames.length);

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.lenientExpandOpen(), "-*")),
equalTo(newHashSet("-testXYZ", "-testXZZ", "-testYYY")));

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.lenientExpandOpen(),
"+testXXX", "+testXXY", "+testXYY", "-testXXY")),
equalTo(newHashSet("testXXX", "testXYY", "testXXY")));

indexNames = indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.lenientExpandOpen(), "*", "-*");
assertEquals(0, indexNames.length);
}

/**
* test resolving _all pattern (null, empty array or "_all") for random IndicesOptions
*/
Expand Down
6 changes: 3 additions & 3 deletions core/src/test/java/org/elasticsearch/cluster/metadata/WildcardExpressionResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ public void testConvertWildcardsJustIndicesTests() {
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("*"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY", "kuku")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("*", "-kuku"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "+testYYY"))), equalTo(newHashSet("testXXX", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "-testXXX"))).size(), equalTo(0));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "-testXXX"))), equalTo(newHashSet("testXXX", "-testXXX")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "+testY*"))), equalTo(newHashSet("testXXX", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "-testX*"))).size(), equalTo(0));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testXXX", "-testX*"))), equalTo(newHashSet("testXXX")));
}

public void testConvertWildcardsTests() {
Expand All @@ -66,7 +66,7 @@ public void testConvertWildcardsTests() {

IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, IndicesOptions.lenientExpandOpen());
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("testYY*", "alias*"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("-kuku"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("-kuku"))), equalTo(newHashSet("-kuku")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+test*", "-testYYY"))), equalTo(newHashSet("testXXX", "testXYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+testX*", "+testYYY"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+testYYY", "+testX*"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
Expand Down