Throw partial errors when failing to delete, forceDelete or restore a…

… model (#1420)
nuwave · Jul 17, 2020 · a5b6e16 · a5b6e16
1 parent cc1fb00
commit a5b6e16
Show file tree

Hide file tree

Showing 13 changed files with 186 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,11 +21,17 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 - Publish config file with tag `lighthouse-config` and default schema with tag `lighthouse-schema`
   instead of the previously used tags `config` and `schema` https://github.com/nuwave/lighthouse/issues/1489
+- Throw partial errors when failing to delete, forceDelete or restore a model https://github.com/nuwave/lighthouse/pull/1420
+- Add `\Nuwave\Lighthouse\Execution\ErrorPool` to allow collection of partial errors https://github.com/nuwave/lighthouse/pull/1420
 
 ### Fixed
 
 - Ensure the `@count` directive works properly with polymorphic relations https://github.com/nuwave/lighthouse/pull/1466
 
+### Deprecated
+
+- Deprecate `\Nuwave\Lighthouse\Execution\ErrorBuffer` in favor of `\Nuwave\Lighthouse\Execution\ErrorPool` https://github.com/nuwave/lighthouse/pull/1420
+
 ## 4.15.0
 
 ### Added

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -195,3 +195,19 @@ If you need to revert to the old behavior of using `fill()`, you can change your
 -   'force_fill' => true,
 +   'force_fill' => false,
 ```
+
+### Replace `ErrorBuffer` with `ErrorPool`
+
+Collecting partial errors is now done through the singleton `\Nuwave\Lighthouse\Execution\ErrorPool`
+instead of `\Nuwave\Lighthouse\Execution\ErrorBuffer`:
+
+```php
+try {
+    // Something that might fail but still allows for a partial result
+} catch (\Throwable $error) {
+    $errorPool = app(\Nuwave\Lighthouse\Execution\ErrorPool::class);
+    $errorPool->record($error);
+}
+
+return $result;
+```
diff --git a/docs/master/digging-deeper/error-handling.md b/docs/master/digging-deeper/error-handling.md
@@ -166,8 +166,21 @@ class ExtensionErrorHandler implements ErrorHandler
 }
 ```
 
-## Collecting Errors
+## Partial Errors
 
 As a GraphQL query may return a partial result, you may not always want to abort
-execution immediately after an error occurred. You can use the [`\Nuwave\Lighthouse\Execution\ErrorBuffer`](https://github.com/nuwave/lighthouse/blob/master/src/Execution/ErrorBuffer.php)
+execution immediately after an error occurred.
+
+Use the [`ErrorPool`](https://github.com/nuwave/lighthouse/blob/master/src/Execution/ErrorPool.php)
 when you want to collect multiple errors before returning a result.
+
+```php
+try {
+    // Something that might fail but still allows for a partial result
+} catch (\Throwable $error) {
+    $errorPool = app(\Nuwave\Lighthouse\Execution\ErrorPool::class);
+    $errorPool->record($error);
+}
+
+return $result;
+```
diff --git a/docs/master/security/authentication.md b/docs/master/security/authentication.md
@@ -58,7 +58,7 @@ to `sanctum` and register Sanctum's `EnsureFrontendRequestsAreStateful` as first
             // ... other middleware
 
             \Laravel\Sanctum\Http\Middleware\EnsureFrontendRequestsAreStateful::class,
-        ]
+        ],
     ],
     'guard' => 'sanctum',
 ```
@@ -133,10 +133,9 @@ Add the following middleware to `config/lighthouse.php`:
             \Illuminate\Session\Middleware\StartSession::class,
 
             // Or this one when using Laravel Sanctum:
-            \Laravel\Sanctum\Http\Middleware\EnsureFrontendRequestsAreStateful::class
-
+            \Laravel\Sanctum\Http\Middleware\EnsureFrontendRequestsAreStateful::class,
             // ... other middleware
-        ]
+        ],
     ],
 ```
 

diff --git a/src/Execution/ErrorBuffer.php b/src/Execution/ErrorBuffer.php
@@ -5,6 +5,10 @@
 use Closure;
 use Nuwave\Lighthouse\Exceptions\GenericException;
 
