From c58f340633a5ecb1368749204d3fac4f3f71f89f Mon Sep 17 00:00:00 2001 From: Alex Palaistras Date: Wed, 10 Mar 2021 11:16:24 +0000 Subject: [PATCH] Adapter/Guzzle: Fix error handling for v4 API This commit represents a partial overhaul of error handling for requests made against the v4 Cloudflare API, with an aim of unifying disparate kinds of exceptions under a single `ResponseException` type, and the covering of additional cases where errors were unhandled. Specifically: - The `Guzzle::request()` function will now catch Guzzle exceptions normally thrown in cases of client and server errors (4xx and 5xx) response codes, and convert these to `ResponseException` types before re-throwing. These types of errors were previously not caught and were instead returned verbatim, expecting downstream clients to be aware of internal details of how these functions operate. - Conversely, we no longer assume that all responses are JSON-encoded, and no longer try to derive errors from non-4xx or 5xx responses. All public endpoints under the v4 API are expected to be well-behaved in that regard, and never return an error response where none is indicated in the HTTP code. Code has been moved around and test-cases added in support of these changes. In most cases, these changes won't break any existing expectations and won't require any changes to downstream code, but users of the Cloudflare SDK should ensure that they are indeed set up for catching `ResponseException` instances thrown during requests, and should not expect to see Guzzle exceptions directly (though these are still available in calls to `ResponseException::getPrevious()`). Fixes: #152 --- src/Adapter/Guzzle.php | 37 +++--------- src/Adapter/ResponseException.php | 31 ++++++++++ tests/Adapter/GuzzleTest.php | 55 +++-------------- tests/Adapter/ResponseExceptionTest.php | 78 +++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 75 deletions(-) create mode 100644 tests/Adapter/ResponseExceptionTest.php diff --git a/src/Adapter/Guzzle.php b/src/Adapter/Guzzle.php index a7d8ad1..2cc7636 100644 --- a/src/Adapter/Guzzle.php +++ b/src/Adapter/Guzzle.php @@ -1,14 +1,10 @@ client->$method($uri, [ - 'headers' => $headers, - ($method === 'get' ? 'query' : 'json') => $data, - ]); - - $this->checkError($response); + try { + $response = $this->client->$method($uri, [ + 'headers' => $headers, + ($method === 'get' ? 'query' : 'json') => $data, + ]); + } catch (RequestException $err) { + throw ResponseException::fromRequestException($err); + } return $response; } - - private function checkError(ResponseInterface $response) - { - $json = json_decode($response->getBody()); - - if (json_last_error() !== JSON_ERROR_NONE) { - throw new JSONException(); - } - - if (isset($json->errors) && count($json->errors) >= 1) { - throw new ResponseException($json->errors[0]->message, $json->errors[0]->code); - } - - if (isset($json->success) && !$json->success) { - throw new ResponseException('Request was unsuccessful.'); - } - } } diff --git a/src/Adapter/ResponseException.php b/src/Adapter/ResponseException.php index 22c01d4..d1c0ce2 100644 --- a/src/Adapter/ResponseException.php +++ b/src/Adapter/ResponseException.php @@ -8,6 +8,37 @@ namespace Cloudflare\API\Adapter; +use GuzzleHttp\Exception\RequestException; + class ResponseException extends \Exception { + /** + * Generates a ResponseException from a Guzzle RequestException. + * + * @param RequestException $err The client request exception (typicall 4xx or 5xx response). + * @return ResponseException + */ + public static function fromRequestException(RequestException $err): self + { + if (!$err->hasResponse()) { + return new ResponseException($err->getMessage(), 0, $err); + } + + $response = $err->getResponse(); + $contentType = $response->getHeaderLine('Content-Type'); + + // Attempt to derive detailed error from standard JSON response. + if (strpos($contentType, 'application/json') !== false) { + $json = json_decode($response->getBody()); + if (json_last_error() !== JSON_ERROR_NONE) { + return new ResponseException($err->getMessage(), 0, new JSONException(json_last_error_msg(), 0, $err)); + } + + if (isset($json->errors) && count($json->errors) >= 1) { + return new ResponseException($json->errors[0]->message, $json->errors[0]->code, $err); + } + } + + return new ResponseException($err->getMessage(), 0, $err); + } } diff --git a/tests/Adapter/GuzzleTest.php b/tests/Adapter/GuzzleTest.php index 505ed72..25208df 100644 --- a/tests/Adapter/GuzzleTest.php +++ b/tests/Adapter/GuzzleTest.php @@ -1,12 +1,6 @@ assertEquals('Testing a DELETE request.', $body->json->{'X-Delete-Test'}); } - public function testErrors() - { - $class = new ReflectionClass(\Cloudflare\API\Adapter\Guzzle::class); - $method = $class->getMethod('checkError'); - $method->setAccessible(true); - - $body = - '{ - "result": null, - "success": false, - "errors": [{"code":1003,"message":"Invalid or missing zone id."}], - "messages": [] - }' - ; - $response = new Response(200, [], $body); - - $this->expectException(\Cloudflare\API\Adapter\ResponseException::class); - $method->invokeArgs($this->client, [$response]); - - $body = - '{ - "result": null, - "success": false, - "errors": [], - "messages": [] - }' - ; - $response = new Response(200, [], $body); - - $this->expectException(\Cloudflare\API\Adapter\ResponseException::class); - $method->invokeArgs($this->client, [$response]); - - $body = 'this isnt json.'; - $response = new Response(200, [], $body); - - $this->expectException(\Cloudflare\API\Adapter\JSONException::class); - $method->invokeArgs($this->client, [$response]); - } - public function testNotFound() { - $this->expectException(\GuzzleHttp\Exception\RequestException::class); + $this->expectException(ResponseException::class); $this->client->get('https://httpbin.org/status/404'); } + + public function testServerError() + { + $this->expectException(ResponseException::class); + $this->client->get('https://httpbin.org/status/500'); + } } diff --git a/tests/Adapter/ResponseExceptionTest.php b/tests/Adapter/ResponseExceptionTest.php new file mode 100644 index 0000000..d876deb --- /dev/null +++ b/tests/Adapter/ResponseExceptionTest.php @@ -0,0 +1,78 @@ +assertInstanceOf(ResponseException::class, $respErr); + $this->assertEquals($reqErr->getMessage(), $respErr->getMessage()); + $this->assertEquals(0, $respErr->getCode()); + $this->assertEquals($reqErr, $respErr->getPrevious()); + } + + public function testFromRequestExceptionEmptyContentType() + { + $resp = new Response(404); + $reqErr = new RequestException('foo', new Request('GET', '/test'), $resp); + $respErr = ResponseException::fromRequestException($reqErr); + + $this->assertInstanceOf(ResponseException::class, $respErr); + $this->assertEquals($reqErr->getMessage(), $respErr->getMessage()); + $this->assertEquals(0, $respErr->getCode()); + $this->assertEquals($reqErr, $respErr->getPrevious()); + } + + + public function testFromRequestExceptionUnknownContentType() + { + $resp = new Response(404, ['Content-Type' => ['application/octet-stream']]); + $reqErr = new RequestException('foo', new Request('GET', '/test'), $resp); + $respErr = ResponseException::fromRequestException($reqErr); + + $this->assertInstanceOf(ResponseException::class, $respErr); + $this->assertEquals($reqErr->getMessage(), $respErr->getMessage()); + $this->assertEquals(0, $respErr->getCode()); + $this->assertEquals($reqErr, $respErr->getPrevious()); + } + + public function testFromRequestExceptionJSONDecodeError() + { + $resp = new Response(404, ['Content-Type' => ['application/json; charset=utf-8']], '[what]'); + $reqErr = new RequestException('foo', new Request('GET', '/test'), $resp); + $respErr = ResponseException::fromRequestException($reqErr); + + $this->assertInstanceOf(ResponseException::class, $respErr); + $this->assertEquals($reqErr->getMessage(), $respErr->getMessage()); + $this->assertEquals(0, $respErr->getCode()); + $this->assertInstanceOf(JSONException::class, $respErr->getPrevious()); + $this->assertEquals($reqErr, $respErr->getPrevious()->getPrevious()); + } + + public function testFromRequestExceptionJSONWithErrors() + { + $body = '{ + "result": null, + "success": false, + "errors": [{"code":1003, "message":"This is an error"}], + "messages": [] + }'; + + $resp = new Response(404, ['Content-Type' => ['application/json; charset=utf-8']], $body); + $reqErr = new RequestException('foo', new Request('GET', '/test'), $resp); + $respErr = ResponseException::fromRequestException($reqErr); + + $this->assertInstanceOf(ResponseException::class, $respErr); + $this->assertEquals('This is an error', $respErr->getMessage()); + $this->assertEquals(1003, $respErr->getCode()); + $this->assertEquals($reqErr, $respErr->getPrevious()); + } +}