From ec6d26201cfad471124440654ef828a19153ca0f Mon Sep 17 00:00:00 2001 From: Clemens Pflaum Date: Wed, 20 Feb 2019 22:58:10 +0100 Subject: [PATCH 1/3] Changed depth and maxDepth handling for selfreferencing subresourceOperations. Addresses: https://github.com/api-platform/core/issues/2533. --- .../Factory/SubresourceOperationFactory.php | 15 ++-- .../SubresourceOperationFactoryTest.php | 79 ++++++++++++++++++- 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/Operation/Factory/SubresourceOperationFactory.php b/src/Operation/Factory/SubresourceOperationFactory.php index ece32c2240f..5e9c0032af5 100644 --- a/src/Operation/Factory/SubresourceOperationFactory.php +++ b/src/Operation/Factory/SubresourceOperationFactory.php @@ -64,6 +64,7 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre { if (null === $rootResourceClass) { $rootResourceClass = $resourceClass; + $depth = 0; } foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $property) { @@ -85,12 +86,6 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $visiting = "$resourceClass $property $subresourceClass"; // Handle maxDepth - if ($rootResourceClass === $resourceClass) { - $maxDepth = $subresource->getMaxDepth(); - // reset depth when we return to rootResourceClass - $depth = 0; - } - if (null !== $maxDepth && $depth >= $maxDepth) { break; } @@ -134,7 +129,7 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $operation['path'] = $subresourceOperation['path'] ?? sprintf( '/%s%s/{id}/%s%s', $prefix, - $this->pathSegmentNameGenerator->getSegmentName($rootShortname), + $this->pathSegmentNameGenerator->getSegmentName($rootShortname, true), $this->pathSegmentNameGenerator->getSegmentName($operation['property'], $operation['collection']), self::FORMAT_SUFFIX ); @@ -183,7 +178,11 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $tree[$operation['route_name']] = $operation; - $this->computeSubresourceOperations($subresourceClass, $tree, $rootResourceClass, $operation, $visited + [$visiting => true], ++$depth, $maxDepth); + // get the minimum maxDepth from the rootMaxDepth and the maxDepth of the to be visited Subresource + $currentMaxDepth = array_diff([$maxDepth, $subresource->getMaxDepth()], [null]); + $currentMaxDepth = empty($currentMaxDepth) ? null : min($currentMaxDepth); + + $this->computeSubresourceOperations($subresourceClass, $tree, $rootResourceClass, $operation, $visited + [$visiting => true], $depth + 1, $currentMaxDepth); } } } diff --git a/tests/Operation/Factory/SubresourceOperationFactoryTest.php b/tests/Operation/Factory/SubresourceOperationFactoryTest.php index 74cd77841ff..cbc89b6e754 100644 --- a/tests/Operation/Factory/SubresourceOperationFactoryTest.php +++ b/tests/Operation/Factory/SubresourceOperationFactoryTest.php @@ -465,7 +465,7 @@ public function testCreateWithMaxDepthMultipleSubresourcesSameMaxDepth() public function testCreateSelfReferencingSubresources() { /** - * DummyEntity -subresource-> DummyEntity -subresource-> DummyEntity ... + * DummyEntity -subresource-> DummyEntity --> DummyEntity ... */ $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $resourceMetadataFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('dummyEntity')); @@ -505,6 +505,83 @@ public function testCreateSelfReferencingSubresources() ], $subresourceOperationFactory->create(DummyEntity::class)); } + /** + * Test for issue: https://github.com/api-platform/core/issues/2533. + */ + public function testCreateWithDifferentMaxDepthSelfReferencingSubresources() + { + /** + * subresource: maxDepth = 2 + * secondSubresource: maxDepth = 1 + * DummyEntity -subresource-> DummyEntity -secondSubresource-> DummyEntity ... + * DummyEntity -secondSubresource-> DummyEntity !!!-subresource-> DummyEntity ... + */ + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('dummyEntity')); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['subresource', 'secondSubresource'])); + + $subresourceWithMaxDepthMetadata = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(DummyEntity::class, false, 2)); + $secondSubresourceWithMaxDepthMetadata = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(DummyEntity::class, false, 1)); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'subresource')->shouldBeCalled()->willReturn($subresourceWithMaxDepthMetadata); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'secondSubresource')->shouldBeCalled()->willReturn($secondSubresourceWithMaxDepthMetadata); + + $pathSegmentNameGeneratorProphecy = $this->prophesize(PathSegmentNameGeneratorInterface::class); + $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity', true)->shouldBeCalled()->willReturn('dummy_entities'); + $pathSegmentNameGeneratorProphecy->getSegmentName('subresource', false)->shouldBeCalled()->willReturn('subresources'); + $pathSegmentNameGeneratorProphecy->getSegmentName('secondSubresource', false)->shouldBeCalled()->willReturn('second_subresources'); + + $subresourceOperationFactory = new SubresourceOperationFactory( + $resourceMetadataFactoryProphecy->reveal(), + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $pathSegmentNameGeneratorProphecy->reveal() + ); + + $this->assertEquals([ + 'api_dummy_entities_subresource_get_subresource' => [ + 'property' => 'subresource', + 'collection' => false, + 'resource_class' => DummyEntity::class, + 'shortNames' => ['dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/subresources.{_format}', + 'operation_name' => 'subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + 'api_dummy_entities_subresource_second_subresource_get_subresource' => [ + 'property' => 'secondSubresource', + 'collection' => false, + 'resource_class' => DummyEntity::class, + 'shortNames' => ['dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ['subresource', DummyEntity::class, false], + ], + 'route_name' => 'api_dummy_entities_subresource_second_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/subresources/second_subresources.{_format}', + 'operation_name' => 'subresource_second_subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + 'api_dummy_entities_second_subresource_get_subresource' => [ + 'property' => 'secondSubresource', + 'collection' => false, + 'resource_class' => DummyEntity::class, + 'shortNames' => ['dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_second_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/second_subresources.{_format}', + 'operation_name' => 'second_subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + ], $subresourceOperationFactory->create(DummyEntity::class)); + } + public function testCreateWithEnd() { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); From 3d7b144039b7ea25c3a9b8a03de720ce9d9d40be Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 22 Jun 2020 11:46:42 +0200 Subject: [PATCH 2/3] Use single name for subpath --- src/Operation/Factory/SubresourceOperationFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Operation/Factory/SubresourceOperationFactory.php b/src/Operation/Factory/SubresourceOperationFactory.php index 5e9c0032af5..75335fe080d 100644 --- a/src/Operation/Factory/SubresourceOperationFactory.php +++ b/src/Operation/Factory/SubresourceOperationFactory.php @@ -129,7 +129,7 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $operation['path'] = $subresourceOperation['path'] ?? sprintf( '/%s%s/{id}/%s%s', $prefix, - $this->pathSegmentNameGenerator->getSegmentName($rootShortname, true), + $this->pathSegmentNameGenerator->getSegmentName($rootShortname), $this->pathSegmentNameGenerator->getSegmentName($operation['property'], $operation['collection']), self::FORMAT_SUFFIX ); From 40137954163d4302f968dcd2a8395c0776964bf4 Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 27 Jul 2020 14:49:37 +0200 Subject: [PATCH 3/3] fix: address @rkopera comments --- .../Factory/SubresourceOperationFactory.php | 5 +- .../SubresourceOperationFactoryTest.php | 63 ++++++++++++++++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/Operation/Factory/SubresourceOperationFactory.php b/src/Operation/Factory/SubresourceOperationFactory.php index 75335fe080d..227b9d494fb 100644 --- a/src/Operation/Factory/SubresourceOperationFactory.php +++ b/src/Operation/Factory/SubresourceOperationFactory.php @@ -64,7 +64,6 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre { if (null === $rootResourceClass) { $rootResourceClass = $resourceClass; - $depth = 0; } foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $property) { @@ -178,8 +177,8 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre $tree[$operation['route_name']] = $operation; - // get the minimum maxDepth from the rootMaxDepth and the maxDepth of the to be visited Subresource - $currentMaxDepth = array_diff([$maxDepth, $subresource->getMaxDepth()], [null]); + // Get the minimum maxDepth between the rootMaxDepth and the maxDepth of the to be visited Subresource + $currentMaxDepth = array_filter([$maxDepth, $subresource->getMaxDepth()], 'is_int'); $currentMaxDepth = empty($currentMaxDepth) ? null : min($currentMaxDepth); $this->computeSubresourceOperations($subresourceClass, $tree, $rootResourceClass, $operation, $visited + [$visiting => true], $depth + 1, $currentMaxDepth); diff --git a/tests/Operation/Factory/SubresourceOperationFactoryTest.php b/tests/Operation/Factory/SubresourceOperationFactoryTest.php index cbc89b6e754..292fcd4c478 100644 --- a/tests/Operation/Factory/SubresourceOperationFactoryTest.php +++ b/tests/Operation/Factory/SubresourceOperationFactoryTest.php @@ -530,7 +530,7 @@ public function testCreateWithDifferentMaxDepthSelfReferencingSubresources() $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'secondSubresource')->shouldBeCalled()->willReturn($secondSubresourceWithMaxDepthMetadata); $pathSegmentNameGeneratorProphecy = $this->prophesize(PathSegmentNameGeneratorInterface::class); - $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity', true)->shouldBeCalled()->willReturn('dummy_entities'); + $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity')->shouldBeCalled()->willReturn('dummy_entities'); $pathSegmentNameGeneratorProphecy->getSegmentName('subresource', false)->shouldBeCalled()->willReturn('subresources'); $pathSegmentNameGeneratorProphecy->getSegmentName('secondSubresource', false)->shouldBeCalled()->willReturn('second_subresources'); @@ -729,4 +729,65 @@ public function testCreateWithRootResourcePrefix() ] + SubresourceOperationFactory::ROUTE_OPTIONS, ], $subresourceOperationFactory->create(DummyEntity::class)); } + + public function testCreateSelfReferencingSubresourcesWithSubresources() + { + /** + * DummyEntity -otherSubresource-> RelatedDummyEntity + * DummyEntity -subresource (maxDepth=1) -> DummyEntity -otherSubresource-> RelatedDummyEntity. + */ + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('dummyEntity')); + $resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new ResourceMetadata('relatedDummyEntity')); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(DummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection(['subresource', 'otherSubresource'])); + $propertyNameCollectionFactoryProphecy->create(RelatedDummyEntity::class)->shouldBeCalled()->willReturn(new PropertyNameCollection([])); + + $subresource = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(DummyEntity::class, false, 1)); + $otherSubresourceSubresource = (new PropertyMetadata())->withSubresource(new SubresourceMetadata(RelatedDummyEntity::class, false)); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'subresource')->shouldBeCalled()->willReturn($subresource); + $propertyMetadataFactoryProphecy->create(DummyEntity::class, 'otherSubresource')->shouldBeCalled()->willReturn($otherSubresourceSubresource); + + $pathSegmentNameGeneratorProphecy = $this->prophesize(PathSegmentNameGeneratorInterface::class); + $pathSegmentNameGeneratorProphecy->getSegmentName('dummyEntity')->shouldBeCalled()->willReturn('dummy_entities'); + $pathSegmentNameGeneratorProphecy->getSegmentName('subresource', false)->shouldBeCalled()->willReturn('subresources'); + $pathSegmentNameGeneratorProphecy->getSegmentName('otherSubresource', false)->shouldBeCalled()->willReturn('other_subresources'); + + $subresourceOperationFactory = new SubresourceOperationFactory( + $resourceMetadataFactoryProphecy->reveal(), + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $pathSegmentNameGeneratorProphecy->reveal() + ); + + $this->assertEquals([ + 'api_dummy_entities_subresource_get_subresource' => [ + 'property' => 'subresource', + 'collection' => false, + 'resource_class' => DummyEntity::class, + 'shortNames' => ['dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/subresources.{_format}', + 'operation_name' => 'subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + 'api_dummy_entities_other_subresource_get_subresource' => [ + 'property' => 'otherSubresource', + 'collection' => false, + 'resource_class' => RelatedDummyEntity::class, + 'shortNames' => ['relatedDummyEntity', 'dummyEntity'], + 'identifiers' => [ + ['id', DummyEntity::class, true], + ], + 'route_name' => 'api_dummy_entities_other_subresource_get_subresource', + 'path' => '/dummy_entities/{id}/other_subresources.{_format}', + 'operation_name' => 'other_subresource_get_subresource', + ] + SubresourceOperationFactory::ROUTE_OPTIONS, + ], $subresourceOperationFactory->create(DummyEntity::class)); + } }