Improve exception handling in Type::getDocument()

[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.

[rmruano]

Check issue #687

[fprochazka]

This is a BC break, you should be passing the previous exception to the new one.

[rmruano]

It is, I've upgraded it to the other proposal I did which doesn't break BC, but IMHO It doesn't seem appropiate to throw a NotFoundException for ResponseExceptions.

[coveralls]

Coverage remained the same when pulling c3dc16e on rmruano:type-get-document into ffbfa5d on ruflin:master.

[ruflin]

@rmruano I see your point. Is there an option we can actually make the difference between the two exceptions? Means, in case we get a response exception and it is a notfound, we throw this one, but in your case throw a response or a more specific exception. And if none both hits, we throw the general response exception.

[rmruano]

@ruflin The thing is that ES returns a 404 response code for not founds (not sure about older versions) and it doesn't return an error response, so no ResponseException is triggered and no NotFound should be ever thrown in that case. If you don't want to break BC you can merge this PR as is right now, at least the exception includes now a better message and the original exception, but I still think the ResponseException should not be catched at all (like all other methods at the Type class do).

Having said that, what do you think about creating a BadRequestException extending (or not) ResponseException?, AbstractTransport::exec() methods could be updated to consider this case:

public function exec(Request $request, array $params)
{
    ....

    $transferInfo = curl_getinfo($conn);
    $response->setTransferInfo($transferInfo);

    if ($response->hasError()) {
        switch($transferInfo['http_code']) {
            case "400":
                throw new BadRequestException($request, $response);
                break;
            default:
                throw new ResponseException($request, $response);
                break;
        }
    }

    ...

    return $response;
}

And Type::getDocument() could detect it:

public function getDocument($id, $options = array())
{
    $path = urlencode($id);

    try {
        $response = $this->request($path, Request::GET, array(), $options);
        $result = $response->getData();
    } catch (ResponseException $e) {
        if ($e instanceof BadRequestException) {
            throw $e;
        } else {
            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');
    }

    ...
}

• In case BadRequestException doesn't extend ResponseException no modification should be needed.

Anyway, I would still remove the whole try / catch :)