From facb70f34e99672a8e3a4180343811f38ef58e5b Mon Sep 17 00:00:00 2001
From: Eric Stern <eric@snapauth.app>
Date: Wed, 16 Oct 2024 09:44:37 -0700
Subject: [PATCH] Add more detail when errors occur (#24)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Adds several specific exceptions, including a CodedError with additional
codes, to handle various configuration and runtime errors. This converts
`ApiError` from a concrete class to an interface, which ensures that
catch blocks following the documented best practices will continue to
work as before.

Fixes #23

In a real application, you'll now (typically) get back some actionable
details.

![Screenshot 2024-09-25 at 12 23
03 PM](https://github.com/user-attachments/assets/9c357712-945e-4cb4-8365-3cf28f7b01ee)
---
 phpunit.xml                               |  2 +-
 src/ApiError.php                          |  4 +-
 src/Client.php                            | 69 ++++++++++++++---------
 src/ErrorCode.php                         | 30 ++++++++++
 src/Exception/CodedError.php              | 41 ++++++++++++++
 src/Exception/InvalidSecretKey.php        | 18 ++++++
 src/Exception/MalformedResponse.php       | 31 ++++++++++
 src/Exception/MissingSecretKey.php        | 19 +++++++
 src/Exception/Network.php                 | 28 +++++++++
 tests/ClientTest.php                      |  8 +--
 tests/Exception/CodedErrorTest.php        | 28 +++++++++
 tests/Exception/InvalidSecretKeyTest.php  | 21 +++++++
 tests/Exception/MalformedResponseTest.php | 22 ++++++++
 tests/Exception/MissingSecretKeyTest.php  | 21 +++++++
 tests/Exception/NetworkTest.php           | 23 ++++++++
 15 files changed, 329 insertions(+), 36 deletions(-)
 create mode 100644 src/ErrorCode.php
 create mode 100644 src/Exception/CodedError.php
 create mode 100644 src/Exception/InvalidSecretKey.php
 create mode 100644 src/Exception/MalformedResponse.php
 create mode 100644 src/Exception/MissingSecretKey.php
 create mode 100644 src/Exception/Network.php
 create mode 100644 tests/Exception/CodedErrorTest.php
 create mode 100644 tests/Exception/InvalidSecretKeyTest.php
 create mode 100644 tests/Exception/MalformedResponseTest.php
 create mode 100644 tests/Exception/MissingSecretKeyTest.php
 create mode 100644 tests/Exception/NetworkTest.php

diff --git a/phpunit.xml b/phpunit.xml
index 41d88c5..0ba7278 100644
--- a/phpunit.xml
+++ b/phpunit.xml
@@ -8,7 +8,7 @@
          failOnWarning="true"
          cacheDirectory=".phpunit.cache"
          requireCoverageMetadata="true"
-         beStrictAboutCoverageMetadata="true">
+         beStrictAboutCoverageMetadata="false">
     <testsuites>
         <testsuite name="default">
             <directory suffix="Test.php">tests</directory>
diff --git a/src/ApiError.php b/src/ApiError.php
index 6ddd4ba..bfc21ac 100644
--- a/src/ApiError.php
+++ b/src/ApiError.php
@@ -4,8 +4,6 @@
 
 namespace SnapAuth;
 
-use Exception;
-
-class ApiError extends Exception
+interface ApiError
 {
 }
diff --git a/src/Client.php b/src/Client.php
index 6664889..d538c9c 100644
--- a/src/Client.php
+++ b/src/Client.php
@@ -53,18 +53,12 @@ public function __construct(
         if ($secretKey === null) {
             $env = getenv('SNAPAUTH_SECRET_KEY');
             if ($env === false) {
-                throw new ApiError(
-                    'Secret key missing. It can be explictly provided, or it ' .
-                    'can be auto-detected from the SNAPAUTH_SECRET_KEY ' .
-                    'environment variable.',
-                );
+                throw new Exception\MissingSecretKey();
             }
             $secretKey = $env;
         }
         if (!str_starts_with($secretKey, 'secret_')) {
-            throw new ApiError(
-                'Invalid secret key. Please verify you copied the full value from the SnapAuth dashboard.',
-            );
+            throw new Exception\InvalidSecretKey();
         }
 
         $this->secretKey = $secretKey;
@@ -136,31 +130,52 @@ public function makeApiCall(string $route, array $params): array
             $code = curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
 
             if ($response === false || $errno !== CURLE_OK) {
-                $this->error();
+                throw new Exception\Network($errno);
             }
+        } finally {
+            curl_close($ch);
+        }
 
-            if ($code >= 300) {
-                $this->error();
-            }
-            // Handle non-200s, non-JSON (severe upstream error)
-            assert(is_string($response));
+        assert(is_string($response), 'No response body despite CURLOPT_RETURNTRANSFER');
+        try {
             $decoded = json_decode($response, true, flags: JSON_THROW_ON_ERROR);
-            assert(is_array($decoded));
-            return $decoded['result'];
         } catch (JsonException) {
-            $this->error();
-        } finally {
-            curl_close($ch);
+            // Received non-JSON response - wrap and rethrow
+            throw new Exception\MalformedResponse('Received non-JSON response', $code);
         }
-    }
 
-    /**
-     * TODO: specific error info!
-     */
-    private function error(): never
-    {
-        throw new ApiError();
-        // TODO: also make this more specific
+        if (!is_array($decoded) || !array_key_exists('result', $decoded)) {
+            // Received JSON response in an unexpected format
+            throw new Exception\MalformedResponse('Received JSON in an unexpected format', $code);
+        }
+
+        // Success!
+        if ($decoded['result'] !== null) {
+            assert($code >= 200 && $code < 300, 'Got a result with a non-2xx response');
+            return $decoded['result'];
+        }
+
+        // The `null` result indicated an error. Parse out the response shape
+        // more and throw an appropriate ApiError.
+        if (!array_key_exists('errors', $decoded)) {
+            throw new Exception\MalformedResponse('Error details missing', $code);
+        }
+        $errors = $decoded['errors'];
+        if (!is_array($errors) || !array_is_list($errors) || count($errors) === 0) {
+            throw new Exception\MalformedResponse('Error details are invalid or empty', $code);
+        }
+
+        $primaryError = $errors[0];
+        if (
+            !is_array($primaryError)
+            || !array_key_exists('code', $primaryError)
+            || !array_key_exists('message', $primaryError)
+        ) {
+            throw new Exception\MalformedResponse('Error details are invalid or empty', $code);
+        }
+
+        // Finally, the error details are known to be in the correct shape.
+        throw new Exception\CodedError($primaryError['message'], $primaryError['code'], $code);
     }
 
     public function __debugInfo(): array
diff --git a/src/ErrorCode.php b/src/ErrorCode.php
new file mode 100644
index 0000000..84fc85d
--- /dev/null
+++ b/src/ErrorCode.php
@@ -0,0 +1,30 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth;
+
+enum ErrorCode: string
+{
+    case AuthenticatingUserAccountNotFound = 'AuthenticatingUserAccountNotFound';
+    case EntityNotFound = 'EntityNotFound';
+    case HandleCannotChange = 'HandleCannotChange';
+    case HandleInUseByDifferentAccount = 'HandleInUseByDifferentAccount';
+    case InvalidAuthorizationHeader = 'InvalidAuthorizationHeader';
+    case InvalidInput = 'InvalidInput';
+    case PermissionViolation = 'PermissionViolation';
+    case PublishableKeyNotFound = 'PublishableKeyNotFound';
+    case RegisteredUserLimitReached = 'RegisteredUserLimitReached';
+    case SecretKeyExpired = 'SecretKeyExpired';
+    case SecretKeyNotFound = 'SecretKeyNotFound';
+    case TokenExpired = 'TokenExpired';
+    case TokenNotFound = 'TokenNotFound';
+    case UsingDeactivatedCredential = 'UsingDeactivatedCredential';
+
+    /**
+     * This is a catch-all code if the API has returned an error code that's
+     * unknown to this SDK. Often this means that a new SDK version will handle
+     * the new code.
+     */
+    case Unknown = '(unknown)';
+}
diff --git a/src/Exception/CodedError.php b/src/Exception/CodedError.php
new file mode 100644
index 0000000..239f54b
--- /dev/null
+++ b/src/Exception/CodedError.php
@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use RuntimeException;
+use SnapAuth\{
+    ApiError,
+    ErrorCode,
+};
+
+use function sprintf;
+
+/**
+ * The API returned a well-formed coded error message. Examine the $errorCode
+ * property for additional information.
+ */
+class CodedError extends RuntimeException implements ApiError
+{
+    public readonly ErrorCode $errorCode;
+
+    /**
+     * @param int $httpCode The HTTP status code of the error response
+     *
+     * @internal Constructing errors is not covered by BC
+     */
+    public function __construct(string $message, string $errorCode, int $httpCode)
+    {
+        parent::__construct(
+            message: sprintf(
+                '[HTTP %d] %s: %s',
+                $httpCode,
+                $errorCode,
+                $message,
+            ),
+            code: $httpCode,
+        );
+        $this->errorCode = ErrorCode::tryFrom($errorCode) ?? ErrorCode::Unknown;
+    }
+}
diff --git a/src/Exception/InvalidSecretKey.php b/src/Exception/InvalidSecretKey.php
new file mode 100644
index 0000000..a90b02a
--- /dev/null
+++ b/src/Exception/InvalidSecretKey.php
@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use InvalidArgumentException;
+use SnapAuth\ApiError;
+
+class InvalidSecretKey extends InvalidArgumentException implements ApiError
+{
+    public function __construct()
+    {
+        parent::__construct(
+            message: 'Invalid secret key. Please verify you copied the full value from the SnapAuth dashboard.',
+        );
+    }
+}
diff --git a/src/Exception/MalformedResponse.php b/src/Exception/MalformedResponse.php
new file mode 100644
index 0000000..5fe9073
--- /dev/null
+++ b/src/Exception/MalformedResponse.php
@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use RuntimeException;
+use SnapAuth\ApiError;
+
+use function sprintf;
+
+/**
+ * A response arrived, but was not in an expected format
+ */
+class MalformedResponse extends RuntimeException implements ApiError
+{
+    /**
+     * @internal Constructing errors is not covered by BC
+     */
+    public function __construct(string $details, int $statusCode)
+    {
+        parent::__construct(
+            message: sprintf(
+                '[HTTP %d] SnapAuth API returned data in an unexpected format: %s',
+                $statusCode,
+                $details,
+            ),
+            code: $statusCode,
+        );
+    }
+}
diff --git a/src/Exception/MissingSecretKey.php b/src/Exception/MissingSecretKey.php
new file mode 100644
index 0000000..0305069
--- /dev/null
+++ b/src/Exception/MissingSecretKey.php
@@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use InvalidArgumentException;
+use SnapAuth\ApiError;
+
+class MissingSecretKey extends InvalidArgumentException implements ApiError
+{
+    public function __construct()
+    {
+        parent::__construct(
+            'Secret key missing. It can be explictly provided, or it can be ' .
+            'auto-detected from the SNAPAUTH_SECRET_KEY environment variable.'
+        );
+    }
+}
diff --git a/src/Exception/Network.php b/src/Exception/Network.php
new file mode 100644
index 0000000..a38fdf2
--- /dev/null
+++ b/src/Exception/Network.php
@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use RuntimeException;
+use SnapAuth\ApiError;
+
+/**
+ * A network interruption occurred.
+ */
+class Network extends RuntimeException implements ApiError
+{
+    /**
+     * @param $code a cURL error code
+     *
+     * @internal Constructing errors is not covered by BC
+     */
+    public function __construct(int $code)
+    {
+        $message = curl_strerror($code);
+        parent::__construct(
+            message: 'SnapAuth network error: ' . $message,
+            code: $code,
+        );
+    }
+}
diff --git a/tests/ClientTest.php b/tests/ClientTest.php
index dace054..75fe85d 100644
--- a/tests/ClientTest.php
+++ b/tests/ClientTest.php
@@ -23,22 +23,20 @@ public function testConstructSecretKeyAutodetectInvalid(): void
     {
         assert(getenv('SNAPAUTH_SECRET_KEY') === false);
         putenv('SNAPAUTH_SECRET_KEY=invalid');
-        self::expectException(ApiError::class);
-        self::expectExceptionMessage('Invalid secret key.');
+        self::expectException(Exception\InvalidSecretKey::class);
         new Client();
     }
 
     public function testConstructSecretKeyAutodetectMissing(): void
     {
         assert(getenv('SNAPAUTH_SECRET_KEY') === false);
-        self::expectException(ApiError::class);
-        self::expectExceptionMessage('Secret key missing.');
+        self::expectException(Exception\MissingSecretKey::class);
         new Client();
     }
 
     public function testSecretKeyValidation(): void
     {
-        self::expectException(ApiError::class);
+        self::expectException(Exception\InvalidSecretKey::class);
         new Client(secretKey: 'not_a_secret');
     }
 
diff --git a/tests/Exception/CodedErrorTest.php b/tests/Exception/CodedErrorTest.php
new file mode 100644
index 0000000..080a854
--- /dev/null
+++ b/tests/Exception/CodedErrorTest.php
@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use SnapAuth\ErrorCode;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Small;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(CodedError::class)]
+#[Small]
+class CodedErrorTest extends TestCase
+{
+    public function testFormattingFromKnownErrorCode(): void
+    {
+        $e = new CodedError('Missing parameter foo', 'InvalidInput', 400);
+        self::assertSame(ErrorCode::InvalidInput, $e->errorCode);
+    }
+
+    public function testFormattingFromUnknownErrorCode(): void
+    {
+        $e = new CodedError('Something bad happened', 'SevereError', 400);
+        self::assertSame(ErrorCode::Unknown, $e->errorCode);
+    }
+}
diff --git a/tests/Exception/InvalidSecretKeyTest.php b/tests/Exception/InvalidSecretKeyTest.php
new file mode 100644
index 0000000..449511b
--- /dev/null
+++ b/tests/Exception/InvalidSecretKeyTest.php
@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Small;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(InvalidSecretKey::class)]
+#[Small]
+class InvalidSecretKeyTest extends TestCase
+{
+    public function testConstruct(): void
+    {
+        $e = new InvalidSecretKey();
+        self::assertStringContainsString('Invalid secret key', $e->getMessage());
+    }
+}
diff --git a/tests/Exception/MalformedResponseTest.php b/tests/Exception/MalformedResponseTest.php
new file mode 100644
index 0000000..a8cc765
--- /dev/null
+++ b/tests/Exception/MalformedResponseTest.php
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Small;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(MalformedResponse::class)]
+#[Small]
+class MalformedResponseTest extends TestCase
+{
+    public function testConstruct(): void
+    {
+        $e = new MalformedResponse('Invalid data', 503);
+        self::assertStringContainsString('Invalid data', $e->getMessage());
+        self::assertSame(503, $e->getCode());
+    }
+}
diff --git a/tests/Exception/MissingSecretKeyTest.php b/tests/Exception/MissingSecretKeyTest.php
new file mode 100644
index 0000000..f968876
--- /dev/null
+++ b/tests/Exception/MissingSecretKeyTest.php
@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Small;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(MissingSecretKey::class)]
+#[Small]
+class MissingSecretKeyTest extends TestCase
+{
+    public function testConstruct(): void
+    {
+        $e = new MissingSecretKey();
+        self::assertStringContainsString('SNAPAUTH_SECRET_KEY', $e->getMessage());
+    }
+}
diff --git a/tests/Exception/NetworkTest.php b/tests/Exception/NetworkTest.php
new file mode 100644
index 0000000..c80e04d
--- /dev/null
+++ b/tests/Exception/NetworkTest.php
@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SnapAuth\Exception;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Small;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(Network::class)]
+#[Small]
+class NetworkTest extends TestCase
+{
+    public function testConstruct(): void
+    {
+        $code = CURLE_OPERATION_TIMEDOUT;
+        $e = new Network($code);
+        // @phpstan-ignore argument.type (curl_strerror will not return null here)
+        self::assertStringContainsString(curl_strerror($code), $e->getMessage());
+    }
+}