401 - Unauthorized Code Exception Handling

[johnbburg]

Summary of problem or feature request

Edit: Since posting this, I identified that this is addressed in 7.x, while I was working with 6.x.

I'm using the elasticsearch_connector Drupal module integration, and I am trying delete some indexes that were created, but no longer needed. I noticed that I am unable to delete these on the Drupal side, because Drupal will try to use this library to send a request to delete the index on the server, which instead responds with 401 - Unauthorized status code. At this point, the Drupal fails and doesn't know what to do, and fails to delete the reference to the index on that side. While I've filed a separate issue to improve the error handling on that end. I am raising this issue here to propose adding an additional Exception class for this type of response.

Also, in these types of responses, from elastic.co's service at least, the response body is empty, which translates to a null when that body is converted to a string, and passed as the exception message text. In Drupal, this results in a status message with just "null" being printed. This is very confusing to debug. It would be helpful to have a more specific message here.

Code snippet of problem

Looking at the list of exceptions thrown https://github.com/elastic/elasticsearch-php/blob/6.7.x/src/Elasticsearch/Connections/Connection.php#L636, We could add an additional one for 401 responses with some a more informative exception message in cases where the response body is empty.

[johnbburg]

There already seems to be an exception class for this. https://github.com/elastic/elasticsearch-php/blob/master/src/Elasticsearch/Common/Exceptions/Unauthorized401Exception.php which seems unused on the 6.x
I am looking at. But is used on 7.x. So that would explain it.

I don't recall at this moment why I had opted for the 6.x branch, but I think there was some issue limiting my option here.

So this may be a moot issue if there is no intention to backport changes like this to 6.x.

[ezimuel]

@johnbburg thanks for report this issue. I planned to release another version for 6.x branch that will be 6.8.0. I already have another feature to backport, see the list here.
Are you interested in sending a PR for this feature? Please send to 6.x branch. Thanks!

[johnbburg]

@ezimuel yes, I can work on this. I have signed the contributor agreement. Though I am still very new to the workflow of this project. I'll do my best.

[ezimuel]

@johnbburg thanks a ton and don't worry if something is not clear yet. Let me know if you need any help. Thanks again!

[johnbburg]

Great, I think I have something ready. I attempted to run unit tests, but they seem like they might be a bit out of date. Several tests skipped with All of Sniffing unit tests use outdated cluster state format, need to redo and several tests with errors with Function ReflectionType::__toString() is deprecated. This is running with PHP 7.4.3.

I assume I should just disregard these results for now. The contributor notes says each PR should have a test, but I'm not sure that's really necessary if we are just looking into throwing some different exceptions here.

I'll try to create a PR later today.

[ezimuel]

@johnbburg the skipped test with message All of Sniffing... are not relevant. I'm not sure about the other error messages. Anyway, I'll try to fix these and complete the unit tests, don't worry. Thanks!