Skip to content

Fix 1246: configure default order on ApiResource annotation #1321

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.

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

Merged
merged 2 commits into from
Aug 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions features/bootstrap/FeatureContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\EmbeddableDummy;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\EmbeddedDummy;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Node;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Question;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy;
Expand Down Expand Up @@ -132,6 +133,23 @@ public function thereIsDummyObjects($nb)
$this->manager->flush();
}

/**
* @Given there are :nb foo objects with fake names
*/
public function thereAreFooObjectsWithFakeNames($nb)
{
$names = ['Hawsepipe', 'Sthenelus', 'Ephesian', 'Separativeness', 'Balbo'];

for ($i = 0; $i < $nb; ++$i) {
$foo = new Foo();
$foo->setName($names[$i]);

$this->manager->persist($foo);
}

$this->manager->flush();
}

/**
* @Given there is :nb dummy group objects
*/
Expand Down
57 changes: 57 additions & 0 deletions features/main/default_order.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
Feature: Default order
In order to get a list in a specific order,
As a client software developer,
I need to be able to specify default order.

@createSchema @dropSchema
Scenario: Override custom order
Given there are 5 foo objects with fake names
When I send a "GET" request to "/foos?itemsPerPage=10"
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 equal to:
"""
{
"@context": "/contexts/Foo",
"@id": "/foos",
"@type": "hydra:Collection",
"hydra:member": [
{
"@id": "/foos/2",
"@type": "Foo",
"id": 2,
"name": "Sthenelus"
},
{
"@id": "/foos/4",
"@type": "Foo",
"id": 4,
"name": "Separativeness"
},
{
"@id": "/foos/1",
"@type": "Foo",
"id": 1,
"name": "Hawsepipe"
},
{
"@id": "/foos/3",
"@type": "Foo",
"id": 3,
"name": "Ephesian"
},
{
"@id": "/foos/5",
"@type": "Foo",
"id": 5,
"name": "Balbo"
}
],
"hydra:totalItems": 5,
"hydra:view": {
"@id": "/foos?itemsPerPage=10",
"@type": "hydra:PartialCollectionView"
}
}
"""
14 changes: 13 additions & 1 deletion src/Bridge/Doctrine/Orm/Extension/OrderExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,24 @@
namespace ApiPlatform\Core\Bridge\Doctrine\Orm\Extension;

use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use Doctrine\ORM\QueryBuilder;

/**
* Applies selected ordering while querying resource collection.
*
* @author Kévin Dunglas <[email protected]>
* @author Samuel ROZE <[email protected]>
* @author Vincent Chalamon <[email protected]>
*/
final class OrderExtension implements QueryCollectionExtensionInterface
{
private $order;
private $resourceMetadataFactory;

public function __construct(string $order = null)
public function __construct(string $order = null, ResourceMetadataFactoryInterface $resourceMetadataFactory = null)
{
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->order = $order;
}

Expand All @@ -38,6 +42,14 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
{
$classMetaData = $queryBuilder->getEntityManager()->getClassMetadata($resourceClass);
$identifiers = $classMetaData->getIdentifier();
if (null !== $this->resourceMetadataFactory) {
$defaultOrder = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order');
if (null !== $defaultOrder) {
$queryBuilder->addOrderBy('o.'.$defaultOrder[0], $defaultOrder[1] ?? 'ASC');

return;
}
}

if (null !== $this->order) {
foreach ($identifiers as $identifier) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@

<service id="api_platform.doctrine.orm.query_extension.order" class="ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension" public="false">
<argument>%api_platform.collection.order%</argument>
<argument type="service" id="api_platform.metadata.resource.metadata_factory" />

<tag name="api_platform.doctrine.orm.query_extension.collection" priority="16" />
</service>
Expand Down
46 changes: 46 additions & 0 deletions tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Mapping\ClassMetadata;
Expand Down Expand Up @@ -62,4 +64,48 @@ public function testApplyToCollectionWithWrongOrder()
$orderExtensionTest = new OrderExtension();
$orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class);
}

public function testApplyToCollectionWithOrderOverriden()
{
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$queryBuilderProphecy = $this->prophesize(QueryBuilder::class);

$queryBuilderProphecy->addOrderBy('o.foo', 'DESC')->shouldBeCalled();

$classMetadataProphecy = $this->prophesize(ClassMetadata::class);
$classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn(['name']);

$resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['foo', 'DESC']]));

$emProphecy = $this->prophesize(EntityManager::class);
$emProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal());

$queryBuilderProphecy->getEntityManager()->shouldBeCalled()->willReturn($emProphecy->reveal());

$queryBuilder = $queryBuilderProphecy->reveal();
$orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal());
$orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class);
}

public function testApplyToCollectionWithOrderOverridenWithNoDirection()
{
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$queryBuilderProphecy = $this->prophesize(QueryBuilder::class);

$queryBuilderProphecy->addOrderBy('o.foo', 'ASC')->shouldBeCalled();

$classMetadataProphecy = $this->prophesize(ClassMetadata::class);
$classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn(['name']);

$resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['foo']]));

$emProphecy = $this->prophesize(EntityManager::class);
$emProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal());

$queryBuilderProphecy->getEntityManager()->shouldBeCalled()->willReturn($emProphecy->reveal());

$queryBuilder = $queryBuilderProphecy->reveal();
$orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal());
$orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class);
}
}
2 changes: 1 addition & 1 deletion tests/Fixtures/TestBundle/Entity/Dummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* "my_dummy.order",
* "my_dummy.range",
* "my_dummy.search",
* },
* }
* })
* @ORM\Entity
*/
Expand Down
65 changes: 65 additions & 0 deletions tests/Fixtures/TestBundle/Entity/Foo.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity;

use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;

/**
* Foo.
*
* @author Vincent Chalamon <[email protected]>
*
* @ApiResource(attributes={
* "order"={"name", "DESC"}
Copy link
Member

Choose a reason for hiding this comment

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

I probably have been too fast to merge. Wouldn't be better to allow complex ordering? I suggest the following:

@ApiResource(attributes={"name": "DESC", "firstname": "ASC"})

(Order by name then by firstname). WDYT @vincentchalamon? Can you do a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though about adding the multiple default ordering, but I wasn't sure it was very useful.
I would prefer a syntax like that:

@ApiResource(attributes={
    "order"={"name": "DESC", "firstname": "ASC"}
})

WDYT @dunglas? If so, I could open another PR to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's useful (ordering by name then firstname is very common for instance). You're right for the syntax, my example is faulty (missing order key). A PR would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* })
* @ORM\Entity
*/
class Foo
{
/**
* @var int The id
*
* @ORM\Column(type="integer")
* @ORM\Id
* @ORM\GeneratedValue(strategy="AUTO")
*/
private $id;

/**
* @var string The dummy name
*
* @ORM\Column
* @Assert\NotBlank
* @ApiProperty(iri="http://schema.org/name")
*/
private $name;

public function getId()
{
return $this->id;
}

public function setName($name)
{
$this->name = $name;
}

public function getName()
{
return $this->name;
}
}