saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
2.09k stars 107 forks source link

Get clean API response when an exception occurs #313

Closed darkons closed 1 year ago

darkons commented 1 year ago

After having created my first SDK for a Laravel API, I have encountered the following problem using AlwaysThrowOnErrors trait.

class MySDKService extends Connector
{
    use AlwaysThrowOnErrors;

    public function users(): UserResource
    {
        return new UserResource($this);
    }
}
class UserResource extends Resource
{
    public function list(array $queryParams = null): object
    {
        return $this->connector->send(
            new GetRequest(
                path: '/users',
                queryParams: $queryParams,
            )
        )->object();
    }
}

Now, in the case that, for example, Laravel responds with a validation exception (422), I can't get the validation errors from the response since Saloon constructs the exception message in the following way:

// Saloon\Exceptions\Request\RequestException.php
$message = sprintf('%s (%s) Response: %s', $statusCodeMessage, $status, $exceptionBodyMessage);

Am I doing something wrong? Should I do it in another way?

Thank you very much!

juse-less commented 1 year ago

Hi!

A few things I think would help make it easier to help:

  1. Which version of Saloon are you using?
  2. What content type is the response body in? (JSON, XML, plain text, etc)
  3. Could you check exactly which exceptions are thrown? Just to ensure all of them are based on the RequestException.
  4. Clarification on what you mean with 'can't get validation errors'. The logic you mention formats the response message, but the line right above it also cuts off the message after 200 characters. So it'd help knowing more exactly what the experienced issue is. 🙂

I'm not quite sure what the issue could be here. The logic/formatter you mention is only run if the RequestException (or its children calling RequestException::__construct()) isn't passed an error message. I'm not sure how often that happens or when, but could very well be the reason.

A possible solution in Saloon itself could be to check the response content type, and not set a custom message unless content type is text/plain. I.e., if you get a JSON response with the error messages, don't embed in the custom message as it's gonna be harder to deal with. Although, this would require additional work to handle errors in different formats (JSON, XML, etc), rather than giving null as the message, or the raw body string.

In the meantime, you could use RequestException::getResponse()->body() to retrieve the raw body. Or, in the case of JSON, RequestException::getResponse()->json().

Another, probably better, option would be to create- and use your own exceptions that extends the relevant ones. Like InvalidUserQueryParams, that extends UnprocessableEntityException, and stores the errors in a property. It's untested, but maybe something like

class InvalidUserQueryParamsException extends UnprocessableEntityException
{
    /**
     * @var array<string, string> field-message pair.
     */
    protected array $errors;

    public function __construct(Response $response, ?string $message = null, int $code = 0, ?Throwable $previous = null)
    {
        parent::__construct($response, $message, $code, $previous);

        $this->errors = $response->json();
    }

    /**
     * @return array<string, string> field-message pair.
     */
    public function errors(): array
    {
        return $this->errors;
    }
}

class ListUsersRequest extends Request
{
    public function getRequestException(Response $response, ?Throwable $senderException): ?Throwable
    {
        return match (true) {
            $response->status() === 422 => new InvalidUserQueryParamsException($response, previous: $senderException),
            default => parent::getRequestException($response, $senderException),
        }
    }
}
darkons commented 1 year ago

Hello @juse-less!

Thank you for your tips. I'm currently working with Saloon 3.0 and the Laravel API responses are JSON even when dealing with the typical validation exception.

This is the response of Laravel API after a validation error (422 code):

{
    "message": "The given data was invalid.",
    "errors": {
        "name": [
            "The name field is required."
        ],
        "email": [
            "The email field must be a valid email address."
        ]
    }
}

The problem is that Saloon captures the Laravel exception through RequestExceptionHelper class and throws a Saloon\Exceptions\Request\Statuses\UnprocessableEntityException formatting the response in a custom string like

$message = sprintf('%s (%s) Response: %s', $statusCodeMessage, $status, $exceptionBodyMessage);

instead return the pure JSON API response.

Thank you!

juse-less commented 1 year ago

I've edited this message more times than I can count on both hands and feet. 🙈 But I hope it makes sense.


@darkons Thanks!

I'm not sure what a possible solution could be in terms of formatting/not formatting the message. Keep in mind that the exception message is what ends up in logging/errors tracking systems (usually as the title), and as the message in log files. So using a JSON string might not be the best option. I'm also a bit unsure how various common logging and error tracking systems would cope with JSON (and larger messages). I'm aware that there are log/message processors, and some systems has them baked in, but still worth noting.

While I think it's doable in Saloon to handle different response content types, Saloon itself wouldn't know the specific APIs schema. So it'd be trickier if we want to handle things like error message and field messages individually. I guess Saloon could check if there's an element message at the root of the JSON/XML/etc, and use that.

