From 593036c30e1911f1ef6ffc0edee2c8f6012daf68 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 28 Jul 2026 16:49:53 +0200 Subject: [PATCH 1/5] Fix SaveAwareArgResolver ignored in nested mutations The default arg partitioner in ResolveNested unconditionally routed all arguments with resolvers into the post-save set. This meant SaveAwareArgResolver::runBeforeSave() was only honoured at the top level (where ModelMutationDirective explicitly passed the pre-save-aware partitioner) but silently ignored one level down inside nested HasMany/HasOne/ManyToMany/BelongsTo mutations. Collapse the two partitioner methods into one that always respects pre-save semantics. When root is not a Model, behavior is unchanged (all resolvers go to nested). When root is a Model, SaveAwareArgResolvers with runBeforeSave=true stay in the regular set so SaveModel can execute them before persisting. --- src/Execution/Arguments/ArgPartitioner.php | 24 +- .../Directives/ModelMutationDirective.php | 1 - .../Schema/Directives/CreateDirectiveTest.php | 273 ++++++++++++++++++ .../Arguments/ArgPartitionerTest.php | 27 +- tests/Utils/Models/Post.php | 2 + tests/Utils/Models/Task.php | 2 + ...28_000003_create_testbench_tasks_table.php | 2 + ...28_000004_create_testbench_posts_table.php | 2 + 8 files changed, 285 insertions(+), 48 deletions(-) diff --git a/src/Execution/Arguments/ArgPartitioner.php b/src/Execution/Arguments/ArgPartitioner.php index e001f2c741..d9f9296c05 100644 --- a/src/Execution/Arguments/ArgPartitioner.php +++ b/src/Execution/Arguments/ArgPartitioner.php @@ -20,33 +20,15 @@ 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); diff --git a/src/Schema/Directives/ModelMutationDirective.php b/src/Schema/Directives/ModelMutationDirective.php index 8c0b126148..3e272a6d65 100644 --- a/src/Schema/Directives/ModelMutationDirective.php +++ b/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( diff --git a/tests/Integration/Schema/Directives/CreateDirectiveTest.php b/tests/Integration/Schema/Directives/CreateDirectiveTest.php index e484b269db..d707517e66 100644 --- a/tests/Integration/Schema/Directives/CreateDirectiveTest.php +++ b/tests/Integration/Schema/Directives/CreateDirectiveTest.php @@ -876,4 +876,277 @@ public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNest(): vo ], ]); } + + public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNestedHasMany(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + createUser(input: CreateUserInput! @spread): User @create + } + + input CreateUserInput { + name: String! + tasks: CreateTaskRelation + } + + input CreateTaskRelation { + create: [CreateTaskInput!] + } + + input CreateTaskInput { + name: String! + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createUser(input: { + name: "Geo Parent" + tasks: { + create: [{ + name: "Geo Task" + location: { + lat: 48.1351 + lng: 11.5820 + } + }] + } + }) { + id + name + tasks { + id + name + latitude + longitude + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createUser' => [ + 'name' => 'Geo Parent', + 'tasks' => [ + [ + 'name' => 'Geo Task', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ], + ], + ]); + } + + public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNestInsideNestedHasMany(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + createUser(input: CreateUserInput! @spread): User @create + } + + input CreateUserInput { + name: String! + tasks: CreateTaskRelation + } + + input CreateTaskRelation { + create: [CreateTaskInput!] + } + + input CreateTaskInput { + name: String! + nested: TaskNestedInput @nest + } + + input TaskNestedInput { + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createUser(input: { + name: "Geo Nest Parent" + tasks: { + create: [{ + name: "Geo Nest Task" + nested: { + location: { + lat: 48.1351 + lng: 11.5820 + } + } + }] + } + }) { + id + name + tasks { + id + name + latitude + longitude + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createUser' => [ + 'name' => 'Geo Nest Parent', + 'tasks' => [ + [ + 'name' => 'Geo Nest Task', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ], + ], + ]); + } + + public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideDeeplyNestedRelation(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Post { + id: ID! + title: String! + latitude: Float + longitude: Float + } + + type Task { + id: ID! + name: String! + post: Post @hasOne + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + createUser(input: CreateUserInput! @spread): User @create + } + + input CreateUserInput { + name: String! + tasks: CreateTaskRelation + } + + input CreateTaskRelation { + create: [CreateTaskInput!] + } + + input CreateTaskInput { + name: String! + post: CreatePostRelation + } + + input CreatePostRelation { + create: CreatePostInput + } + + input CreatePostInput { + title: String! + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createUser(input: { + name: "Geo Deep Parent" + tasks: { + create: [{ + name: "Geo Deep Task" + post: { + create: { + title: "Geo Deep Post" + location: { + lat: 48.1351 + lng: 11.5820 + } + } + } + }] + } + }) { + id + name + tasks { + id + name + post { + id + title + latitude + longitude + } + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createUser' => [ + 'name' => 'Geo Deep Parent', + 'tasks' => [ + [ + 'name' => 'Geo Deep Task', + 'post' => [ + 'title' => 'Geo Deep Post', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ], + ], + ], + ]); + } } diff --git a/tests/Unit/Execution/Arguments/ArgPartitionerTest.php b/tests/Unit/Execution/Arguments/ArgPartitionerTest.php index 581cf9a894..62aa900d6c 100644 --- a/tests/Unit/Execution/Arguments/ArgPartitionerTest.php +++ b/tests/Unit/Execution/Arguments/ArgPartitionerTest.php @@ -138,31 +138,6 @@ public function testSaveAwareArgResolverWithNonModelRoot(): void ); } - public function testSaveAwareArgResolverWithNonModelRootInWithoutPreSave(): void - { - $argumentSet = new ArgumentSet(); - - $regular = new Argument(); - $argumentSet->arguments['regular'] = $regular; - - $saveAware = new Argument(); - $saveAware->directives->push(new SaveAwareNested()); - $argumentSet->arguments['saveAware'] = $saveAware; - - [$nestedArgs, $regularArgs] = ArgPartitioner::nestedArgResolversWithoutPreSave($argumentSet, null); - - $this->assertSame( - ['regular' => $regular], - $regularArgs->arguments, - ); - - $this->assertSame( - ['saveAware' => $saveAware], - $nestedArgs->arguments, - 'SaveAwareArgResolver should be in nested set when root is not a Model', - ); - } - public function testSaveAwareArgResolverWithModelRoot(): void { $argumentSet = new ArgumentSet(); @@ -174,7 +149,7 @@ public function testSaveAwareArgResolverWithModelRoot(): void $saveAware->directives->push(new SaveAwareNested()); $argumentSet->arguments['saveAware'] = $saveAware; - [$nestedArgs, $regularArgs] = ArgPartitioner::nestedArgResolversWithoutPreSave($argumentSet, new User()); + [$nestedArgs, $regularArgs] = ArgPartitioner::nestedArgResolvers($argumentSet, new User()); $this->assertSame( ['regular' => $regular, 'saveAware' => $saveAware], diff --git a/tests/Utils/Models/Post.php b/tests/Utils/Models/Post.php index 9bb1df79c9..c2e98f6817 100644 --- a/tests/Utils/Models/Post.php +++ b/tests/Utils/Models/Post.php @@ -21,6 +21,8 @@ * Attributes * @property string $title * @property string|null $body + * @property float|null $latitude + * @property float|null $longitude * * Timestamps * @property \Illuminate\Support\Carbon $created_at diff --git a/tests/Utils/Models/Task.php b/tests/Utils/Models/Task.php index bc6ea94a55..6420ce8355 100644 --- a/tests/Utils/Models/Task.php +++ b/tests/Utils/Models/Task.php @@ -22,6 +22,8 @@ * @property string $name * @property int|null $difficulty * @property string|null $guard + * @property float|null $latitude + * @property float|null $longitude * @property \Illuminate\Support\Carbon $completed_at * * Timestamps diff --git a/tests/database/migrations/2018_02_28_000003_create_testbench_tasks_table.php b/tests/database/migrations/2018_02_28_000003_create_testbench_tasks_table.php index e2cb57ecb5..bdf411c756 100644 --- a/tests/database/migrations/2018_02_28_000003_create_testbench_tasks_table.php +++ b/tests/database/migrations/2018_02_28_000003_create_testbench_tasks_table.php @@ -16,6 +16,8 @@ public function up(): void ->comment('The purpose of this property is to collide with a native model method name'); $table->unsignedBigInteger('difficulty')->nullable(); $table->unsignedBigInteger('user_id')->nullable(); + $table->double('latitude')->nullable(); + $table->double('longitude')->nullable(); $table->timestamp('completed_at')->nullable(); $table->timestamps(); $table->softDeletes(); diff --git a/tests/database/migrations/2018_02_28_000004_create_testbench_posts_table.php b/tests/database/migrations/2018_02_28_000004_create_testbench_posts_table.php index 547db555c2..89236dfad2 100644 --- a/tests/database/migrations/2018_02_28_000004_create_testbench_posts_table.php +++ b/tests/database/migrations/2018_02_28_000004_create_testbench_posts_table.php @@ -15,6 +15,8 @@ public function up(): void $table->unsignedBigInteger('task_id'); $table->unsignedBigInteger('user_id')->nullable(); $table->unsignedBigInteger('parent_id')->nullable(); + $table->double('latitude')->nullable(); + $table->double('longitude')->nullable(); $table->timestamps(); $table->softDeletes(); }); From 304daf63adec34c53c76adfde787a785c8e34003 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 29 Jul 2026 09:12:06 +0200 Subject: [PATCH 2/5] Cover how @nest children with colliding names behave Lifting pre-save resolvers out of @nest keys them by bare field name, so a @nest child replaces a sibling of the same name in the regular arg set without raising an error. Making nestedArgResolvers() the default partitioner widens where this can happen, so pin down the current behavior. --- .../Schema/Directives/CreateDirectiveTest.php | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/tests/Integration/Schema/Directives/CreateDirectiveTest.php b/tests/Integration/Schema/Directives/CreateDirectiveTest.php index d707517e66..0b28a4ca23 100644 --- a/tests/Integration/Schema/Directives/CreateDirectiveTest.php +++ b/tests/Integration/Schema/Directives/CreateDirectiveTest.php @@ -1149,4 +1149,100 @@ public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideDeeplyNest ], ]); } + + /** + * Lifting pre-save resolvers out of `@nest` keys them by their bare field name, + * so a `@nest` child silently replaces a sibling of the same name rather than erroring. + * Documents the status quo, not a guarantee - such a schema is ambiguous by design. + */ + public function testNestChildOverwritesSiblingWithSameNameInsideNestedHasMany(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + createUser(input: CreateUserInput! @spread): User @create + } + + input CreateUserInput { + name: String! + tasks: CreateTaskRelation + } + + input CreateTaskRelation { + create: [CreateTaskInput!] + } + + input CreateTaskInput { + name: String! + location: LocationInput @geocode + nested: TaskNestedInput @nest + } + + input TaskNestedInput { + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createUser(input: { + name: "Geo Collision Parent" + tasks: { + create: [{ + name: "Geo Collision Task" + location: { + lat: 1.0 + lng: 2.0 + } + nested: { + location: { + lat: 48.1351 + lng: 11.5820 + } + } + }] + } + }) { + id + name + tasks { + id + name + latitude + longitude + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createUser' => [ + 'name' => 'Geo Collision Parent', + 'tasks' => [ + [ + 'name' => 'Geo Collision Task', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ], + ], + ]); + } } From e4c5326738731fa4d17fec404704512ebfb7dee7 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 29 Jul 2026 09:16:30 +0200 Subject: [PATCH 3/5] Cover pre-save arg resolvers in nested update and upsert The update and upsert branches of NestedOneToMany build ResolveNested with the same default partitioner as create, so they are fixed by the same change. Both tests fail without it, the attributes end up null. --- .../Schema/Directives/UpdateDirectiveTest.php | 90 +++++++++++++++ .../Schema/Directives/UpsertDirectiveTest.php | 104 ++++++++++++++++++ 2 files changed, 194 insertions(+) diff --git a/tests/Integration/Schema/Directives/UpdateDirectiveTest.php b/tests/Integration/Schema/Directives/UpdateDirectiveTest.php index 6ef61bf91e..bf8866443f 100644 --- a/tests/Integration/Schema/Directives/UpdateDirectiveTest.php +++ b/tests/Integration/Schema/Directives/UpdateDirectiveTest.php @@ -423,4 +423,94 @@ public function testNestedUpdateOnInputList(): void ], ]); } + + public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNestedHasMany(): void + { + $user = factory(User::class)->create(); + $this->assertInstanceOf(User::class, $user); + + $task = factory(Task::class)->make(); + $this->assertInstanceOf(Task::class, $task); + $task->user()->associate($user); + $task->save(); + + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + updateUser(input: UpdateUserInput! @spread): User @update + } + + input UpdateUserInput { + id: ID! + name: String + tasks: UpdateTaskRelation + } + + input UpdateTaskRelation { + update: [UpdateTaskInput!] + } + + input UpdateTaskInput { + id: ID! + name: String + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<id} + name: "Geo Update Parent" + tasks: { + update: [{ + id: {$task->id} + name: "Geo Update Task" + location: { + lat: 48.1351 + lng: 11.5820 + } + }] + } + }) { + name + tasks { + name + latitude + longitude + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'updateUser' => [ + 'name' => 'Geo Update Parent', + 'tasks' => [ + [ + 'name' => 'Geo Update Task', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ], + ], + ]); + } } diff --git a/tests/Integration/Schema/Directives/UpsertDirectiveTest.php b/tests/Integration/Schema/Directives/UpsertDirectiveTest.php index 49324c7c41..e7cdb9feeb 100644 --- a/tests/Integration/Schema/Directives/UpsertDirectiveTest.php +++ b/tests/Integration/Schema/Directives/UpsertDirectiveTest.php @@ -550,6 +550,110 @@ public function testUpsertByIdentifyingColumnWithInputSpread(): void $this->assertSame('baz', User::firstOrFail()->name); } + public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNestedHasMany(): void + { + $user = factory(User::class)->create(); + $this->assertInstanceOf(User::class, $user); + + $task = factory(Task::class)->make(); + $this->assertInstanceOf(Task::class, $task); + $task->user()->associate($user); + $task->save(); + + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type User { + id: ID! + name: String! + tasks: [Task!]! @hasMany + } + + type Mutation { + updateUser(input: UpdateUserInput! @spread): User @update + } + + input UpdateUserInput { + id: ID! + name: String + tasks: UpsertTaskRelation + } + + input UpsertTaskRelation { + upsert: [UpsertTaskInput!] + } + + input UpsertTaskInput { + id: ID + name: String + location: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<id} + name: "Geo Upsert Parent" + tasks: { + upsert: [ + { + id: {$task->id} + name: "Geo Upserted Task" + location: { + lat: 48.1351 + lng: 11.5820 + } + } + { + name: "Geo Inserted Task" + location: { + lat: 52.5200 + lng: 13.4050 + } + } + ] + } + }) { + name + tasks { + name + latitude + longitude + } + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'updateUser' => [ + 'name' => 'Geo Upsert Parent', + 'tasks' => [ + [ + 'name' => 'Geo Upserted Task', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + [ + 'name' => 'Geo Inserted Task', + 'latitude' => 52.52, + 'longitude' => 13.405, + ], + ], + ], + ], + ]); + } + public static function resolveType(): Type { $typeRegistry = Container::getInstance()->make(TypeRegistry::class); From 18882fb6657cdeb2ed91bef8b516e5a8cf2ae0dd Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 29 Jul 2026 09:19:26 +0200 Subject: [PATCH 4/5] Add changelog entry for nested pre-save arg resolver fix --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 584286f004..da0a90b0ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ 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 + ## v6.69.1 ### Fixed From 7e1b10102c6bee8f618a878603e96ce655be237a Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 29 Jul 2026 10:54:41 +0200 Subject: [PATCH 5/5] Run all pre-save arg resolvers lifted out of @nest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lifting keyed resolvers by bare field name, so a child of a @nest whose name collided with a sibling silently replaced it. Field names are unique per input type, not across an input tree. Lifted resolvers now travel as an ordered list handed to the saver through the PreSaveArgumentsAware capability interface. Savers that can not consume them get nothing lifted, so their children run post-save as before. 🤖 Generated with Claude Code --- CHANGELOG.md | 1 + src/Execution/Arguments/ArgPartitioner.php | 36 +++++---- src/Execution/Arguments/ResolveNested.php | 47 ++++++++++-- src/Execution/Arguments/SaveModel.php | 20 ++++- src/Execution/Arguments/UpdateModel.php | 21 ++++- src/Execution/Arguments/UpsertModel.php | 21 ++++- .../Contracts/PreSaveArgumentsAware.php | 23 ++++++ .../Schema/Directives/CreateDirectiveTest.php | 69 ++++++++++++++++- .../Schema/Directives/NestDirectiveTest.php | 76 +++++++++++++++++++ .../Schema/Directives/UpdateDirectiveTest.php | 74 ++++++++++++++++++ .../Execution/Arguments/ResolveNestedTest.php | 58 ++++++++++++++ .../Directives/EmailAddressDirective.php | 37 +++++++++ 12 files changed, 451 insertions(+), 32 deletions(-) create mode 100644 src/Support/Contracts/PreSaveArgumentsAware.php create mode 100644 tests/Unit/Execution/Arguments/ResolveNestedTest.php create mode 100644 tests/Utils/Directives/EmailAddressDirective.php diff --git a/CHANGELOG.md b/CHANGELOG.md index da0a90b0ae..d217861a9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ You can find and compare releases at the [GitHub release page](https://github.co ### 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 diff --git a/src/Execution/Arguments/ArgPartitioner.php b/src/Execution/Arguments/ArgPartitioner.php index d9f9296c05..cff8729059 100644 --- a/src/Execution/Arguments/ArgPartitioner.php +++ b/src/Execution/Arguments/ArgPartitioner.php @@ -32,7 +32,7 @@ public static function nestedArgResolvers(ArgumentSet $argumentSet, mixed $root) { $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; @@ -53,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]; } /** @@ -191,13 +184,22 @@ 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. + * + * 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. * - * @param \ReflectionClass<\Illuminate\Database\Eloquent\Model> $model + * 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); + $lifted = []; + foreach ($nested->arguments as $argument) { if (! $argument->resolver instanceof NestDirective) { continue; @@ -216,18 +218,22 @@ protected static function liftPreSaveResolversFromNest(ArgumentSet $nested, Argu $resolver = $childArgument->resolver; if (self::shouldRunBeforeSave($resolver, $root)) { - $regular->arguments[$childName] = $childArgument; - unset($nestValue->arguments[$childName]); + $lifted[] = $childArgument; continue; } if ($resolver instanceof NestDirective) { $childNested = new ArgumentSet(); $childNested->arguments[$childName] = $childArgument; - static::liftPreSaveResolversFromNest($childNested, $regular, $root, $model); + + foreach (static::liftPreSaveResolversFromNest($childNested, $root) as $deeplyNested) { + $lifted[] = $deeplyNested; + } } } } + + return $lifted; } /** @return \ReflectionClass<\Illuminate\Database\Eloquent\Model>|null */ diff --git a/src/Execution/Arguments/ResolveNested.php b/src/Execution/Arguments/ResolveNested.php index 83a738181c..0079fb152d 100644 --- a/src/Execution/Arguments/ResolveNested.php +++ b/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,43 @@ 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) { + $liftable = ArgPartitioner::liftPreSaveResolversFromNest($nestedArgs, $root); + + if ($liftable !== []) { + $withPreSave = $previous->withPreSaveArguments($liftable); + + if ($withPreSave !== null) { + $previous = $withPreSave; + $liftedPreSave = $liftable; + } + } } + 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, true)) { + continue; + } $value = $nested->value; if ($resolver instanceof NestDirective) { @@ -42,14 +74,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; } } diff --git a/src/Execution/Arguments/SaveModel.php b/src/Execution/Arguments/SaveModel.php index 4222aebaf0..acb9ad51ef 100644 --- a/src/Execution/Arguments/SaveModel.php +++ b/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,10 @@ 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) { diff --git a/src/Execution/Arguments/UpdateModel.php b/src/Execution/Arguments/UpdateModel.php index c5e998564f..81b971fae4 100644 --- a/src/Execution/Arguments/UpdateModel.php +++ b/src/Execution/Arguments/UpdateModel.php @@ -5,8 +5,9 @@ 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 { public const MISSING_PRIMARY_KEY_FOR_UPDATE = 'Missing primary key for update.'; @@ -19,6 +20,24 @@ public function __construct(callable $previous) $this->previous = $previous; } + 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; + } + /** * @param \Illuminate\Database\Eloquent\Model $model * @param ArgumentSet $args diff --git a/src/Execution/Arguments/UpsertModel.php b/src/Execution/Arguments/UpsertModel.php index 4e7f3589c5..4dab2491f4 100644 --- a/src/Execution/Arguments/UpsertModel.php +++ b/src/Execution/Arguments/UpsertModel.php @@ -5,10 +5,11 @@ 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 { public const MISSING_IDENTIFYING_COLUMNS_FOR_UPSERT = 'All configured identifying columns must be present and non-null for upsert.'; @@ -24,6 +25,24 @@ public function __construct( $this->previous = $previous; } + 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; + } + /** * @param \Illuminate\Database\Eloquent\Model $model * @param ArgumentSet $args diff --git a/src/Support/Contracts/PreSaveArgumentsAware.php b/src/Support/Contracts/PreSaveArgumentsAware.php new file mode 100644 index 0000000000..17033531e0 --- /dev/null +++ b/src/Support/Contracts/PreSaveArgumentsAware.php @@ -0,0 +1,23 @@ + $arguments + * + * @return static|null null when this resolver can not run pre-save arguments + */ + public function withPreSaveArguments(array $arguments): ?static; +} diff --git a/tests/Integration/Schema/Directives/CreateDirectiveTest.php b/tests/Integration/Schema/Directives/CreateDirectiveTest.php index 0b28a4ca23..b1de206992 100644 --- a/tests/Integration/Schema/Directives/CreateDirectiveTest.php +++ b/tests/Integration/Schema/Directives/CreateDirectiveTest.php @@ -1151,11 +1151,10 @@ public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideDeeplyNest } /** - * Lifting pre-save resolvers out of `@nest` keys them by their bare field name, - * so a `@nest` child silently replaces a sibling of the same name rather than erroring. - * Documents the status quo, not a guarantee - such a schema is ambiguous by design. + * The plain sibling `location` and the same-named `@nest` child both run, in that order. + * Both write the same columns, so the last write wins. */ - public function testNestChildOverwritesSiblingWithSameNameInsideNestedHasMany(): void + public function testNestChildAndSiblingWithSameNameBothRunInsideNestedHasMany(): void { $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' type Task { @@ -1245,4 +1244,66 @@ public function testNestChildOverwritesSiblingWithSameNameInsideNestedHasMany(): ], ]); } + + /** A `@nest` child named like a plain sibling writes different columns, so both values must persist. */ + public function testNestChildNamedLikePlainSiblingKeepsBoth(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + difficulty: Int + latitude: Float + longitude: Float + } + + type Mutation { + createTask(input: CreateTaskInput! @spread): Task @create + } + + input CreateTaskInput { + name: String! + difficulty: Int + nested: TaskNestedInput @nest + } + + input TaskNestedInput { + difficulty: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createTask(input: { + name: "Colliding Names" + difficulty: 3 + nested: { + difficulty: { + lat: 48.1351 + lng: 11.5820 + } + } + }) { + name + difficulty + latitude + longitude + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createTask' => [ + 'name' => 'Colliding Names', + 'difficulty' => 3, + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ]); + } } diff --git a/tests/Integration/Schema/Directives/NestDirectiveTest.php b/tests/Integration/Schema/Directives/NestDirectiveTest.php index 18020540c7..61e140dc26 100644 --- a/tests/Integration/Schema/Directives/NestDirectiveTest.php +++ b/tests/Integration/Schema/Directives/NestDirectiveTest.php @@ -351,6 +351,82 @@ public function testSiblingNestBlocksWithSameChildNameLastWins(): void ]); } + /** Same-named children of sibling `@nest` blocks that write different columns must both run. */ + public function testSiblingNestBlocksWithSameChildNameBothRun(): void + { + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Mutation { + createUser(input: CreateUserInput! @spread): User @create + } + + input CreateUserInput { + name: String! + alpha: AlphaInput @nest + beta: BetaInput @nest + } + + input AlphaInput { + detail: LocationInput @geocode + } + + input BetaInput { + detail: EmailAddressInput @emailAddress + } + + input LocationInput { + lat: Float! + lng: Float! + } + + input EmailAddressInput { + local: String! + domain: String! + } + + type User { + id: ID! + name: String! + email: String + latitude: Float + longitude: Float + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation { + createUser(input: { + name: "Sibling Nest" + alpha: { + detail: { + lat: 48.0 + lng: 11.0 + } + } + beta: { + detail: { + local: "sibling" + domain: "example.com" + } + } + }) { + name + email + latitude + longitude + } + } + GRAPHQL)->assertJson([ + 'data' => [ + 'createUser' => [ + 'name' => 'Sibling Nest', + 'email' => 'sibling@example.com', + 'latitude' => 48.0, + 'longitude' => 11.0, + ], + ], + ]); + } + public function testNullableNestWithNullValue(): void { $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' diff --git a/tests/Integration/Schema/Directives/UpdateDirectiveTest.php b/tests/Integration/Schema/Directives/UpdateDirectiveTest.php index bf8866443f..fb0839dc88 100644 --- a/tests/Integration/Schema/Directives/UpdateDirectiveTest.php +++ b/tests/Integration/Schema/Directives/UpdateDirectiveTest.php @@ -513,4 +513,78 @@ public function testCustomDirectiveSetsModelAttributesBeforeSaveInsideNestedHasM ], ]); } + + /** A `@nest` child named `id` must not clobber the primary key that identifies the row to update. */ + public function testNestChildNamedIdDoesNotClobberPrimaryKey(): void + { + $decoy = new Task(); + $decoy->name = 'Decoy'; + $decoy->save(); + + $task = new Task(); + $task->name = 'Target'; + $task->save(); + + $this->schema .= /** @lang GraphQL */ <<<'GRAPHQL' + type Task { + id: ID! + name: String! + latitude: Float + longitude: Float + } + + type Mutation { + updateTask(input: UpdateTaskInput! @spread): Task @update + } + + input UpdateTaskInput { + id: ID! + name: String + nested: TaskNestedInput @nest + } + + input TaskNestedInput { + id: LocationInput @geocode + } + + input LocationInput { + lat: Float! + lng: Float! + } + GRAPHQL; + + $this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL' + mutation ($id: ID!) { + updateTask(input: { + id: $id + name: "Renamed target" + nested: { + id: { + lat: 48.1351 + lng: 11.5820 + } + } + }) { + id + name + latitude + longitude + } + } + GRAPHQL, [ + 'id' => $task->id, + ])->assertJson([ + 'data' => [ + 'updateTask' => [ + 'id' => (string) $task->id, + 'name' => 'Renamed target', + 'latitude' => 48.1351, + 'longitude' => 11.582, + ], + ], + ]); + + $decoy->refresh(); + $this->assertSame('Decoy', $decoy->name); + } } diff --git a/tests/Unit/Execution/Arguments/ResolveNestedTest.php b/tests/Unit/Execution/Arguments/ResolveNestedTest.php new file mode 100644 index 0000000000..8f04e4e473 --- /dev/null +++ b/tests/Unit/Execution/Arguments/ResolveNestedTest.php @@ -0,0 +1,58 @@ +value = new ArgumentSet(); + $preSaveChild->directives->push($saveAwareResolver); + + $nestValue = new ArgumentSet(); + $nestValue->arguments['location'] = $preSaveChild; + + $nest = new Argument(); + $nest->value = $nestValue; + $nest->directives->push(new NestDirective()); + + $name = new Argument(); + $name->value = 'Sepp'; + + $argumentSet = new ArgumentSet(); + $argumentSet->arguments['name'] = $name; + $argumentSet->arguments['nested'] = $nest; + + $model = new User(); + $passedToSaver = null; + $resolveNested = new ResolveNested(static function (mixed $root, ArgumentSet $args) use (&$passedToSaver): mixed { + $passedToSaver = $args; + + return $root; + }); + + $resolveNested($model, $argumentSet); + + $this->assertTrue($saveAwareResolver->wasCalled, 'Pre-save resolvers inside @nest must run even when the saver cannot run them before save'); + $this->assertSame($model, $saveAwareResolver->receivedRoot); + + $this->assertInstanceOf(ArgumentSet::class, $passedToSaver); + $this->assertSame( + ['name' => $name], + $passedToSaver->arguments, + 'Savers that cannot run pre-save arguments must not receive them', + ); + } +} diff --git a/tests/Utils/Directives/EmailAddressDirective.php b/tests/Utils/Directives/EmailAddressDirective.php new file mode 100644 index 0000000000..74a5b08ff5 --- /dev/null +++ b/tests/Utils/Directives/EmailAddressDirective.php @@ -0,0 +1,37 @@ +toArray(); + $model->setAttribute('email', "{$parts['local']}@{$parts['domain']}"); + } +}