From 5109e04018f8e5894739bd7ae5aa203979ce87f3 Mon Sep 17 00:00:00 2001 From: Adrien Crivelli Date: Thu, 2 Nov 2023 19:20:02 +0900 Subject: [PATCH] Felix decides how the client handles errors #9900 Because webonyx/graphql dropped support for `category` we need a new mechanism to decide how to handle error in the client. This is now the role of Felix and the client is simplified. An error may have one the extension set to true: - `showSnack` means the error has a friendly user message that should be shown via a snackbar (not a full page error) - `objectNotFound` means an object was not found. The full page error does not suggest reloading, since it is not an API incompatibility issue All other errors are redirected to full page error with a suggestion to reload the app because we suspect an incompatibility between client and server (or a temporary failure of the server, but we don't want to let the client enter an incoherent state) --- src/Api/Server.php | 15 ++- tests/Api/ServerTest.php | 191 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 200 insertions(+), 6 deletions(-) diff --git a/src/Api/Server.php b/src/Api/Server.php index 1769512..31d441b 100644 --- a/src/Api/Server.php +++ b/src/Api/Server.php @@ -5,8 +5,8 @@ namespace Ecodev\Felix\Api; use Doctrine\DBAL\Exception\DriverException; -use GraphQL\Error\ClientAware; use GraphQL\Error\DebugFlag; +use GraphQL\Error\UserError; use GraphQL\Executor\ExecutionResult; use GraphQL\GraphQL; use GraphQL\Server\ServerConfig; @@ -102,9 +102,16 @@ private function handleError(Throwable $exception, callable $formatter): array $result = $formatter($exception); - if (!$exception instanceof ClientAware || !$exception->isClientSafe()) { - $result['extensions'] ??= []; - $result['extensions']['category'] = 'internal'; + // Invalid variable that end-user might have crafted via the URL + $isInvalidVariables = preg_match('~^Variable \".*\" got invalid value ~', $exception->getMessage()); + $isFelixException = $exception->getPrevious() instanceof Exception; + if ($isFelixException || $isInvalidVariables) { + $result['extensions']['showSnack'] = true; + } + + // Not found object + if ($exception->getPrevious() instanceof UserError && preg_match('~^Entity not found for class `~', $exception->getMessage())) { + $result['extensions']['objectNotFound'] = true; } return $result; diff --git a/tests/Api/ServerTest.php b/tests/Api/ServerTest.php index c84f15a..29dcdab 100644 --- a/tests/Api/ServerTest.php +++ b/tests/Api/ServerTest.php @@ -5,21 +5,81 @@ namespace EcodevTests\Felix\Api; use Ecodev\Felix\Api\Server; +use EcodevTests\Felix\Traits\TestWithContainer; +use Exception; +use GraphQL\Error\UserError; use GraphQL\Executor\ExecutionResult; use GraphQL\Type\Definition\ObjectType; +use GraphQL\Type\Definition\Type; use GraphQL\Type\Schema; +use Laminas\ConfigAggregator\ArrayProvider; +use Laminas\ConfigAggregator\ConfigAggregator; use Laminas\Diactoros\CallbackStream; use Laminas\Diactoros\ServerRequest; +use Laminas\Log\LoggerInterface; use PHPUnit\Framework\TestCase; class ServerTest extends TestCase { + use TestWithContainer; + + /** + * @var LoggerInterface&\PHPUnit\Framework\MockObject\MockObject + */ + private LoggerInterface $logger; + + protected function setUp(): void + { + $this->logger = $this->createMock(LoggerInterface::class); + $aggregator = new ConfigAggregator([ + new ArrayProvider([ + 'dependencies' => [ + 'factories' => [ + LoggerInterface::class => fn () => $this->logger, + ], + ], + ]), + ]); + + $this->createContainer($aggregator); + } + /** * @dataProvider providerExecute */ - public function testExecute(string $body, array $expected): void + public function testExecute(string $body, array $expected, string $expectedLog = ''): void { - $schema = new Schema(['query' => new ObjectType(['name' => 'Query', 'fields' => []])]); + $schema = new Schema(['query' => new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'nativeException' => [ + 'type' => Type::boolean(), + 'resolve' => fn () => throw new Exception('Fake message'), + ], + 'felixException' => [ + 'type' => Type::boolean(), + 'resolve' => fn () => throw new \Ecodev\Felix\Api\Exception('Fake message'), + ], + 'notFoundException' => [ + 'type' => Type::boolean(), + 'resolve' => fn () => throw new UserError('Entity not found for class `foo` and ID `bar`.'), + ], + 'invalidVariables' => [ + 'args' => [ + 'myArg' => Type::boolean(), + ], + 'type' => Type::boolean(), + 'resolve' => fn () => true, + ], + ], + ])]); + + if ($expectedLog) { + $this->logger->expects(self::once())->method('err')->with($expectedLog); + } else { + $this->logger->expects(self::never())->method('err'); + } + $server = new Server($schema, false); $request = new ServerRequest(method: 'POST', body: new CallbackStream(fn () => $body)); $request = $request->withHeader('content-type', 'application/json'); @@ -73,5 +133,132 @@ public static function providerExecute(): iterable ], ], ]; + + yield 'native exception' => [ + '{"query": "{ nativeException }"}', + [ + 'errors' => [ + [ + 'message' => 'Internal server error', + 'locations' => [ + [ + 'line' => 1, + 'column' => 3, + ], + ], + 'path' => [ + 'nativeException', + ], + ], + ], + 'data' => [ + 'nativeException' => null, + ], + ], + << [ + '{"query": "{ felixException }"}', + [ + 'errors' => [ + [ + 'message' => 'Fake message', + 'locations' => [ + [ + 'line' => 1, + 'column' => 3, + ], + ], + 'path' => [ + 'felixException', + ], + 'extensions' => [ + 'showSnack' => true, + ], + ], + ], + 'data' => [ + 'felixException' => null, + ], + ], + << [ + '{"query": "{ notFoundException }"}', + [ + 'errors' => [ + [ + 'message' => 'Entity not found for class `foo` and ID `bar`.', + 'locations' => [ + [ + 'line' => 1, + 'column' => 3, + ], + ], + 'path' => [ + 'notFoundException', + ], + 'extensions' => [ + 'objectNotFound' => true, + ], + ], + ], + 'data' => [ + 'notFoundException' => null, + ], + ], + << [ + '{"query": "query ($v: Boolean) { invalidVariables(myArg: $v) }", "variables": {"v": 123}}', + [ + 'errors' => [ + [ + 'message' => 'Variable "$v" got invalid value 123; Boolean cannot represent a non boolean value: 123', + 'locations' => [ + [ + 'line' => 1, + 'column' => 8, + ], + ], + 'extensions' => [ + 'showSnack' => true, + ], + ], + ], + ], + <<