Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix Doctrine query for nested subresources #1608

Merged
merged 7 commits into from
Dec 29, 2017
Merged

Fix Doctrine query for nested subresources #1608

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
620637b
Add a Fourth Level Entity in fixtures
alanpoulain Dec 26, 2017
8c705b7
Add the fourth level to the third level
alanpoulain Dec 26, 2017
c2cbed3
Add the breaking scenario
alanpoulain Dec 26, 2017
3ef7c9e
Fix the query build of recursive subresources
alanpoulain Dec 26, 2017
5b23e18
Remove useless strategy in GeneratedValue annotation
alanpoulain Dec 26, 2017
f71432d
Useless new lines
alanpoulain Dec 27, 2017
1a6aa73
Use comparison instead of negation
alanpoulain Dec 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions features/main/relation.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Feature: Relations support
"@context": "/contexts/ThirdLevel",
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": null,
"id": 1,
"level": 3,
"test": true
Expand Down Expand Up @@ -64,7 +65,11 @@ Feature: Relations support
"name": null,
"symfony": "symfony",
"dummyDate": null,
"thirdLevel": "/third_levels/1",
"thirdLevel": {
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": null
},
"relatedToDummyFriend": [],
"dummyBoolean": null,
"embeddedDummy": null,
Expand Down Expand Up @@ -258,7 +263,8 @@ Feature: Relations support
"thirdLevel": {
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"level": 3
"level": 3,
"fourthLevel": null
}
}
}
Expand Down
92 changes: 71 additions & 21 deletions features/main/subresource.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ Feature: Subresource support
And the JSON should be equal to:
"""
{
"@context": "/contexts/Answer",
"@id": "/answers/1",
"@type": "Answer",
"id": 1,
"content": "42",
"question": "/questions/1",
"relatedQuestions": [
"/questions/1"
]
"@context": "/contexts/Answer",
"@id": "/answers/1",
"@type": "Answer",
"id": 1,
"content": "42",
"question": "/questions/1",
"relatedQuestions": [
"/questions/1"
]
}
"""

Expand All @@ -35,23 +35,43 @@ Feature: Subresource support
"@id": "/questions/1/answer/related_questions",
"@type": "hydra:Collection",
"hydra:member": [
{
"@id": "/questions/1",
"@type": "Question",
"content": "What's the answer to the Ultimate Question of Life, the Universe and Everything?",
"id": 1,
"answer": "/answers/1"
}
{
"@id": "/questions/1",
"@type": "Question",
"content": "What's the answer to the Ultimate Question of Life, the Universe and Everything?",
"id": 1,
"answer": "/answers/1"
}
],
"hydra:totalItems": 1
}
"""

Scenario: Create a fourth level
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/fourth_levels" with body:
"""
{"level": 4}
"""
Then the response status code should be 201
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/FourthLevel",
"@id": "/fourth_levels/1",
"@type": "FourthLevel",
"id": 1,
"level": 4
}
"""

Scenario: Create a third level
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/third_levels" with body:
"""
{"level": 3}
{"level": 3, "fourthLevel": "/fourth_levels/1"}
"""
Then the response status code should be 201
And the response should be in JSON
Expand All @@ -62,6 +82,7 @@ Feature: Subresource support
"@context": "/contexts/ThirdLevel",
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": "/fourth_levels/1",
"id": 1,
"level": 3,
"test": true
Expand Down Expand Up @@ -125,7 +146,11 @@ Feature: Subresource support
"name": "Hello",
"symfony": "symfony",
"dummyDate": null,
"thirdLevel": "/third_levels/1",
"thirdLevel": {
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": "/fourth_levels/1"
},
"relatedToDummyFriend": [],
"dummyBoolean": null,
"embeddedDummy": [],
Expand All @@ -138,7 +163,11 @@ Feature: Subresource support
"name": null,
"symfony": "symfony",
"dummyDate": null,
"thirdLevel": "/third_levels/1",
"thirdLevel": {
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": "/fourth_levels/1"
},
"relatedToDummyFriend": [],
"dummyBoolean": null,
"embeddedDummy": [],
Expand Down Expand Up @@ -193,7 +222,11 @@ Feature: Subresource support
"name": "Hello",
"symfony": "symfony",
"dummyDate": null,
"thirdLevel": "/third_levels/1",
"thirdLevel": {
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": "/fourth_levels/1"
},
"relatedToDummyFriend": [],
"dummyBoolean": null,
"embeddedDummy": [],
Expand Down Expand Up @@ -233,7 +266,7 @@ Feature: Subresource support
}
"""

Scenario: Get the embedded relation collection
Scenario: Get the embedded relation collection 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
Expand All @@ -244,12 +277,29 @@ Feature: Subresource support
"@context": "/contexts/ThirdLevel",
"@id": "/third_levels/1",
"@type": "ThirdLevel",
"fourthLevel": "/fourth_levels/1",
"id": 1,
"level": 3,
"test": true
}
"""

Scenario: Get the embedded relation collection 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
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
And the JSON should be equal to:
"""
{
"@context": "/contexts/FourthLevel",
"@id": "/fourth_levels/1",
"@type": "FourthLevel",
"id": 1,
"level": 4
}
"""

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"
Expand Down
152 changes: 76 additions & 76 deletions src/Bridge/Doctrine/Orm/SubresourceDataProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\QueryBuilder;

