Skip to content
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

GET aliases should 404 if aliases are missing #25043

Merged
merged 9 commits into from
Jun 6, 2017
Merged

GET aliases should 404 if aliases are missing #25043

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af8cf64
GET aliases should 404 if aliases are missing
jasontedor Jun 3, 2017
aca1a4f
Handle wildcards
jasontedor Jun 4, 2017
e52f4bd
Fix comment
jasontedor Jun 4, 2017
0e6d1f7
Include existing aliases
jasontedor Jun 4, 2017
e13b5d8
Merge branch 'master' into get-aliases-not-found
jasontedor Jun 6, 2017
af53623
Fix status
jasontedor Jun 6, 2017
23e50a9
Simplify
jasontedor Jun 6, 2017
90022ae
Reverse
jasontedor Jun 6, 2017
6917440
Remove import
jasontedor Jun 6, 2017
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
49 changes: 49 additions & 0 deletions core/src/main/java/org/elasticsearch/common/util/set/Sets.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,19 @@

import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Objects;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiConsumer;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collector;
import java.util.stream.Collectors;

public final class Sets {
Expand Down Expand Up @@ -69,6 +77,47 @@ public static <T> Set<T> difference(Set<T> left, Set<T> right) {
return left.stream().filter(k -> !right.contains(k)).collect(Collectors.toSet());
}

public static <T> SortedSet<T> sortedDifference(Set<T> left, Set<T> right) {
Objects.requireNonNull(left);
Objects.requireNonNull(right);
return left.stream().filter(k -> !right.contains(k)).collect(new SortedSetCollector<>());
}

private static class SortedSetCollector<T> implements Collector<T, SortedSet<T>, SortedSet<T>> {

@Override
public Supplier<SortedSet<T>> supplier() {
return TreeSet::new;
}

@Override
public BiConsumer<SortedSet<T>, T> accumulator() {
return (s, e) -> s.add(e);
}

@Override
public BinaryOperator<SortedSet<T>> combiner() {
return (s, t) -> {
s.addAll(t);
return s;
};
}

@Override
public Function<SortedSet<T>, SortedSet<T>> finisher() {
return Function.identity();
}

static final Set<Characteristics> CHARACTERISTICS =
Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.IDENTITY_FINISH));

@Override
public Set<Characteristics> characteristics() {
return CHARACTERISTICS;
}

}

