From 5e8f5eb99152a8914b725ffe3f4beea72ce6e5b6 Mon Sep 17 00:00:00 2001 From: jotwea Date: Mon, 27 Nov 2023 10:18:08 +0100 Subject: [PATCH] fix(graphql): consider writable flag also for nested input types (#5954) Co-authored-by: josef.wagner --- features/graphql/mutation.feature | 73 +++++++++++++++++++ features/main/default_order.feature | 10 +++ src/GraphQl/Tests/Type/FieldsBuilderTest.php | 4 +- src/GraphQl/Type/FieldsBuilder.php | 2 +- tests/Behat/DoctrineContext.php | 22 +++++- .../Fixtures/TestBundle/Document/FooDummy.php | 31 +++++++- .../TestBundle/Document/FooEmbeddable.php | 53 ++++++++++++++ .../TestBundle/Entity/EmbeddableDummy.php | 2 + tests/Fixtures/TestBundle/Entity/FooDummy.php | 31 +++++++- .../TestBundle/Entity/FooEmbeddable.php | 57 +++++++++++++++ 10 files changed, 280 insertions(+), 5 deletions(-) create mode 100644 tests/Fixtures/TestBundle/Document/FooEmbeddable.php create mode 100644 tests/Fixtures/TestBundle/Entity/FooEmbeddable.php diff --git a/features/graphql/mutation.feature b/features/graphql/mutation.feature index 8a5a09be3a9..17451af053c 100644 --- a/features/graphql/mutation.feature +++ b/features/graphql/mutation.feature @@ -637,6 +637,79 @@ Feature: GraphQL mutation support And the JSON node "data.updateDummy.dummy.relatedDummies.edges[0].node.name" should be equal to "RelatedDummy11" And the JSON node "data.updateDummy.clientMutationId" should be equal to "myId" + @createSchema + @!mongodb + Scenario: Modify an item with embedded object through a mutation + Given there is a fooDummy objects with fake names and embeddable + When I send the following GraphQL request: + """ + mutation { + updateFooDummy(input: {id: "/foo_dummies/1", name: "modifiedName", embeddedFoo: {dummyName: "Embedded name"}, clientMutationId: "myId"}) { + fooDummy { + id + name + embeddedFoo { + dummyName + } + } + clientMutationId + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.updateFooDummy.fooDummy.name" should be equal to "modifiedName" + And the JSON node "data.updateFooDummy.fooDummy.embeddedFoo.dummyName" should be equal to "Embedded name" + And the JSON node "data.updateFooDummy.clientMutationId" should be equal to "myId" + + @createSchema + Scenario: Try to modify a non writable property through a mutation + Given there is a fooDummy objects with fake names and embeddable + When I send the following GraphQL request: + """ + mutation { + updateFooDummy(input: {id: "/foo_dummies/1", name: "modifiedName", nonWritableProp: "written", embeddedFoo: {dummyName: "Embedded name"}, clientMutationId: "myId"}) { + fooDummy { + id + name + embeddedFoo { + dummyName + } + } + clientMutationId + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "errors[0].message" should match '/^Field "nonWritableProp" is not defined by type "?updateFooDummyInput"?\.$/' + + @createSchema + @!mongodb + Scenario: Try to modify a non writable embedded property through a mutation + Given there is a fooDummy objects with fake names and embeddable + When I send the following GraphQL request: + """ + mutation { + updateFooDummy(input: {id: "/foo_dummies/1", name: "modifiedName", embeddedFoo: {dummyName: "Embedded name", nonWritableProp: "written"}, clientMutationId: "myId"}) { + fooDummy { + id + name + embeddedFoo { + dummyName + } + } + clientMutationId + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "errors[0].message" should match '/^Field "nonWritableProp" is not defined by type "?FooEmbeddableNestedInput"?\.$/' + @!mongodb Scenario: Modify an item with composite identifiers through a mutation Given there are Composite identifier objects diff --git a/features/main/default_order.feature b/features/main/default_order.feature index 7458c2456e9..ad05e08a835 100644 --- a/features/main/default_order.feature +++ b/features/main/default_order.feature @@ -79,6 +79,8 @@ Feature: Default order "@type": "FooDummy", "id": 5, "name": "Balbo", + "nonWritableProp": "readonly", + "embeddedFoo": null, "dummy": "/dummies/5", "soManies": [ "/so_manies/13", @@ -92,6 +94,8 @@ Feature: Default order "@type": "FooDummy", "id": 3, "name": "Sthenelus", + "nonWritableProp": "readonly", + "embeddedFoo": null, "dummy": "/dummies/3", "soManies": [ "/so_manies/7", @@ -104,6 +108,8 @@ Feature: Default order "@type": "FooDummy", "id": 2, "name": "Ephesian", + "nonWritableProp": "readonly", + "embeddedFoo": null, "dummy": "/dummies/2", "soManies": [ "/so_manies/4", @@ -116,6 +122,8 @@ Feature: Default order "@type": "FooDummy", "id": 1, "name": "Hawsepipe", + "nonWritableProp": "readonly", + "embeddedFoo": null, "dummy": "/dummies/1", "soManies": [ "/so_manies/1", @@ -128,6 +136,8 @@ Feature: Default order "@type": "FooDummy", "id": 4, "name": "Separativeness", + "nonWritableProp": "readonly", + "embeddedFoo": null, "dummy": "/dummies/4", "soManies": [ "/so_manies/10", diff --git a/src/GraphQl/Tests/Type/FieldsBuilderTest.php b/src/GraphQl/Tests/Type/FieldsBuilderTest.php index 7cdade600a4..af610ea05c4 100644 --- a/src/GraphQl/Tests/Type/FieldsBuilderTest.php +++ b/src/GraphQl/Tests/Type/FieldsBuilderTest.php @@ -582,7 +582,8 @@ public static function resourceObjectTypeFieldsProvider(): iterable yield 'query input' => ['resourceClass', (new Query())->withClass('resourceClass'), [ 'property' => new ApiProperty(), - 'propertyBool' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_BOOL)])->withReadable(false)->withWritable(false), + 'propertyBool' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_BOOL)])->withReadable(false)->withWritable(true), + 'nonWritableProperty' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_STRING)])->withReadable(false)->withWritable(false), ], true, 0, null, [ @@ -671,6 +672,7 @@ public static function resourceObjectTypeFieldsProvider(): iterable 'property' => new ApiProperty(), 'propertyBool' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_BOOL)])->withDescription('propertyBool description')->withReadable(false)->withWritable(true)->withDeprecationReason('not useful'), 'propertySubresource' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_BOOL)])->withReadable(false)->withWritable(true), + 'nonWritableProperty' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_STRING)])->withReadable(false)->withWritable(false), 'id' => (new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_INT)])->withReadable(false)->withWritable(true), ], true, 0, null, diff --git a/src/GraphQl/Type/FieldsBuilder.php b/src/GraphQl/Type/FieldsBuilder.php index 727f35ad227..dd0ece246d9 100644 --- a/src/GraphQl/Type/FieldsBuilder.php +++ b/src/GraphQl/Type/FieldsBuilder.php @@ -220,7 +220,7 @@ public function getResourceObjectTypeFields(?string $resourceClass, Operation $o if ( !$propertyTypes || (!$input && false === $propertyMetadata->isReadable()) - || ($input && $operation instanceof Mutation && false === $propertyMetadata->isWritable()) + || ($input && false === $propertyMetadata->isWritable()) ) { continue; } diff --git a/tests/Behat/DoctrineContext.php b/tests/Behat/DoctrineContext.php index 89d4d9bfdd9..503eaf747e6 100644 --- a/tests/Behat/DoctrineContext.php +++ b/tests/Behat/DoctrineContext.php @@ -58,6 +58,7 @@ use ApiPlatform\Tests\Fixtures\TestBundle\Document\FileConfigDummy as FileConfigDummyDocument; use ApiPlatform\Tests\Fixtures\TestBundle\Document\Foo as FooDocument; use ApiPlatform\Tests\Fixtures\TestBundle\Document\FooDummy as FooDummyDocument; +use ApiPlatform\Tests\Fixtures\TestBundle\Document\FooEmbeddable as FooEmbeddableDocument; use ApiPlatform\Tests\Fixtures\TestBundle\Document\FourthLevel as FourthLevelDocument; use ApiPlatform\Tests\Fixtures\TestBundle\Document\Greeting as GreetingDocument; use ApiPlatform\Tests\Fixtures\TestBundle\Document\InitializeInput as InitializeInputDocument; @@ -144,6 +145,7 @@ use ApiPlatform\Tests\Fixtures\TestBundle\Entity\FileConfigDummy; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Foo; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\FooDummy; +use ApiPlatform\Tests\Fixtures\TestBundle\Entity\FooEmbeddable; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\FourthLevel; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Greeting; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\InitializeInput; @@ -354,7 +356,7 @@ public function thereAreFooObjectsWithFakeNames(int $nb): void /** * @Given there are :nb fooDummy objects with fake names */ - public function thereAreFooDummyObjectsWithFakeNames($nb): void + public function thereAreFooDummyObjectsWithFakeNames(int $nb, $embedd = false): void { $names = ['Hawsepipe', 'Ephesian', 'Sthenelus', 'Separativeness', 'Balbo']; $dummies = ['Lorem', 'Ipsum', 'Dolor', 'Sit', 'Amet']; @@ -365,6 +367,11 @@ public function thereAreFooDummyObjectsWithFakeNames($nb): void $foo = $this->buildFooDummy(); $foo->setName($names[$i]); + if ($embedd) { + $embeddedFoo = $this->buildFooEmbeddable(); + $embeddedFoo->setDummyName('embedded'.$names[$i]); + $foo->setEmbeddedFoo($embeddedFoo); + } $foo->setDummy($dummy); for ($j = 0; $j < 3; ++$j) { $soMany = $this->buildSoMany(); @@ -379,6 +386,14 @@ public function thereAreFooDummyObjectsWithFakeNames($nb): void $this->manager->flush(); } + /** + * @Given there is a fooDummy objects with fake names and embeddable + */ + public function thereAreFooDummyObjectsWithFakeNamesAndEmbeddable(): void + { + $this->thereAreFooDummyObjectsWithFakeNames(1, true); + } + /** * @Given there are :nb dummy group objects */ @@ -2399,6 +2414,11 @@ private function buildFooDummy(): FooDummy|FooDummyDocument return $this->isOrm() ? new FooDummy() : new FooDummyDocument(); } + private function buildFooEmbeddable(): FooEmbeddable|FooEmbeddableDocument + { + return $this->isOrm() ? new FooEmbeddable() : new FooEmbeddableDocument(); + } + private function buildFourthLevel(): FourthLevel|FourthLevelDocument { return $this->isOrm() ? new FourthLevel() : new FourthLevelDocument(); diff --git a/tests/Fixtures/TestBundle/Document/FooDummy.php b/tests/Fixtures/TestBundle/Document/FooDummy.php index 8736509e855..c29cab1a41c 100644 --- a/tests/Fixtures/TestBundle/Document/FooDummy.php +++ b/tests/Fixtures/TestBundle/Document/FooDummy.php @@ -13,7 +13,9 @@ namespace ApiPlatform\Tests\Fixtures\TestBundle\Document; +use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\ApiResource; +use ApiPlatform\Metadata\GraphQl\Mutation; use ApiPlatform\Metadata\GraphQl\QueryCollection; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; @@ -24,7 +26,7 @@ * * @author Vincent Chalamon */ -#[ApiResource(graphQlOperations: [new QueryCollection(name: 'collection_query', paginationType: 'page')], order: ['dummy.name'])] +#[ApiResource(graphQlOperations: [new QueryCollection(name: 'collection_query', paginationType: 'page'), new Mutation(name: 'update')], order: ['dummy.name'])] #[ODM\Document] class FooDummy { @@ -33,17 +35,26 @@ class FooDummy */ #[ODM\Id(strategy: 'INCREMENT', type: 'int')] private ?int $id = null; + /** * @var string The foo name */ #[ODM\Field] private $name; + + #[ODM\Field(nullable: true)] + private $nonWritableProp; + /** * @var Dummy The foo dummy */ #[ODM\ReferenceOne(targetDocument: Dummy::class, cascade: ['persist'], storeAs: 'id')] private ?Dummy $dummy = null; + #[ApiProperty(readableLink: true, writableLink: true)] + #[ODM\EmbedOne(targetDocument: FooEmbeddable::class)] + private ?FooEmbeddable $embeddedFoo = null; + /** * @var Collection */ @@ -52,6 +63,7 @@ class FooDummy public function __construct() { + $this->nonWritableProp = 'readonly'; $this->soManies = new ArrayCollection(); } @@ -70,6 +82,11 @@ public function getName() return $this->name; } + public function getNonWritableProp() + { + return $this->nonWritableProp; + } + public function getDummy(): ?Dummy { return $this->dummy; @@ -79,4 +96,16 @@ public function setDummy(Dummy $dummy): void { $this->dummy = $dummy; } + + public function getEmbeddedFoo(): ?FooEmbeddable + { + return $this->embeddedFoo && !$this->embeddedFoo->getDummyName() && !$this->embeddedFoo->getNonWritableProp() ? null : $this->embeddedFoo; + } + + public function setEmbeddedFoo(?FooEmbeddable $embeddedFoo): self + { + $this->embeddedFoo = $embeddedFoo; + + return $this; + } } diff --git a/tests/Fixtures/TestBundle/Document/FooEmbeddable.php b/tests/Fixtures/TestBundle/Document/FooEmbeddable.php new file mode 100644 index 00000000000..bf12471cfa9 --- /dev/null +++ b/tests/Fixtures/TestBundle/Document/FooEmbeddable.php @@ -0,0 +1,53 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\Document; + +use ApiPlatform\Metadata\ApiProperty; +use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM; + +/** + * Embeddable Foo. + */ +#[ODM\EmbeddedDocument] +class FooEmbeddable +{ + /** + * @var string|null The dummy name + */ + #[ApiProperty(identifier: true)] + #[ODM\Field(type: 'string')] + private ?string $dummyName = null; + + #[ODM\Field(nullable: true)] + private $nonWritableProp; + + public function __construct() + { + } + + public function getDummyName(): ?string + { + return $this->dummyName; + } + + public function setDummyName(string $dummyName): void + { + $this->dummyName = $dummyName; + } + + public function getNonWritableProp() + { + return $this->nonWritableProp; + } +} diff --git a/tests/Fixtures/TestBundle/Entity/EmbeddableDummy.php b/tests/Fixtures/TestBundle/Entity/EmbeddableDummy.php index 8e6a22f590d..e3bc042b3c9 100644 --- a/tests/Fixtures/TestBundle/Entity/EmbeddableDummy.php +++ b/tests/Fixtures/TestBundle/Entity/EmbeddableDummy.php @@ -13,6 +13,7 @@ namespace ApiPlatform\Tests\Fixtures\TestBundle\Entity; +use ApiPlatform\Metadata\ApiProperty; use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Serializer\Annotation\Groups; use Symfony\Component\Validator\Constraints as Assert; @@ -28,6 +29,7 @@ class EmbeddableDummy /** * @var string The dummy name */ + #[ApiProperty(identifier: true)] #[ORM\Column(nullable: true)] #[Groups(['embed'])] private ?string $dummyName = null; diff --git a/tests/Fixtures/TestBundle/Entity/FooDummy.php b/tests/Fixtures/TestBundle/Entity/FooDummy.php index 92596dad682..ae6e98e8a1c 100644 --- a/tests/Fixtures/TestBundle/Entity/FooDummy.php +++ b/tests/Fixtures/TestBundle/Entity/FooDummy.php @@ -13,7 +13,9 @@ namespace ApiPlatform\Tests\Fixtures\TestBundle\Entity; +use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\ApiResource; +use ApiPlatform\Metadata\GraphQl\Mutation; use ApiPlatform\Metadata\GraphQl\QueryCollection; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; @@ -24,7 +26,7 @@ * * @author Vincent Chalamon */ -#[ApiResource(graphQlOperations: [new QueryCollection(name: 'collection_query', paginationType: 'page')], order: ['dummy.name'])] +#[ApiResource(graphQlOperations: [new QueryCollection(name: 'collection_query', paginationType: 'page'), new Mutation(name: 'update')], order: ['dummy.name'])] #[ORM\Entity] class FooDummy { @@ -35,11 +37,20 @@ class FooDummy #[ORM\Id] #[ORM\GeneratedValue(strategy: 'AUTO')] private ?int $id = null; + /** * @var string The foo name */ #[ORM\Column] private $name; + + #[ORM\Column(nullable: true)] + private $nonWritableProp; + + #[ApiProperty(readableLink: true, writableLink: true)] + #[ORM\Embedded(class: FooEmbeddable::class)] + private ?FooEmbeddable $embeddedFoo = null; + /** * @var Dummy|null The foo dummy */ @@ -54,6 +65,7 @@ class FooDummy public function __construct() { + $this->nonWritableProp = 'readonly'; $this->soManies = new ArrayCollection(); } @@ -72,11 +84,28 @@ public function getName() return $this->name; } + public function getNonWritableProp() + { + return $this->nonWritableProp; + } + public function getDummy(): ?Dummy { return $this->dummy; } + public function getEmbeddedFoo(): ?FooEmbeddable + { + return $this->embeddedFoo && !$this->embeddedFoo->getDummyName() && !$this->embeddedFoo->getNonWritableProp() ? null : $this->embeddedFoo; + } + + public function setEmbeddedFoo(?FooEmbeddable $embeddedFoo): self + { + $this->embeddedFoo = $embeddedFoo; + + return $this; + } + public function setDummy(Dummy $dummy): void { $this->dummy = $dummy; diff --git a/tests/Fixtures/TestBundle/Entity/FooEmbeddable.php b/tests/Fixtures/TestBundle/Entity/FooEmbeddable.php new file mode 100644 index 00000000000..46bb7907df4 --- /dev/null +++ b/tests/Fixtures/TestBundle/Entity/FooEmbeddable.php @@ -0,0 +1,57 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\Entity; + +use ApiPlatform\Metadata\ApiProperty; +use ApiPlatform\Metadata\ApiResource; +use Doctrine\ORM\Mapping as ORM; + +/** + * Embeddable Foo. + * + * @author Jordan Samouh + */ +#[ApiResource(operations: [], graphQlOperations: [])] +#[ORM\Embeddable] +class FooEmbeddable +{ + /** + * @var string The dummy name + */ + #[ApiProperty(identifier: true)] + #[ORM\Column(nullable: true)] + private ?string $dummyName = null; + + #[ORM\Column(nullable: true)] + private $nonWritableProp; + + public function __construct() + { + } + + public function getDummyName(): ?string + { + return $this->dummyName; + } + + public function setDummyName(string $dummyName): void + { + $this->dummyName = $dummyName; + } + + public function getNonWritableProp() + { + return $this->nonWritableProp; + } +}