+/**
+ * @deprecated in favor of
+ * @see \Nuwave\Lighthouse\Execution\ErrorPool
+ */
 class ErrorBuffer
 {
     /**

diff --git a/src/Execution/ErrorPool.php b/src/Execution/ErrorPool.php
@@ -0,0 +1,53 @@
+<?php
+
+namespace Nuwave\Lighthouse\Execution;
+
+use GraphQL\Error\Error;
+use Throwable;
+
+class ErrorPool
+{
+    /**
+     * The buffered errors.
+     *
+     * @var array<\Throwable>
+     */
+    protected $throwables = [];
+
+    /**
+     * Stores an error that will be added to the result.
+     */
+    public function record(Throwable $throwable): void
+    {
+        $this->throwables [] = $throwable;
+    }
+
+    /**
+     * @return array<\GraphQL\Error\Error>
+     */
+    public function errors(): array
+    {
+        return array_map(
+            function (Throwable $throwable): Error {
+                if ($throwable instanceof Error) {
+                    return $throwable;
+                }
+
+                return new Error(
+                    $throwable->getMessage(),
+                    null,
+                    null,
+                    null,
+                    null,
+                    $throwable
+                );
+            },
+            $this->throwables
+        );
+    }
+
+    public function clear(): void
+    {
+        $this->throwables = [];
+    }
+}
diff --git a/src/GraphQL.php b/src/GraphQL.php
@@ -11,6 +11,7 @@
 use Nuwave\Lighthouse\Events\ManipulateResult;
 use Nuwave\Lighthouse\Events\StartExecution;
 use Nuwave\Lighthouse\Execution\DataLoader\BatchLoader;
+use Nuwave\Lighthouse\Execution\ErrorPool;
 use Nuwave\Lighthouse\Execution\GraphQLRequest;
 use Nuwave\Lighthouse\Schema\AST\ASTBuilder;
 use Nuwave\Lighthouse\Schema\AST\DocumentAST;
@@ -53,7 +54,12 @@ class GraphQL
     protected $createsContext;
 
     /**
-     * @var ProvidesValidationRules
+     * @var \Nuwave\Lighthouse\Execution\ErrorPool
+     */
+    protected $errorPool;
+
+    /**
+     * @var \Nuwave\Lighthouse\Support\Contracts\ProvidesValidationRules
      */
     protected $providesValidationRules;
 
@@ -63,13 +69,15 @@ public function __construct(
         EventDispatcher $eventDispatcher,
         ASTBuilder $astBuilder,
         CreatesContext $createsContext,
+        ErrorPool $errorPool,
         ProvidesValidationRules $providesValidationRules
     ) {
         $this->schemaBuilder = $schemaBuilder;
         $this->pipeline = $pipeline;
         $this->eventDispatcher = $eventDispatcher;
         $this->astBuilder = $astBuilder;
         $this->createsContext = $createsContext;
+        $this->errorPool = $errorPool;
         $this->providesValidationRules = $providesValidationRules;
     }
 
@@ -159,6 +167,10 @@ public function executeQuery(
             }
         }
 