public static <T> Set<T> union(Set<T> left, Set<T> right) {
Objects.requireNonNull(left);
Objects.requireNonNull(right);
Expand Down
85 changes: 59 additions & 26 deletions core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@
package org.elasticsearch.rest.action.admin.indices;

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;

import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest;
import org.elasticsearch.action.admin.indices.alias.get.GetAliasesResponse;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.cluster.metadata.AliasMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.BaseRestHandler;
Expand All @@ -39,9 +41,13 @@
import org.elasticsearch.rest.action.RestBuilderListener;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.SortedSet;
import java.util.stream.Collectors;

import static org.elasticsearch.rest.RestRequest.Method.GET;
Expand Down Expand Up @@ -80,41 +86,68 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
return channel -> client.admin().indices().getAliases(getAliasesRequest, new RestBuilderListener<GetAliasesResponse>(channel) {
@Override
public RestResponse buildResponse(GetAliasesResponse response, XContentBuilder builder) throws Exception {
if (response.getAliases().isEmpty()) {
// empty body if indices were specified but no matching aliases exist
if (indices.length > 0) {
return new BytesRestResponse(OK, builder.startObject().endObject());
} else {
final String message = String.format(Locale.ROOT, "alias [%s] missing", toNamesString(getAliasesRequest.aliases()));
builder.startObject();
{
builder.field("error", message);
builder.field("status", RestStatus.NOT_FOUND.getStatus());
final ImmutableOpenMap<String, List<AliasMetaData>> aliasMap = response.getAliases();

final Set<String> aliasNames = new HashSet<>();
for (final ObjectObjectCursor<String, List<AliasMetaData>> cursor : aliasMap) {
Copy link
Member

Choose a reason for hiding this comment

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

You can probably use aliasMap.values() since you don't need the key for anything right? I don't think it's necessarily any better though, so either way is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay.

for (final AliasMetaData aliasMetaData : cursor.value) {
aliasNames.add(aliasMetaData.alias());
}
}

// first remove requested aliases that are exact matches
final SortedSet<String> difference = Sets.sortedDifference(Arrays.stream(aliases).collect(Collectors.toSet()), aliasNames);

// now remove requested aliases that contain wildcards that are simple matches
final List<String> matches = new ArrayList<>();
outer:
for (final String pattern : difference) {
if (pattern.contains("*")) {
for (final String aliasName : aliasNames) {
if (Regex.simpleMatch(pattern, aliasName)) {
matches.add(pattern);
continue outer;
}
}
builder.endObject();
return new BytesRestResponse(RestStatus.NOT_FOUND, builder);
}
} else {
builder.startObject();
{
for (final ObjectObjectCursor<String, List<AliasMetaData>> entry : response.getAliases()) {
builder.startObject(entry.key);
}
difference.removeAll(matches);

final RestStatus status;
builder.startObject();
{
if (!difference.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the ! if we reverse this if statement, so

if (difference.isEmpty()) {
    status = RestStatus.OK;
} else {
    ... the error stuff ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay.

status = RestStatus.NOT_FOUND;
Copy link
Member

Choose a reason for hiding this comment

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

Personally I think we should return RestStatus.PARTIAL_CONTENT (206) in the event that requested aliases were requested and some were found while some were not, but it sounds like this ship has already sailed as far as what to return.

Copy link
Member Author

Choose a reason for hiding this comment

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

206 is wrong, it's for range requests only. Also, 2xx is often not seen as an error client side while 4xx is.

final String message;
if (difference.size() == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have a consistent message regardless of the number of aliases, I know it's bad form for people to write tests against error messages, but that doesn't mean people don't. So, I think we should stick with just aliases [foo] missing. What do you think? (I'm only +0 on the change)

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree that it's bad form, and I have a test for both cases. It's a pet peeve of mine to see messages like "error(s) were encountered while processing"; the information to determine whether "an error was encountered while processing" or "errors were encountered while processing" should be provided is available, the programmer just needs to write some simple code to give a correct error message.

Why do you think it should be consistent instead of correct in every case?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you think it should be consistent instead of correct in every case?

As I mentioned above, mostly for users wrapping ES in their own application, anyone that wants to "wrap" this error needs to change parsing aliases [.*] missing to alias(es)? [.*] missing, which will not be obvious to an end user unless they hit the endpoint with both single and mulitple aliases. I would expect someone who wanted to wrap ES to hit the endpoint, see the error, and then write something that wrapped that error in a prettier way for their application, not realizing that the error may change depending on the cardinality of the aliases.

Like I said above though, I'm only +0 on it, if you don't agree we don't have to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dakrone Yeah I'm not sure about that concern since I do not think we offer any guarantees on these error messages.

message = String.format(Locale.ROOT, "alias [%s] missing", toNamesString(difference.iterator().next()));
} else {
message = String.format(Locale.ROOT, "aliases [%s] missing", toNamesString(difference.toArray(new String[0])));
}
builder.field("error", message);
builder.field("status", RestStatus.NOT_FOUND.getStatus());
Copy link
Member

Choose a reason for hiding this comment

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

could be status.getStatus() in the event we change the local variable above.

} else {
status = RestStatus.OK;
}

for (final ObjectObjectCursor<String, List<AliasMetaData>> entry : response.getAliases()) {
builder.startObject(entry.key);
{
builder.startObject("aliases");
{
builder.startObject("aliases");
{
for (final AliasMetaData alias : entry.value) {
AliasMetaData.Builder.toXContent(alias, builder, ToXContent.EMPTY_PARAMS);
}
for (final AliasMetaData alias : entry.value) {
AliasMetaData.Builder.toXContent(alias, builder, ToXContent.EMPTY_PARAMS);
}
builder.endObject();
}
builder.endObject();
}
builder.endObject();
}
builder.endObject();
return new BytesRestResponse(OK, builder);
}
builder.endObject();
return new BytesRestResponse(status, builder);
}

});
}

Expand Down
6 changes: 6 additions & 0 deletions modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ public void testAliasExists() throws IOException {
}
}

public void testAliasDoesNotExist() throws IOException {
createTestDoc();
headTestCase("/_alias/test_alias", emptyMap(), NOT_FOUND.getStatus(), greaterThan(0));
headTestCase("/test/_alias/test_alias", emptyMap(), NOT_FOUND.getStatus(), greaterThan(0));
}

public void testTemplateExists() throws IOException {
try (XContentBuilder builder = jsonBuilder()) {
builder.startObject();
Expand Down
7 changes: 6 additions & 1 deletion rest-api-spec/src/main/resources/rest-api-spec/test/indices.delete_alias/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
---
"Basic test for delete alias":
- skip:
version: " - 5.99.99"
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 5.4.99, since this will be backported, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, see my top-level comment.

reason: Previous versions did not 404 on missing aliases

- do:
indices.create:
Expand All @@ -25,8 +28,10 @@
name: testali

- do:
catch: missing
indices.get_alias:
index: testind
name: testali

- match: { '': {}}
- match: { 'status': 404 }
- match: { 'error': 'alias [testali] missing' }
37 changes: 31 additions & 6 deletions rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_alias/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ setup:

---
"Get aliases via /{index}/_alias/prefix*":

- do:
indices.get_alias:
index: test_index
Expand Down Expand Up @@ -166,25 +165,51 @@ setup:


---
"Non-existent alias on an existing index returns an empty body":
"Non-existent alias on an existing index returns 404":
- skip:
version: " - 5.99.99"
Copy link
Member

Choose a reason for hiding this comment

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

Same here about version

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, see my top-level comment.

reason: Previous versions did not 404 on missing aliases

- do:
catch: missing
indices.get_alias:
index: test_index
name: non-existent

- match: { '': {}}
- match: { 'status': 404}
- match: { 'error': 'alias [non-existent] missing' }

---
"Existent and non-existent alias returns just the existing":
"Existent and non-existent alias returns 404 and the existing alias":
- skip:
version: " - 5.99.99"
Copy link
Member

Choose a reason for hiding this comment

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

Same here for version

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, see my top-level comment.

reason: Previous versions did not 404 on missing aliases

- do:
catch: missing
indices.get_alias:
index: test_index
name: test_alias,non-existent

- match: {test_index.aliases.test_alias: {}}
- is_false: test_index.aliases.non-existent
- match: { 'status': 404 }
- match: { 'error': 'alias [non-existent] missing' }
- match: { test_index.aliases.test_alias: { } }

---
"Existent and non-existent aliases returns 404 and the existing alias":
- skip:
version: " - 5.99.99"
Copy link
Member

Choose a reason for hiding this comment

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

And same here for version

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, see my top-level comment.

reason: Previous versions did not 404 on missing aliases

- do:
catch: missing
indices.get_alias:
index: test_index
name: test_alias,non-existent,another-non-existent

- match: { 'status': 404 }
- match: { 'error': 'aliases [another-non-existent,non-existent] missing' }
- match: { test_index.aliases.test_alias: { } }

---
"Getting alias on an non-existent index should return 404":
Expand Down