So maybe use the plain/text body as message, or check message in the root. If neither is found, then don't set a message at all. Then, for accessing individual fields, use the response itself like I mentioned before:

$rawBodyString = RequestException::getResponse()->body();
$jsonBodyArray = RequestException::getResponse()->json();

But, since we know our SDKs/APIs best, using custom exceptions (at least for some of them, like UnprocessableEntityException) is probably the better solution in the end (at least in my mind). Especially since we can add custom response exceptions in the request itself, keeping it simple to manage.

Looking at the JSON output, I'd probably do something like this with custom exceptions:

class InvalidUserQueryParamsException extends UnprocessableEntityException
{
    /**
     * @var array<string, list<string>> field-message pair.
     */
    protected array $errors;

    public function __construct(Response $response, ?string $message = null, int $code = 0, ?Throwable $previous = null)
    {
        // Note: Since we pass our own message, Saloon will not truncate-, nor create a message from the body.
        //       I.e., the message from the JSON payload is used as actual message.
        parent::__construct($response, $response->json('message'), $code, $previous);

        $this->errors = $response->json('errors');
    }

    /**
     * @return array<string, list<string>> field-message pair.
     */
    public function errors(): array
    {
        return $this->errors;
    }
}

class ListUsersRequest extends Request
{
    public function getRequestException(Response $response, ?Throwable $senderException): ?Throwable
    {
        return match (true) {
            $response->status() === 422 => new InvalidUserQueryParamsException($response, previous: $senderException),
            default => parent::getRequestException($response, $senderException),
        }
    }
}

// Usage

try {
    $result = $connector->send(new ListUsersRequest(...));
} catch (InvalidUserQueryParamsException $ex) {
    // 'name' => ['Invalid name', '...'],
    // 'email' => ['Invalid email', '...'],
    $ex->errors();
}

I guess you could also add an optional argument to InvalidUserQueryParamsException::errors() for which field(s) you'd like to return.

I also realised that the connector also use the ManagesExceptions trait; so you can actually move that method from my ListUsersRequest above into the connector. Since errors like validation errors should be consistent across the same API/app, it might also be easier to manage.


class MyUnprocessableEntityException extends UnprocessableEntityException
{
    /**
     * @var array<string, list<string>> field-message pair.
     */
    protected array $errors;

    public function __construct(Response $response, ?string $message = null, int $code = 0, ?Throwable $previous = null)
    {
        // Note: Since we pass our own message, Saloon will not truncate-, nor create a message from the body.
        //       I.e., the message from the JSON payload is used as actual message.
        parent::__construct($response, $response->json('message'), $code, $previous);

        $this->errors = $response->json('errors');
    }

    /**
     * @param  string|list<string>|null  $fields
     * 
     * @return ($fields is string ? list<string> : array<string, list<string>>) Messages for single field, otherwise field-message pair.
     */
    public function errors(string|array|null $fields = null): array
    {
        return match (true) {
            is_array($fields) => array_intersect_key($this->errors, array_flip($fields)),
            is_string($fields) => $this->errors[ $fields ] ?? [],
            default => $this->errors,
        };
    }
}

class MySDKService extends Connector
{
    use AlwaysThrowOnErrors;

    public function getRequestException(Response $response, ?Throwable $senderException): ?Throwable
    {
        return match (true) {
            $response->status() === 422 => new MyUnprocessableEntityException($response, previous: $senderException),
            default => parent::getRequestException($response, $senderException),
        }
    }
}

// Usage

try {
    $result = $connector->send(new ListUsersRequest(...));
} catch (MyUnprocessableEntityException $ex) {
    // 'name' => ['Invalid name', '...'],
    // 'email' => ['Invalid email', '...'],
    $ex->errors();

    // ['Invalid email', '...']
    $ex->errors('email');

    // 'name' => ['Invalid name', '...'],
    $ex->errors(['name', 'non-existent-field']);
}
Sammyjo20 commented 1 year ago

I agree with @juse-less here - I think what you're after @darkons is the getResponse method on the exception? So you can catch the exception in your code like this:

try {
     $response = $connector->send($request);
} catch (UnprocessableEntityException $exception) {
     $errors = $exception->getResponse()->json('errors');

     // Do something with the errors
}

Juse's example is really good too as they cover how to extract this logic into your own exception class so you don't have to parse the errors every time.

darkons commented 1 year ago

After careful consideration, I think that the best thing to do is to create my own exception and catch possible errors from my API there.

Thank you very much to both of you!

Sammyjo20 commented 1 year ago

No worries! I’m going to close this issue for now - but if you need anything just reopen.