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.

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

Conversation

@jasontedor
Copy link
Member

commented Jun 3, 2017

Previously the HEAD and GET aliases endpoints were misaligned in behavior. The HEAD verb would 404 if any aliases are missing while the GET verb would not if any aliases existed. When HEAD was aligned with GET, this broke the previous usage of HEAD to serve as an existence check for aliases. It is the behavior of GET that is problematic here though, if any alias is missing the request should 404. This commit addresses this by modifying the behavior of GET to behave in this way. This fixes the behavior for HEAD to also 404 when aliases are missing.

Closes #24644

GET aliases should 404 if aliases are missing
Previously the HEAD and GET aliases endpoints were misaigned in
behavior. The HEAD verb would 404 if any aliases are missing while the
GET verb would not if any aliases existed. When HEAD was aligned with
GET, this broke the previous usage of HEAD to serve as an existence
check for aliases. It is the behavior of GET that is problematic here
though, if any alias is missing the request should 404. This commit
addresses this by modifying the behavior of GET to behave in this
way. This fixes the behavior for HEAD to also 404 when aliases are
missing.
jasontedor added 4 commits Jun 4, 2017
Merge branch 'master' into get-aliases-not-found
* master:
  Add support for clear scroll to high level REST client (#25038)
  Tiny correction in inner-hits.asciidoc (#25066)
  Added release notes for 6.0.0-alpha2
  Expand index expressions against indices only when managing aliases (#23997)
  Collapse inner hits rest test should not skip 5.x
  Settings: Fix secure settings by prefix (#25064)
  add `exclude_keys` option to KeyValueProcessor (#24876)
  Test: update missing body tests to run against versions >= 5.5.0
  Track EWMA[1] of task execution time in search threadpool executor
  Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current).
  Fixed NPEs caused by requests without content. (#23497)
  Plugins can register pre-configured char filters (#25000)
  Build: Allow preserving shared dir (#24962)
  Tests: Make secure settings available from settings builder for tests (#25037)
@dakrone
Copy link
Member

left a comment

I left some comments, almost all of them are not major, but I think the skip version should allow testing on 5.5 since this will be backported.

final ImmutableOpenMap<String, List<AliasMetaData>> aliasMap = response.getAliases();

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

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

Okay.

if (!difference.isEmpty()) {
status = RestStatus.NOT_FOUND;
final String message;
if (difference.size() == 1) {

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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)

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

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?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

@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, "aliases [%s] missing", toNamesString(difference.toArray(new String[0])));
}
builder.field("error", message);
builder.field("status", RestStatus.NOT_FOUND.getStatus());

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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

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

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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

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

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

Okay.

builder.startObject();
{
if (!difference.isEmpty()) {
status = RestStatus.NOT_FOUND;

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

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

@@ -1,5 +1,8 @@
---
"Basic test for delete alias":
- skip:
version: " - 5.99.99"

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

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

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

I disagree, see my top-level comment.

"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"

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

Same here about version

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

I disagree, see my top-level comment.

"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"

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

Same here for version

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

I disagree, see my top-level comment.

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

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 6, 2017

Member

And same here for version

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 6, 2017

Author Member

I disagree, see my top-level comment.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2017

I think the skip version should allow testing on 5.5 since this will be backported.

@dakrone I don't think this is how we should do things because then this branch can never get to green until this is backported to 5.x. Instead:

  • develop on master with a skip for 5.x
  • backport to 5.x, changing the skip to 5.4.99
  • push a separate change to change the skip on master to 5.4.99
@dakrone

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

I don't think this is how we should do things because then this branch can never get to green until this is backported to 5.x.

Alright, as long as it is changed during the backport, I just don't want us to forget it!

jasontedor added 2 commits Jun 6, 2017
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2017

@dakrone I've responded to your comments, thanks for the suggestions so I pushed some commits too.

@dakrone
dakrone approved these changes Jun 6, 2017
Copy link
Member

left a comment

LGTM

@jasontedor jasontedor merged commit e03c493 into elastic:master Jun 6, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
jasontedor added a commit that referenced this pull request Jun 6, 2017
GET aliases should 404 if aliases are missing
Previously the HEAD and GET aliases endpoints were misaigned in
behavior. The HEAD verb would 404 if any aliases are missing while the
GET verb would not if any aliases existed. When HEAD was aligned with
GET, this broke the previous usage of HEAD to serve as an existence
check for aliases. It is the behavior of GET that is problematic here
though, if any alias is missing the request should 404. This commit
addresses this by modifying the behavior of GET to behave in this
way. This fixes the behavior for HEAD to also 404 when aliases are
missing.

Relates #25043
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2017

Thanks @dakrone.

@jasontedor jasontedor deleted the jasontedor:get-aliases-not-found branch Jun 6, 2017

@m1kola

This comment has been minimized.

Copy link

commented Jun 7, 2017

Thanks for fixing it.

Am I right in thinking that it will not be released in the 5.4 branch and users should avoid using all 5.4.* versions if their applications rely on 404 for missing indexes (in other words, just upgrade from 5.3 to 5.5)?

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2017

Thanks for fixing it.

You're very welcome, and sorry for the issue in the first place.

Am I right in thinking that it will not be released in the 5.4 branch and users should avoid using all 5.4.* versions if their applications rely on 404 for missing indexes (in other words, just upgrade from 5.3 to 5.5)?

That's all correct.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2017

@m1kola Also, would you be able to test 6.0.0-alpha2 and ensure that it has the behavior that you're looking for? I've tested it with your reproduction and I think it does, but I would rather be safe than sorry while we have time to change this for 5.5.0.

@m1kola

This comment has been minimized.

Copy link

commented Jun 7, 2017

@jasontedor thanks for the explanation.

I've tested it with your reproduction and I think it does

I think it should be ok, but I'll double check it on Friday (let me know if you want me to come back with feedback earlier).

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2017

I think it should be ok, but I'll double check it on Friday (let me know if you want me to come back with feedback earlier).

Thank you. Friday will be fine.

@m1kola

This comment has been minimized.

Copy link

commented Jun 9, 2017

Hi @jasontedor. I'm afraid 6.0.0-alpha2 gives the same results: it returns 200 in case when an index exsists, but an alias doesn't exist.

Are you sure that the 6.0.0-alpha2 includes this PR?

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

I thought so, let me double check.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

It does not, I'm so very sorry for the mistake that led to a waste of your time. There's a snapshot for 5.5.0 that does contain the change.

@m1kola

This comment has been minimized.

Copy link

commented Jun 9, 2017

No worries at all. I'm happy to help :)

The snapshot for 5.5.0 works great for me. Thank you for the fix!

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

Okay, I'm so glad to hear that. You're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.