Fix SaveAwareArgResolver ignored in nested mutations

CHANGELOG.md

@@ -9,6 +9,11 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 ## Unreleased
 
+### Fixed
+
+- Honour `SaveAwareArgResolver::runBeforeSave()` in nested mutations instead of only at the top level https://github.com/nuwave/lighthouse/pull/2784
+- Run all pre-save arg resolvers lifted out of `@nest` instead of silently discarding those whose field name collides with a sibling https://github.com/nuwave/lighthouse/pull/2784
+
 ## v6.69.1
 
 ### Fixed

src/Cache/QueryCache.php

@@ -43,14 +43,14 @@ public function isEnabled(): bool
 
     public function clear(?int $opcacheTTLHours, bool $opcacheOnly): void
     {
-        if (in_array($this->mode, ['store', 'hybrid'], true)
+        if (in_array($this->mode, ['store', 'hybrid'], strict: true)
             && ! $opcacheOnly
         ) {
             $store = $this->makeCacheStore();
             $store->clear();
         }
 
-        if (in_array($this->mode, ['opcache', 'hybrid'], true)) {
+        if (in_array($this->mode, ['opcache', 'hybrid'], strict: true)) {
             $files = $this->filesystem->glob($this->opcacheFilePath('*'));
 
             if (is_int($opcacheTTLHours)) {

src/Defer/DeferrableDirective.php

@@ -77,13 +77,13 @@ protected function shouldDefer(TypeNode $fieldType, ResolveInfo $resolveInfo): b
         }
 
         $skips = (new ClientDirective(Directive::SKIP_NAME))->forField($resolveInfo);
-        if (in_array([Directive::IF_ARGUMENT_NAME => true], $skips, true)) {
+        if (in_array([Directive::IF_ARGUMENT_NAME => true], $skips, strict: true)) {
             return false;
         }
 
         $includes = (new ClientDirective(Directive::INCLUDE_NAME))->forField($resolveInfo);
 
-        return ! in_array([Directive::IF_ARGUMENT_NAME => false], $includes, true);
+        return ! in_array([Directive::IF_ARGUMENT_NAME => false], $includes, strict: true);
     }
 
     /** @param  array<array<string, mixed>|null>  $defers */

src/Execution/Arguments/ArgPartitioner.php

@@ -20,37 +20,19 @@ class ArgPartitioner
     /**
      * Partition the arguments into nested and regular.
      *
-     * @return array{
-     *   0: \Nuwave\Lighthouse\Execution\Arguments\ArgumentSet,
-     *   1: \Nuwave\Lighthouse\Execution\Arguments\ArgumentSet,
-     * }
-     */
-    public static function nestedArgResolvers(ArgumentSet $argumentSet, mixed $root): array
-    {
-        static::prepareArgResolvers($argumentSet, $root);
-
-        return static::partition(
-            $argumentSet,
-            static fn (string $name, Argument $argument): bool => isset($argument->resolver),
-        );
-    }
-
-    /**
-     * Like nestedArgResolvers(), but excludes SaveAwareArgResolvers that run before save.
-     *
-     * Used by SaveModel's ResolveNested wrapper so pre-save resolvers stay in the
-     * regular set and reach SaveModel for execution before $model->save().
+     * SaveAwareArgResolvers where runBeforeSave() returns true stay in the regular set
+     * so they reach SaveModel for execution before $model->save().
      *
      * @return array{
      *   0: \Nuwave\Lighthouse\Execution\Arguments\ArgumentSet,
      *   1: \Nuwave\Lighthouse\Execution\Arguments\ArgumentSet,
      * }
      */
-    public static function nestedArgResolversWithoutPreSave(ArgumentSet $argumentSet, mixed $root): array
+    public static function nestedArgResolvers(ArgumentSet $argumentSet, mixed $root): array
     {
         $model = static::prepareArgResolvers($argumentSet, $root);
 
-        [$nested, $regular] = static::partition(
+        return static::partition(
             $argumentSet,
             static function (string $name, Argument $argument) use ($root, $model): bool {
                 $resolver = $argument->resolver;
@@ -71,13 +53,6 @@ static function (string $name, Argument $argument) use ($root, $model): bool {
                 return true;
             },
         );
-
-        if ($model !== null) {
-            assert($root instanceof Model);
-            static::liftPreSaveResolversFromNest($nested, $regular, $root, $model);
-        }
-
-        return [$nested, $regular];
     }
 
     /**
@@ -209,43 +184,70 @@ public static function methodReturnsRelation(
     }
 
     /**
-     * Recursively traverse @nest arguments and lift pre-save resolvers to the regular set
-     * so they reach SaveModel and execute before $model->save().
+     * Recursively collect the pre-save resolvers within @nest arguments,
+     * so they can run before $root->save() instead of after it.
      *
-     * @param  \ReflectionClass<\Illuminate\Database\Eloquent\Model>  $model
+     * The result is an ordered list rather than a name-keyed set:
+     * field names are unique per input type, not across a whole input tree.
+     *
+     * Leaves the given arguments untouched, as they may be shared with cached
+     * or spread ArgumentSets that the caller still owns.
+     *
+     * @return list<\Nuwave\Lighthouse\Execution\Arguments\Argument>
      */
-    protected static function liftPreSaveResolversFromNest(ArgumentSet $nested, ArgumentSet $regular, Model $root, \ReflectionClass $model): void
+    public static function liftPreSaveResolversFromNest(ArgumentSet $nested, Model $root): array
     {
+        $model = new \ReflectionClass($root);
+
+        /** @var list<\Nuwave\Lighthouse\Execution\Arguments\Argument> */
+        $lifted = [];
+
         foreach ($nested->arguments as $argument) {
             if (! $argument->resolver instanceof NestDirective) {
                 continue;
             }
 
-            $nestValue = $argument->value;
-            if ($nestValue === null) {
-                continue;
-            }
+            array_push($lifted, ...static::liftPreSaveResolversFromNestArgument($argument, $root, $model));
+        }
+
+        return $lifted;
+    }
+
+    /**
+     * Collect the pre-save resolvers within a single @nest argument.
+     *
+     * @param  \ReflectionClass<\Illuminate\Database\Eloquent\Model>  $model
+     *
+     * @return list<\Nuwave\Lighthouse\Execution\Arguments\Argument>
+     */
+    protected static function liftPreSaveResolversFromNestArgument(Argument $nest, Model $root, \ReflectionClass $model): array
+    {
+        $nestValue = $nest->value;
+        if ($nestValue === null) {
+            return [];
+        }
 
-            assert($nestValue instanceof ArgumentSet, 'NestDirective validates that @nest is used on non-list input object types.');
+        assert($nestValue instanceof ArgumentSet, 'NestDirective validates that @nest is used on non-list input object types.');
 
-            foreach ($nestValue->arguments as $childName => $childArgument) {
-                static::attachNestedArgResolver($childName, $childArgument, $model);
+        /** @var list<\Nuwave\Lighthouse\Execution\Arguments\Argument> */
+        $lifted = [];
 
-                $resolver = $childArgument->resolver;
+        foreach ($nestValue->arguments as $childName => $childArgument) {
+            static::attachNestedArgResolver($childName, $childArgument, $model);
 
-                if (self::shouldRunBeforeSave($resolver, $root)) {
-                    $regular->arguments[$childName] = $childArgument;
-                    unset($nestValue->arguments[$childName]);
-                    continue;
-                }
+            $resolver = $childArgument->resolver;
 
-                if ($resolver instanceof NestDirective) {
-                    $childNested = new ArgumentSet();
-                    $childNested->arguments[$childName] = $childArgument;
-                    static::liftPreSaveResolversFromNest($childNested, $regular, $root, $model);
-                }
+            if (self::shouldRunBeforeSave($resolver, $root)) {
+                $lifted[] = $childArgument;
+                continue;
+            }
+
+            if ($resolver instanceof NestDirective) {
+                array_push($lifted, ...static::liftPreSaveResolversFromNestArgument($childArgument, $root, $model));
             }
         }
+
+        return $lifted;
     }
 
     /** @return \ReflectionClass<\Illuminate\Database\Eloquent\Model>|null */

src/Execution/Arguments/DelegatesPreSaveArguments.php

@@ -0,0 +1,30 @@
+<?php declare(strict_types=1);
+
+namespace Nuwave\Lighthouse\Execution\Arguments;
+
+use Nuwave\Lighthouse\Support\Contracts\PreSaveArgumentsAware;
+
+/**
+ * Pass pre-save arguments on to the wrapped resolver, which is the one that saves.
+ */
+trait DelegatesPreSaveArguments
+{
+    /** @param  list<\Nuwave\Lighthouse\Execution\Arguments\Argument>  $arguments */
+    public function withPreSaveArguments(array $arguments): ?static
+    {
+        $previous = $this->previous;
+        if (! $previous instanceof PreSaveArgumentsAware) {
+            return null;
+        }
+
+        $previousWithPreSave = $previous->withPreSaveArguments($arguments);
+        if ($previousWithPreSave === null) {
+            return null;
+        }
+
+        $clone = clone $this;
+        $clone->previous = $previousWithPreSave;
+
+        return $clone;
+    }
+}

src/Execution/Arguments/ResolveNested.php

@@ -2,8 +2,10 @@
 
 namespace Nuwave\Lighthouse\Execution\Arguments;
 
+use Illuminate\Database\Eloquent\Model;
 use Nuwave\Lighthouse\Schema\Directives\NestDirective;
 use Nuwave\Lighthouse\Support\Contracts\ArgResolver;
+use Nuwave\Lighthouse\Support\Contracts\PreSaveArgumentsAware;
 
 class ResolveNested implements ArgResolver
 {
@@ -26,13 +28,45 @@ public function __invoke(mixed $root, $args): mixed
         [$nestedArgs, $regularArgs] = ($this->argPartitioner)($args, $root);
         assert($nestedArgs instanceof ArgumentSet);
 
-        if ($this->previous !== null) {
-            $root = ($this->previous)($root, $regularArgs);
+        $previous = $this->previous;
+        $liftedPreSave = [];
+
+        if ($root instanceof Model
+            && $previous instanceof PreSaveArgumentsAware
+        ) {
+            $liftableArguments = ArgPartitioner::liftPreSaveResolversFromNest($nestedArgs, $root);
+
+            if ($liftableArguments !== []) {
+                $previousWithPreSave = $previous->withPreSaveArguments($liftableArguments);
+
+                if ($previousWithPreSave !== null) {
+                    $previous = $previousWithPreSave;
+                    $liftedPreSave = $liftableArguments;
+                }
+            }
         }
 
+        if ($previous !== null) {
+            $root = $previous($root, $regularArgs);
+        }
+
+        $this->resolveNestedArguments($root, $nestedArgs, $liftedPreSave);
+
+        return $root;
+    }
+
+    /** @param  list<\Nuwave\Lighthouse\Execution\Arguments\Argument>  $alreadyRun  arguments the saver ran before saving */
+    protected function resolveNestedArguments(mixed $root, ArgumentSet $nestedArgs, array $alreadyRun): void
+    {
         foreach ($nestedArgs->arguments as $nested) {
             $resolver = $nested->resolver;
-            assert($resolver !== null, 'we know the resolver is there because we partitioned for it');
+            if ($resolver === null) {
+                continue;
+            }
+
+            if (in_array($nested, $alreadyRun, strict: true)) {
+                continue;
+            }
 
             $value = $nested->value;
             if ($resolver instanceof NestDirective) {
@@ -42,14 +76,15 @@ public function __invoke(mixed $root, $args): mixed
 
                 assert($value instanceof ArgumentSet, 'NestDirective validates that @nest is used on non-list input object types.');
 
-                $nestResolver = new self(null, $this->argPartitioner);
-                $nestResolver($root, $value);
+                // Partitioning attaches the resolvers of the children, including implicitly detected relations.
+                // Its classification is irrelevant here: there is no saver to hand regular arguments to,
+                // and children the saver did not run must still resolve.
+                ($this->argPartitioner)($value, $root);
+                $this->resolveNestedArguments($root, $value, $alreadyRun);
                 continue;
             }
 
             $resolver($root, $value);
         }
-
-        return $root;
     }
 }

src/Execution/Arguments/SaveModel.php

@@ -9,15 +9,27 @@
 use Illuminate\Database\Eloquent\Relations\MorphTo;
 use Illuminate\Database\Eloquent\Relations\Relation;
 use Nuwave\Lighthouse\Support\Contracts\ArgResolver;
+use Nuwave\Lighthouse\Support\Contracts\PreSaveArgumentsAware;
 use Nuwave\Lighthouse\Support\Contracts\SaveAwareArgResolver;
 
-class SaveModel implements ArgResolver
+class SaveModel implements ArgResolver, PreSaveArgumentsAware
 {
+    /** @var list<\Nuwave\Lighthouse\Execution\Arguments\Argument> */
+    protected array $preSaveArguments = [];
+
     public function __construct(
         /** @var \Illuminate\Database\Eloquent\Relations\Relation<\Illuminate\Database\Eloquent\Model>|null $parentRelation */
         protected ?Relation $parentRelation = null,
     ) {}
 
+    public function withPreSaveArguments(array $arguments): static
+    {
+        $clone = clone $this;
+        $clone->preSaveArguments = $arguments;
+
+        return $clone;
+    }
+
     /**
      * @param  Model  $model
      * @param  ArgumentSet  $args
@@ -62,10 +74,13 @@ public function __invoke($model, $args): Model
             $morphToResolver($model, $nestedOperations->value);
         }
 
-        foreach ($preSave->arguments as $nested) {
-            $resolver = $nested->resolver;
+        foreach ([
+            ...array_values($preSave->arguments),
+            ...$this->preSaveArguments,
+        ] as $preSaveArgument) {
+            $resolver = $preSaveArgument->resolver;
             assert($resolver instanceof SaveAwareArgResolver, 'Resolver must be a SaveAwareArgResolver because we partitioned for it.');
-            $resolver($model, $nested->value);
+            $resolver($model, $preSaveArgument->value);
         }
 
         if ($this->parentRelation instanceof HasOneOrMany) {

src/Execution/Arguments/UpdateModel.php

@@ -5,9 +5,12 @@
 use GraphQL\Error\Error;
 use Illuminate\Support\Arr;
 use Nuwave\Lighthouse\Support\Contracts\ArgResolver;
+use Nuwave\Lighthouse\Support\Contracts\PreSaveArgumentsAware;
 
-class UpdateModel implements ArgResolver
+class UpdateModel implements ArgResolver, PreSaveArgumentsAware
 {
+    use DelegatesPreSaveArguments;
+
     public const MISSING_PRIMARY_KEY_FOR_UPDATE = 'Missing primary key for update.';
 
     /** @var callable|\Nuwave\Lighthouse\Support\Contracts\ArgResolver */

src/Execution/Arguments/UpsertModel.php

@@ -5,11 +5,14 @@
 use GraphQL\Error\Error;
 use Illuminate\Database\Eloquent\Model;
 use Nuwave\Lighthouse\Support\Contracts\ArgResolver;
+use Nuwave\Lighthouse\Support\Contracts\PreSaveArgumentsAware;
 
 use function Safe\array_flip;
 
-class UpsertModel implements ArgResolver
+class UpsertModel implements ArgResolver, PreSaveArgumentsAware
 {
+    use DelegatesPreSaveArguments;
+
     public const MISSING_IDENTIFYING_COLUMNS_FOR_UPSERT = 'All configured identifying columns must be present and non-null for upsert.';
 
     /** @var callable|\Nuwave\Lighthouse\Support\Contracts\ArgResolver */

src/Schema/Directives/ModelMutationDirective.php

@@ -67,7 +67,6 @@ protected function executeMutation(Model $model, ArgumentSet|array $args, ?Relat
     {
         $update = new ResolveNested(
             $this->makeExecutionFunction($parentRelation),
-            [ArgPartitioner::class, 'nestedArgResolversWithoutPreSave'],
         );
 
         return Utils::mapEach(