Skip to content

Commit

Permalink
[GraphQL] Fix filter arguments order (#3468)
Browse files Browse the repository at this point in the history
* add behat test on graphql order filter arguments order

* change graphql filter type to preserve argument order

* extract array related function into an utility class

* update test

* fix CS

* feat: add a deprecated message when using the old filter syntax

* fix: minor fixes

* chore: add CHANGELOG entry

Co-authored-by: Alan Poulain <contact@alanpoulain.eu>
  • Loading branch information
ArnoudThibaut and alanpoulain committed Oct 15, 2020
1 parent 68163f3 commit b322aff
Show file tree
Hide file tree
Showing 10 changed files with 232 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* GraphQL: Possibility to add a custom description for queries, mutations and subscriptions (#3477, #3514)
* GraphQL: Support for field name conversion (serialized name) (#3455, #3516)
* GraphQL: **BC** `operation` is now `operationName` to follow the standard (#3568)
* GraphQL: **BC** New syntax for the filters' arguments to preserve the order: `order: [{foo: 'asc'}, {bar: 'desc'}]` (#3468)
* OpenAPI: Add PHP default values to the documentation (#2386)
* Deprecate using a validation groups generator service not implementing `ApiPlatform\Core\Bridge\Symfony\Validator\ValidationGroupsGeneratorInterface` (#3346)
* Subresources: subresource resourceClass can now be defined as a container parameter in XML and Yaml definitions
Expand Down
28 changes: 28 additions & 0 deletions features/bootstrap/DoctrineContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,34 @@ public function thereAreDummyObjectsWithRelatedDummy(int $nb)
$this->manager->flush();
}

/**
* @Given there are dummies with similar properties
*/
public function thereAreDummiesWithSimilarProperties()
{
$dummy1 = $this->buildDummy();
$dummy1->setName('foo');
$dummy1->setDescription('bar');

$dummy2 = $this->buildDummy();
$dummy2->setName('baz');
$dummy2->setDescription('qux');

$dummy3 = $this->buildDummy();
$dummy3->setName('foo');
$dummy3->setDescription('qux');

$dummy4 = $this->buildDummy();
$dummy4->setName('baz');
$dummy4->setDescription('bar');

$this->manager->persist($dummy1);
$this->manager->persist($dummy2);
$this->manager->persist($dummy3);
$this->manager->persist($dummy4);
$this->manager->flush();
}

/**
* @Given there are :nb dummyDtoNoInput objects
*/
Expand Down
30 changes: 27 additions & 3 deletions features/graphql/filters.feature
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Feature: Collections filtering
When I send the following GraphQL request:
"""
{
dummies(exists: {relatedDummy: true}) {
dummies(exists: [{relatedDummy: true}]) {
edges {
node {
id
Expand All @@ -52,7 +52,7 @@ Feature: Collections filtering
When I send the following GraphQL request:
"""
{
dummies(dummyDate: {after: "2015-04-02"}) {
dummies(dummyDate: [{after: "2015-04-02"}]) {
edges {
node {
id
Expand Down Expand Up @@ -204,7 +204,7 @@ Feature: Collections filtering
When I send the following GraphQL request:
"""
{
dummies(order: {relatedDummy__name: "DESC"}) {
dummies(order: [{relatedDummy__name: "DESC"}]) {
edges {
node {
name
Expand All @@ -222,6 +222,30 @@ Feature: Collections filtering
And the JSON node "data.dummies.edges[0].node.name" should be equal to "Dummy #2"
And the JSON node "data.dummies.edges[1].node.name" should be equal to "Dummy #1"

@createSchema
Scenario: Retrieve a collection ordered correctly given the order of the argument
Given there are dummies with similar properties
When I send the following GraphQL request:
"""
{
dummies(order: [{description: "ASC"}, {name: "ASC"}]) {
edges {
node {
id
name
description
}
}
}
}
"""
Then the response status code should be 200
And the header "Content-Type" should be equal to "application/json"
And the JSON node "data.dummies.edges[0].node.name" should be equal to "baz"
And the JSON node "data.dummies.edges[0].node.description" should be equal to "bar"
And the JSON node "data.dummies.edges[1].node.name" should be equal to "foo"
And the JSON node "data.dummies.edges[1].node.description" should be equal to "bar"

@createSchema
Scenario: Retrieve a collection filtered using the related search filter with two values and exact strategy
Given there are 3 dummy objects with relatedDummy
Expand Down
18 changes: 18 additions & 0 deletions src/GraphQl/Resolver/Stage/ReadStage.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use ApiPlatform\Core\GraphQl\Serializer\ItemNormalizer;
use ApiPlatform\Core\GraphQl\Serializer\SerializerContextBuilderInterface;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use ApiPlatform\Core\Util\ArrayTrait;
use ApiPlatform\Core\Util\ClassInfoTrait;
use GraphQL\Type\Definition\ResolveInfo;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
Expand All @@ -36,6 +37,7 @@ final class ReadStage implements ReadStageInterface
{
use ClassInfoTrait;
use IdentifierTrait;
use ArrayTrait;

private $resourceMetadataFactory;
private $iriConverter;
Expand Down Expand Up @@ -134,6 +136,22 @@ private function getNormalizedFilters(array $args): array
if (strpos($name, '_list')) {
$name = substr($name, 0, \strlen($name) - \strlen('_list'));
}

// If the value contains arrays, we need to merge them for the filters to understand this syntax, proper to GraphQL to preserve the order of the arguments.
if ($this->isSequentialArrayOfArrays($value)) {
if (\count($value[0]) > 1) {
$deprecationMessage = "The filter syntax \"$name: {";
$filterArgsOld = [];
$filterArgsNew = [];
foreach ($value[0] as $filterArgName => $filterArgValue) {
$filterArgsOld[] = "$filterArgName: \"$filterArgValue\"";
$filterArgsNew[] = sprintf('{%s: "%s"}', $filterArgName, $filterArgValue);
}
$deprecationMessage .= sprintf('%s}" is deprecated since API Platform 2.6, use the following syntax instead: "%s: [%s]".', implode(', ', $filterArgsOld), $name, implode(', ', $filterArgsNew));
@trigger_error($deprecationMessage, E_USER_DEPRECATED);
}
$value = array_merge(...$value);
}
$filters[$name] = $this->getNormalizedFilters($value);
}

Expand Down
4 changes: 2 additions & 2 deletions src/GraphQl/Type/FieldsBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -451,10 +451,10 @@ private function convertFilterArgsToTypes(array $args): array

unset($value['#name']);

$filterArgType = new InputObjectType([
$filterArgType = GraphQLType::listOf(new InputObjectType([
'name' => $name,
'fields' => $this->convertFilterArgsToTypes($value),
]);
]));

$this->typesContainer->set($name, $filterArgType);

Expand Down
42 changes: 42 additions & 0 deletions src/Util/ArrayTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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\Core\Util;

trait ArrayTrait
{
public function isSequentialArrayOfArrays(array $array): bool
{
if (!$this->isSequentialArray($array)) {
return false;
}

return $this->arrayContainsOnly($array, 'array');
}

public function isSequentialArray(array $array): bool
{
if ([] === $array) {
return false;
}

return array_keys($array) === range(0, \count($array) - 1);
}

public function arrayContainsOnly(array $array, string $type): bool
{
return $array === array_filter($array, static function ($item) use ($type): bool {
return $type === \gettype($item);
});
}
}
41 changes: 41 additions & 0 deletions tests/GraphQl/Resolver/Stage/ReadStageTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@
use ApiPlatform\Core\Tests\ProphecyTrait;
use GraphQL\Type\Definition\ResolveInfo;
use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

/**
* @author Alan Poulain <contact@alanpoulain.eu>
*/
class ReadStageTest extends TestCase
{
use ExpectDeprecationTrait;
use ProphecyTrait;

/** @var ReadStage */
Expand Down Expand Up @@ -230,6 +232,13 @@ public function collectionProvider(): array
['filter_list' => 'filtered', 'filter_field_list' => ['filtered1', 'filtered2'], 'filter.list' => 'filtered', 'filter_field' => ['filtered1', 'filtered2'], 'filter.field' => ['filtered1', 'filtered2']],
[],
],
'with array filter syntax' => [
['filter' => [['filterArg1' => 'filterValue1'], ['filterArg2' => 'filterValue2']]],
'myResource',
null,
['filter' => ['filterArg1' => 'filterValue1', 'filterArg2' => 'filterValue2']],
[],
],
'with subresource' => [
[],
'myResource',
Expand All @@ -239,4 +248,36 @@ public function collectionProvider(): array
],
];
}

/**
* @group legacy
*/
public function testApplyCollectionWithDeprecatedFilterSyntax(): void
{
$operationName = 'collection_query';
$resourceClass = 'myResource';
$info = $this->prophesize(ResolveInfo::class)->reveal();
$fieldName = 'subresource';
$info->fieldName = $fieldName;
$context = [
'is_collection' => true,
'is_mutation' => false,
'is_subscription' => false,
'args' => ['filter' => [['filterArg1' => 'filterValue1', 'filterArg2' => 'filterValue2']]],
'info' => $info,
'source' => null,
];
$this->resourceMetadataFactoryProphecy->create($resourceClass)->willReturn(new ResourceMetadata());

$normalizationContext = ['normalization' => true];
$this->serializerContextBuilderProphecy->create($resourceClass, $operationName, $context, true)->shouldBeCalled()->willReturn($normalizationContext);

$this->collectionDataProviderProphecy->getCollection($resourceClass, $operationName, $normalizationContext + ['filters' => ['filter' => ['filterArg1' => 'filterValue1', 'filterArg2' => 'filterValue2']]])->willReturn([]);

$this->expectDeprecation('The filter syntax "filter: {filterArg1: "filterValue1", filterArg2: "filterValue2"}" is deprecated since API Platform 2.6, use the following syntax instead: "filter: [{filterArg1: "filterValue1"}, {filterArg2: "filterValue2"}]".');

$result = ($this->readStage)($resourceClass, 'myResource', $operationName, $context);

$this->assertSame([], $result);
}
}
13 changes: 7 additions & 6 deletions tests/GraphQl/Type/FieldsBuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use ApiPlatform\Core\Tests\ProphecyTrait;
use GraphQL\Type\Definition\InputObjectType;
use GraphQL\Type\Definition\InterfaceType;
use GraphQL\Type\Definition\ListOfType;
use GraphQL\Type\Definition\NonNull;
use GraphQL\Type\Definition\ObjectType;
use GraphQL\Type\Definition\Type as GraphQLType;
Expand Down Expand Up @@ -233,8 +234,8 @@ public function testGetCollectionQueryFields(string $resourceClass, ResourceMeta
$this->filterLocatorProphecy->get('my_filter')->willReturn($filterProphecy->reveal());
$this->typesContainerProphecy->has('ShortNameFilter_dateField')->willReturn(false);
$this->typesContainerProphecy->has('ShortNameFilter_parent__child')->willReturn(false);
$this->typesContainerProphecy->set('ShortNameFilter_dateField', Argument::type(InputObjectType::class));
$this->typesContainerProphecy->set('ShortNameFilter_parent__child', Argument::type(InputObjectType::class));
$this->typesContainerProphecy->set('ShortNameFilter_dateField', Argument::type(ListOfType::class));
$this->typesContainerProphecy->set('ShortNameFilter_parent__child', Argument::type(ListOfType::class));

$queryFields = $this->fieldsBuilder->getCollectionQueryFields($resourceClass, $resourceMetadata, $queryName, $configuration);

Expand Down Expand Up @@ -299,8 +300,8 @@ public function collectionQueryFieldsProvider(): array
],
'boolField' => $graphqlType,
'boolField_list' => GraphQLType::listOf($graphqlType),
'parent__child' => new InputObjectType(['name' => 'ShortNameFilter_parent__child', 'fields' => ['related__nested' => $graphqlType]]),
'dateField' => new InputObjectType(['name' => 'ShortNameFilter_dateField', 'fields' => ['before' => $graphqlType]]),
'parent__child' => GraphQLType::listOf(new InputObjectType(['name' => 'ShortNameFilter_parent__child', 'fields' => ['related__nested' => $graphqlType]])),
'dateField' => GraphQLType::listOf(new InputObjectType(['name' => 'ShortNameFilter_dateField', 'fields' => ['before' => $graphqlType]])),
],
'resolve' => $resolver,
'deprecationReason' => null,
Expand Down Expand Up @@ -351,8 +352,8 @@ public function collectionQueryFieldsProvider(): array
],
'boolField' => $graphqlType,
'boolField_list' => GraphQLType::listOf($graphqlType),
'parent__child' => new InputObjectType(['name' => 'ShortNameFilter_parent__child', 'fields' => ['related__nested' => $graphqlType]]),
'dateField' => new InputObjectType(['name' => 'ShortNameFilter_dateField', 'fields' => ['before' => $graphqlType]]),
'parent__child' => GraphQLType::listOf(new InputObjectType(['name' => 'ShortNameFilter_parent__child', 'fields' => ['related__nested' => $graphqlType]])),
'dateField' => GraphQLType::listOf(new InputObjectType(['name' => 'ShortNameFilter_dateField', 'fields' => ['before' => $graphqlType]])),
],
'resolve' => $resolver,
'deprecationReason' => null,
Expand Down
4 changes: 1 addition & 3 deletions tests/ProphecyTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,7 @@ private function countProphecyAssertions(): void
$this->prophecyAssertionsCounted = true;

foreach ($this->prophet->getProphecies() as $objectProphecy) {
/**
* @var MethodProphecy[] $methodProphecies
*/
/** @var MethodProphecy[] $methodProphecies */
foreach ($objectProphecy->getMethodProphecies() as $methodProphecies) {
foreach ($methodProphecies as $methodProphecy) {
\assert($methodProphecy instanceof MethodProphecy);
Expand Down
65 changes: 65 additions & 0 deletions tests/Util/ArrayTraitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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\Core\Tests\Util;

use ApiPlatform\Core\Util\ArrayTrait;
use PHPUnit\Framework\TestCase;

class ArrayTraitTest extends TestCase
{
private $arrayTraitClass;

protected function setUp(): void
{
$this->arrayTraitClass = (new class() {
use ArrayTrait;
});
}

public function testIsSequentialArrayWithEmptyArray(): void
{
self::assertFalse($this->arrayTraitClass->isSequentialArray([]));
}

public function testIsSequentialArrayWithNonNumericIndex(): void
{
self::assertFalse($this->arrayTraitClass->isSequentialArray(['foo' => 'bar']));
}

public function testIsSequentialArrayWithNumericNonSequentialIndex(): void
{
self::assertFalse($this->arrayTraitClass->isSequentialArray([1 => 'bar', 3 => 'foo']));
}

public function testIsSequentialArrayWithNumericSequentialIndex(): void
{
self::assertTrue($this->arrayTraitClass->isSequentialArray([0 => 'bar', 1 => 'foo']));
}

public function testArrayContainsOnlyWithDifferentTypes(): void
{
self::assertFalse($this->arrayTraitClass->arrayContainsOnly([1, 'foo'], 'string'));
}

public function testArrayContainsOnlyWithSameType(): void
{
self::assertTrue($this->arrayTraitClass->arrayContainsOnly(['foo', 'bar'], 'string'));
}

public function testIsSequentialArrayOfArrays(): void
{
self::assertFalse($this->arrayTraitClass->isSequentialArrayOfArrays([]));
self::assertTrue($this->arrayTraitClass->isSequentialArrayOfArrays([['foo'], ['bar']]));
}
}

0 comments on commit b322aff

Please sign in to comment.