From c2347ec53547535e8de35d6c531b85c44309a7b8 Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 8 Nov 2021 08:31:02 +0100 Subject: [PATCH] Oh yeah --- features/main/subresource.feature | 1 + .../Doctrine/Orm/State/ItemProvider.php | 1 - .../Doctrine/Orm/State/LinksHandlerTrait.php | 183 +++++++++--------- .../Parser/TransformApiSubresourceVisitor.php | 10 +- .../Rector/Service/SubresourceTransformer.php | 1 - ...plateResourceMetadataCollectionFactory.php | 4 +- .../Service/SubresourceTransformerTest.php | 65 ++----- 7 files changed, 112 insertions(+), 153 deletions(-) diff --git a/features/main/subresource.feature b/features/main/subresource.feature index d0662851a1a..ae4969a5abf 100644 --- a/features/main/subresource.feature +++ b/features/main/subresource.feature @@ -301,6 +301,7 @@ Feature: Subresource support } """ + @createSchema Scenario: Get offers subresource from aggregate offers subresource Given I have a product with offers When I send a "GET" request to "/dummy_products/2/offers/1/offers" diff --git a/src/Bridge/Doctrine/Orm/State/ItemProvider.php b/src/Bridge/Doctrine/Orm/State/ItemProvider.php index 7b418e3da75..709c1aaa9d2 100644 --- a/src/Bridge/Doctrine/Orm/State/ItemProvider.php +++ b/src/Bridge/Doctrine/Orm/State/ItemProvider.php @@ -68,7 +68,6 @@ public function provide(string $resourceClass, array $identifiers = [], ?string $this->handleLinks($queryBuilder, $identifiers, $queryNameGenerator, $context, $resourceClass, $operationName); - dd($queryBuilder->getDQL()); foreach ($this->itemExtensions as $extension) { $extension->applyToItem($queryBuilder, $queryNameGenerator, $resourceClass, $identifiers, $operationName, $context); diff --git a/src/Bridge/Doctrine/Orm/State/LinksHandlerTrait.php b/src/Bridge/Doctrine/Orm/State/LinksHandlerTrait.php index 083eba99360..393ab1d1aef 100644 --- a/src/Bridge/Doctrine/Orm/State/LinksHandlerTrait.php +++ b/src/Bridge/Doctrine/Orm/State/LinksHandlerTrait.php @@ -16,6 +16,8 @@ use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator; use ApiPlatform\Metadata\GraphQl\Operation as GraphQlOperation; use ApiPlatform\Metadata\Link; +use ApiPlatform\Tests\Fixtures\TestBundle\Entity\DummyProduct; +use Doctrine\ORM\Mapping\ClassMetadataInfo; use Doctrine\ORM\QueryBuilder; use Doctrine\Persistence\Mapping\ClassMetadata; @@ -30,113 +32,110 @@ private function handleLinks(QueryBuilder $queryBuilder, array $identifiers, Que $links = $operation instanceof GraphQlOperation ? $operation->getLinks() : $operation->getUriVariables(); - if ($linkClass = $context['linkClass'] ?? false) { - foreach ($links as $link) { - if ($linkClass === $link->getTargetClass()) { - foreach ($identifiers as $identifier => $value) { - $this->applyLink($queryBuilder, $queryNameGenerator, $doctrineClassMetadata, $alias, $link, $identifier, $value); - } - - return; - } - } - } - - if (null === $links) { + // if ($linkClass = $context['linkClass'] ?? false) { + // foreach ($links as $link) { + // if ($linkClass === $link->getTargetClass()) { + // foreach ($identifiers as $identifier => $value) { + // $this->applyLink($queryBuilder, $queryNameGenerator, $doctrineClassMetadata, $alias, $link, $identifier, $value); + // } + // + // return; + // } + // } + // } + + if (!$links) { return; } - dd($links) - dump($operation->getUriTemplate()); - $aliases = []; - $previousContext = []; - $conditions = []; - foreach ($links as $parameterName => $link) { - $identifier = $link->getIdentifiers()[0]; - if (!isset($aliases[$link->getTargetClass()])) { - $aliases[$link->getTargetClass()] = $link->getTargetClass() === $operation->getClass() ? $alias : $queryNameGenerator->generateJoinAlias($alias); - } + $previousAlias = $alias; + $previousIdentifier = end($links)->getIdentifiers()[0] ?? 'id'; + $expressions = []; + $i = 0; - $currentAlias = $aliases[$link->getTargetClass()]; - $placeholder = $queryNameGenerator->generateParameterName($parameterName); - // inverse == prop de la targetClass - if (!$link->getInverseProperty() && !$link->getProperty() && !$link->getExpandedValue()) { - $conditions[] = $aliases[$link->getTargetClass()] . ".$identifier = :$placeholder"; - $previousContext = ['alias' => $currentAlias, 'identifier' => $identifier]; + foreach (array_reverse($links) as $parameterName => $link) { + if ($link->getExpandedValue() || !$link->getFromClass()) { + ++$i; continue; } + $identifierProperty = $link->getIdentifiers()[0] ?? 'id'; + $currentAlias = $i === 0 ? $alias : $queryNameGenerator->generateJoinAlias($alias); + $placeholder = $queryNameGenerator->generateParameterName($parameterName); + + if (!$link->getFromProperty() && !$link->getToProperty()) { + $doctrineClassMetadata = $manager->getClassMetadata($link->getFromClass()); - if ($inverseProperty = $link->getInverseProperty()) { - $joinAlias = $queryNameGenerator->generateJoinAlias($alias); - $inverseAlias = $queryNameGenerator->generateJoinAlias($alias); - $conditions[] = "{$previousContext['alias']}.{$previousContext['identifier']} IN (SELECT $inverseAlias.$identifier FROM {$link->getTargetClass()} $joinAlias JOIN $joinAlias.$inverseProperty $inverseAlias WHERE $joinAlias.$identifier = $placeholder)"; - $previousContext = ['alias' => $currentAlias, 'identifier' => $identifier]; + $queryBuilder->andWhere("{$currentAlias}.$identifierProperty = :$placeholder"); + $queryBuilder->setParameter($placeholder, $identifiers[$parameterName], $doctrineClassMetadata->getTypeOfField($identifierProperty)); + $previousAlias = $currentAlias; + $previousIdentifier = $identifierProperty; + ++$i; continue; } - dump($conditions); - dd($link); - // dd($link); - // if ($link->getExpandedValue()) { - // dd($queryBuilder->getDQL()); - // dd('test'); - // continue; - // } + if ($link->getFromProperty()) { + $doctrineClassMetadata = $manager->getClassMetadata($link->getFromClass()); + $joinAlias = $queryNameGenerator->generateJoinAlias('m'); + $assocationMapping = $doctrineClassMetadata->getAssociationMappings()[$link->getFromProperty()]; + $relationType = $assocationMapping['type']; - // $this->applyLink($queryBuilder, $queryNameGenerator, $doctrineClassMetadata, $alias, $link, $identifiers[$parameterName] ?? null); - } - } + if ($relationType & ClassMetadataInfo::TO_MANY) { + $nextAlias = $queryNameGenerator->generateJoinAlias($alias); + + $expressions["$previousAlias.$previousIdentifier"] = "SELECT $joinAlias.{$previousIdentifier} FROM {$link->getFromClass()} $nextAlias INNER JOIN $nextAlias.{$link->getFromProperty()} $joinAlias WHERE $nextAlias.{$identifierProperty} = :$placeholder"; + + $queryBuilder->setParameter($placeholder, $identifiers[$parameterName], $doctrineClassMetadata->getTypeOfField($identifierProperty)); + $previousAlias = $nextAlias; + ++$i; + continue; + } + + + // A single-valued association path expression to an inverse side is not supported in DQL queries. + if ($relationType & ClassMetadataInfo::TO_ONE && !$assocationMapping['isOwningSide']) { + $queryBuilder->innerJoin("$previousAlias.".$assocationMapping['mappedBy'], $joinAlias); + } else { + $queryBuilder->join( + $link->getFromClass(), + $joinAlias, + 'with', + "{$previousAlias}.{$previousIdentifier} = $joinAlias.{$link->getFromProperty()}" + ); + } + + $queryBuilder->andWhere("$joinAlias.$identifierProperty = :$placeholder"); + $queryBuilder->setParameter($placeholder, $identifiers[$parameterName], $doctrineClassMetadata->getTypeOfField($identifierProperty)); + $previousAlias = $joinAlias; + $previousIdentifier = $identifierProperty; + ++$i; + continue; + } - private function applyLink(QueryBuilder $queryBuilder, QueryNameGenerator $queryNameGenerator, ClassMetadata $doctrineClassMetadata, string $alias, Link $link, $value = null) - { - $propertyIdentifier = $link->getIdentifiers()[0]; - $placeholder = ':' . $queryNameGenerator->generateParameterName($propertyIdentifier); - if ($inverseProperty = $link->getInverseProperty()) { $joinAlias = $queryNameGenerator->generateJoinAlias($alias); - $inverseAlias = $queryNameGenerator->generateJoinAlias($alias); - - // SELECT o FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\ThirdLevel o - // WHERE o.id IN ( - // SELECT o_a3.id FROM - // ApiPlatform\Tests\Fixtures\TestBundle\Entity\Dummy o_a1 - // JOIN o_a1.relatedDummies o_a2 - // JOIN o_a2.thirdLevel o_a3 - // WHERE o_a1.id = :id_id - // ) AND o.id = :id_id - - // $queryBuilder->join( - // $link->getTargetClass(), - // $joinAlias, - // 'with', - // "$alias.$propertyIdentifier = $joinAlias.$inverseProperty" - // ); - - $expression = "$alias.$propertyIdentifier IN (SELECT $inverseAlias.$propertyIdentifier FROM {$link->getTargetClass()} $joinAlias JOIN $joinAlias.$inverseProperty $inverseAlias WHERE $joinAlias.$propertyIdentifier = $placeholder)"; - - // $expression = $queryBuilder->expr()->eq( - // "{$joinAlias}.{$propertyIdentifier}", - // $placeholder - // ); - } elseif ($property = $link->getProperty()) { - $joinAlias = $queryNameGenerator->generateJoinAlias($property); - - $queryBuilder->join( - "$alias.$property", - $joinAlias, - ); - - $expression = $queryBuilder->expr()->eq( - "{$joinAlias}.{$propertyIdentifier}", - $placeholder - ); - } else { - $expression = $queryBuilder->expr()->eq( - "{$alias}.{$propertyIdentifier}", $placeholder - ); + $queryBuilder->join("{$previousAlias}.{$link->getToProperty()}", $joinAlias); + $queryBuilder->andWhere("$joinAlias.$identifierProperty = :$placeholder"); + $queryBuilder->setParameter($placeholder, $identifiers[$parameterName], $doctrineClassMetadata->getTypeOfField($identifierProperty)); + $previousAlias = $joinAlias; + $previousIdentifier = $identifierProperty; + ++$i; } - $queryBuilder->andWhere($expression); - $queryBuilder->setParameter($placeholder, $value, $doctrineClassMetadata->getTypeOfField($propertyIdentifier)); + if ($expressions) { + $i = 0; + $clause = ''; + foreach ($expressions as $alias => $expression) { + if ($i === 0) { + $clause .= "$alias IN (" . $expression; + $i++; + continue; + } + + $clause .= " AND $alias IN (" . $expression; + $i++; + } + + $queryBuilder->andWhere($clause . str_repeat(')', $i)); + } } } diff --git a/src/Core/Bridge/Rector/Parser/TransformApiSubresourceVisitor.php b/src/Core/Bridge/Rector/Parser/TransformApiSubresourceVisitor.php index 801fa66602c..d98d64eb1b5 100644 --- a/src/Core/Bridge/Rector/Parser/TransformApiSubresourceVisitor.php +++ b/src/Core/Bridge/Rector/Parser/TransformApiSubresourceVisitor.php @@ -70,11 +70,11 @@ public function enterNode(Node $node) new Node\Expr\ArrayItem( new Node\Expr\ClassConstFetch( new Node\Name( - ($resource['target_class'] === $this->subresourceMetadata['resource_class']) ? 'self' : '\\'.$resource['target_class'] + ($resource['from_class'] === $this->subresourceMetadata['resource_class']) ? 'self' : '\\'.$resource['from_class'] ), 'class' ), - new Node\Scalar\String_('target_class') + new Node\Scalar\String_('from_class') ), new Node\Expr\ArrayItem( new Node\Expr\Array_( @@ -93,10 +93,10 @@ public function enterNode(Node $node) ); } - if (isset($resource['property']) || isset($resource['inverse_property'])) { + if (isset($resource['from_property']) || isset($resource['to_property'])) { $identifierNodes[] = new Node\Expr\ArrayItem( - new Node\Scalar\String_($resource['property'] ?? $resource['inverse_property']), - new Node\Scalar\String_(isset($resource['property']) ? 'property' : 'inverse_property') + new Node\Scalar\String_($resource['to_property'] ?? $resource['from_property']), + new Node\Scalar\String_(isset($resource['to_property']) ? 'to_property' : 'from_property') ); } diff --git a/src/Core/Bridge/Rector/Service/SubresourceTransformer.php b/src/Core/Bridge/Rector/Service/SubresourceTransformer.php index ef71d610e23..5bd5826ec87 100644 --- a/src/Core/Bridge/Rector/Service/SubresourceTransformer.php +++ b/src/Core/Bridge/Rector/Service/SubresourceTransformer.php @@ -45,7 +45,6 @@ public function toUriVariables(array $subresourceMetadata): array $fromClassMetadataAssociationMappings = $fromClassMetadata->getAssociationMappings(); $uriVariables[$identifier] = [ - 'target_class' => $fromClass, 'from_class' => $fromClass, 'inverse_property' => null, 'from_property' => null, diff --git a/src/Metadata/Resource/Factory/UriTemplateResourceMetadataCollectionFactory.php b/src/Metadata/Resource/Factory/UriTemplateResourceMetadataCollectionFactory.php index bcb58ea263a..d11615c8a5f 100644 --- a/src/Metadata/Resource/Factory/UriTemplateResourceMetadataCollectionFactory.php +++ b/src/Metadata/Resource/Factory/UriTemplateResourceMetadataCollectionFactory.php @@ -175,13 +175,13 @@ private function normalizeUriVariables($operation) $normalizedUriVariable = (new Link())->withIdentifiers([$normalizedUriVariable])->withFromClass($resourceClass); } if (\is_array($normalizedUriVariable)) { - if (!isset($normalizedUriVariable['target_class'])) { + if (!isset($normalizedUriVariable['from_class']) && !isset($normalizedUriVariable['expanded_value'])) { if (2 !== \count($normalizedUriVariable)) { throw new \LogicException("The uriVariables shortcut syntax needs to be the tuple: 'uriVariable' => [fromClass, fromProperty]"); } $normalizedUriVariable = (new Link())->withIdentifiers([$normalizedUriVariable[1]])->withFromClass($normalizedUriVariable[0]); } else { - $normalizedUriVariable = new Link(null, $normalizedUriVariable['from_property'] ?? null, $normalizedUriVariable['to_property'] ?? null, $normalizedUriVariable['class'], $normalizedUriVariable['to_class'] ?? null, $normalizedUriVariable['identifiers'] ?? null, $normalizedUriVariable['composite_identifier'] ?? null, $normalizedUriVariable['expanded_value'] ?? null); + $normalizedUriVariable = new Link($normalizedParameterName, $normalizedUriVariable['from_property'] ?? null, $normalizedUriVariable['to_property'] ?? null, $normalizedUriVariable['from_class'] ?? null, $normalizedUriVariable['to_class'] ?? null, $normalizedUriVariable['identifiers'] ?? null, $normalizedUriVariable['composite_identifier'] ?? null, $normalizedUriVariable['expanded_value'] ?? null); } } if (null !== ($hasCompositeIdentifier = $operation->getCompositeIdentifier())) { diff --git a/tests/Core/Bridge/Rector/Service/SubresourceTransformerTest.php b/tests/Core/Bridge/Rector/Service/SubresourceTransformerTest.php index 48e6f58f50e..32b345f4b4f 100644 --- a/tests/Core/Bridge/Rector/Service/SubresourceTransformerTest.php +++ b/tests/Core/Bridge/Rector/Service/SubresourceTransformerTest.php @@ -52,12 +52,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Question::class, 'from_class' => Question::class, - 'inverse_property' => 'answer', 'from_property' => 'answer', 'to_class' => Answer::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -88,24 +85,18 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Question::class, 'from_class' => Question::class, - 'inverse_property' => 'answer', 'from_property' => 'answer', 'to_class' => Answer::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, ], 'answer' => [ - 'target_class' => Answer::class, 'from_class' => Answer::class, - 'inverse_property' => 'relatedQuestions', 'from_property' => 'relatedQuestions', 'to_class' => Question::class, - 'property' => null, 'to_property' => null, 'identifiers' => [], 'composite_identifier' => false, @@ -125,12 +116,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Dummy::class, 'from_class' => Dummy::class, - 'inverse_property' => 'relatedDummies', 'from_property' => 'relatedDummies', 'to_class' => RelatedDummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -153,24 +141,18 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Dummy::class, 'from_class' => Dummy::class, - 'inverse_property' => 'relatedDummies', 'from_property' => 'relatedDummies', 'to_class' => RelatedDummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, ], 'relatedDummies' => [ - 'target_class' => RelatedDummy::class, 'from_class' => RelatedDummy::class, - 'inverse_property' => null, 'from_property' => null, 'to_class' => RelatedDummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -193,24 +175,18 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Dummy::class, 'from_class' => Dummy::class, - 'inverse_property' => 'relatedDummies', 'from_property' => 'relatedDummies', 'to_class' => RelatedDummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, ], 'relatedDummies' => [ - 'target_class' => RelatedDummy::class, 'from_class' => RelatedDummy::class, - 'inverse_property' => 'thirdLevel', 'from_property' => 'thirdLevel', 'to_class' => ThirdLevel::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -234,36 +210,27 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Dummy::class, 'from_class' => Dummy::class, - 'inverse_property' => 'relatedDummies', 'from_property' => 'relatedDummies', 'to_class' => RelatedDummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, ], 'relatedDummies' => [ - 'target_class' => RelatedDummy::class, 'from_class' => RelatedDummy::class, - 'inverse_property' => 'thirdLevel', 'from_property' => 'thirdLevel', 'to_class' => ThirdLevel::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, ], 'thirdLevel' => [ - 'target_class' => ThirdLevel::class, 'from_class' => ThirdLevel::class, - 'inverse_property' => 'fourthLevel', 'from_property' => 'fourthLevel', 'to_class' => FourthLevel::class, - 'property' => null, 'to_property' => null, 'identifiers' => [], 'composite_identifier' => false, @@ -284,17 +251,23 @@ public function toUriVariablesProvider(): \Generator true ] ], - 'path' => '/dummy_products/{id}/offers.{_format}' + 'path' => '/dummy_products/{id}/offers/{offers}/offers.{_format}' ], [ 'id' => [ - 'target_class' => DummyProduct::class, 'from_class' => DummyProduct::class, - 'inverse_property' => null, - 'from_property' => null, - 'to_class' => DummyAggregateOffer::class, - 'property' => 'offers', - 'to_property' => 'offers', + 'to_property' => 'product', + 'to_class' => null, + 'to_property' => null, + 'identifiers' => ['id'], + 'composite_identifier' => false, + 'expanded_value' => null, + ], + 'offers' => [ + 'from_class' => DummyAggregateOffer::class, + 'to_property' => 'aggregate', + 'to_class' => null, + 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, 'expanded_value' => null, @@ -314,12 +287,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => DummyAggregateOffer::class, 'from_class' => DummyAggregateOffer::class, - 'inverse_property' => 'offers', 'from_property' => 'offers', 'to_class' => DummyOffer::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -340,12 +310,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => Person::class, 'from_class' => Person::class, - 'inverse_property' => 'sentGreetings', 'from_property' => 'sentGreetings', 'to_class' => Greeting::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false, @@ -366,12 +333,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => RelatedOwnedDummy::class, 'from_class' => RelatedOwnedDummy::class, - 'inverse_property' => null, 'from_property' => null, 'to_class' => Dummy::class, - 'property' => 'owningDummy', 'to_property' => 'owningDummy', 'identifiers' => ['id'], 'composite_identifier' => false, @@ -392,12 +356,9 @@ public function toUriVariablesProvider(): \Generator ], [ 'id' => [ - 'target_class' => RelatedOwningDummy::class, 'from_class' => RelatedOwningDummy::class, - 'inverse_property' => 'ownedDummy', 'from_property' => 'ownedDummy', 'to_class' => Dummy::class, - 'property' => null, 'to_property' => null, 'identifiers' => ['id'], 'composite_identifier' => false,