From c58f340633a5ecb1368749204d3fac4f3f71f89f Mon Sep 17 00:00:00 2001 From: Alex Palaistras Date: Wed, 10 Mar 2021 11:16:24 +0000 Subject: [PATCH 1/2] 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 | 53 +++-------------- tests/Adapter/ResponseExceptionTest.php | 78 +++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 74 deletions(-) create mode 100644 tests/Adapter/ResponseExceptionTest.php diff --git a/src/Adapter/Guzzle.php b/src/Adapter/Guzzle.php index a7d8ad1c..2cc76361 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); - - 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); + try { + $response = $this->client->$method($uri, [ + 'headers' => $headers, + ($method === 'get' ? 'query' : 'json') => $data, + ]); + } catch (RequestException $err) { + throw ResponseException::fromRequestException($err); } - if (isset($json->success) && !$json->success) { - throw new ResponseException('Request was unsuccessful.'); - } + return $response; } } diff --git a/src/Adapter/ResponseException.php b/src/Adapter/ResponseException.php index 22c01d44..d1c0ce29 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 505ed72e..25208df1 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() + public function testNotFound() { - $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]); + $this->expectException(ResponseException::class); + $this->client->get('https://httpbin.org/status/404'); } - public function testNotFound() + public function testServerError() { - $this->expectException(\GuzzleHttp\Exception\RequestException::class); - $this->client->get('https://httpbin.org/status/404'); + $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 00000000..d876deb4 --- /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()); + } +} From d2c4d225e0c438e0be9e649446fb8c6f8b4724fc Mon Sep 17 00:00:00 2001 From: Jacob Bednarz Date: Fri, 28 May 2021 10:32:02 +1000 Subject: [PATCH 2/2] suppress warnings on static access --- src/Adapter/Guzzle.php | 3 +++ tests/Adapter/ResponseExceptionTest.php | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/Adapter/Guzzle.php b/src/Adapter/Guzzle.php index 2cc76361..71e7dd75 100644 --- a/src/Adapter/Guzzle.php +++ b/src/Adapter/Guzzle.php @@ -70,6 +70,9 @@ public function delete(string $uri, array $data = [], array $headers = []): Resp return $this->request('delete', $uri, $data, $headers); } + /** + * @SuppressWarnings(PHPMD.StaticAccess) + */ public function request(string $method, string $uri, array $data = [], array $headers = []) { if (!in_array($method, ['get', 'post', 'put', 'patch', 'delete'])) { diff --git a/tests/Adapter/ResponseExceptionTest.php b/tests/Adapter/ResponseExceptionTest.php index d876deb4..8283e0fc 100644 --- a/tests/Adapter/ResponseExceptionTest.php +++ b/tests/Adapter/ResponseExceptionTest.php @@ -6,6 +6,9 @@ use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; +/** + * @SuppressWarnings(PHPMD.StaticAccess) + */ class ResponseExceptionTest extends TestCase { public function testFromRequestExceptionNoResponse()