/**
* Subresource data provider for the Doctrine ORM.
Expand Down Expand Up @@ -79,82 +80,10 @@ public function getSubresource(string $resourceClass, array $identifiers, array
throw new ResourceClassNotSupportedException('The given resource class is not a subresource.');
}

$originAlias = 'o';
$queryBuilder = $repository->createQueryBuilder($originAlias);
$queryNameGenerator = new QueryNameGenerator();
$previousQueryBuilder = null;
$previousAlias = null;

$num = \count($context['identifiers']);

while ($num--) {
list($identifier, $identifierResourceClass) = $context['identifiers'][$num];
$previousAssociationProperty = $context['identifiers'][$num + 1][0] ?? $context['property'];

$manager = $this->managerRegistry->getManagerForClass($identifierResourceClass);

if (!$manager instanceof EntityManagerInterface) {
throw new RuntimeException("The manager for $identifierResourceClass must be an EntityManager.");
}

$classMetadata = $manager->getClassMetadata($identifierResourceClass);

if (!$classMetadata instanceof ClassMetadataInfo) {
throw new RuntimeException("The class metadata for $identifierResourceClass must be an instance of ClassMetadataInfo.");
}

$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'];

// first pass, o.property instead of alias.property
if (null === $previousQueryBuilder) {
$originAlias = "$originAlias.$mappedBy";
} else {
$previousAlias = "$previousAlias.$mappedBy";
}

$qb->select($alias)
->from($identifierResourceClass, $alias);
break;
default:
$qb->select("IDENTITY($alias.$previousAssociationProperty)")
->from($identifierResourceClass, $alias);
}

// Add where clause for identifiers
foreach ($normalizedIdentifiers as $key => $value) {
$placeholder = $queryNameGenerator->generateParameterName($key);
$qb->andWhere("$alias.$key = :$placeholder");
$queryBuilder->setParameter($placeholder, $value);
}

// recurse queries
if (null === $previousQueryBuilder) {
$previousQueryBuilder = $qb;
} else {
$previousQueryBuilder->andWhere($qb->expr()->in($previousAlias, $qb->getDQL()));
}
Copy link
Member

Choose a reason for hiding this comment

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

this is recursive already no? Not sure what was going on wrong here 😸

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this was not recursive (despite the comment!).

Copy link
Member

Choose a reason for hiding this comment

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

Should've been ? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! :D


$previousAlias = $alias;
}

/*
* The following translate to this pseudo-dql:
* The following recursively translates to this pseudo-dql:
*
* SELECT thirdLevel WHERE thirdLevel IN (
* SELECT thirdLevel FROM relatedDummies WHERE relatedDummies = ? AND relatedDummies IN (
Expand All @@ -164,9 +93,7 @@ public function getSubresource(string $resourceClass, array $identifiers, array
*
* By using subqueries, we're forcing the SQL execution plan to go through indexes on doctrine identifiers.
*/
$queryBuilder->where(
$queryBuilder->expr()->in($originAlias, $previousQueryBuilder->getDQL())
);
$queryBuilder = $this->buildQuery($identifiers, $context, $queryNameGenerator, $repository->createQueryBuilder($alias = 'o'), $alias, \count($context['identifiers']));

if (true === $context['collection']) {
foreach ($this->collectionExtensions as $extension) {
Expand Down Expand Up @@ -195,4 +122,77 @@ public function getSubresource(string $resourceClass, array $identifiers, array

return $context['collection'] ? $query->getResult() : $query->getOneOrNullResult();
}

/**
* @throws RuntimeException
*/
private function buildQuery(array $identifiers, array $context, QueryNameGenerator $queryNameGenerator, QueryBuilder $previousQueryBuilder, string $previousAlias, int $remainingIdentifiers, QueryBuilder $topQueryBuilder = null): QueryBuilder
{
if (!$remainingIdentifiers) {
Copy link
Member

Choose a reason for hiding this comment

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

0 < --$remainingIdentifiers then we can avoid -- below and the $remainingIdentifiers - 1 operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's clearer this way. Because if -- is done here it's more the position of the identifier and not the remaining ones. And you have to add the operation $remainingIdentifiers + 1 where there is $remainingIdentifiers. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

ok then maybe 0 === $remainingIdentifiers instead of !?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you prefer. Then not 0 >= $remainingIdentifiers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or even $remainingIdentifiers <= 0 (I don't really like Yoda style for comparison)?

Copy link
Member

Choose a reason for hiding this comment

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

Dunno it's fine like this as well.

return $previousQueryBuilder;
}

$topQueryBuilder = $topQueryBuilder ?? $previousQueryBuilder;

list($identifier, $identifierResourceClass) = $context['identifiers'][$remainingIdentifiers - 1];
$previousAssociationProperty = $context['identifiers'][$remainingIdentifiers][0] ?? $context['property'];

$manager = $this->managerRegistry->getManagerForClass($identifierResourceClass);

if (!$manager instanceof EntityManagerInterface) {
throw new RuntimeException("The manager for $identifierResourceClass must be an EntityManager.");
}

$classMetadata = $manager->getClassMetadata($identifierResourceClass);

if (!$classMetadata instanceof ClassMetadataInfo) {
throw new RuntimeException(
"The class metadata for $identifierResourceClass must be an instance of ClassMetadataInfo."
);
}

$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);

Copy link
Member

Choose a reason for hiding this comment

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

for cosmetics, new line is useless here imo \o/

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure :)

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);
}

// Add where clause for identifiers
foreach ($normalizedIdentifiers as $key => $value) {
$placeholder = $queryNameGenerator->generateParameterName($key);
$qb->andWhere("$alias.$key = :$placeholder");
$topQueryBuilder->setParameter($placeholder, $value);
}

// Recurse queries
$qb = $this->buildQuery($identifiers, $context, $queryNameGenerator, $qb, $alias, --$remainingIdentifiers, $topQueryBuilder);

return $previousQueryBuilder->andWhere($qb->expr()->in($previousAlias, $qb->getDQL()));
}
}
Loading