Get the root reason from elasticsearch's json when available #1111

Merged
merged 1 commit into from Jun 15, 2016

Conversation

Projects
None yet
2 participants
@maxgalbu
Contributor

maxgalbu commented Jun 14, 2016

Sometimes elasticsearch (i'm using 2.3.3) returns an error with two reasons:

{
  "error": {
    "root_cause": [
      {
        "type": "index_creation_exception",
        "reason": "failed to create index"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "synonym requires either `synonyms` or `synonyms_path` to be configured"
  },
  "status": 400
}

But the current code uses only the "root_cause" reason, leaving me with an exception that says "failed to create index" and not much else. This change uses the "root" reason key if it exists.

@maxgalbu maxgalbu changed the title from Get the root reason from elasicsearch's json when available to Get the root reason from elasticsearch's json when available Jun 14, 2016

lib/Elastica/Response.php
if (isset($error['root_cause'][0])) {
- $error = $error['root_cause'][0];
+ $root_error = $error['root_cause'][0];

This comment has been minimized.

@ruflin

ruflin Jun 15, 2016

Owner

Could you use $rootError for the name here to adhere to our naming standard?

@ruflin

ruflin Jun 15, 2016

Owner

Could you use $rootError for the name here to adhere to our naming standard?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jun 15, 2016

Owner

LGTM. One minor comment about the naming. Could you please also update the CHANGELOG.md? And if possible, rebase it on master. That will fix the broken 5.4 build.

Owner

ruflin commented Jun 15, 2016

LGTM. One minor comment about the naming. Could you please also update the CHANGELOG.md? And if possible, rebase it on master. That will fix the broken 5.4 build.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jun 15, 2016

Owner

@maxgalbu Oh, Github started to send notifications for new commits. Not sure if this is complete yet, but you missed the CHANGELOG.md part ;-)

Owner

ruflin commented Jun 15, 2016

@maxgalbu Oh, Github started to send notifications for new commits. Not sure if this is complete yet, but you missed the CHANGELOG.md part ;-)

@maxgalbu

This comment has been minimized.

Show comment
Hide comment
@maxgalbu

maxgalbu Jun 15, 2016

Contributor

you're right, I had a brief moment of pause at work so I did the quick fix :)

Contributor

maxgalbu commented Jun 15, 2016

you're right, I had a brief moment of pause at work so I did the quick fix :)

@ruflin ruflin merged commit 45ee2de into ruflin:master Jun 15, 2016

2 checks passed

codecov/project 84.64% (+<.01%) compared to 256ff2c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jun 15, 2016

Owner

Merged. Thanks a lot for the improvement. Unfortunately I already made the 3.2.1 release today in the morning, so it will go into the next minor.

Owner

ruflin commented Jun 15, 2016

Merged. Thanks a lot for the improvement. Unfortunately I already made the 3.2.1 release today in the morning, so it will go into the next minor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment