Skip to content

fix(graphql): use right nested operation #5102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions features/graphql/collection.feature
Original file line number Diff line number Diff line change
Expand Up @@ -910,3 +910,49 @@ Feature: GraphQL collection support
Then the response status code should be 200
And the response should be in JSON
And the JSON node "data.fooDummies.collection" should have 1 element

@createSchema
Scenario: Retrieve paginated collections using mixed pagination
Given there are 5 fooDummy objects with fake names
When I send the following GraphQL request:
"""
{
fooDummies(page: 1) {
collection {
id
name
soManies(first: 2) {
edges {
node {
content
}
cursor
}
pageInfo {
startCursor
endCursor
hasNextPage
hasPreviousPage
}
}
}
paginationInfo {
itemsPerPage
lastPage
totalCount
}
}
}
"""
Then the response status code should be 200
And the response should be in JSON
And the JSON node "data.fooDummies.collection" should have 3 elements
And the JSON node "data.fooDummies.collection[2].id" should exist
And the JSON node "data.fooDummies.collection[2].name" should exist
And the JSON node "data.fooDummies.collection[2].soManies" should exist
And the JSON node "data.fooDummies.collection[2].soManies.edges" should have 2 elements
And the JSON node "data.fooDummies.collection[2].soManies.edges[1].node.content" should be equal to "So many 1"
And the JSON node "data.fooDummies.collection[2].soManies.pageInfo.startCursor" should be equal to "MA=="
And the JSON node "data.fooDummies.paginationInfo.itemsPerPage" should be equal to the number 3
And the JSON node "data.fooDummies.paginationInfo.lastPage" should be equal to the number 2
And the JSON node "data.fooDummies.paginationInfo.totalCount" should be equal to the number 5
36 changes: 31 additions & 5 deletions features/main/default_order.feature
Original file line number Diff line number Diff line change
Expand Up @@ -79,35 +79,61 @@ Feature: Default order
"@type": "FooDummy",
"id": 5,
"name": "Balbo",
"dummy": "/dummies/5"
"dummy": "/dummies/5",
"soManies": [
"/so_manies/13",
"/so_manies/14",
"/so_manies/15"
]

},
{
"@id": "/foo_dummies/3",
"@type": "FooDummy",
"id": 3,
"name": "Sthenelus",
"dummy": "/dummies/3"
"dummy": "/dummies/3",
"soManies": [
"/so_manies/7",
"/so_manies/8",
"/so_manies/9"
]
},
{
"@id": "/foo_dummies/2",
"@type": "FooDummy",
"id": 2,
"name": "Ephesian",
"dummy": "/dummies/2"
"dummy": "/dummies/2",
"soManies": [
"/so_manies/4",
"/so_manies/5",
"/so_manies/6"
]
},
{
"@id": "/foo_dummies/1",
"@type": "FooDummy",
"id": 1,
"name": "Hawsepipe",
"dummy": "/dummies/1"
"dummy": "/dummies/1",
"soManies": [
"/so_manies/1",
"/so_manies/2",
"/so_manies/3"
]
},
{
"@id": "/foo_dummies/4",
"@type": "FooDummy",
"id": 4,
"name": "Separativeness",
"dummy": "/dummies/4"
"dummy": "/dummies/4",
"soManies": [
"/so_manies/10",
"/so_manies/11",
"/so_manies/12"
]
}
],
"hydra:totalItems": 5,
Expand Down
75 changes: 35 additions & 40 deletions src/GraphQl/Type/FieldsBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@
use ApiPlatform\Exception\OperationNotFoundException;
use ApiPlatform\GraphQl\Resolver\Factory\ResolverFactoryInterface;
use ApiPlatform\GraphQl\Type\Definition\TypeInterface;
use ApiPlatform\Metadata\Extractor\DynamicResourceExtractorInterface;
use ApiPlatform\Metadata\GraphQl\Mutation;
use ApiPlatform\Metadata\GraphQl\Operation;
use ApiPlatform\Metadata\GraphQl\Query;
use ApiPlatform\Metadata\GraphQl\QueryCollection;
use ApiPlatform\Metadata\GraphQl\Subscription;
use ApiPlatform\Metadata\Operation as AbstractOperation;
use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaks BC layer

{
}

Expand Down Expand Up @@ -256,7 +254,25 @@ private function getResourceFieldConfiguration(?string $property, ?string $field
$resourceClass = $type->getClassName();
}

$graphqlType = $this->convertType($type, $input, $rootOperation, $resourceClass ?? '', $rootResource, $property, $depth, $forceNullable);
$resourceOperation = $rootOperation;
if ($resourceClass && $rootOperation->getClass() && $this->resourceClassResolver->isResourceClass($resourceClass) && $rootOperation->getClass() !== $resourceClass) {
$resourceMetadataCollection = $this->resourceMetadataCollectionFactory->create($resourceClass);
try {
$resourceOperation = $resourceMetadataCollection->getOperation($isCollectionType ? 'collection_query' : 'item_query');
} catch (OperationNotFoundException) {
// If there is no query operation for a nested resource, use a dynamic resource to get one.
$dynamicResourceMetadataCollection = $this->resourceMetadataCollectionFactory->create($this->dynamicResourceExtractor->addResource($resourceClass));

$resourceOperation = $dynamicResourceMetadataCollection->getOperation($isCollectionType ? 'collection_query' : 'item_query')
->withResource($resourceMetadataCollection[0]);
}
}

