From 259775a413ba7745625df831c491af330766820c Mon Sep 17 00:00:00 2001 From: meyerbaptiste Date: Thu, 12 Apr 2018 11:45:29 +0200 Subject: [PATCH 01/24] Fix the Behat test suite with low dependencies --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 0d1bb38881e..20d92c5d136 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,7 @@ "behat/mink-browserkit-driver": "^1.3.1", "behat/mink-extension": "^2.2", "behat/symfony2-extension": "^2.1.1", - "behatch/contexts": "^3.0@dev", + "behatch/contexts": "^3.1", "doctrine/annotations": "^1.2", "doctrine/doctrine-bundle": "^1.6.3", "doctrine/orm": "^2.5.2", From d6e7d66b7452a51e320a14180dd59ba7542969f7 Mon Sep 17 00:00:00 2001 From: Baptiste Meyer Date: Thu, 12 Apr 2018 15:38:22 +0200 Subject: [PATCH 02/24] Merge pull request #1853 from meyerbaptiste/fix_behatch_low_dependencies Fix the Behat test suite with low dependencies From d120915bd78bb7f9fcd930134c39e1a09f4e7738 Mon Sep 17 00:00:00 2001 From: Ion Bazan <1985514+IonBazan@users.noreply.github.com> Date: Sun, 15 Apr 2018 18:27:53 +0200 Subject: [PATCH 03/24] add page number type validation --- features/hydra/collection.feature | 2 +- features/jsonapi/pagination.feature | 4 ++++ src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php | 8 ++++++-- .../Doctrine/Orm/Extension/PaginationExtensionTest.php | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/features/hydra/collection.feature b/features/hydra/collection.feature index 61935e63e39..4ffdfa851a4 100644 --- a/features/hydra/collection.feature +++ b/features/hydra/collection.feature @@ -436,4 +436,4 @@ Feature: Collections support When I send a "GET" request to "/dummies?itemsPerPage=0&page=2" Then the response status code should be 400 - And the JSON node "hydra:description" should be equal to "Page should not be greater than 1 if itemsPegPage is equal to 0" + And the JSON node "hydra:description" should be equal to "Page should not be greater than 1 if itemsPerPage is equal to 0" diff --git a/features/jsonapi/pagination.feature b/features/jsonapi/pagination.feature index ff6e17213d8..12d822ea6bd 100644 --- a/features/jsonapi/pagination.feature +++ b/features/jsonapi/pagination.feature @@ -33,3 +33,7 @@ Feature: JSON API pagination handling And the JSON node "meta.totalItems" should be equal to the number 10 And the JSON node "meta.itemsPerPage" should be equal to the number 15 And the JSON node "meta.currentPage" should be equal to the number 1 + + Scenario: Get an error when provided page number is not valid + When I send a "GET" request to "/dummies?page[page]=0" + Then the response status code should be 400 diff --git a/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php b/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php index 4427cd3cf63..e1f159c3271 100644 --- a/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php +++ b/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php @@ -103,10 +103,14 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator throw new InvalidArgumentException('Item per page parameter should not be less than 0'); } - $page = $this->getPaginationParameter($request, $this->pageParameterName, 1); + $page = (int) $this->getPaginationParameter($request, $this->pageParameterName, 1); + + if (1 > $page) { + throw new InvalidArgumentException('Page should not be less than 1'); + } if (0 === $itemsPerPage && 1 < $page) { - throw new InvalidArgumentException('Page should not be greater than 1 if itemsPegPage is equal to 0'); + throw new InvalidArgumentException('Page should not be greater than 1 if itemsPerPage is equal to 0'); } $firstResult = ($page - 1) * $itemsPerPage; diff --git a/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php index a5e2484b614..7b1cc77e41c 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php @@ -101,7 +101,7 @@ public function testApplyToCollectionWithItemPerPageZero() /** * @expectedException \ApiPlatform\Core\Exception\InvalidArgumentException - * @expectedExceptionMessage Page should not be greater than 1 if itemsPegPage is equal to 0 + * @expectedExceptionMessage Page should not be greater than 1 if itemsPerPage is equal to 0 */ public function testApplyToCollectionWithItemPerPageZeroAndPage2() { From d99dc235eb362495500a5af9ee7480f23a7fe647 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Wed, 18 Apr 2018 14:42:55 +0200 Subject: [PATCH 04/24] Merge pull request #1871 from IonBazan/page-number-int Page number int casting From faeef0ff5f41b3e242932b949a7b7c20aea0af2e Mon Sep 17 00:00:00 2001 From: abluchet Date: Thu, 19 Apr 2018 09:35:12 +0200 Subject: [PATCH 05/24] add missing doctrine alias removed in doctrine-bundle@1.9 --- src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml | 1 + .../Bundle/DependencyInjection/ApiPlatformExtensionTest.php | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml index 82d59e76390..777b81575db 100644 --- a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml +++ b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml @@ -164,6 +164,7 @@ + diff --git a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index b5ad216ed93..52af3f90e8f 100644 --- a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -38,6 +38,7 @@ use ApiPlatform\Core\Tests\Fixtures\TestBundle\TestBundle; use ApiPlatform\Core\Validator\ValidatorInterface; use Doctrine\Bundle\DoctrineBundle\DoctrineBundle; +use Doctrine\Common\Persistence\ManagerRegistry; use FOS\UserBundle\FOSUserBundle; use Nelmio\ApiDocBundle\NelmioApiDocBundle; use PHPUnit\Framework\TestCase; @@ -704,6 +705,7 @@ private function getBaseContainerBuilderProphecy() PaginationExtension::class => 'api_platform.doctrine.orm.query_extension.pagination', OrderExtension::class => 'api_platform.doctrine.orm.query_extension.order', ValidatorInterface::class => 'api_platform.validator', + ManagerRegistry::class => 'doctrine', ]; foreach ($aliases as $alias => $service) { From 8b395db1beebcc5d0841a9ecb834bfb44aaa2779 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Thu, 19 Apr 2018 09:54:52 +0200 Subject: [PATCH 06/24] Merge pull request #1876 from soyuka/fix-tests Add missing doctrine alias removed in doctrine-bundle@1.9 From 27ca12146ab2109ad6f46942bf84231ac0f69f5a Mon Sep 17 00:00:00 2001 From: Ion Bazan <1985514+IonBazan@users.noreply.github.com> Date: Fri, 13 Apr 2018 09:04:42 +0200 Subject: [PATCH 07/24] fix exists filter --- features/doctrine/date_filter.feature | 16 +++++++++++-- features/graphql/filters.feature | 24 +++++++++++++++++++ features/main/crud.feature | 8 ++++++- .../Doctrine/Orm/Filter/ExistsFilter.php | 4 ++-- tests/Fixtures/app/config/config_test.yml | 2 +- 5 files changed, 48 insertions(+), 6 deletions(-) diff --git a/features/doctrine/date_filter.feature b/features/doctrine/date_filter.feature index d7b8c7885d9..d6ae3af2785 100644 --- a/features/doctrine/date_filter.feature +++ b/features/doctrine/date_filter.feature @@ -406,7 +406,7 @@ Feature: Date filter on collections }, "hydra:search": { "@type": "hydra:IriTemplate", - "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],dummyFloat,dummyPrice,order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,properties[]}", + "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyPrice,order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,properties[]}", "hydra:variableRepresentation": "BasicRepresentation", "hydra:mapping": [ { @@ -487,6 +487,12 @@ Feature: Date filter on collections "property": "dummyBoolean", "required": false }, + { + "@type": "IriTemplateMapping", + "variable": "relatedDummy[exists]", + "property": "relatedDummy", + "required": false + }, { "@type": "IriTemplateMapping", "variable": "dummyFloat", @@ -721,7 +727,7 @@ Feature: Date filter on collections }, "hydra:search": { "@type": "hydra:IriTemplate", - "hydra:template": "/dummies{?dummyBoolean,dummyDate[before],dummyDate[after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],dummyFloat,dummyPrice,order[id],order[name],order[relatedDummy.symfony],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name}", + "hydra:template": "/dummies{?dummyBoolean,dummyDate[before],dummyDate[after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyPrice,order[id],order[name],order[relatedDummy.symfony],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name}", "hydra:variableRepresentation": "BasicRepresentation", "hydra:mapping": [ { @@ -790,6 +796,12 @@ Feature: Date filter on collections "property": "relatedDummy.name", "required": false }, + { + "@type": "IriTemplateMapping", + "variable": "relatedDummy[exists]", + "property": "relatedDummy", + "required": false + }, { "@type": "IriTemplateMapping", "variable": "dummyBoolean[exists]", diff --git a/features/graphql/filters.feature b/features/graphql/filters.feature index f1c68392ea2..011005cea5d 100644 --- a/features/graphql/filters.feature +++ b/features/graphql/filters.feature @@ -24,6 +24,30 @@ Feature: Collections filtering Then the JSON node "data.dummies.edges" should have 1 element And the JSON node "data.dummies.edges[0].node.dummyBoolean" should be false + @createSchema + @dropSchema + Scenario: Retrieve a collection filtered using the exists filter + Given there are 3 dummy objects + And there are 2 dummy objects with relatedDummy + When I send the following GraphQL request: + """ + { + dummies(relatedDummy: {exists: true}) { + edges { + node { + id + relatedDummy { + name + } + } + } + } + } + """ + Then the response status code should be 200 + And the JSON node "data.dummies.edges" should have 2 elements + And the JSON node "data.dummies.edges[0].node.relatedDummy" should have 1 element + @createSchema @dropSchema Scenario: Retrieve a collection filtered using the date filter diff --git a/features/main/crud.feature b/features/main/crud.feature index e4c1918cfca..4986c25a77d 100644 --- a/features/main/crud.feature +++ b/features/main/crud.feature @@ -129,7 +129,7 @@ Feature: Create-Retrieve-Update-Delete "hydra:totalItems": 1, "hydra:search": { "@type": "hydra:IriTemplate", - "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],dummyFloat,dummyPrice,order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,properties[]}", + "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyPrice,order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,properties[]}", "hydra:variableRepresentation": "BasicRepresentation", "hydra:mapping": [ { @@ -210,6 +210,12 @@ Feature: Create-Retrieve-Update-Delete "property": "dummyBoolean", "required": false }, + { + "@type": "IriTemplateMapping", + "variable": "relatedDummy[exists]", + "property": "relatedDummy", + "required": false + }, { "@type": "IriTemplateMapping", "variable": "dummyFloat", diff --git a/src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php b/src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php index 4a8be8b8bee..4e0f5edddf5 100644 --- a/src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php +++ b/src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php @@ -75,9 +75,9 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB return; } - if (\in_array($value[self::QUERY_PARAMETER_KEY], ['true', '1', '', null], true)) { + if (\in_array($value[self::QUERY_PARAMETER_KEY], [true, 'true', '1', '', null], true)) { $value = true; - } elseif (\in_array($value[self::QUERY_PARAMETER_KEY], ['false', '0'], true)) { + } elseif (\in_array($value[self::QUERY_PARAMETER_KEY], [false, 'false', '0'], true)) { $value = false; } else { $this->logger->notice('Invalid filter ignored', [ diff --git a/tests/Fixtures/app/config/config_test.yml b/tests/Fixtures/app/config/config_test.yml index 48e384d9124..e8513018962 100644 --- a/tests/Fixtures/app/config/config_test.yml +++ b/tests/Fixtures/app/config/config_test.yml @@ -152,7 +152,7 @@ services: app.my_dummy_resource.exists_filter: parent: 'api_platform.doctrine.orm.exists_filter' - arguments: [ { 'description': ~, 'relatedDummy.name': ~, 'dummyBoolean': ~ } ] + arguments: [ { 'description': ~, 'relatedDummy.name': ~, 'dummyBoolean': ~, 'relatedDummy': ~ } ] tags: [ { name: 'api_platform.filter', id: 'my_dummy.exists' } ] app.my_dummy_resource.property_filter: From a4bfa4ead11b2ac4763eab008ef6fcedbd456a15 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Thu, 19 Apr 2018 11:11:23 +0200 Subject: [PATCH 08/24] Merge pull request #1861 from IonBazan/exists-fix fix exists filter From a675a8eb305d99aca27a2bd93de973a55932f367 Mon Sep 17 00:00:00 2001 From: abluchet Date: Wed, 18 Apr 2018 19:07:12 +0200 Subject: [PATCH 09/24] Enable item route on collection subresources --- features/main/subresource.feature | 37 ++++++- .../Doctrine/Orm/SubresourceDataProvider.php | 45 ++++---- .../AnnotationSubresourceMetadataFactory.php | 11 +- .../Factory/SubresourceOperationFactory.php | 24 +++- .../Orm/SubresourceDataProviderTest.php | 96 ++++++++++++++++ .../TestBundle/Entity/RelatedDummy.php | 1 + .../SubresourceOperationFactoryTest.php | 103 ++++++++++++++++++ 7 files changed, 284 insertions(+), 33 deletions(-) diff --git a/features/main/subresource.feature b/features/main/subresource.feature index 6c69e708d55..c3034b8adb1 100644 --- a/features/main/subresource.feature +++ b/features/main/subresource.feature @@ -30,7 +30,7 @@ Feature: Subresource support And the response status code should be 404 And the response should be in JSON - Scenario: Get subresource one to one relation + Scenario: Get recursive subresource one to many relation When I send a "GET" request to "/questions/1/answer/related_questions" And the response status code should be 200 And the response should be in JSON @@ -209,7 +209,7 @@ Feature: Subresource support } """ - Scenario: Get filtered embedded relation collection + Scenario: Get filtered embedded relation subresource collection When I send a "GET" request to "/dummies/1/related_dummies?name=Hello" And the response status code should be 200 And the response should be in JSON @@ -272,7 +272,34 @@ Feature: Subresource support } """ - Scenario: Get the embedded relation collection at the third level + Scenario: Get the subresource relation item + When I send a "GET" request to "/dummies/1/related_dummies/2" + And the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8" + And the JSON should be equal to: + """ + { + "@context": "/contexts/RelatedDummy", + "@id": "/related_dummies/2", + "@type": "https://schema.org/Product", + "id": 2, + "name": null, + "symfony": "symfony", + "dummyDate": null, + "thirdLevel": { + "@id": "/third_levels/1", + "@type": "ThirdLevel", + "fourthLevel": "/fourth_levels/1" + }, + "relatedToDummyFriend": [], + "dummyBoolean": null, + "embeddedDummy": [], + "age": null + } + """ + + Scenario: Get the embedded relation subresource item at the third level When I send a "GET" request to "/dummies/1/related_dummies/1/third_level" And the response status code should be 200 And the response should be in JSON @@ -290,7 +317,7 @@ Feature: Subresource support } """ - Scenario: Get the embedded relation collection at the fourth level + Scenario: Get the embedded relation subresource item at the fourth level When I send a "GET" request to "/dummies/1/related_dummies/1/third_level/fourth_level" And the response status code should be 200 And the response should be in JSON @@ -356,7 +383,7 @@ Feature: Subresource support """ @dropSchema - Scenario: test + Scenario: Recursive resource When I send a "GET" request to "/dummy_products/2" And the response status code should be 200 And the response should be in JSON diff --git a/src/Bridge/Doctrine/Orm/SubresourceDataProvider.php b/src/Bridge/Doctrine/Orm/SubresourceDataProvider.php index a8992514663..a470828bf15 100644 --- a/src/Bridge/Doctrine/Orm/SubresourceDataProvider.php +++ b/src/Bridge/Doctrine/Orm/SubresourceDataProvider.php @@ -151,32 +151,37 @@ private function buildQuery(array $identifiers, array $context, QueryNameGenerat $qb = $manager->createQueryBuilder(); $alias = $queryNameGenerator->generateJoinAlias($identifier); - $relationType = $classMetadata->getAssociationMapping($previousAssociationProperty)['type']; $normalizedIdentifiers = isset($identifiers[$identifier]) ? $this->normalizeIdentifiers( $identifiers[$identifier], $manager, $identifierResourceClass ) : []; - switch ($relationType) { - // MANY_TO_MANY relations need an explicit join so that the identifier part can be retrieved - case ClassMetadataInfo::MANY_TO_MANY: - $joinAlias = $queryNameGenerator->generateJoinAlias($previousAssociationProperty); - - $qb->select($joinAlias) - ->from($identifierResourceClass, $alias) - ->innerJoin("$alias.$previousAssociationProperty", $joinAlias); - break; - case ClassMetadataInfo::ONE_TO_MANY: - $mappedBy = $classMetadata->getAssociationMapping($previousAssociationProperty)['mappedBy']; - $previousAlias = "$previousAlias.$mappedBy"; - - $qb->select($alias) - ->from($identifierResourceClass, $alias); - break; - default: - $qb->select("IDENTITY($alias.$previousAssociationProperty)") - ->from($identifierResourceClass, $alias); + if ($classMetadata->hasAssociation($previousAssociationProperty)) { + $relationType = $classMetadata->getAssociationMapping($previousAssociationProperty)['type']; + switch ($relationType) { + // MANY_TO_MANY relations need an explicit join so that the identifier part can be retrieved + case ClassMetadataInfo::MANY_TO_MANY: + $joinAlias = $queryNameGenerator->generateJoinAlias($previousAssociationProperty); + + $qb->select($joinAlias) + ->from($identifierResourceClass, $alias) + ->innerJoin("$alias.$previousAssociationProperty", $joinAlias); + break; + case ClassMetadataInfo::ONE_TO_MANY: + $mappedBy = $classMetadata->getAssociationMapping($previousAssociationProperty)['mappedBy']; + $previousAlias = "$previousAlias.$mappedBy"; + + $qb->select($alias) + ->from($identifierResourceClass, $alias); + break; + default: + $qb->select("IDENTITY($alias.$previousAssociationProperty)") + ->from($identifierResourceClass, $alias); + } + } elseif ($classMetadata->isIdentifier($previousAssociationProperty)) { + $qb->select($alias) + ->from($identifierResourceClass, $alias); } // Add where clause for identifiers diff --git a/src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php b/src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php index 122e5ce8244..9a57518b365 100644 --- a/src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php +++ b/src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php @@ -52,7 +52,7 @@ public function create(string $resourceClass, string $property, array $options = $annotation = $this->reader->getPropertyAnnotation($reflectionClass->getProperty($property), ApiSubresource::class); if (null !== $annotation) { - return $this->updateMetadata($annotation, $propertyMetadata); + return $this->updateMetadata($annotation, $propertyMetadata, $resourceClass); } } @@ -70,19 +70,24 @@ public function create(string $resourceClass, string $property, array $options = $annotation = $this->reader->getMethodAnnotation($reflectionMethod, ApiSubresource::class); if (null !== $annotation) { - return $this->updateMetadata($annotation, $propertyMetadata); + return $this->updateMetadata($annotation, $propertyMetadata, $resourceClass); } } return $propertyMetadata; } - private function updateMetadata(ApiSubresource $annotation, PropertyMetadata $propertyMetadata): PropertyMetadata + private function updateMetadata(ApiSubresource $annotation, PropertyMetadata $propertyMetadata, string $originResourceClass): PropertyMetadata { $type = $propertyMetadata->getType(); $isCollection = $type->isCollection(); $resourceClass = $isCollection ? $type->getCollectionValueType()->getClassName() : $type->getClassName(); $maxDepth = $annotation->maxDepth; + // @ApiSubresource is on the class identifier (/collection/{id}/subcollection/{subcollectionId}) + if (null === $resourceClass) { + $resourceClass = $originResourceClass; + $isCollection = false; + } return $propertyMetadata->withSubresource(new SubresourceMetadata($resourceClass, $isCollection, $maxDepth)); } diff --git a/src/Operation/Factory/SubresourceOperationFactory.php b/src/Operation/Factory/SubresourceOperationFactory.php index b4ae89a0f13..bca23d2a474 100644 --- a/src/Operation/Factory/SubresourceOperationFactory.php +++ b/src/Operation/Factory/SubresourceOperationFactory.php @@ -79,6 +79,12 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $subresource = $propertyMetadata->getSubresource(); $subresourceClass = $subresource->getResourceClass(); $subresourceMetadata = $this->resourceMetadataFactory->create($subresourceClass); + $isLastItem = $resourceClass === $parentOperation['resource_class'] && $propertyMetadata->isIdentifier(); + + // A subresource that is also an identifier can't be a start point + if ($isLastItem && (null === $parentOperation || false === $parentOperation['collection'])) { + continue; + } $visiting = "$resourceClass $property $subresourceClass"; @@ -135,10 +141,12 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre } else { $resourceMetadata = $this->resourceMetadataFactory->create($resourceClass); $operation['identifiers'] = $parentOperation['identifiers']; - $operation['identifiers'][] = [$parentOperation['property'], $resourceClass, $parentOperation['collection']]; - - $operation['operation_name'] = str_replace('get'.self::SUBRESOURCE_SUFFIX, RouteNameGenerator::inflector($property, $operation['collection']).'_get'.self::SUBRESOURCE_SUFFIX, $parentOperation['operation_name']); - + $operation['identifiers'][] = [$parentOperation['property'], $resourceClass, $isLastItem ? true : $parentOperation['collection']]; + $operation['operation_name'] = str_replace( + 'get'.self::SUBRESOURCE_SUFFIX, + RouteNameGenerator::inflector($isLastItem ? 'item' : $property, $operation['collection']).'_get'.self::SUBRESOURCE_SUFFIX, + $parentOperation['operation_name'] + ); $operation['route_name'] = str_replace($parentOperation['operation_name'], $operation['operation_name'], $parentOperation['route_name']); if (!\in_array($resourceMetadata->getShortName(), $operation['shortNames'], true)) { @@ -151,11 +159,17 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $operation['path'] = $subresourceOperation['path']; } else { $operation['path'] = str_replace(self::FORMAT_SUFFIX, '', $parentOperation['path']); + if ($parentOperation['collection']) { list($key) = end($operation['identifiers']); $operation['path'] .= sprintf('/{%s}', $key); } - $operation['path'] .= sprintf('/%s%s', $this->pathSegmentNameGenerator->getSegmentName($property, $operation['collection']), self::FORMAT_SUFFIX); + + if ($isLastItem) { + $operation['path'] .= self::FORMAT_SUFFIX; + } else { + $operation['path'] .= sprintf('/%s%s', $this->pathSegmentNameGenerator->getSegmentName($property, $operation['collection']), self::FORMAT_SUFFIX); + } } } diff --git a/tests/Bridge/Doctrine/Orm/SubresourceDataProviderTest.php b/tests/Bridge/Doctrine/Orm/SubresourceDataProviderTest.php index 2032205abb2..161ee9422e2 100644 --- a/tests/Bridge/Doctrine/Orm/SubresourceDataProviderTest.php +++ b/tests/Bridge/Doctrine/Orm/SubresourceDataProviderTest.php @@ -143,6 +143,7 @@ public function testGetSubresource() $classMetadataProphecy = $this->prophesize(ClassMetadata::class); $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); $classMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $classMetadataProphecy->hasAssociation('relatedDummies')->willReturn(true)->shouldBeCalled(); $classMetadataProphecy->getAssociationMapping('relatedDummies')->shouldBeCalled()->willReturn(['type' => ClassMetadata::MANY_TO_MANY]); $managerProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); @@ -202,6 +203,7 @@ public function testGetSubSubresourceItem() $classMetadataProphecy = $this->prophesize(ClassMetadata::class); $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); $classMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $classMetadataProphecy->hasAssociation('relatedDummies')->willReturn(true)->shouldBeCalled(); $classMetadataProphecy->getAssociationMapping('relatedDummies')->shouldBeCalled()->willReturn(['type' => ClassMetadata::MANY_TO_MANY]); $dummyManagerProphecy = $this->prophesize(EntityManager::class); @@ -229,6 +231,7 @@ public function testGetSubSubresourceItem() $rClassMetadataProphecy = $this->prophesize(ClassMetadata::class); $rClassMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); $rClassMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $rClassMetadataProphecy->hasAssociation('thirdLevel')->shouldBeCalled()->willReturn(true); $rClassMetadataProphecy->getAssociationMapping('thirdLevel')->shouldBeCalled()->willReturn(['type' => ClassMetadata::MANY_TO_ONE]); $rDummyManagerProphecy = $this->prophesize(EntityManager::class); @@ -290,6 +293,7 @@ public function testQueryResultExtension() $classMetadataProphecy = $this->prophesize(ClassMetadata::class); $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); $classMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $classMetadataProphecy->hasAssociation('relatedDummies')->willReturn(true)->shouldBeCalled(); $classMetadataProphecy->getAssociationMapping('relatedDummies')->shouldBeCalled()->willReturn(['type' => ClassMetadata::MANY_TO_MANY]); $managerProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); @@ -363,4 +367,96 @@ public function testThrowResourceClassNotSupportedException() $dataProvider = new SubresourceDataProvider($managerRegistryProphecy->reveal(), $propertyNameCollectionFactory, $propertyMetadataFactory); $dataProvider->getSubresource(Dummy::class, ['id' => 1], []); } + + public function testGetSubresourceCollectionItem() + { + $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class); + $identifiers = ['id']; + $funcProphecy = $this->prophesize(Func::class); + $func = $funcProphecy->reveal(); + + // First manager (Dummy) + $dummyDQL = 'dql'; + + $qb = $this->prophesize(QueryBuilder::class); + $qb->select('relatedDummies_a3')->shouldBeCalled()->willReturn($qb); + $qb->from(Dummy::class, 'id_a2')->shouldBeCalled()->willReturn($qb); + $qb->innerJoin('id_a2.relatedDummies', 'relatedDummies_a3')->shouldBeCalled()->willReturn($qb); + $qb->andWhere('id_a2.id = :id_p2')->shouldBeCalled()->willReturn($qb); + + $dummyFunc = new Func('in', ['any']); + + $dummyExpProphecy = $this->prophesize(Expr::class); + $dummyExpProphecy->in('relatedDummies_a1', $dummyDQL)->willReturn($dummyFunc)->shouldBeCalled(); + + $qb->expr()->shouldBeCalled()->willReturn($dummyExpProphecy->reveal()); + + $qb->getDQL()->shouldBeCalled()->willReturn($dummyDQL); + + $classMetadataProphecy = $this->prophesize(ClassMetadata::class); + $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); + $classMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $classMetadataProphecy->hasAssociation('relatedDummies')->willReturn(true)->shouldBeCalled(); + $classMetadataProphecy->getAssociationMapping('relatedDummies')->shouldBeCalled()->willReturn(['type' => ClassMetadata::MANY_TO_MANY]); + + $dummyManagerProphecy = $this->prophesize(EntityManager::class); + $dummyManagerProphecy->createQueryBuilder()->shouldBeCalled()->willReturn($qb->reveal()); + $dummyManagerProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); + $this->assertIdentifierManagerMethodCalls($dummyManagerProphecy); + + $managerRegistryProphecy->getManagerForClass(Dummy::class)->shouldBeCalled()->willReturn($dummyManagerProphecy->reveal()); + + // Second manager (RelatedDummy) + $relatedDQL = 'relateddql'; + + $rqb = $this->prophesize(QueryBuilder::class); + $rqb->select('relatedDummies_a1')->shouldBeCalled()->willReturn($rqb); + $rqb->from(RelatedDummy::class, 'relatedDummies_a1')->shouldBeCalled()->willReturn($rqb); + $rqb->andWhere('relatedDummies_a1.id = :id_p1')->shouldBeCalled()->willReturn($rqb); + $rqb->andWhere($dummyFunc)->shouldBeCalled()->willReturn($rqb); + $rqb->getDQL()->shouldBeCalled()->willReturn($relatedDQL); + + $relatedExpProphecy = $this->prophesize(Expr::class); + $relatedExpProphecy->in('o', $relatedDQL)->willReturn($func)->shouldBeCalled(); + + $rqb->expr()->shouldBeCalled()->willReturn($relatedExpProphecy->reveal()); + + $rClassMetadataProphecy = $this->prophesize(ClassMetadata::class); + $rClassMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn($identifiers); + $rClassMetadataProphecy->getTypeOfField('id')->shouldBeCalled()->willReturn('integer'); + $rClassMetadataProphecy->hasAssociation('id')->shouldBeCalled()->willReturn(false); + $rClassMetadataProphecy->isIdentifier('id')->shouldBeCalled()->willReturn(true); + + $rDummyManagerProphecy = $this->prophesize(EntityManager::class); + $rDummyManagerProphecy->createQueryBuilder()->shouldBeCalled()->willReturn($rqb->reveal()); + $rDummyManagerProphecy->getClassMetadata(RelatedDummy::class)->shouldBeCalled()->willReturn($rClassMetadataProphecy->reveal()); + $this->assertIdentifierManagerMethodCalls($rDummyManagerProphecy); + + $managerRegistryProphecy->getManagerForClass(RelatedDummy::class)->shouldBeCalled()->willReturn($rDummyManagerProphecy->reveal()); + + $result = new \StdClass(); + $queryProphecy = $this->prophesize(AbstractQuery::class); + $queryProphecy->getOneOrNullResult()->shouldBeCalled()->willReturn($result); + + $queryBuilder = $this->prophesize(QueryBuilder::class); + + $queryBuilder->andWhere($func)->shouldBeCalled()->willReturn($queryBuilder); + + $queryBuilder->getQuery()->shouldBeCalled()->willReturn($queryProphecy->reveal()); + $queryBuilder->setParameter('id_p1', 2)->shouldBeCalled()->willReturn($queryBuilder); + $queryBuilder->setParameter('id_p2', 1)->shouldBeCalled()->willReturn($queryBuilder); + + $repositoryProphecy = $this->prophesize(EntityRepository::class); + $repositoryProphecy->createQueryBuilder('o')->shouldBeCalled()->willReturn($queryBuilder->reveal()); + + $rDummyManagerProphecy->getRepository(RelatedDummy::class)->shouldBeCalled()->willReturn($repositoryProphecy->reveal()); + + list($propertyNameCollectionFactory, $propertyMetadataFactory) = $this->getMetadataProphecies([Dummy::class => $identifiers, RelatedDummy::class => $identifiers]); + + $dataProvider = new SubresourceDataProvider($managerRegistryProphecy->reveal(), $propertyNameCollectionFactory, $propertyMetadataFactory); + + $context = ['property' => 'id', 'identifiers' => [['id', Dummy::class, true], ['relatedDummies', RelatedDummy::class, true]], 'collection' => false]; + + $this->assertEquals($result, $dataProvider->getSubresource(RelatedDummy::class, ['id' => 1, 'relatedDummies' => 2], $context)); + } } diff --git a/tests/Fixtures/TestBundle/Entity/RelatedDummy.php b/tests/Fixtures/TestBundle/Entity/RelatedDummy.php index 5a1ac538ad7..24e3782ab78 100644 --- a/tests/Fixtures/TestBundle/Entity/RelatedDummy.php +++ b/tests/Fixtures/TestBundle/Entity/RelatedDummy.php @@ -31,6 +31,7 @@ class RelatedDummy extends ParentDummy { /** + * @ApiSubresource * @ORM\Column(type="integer") * @ORM\Id * @ORM\GeneratedValue(strategy="AUTO") diff --git a/tests/Operation/Factory/SubresourceOperationFactoryTest.php b/tests/Operation/Factory/SubresourceOperationFactoryTest.php index aeb8da8ab99..f6afeb75eb5 100644 --- a/tests/Operation/Factory/SubresourceOperationFactoryTest.php +++ b/tests/Operation/Factory/SubresourceOperationFactoryTest.php @@ -304,4 +304,107 @@ public function testCreateWithMaxDepth() ] + SubresourceOperationFactory::ROUTE_OPTIONS, ], $subresourceOperationFactory->create(DummyEntity::class)); } + + public function testCreateWithEnd() + { + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('relatedDummyEntity')); + $resourceMetadataFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('dummyEntity')); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['subresource'])); + $propertyNameCollectionFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['id'])); + + $subresourceMetadataCollection = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(RelatedDummyEntity::class, true)); + $identifierSubresourceMetadata = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(DummyEntity::class, false))->withIdentifier(true); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'subresource')->shouldBeCalled()->willReturn($subresourceMetadataCollection); + $propertyMetadataFactoryProphecy->create(RelatedDummyEntity::class, 'id')->shouldBeCalled()->willReturn($identifierSubresourceMetadata); + + $pathSegmentNameGeneratorProphecy = $this->prophesize(PathSegmentNameGeneratorInterface::class); + $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity', true)->shouldBeCalled()->willReturn('dummy_entities'); + $pathSegmentNameGeneratorProphecy->getSegmentName('subresource', true)->shouldBeCalled()->willReturn('subresource'); + + $subresourceOperationFactory = new SubresourceOperationFactory( + $resourceMetadataFactoryProphecy->reveal(), + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $pathSegmentNameGeneratorProphecy->reveal() + ); + + $result = $subresourceOperationFactory->create(DummyEntity::class); + $this->assertEquals([ + 'api_dummy_entities_subresources_get_subresource' => [ + 'property' => 'subresource', + 'collection' => true, + 'resource_class' => RelatedDummyEntity::class, + 'shortNames' => ['relatedDummyEntity', 'dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_subresources_get_subresource', + 'path' => '/dummy_entities/{id}/subresource.{_format}', + 'operation_name' => 'subresources_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + 'api_dummy_entities_subresources_item_get_subresource' => [ + 'property' => 'id', + 'collection' => false, + 'resource_class' => DummyEntity::class, + 'shortNames' => ['dummyEntity', 'relatedDummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ['subresource', RelatedDummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_subresources_item_get_subresource', + 'path' => '/dummy_entities/{id}/subresource/{subresource}.{_format}', + 'operation_name' => 'subresources_item_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + ], $result); + } + + public function testCreateWithEndButNoCollection() + { + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('relatedDummyEntity')); + $resourceMetadataFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('dummyEntity')); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['subresource'])); + $propertyNameCollectionFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['id'])); + + $subresourceMetadataCollection = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(RelatedDummyEntity::class, false)); + $identifierSubresourceMetadata = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(DummyEntity::class, false))->withIdentifier(true); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'subresource')->shouldBeCalled()->willReturn($subresourceMetadataCollection); + $propertyMetadataFactoryProphecy->create(RelatedDummyEntity::class, 'id')->shouldBeCalled()->willReturn($identifierSubresourceMetadata); + + $pathSegmentNameGeneratorProphecy = $this->prophesize(PathSegmentNameGeneratorInterface::class); + $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity', true)->shouldBeCalled()->willReturn('dummy_entities'); + $pathSegmentNameGeneratorProphecy->getSegmentName('subresource', false)->shouldBeCalled()->willReturn('subresource'); + + $subresourceOperationFactory = new SubresourceOperationFactory( + $resourceMetadataFactoryProphecy->reveal(), + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $pathSegmentNameGeneratorProphecy->reveal() + ); + + $result = $subresourceOperationFactory->create(DummyEntity::class); + $this->assertEquals([ + 'api_dummy_entities_subresource_get_subresource' => [ + 'property' => 'subresource', + 'collection' => false, + 'resource_class' => RelatedDummyEntity::class, + 'shortNames' => ['relatedDummyEntity', 'dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/subresource.{_format}', + 'operation_name' => 'subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + ], $result); + } } From af6a01889d7c0162f948df632949101edf62acf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Fri, 20 Apr 2018 10:53:03 +0200 Subject: [PATCH 10/24] Merge pull request #1875 from soyuka/subresource-item Enable item route on collection subresources From 81fcdd85540109ca78d3f9dabcc193beddadf41c Mon Sep 17 00:00:00 2001 From: abluchet Date: Mon, 23 Apr 2018 10:27:41 +0200 Subject: [PATCH 11/24] Revert 3465b41bd55f46faeab1aae9c80cd16a08d5e27e --- src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml | 1 - .../Bundle/DependencyInjection/ApiPlatformExtensionTest.php | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml index 777b81575db..82d59e76390 100644 --- a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml +++ b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml @@ -164,7 +164,6 @@ - diff --git a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index 52af3f90e8f..b5ad216ed93 100644 --- a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -38,7 +38,6 @@ use ApiPlatform\Core\Tests\Fixtures\TestBundle\TestBundle; use ApiPlatform\Core\Validator\ValidatorInterface; use Doctrine\Bundle\DoctrineBundle\DoctrineBundle; -use Doctrine\Common\Persistence\ManagerRegistry; use FOS\UserBundle\FOSUserBundle; use Nelmio\ApiDocBundle\NelmioApiDocBundle; use PHPUnit\Framework\TestCase; @@ -705,7 +704,6 @@ private function getBaseContainerBuilderProphecy() PaginationExtension::class => 'api_platform.doctrine.orm.query_extension.pagination', OrderExtension::class => 'api_platform.doctrine.orm.query_extension.order', ValidatorInterface::class => 'api_platform.validator', - ManagerRegistry::class => 'doctrine', ]; foreach ($aliases as $alias => $service) { From d917eaa6148ec00285f519e2ec83f9776a14bc0c Mon Sep 17 00:00:00 2001 From: Anto Date: Sat, 14 Apr 2018 22:53:17 +0200 Subject: [PATCH 12/24] The OrderFilter is still triggering bad deprecation/function calls --- src/Bridge/Doctrine/Orm/Filter/OrderFilter.php | 5 ++++- tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Bridge/Doctrine/Orm/Filter/OrderFilter.php b/src/Bridge/Doctrine/Orm/Filter/OrderFilter.php index 6f5af8744ba..39a83f83a6a 100644 --- a/src/Bridge/Doctrine/Orm/Filter/OrderFilter.php +++ b/src/Bridge/Doctrine/Orm/Filter/OrderFilter.php @@ -81,8 +81,11 @@ public function __construct(ManagerRegistry $managerRegistry, $requestStack = nu */ public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null, array $context = []) { + if (isset($context['filters']) && !isset($context['filters'][$this->orderParameterName])) { + return; + } + if (!isset($context['filters'][$this->orderParameterName]) || !\is_array($context['filters'][$this->orderParameterName])) { - $context['filters'] = null; parent::apply($queryBuilder, $queryNameGenerator, $resourceClass, $operationName, $context); return; diff --git a/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php index f4640799b17..42c031c857c 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php @@ -341,6 +341,18 @@ public function provideApplyTestData(): array null, $orderFilterFactory, ], + 'not having order should not throw a deprecation (select unchanged)' => [ + [ + 'id' => null, + 'name' => null, + ], + [ + 'name' => 'q', + ], + sprintf('SELECT o FROM %s o', Dummy::class), + null, + $orderFilterFactory, + ], ]; } } From 4bd2b82e3b06d0dadea83a3b68aa6ab75f34a30e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Fri, 20 Apr 2018 12:01:13 +0200 Subject: [PATCH 13/24] Merge pull request #1866 from antograssiot/order-filter The OrderFilter is still triggering bad deprecation/function calls From f92ea0d2950a6eed777337e293ef3dec6e46ddc8 Mon Sep 17 00:00:00 2001 From: Anto Date: Fri, 20 Apr 2018 11:53:45 +0200 Subject: [PATCH 14/24] Allow GraphQL to filter on nested property (#1868) * Allow GraphQL to filter on nested property fixes #1714, #1867 * Allow ordering on nested property values --- features/doctrine/order_filter.feature | 53 +++++++++++++++++++ features/graphql/filters.feature | 45 ++++++++++++++++ .../Factory/CollectionResolverFactory.php | 20 ++++++- src/GraphQl/Type/SchemaBuilder.php | 7 +++ 4 files changed, 124 insertions(+), 1 deletion(-) diff --git a/features/doctrine/order_filter.feature b/features/doctrine/order_filter.feature index 3629747ada1..99e3eeaff28 100644 --- a/features/doctrine/order_filter.feature +++ b/features/doctrine/order_filter.feature @@ -831,3 +831,56 @@ Feature: Order filter on collections } } """ + + @createSchema + @dropSchema + Scenario: Get collection ordered in descending order on a related property + Given there are 2 dummy objects with relatedDummy + When I send a "GET" request to "/dummies?order[relatedDummy.name]=desc" + 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/ld+json; charset=utf-8" + And the JSON should be valid according to this schema: + """ + { + "type": "object", + "properties": { + "@context": {"pattern": "^/contexts/Dummy$"}, + "@id": {"pattern": "^/dummies$"}, + "@type": {"pattern": "^hydra:Collection$"}, + "hydra:member": { + "type": "array", + "items": [ + { + "type": "object", + "properties": { + "@id": { + "type": "string", + "pattern": "^/dummies/2$" + } + } + }, + { + "type": "object", + "properties": { + "@id": { + "type": "string", + "pattern": "^/dummies/1$" + } + } + } + ], + "additionalItems": false, + "maxItems": 2, + "minItems": 2 + }, + "hydra:view": { + "type": "object", + "properties": { + "@id": {"pattern": "^/dummies\\?order%5BrelatedDummy.name%5D=desc"}, + "@type": {"pattern": "^hydra:PartialCollectionView$"} + } + } + } + } + """ diff --git a/features/graphql/filters.feature b/features/graphql/filters.feature index 011005cea5d..17607d76812 100644 --- a/features/graphql/filters.feature +++ b/features/graphql/filters.feature @@ -116,3 +116,48 @@ Feature: Collections filtering And the JSON node "data.dummies.edges[1].node.relatedDummies.edges" should have 0 elements And the JSON node "data.dummies.edges[2].node.relatedDummies.edges" should have 1 element And the JSON node "data.dummies.edges[2].node.relatedDummies.edges[0].node.name" should be equal to "RelatedDummy13" + + @createSchema + @dropSchema + Scenario: Retrieve a collection filtered using the related search filter + Given there are 1 dummy objects having each 2 relatedDummies + And there are 1 dummy objects having each 3 relatedDummies + When I send the following GraphQL request: + """ + { + dummies(relatedDummies_name: "RelatedDummy31") { + edges { + node { + id + } + } + } + } + """ + And the response status code should be 200 + And the JSON node "data.dummies.edges" should have 1 element + + @createSchema + @dropSchema + Scenario: Retrieve a collection ordered using nested properties + Given there are 2 dummy objects with relatedDummy + When I send the following GraphQL request: + """ + { + dummies(order: {relatedDummy_name: "DESC"}) { + edges { + node { + name + relatedDummy { + id + name + } + } + } + } + } + """ + 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 "Dummy #2" + And the JSON node "data.dummies.edges[1].node.name" should be equal to "Dummy #1" diff --git a/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php b/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php index 07ac49c4a0a..b8d93e9a640 100644 --- a/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php +++ b/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php @@ -73,7 +73,7 @@ public function __invoke(string $resourceClass = null, string $rootClass = null, $resourceMetadata = $this->resourceMetadataFactory->create($resourceClass); $dataProviderContext = $resourceMetadata->getGraphqlAttribute('query', 'normalization_context', [], true); $dataProviderContext['attributes'] = $this->fieldsToAttributes($info); - $dataProviderContext['filters'] = $args; + $dataProviderContext['filters'] = $this->getNormalizedFilters($args); if (isset($source[$rootProperty = $info->fieldName], $source[ItemNormalizer::ITEM_KEY])) { $rootResolvedFields = $this->identifiersExtractor->getIdentifiersFromItem(unserialize($source[ItemNormalizer::ITEM_KEY])); @@ -160,4 +160,22 @@ private function getSubresource(string $rootClass, array $rootResolvedFields, ar 'collection' => $isCollection, ]); } + + private function getNormalizedFilters(array $args): array + { + $filters = $args; + foreach ($filters as $name => $value) { + if (\is_array($value)) { + $filters[$name] = $this->getNormalizedFilters($value); + continue; + } + + if (strpos($name, '_')) { + // Gives a chance to relations/nested fields. + $filters[str_replace('_', '.', $name)] = $value; + } + } + + return $filters; + } } diff --git a/src/GraphQl/Type/SchemaBuilder.php b/src/GraphQl/Type/SchemaBuilder.php index f886740aa36..838e8c80743 100644 --- a/src/GraphQl/Type/SchemaBuilder.php +++ b/src/GraphQl/Type/SchemaBuilder.php @@ -290,6 +290,13 @@ private function mergeFilterArgs(array $args, array $parsed, ResourceMetadata $r private function convertFilterArgsToTypes(array $args): array { + foreach ($args as $key => $value) { + if (strpos($key, '.')) { + // Declare relations/nested fields in a GraphQL compatible syntax. + $args[str_replace('.', '_', $key)] = $value; + } + } + foreach ($args as $key => $value) { if (!\is_array($value) || !isset($value['#name'])) { continue; From 753a36e2a64d91559363bab43bdf6e0dfa4785a0 Mon Sep 17 00:00:00 2001 From: Alan Poulain Date: Mon, 23 Apr 2018 15:51:23 +0200 Subject: [PATCH 15/24] Remove useless double check of access_control in resolvers --- .../Resolver/Factory/CollectionResolverFactory.php | 7 ------- src/GraphQl/Resolver/ItemResolver.php | 8 -------- 2 files changed, 15 deletions(-) diff --git a/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php b/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php index b8d93e9a640..646c053c094 100644 --- a/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php +++ b/src/GraphQl/Resolver/Factory/CollectionResolverFactory.php @@ -85,13 +85,6 @@ public function __invoke(string $resourceClass = null, string $rootClass = null, $this->canAccess($this->resourceAccessChecker, $resourceMetadata, $resourceClass, $info, $collection, 'query'); - if (null !== $this->resourceAccessChecker) { - $isGranted = $resourceMetadata->getGraphqlAttribute('query', 'access_control', null, true); - if (null !== $isGranted && !$this->resourceAccessChecker->isGranted($resourceClass, $isGranted, ['object' => $collection])) { - throw Error::createLocatedError('Access Denied.', $info->fieldNodes, $info->path); - } - } - if (!$this->paginationEnabled) { $data = []; foreach ($collection as $index => $object) { diff --git a/src/GraphQl/Resolver/ItemResolver.php b/src/GraphQl/Resolver/ItemResolver.php index 09a03a1a14e..11c0dc74acb 100644 --- a/src/GraphQl/Resolver/ItemResolver.php +++ b/src/GraphQl/Resolver/ItemResolver.php @@ -19,7 +19,6 @@ use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; use ApiPlatform\Core\Security\ResourceAccessCheckerInterface; use ApiPlatform\Core\Util\ClassInfoTrait; -use GraphQL\Error\Error; use GraphQL\Type\Definition\ResolveInfo; use Symfony\Component\Serializer\Normalizer\NormalizerInterface; @@ -71,13 +70,6 @@ public function __invoke($source, $args, $context, ResolveInfo $info) $resourceMetadata = $this->resourceMetadataFactory->create($resourceClass); $this->canAccess($this->resourceAccessChecker, $resourceMetadata, $resourceClass, $info, $item, 'query'); - if (null !== $this->resourceAccessChecker) { - $isGranted = $resourceMetadata->getGraphqlAttribute('query', 'access_control', null, true); - if (null !== $isGranted && !$this->resourceAccessChecker->isGranted($resourceClass, $isGranted, ['object' => $item])) { - throw Error::createLocatedError('Access Denied.', $info->fieldNodes, $info->path); - } - } - $normalizationContext = $resourceMetadata->getGraphqlAttribute('query', 'normalization_context', [], true); return $this->normalizer->normalize($item, ItemNormalizer::FORMAT, $normalizationContext + $baseNormalizationContext); From f522a6f1361cfe7a6488a28ddec212c53fd6ce67 Mon Sep 17 00:00:00 2001 From: Teoh Han Hui Date: Mon, 23 Apr 2018 17:51:09 +0200 Subject: [PATCH 16/24] Move coverage jobs to CircleCI --- .circleci/config.yml | 191 ++++++++++++++++++ .editorconfig | 4 + .travis.yml | 48 +---- phpunit.xml.dist | 2 +- .../Test/DoctrineOrmFilterTestCase.php | 4 +- .../Doctrine/Orm/Filter/BooleanFilterTest.php | 3 +- .../Doctrine/Orm/Filter/DateFilterTest.php | 3 +- .../Doctrine/Orm/Filter/ExistsFilterTest.php | 3 +- .../Doctrine/Orm/Filter/NumericFilterTest.php | 3 +- .../Doctrine/Orm/Filter/OrderFilterTest.php | 3 +- .../Doctrine/Orm/Filter/RangeFilterTest.php | 3 +- .../Doctrine/Orm/Filter/SearchFilterTest.php | 3 +- .../app/config/config_test_phpunit.yml | 2 - 13 files changed, 223 insertions(+), 49 deletions(-) create mode 100644 .circleci/config.yml rename tests/Bridge/Doctrine/Orm/Filter/AbstractFilterTest.php => src/Test/DoctrineOrmFilterTestCase.php (97%) delete mode 100644 tests/Fixtures/app/config/config_test_phpunit.yml diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 00000000000..513d7342876 --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,191 @@ +version: 2 + +reusable-steps: + - &clear-test-app-cache + run: + name: Clear test app cache + command: tests/Fixtures/app/console cache:clear + - &disable-php-memory-limit + run: + name: Disable PHP memory limit + command: echo 'memory_limit=-1' | sudo tee -a /usr/local/etc/php/php.ini + - &disable-xdebug-php-extension + run: + name: Disable Xdebug PHP extension + command: sudo rm /usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini + - &restore-composer-cache + restore_cache: + keys: + - composer-cache-{{ .Revision }} + - composer-cache-{{ .Branch }} + - composer-cache + - &restore-npm-cache + restore_cache: + keys: + - npm-cache-{{ .Revision }} + - npm-cache-{{ .Branch }} + - npm-cache + - &save-composer-cache-by-branch + save_cache: + paths: + - ~/.composer/cache + key: composer-cache-{{ .Branch }}-{{ .BuildNum }} + - &save-composer-cache-by-revision + save_cache: + paths: + - ~/.composer/cache + key: composer-cache-{{ .Revision }}-{{ .BuildNum }} + - &save-npm-cache-by-branch + save_cache: + paths: + - ~/.npm + key: npm-cache-{{ .Branch }}-{{ .BuildNum }} + - &save-npm-cache-by-revision + save_cache: + paths: + - ~/.npm + key: npm-cache-{{ .Revision }}-{{ .BuildNum }} + - &update-composer + run: + name: Update Composer + command: composer self-update + - &update-project-dependencies + run: + name: Update project dependencies + command: composer update --prefer-dist --no-progress --no-suggest --ansi + +jobs: + phpunit-php-7.2-coverage: + docker: + - image: circleci/php:7.2-node-browsers + environment: + SYMFONY_DEPRECATIONS_HELPER: weak_vendors + APP_ENV: test + parallelism: 2 + working_directory: ~/api-platform/core + steps: + - checkout + - *restore-composer-cache + - *restore-npm-cache + - *disable-xdebug-php-extension + - *disable-php-memory-limit + - *update-composer + - *update-project-dependencies + - *save-composer-cache-by-revision + - *save-composer-cache-by-branch + - *clear-test-app-cache + - run: + name: Run PHPUnit tests + command: |- + mkdir -p build/logs/tmp build/cov + find tests -name '*Test.php' | circleci tests split --split-by=timings | parallel -j10% --rpl '{_} s/\//_/g;' \ + phpdbg -qrr vendor/bin/phpunit --coverage-php build/cov/coverage-{_}.cov --log-junit build/logs/tmp/{_}.xml --colors=always {} + - run: + name: Merge PHPUnit test reports + command: |- + mkdir -p build/logs/phpunit + npx junit-merge --out build/logs/phpunit/junit.xml --dir build/logs/tmp + rm -r build/logs/tmp + - store_test_results: + path: build/logs + - store_artifacts: + path: build/logs/phpunit/junit.xml + destination: build/logs/phpunit/junit.xml + - persist_to_workspace: + root: build + paths: + - cov + - *save-npm-cache-by-revision + - *save-npm-cache-by-branch + + behat-php-7.2-coverage: + docker: + - image: circleci/php:7.2-node-browsers + environment: + SYMFONY_DEPRECATIONS_HELPER: weak_vendors + APP_ENV: test + parallelism: 2 + working_directory: ~/api-platform/core + steps: + - checkout + - *restore-composer-cache + - *restore-npm-cache + - *disable-xdebug-php-extension + - *disable-php-memory-limit + - *update-composer + - *update-project-dependencies + - *save-composer-cache-by-revision + - *save-composer-cache-by-branch + - *clear-test-app-cache + - run: + name: Run Behat tests + command: |- + mkdir -p build/logs/tmp build/cov + for f in $(find features -name '*.feature' -not -path 'features/main/exposed_state.feature' | circleci tests split --split-by=timings); do + _f=${f//\//_} + FEATURE="${_f}" phpdbg -qrr vendor/bin/behat --profile=coverage --suite=default --tags=~@postgres --format=progress --out=std --format=junit --out=build/logs/tmp/"${_f}" "$f" + done + - run: + name: Merge Behat test reports + command: |- + mkdir -p build/logs/behat + npx junit-merge --out build/logs/behat/junit.xml --dir build/logs/tmp --recursive + rm -r build/logs/tmp + - store_test_results: + path: build/logs + - store_artifacts: + path: build/logs/behat/junit.xml + destination: build/logs/behat/junit.xml + - persist_to_workspace: + root: build + paths: + - cov + - *save-npm-cache-by-revision + - *save-npm-cache-by-branch + + merge-and-upload-coverage: + docker: + - image: circleci/php:7.2-node-browsers + working_directory: ~/api-platform/core + steps: + - checkout + - *restore-npm-cache + - *disable-xdebug-php-extension + - *disable-php-memory-limit + - run: + name: Download phpcov + command: wget https://phar.phpunit.de/phpcov.phar + - attach_workspace: + at: build + - run: + name: Merge code coverage reports + command: |- + mkdir -p build/logs + phpdbg -qrr phpcov.phar merge --clover build/logs/clover.xml build/cov + - store_artifacts: + path: build/logs/clover.xml + destination: build/logs/clover.xml + - run: + name: Upload code coverage report to Coveralls + command: |- + if [ ! -z "$COVERALLS_REPO_TOKEN" ]; then + npx @cedx/coveralls build/logs/clover.xml + else + echo 'Skipped' + fi + - run: + name: Upload code coverage report to Codecov + command: npx codecov --file=build/logs/clover.xml --disable=gcov + - *save-npm-cache-by-revision + - *save-npm-cache-by-branch + +workflows: + version: 2 + test-with-coverage: + jobs: + - phpunit-php-7.2-coverage + - behat-php-7.2-coverage + - merge-and-upload-coverage: + requires: + - phpunit-php-7.2-coverage + - behat-php-7.2-coverage diff --git a/.editorconfig b/.editorconfig index 8f16257d8c8..b9f9286b469 100644 --- a/.editorconfig +++ b/.editorconfig @@ -39,6 +39,10 @@ indent_style = space indent_size = 4 trim_trailing_whitespace = false +[.circleci/config.yml] +indent_style = space +indent_size = 2 + [.gitmodules] indent_style = tab diff --git a/.travis.yml b/.travis.yml index 3f8b74d4153..23871400fbb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,4 @@ language: php - sudo: false cache: @@ -17,8 +16,6 @@ matrix: - php: '7.0' - php: '7.1' - php: '7.2' - - php: '7.2' - env: coverage=1 - php: '7.2' env: lint=1 - php: '7.2' @@ -43,18 +40,6 @@ matrix: before_install: - phpenv config-rm xdebug.ini || echo "xdebug not available" - echo "memory_limit=-1" >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini - - if [[ $coverage != 1 && $lint != 1 ]]; then - npm install -g swagger-cli; - fi - - if [[ $coverage = 1 ]]; then - mkdir -p build/logs build/cov; - fi - - if [[ $coverage = 1 ]]; then - wget https://phar.phpunit.de/phpcov.phar; - fi - - if [[ $coverage = 1 ]]; then - wget https://github.com/satooshi/php-coveralls/releases/download/v1.0.1/coveralls.phar; - fi - if [[ $lint = 1 ]]; then wget https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/download/v2.8.4/php-cs-fixer.phar; fi @@ -64,9 +49,6 @@ before_install: - export PATH="$PATH:$HOME/.composer/vendor/bin" install: - - if [[ $coverage = 1 ]]; then - composer require --dev --no-update 'phpunit/php-code-coverage:^5.2.2'; - fi - if [[ $deps = 'low' ]]; then composer update --prefer-dist --no-progress --no-suggest --prefer-stable --prefer-lowest --ansi; else @@ -74,25 +56,22 @@ install: fi script: - - if [[ $coverage = 1 ]]; then - APP_ENV=test_phpunit phpdbg -qrr vendor/bin/phpunit --coverage-php build/cov/coverage-phpunit.cov; - elif [[ $lint != 1 ]]; then - APP_ENV=test_phpunit vendor/bin/phpunit; + - if [[ $lint != 1 ]]; then + tests/Fixtures/app/console cache:clear; + fi + - if [[ $lint != 1 ]]; then + vendor/bin/phpunit; + fi + - if [[ $lint != 1 ]]; then + tests/Fixtures/app/console cache:clear; fi - - if [[ $coverage = 1 ]]; then - for f in $(find features -name '*.feature' -not -path 'features/main/exposed_state.feature'); do - FEATURE=${f//\//_} phpdbg -qrr vendor/bin/behat --profile=coverage --suite=default --tags=~@postgress --format=progress $f || exit $?; - done; - elif [[ $APP_ENV = 'postgres' ]]; then + - if [[ $APP_ENV = 'postgres' ]]; then vendor/bin/behat --suite=postgres --format=progress; elif [[ $lint != 1 ]]; then vendor/bin/behat --suite=default --format=progress; fi - - if [[ $coverage = 1 ]]; then - phpdbg -qrr phpcov.phar merge --clover build/logs/clover.xml build/cov; - fi - - if [[ $coverage != 1 && $lint != 1 ]]; then - tests/Fixtures/app/console api:swagger:export > swagger.json && swagger-cli validate swagger.json && rm swagger.json; + - if [[ $lint != 1 ]]; then + tests/Fixtures/app/console api:swagger:export > swagger.json && npx swagger-cli validate swagger.json && rm swagger.json; fi - if [[ $lint = 1 ]]; then php php-cs-fixer.phar fix --dry-run --diff --no-ansi; @@ -100,8 +79,3 @@ script: - if [[ $lint = 1 ]]; then phpstan analyse -c phpstan.neon -l5 --ansi src tests; fi - -after_success: - - if [[ $coverage = 1 ]]; then - travis_retry php coveralls.phar; - fi diff --git a/phpunit.xml.dist b/phpunit.xml.dist index e9ebaccbed0..ceed9ab000d 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -12,7 +12,7 @@ - + diff --git a/tests/Bridge/Doctrine/Orm/Filter/AbstractFilterTest.php b/src/Test/DoctrineOrmFilterTestCase.php similarity index 97% rename from tests/Bridge/Doctrine/Orm/Filter/AbstractFilterTest.php rename to src/Test/DoctrineOrmFilterTestCase.php index eb1892ea3ca..1897414642d 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/AbstractFilterTest.php +++ b/src/Test/DoctrineOrmFilterTestCase.php @@ -11,7 +11,7 @@ declare(strict_types=1); -namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; +namespace ApiPlatform\Core\Test; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\FilterInterface; use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator; @@ -26,7 +26,7 @@ /** * @author Kévin Dunglas */ -abstract class AbstractFilterTest extends KernelTestCase +abstract class DoctrineOrmFilterTestCase extends KernelTestCase { /** * @var ManagerRegistry diff --git a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php index 17c73c682a5..27141419db7 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php @@ -14,12 +14,13 @@ namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\BooleanFilter; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; /** * @author Amrouche Hamza */ -class BooleanFilterTest extends AbstractFilterTest +class BooleanFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = BooleanFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/DateFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/DateFilterTest.php index ebf9a6bf837..b0269a90d50 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/DateFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/DateFilterTest.php @@ -15,6 +15,7 @@ use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\DateFilter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyDate; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyImmutableDate; @@ -25,7 +26,7 @@ * @author Théo FIDRY * @author Vincent CHALAMON */ -class DateFilterTest extends AbstractFilterTest +class DateFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = DateFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php index 0dee35c7712..820317b381d 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php @@ -14,12 +14,13 @@ namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\ExistsFilter; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; /** * @author Antoine Bluchet */ -class ExistsFilterTest extends AbstractFilterTest +class ExistsFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = ExistsFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/NumericFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/NumericFilterTest.php index 42e0eedddfd..270240a52c9 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/NumericFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/NumericFilterTest.php @@ -14,12 +14,13 @@ namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\NumericFilter; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; /** * @author Amrouche Hamza */ -class NumericFilterTest extends AbstractFilterTest +class NumericFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = NumericFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php index 42c031c857c..3079dd3e95f 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/OrderFilterTest.php @@ -14,6 +14,7 @@ namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\OrderFilter; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; use Doctrine\Common\Persistence\ManagerRegistry; use Symfony\Component\HttpFoundation\RequestStack; @@ -22,7 +23,7 @@ * @author Théo FIDRY * @author Vincent CHALAMON */ -class OrderFilterTest extends AbstractFilterTest +class OrderFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = OrderFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/RangeFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/RangeFilterTest.php index 8757714680b..06af0fb9571 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/RangeFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/RangeFilterTest.php @@ -14,12 +14,13 @@ namespace ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\RangeFilter; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; /** * @author Lee Siong Chan */ -class RangeFilterTest extends AbstractFilterTest +class RangeFilterTest extends DoctrineOrmFilterTestCase { protected $filterClass = RangeFilter::class; diff --git a/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php index 30dc3e5c857..85c14d15bf6 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php @@ -17,6 +17,7 @@ use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\SearchFilter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator; use ApiPlatform\Core\Exception\InvalidArgumentException; +use ApiPlatform\Core\Test\DoctrineOrmFilterTestCase; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy; use Doctrine\Common\Persistence\ManagerRegistry; @@ -28,7 +29,7 @@ * @author Julien Deniau * @author Vincent CHALAMON */ -class SearchFilterTest extends AbstractFilterTest +class SearchFilterTest extends DoctrineOrmFilterTestCase { protected $alias = 'oo'; protected $filterClass = SearchFilter::class; diff --git a/tests/Fixtures/app/config/config_test_phpunit.yml b/tests/Fixtures/app/config/config_test_phpunit.yml deleted file mode 100644 index 3cc6d1aa082..00000000000 --- a/tests/Fixtures/app/config/config_test_phpunit.yml +++ /dev/null @@ -1,2 +0,0 @@ -imports: - - { resource: config_test.yml } From 40dffef23bab5e494d9487d21b4ae97cc6036540 Mon Sep 17 00:00:00 2001 From: Ben Davies Date: Thu, 26 Apr 2018 00:08:51 +0100 Subject: [PATCH 17/24] add some local caches where they are missing --- .../Routing/CachedRouteNameResolver.php | 19 +++--- .../CachedSubresourceOperationFactory.php | 23 +++++-- .../Routing/CachedRouteNameResolverTest.php | 66 ++++++++++++------- .../CachedSubresourceOperationFactoryTest.php | 35 +++++----- 4 files changed, 88 insertions(+), 55 deletions(-) diff --git a/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php b/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php index 095d27ed06e..548787f1ae3 100644 --- a/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php +++ b/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php @@ -27,6 +27,7 @@ final class CachedRouteNameResolver implements RouteNameResolverInterface private $cacheItemPool; private $decorated; + private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, RouteNameResolverInterface $decorated) { @@ -42,26 +43,24 @@ public function getRouteName(string $resourceClass, $operationType /**, array $c $context = \func_num_args() > 2 ? func_get_arg(2) : []; $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass, $operationType, $context['subresource_resources'] ?? null])); + if (isset($this->localCache[$cacheKey])) { + return $this->localCache[$cacheKey]; + } + try { $cacheItem = $this->cacheItemPool->getItem($cacheKey); if ($cacheItem->isHit()) { - return $cacheItem->get(); + return $this->localCache[$cacheKey] = $cacheItem->get(); } } catch (CacheException $e) { - // do nothing - } - - if (\func_num_args() > 2) { - $context = func_get_arg(2); - } else { - $context = []; + //do nothing } $routeName = $this->decorated->getRouteName($resourceClass, $operationType, $context); if (!isset($cacheItem)) { - return $routeName; + return $this->localCache[$cacheKey] = $routeName; } try { @@ -71,6 +70,6 @@ public function getRouteName(string $resourceClass, $operationType /**, array $c // do nothing } - return $routeName; + return $this->localCache[$cacheKey] = $routeName; } } diff --git a/src/Operation/Factory/CachedSubresourceOperationFactory.php b/src/Operation/Factory/CachedSubresourceOperationFactory.php index a2cdd353743..c9dc94aebe4 100644 --- a/src/Operation/Factory/CachedSubresourceOperationFactory.php +++ b/src/Operation/Factory/CachedSubresourceOperationFactory.php @@ -25,6 +25,7 @@ final class CachedSubresourceOperationFactory implements SubresourceOperationFac private $cacheItemPool; private $decorated; + private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, SubresourceOperationFactoryInterface $decorated) { @@ -39,21 +40,33 @@ public function create(string $resourceClass): array { $cacheKey = self::CACHE_KEY_PREFIX.md5($resourceClass); + if (isset($this->localCache[$cacheKey])) { + return $this->localCache[$cacheKey]; + } + try { $cacheItem = $this->cacheItemPool->getItem($cacheKey); if ($cacheItem->isHit()) { - return $cacheItem->get(); + return $this->localCache[$cacheKey] = $cacheItem->get(); } } catch (CacheException $e) { - return $this->decorated->create($resourceClass); + // do nothing } $subresourceOperations = $this->decorated->create($resourceClass); - $cacheItem->set($subresourceOperations); - $this->cacheItemPool->save($cacheItem); + if (!isset($cacheItem)) { + return $this->localCache[$cacheKey] = $subresourceOperations; + } + + try { + $cacheItem->set($subresourceOperations); + $this->cacheItemPool->save($cacheItem); + } catch (CacheException $e) { + // do nothing + } - return $subresourceOperations; + return $this->localCache[$cacheKey] = $subresourceOperations; } } diff --git a/tests/Bridge/Symfony/Routing/CachedRouteNameResolverTest.php b/tests/Bridge/Symfony/Routing/CachedRouteNameResolverTest.php index 988c6c122e4..573ebfd479a 100644 --- a/tests/Bridge/Symfony/Routing/CachedRouteNameResolverTest.php +++ b/tests/Bridge/Symfony/Routing/CachedRouteNameResolverTest.php @@ -19,6 +19,7 @@ use ApiPlatform\Core\Exception\InvalidArgumentException; use PHPUnit\Framework\TestCase; use Prophecy\Argument; +use Psr\Cache\CacheException; use Psr\Cache\CacheItemInterface; use Psr\Cache\CacheItemPoolInterface; @@ -63,39 +64,39 @@ public function testGetRouteNameForItemRouteWithNoMatchingRoute() public function testGetRouteNameForItemRouteOnCacheMiss() { $cacheItemProphecy = $this->prophesize(CacheItemInterface::class); - $cacheItemProphecy->isHit()->willReturn(false)->shouldBeCalled(); - $cacheItemProphecy->set('some_item_route')->shouldBeCalled(); + $cacheItemProphecy->isHit()->willReturn(false)->shouldBeCalledTimes(1); + $cacheItemProphecy->set('some_item_route')->shouldBeCalledTimes(1); $cacheItemPoolProphecy = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPoolProphecy->getItem(Argument::type('string'))->willReturn($cacheItemProphecy); - $cacheItemPoolProphecy->save($cacheItemProphecy)->willReturn(true)->shouldBeCalled(); + $cacheItemPoolProphecy->getItem(Argument::type('string'))->shouldBeCalledTimes(1)->willReturn($cacheItemProphecy); + $cacheItemPoolProphecy->save($cacheItemProphecy)->shouldBeCalledTimes(1)->willReturn(true); $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); - $decoratedProphecy->getRouteName('AppBundle\Entity\User', false, [])->willReturn('some_item_route')->shouldBeCalled(); + $decoratedProphecy->getRouteName('AppBundle\Entity\User', false, [])->willReturn('some_item_route')->shouldBeCalledTimes(1); $cachedRouteNameResolver = new CachedRouteNameResolver($cacheItemPoolProphecy->reveal(), $decoratedProphecy->reveal()); - $actual = $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', false); - $this->assertSame('some_item_route', $actual); + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', false)); + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', false), 'Trigger the local cache'); } public function testGetRouteNameForItemRouteOnCacheHit() { $cacheItemProphecy = $this->prophesize(CacheItemInterface::class); - $cacheItemProphecy->isHit()->willReturn(true)->shouldBeCalled(); - $cacheItemProphecy->get()->willReturn('some_item_route')->shouldBeCalled(); + $cacheItemProphecy->isHit()->shouldBeCalledTimes(1)->willReturn(true); + $cacheItemProphecy->get()->shouldBeCalledTimes(1)->willReturn('some_item_route'); $cacheItemPoolProphecy = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPoolProphecy->getItem(Argument::type('string'))->willReturn($cacheItemProphecy); + $cacheItemPoolProphecy->getItem(Argument::type('string'))->shouldBeCalledTimes(1)->willReturn($cacheItemProphecy); $cacheItemPoolProphecy->save($cacheItemProphecy)->shouldNotBeCalled(); $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); $decoratedProphecy->getRouteName(Argument::cetera())->shouldNotBeCalled(); $cachedRouteNameResolver = new CachedRouteNameResolver($cacheItemPoolProphecy->reveal(), $decoratedProphecy->reveal()); - $actual = $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::ITEM); - $this->assertSame('some_item_route', $actual); + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::ITEM)); + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::ITEM), 'Trigger the local cache'); } /** @@ -123,38 +124,55 @@ public function testGetRouteNameForCollectionRouteWithNoMatchingRoute() public function testGetRouteNameForCollectionRouteOnCacheMiss() { $cacheItemProphecy = $this->prophesize(CacheItemInterface::class); - $cacheItemProphecy->isHit()->willReturn(false)->shouldBeCalled(); - $cacheItemProphecy->set('some_collection_route')->shouldBeCalled(); + $cacheItemProphecy->isHit()->shouldBeCalledTimes(1)->willReturn(false); + $cacheItemProphecy->set('some_collection_route')->shouldBeCalledTimes(1); $cacheItemPoolProphecy = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPoolProphecy->getItem(Argument::type('string'))->willReturn($cacheItemProphecy); - $cacheItemPoolProphecy->save($cacheItemProphecy)->willReturn(true)->shouldBeCalled(); + $cacheItemPoolProphecy->getItem(Argument::type('string'))->shouldBeCalledTimes(1)->willReturn($cacheItemProphecy); + $cacheItemPoolProphecy->save($cacheItemProphecy)->shouldBeCalledTimes(1)->willReturn(true); $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); - $decoratedProphecy->getRouteName('AppBundle\Entity\User', true, [])->willReturn('some_collection_route')->shouldBeCalled(); + $decoratedProphecy->getRouteName('AppBundle\Entity\User', true, [])->willReturn('some_collection_route')->shouldBeCalledTimes(1); $cachedRouteNameResolver = new CachedRouteNameResolver($cacheItemPoolProphecy->reveal(), $decoratedProphecy->reveal()); - $actual = $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', true); - $this->assertSame('some_collection_route', $actual); + $this->assertSame('some_collection_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', true)); + $this->assertSame('some_collection_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', true), 'Trigger the local cache'); } public function testGetRouteNameForCollectionRouteOnCacheHit() { $cacheItemProphecy = $this->prophesize(CacheItemInterface::class); - $cacheItemProphecy->isHit()->willReturn(true)->shouldBeCalled(); - $cacheItemProphecy->get()->willReturn('some_collection_route')->shouldBeCalled(); + $cacheItemProphecy->isHit()->willReturn(true)->shouldBeCalledTimes(1); + $cacheItemProphecy->get()->willReturn('some_collection_route')->shouldBeCalledTimes(1); $cacheItemPoolProphecy = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPoolProphecy->getItem(Argument::type('string'))->willReturn($cacheItemProphecy); + $cacheItemPoolProphecy->getItem(Argument::type('string'))->shouldBeCalledTimes(1)->willReturn($cacheItemProphecy); $cacheItemPoolProphecy->save($cacheItemProphecy)->shouldNotBeCalled(); $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); $decoratedProphecy->getRouteName(Argument::cetera())->shouldNotBeCalled(); $cachedRouteNameResolver = new CachedRouteNameResolver($cacheItemPoolProphecy->reveal(), $decoratedProphecy->reveal()); - $actual = $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::COLLECTION); - $this->assertSame('some_collection_route', $actual); + $this->assertSame('some_collection_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::COLLECTION)); + $this->assertSame('some_collection_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::COLLECTION), 'Trigger the local cache'); + } + + public function testGetRouteNameWithCacheItemThrowsCacheException() + { + $cacheException = $this->prophesize(CacheException::class); + $cacheException->willExtend(\Exception::class); + + $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class); + $cacheItemPool->getItem(Argument::type('string'))->shouldBeCalledTimes(1)->willThrow($cacheException->reveal()); + + $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); + $decoratedProphecy->getRouteName('AppBundle\Entity\User', OperationType::ITEM, [])->willReturn('some_item_route')->shouldBeCalledTimes(1); + + $cachedRouteNameResolver = new CachedRouteNameResolver($cacheItemPool->reveal(), $decoratedProphecy->reveal()); + + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::ITEM)); + $this->assertSame('some_item_route', $cachedRouteNameResolver->getRouteName('AppBundle\Entity\User', OperationType::ITEM), 'Trigger the local cache'); } } diff --git a/tests/Operation/Factory/CachedSubresourceOperationFactoryTest.php b/tests/Operation/Factory/CachedSubresourceOperationFactoryTest.php index a17dd20529f..f7cf9ec6a50 100644 --- a/tests/Operation/Factory/CachedSubresourceOperationFactoryTest.php +++ b/tests/Operation/Factory/CachedSubresourceOperationFactoryTest.php @@ -29,38 +29,40 @@ class CachedSubresourceOperationFactoryTest extends TestCase public function testCreateWithItemHit() { $cacheItem = $this->prophesize(CacheItemInterface::class); - $cacheItem->isHit()->willReturn(true)->shouldBeCalled(); - $cacheItem->get()->willReturn(['foo' => 'bar'])->shouldBeCalled(); + $cacheItem->isHit()->willReturn(true)->shouldBeCalledTimes(1); + $cacheItem->get()->willReturn(['foo' => 'bar'])->shouldBeCalledTimes(1); $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPool->getItem($this->generateCacheKey())->willReturn($cacheItem->reveal())->shouldBeCalled(); + $cacheItemPool->getItem($this->generateCacheKey())->willReturn($cacheItem->reveal())->shouldBeCalledTimes(1); $decoratedSubresourceOperationFactory = $this->prophesize(SubresourceOperationFactoryInterface::class); $decoratedSubresourceOperationFactory->create()->shouldNotBeCalled(); $cachedSubresourceOperationFactory = new CachedSubresourceOperationFactory($cacheItemPool->reveal(), $decoratedSubresourceOperationFactory->reveal()); - $resultedSubresourceOperation = $cachedSubresourceOperationFactory->create(Dummy::class); - $this->assertEquals(['foo' => 'bar'], $resultedSubresourceOperation); + $expectedResult = ['foo' => 'bar']; + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class)); + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class), 'Trigger the local cache'); } public function testCreateWithItemNotHit() { $cacheItem = $this->prophesize(CacheItemInterface::class); - $cacheItem->isHit()->willReturn(false)->shouldBeCalled(); - $cacheItem->set(['foo' => 'bar'])->willReturn($cacheItem->reveal())->shouldBeCalled(); + $cacheItem->isHit()->willReturn(false)->shouldBeCalledTimes(1); + $cacheItem->set(['foo' => 'bar'])->willReturn($cacheItem->reveal())->shouldBeCalledTimes(1); $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPool->getItem($this->generateCacheKey())->willReturn($cacheItem->reveal())->shouldBeCalled(); - $cacheItemPool->save($cacheItem->reveal())->willReturn(true)->shouldBeCalled(); + $cacheItemPool->getItem($this->generateCacheKey())->willReturn($cacheItem->reveal())->shouldBeCalledTimes(1); + $cacheItemPool->save($cacheItem->reveal())->willReturn(true)->shouldBeCalledTimes(1); $decoratedSubresourceOperationFactory = $this->prophesize(SubresourceOperationFactoryInterface::class); - $decoratedSubresourceOperationFactory->create(Dummy::class)->shouldBeCalled()->willReturn(['foo' => 'bar']); + $decoratedSubresourceOperationFactory->create(Dummy::class)->shouldBeCalledTimes(1)->willReturn(['foo' => 'bar']); $cachedSubresourceOperationFactory = new CachedSubresourceOperationFactory($cacheItemPool->reveal(), $decoratedSubresourceOperationFactory->reveal()); - $resultedSubresourceOperation = $cachedSubresourceOperationFactory->create(Dummy::class); - $this->assertEquals(['foo' => 'bar'], $resultedSubresourceOperation); + $expectedResult = ['foo' => 'bar']; + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class)); + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class), 'Trigger the local cache'); } public function testCreateWithGetCacheItemThrowsCacheException() @@ -69,15 +71,16 @@ public function testCreateWithGetCacheItemThrowsCacheException() $cacheException->willExtend(\Exception::class); $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class); - $cacheItemPool->getItem($this->generateCacheKey())->willThrow($cacheException->reveal())->shouldBeCalled(); + $cacheItemPool->getItem($this->generateCacheKey())->willThrow($cacheException->reveal())->shouldBeCalledTimes(1); $decoratedSubresourceOperationFactory = $this->prophesize(SubresourceOperationFactoryInterface::class); - $decoratedSubresourceOperationFactory->create(Dummy::class)->shouldBeCalled()->willReturn(['foo' => 'bar']); + $decoratedSubresourceOperationFactory->create(Dummy::class)->shouldBeCalledTimes(1)->willReturn(['foo' => 'bar']); $cachedSubresourceOperationFactory = new CachedSubresourceOperationFactory($cacheItemPool->reveal(), $decoratedSubresourceOperationFactory->reveal()); - $resultedSubresourceOperation = $cachedSubresourceOperationFactory->create(Dummy::class); - $this->assertEquals(['foo' => 'bar'], $resultedSubresourceOperation); + $expectedResult = ['foo' => 'bar']; + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class)); + $this->assertEquals($expectedResult, $cachedSubresourceOperationFactory->create(Dummy::class), 'Trigger the local cache'); } private function generateCacheKey(string $resourceClass = Dummy::class) From 7e572bdeeb452a10d8a28c2bc23e7e9160ed1ed3 Mon Sep 17 00:00:00 2001 From: Ben Davies Date: Thu, 26 Apr 2018 10:12:15 +0100 Subject: [PATCH 18/24] fix normalizer cache key generation --- src/Hal/Serializer/ItemNormalizer.php | 13 +++++++++---- src/JsonApi/Serializer/ItemNormalizer.php | 16 ++++++++++++---- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/Hal/Serializer/ItemNormalizer.php b/src/Hal/Serializer/ItemNormalizer.php index 3c0c7691df7..bebb8c07059 100644 --- a/src/Hal/Serializer/ItemNormalizer.php +++ b/src/Hal/Serializer/ItemNormalizer.php @@ -43,7 +43,10 @@ public function supportsNormalization($data, $format = null) */ public function normalize($object, $format = null, array $context = []) { - $context['cache_key'] = $this->getHalCacheKey($format, $context); + if (!isset($context['cache_key'])) { + $context['cache_key'] = $this->getHalCacheKey($format, $context); + } + $resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class'] ?? null, true); $context = $this->initContext($resourceClass, $context); $context['iri'] = $this->iriConverter->getIriFromItem($object); @@ -99,8 +102,10 @@ protected function getAttributes($object, $format = null, array $context) */ private function getComponents($object, string $format = null, array $context) { - if (false !== $context['cache_key'] && isset($this->componentsCache[$context['cache_key']])) { - return $this->componentsCache[$context['cache_key']]; + $cacheKey = \get_class($object).'-'.$context['cache_key']; + + if (isset($this->componentsCache[$cacheKey])) { + return $this->componentsCache[$cacheKey]; } $attributes = parent::getAttributes($object, $format, $context); @@ -142,7 +147,7 @@ private function getComponents($object, string $format = null, array $context) } if (false !== $context['cache_key']) { - $this->componentsCache[$context['cache_key']] = $components; + $this->componentsCache[$cacheKey] = $components; } return $components; diff --git a/src/JsonApi/Serializer/ItemNormalizer.php b/src/JsonApi/Serializer/ItemNormalizer.php index d460d5c21b6..3f39e432f8e 100644 --- a/src/JsonApi/Serializer/ItemNormalizer.php +++ b/src/JsonApi/Serializer/ItemNormalizer.php @@ -60,7 +60,9 @@ public function supportsNormalization($data, $format = null) */ public function normalize($object, $format = null, array $context = []) { - $context['cache_key'] = $this->getJsonApiCacheKey($format, $context); + if (!isset($context['cache_key'])) { + $context['cache_key'] = $this->getJsonApiCacheKey($format, $context); + } // Get and populate attributes data $objectAttributesData = parent::normalize($object, $format, $context); @@ -221,8 +223,10 @@ protected function isAllowedAttribute($classOrObject, $attribute, $format = null */ private function getComponents($object, string $format = null, array $context) { - if (isset($this->componentsCache[$context['cache_key']])) { - return $this->componentsCache[$context['cache_key']]; + $cacheKey = \get_class($object).'-'.$context['cache_key']; + + if (isset($this->componentsCache[$cacheKey])) { + return $this->componentsCache[$cacheKey]; } $attributes = parent::getAttributes($object, $format, $context); @@ -267,7 +271,11 @@ private function getComponents($object, string $format = null, array $context) $components['relationships'][] = $relation; } - return $this->componentsCache[$context['cache_key']] = $components; + if (false !== $context['cache_key']) { + $this->componentsCache[$cacheKey] = $components; + } + + return $components; } /** From 60fe68744eeb082ce185ae4c784cea66a28f5d0f Mon Sep 17 00:00:00 2001 From: Ben Davies Date: Thu, 26 Apr 2018 20:26:36 +0100 Subject: [PATCH 19/24] respect forceEager=false on an association --- .../Orm/Extension/EagerLoadingExtension.php | 12 +++--- .../Extension/EagerLoadingExtensionTest.php | 41 +++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php index b16c46460ca..65a9651d440 100644 --- a/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php +++ b/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php @@ -170,14 +170,12 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt $inAttributes = null; } + if (false === $fetchEager = $propertyMetadata->getAttribute('fetchEager')) { + continue; + } + $isNotReadableLink = false === $propertyMetadata->isReadableLink(); - if ( - false === $propertyMetadata->getAttribute('fetchEager', false) && - ( - false === $propertyMetadata->isReadable() || - ((null === $inAttributes && $isNotReadableLink) || (false === $inAttributes)) - ) - ) { + if (null === $fetchEager && (false === $propertyMetadata->isReadable() || ((null === $inAttributes && $isNotReadableLink) || (false === $inAttributes)))) { continue; } diff --git a/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php index 5d3e52ad6d7..9522168c2dc 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php @@ -850,4 +850,45 @@ public function testApplyToCollectionWithANonRedableButFetchEagerProperty() $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30); $eagerExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class); } + + public function testApplyToCollectionWithARedableButNotFetchEagerProperty() + { + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(Dummy::class)->willReturn(new ResourceMetadata()); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $relationPropertyMetadata = new PropertyMetadata(); + $relationPropertyMetadata = $relationPropertyMetadata->withAttributes(['fetchEager' => false]); + $relationPropertyMetadata = $relationPropertyMetadata->withReadableLink(true); + $relationPropertyMetadata = $relationPropertyMetadata->withReadable(true); + + $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', [])->willReturn($relationPropertyMetadata)->shouldBeCalled(); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy2', [])->willReturn($relationPropertyMetadata)->shouldBeCalled(); + + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + + $classMetadataProphecy = $this->prophesize(ClassMetadata::class); + $classMetadataProphecy->associationMappings = [ + 'relatedDummy' => ['fetch' => ClassMetadataInfo::FETCH_EAGER, 'joinColumns' => [['nullable' => true]], 'targetEntity' => RelatedDummy::class], + 'relatedDummy2' => ['fetch' => ClassMetadataInfo::FETCH_EAGER, 'joinColumns' => [['nullable' => false]], 'targetEntity' => RelatedDummy::class], + ]; + + $emProphecy = $this->prophesize(EntityManager::class); + $emProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); + $emProphecy->getClassMetadata(RelatedDummy::class)->shouldNotBecalled(); + + $queryBuilderProphecy->getRootAliases()->willReturn(['o']); + $queryBuilderProphecy->getEntityManager()->willReturn($emProphecy); + + $queryBuilderProphecy->leftJoin('o.relatedDummy', 'relatedDummy_a1')->shouldNotBeCalled(); + $queryBuilderProphecy->innerJoin('o.relatedDummy2', 'relatedDummy2_a2')->shouldNotBeCalled(); + $queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldNotBeCalled(); + $queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldNotBeCalled(); + + $queryBuilder = $queryBuilderProphecy->reveal(); + $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30); + $eagerExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class); + } } From 9630a8489410737e592bd609ec6ebeff3bee697d Mon Sep 17 00:00:00 2001 From: Ben Davies Date: Mon, 30 Apr 2018 10:34:28 +0100 Subject: [PATCH 20/24] simplify boolean filter --- src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php | 6 ++---- .../Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php | 12 ++++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php b/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php index 8deb9dfe1cc..f868ed5376e 100644 --- a/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php +++ b/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php @@ -96,11 +96,9 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB list($alias, $field) = $this->addJoinsForNestedProperty($property, $alias, $queryBuilder, $queryNameGenerator, $resourceClass); } - $valueParameter = $queryNameGenerator->generateParameterName($field); - + $expr = $queryBuilder->expr(); $queryBuilder - ->andWhere(sprintf('%s.%s = :%s', $alias, $field, $valueParameter)) - ->setParameter($valueParameter, $value); + ->andWhere($expr->eq(sprintf('%s.%s', $alias, $field), $expr->literal($value))); } /** diff --git a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php index 27141419db7..d65335d1d8c 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php @@ -67,7 +67,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => 'true', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = true', Dummy::class), ], 'string ("false")' => [ [ @@ -78,7 +78,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => 'false', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), ], 'non-boolean' => [ [ @@ -100,7 +100,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => '0', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), ], 'numeric string ("1")' => [ [ @@ -111,7 +111,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => '1', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = true', Dummy::class), ], 'nested properties' => [ [ @@ -122,7 +122,7 @@ public function provideApplyTestData(): array [ 'relatedDummy.dummyBoolean' => '1', ], - sprintf('SELECT o FROM %s o INNER JOIN o.relatedDummy relatedDummy_a1 WHERE relatedDummy_a1.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o INNER JOIN o.relatedDummy relatedDummy_a1 WHERE relatedDummy_a1.dummyBoolean = true', Dummy::class), ], 'numeric string ("1") on non-boolean property' => [ [ @@ -180,7 +180,7 @@ public function provideApplyTestData(): array 'name' => 'true', 'id' => '0', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), ], ]; } From 0cd2625663ad4999c9edc1d384e6e549ee818597 Mon Sep 17 00:00:00 2001 From: Teoh Han Hui Date: Mon, 30 Apr 2018 12:08:56 +0200 Subject: [PATCH 21/24] Revert "Simplify boolean filter" --- src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php | 6 ++++-- .../Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php | 12 ++++++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php b/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php index f868ed5376e..8deb9dfe1cc 100644 --- a/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php +++ b/src/Bridge/Doctrine/Orm/Filter/BooleanFilter.php @@ -96,9 +96,11 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB list($alias, $field) = $this->addJoinsForNestedProperty($property, $alias, $queryBuilder, $queryNameGenerator, $resourceClass); } - $expr = $queryBuilder->expr(); + $valueParameter = $queryNameGenerator->generateParameterName($field); + $queryBuilder - ->andWhere($expr->eq(sprintf('%s.%s', $alias, $field), $expr->literal($value))); + ->andWhere(sprintf('%s.%s = :%s', $alias, $field, $valueParameter)) + ->setParameter($valueParameter, $value); } /** diff --git a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php index d65335d1d8c..27141419db7 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/BooleanFilterTest.php @@ -67,7 +67,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => 'true', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = true', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], 'string ("false")' => [ [ @@ -78,7 +78,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => 'false', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], 'non-boolean' => [ [ @@ -100,7 +100,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => '0', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], 'numeric string ("1")' => [ [ @@ -111,7 +111,7 @@ public function provideApplyTestData(): array [ 'dummyBoolean' => '1', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = true', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], 'nested properties' => [ [ @@ -122,7 +122,7 @@ public function provideApplyTestData(): array [ 'relatedDummy.dummyBoolean' => '1', ], - sprintf('SELECT o FROM %s o INNER JOIN o.relatedDummy relatedDummy_a1 WHERE relatedDummy_a1.dummyBoolean = true', Dummy::class), + sprintf('SELECT o FROM %s o INNER JOIN o.relatedDummy relatedDummy_a1 WHERE relatedDummy_a1.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], 'numeric string ("1") on non-boolean property' => [ [ @@ -180,7 +180,7 @@ public function provideApplyTestData(): array 'name' => 'true', 'id' => '0', ], - sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = false', Dummy::class), + sprintf('SELECT o FROM %s o WHERE o.dummyBoolean = :dummyBoolean_p1', Dummy::class), ], ]; } From fe04848c7cb9904fcd51218d223a9d0cd032ee6d Mon Sep 17 00:00:00 2001 From: Ben Davies Date: Mon, 30 Apr 2018 11:56:09 +0100 Subject: [PATCH 22/24] introduce cache trait to reuse common pattern --- .../Routing/CachedRouteNameResolver.php | 37 ++---------- src/Cache/CachedTrait.php | 59 +++++++++++++++++++ .../Factory/CachedPropertyMetadataFactory.php | 36 +++-------- .../CachedPropertyNameCollectionFactory.php | 36 +++-------- .../Factory/CachedResourceMetadataFactory.php | 33 ++--------- .../CachedResourceNameCollectionFactory.php | 33 ++--------- .../CachedSubresourceOperationFactory.php | 38 ++---------- 7 files changed, 97 insertions(+), 175 deletions(-) create mode 100644 src/Cache/CachedTrait.php diff --git a/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php b/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php index 548787f1ae3..7b2c079d449 100644 --- a/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php +++ b/src/Bridge/Symfony/Routing/CachedRouteNameResolver.php @@ -13,7 +13,7 @@ namespace ApiPlatform\Core\Bridge\Symfony\Routing; -use Psr\Cache\CacheException; +use ApiPlatform\Core\Cache\CachedTrait; use Psr\Cache\CacheItemPoolInterface; /** @@ -23,11 +23,11 @@ */ final class CachedRouteNameResolver implements RouteNameResolverInterface { + use CachedTrait; + const CACHE_KEY_PREFIX = 'route_name_'; - private $cacheItemPool; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, RouteNameResolverInterface $decorated) { @@ -43,33 +43,8 @@ public function getRouteName(string $resourceClass, $operationType /**, array $c $context = \func_num_args() > 2 ? func_get_arg(2) : []; $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass, $operationType, $context['subresource_resources'] ?? null])); - if (isset($this->localCache[$cacheKey])) { - return $this->localCache[$cacheKey]; - } - - try { - $cacheItem = $this->cacheItemPool->getItem($cacheKey); - - if ($cacheItem->isHit()) { - return $this->localCache[$cacheKey] = $cacheItem->get(); - } - } catch (CacheException $e) { - //do nothing - } - - $routeName = $this->decorated->getRouteName($resourceClass, $operationType, $context); - - if (!isset($cacheItem)) { - return $this->localCache[$cacheKey] = $routeName; - } - - try { - $cacheItem->set($routeName); - $this->cacheItemPool->save($cacheItem); - } catch (CacheException $e) { - // do nothing - } - - return $this->localCache[$cacheKey] = $routeName; + return $this->getCached($cacheKey, function () use ($resourceClass, $operationType, $context) { + return $this->decorated->getRouteName($resourceClass, $operationType, $context); + }); } } diff --git a/src/Cache/CachedTrait.php b/src/Cache/CachedTrait.php new file mode 100644 index 00000000000..c7a446e5775 --- /dev/null +++ b/src/Cache/CachedTrait.php @@ -0,0 +1,59 @@ + + * + * 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\Cache; + +use Psr\Cache\CacheException; +use Psr\Cache\CacheItemPoolInterface; + +/** + * @internal + */ +trait CachedTrait +{ + /** @var CacheItemPoolInterface */ + private $cacheItemPool; + private $localCache = []; + + private function getCached(string $cacheKey, callable $getValue) + { + if (array_key_exists($cacheKey, $this->localCache)) { + return $this->localCache[$cacheKey]; + } + + try { + $cacheItem = $this->cacheItemPool->getItem($cacheKey); + + if ($cacheItem->isHit()) { + return $this->localCache[$cacheKey] = $cacheItem->get(); + } + } catch (CacheException $e) { + //do nothing + } + + $value = $getValue(); + + if (!isset($cacheItem)) { + return $this->localCache[$cacheKey] = $value; + } + + try { + $cacheItem->set($value); + $this->cacheItemPool->save($cacheItem); + } catch (CacheException $e) { + // do nothing + } + + return $this->localCache[$cacheKey] = $value; + } +} diff --git a/src/Metadata/Property/Factory/CachedPropertyMetadataFactory.php b/src/Metadata/Property/Factory/CachedPropertyMetadataFactory.php index 7dd131a9d72..8d1b9915e59 100644 --- a/src/Metadata/Property/Factory/CachedPropertyMetadataFactory.php +++ b/src/Metadata/Property/Factory/CachedPropertyMetadataFactory.php @@ -13,8 +13,8 @@ namespace ApiPlatform\Core\Metadata\Property\Factory; +use ApiPlatform\Core\Cache\CachedTrait; use ApiPlatform\Core\Metadata\Property\PropertyMetadata; -use Psr\Cache\CacheException; use Psr\Cache\CacheItemPoolInterface; /** @@ -24,11 +24,11 @@ */ final class CachedPropertyMetadataFactory implements PropertyMetadataFactoryInterface { + use CachedTrait; + const CACHE_KEY_PREFIX = 'property_metadata_'; - private $cacheItemPool; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyMetadataFactoryInterface $decorated) { @@ -41,32 +41,10 @@ public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyMetad */ public function create(string $resourceClass, string $property, array $options = []): PropertyMetadata { - $localCacheKey = serialize([$resourceClass, $property, $options]); - if (isset($this->localCache[$localCacheKey])) { - return $this->localCache[$localCacheKey]; - } - - $cacheKey = self::CACHE_KEY_PREFIX.md5($localCacheKey); - - try { - $cacheItem = $this->cacheItemPool->getItem($cacheKey); - - if ($cacheItem->isHit()) { - return $this->localCache[$localCacheKey] = $cacheItem->get(); - } - } catch (CacheException $e) { - // do nothing - } - - $propertyMetadata = $this->decorated->create($resourceClass, $property, $options); - - if (!isset($cacheItem)) { - return $this->localCache[$localCacheKey] = $propertyMetadata; - } - - $cacheItem->set($propertyMetadata); - $this->cacheItemPool->save($cacheItem); + $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass, $property, $options])); - return $this->localCache[$localCacheKey] = $propertyMetadata; + return $this->getCached($cacheKey, function () use ($resourceClass, $property, $options) { + return $this->decorated->create($resourceClass, $property, $options); + }); } } diff --git a/src/Metadata/Property/Factory/CachedPropertyNameCollectionFactory.php b/src/Metadata/Property/Factory/CachedPropertyNameCollectionFactory.php index 84a842c4b36..2c89edf78e5 100644 --- a/src/Metadata/Property/Factory/CachedPropertyNameCollectionFactory.php +++ b/src/Metadata/Property/Factory/CachedPropertyNameCollectionFactory.php @@ -13,8 +13,8 @@ namespace ApiPlatform\Core\Metadata\Property\Factory; +use ApiPlatform\Core\Cache\CachedTrait; use ApiPlatform\Core\Metadata\Property\PropertyNameCollection; -use Psr\Cache\CacheException; use Psr\Cache\CacheItemPoolInterface; /** @@ -24,11 +24,11 @@ */ final class CachedPropertyNameCollectionFactory implements PropertyNameCollectionFactoryInterface { + use CachedTrait; + const CACHE_KEY_PREFIX = 'property_name_collection_'; - private $cacheItemPool; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyNameCollectionFactoryInterface $decorated) { @@ -41,32 +41,10 @@ public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyNameC */ public function create(string $resourceClass, array $options = []): PropertyNameCollection { - $localCacheKey = serialize([$resourceClass, $options]); - if (isset($this->localCache[$localCacheKey])) { - return $this->localCache[$localCacheKey]; - } - - $cacheKey = self::CACHE_KEY_PREFIX.md5($localCacheKey); - - try { - $cacheItem = $this->cacheItemPool->getItem($cacheKey); - - if ($cacheItem->isHit()) { - return $this->localCache[$localCacheKey] = $cacheItem->get(); - } - } catch (CacheException $e) { - // do nothing - } - - $propertyNameCollection = $this->decorated->create($resourceClass, $options); - - if (!isset($cacheItem)) { - return $this->localCache[$localCacheKey] = $propertyNameCollection; - } - - $cacheItem->set($propertyNameCollection); - $this->cacheItemPool->save($cacheItem); + $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass, $options])); - return $this->localCache[$localCacheKey] = $propertyNameCollection; + return $this->getCached($cacheKey, function () use ($resourceClass, $options) { + return $this->decorated->create($resourceClass, $options); + }); } } diff --git a/src/Metadata/Resource/Factory/CachedResourceMetadataFactory.php b/src/Metadata/Resource/Factory/CachedResourceMetadataFactory.php index 960cc5503de..e99bf7d3886 100644 --- a/src/Metadata/Resource/Factory/CachedResourceMetadataFactory.php +++ b/src/Metadata/Resource/Factory/CachedResourceMetadataFactory.php @@ -13,8 +13,8 @@ namespace ApiPlatform\Core\Metadata\Resource\Factory; +use ApiPlatform\Core\Cache\CachedTrait; use ApiPlatform\Core\Metadata\Resource\ResourceMetadata; -use Psr\Cache\CacheException; use Psr\Cache\CacheItemPoolInterface; /** @@ -24,11 +24,11 @@ */ final class CachedResourceMetadataFactory implements ResourceMetadataFactoryInterface { + use CachedTrait; + const CACHE_KEY_PREFIX = 'resource_metadata_'; - private $cacheItemPool; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, ResourceMetadataFactoryInterface $decorated) { @@ -41,31 +41,10 @@ public function __construct(CacheItemPoolInterface $cacheItemPool, ResourceMetad */ public function create(string $resourceClass): ResourceMetadata { - if (isset($this->localCache[$resourceClass])) { - return $this->localCache[$resourceClass]; - } - $cacheKey = self::CACHE_KEY_PREFIX.md5($resourceClass); - try { - $cacheItem = $this->cacheItemPool->getItem($cacheKey); - - if ($cacheItem->isHit()) { - return $this->localCache[$resourceClass] = $cacheItem->get(); - } - } catch (CacheException $e) { - // do nothing - } - - $resourceMetadata = $this->decorated->create($resourceClass); - - if (!isset($cacheItem)) { - return $this->localCache[$resourceClass] = $resourceMetadata; - } - - $cacheItem->set($resourceMetadata); - $this->cacheItemPool->save($cacheItem); - - return $this->localCache[$resourceClass] = $resourceMetadata; + return $this->getCached($cacheKey, function () use ($resourceClass) { + return $this->decorated->create($resourceClass); + }); } } diff --git a/src/Metadata/Resource/Factory/CachedResourceNameCollectionFactory.php b/src/Metadata/Resource/Factory/CachedResourceNameCollectionFactory.php index 3f48fff4877..f9e077ee723 100644 --- a/src/Metadata/Resource/Factory/CachedResourceNameCollectionFactory.php +++ b/src/Metadata/Resource/Factory/CachedResourceNameCollectionFactory.php @@ -13,8 +13,8 @@ namespace ApiPlatform\Core\Metadata\Resource\Factory; +use ApiPlatform\Core\Cache\CachedTrait; use ApiPlatform\Core\Metadata\Resource\ResourceNameCollection; -use Psr\Cache\CacheException; use Psr\Cache\CacheItemPoolInterface; /** @@ -24,11 +24,11 @@ */ final class CachedResourceNameCollectionFactory implements ResourceNameCollectionFactoryInterface { + use CachedTrait; + const CACHE_KEY = 'resource_name_collection'; - private $cacheItemPool; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, ResourceNameCollectionFactoryInterface $decorated) { @@ -41,29 +41,8 @@ public function __construct(CacheItemPoolInterface $cacheItemPool, ResourceNameC */ public function create(): ResourceNameCollection { - if (isset($this->localCache[self::CACHE_KEY])) { - return $this->localCache[self::CACHE_KEY]; - } - - try { - $cacheItem = $this->cacheItemPool->getItem(self::CACHE_KEY); - - if ($cacheItem->isHit()) { - return $this->localCache[self::CACHE_KEY] = $cacheItem->get(); - } - } catch (CacheException $e) { - // do nothing - } - - $resourceNameCollection = $this->decorated->create(); - - if (!isset($cacheItem)) { - return $this->localCache[self::CACHE_KEY] = $resourceNameCollection; - } - - $cacheItem->set($resourceNameCollection); - $this->cacheItemPool->save($cacheItem); - - return $this->localCache[self::CACHE_KEY] = $resourceNameCollection; + return $this->getCached(self::CACHE_KEY, function () { + return $this->decorated->create(); + }); } } diff --git a/src/Operation/Factory/CachedSubresourceOperationFactory.php b/src/Operation/Factory/CachedSubresourceOperationFactory.php index c9dc94aebe4..072ea12a711 100644 --- a/src/Operation/Factory/CachedSubresourceOperationFactory.php +++ b/src/Operation/Factory/CachedSubresourceOperationFactory.php @@ -13,7 +13,7 @@ namespace ApiPlatform\Core\Operation\Factory; -use Psr\Cache\CacheException; +use ApiPlatform\Core\Cache\CachedTrait; use Psr\Cache\CacheItemPoolInterface; /** @@ -21,11 +21,10 @@ */ final class CachedSubresourceOperationFactory implements SubresourceOperationFactoryInterface { - const CACHE_KEY_PREFIX = 'subresource_operations_'; + use CachedTrait; - private $cacheItemPool; + const CACHE_KEY_PREFIX = 'subresource_operations_'; private $decorated; - private $localCache = []; public function __construct(CacheItemPoolInterface $cacheItemPool, SubresourceOperationFactoryInterface $decorated) { @@ -40,33 +39,8 @@ public function create(string $resourceClass): array { $cacheKey = self::CACHE_KEY_PREFIX.md5($resourceClass); - if (isset($this->localCache[$cacheKey])) { - return $this->localCache[$cacheKey]; - } - - try { - $cacheItem = $this->cacheItemPool->getItem($cacheKey); - - if ($cacheItem->isHit()) { - return $this->localCache[$cacheKey] = $cacheItem->get(); - } - } catch (CacheException $e) { - // do nothing - } - - $subresourceOperations = $this->decorated->create($resourceClass); - - if (!isset($cacheItem)) { - return $this->localCache[$cacheKey] = $subresourceOperations; - } - - try { - $cacheItem->set($subresourceOperations); - $this->cacheItemPool->save($cacheItem); - } catch (CacheException $e) { - // do nothing - } - - return $this->localCache[$cacheKey] = $subresourceOperations; + return $this->getCached($cacheKey, function () use ($resourceClass) { + return $this->decorated->create($resourceClass); + }); } } From 1fd33ce294c57f0b511a753698fe889c70464ae7 Mon Sep 17 00:00:00 2001 From: Alan Poulain Date: Sun, 15 Apr 2018 18:41:08 +0200 Subject: [PATCH 23/24] Update graphql-php to 0.11.5 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 20d92c5d136..98c1f5b361d 100644 --- a/composer.json +++ b/composer.json @@ -64,7 +64,7 @@ "symfony/twig-bundle": "^3.1 || ^4.0", "symfony/validator": "^3.3 || ^4.0", "symfony/yaml": "^3.3 || ^4.0", - "webonyx/graphql-php": "^0.10.2" + "webonyx/graphql-php": "^0.11.5" }, "conflict": { "symfony/dependency-injection": "<3.3" From 5b82c7018c691295f156a256b7e3b55696c3993f Mon Sep 17 00:00:00 2001 From: Alan Poulain Date: Sun, 15 Apr 2018 18:41:43 +0200 Subject: [PATCH 24/24] Add a better test for introspection --- features/bootstrap/GraphqlContext.php | 10 ++++++++++ features/graphql/introspection.feature | 13 +++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/features/bootstrap/GraphqlContext.php b/features/bootstrap/GraphqlContext.php index fc31e4efc3c..fb9d94c701a 100644 --- a/features/bootstrap/GraphqlContext.php +++ b/features/bootstrap/GraphqlContext.php @@ -17,6 +17,7 @@ use Behat\Gherkin\Node\PyStringNode; use Behatch\Context\RestContext; use Behatch\HttpCall\Request; +use GraphQL\Type\Introspection; /** * Context for GraphQL. @@ -95,6 +96,15 @@ public function ISendTheGraphqlRequestWithOperation(string $operation) $this->sendGraphqlRequest(); } + /** + * @When I send the query to introspect the schema + */ + public function ISendTheQueryToIntrospectTheSchema() + { + $this->graphqlRequest = ['query' => Introspection::getIntrospectionQuery()]; + $this->sendGraphqlRequest(); + } + private function sendGraphqlRequest() { $this->request->setHttpHeader('Accept', null); diff --git a/features/graphql/introspection.feature b/features/graphql/introspection.feature index 1a538101882..a766b0dba40 100644 --- a/features/graphql/introspection.feature +++ b/features/graphql/introspection.feature @@ -9,20 +9,13 @@ Feature: GraphQL introspection support And the JSON node "errors[0].message" should be equal to "GraphQL query is not valid" Scenario: Introspect the GraphQL schema - When I send the following GraphQL request: - """ - { - __schema { - types { - name - } - } - } - """ + When I send the query to introspect the schema 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.__schema.types" should exist + And the JSON node "data.__schema.queryType.name" should be equal to "Query" + And the JSON node "data.__schema.mutationType.name" should be equal to "Mutation" Scenario: Introspect types When I send the following GraphQL request: