Response::isOk vs Response::hasError

[ebernhardson]

I understand this is pretty abitrary, but i find the difference between these two methods on Response objects to be rather error prone.

hasError: The function documentation says True if the response has error.. The implementation though is: True if the request returned a parsable json document and that document has an 'error' key. Notably $response->hasError() on a 5xx http response returns false. In my opinion this violates the principle of least surprise.

isOk: The function documentation says Checks if the query returned ok. I think this is the method most code should be using as a first step to check that the request wasn't a total failure. Perhaps notable is that if the returned json document as an error key, isOk can still return true.

I suppose the purpose of this ticket is to see if others agree this is a problem, and debate about what could be done about it. I worry changing the semantics of error handling could subtly break peoples applications in ways they previously "worked".

[ruflin]

I agree this is not optimal. If I remember correctly the history is that somehow there was a change on the ES side that required us to introduce a second method to not break BC. Any suggestion on how we should fix it? As 6.0 GA is not out yet, we can still do breaking changes ;-)

[ebernhardson]

I'm not 100% sure. The most obvious thing for me would be if one was implemented in terms of the other, so they both do the same thing.

Tacking on the error key check to the end of isOK() and making hasError() return !$this->isOK() would probably be the most direct. This has a knock on effect though, hasError() can return true but getError() will return an empty string. Should that also be updated to generate errors for things other than the 'error' key in the response?

Quasi related, isOK() currently expects a returned value that contains a 'status' key to have that key contain an http status code, but some endpoints such as /_cluster/health/{indexName} will return a json document containing the string green/yellow/red there. It might be difficult to have a single method that can look at any elasticsearch response and say whatever you requested must have worked.

[ruflin]

I'm not sure if we can cover the different cases with one function. But perhaps we could improve the naming of the functions to make it more obvious what they do?