From cdab18d099532f02eb001fe2b52648d37d7cea83 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sat, 18 Apr 2020 22:59:57 +0200 Subject: [PATCH 1/8] Add error handler that reports to Laravel's ExceptionHandler --- src/Execution/ErrorHandler.php | 1 + src/Execution/ExtensionErrorHandler.php | 10 +++---- src/Execution/ReportErrorHandler.php | 22 +++++++++++++++ src/lighthouse.php | 1 + tests/Integration/ReportHandlerTest.php | 37 +++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 src/Execution/ReportErrorHandler.php create mode 100644 tests/Integration/ReportHandlerTest.php diff --git a/src/Execution/ErrorHandler.php b/src/Execution/ErrorHandler.php index 13a7c689e1..c8b5ac49cd 100644 --- a/src/Execution/ErrorHandler.php +++ b/src/Execution/ErrorHandler.php @@ -12,6 +12,7 @@ interface ErrorHandler * * Always call $next($error) to keep the Pipeline going. Multiple such Handlers may be registered * as an array in the config. + * TODO change to non-static in v5 */ public static function handle(Error $error, Closure $next): array; } diff --git a/src/Execution/ExtensionErrorHandler.php b/src/Execution/ExtensionErrorHandler.php index edd48e0388..1373ea6fa4 100644 --- a/src/Execution/ExtensionErrorHandler.php +++ b/src/Execution/ExtensionErrorHandler.php @@ -6,13 +6,13 @@ use GraphQL\Error\Error; use Nuwave\Lighthouse\Exceptions\RendersErrorsExtensions; +/** + * Handle Exceptions that implement Nuwave\Lighthouse\Exceptions\RendersErrorsExtensions + * and add extra content from them to the 'extensions' key of the Error that is rendered + * to the User. + */ class ExtensionErrorHandler implements ErrorHandler { - /** - * Handle Exceptions that implement Nuwave\Lighthouse\Exceptions\RendersErrorsExtensions - * and add extra content from them to the 'extensions' key of the Error that is rendered - * to the User. - */ public static function handle(Error $error, Closure $next): array { $underlyingException = $error->getPrevious(); diff --git a/src/Execution/ReportErrorHandler.php b/src/Execution/ReportErrorHandler.php new file mode 100644 index 0000000000..3d96debe60 --- /dev/null +++ b/src/Execution/ReportErrorHandler.php @@ -0,0 +1,22 @@ +report($error); + + return $next($error); + } +} diff --git a/src/lighthouse.php b/src/lighthouse.php index 5f74410184..72636ecb12 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -180,6 +180,7 @@ 'error_handlers' => [ \Nuwave\Lighthouse\Execution\ExtensionErrorHandler::class, + \Nuwave\Lighthouse\Execution\ReportErrorHandler::class, ], /* diff --git a/tests/Integration/ReportHandlerTest.php b/tests/Integration/ReportHandlerTest.php new file mode 100644 index 0000000000..8525513d4a --- /dev/null +++ b/tests/Integration/ReportHandlerTest.php @@ -0,0 +1,37 @@ +mockResolver(function () use ($exception) { + throw $exception; + }); + + $handler = $this->mock(ExceptionHandler::class); + $handler + ->shouldReceive('report') + ->with($exception); + app()->singleton(ExceptionHandler::class, function () use ($handler) { + return $handler; + }); + + $this->schema = /** @lang GraphQL */ ' + type Query { + foo: ID @mock + } + '; + + $this->graphQL(/** @lang GraphQL */ ' + { + foo + } + '); + } +} From 9cedd34e84a9e827ee4d8f872571c0dede7bf677 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sat, 18 Apr 2020 23:05:52 +0200 Subject: [PATCH 2/8] mock --- tests/Integration/ReportHandlerTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/Integration/ReportHandlerTest.php b/tests/Integration/ReportHandlerTest.php index 8525513d4a..28cac40cf6 100644 --- a/tests/Integration/ReportHandlerTest.php +++ b/tests/Integration/ReportHandlerTest.php @@ -14,9 +14,10 @@ public function testReportsErrors(): void throw $exception; }); - $handler = $this->mock(ExceptionHandler::class); + $handler = $this->createMock(ExceptionHandler::class); $handler - ->shouldReceive('report') + ->expects($this->once()) + ->method('report') ->with($exception); app()->singleton(ExceptionHandler::class, function () use ($handler) { return $handler; From 6d358536a59682b4334c9c472a984194d0803d3e Mon Sep 17 00:00:00 2001 From: spawnia Date: Sun, 19 Apr 2020 10:33:46 +0200 Subject: [PATCH 3/8] Only throw previous --- src/Execution/ReportErrorHandler.php | 12 +++++++++--- src/Testing/MocksResolvers.php | 4 +--- tests/Integration/ReportHandlerTest.php | 2 +- tests/Unit/Schema/Directives/Fixtures/Foo.php | 1 - tests/Unit/Schema/SecurityTest.php | 14 +++++++------- tests/Unit/Subscriptions/StorageManagerTest.php | 4 +--- tests/Unit/Subscriptions/SubscriberTest.php | 4 +--- 7 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/Execution/ReportErrorHandler.php b/src/Execution/ReportErrorHandler.php index 3d96debe60..89c313a825 100644 --- a/src/Execution/ReportErrorHandler.php +++ b/src/Execution/ReportErrorHandler.php @@ -13,9 +13,15 @@ class ReportErrorHandler implements ErrorHandler { public static function handle(Error $error, Closure $next): array { - /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ - $reporter = app(ExceptionHandler::class); - $reporter->report($error); + $previous = $error->getPrevious(); + + // If the Error does not wrap another Error, it is related to a client error + // and shown in the error response anyway, we don't really need to report it + if($previous) { + /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ + $reporter = app(ExceptionHandler::class); + $reporter->report($previous); + } return $next($error); } diff --git a/src/Testing/MocksResolvers.php b/src/Testing/MocksResolvers.php index 3a76f35ccc..9bfd39ad80 100644 --- a/src/Testing/MocksResolvers.php +++ b/src/Testing/MocksResolvers.php @@ -34,9 +34,7 @@ protected function mockResolver($resolverOrValue = null, string $key = 'default' */ protected function mockResolverExpects(/* TODO add strong type hint when bumping PHPUnit */ $invocationOrder, string $key = 'default'): InvocationMocker { - $mock = $this - ->getMockBuilder(MockResolver::class) - ->getMock(); + $mock = $this->createMock(MockResolver::class); $this->registerMockResolver($mock, $key); diff --git a/tests/Integration/ReportHandlerTest.php b/tests/Integration/ReportHandlerTest.php index 28cac40cf6..294d9425fe 100644 --- a/tests/Integration/ReportHandlerTest.php +++ b/tests/Integration/ReportHandlerTest.php @@ -16,7 +16,7 @@ public function testReportsErrors(): void $handler = $this->createMock(ExceptionHandler::class); $handler - ->expects($this->once()) + ->expects($this->atLeastOnce()) ->method('report') ->with($exception); app()->singleton(ExceptionHandler::class, function () use ($handler) { diff --git a/tests/Unit/Schema/Directives/Fixtures/Foo.php b/tests/Unit/Schema/Directives/Fixtures/Foo.php index ba2eaee2bd..ce315428a0 100644 --- a/tests/Unit/Schema/Directives/Fixtures/Foo.php +++ b/tests/Unit/Schema/Directives/Fixtures/Foo.php @@ -10,6 +10,5 @@ class Foo { public function bar() { - // } } diff --git a/tests/Unit/Schema/SecurityTest.php b/tests/Unit/Schema/SecurityTest.php index d7712a279b..f450a06b5d 100644 --- a/tests/Unit/Schema/SecurityTest.php +++ b/tests/Unit/Schema/SecurityTest.php @@ -13,11 +13,11 @@ public function testCanSetMaxComplexityThroughConfig(): void { config(['lighthouse.security.max_query_complexity' => 1]); - $this->schema = ' + $this->schema = /** @lang GraphQL */ ' type Query { user: User @first } - + type User { name: String } @@ -30,11 +30,11 @@ public function testCanSetMaxDepthThroughConfig(): void { config(['lighthouse.security.max_query_depth' => 1]); - $this->schema = ' + $this->schema = /** @lang GraphQL */ ' type Query { user: User @first } - + type User { name: String user: User @@ -53,7 +53,7 @@ public function testCanDisableIntrospectionThroughConfig(): void protected function assertMaxQueryComplexityIs1(): void { - $result = $this->graphQL(' + $result = $this->graphQL(/** @lang GraphQL */ ' { user { name @@ -69,7 +69,7 @@ protected function assertMaxQueryComplexityIs1(): void protected function assertMaxQueryDepthIs1(): void { - $result = $this->graphQL(' + $result = $this->graphQL(/** @lang GraphQL */ ' { user { user { @@ -89,7 +89,7 @@ protected function assertMaxQueryDepthIs1(): void protected function assertIntrospectionIsDisabled(): void { - $result = $this->graphQL(' + $result = $this->graphQL(/** @lang GraphQL */ ' { __schema { queryType { diff --git a/tests/Unit/Subscriptions/StorageManagerTest.php b/tests/Unit/Subscriptions/StorageManagerTest.php index a941c78104..307a723b56 100644 --- a/tests/Unit/Subscriptions/StorageManagerTest.php +++ b/tests/Unit/Subscriptions/StorageManagerTest.php @@ -37,9 +37,7 @@ protected function setUp(): void protected function subscriber(string $queryString): Subscriber { /** @var \Nuwave\Lighthouse\Subscriptions\Subscriber $subscriber */ - $subscriber = $this->getMockBuilder(Subscriber::class) - ->disableOriginalConstructor() - ->getMock(); + $subscriber = $this->createMock(Subscriber::class); $subscriber->channel = Subscriber::uniqueChannelName(); $subscriber->query = Parser::parse($queryString); diff --git a/tests/Unit/Subscriptions/SubscriberTest.php b/tests/Unit/Subscriptions/SubscriberTest.php index 62444baf23..984512cc12 100644 --- a/tests/Unit/Subscriptions/SubscriberTest.php +++ b/tests/Unit/Subscriptions/SubscriberTest.php @@ -25,9 +25,7 @@ public function testSerializable(): void { $args = ['foo' => 'bar']; - $resolveInfo = $this->getMockBuilder(ResolveInfo::class) - ->disableOriginalConstructor() - ->getMock(); + $resolveInfo = $this->createMock(ResolveInfo::class); $operationName = 'baz'; $resolveInfo->operation = (object) [ 'name' => (object) [ From fb5e09e7352b24f53ec1104dbe415379ed2e7e9a Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Sun, 19 Apr 2020 08:33:59 +0000 Subject: [PATCH 4/8] Apply fixes from StyleCI --- src/Execution/ReportErrorHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Execution/ReportErrorHandler.php b/src/Execution/ReportErrorHandler.php index 89c313a825..c626ac5ab2 100644 --- a/src/Execution/ReportErrorHandler.php +++ b/src/Execution/ReportErrorHandler.php @@ -17,7 +17,7 @@ public static function handle(Error $error, Closure $next): array // If the Error does not wrap another Error, it is related to a client error // and shown in the error response anyway, we don't really need to report it - if($previous) { + if ($previous) { /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ $reporter = app(ExceptionHandler::class); $reporter->report($previous); From 770bacbd33b0dafe5f9f8e392c3889af889ac282 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sun, 19 Apr 2020 12:34:13 +0200 Subject: [PATCH 5/8] Rename to ReportingErrorHandler.php, add TODO to use DI --- .../{ReportErrorHandler.php => ReportingErrorHandler.php} | 3 ++- src/lighthouse.php | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) rename src/Execution/{ReportErrorHandler.php => ReportingErrorHandler.php} (85%) diff --git a/src/Execution/ReportErrorHandler.php b/src/Execution/ReportingErrorHandler.php similarity index 85% rename from src/Execution/ReportErrorHandler.php rename to src/Execution/ReportingErrorHandler.php index c626ac5ab2..4a235167f6 100644 --- a/src/Execution/ReportErrorHandler.php +++ b/src/Execution/ReportingErrorHandler.php @@ -9,7 +9,7 @@ /** * Report errors through the default exception handler configured in Laravel. */ -class ReportErrorHandler implements ErrorHandler +class ReportingErrorHandler implements ErrorHandler { public static function handle(Error $error, Closure $next): array { @@ -18,6 +18,7 @@ public static function handle(Error $error, Closure $next): array // If the Error does not wrap another Error, it is related to a client error // and shown in the error response anyway, we don't really need to report it if ($previous) { + // TODO inject through constructor once handle is non-static /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ $reporter = app(ExceptionHandler::class); $reporter->report($previous); diff --git a/src/lighthouse.php b/src/lighthouse.php index 72636ecb12..bbb19aa0c5 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -180,7 +180,7 @@ 'error_handlers' => [ \Nuwave\Lighthouse\Execution\ExtensionErrorHandler::class, - \Nuwave\Lighthouse\Execution\ReportErrorHandler::class, + \Nuwave\Lighthouse\Execution\ReportingErrorHandler::class, ], /* From 6adf4cf4ebf96d55d8887fea937b547555f6be19 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sun, 19 Apr 2020 15:22:43 +0200 Subject: [PATCH 6/8] Report errors that are not client safe --- src/Execution/ReportingErrorHandler.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Execution/ReportingErrorHandler.php b/src/Execution/ReportingErrorHandler.php index 4a235167f6..c0e47b1c91 100644 --- a/src/Execution/ReportingErrorHandler.php +++ b/src/Execution/ReportingErrorHandler.php @@ -13,17 +13,17 @@ class ReportingErrorHandler implements ErrorHandler { public static function handle(Error $error, Closure $next): array { - $previous = $error->getPrevious(); - - // If the Error does not wrap another Error, it is related to a client error - // and shown in the error response anyway, we don't really need to report it - if ($previous) { - // TODO inject through constructor once handle is non-static - /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ - $reporter = app(ExceptionHandler::class); - $reporter->report($previous); + // Client-safe errors are assumed to be something that a client can handle + // or is expected to happen, e.g. wrong syntax, authentication or validation + if ($error->isClientSafe()) { + return $next($error); } + // TODO inject through constructor once handle is non-static + /** @var \Illuminate\Contracts\Debug\ExceptionHandler $reporter */ + $reporter = app(ExceptionHandler::class); + $reporter->report($error->getPrevious()); + return $next($error); } } From f85401a6a544faf6ad6c84119665d5c307645af1 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sun, 19 Apr 2020 15:23:43 +0200 Subject: [PATCH 7/8] CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff55a8509d..ac01cfa8ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ You can find and compare releases at the [GitHub release page](https://github.co - Remove subscriber reference from topic when deleted https://github.com/nuwave/lighthouse/pull/1288 - Improve subscription context serializer https://github.com/nuwave/lighthouse/pull/1283 - Allow replacing the `SubscriptionRegistry` implementation using the container https://github.com/nuwave/lighthouse/pull/1286 +- Report errors that are not client-safe through Laravel's `ExceptionHandler` https://github.com/nuwave/lighthouse/pull/1303 ## 4.11.0 From f84b5cf4fd01912067625a76d98f5b2661b5b2a9 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sun, 19 Apr 2020 15:28:41 +0200 Subject: [PATCH 8/8] Add test --- tests/Integration/ReportHandlerTest.php | 38 ------------ .../Integration/ReportingErrorHandlerTest.php | 62 +++++++++++++++++++ 2 files changed, 62 insertions(+), 38 deletions(-) delete mode 100644 tests/Integration/ReportHandlerTest.php create mode 100644 tests/Integration/ReportingErrorHandlerTest.php diff --git a/tests/Integration/ReportHandlerTest.php b/tests/Integration/ReportHandlerTest.php deleted file mode 100644 index 294d9425fe..0000000000 --- a/tests/Integration/ReportHandlerTest.php +++ /dev/null @@ -1,38 +0,0 @@ -mockResolver(function () use ($exception) { - throw $exception; - }); - - $handler = $this->createMock(ExceptionHandler::class); - $handler - ->expects($this->atLeastOnce()) - ->method('report') - ->with($exception); - app()->singleton(ExceptionHandler::class, function () use ($handler) { - return $handler; - }); - - $this->schema = /** @lang GraphQL */ ' - type Query { - foo: ID @mock - } - '; - - $this->graphQL(/** @lang GraphQL */ ' - { - foo - } - '); - } -} diff --git a/tests/Integration/ReportingErrorHandlerTest.php b/tests/Integration/ReportingErrorHandlerTest.php new file mode 100644 index 0000000000..5b6fde1bd2 --- /dev/null +++ b/tests/Integration/ReportingErrorHandlerTest.php @@ -0,0 +1,62 @@ +mockResolver(function () use ($exception) { + throw $exception; + }); + + $handler = $this->createMock(ExceptionHandler::class); + $handler + ->expects($this->atLeastOnce()) + ->method('report') + ->with($exception); + app()->singleton(ExceptionHandler::class, function () use ($handler) { + return $handler; + }); + + $this->graphQL(/** @lang GraphQL */ ' + { + foo + } + '); + } + + public function testDoesNotReportClientSafeErrors(): void + { + $error = new Error('an expected error that is shown to clients'); + $this->mockResolver(function () use ($error) { + throw $error; + }); + + $handler = $this->createMock(ExceptionHandler::class); + $handler + ->expects($this->never()) + ->method('report') + ->with($error); + app()->singleton(ExceptionHandler::class, function () use ($handler) { + return $handler; + }); + + $this->graphQL(/** @lang GraphQL */ ' + { + foo + } + '); + } +}