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

@@ -1,14 +1,10 @@
 <?php
-/**
- * User: junade
- * Date: 13/01/2017
- * Time: 18:26
- */
 
 namespace Cloudflare\API\Adapter;
 
 use Cloudflare\API\Auth\Auth;
 use GuzzleHttp\Client;
+use GuzzleHttp\Exception\RequestException;
 use Psr\Http\Message\ResponseInterface;
 
 class Guzzle implements Adapter
@@ -80,30 +76,15 @@ class Guzzle implements Adapter
             throw new \InvalidArgumentException('Request method must be get, post, put, patch, or delete');
         }
 
-        $response = $this->client->$method($uri, [
+        try {
-            'headers' => $headers,
+            $response = $this->client->$method($uri, [
-            ($method === 'get' ? 'query' : 'json') => $data,
+                'headers' => $headers,
-        ]);
+                ($method === 'get' ? 'query' : 'json') => $data,
-
+            ]);
-        $this->checkError($response);
+        } 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.');
-        }
-    }
 }

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);
+    }
 }

tests/Adapter/GuzzleTest.php

@@ -1,12 +1,6 @@
 <?php
 
-/**
+use Cloudflare\API\Adapter\ResponseException;
- * User: junade
- * Date: 13/01/2017
- * Time: 23:35
- */
-
-use GuzzleHttp\Psr7\Response;
 
 class GuzzleTest extends TestCase
 {
@@ -89,48 +83,15 @@ class GuzzleTest extends TestCase
         $this->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');
+    }
 }

tests/Adapter/ResponseExceptionTest.php

@@ -0,0 +1,78 @@
+<?php
+
+use Cloudflare\API\Adapter\ResponseException;
+use Cloudflare\API\Adapter\JSONException;
+use GuzzleHttp\Exception\RequestException;
+use GuzzleHttp\Psr7\Request;
+use GuzzleHttp\Psr7\Response;
+
+class ResponseExceptionTest extends TestCase
+{
+    public function testFromRequestExceptionNoResponse()
+    {
+        $reqErr = new RequestException('foo', new Request('GET', '/test'));
+        $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 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());
+    }
+}