Merge pull request #164 from cloudflare/apalaistras/fix-request-error…

…-handling Adapter/Guzzle: Fix error handling for v4 API
cloudflare · May 28, 2021 · 1e58a65 · 1e58a65
2 parents 658ab3f + d2c4d22
commit 1e58a65
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 74 deletions.
diff --git a/src/Adapter/Guzzle.php b/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
@@ -74,36 +70,24 @@ 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'])) {
             throw new \InvalidArgumentException('Request method must be get, post, put, patch, or delete');
         }
 
-        $response = $this->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();
+        try {
+            $response = $this->client->$method($uri, [
+                'headers' => $headers,
+                ($method === 'get' ? 'query' : 'json') => $data,
+            ]);
+        } catch (RequestException $err) {
+            throw ResponseException::fromRequestException($err);
         }
 
-        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.');
-        }
+        return $response;
     }
 }
diff --git 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
@@ -1,12 +1,6 @@
 <?php
 
-/**
- * User: junade
- * Date: 13/01/2017
- * Time: 23:35
- */
-
-use GuzzleHttp\Psr7\Response;
+use Cloudflare\API\Adapter\ResponseException;
 
 class GuzzleTest extends TestCase
 {
@@ -89,48 +83,15 @@ public function testDelete()
         $this->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
@@ -0,0 +1,81 @@
+<?php
+
+use Cloudflare\API\Adapter\ResponseException;
+use Cloudflare\API\Adapter\JSONException;
+use GuzzleHttp\Exception\RequestException;
+use GuzzleHttp\Psr7\Request;
+use GuzzleHttp\Psr7\Response;
+
+/**
+ * @SuppressWarnings(PHPMD.StaticAccess)
+ */
+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());
+    }
+}