Type::getDocument() throws NotFoundException when a ResponseException is captured

[rmruano]

Requesting existing documents without providing a routing parameter (for types with parent docs) rise a NotFoundException which is confusing and makes debugging a lot harder.

Original code at Type::getDocument():

try {
    $response = $this->request($path, Request::GET, array(), $options);
    $result = $response->getData();
} catch (ResponseException $e) {
    throw new NotFoundException('doc id ' . $id . ' not found');
}

$info = $response->getTransferInfo();
if ($info['http_code'] !== 200) {
    throw new NotFoundException('doc id ' . $id . ' not found');
}

I suggest to just leave the ResponseException uncatched (preferred) or to at least improve the exception message:

try {
    $response = $this->request($path, Request::GET, array(), $options);
    $result = $response->getData();
} catch (ResponseException $e) {
    throw new NotFoundException('unable to retrieve doc id ' . $id. ': '.$e->getMessage(), $e->getCode(), $e);
}

$info = $response->getTransferInfo();
if ($info['http_code'] !== 200) {
    throw new NotFoundException('doc id ' . $id . ' not found');
}

[ruflin]

Good point. Can you open a pull request with this change?

[rmruano]

I've just improved the raised exception in my PR.

[ruflin]

Ok, I now merged your pull request to have a first "improved" fix. I also went through your comments. What you say is:

Exceptions from the $request object should no be catched. If later it is not a 200 response code, the exception is thrown anyway, right?

[ruflin]

Side note: If it makes sense, breaking BC is not a problem ;-)

[rmruano]

@ruflin Thanks!. That's it, the request exception should not be catched at all. A NotFoundException will be thrown if it's not a 200 OK response, and a ResponseException will be thrown if something went bad (ES response contains an error).

[ruflin]

Can we try that out in a new pull request and see what the "consequences" on the current tests are?

[rmruano]

The only broken test is TypeTest::testGetDocumentNotExistingIndex().

Failed asserting that exception of type "Elastica\Exception\ResponseException" matches expected exception "\Elastica\Exception\NotFoundException". Message was: "IndexMissingException[[index] missing]"

It seems that ES sends an error response in combination with a 404 header when the index is missing, that triggers the ResponseException which now goes uncatched.

An easy fix would be to bring back the catch in Type::getDocument() but only throwing the NotFoundException in case of a 404 response code.

...
try {
    $response = $this->request($path, Request::GET, array(), $options);
    $result = $response->getData();
} catch (ResponseException $e) {
    $info = $e->getResponse()->getTransferInfo();
    if ($info['http_code'] === 404) {
        throw new NotFoundException('unable to retrieve doc id ' . $id. ': '.$e->getMessage(), $e->getCode(), $e);
    }
}

$info = $response->getTransferInfo();
if ($info['http_code'] !== 200) {
    throw new NotFoundException('doc id ' . $id . ' not found');
}
...

It feels odd being forced to handle 404s 2 times. Maybe the ResponseException should not be thrown by the transport if the response has a 404 http code (It's not really an exception)... but I suppose that will be a pretty big BC break. What do you think?

[ruflin]

Sorry, for the late reply. About the 404: From a theoretical point of view you are probably right about the 404 not throwing an exception. But the main question I ask is what the engineer expects. If he uses Elastica, he doesn't care too much if it is http or thrift in the background. So is 404 for him something he would expect an exception because something went "unexpected"? Or what are the main reasons for 404? Here it is, because the document was not found. Why was the documented not found? Because it didn't exist or was not populated yet. I would assume, an engineer doesn't use this call when he does not expect the document to exist?

What do you think?

[rmruano]

NP, we're all pretty busy :p

I think we should behave just like ES do: only throw a NotFound exception (ES response without any errors where found===false) if the document is not found because it hasn't been populated yet. If the reason for the document not being retrieved is another one like a bad request (i.e. child doc without parent parameter) or a missing index, the ResponseError triggered should not be captured, that way the engineer can easily know what failed.

In case we need/want to be completely agnostic about the transport we should stop using $response->getTransferInfo() at all, because it's totally dependent on the HTTP transport (in fact, $info['http_code'] !== 404 is actually throwing notices if other transport is used). There are a few references to $response->getTransferInfo() within the code.

Having said that, my "final" proposal would be:

....
$response = $this->request($path, Request::GET, array(), $options);
$result = $response->getData();

if (!isset($result['found']) || $result['found'] === false) {
    throw new NotFoundException('doc id ' . $id . ' not found');
}
....

In order to pass the tests the only change required to testGetDocumentNotExistingIndex would be to expect a \Elastica\Exception\ResponseException

[ruflin]

I agree with both points. We should only trigger it if it is not found and outside the transport we should get rid of "404" checks etc. But we should treat these changes as two different pull requests, as the second one we should get in step by step.

Can you open a pull request with your code above?

[rmruano]

Done! #713

[rmruano]

Ok, since the main problem has been resolved by the #713 merge, should I close this issue?. Remember there are still a few $response->getTransferInfo() references within the code.

[ruflin]

@rmruano Can you open an new "Issue" so we don't forget to change these at a later stage?

[rmruano]

Sure, will do a little research first, if there are no inherited problems I'll just create a PR with it. Closing this issue.

[ruflin]

👍