if (!$resourceOperation instanceof Operation) {
throw new \LogicException('The resource operation should be a GraphQL operation.');
}

$graphqlType = $this->convertType($type, $input, $resourceOperation, $rootOperation, $resourceClass ?? '', $rootResource, $property, $depth, $forceNullable);

$graphqlWrappedType = $graphqlType instanceof WrappingType ? $graphqlType->getWrappedType(true) : $graphqlType;
$isStandardGraphqlType = \in_array($graphqlWrappedType, GraphQLType::getStandardTypes(), true);
Expand All @@ -271,43 +287,22 @@ private function getResourceFieldConfiguration(?string $property, ?string $field

$args = [];

$resolverOperation = $rootOperation;

if ($resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass) && $rootOperation->getClass() !== $resourceClass) {
$resourceMetadataCollection = $this->resourceMetadataCollectionFactory->create($resourceClass);
$resolverOperation = $resourceMetadataCollection->getOperation(null, $isCollectionType);

if (!$resolverOperation instanceof Operation) {
$resolverOperation = ($isCollectionType ? new QueryCollection() : new Query())->withOperation($resolverOperation);
}
}

if (!$input && !$rootOperation instanceof Mutation && !$rootOperation instanceof Subscription && !$isStandardGraphqlType && $isCollectionType) {
if ($this->pagination->isGraphQlEnabled($rootOperation)) {
$args = $this->getGraphQlPaginationArgs($rootOperation);
}

// Find the collection operation to get filters, there might be a smarter way to do this
$operation = null;
if (!empty($resourceClass)) {
$resourceMetadataCollection = $this->resourceMetadataCollectionFactory->create($resourceClass);
try {
$operation = $resourceMetadataCollection->getOperation(null, true);
} catch (OperationNotFoundException) {
}
if ($this->pagination->isGraphQlEnabled($resourceOperation)) {
$args = $this->getGraphQlPaginationArgs($resourceOperation);
}

$args = $this->getFilterArgs($args, $resourceClass, $rootResource, $rootOperation, $property, $depth, $operation);
$args = $this->getFilterArgs($args, $resourceClass, $rootResource, $resourceOperation, $rootOperation, $property, $depth);
}

if ($isStandardGraphqlType || $input) {
$resolve = null;
} elseif (($rootOperation instanceof Mutation || $rootOperation instanceof Subscription) && $depth <= 0) {
$resolve = $rootOperation instanceof Mutation ? ($this->itemMutationResolverFactory)($resourceClass, $rootResource, $resolverOperation) : ($this->itemSubscriptionResolverFactory)($resourceClass, $rootResource, $resolverOperation);
$resolve = $rootOperation instanceof Mutation ? ($this->itemMutationResolverFactory)($resourceClass, $rootResource, $resourceOperation) : ($this->itemSubscriptionResolverFactory)($resourceClass, $rootResource, $resourceOperation);
} elseif ($this->typeBuilder->isCollection($type)) {
$resolve = ($this->collectionResolverFactory)($resourceClass, $rootResource, $resolverOperation);
$resolve = ($this->collectionResolverFactory)($resourceClass, $rootResource, $resourceOperation);
} else {
$resolve = ($this->itemResolverFactory)($resourceClass, $rootResource, $resolverOperation);
$resolve = ($this->itemResolverFactory)($resourceClass, $rootResource, $resourceOperation);
}

return [
Expand Down Expand Up @@ -368,21 +363,21 @@ private function getGraphQlPaginationArgs(Operation $queryOperation): array
return $args;
}

private function getFilterArgs(array $args, ?string $resourceClass, string $rootResource, Operation $rootOperation, ?string $property, int $depth, ?AbstractOperation $operation = null): array
private function getFilterArgs(array $args, ?string $resourceClass, string $rootResource, Operation $resourceOperation, Operation $rootOperation, ?string $property, int $depth): array
{
if (null === $operation || null === $resourceClass) {
if (null === $resourceClass) {
return $args;
}

foreach ($operation->getFilters() ?? [] as $filterId) {
foreach ($resourceOperation->getFilters() ?? [] as $filterId) {
if (!$this->filterLocator->has($filterId)) {
continue;
}

foreach ($this->filterLocator->get($filterId)->getDescription($resourceClass) as $key => $value) {
$nullable = isset($value['required']) ? !$value['required'] : true;
$filterType = \in_array($value['type'], Type::$builtinTypes, true) ? new Type($value['type'], $nullable) : new Type('object', $nullable, $value['type']);
$graphqlFilterType = $this->convertType($filterType, false, $rootOperation, $resourceClass, $rootResource, $property, $depth);
$graphqlFilterType = $this->convertType($filterType, false, $resourceOperation, $rootOperation, $resourceClass, $rootResource, $property, $depth);

if (str_ends_with($key, '[]')) {
$graphqlFilterType = GraphQLType::listOf($graphqlFilterType);
Expand All @@ -399,14 +394,14 @@ private function getFilterArgs(array $args, ?string $resourceClass, string $root
array_walk_recursive($parsed, static function (&$value) use ($graphqlFilterType): void {
$value = $graphqlFilterType;
});
$args = $this->mergeFilterArgs($args, $parsed, $operation, $key);
$args = $this->mergeFilterArgs($args, $parsed, $resourceOperation, $key);
}
}

return $this->convertFilterArgsToTypes($args);
}

private function mergeFilterArgs(array $args, array $parsed, ?AbstractOperation $operation = null, string $original = ''): array
private function mergeFilterArgs(array $args, array $parsed, ?Operation $operation = null, string $original = ''): array
{
foreach ($parsed as $key => $value) {
// Never override keys that cannot be merged
Expand Down Expand Up @@ -470,7 +465,7 @@ private function convertFilterArgsToTypes(array $args): array
*
* @throws InvalidTypeException
*/
private function convertType(Type $type, bool $input, Operation $rootOperation, string $resourceClass, string $rootResource, ?string $property, int $depth, bool $forceNullable = false): GraphQLType|ListOfType|NonNull
private function convertType(Type $type, bool $input, Operation $resourceOperation, Operation $rootOperation, string $resourceClass, string $rootResource, ?string $property, int $depth, bool $forceNullable = false): GraphQLType|ListOfType|NonNull
{
$graphqlType = $this->typeConverter->convertType($type, $input, $rootOperation, $resourceClass, $rootResource, $property, $depth);

Expand All @@ -487,7 +482,7 @@ private function convertType(Type $type, bool $input, Operation $rootOperation,
}

if ($this->typeBuilder->isCollection($type)) {
return $this->pagination->isGraphQlEnabled($rootOperation) && !$input ? $this->typeBuilder->getResourcePaginatedCollectionType($graphqlType, $resourceClass, $rootOperation) : GraphQLType::listOf($graphqlType);
return $this->pagination->isGraphQlEnabled($resourceOperation) && !$input ? $this->typeBuilder->getResourcePaginatedCollectionType($graphqlType, $resourceOperation) : GraphQLType::listOf($graphqlType);
}

return $forceNullable || !$graphqlType instanceof NullableType || $type->isNullable() || ($rootOperation instanceof Mutation && 'update' === $rootOperation->getName())
Expand Down
2 changes: 1 addition & 1 deletion src/GraphQl/Type/TypeBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BC layer is broken

{
$shortName = $resourceType->name;
$paginationType = $this->pagination->getGraphQlPaginationType($operation);
Expand Down
2 changes: 1 addition & 1 deletion src/GraphQl/Type/TypeBuilderInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change an interface like this right?


/**
* Returns true if a type is a collection.
Expand Down
50 changes: 50 additions & 0 deletions src/Metadata/Extractor/DynamicResourceExtractor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* 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\Metadata\Extractor;

use ApiPlatform\Metadata\Resource\ResourceMetadataCollection;

/**
* Extracts a dynamic resource (used by GraphQL for nested resources).
*
* @author Alan Poulain <[email protected]>
*/
final class DynamicResourceExtractor implements DynamicResourceExtractorInterface
{
private array $dynamicResources = [];

/**
* {@inheritdoc}
*/
public function getResources(): array
{
return $this->dynamicResources;
}

public function addResource(string $resourceClass, array $config = []): string
{
$dynamicResourceName = $this->getDynamicResourceName($resourceClass);

$this->dynamicResources[$dynamicResourceName] = [
array_merge(['class' => $resourceClass], $config),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

];

return $dynamicResourceName;
}

private function getDynamicResourceName(string $resourceClass): string
{
return ResourceMetadataCollection::DYNAMIC_RESOURCE_CLASS_PREFIX.$resourceClass;
}
}
24 changes: 24 additions & 0 deletions src/Metadata/Extractor/DynamicResourceExtractorInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* 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\Metadata\Extractor;

/**
* Extracts a dynamic resource (used by GraphQL for nested resources).
*
* @author Alan Poulain <[email protected]>
*/
interface DynamicResourceExtractorInterface extends ResourceExtractorInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@alanpoulain alanpoulain Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public function addResource(string $resourceClass, array $config = []): string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ public function create(string $resourceClass): ResourceMetadataCollection
$resourceMetadataCollection = $this->decorated->create($resourceClass);
}

if ($resourceMetadataCollection->isDynamic()) {
return $resourceMetadataCollection;
}

try {
$reflectionClass = new \ReflectionClass($resourceClass);
} catch (\ReflectionException) {
Expand Down
Loading