fix(graphql): use right nested operation #5102

alanpoulain · 2022-10-27T16:48:57Z

Supersedes #4930.

The principal issue was mainly because the root operation was used instead of the nested resource one.

This PR also improves how GraphQL handles a nested resource when the query operation does not exist.
Instead of having a fallback on the REST operation (and an exception if it does not exist), it creates a "dynamic" resource having the default operations.
This dynamic resource is created using a new DynamicExtractor with an addResource method, which can take an optional configuration array.

In the future, I think this could be useful to control how a nested resource is handled by GraphQL (for instance by using the ApiProperty attribute).

soyuka · 2022-11-02T10:04:19Z

src/GraphQl/Type/FieldsBuilder.php

@@ -47,7 +45,7 @@
 */
 final class FieldsBuilder implements FieldsBuilderInterface
 {
-    public function __construct(private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly ResourceClassResolverInterface $resourceClassResolver, private readonly TypesContainerInterface $typesContainer, private readonly TypeBuilderInterface $typeBuilder, private readonly TypeConverterInterface $typeConverter, private readonly ResolverFactoryInterface $itemResolverFactory, private readonly ResolverFactoryInterface $collectionResolverFactory, private readonly ResolverFactoryInterface $itemMutationResolverFactory, private readonly ResolverFactoryInterface $itemSubscriptionResolverFactory, private readonly ContainerInterface $filterLocator, private readonly Pagination $pagination, private readonly ?NameConverterInterface $nameConverter, private readonly string $nestingSeparator)
+    public function __construct(private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly DynamicResourceExtractorInterface $dynamicResourceExtractor, private readonly ResourceClassResolverInterface $resourceClassResolver, private readonly TypesContainerInterface $typesContainer, private readonly TypeBuilderInterface $typeBuilder, private readonly TypeConverterInterface $typeConverter, private readonly ResolverFactoryInterface $itemResolverFactory, private readonly ResolverFactoryInterface $collectionResolverFactory, private readonly ResolverFactoryInterface $itemMutationResolverFactory, private readonly ResolverFactoryInterface $itemSubscriptionResolverFactory, private readonly ContainerInterface $filterLocator, private readonly Pagination $pagination, private readonly ?NameConverterInterface $nameConverter, private readonly string $nestingSeparator)


breaks BC layer

soyuka · 2022-11-02T10:05:22Z

src/GraphQl/Type/TypeBuilder.php

@@ -210,7 +210,7 @@ public function getNodeInterface(): InterfaceType
    /**
     * {@inheritdoc}
     */
-    public function getResourcePaginatedCollectionType(GraphQLType $resourceType, string $resourceClass, Operation $operation): GraphQLType
+    public function getResourcePaginatedCollectionType(GraphQLType $resourceType, Operation $operation): GraphQLType


BC layer is broken

soyuka · 2022-11-02T10:05:37Z

src/GraphQl/Type/TypeBuilderInterface.php

@@ -43,7 +43,7 @@ public function getNodeInterface(): InterfaceType;
    /**
     * Gets the type of a paginated collection of the given resource type.
     */
-    public function getResourcePaginatedCollectionType(GraphQLType $resourceType, string $resourceClass, Operation $operation): GraphQLType;
+    public function getResourcePaginatedCollectionType(GraphQLType $resourceType, Operation $operation): GraphQLType;


We can't change an interface like this right?

soyuka · 2022-11-02T10:06:07Z

src/Metadata/Extractor/DynamicResourceExtractor.php

+        $dynamicResourceName = $this->getDynamicResourceName($resourceClass);
+
+        $this->dynamicResources[$dynamicResourceName] = [
+            array_merge(['class' => $resourceClass], $config),


array_merge is quite slow for this we could just add the class to the config right?

I think it doesn't impact performance but we can.

soyuka · 2022-11-02T10:07:22Z

src/Metadata/Extractor/DynamicResourceExtractorInterface.php

+ *
+ * @author Alan Poulain <[email protected]>
+ */
+interface DynamicResourceExtractorInterface extends ResourceExtractorInterface


do we really need to add a new interface? its wanted that a ResourceExtractorInterface doesn't have an addResource we're using a Factory to prepare resources inside it no?

We need an interface since this method needs to be called from the fields builder.

soyuka · 2022-11-02T10:08:34Z

src/Metadata/Resource/ResourceMetadataCollection.php

+    public function isDynamic(): bool
+    {
+        return str_starts_with($this->resourceClass, self::DYNAMIC_RESOURCE_CLASS_PREFIX);
+    }


I really don't like this graphql-specific code here, can you explain this?

This is not GraphQL-specific. The aim is to create metadata for a resource which doesn't exist.

soyuka · 2022-11-02T10:08:59Z

src/Metadata/Resource/ResourceMetadataCollection.php

@@ -86,6 +88,11 @@ public function getOperation(?string $operationName = null, bool $forceCollectio
        $this->handleNotFound($operationName, $metadata);
    }

+    public function isDynamic(): bool
+    {
+        return str_starts_with($this->resourceClass, self::DYNAMIC_RESOURCE_CLASS_PREFIX);


imo resourceClass should be a class-string this breaks it.

Sure but we need a way to distinguish a dynamic resource for a normal one.

This reverts commit 44337dd.

* fix: update yaml extractor test file coding standard (#5068) * fix(graphql): add clearer error message when TwigBundle is disabled but graphQL clients are enabled (#5064) * fix(metadata): add class key in payload argument resolver (#5067) * fix: add class key in payload argument resolver * add null if everything else goes wrong * fix: upgrade command remove ApiSubresource attribute (#5049) Fixes #5038 * fix(doctrine): use abitrary index instead of value (#5079) * fix: uri template should respect rfc 6570 (#5080) * fix: remove @internal annotation for Operations (#5089) See #5084 * fix(metadata): define a name on a single operation (#5090) fixes #5082 * fix(metadata): deprecate when user decorates in legacy mode (#5091) fixes #5078 * fix(graphql): use right nested operation (#5102) * Revert "fix(graphql): use right nested operation (#5102)" (#5111) This reverts commit 44337dd. * fix(graphql): always allow to query nested resources (#5112) * fix(graphql): always allow to query nested resources * review Co-authored-by: Alan Poulain <[email protected]> * chore: php-cs-fixer update * fix: only alias if exists for opcache preload Fixes api-platform/api-platform#2284 (#5110) Co-authored-by: Liviu Mirea <[email protected]> * chore: php-cs-fixer update (#5118) * chore: php-cs-fixer update * chore: php-cs-fixer update * fix(metadata): upgrade script keep operation name (#5109) origin: #5105 Co-authored-by: WilliamPeralta <[email protected]> * chore: v2.7.3 changelog * chore: v3.0.3 changelog Co-authored-by: helyakin <[email protected]> Co-authored-by: ArnoudThibaut <[email protected]> Co-authored-by: davy-beauzil <[email protected]> Co-authored-by: Baptiste Leduc <[email protected]> Co-authored-by: Xavier Laviron <[email protected]> Co-authored-by: Alan Poulain <[email protected]> Co-authored-by: Liviu Cristian Mirea-Ghiban <[email protected]> Co-authored-by: Liviu Mirea <[email protected]> Co-authored-by: WilliamPeralta <[email protected]>

fix(graphql): use right nested operation

ee8fc86

alanpoulain force-pushed the fix/graphql-nested-resource-operation branch from 85e4479 to ee8fc86 Compare October 27, 2022 16:57

alanpoulain merged commit 44337dd into api-platform:3.0 Nov 2, 2022

soyuka reviewed Nov 2, 2022

View reviewed changes

This was referenced Nov 3, 2022

Revert "fix(graphql): use right nested operation" #5111

Merged

fix(graphql): always allow to query nested resources #5112

Merged

soyuka added a commit that referenced this pull request Nov 3, 2022

Revert "fix(graphql): use right nested operation (#5102)"

dc68810

This reverts commit 44337dd.

soyuka added a commit that referenced this pull request Nov 4, 2022

Revert "fix(graphql): use right nested operation (#5102)" (#5111)

c1cb3cd

This reverts commit 44337dd.

xavierleune mentioned this pull request Dec 21, 2023

GraphQL support: Issue on pagination api-platform/api-platform#2296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(graphql): use right nested operation #5102

fix(graphql): use right nested operation #5102

alanpoulain commented Oct 27, 2022

soyuka Nov 2, 2022

soyuka Nov 2, 2022

soyuka Nov 2, 2022

soyuka Nov 2, 2022

alanpoulain Nov 2, 2022

soyuka Nov 2, 2022

alanpoulain Nov 2, 2022 •

edited

Loading

soyuka Nov 2, 2022

alanpoulain Nov 2, 2022

soyuka Nov 2, 2022

alanpoulain Nov 2, 2022

fix(graphql): use right nested operation #5102

fix(graphql): use right nested operation #5102

Conversation

alanpoulain commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain Nov 2, 2022 •

edited

Loading