+        foreach ($this->errorPool->errors() as $error) {
+            $result->errors [] = $error;
+        }
+
         $result->setErrorsHandler(
             function (array $errors, callable $formatter): array {
                 // User defined error handlers, implementing \Nuwave\Lighthouse\Execution\ErrorHandler
@@ -209,6 +221,7 @@ public function prepSchema(): Schema
     protected function cleanUp(): void
     {
         BatchLoader::forgetInstances();
+        $this->errorPool->clear();
     }
 
     /**

diff --git a/src/LighthouseServiceProvider.php b/src/LighthouseServiceProvider.php
@@ -27,6 +27,7 @@
 use Nuwave\Lighthouse\Console\UnionCommand;
 use Nuwave\Lighthouse\Console\ValidateSchemaCommand;
 use Nuwave\Lighthouse\Execution\ContextFactory;
+use Nuwave\Lighthouse\Execution\ErrorPool;
 use Nuwave\Lighthouse\Execution\GraphQLRequest;
 use Nuwave\Lighthouse\Execution\GraphQLValidator;
 use Nuwave\Lighthouse\Execution\LighthouseRequest;
@@ -111,6 +112,7 @@ public function register(): void
         $this->app->singleton(DirectiveFactory::class);
         $this->app->singleton(NodeRegistry::class);
         $this->app->singleton(TypeRegistry::class);
+        $this->app->singleton(ErrorPool::class);
         $this->app->singleton(CreatesContext::class, ContextFactory::class);
         $this->app->singleton(CanStreamResponse::class, ResponseStream::class);
 

diff --git a/src/Schema/Directives/DeleteDirective.php b/src/Schema/Directives/DeleteDirective.php
@@ -52,9 +52,9 @@ protected function find(string $modelClass, $idOrIds)
         return $modelClass::find($idOrIds);
     }
 
-    protected function modifyExistence(Model $model): void
+    protected function modifyExistence(Model $model): bool
     {
-        $model->delete();
+        return (bool) $model->delete();
     }
 
     /**

diff --git a/src/Schema/Directives/ModifyModelExistenceDirective.php b/src/Schema/Directives/ModifyModelExistenceDirective.php
@@ -2,13 +2,15 @@
 
 namespace Nuwave\Lighthouse\Schema\Directives;
 
+use GraphQL\Error\Error;
 use GraphQL\Language\AST\FieldDefinitionNode;
 use GraphQL\Language\AST\ListTypeNode;
 use GraphQL\Language\AST\NonNullTypeNode;
 use GraphQL\Language\AST\ObjectTypeDefinitionNode;
 use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Database\Eloquent\Model;
 use Nuwave\Lighthouse\Exceptions\DefinitionException;
+use Nuwave\Lighthouse\Execution\ErrorPool;
 use Nuwave\Lighthouse\Schema\AST\DocumentAST;
 use Nuwave\Lighthouse\Schema\Values\FieldValue;
 use Nuwave\Lighthouse\Support\Contracts\DefinedDirective;
@@ -19,20 +21,26 @@
 abstract class ModifyModelExistenceDirective extends BaseDirective implements FieldResolver, FieldManipulator, DefinedDirective
 {
     /**
-     * The GlobalId resolver.
-     *
      * @var \Nuwave\Lighthouse\Support\Contracts\GlobalId
      */
     protected $globalId;
 
-    public function __construct(GlobalId $globalId)
+    /**
+     * @var \Nuwave\Lighthouse\Execution\ErrorPool
+     */
+    protected $errorPool;
+
+    public function __construct(GlobalId $globalId, ErrorPool $errorPool)
     {
         $this->globalId = $globalId;
+        $this->errorPool = $errorPool;
+    }
+
+    public static function couldNotModify(Model $user): string
+    {
+        return 'Could not modify model '.get_class($user).' with ID '.$user->getKey().'.';
     }
 
-    /**
-     * Resolve the field directive.
-     */
     public function resolveField(FieldValue $fieldValue): FieldValue
     {
         return $fieldValue->setResolver(
@@ -54,11 +62,21 @@ function ($root, array $args) {
                     return;
                 }
 
+                $modifyModelExistence = function (Model $model): void {
+                    if (! $this->modifyExistence($model)) {
+                        $this->errorPool->record(
+                            new Error(
+                                self::couldNotModify($model)
+                            )
+                        );
+                    }
+                };
+
                 if ($modelOrModels instanceof Model) {
-                    $this->modifyExistence($modelOrModels);
+                    $modifyModelExistence($modelOrModels);
                 } elseif ($modelOrModels instanceof Collection) {
                     foreach ($modelOrModels as $model) {
-                        $this->modifyExistence($model);
+                        $modifyModelExistence($model);
                     }
                 }
 
@@ -137,6 +155,8 @@ abstract protected function find(string $modelClass, $idOrIds);
 
     /**
      * Bring a model in or out of existence.
+     *
+     * The return value indicates if the operation was successful.
      */
-    abstract protected function modifyExistence(Model $model): void;
+    abstract protected function modifyExistence(Model $model): bool;
 }
diff --git a/src/SoftDeletes/ForceDeleteDirective.php b/src/SoftDeletes/ForceDeleteDirective.php
@@ -41,10 +41,10 @@ protected function find(string $modelClass, $idOrIds)
         return $modelClass::withTrashed()->find($idOrIds);
     }
 
-    protected function modifyExistence(Model $model): void
+    protected function modifyExistence(Model $model): bool
     {
         /** @var \Illuminate\Database\Eloquent\Model&\Illuminate\Database\Eloquent\SoftDeletes $model */
-        $model->forceDelete();
+        return (bool) $model->forceDelete();
     }
 
     /**

diff --git a/src/SoftDeletes/RestoreDirective.php b/src/SoftDeletes/RestoreDirective.php
@@ -43,10 +43,10 @@ protected function find(string $modelClass, $idOrIds)
         return $modelClass::withTrashed()->find($idOrIds);
     }
 
-    protected function modifyExistence(Model $model): void
+    protected function modifyExistence(Model $model): bool
     {
         /** @var \Illuminate\Database\Eloquent\Model&\Illuminate\Database\Eloquent\SoftDeletes $model */
-        $model->restore();
+        return (bool) $model->restore();
     }
 
     /**