Subresource feature /dummy/1/relatedDummies by soyuka · Pull Request #904 · api-platform/core

soyuka · 2017-01-10T15:52:21Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no (should not be any)
Deprecations?	no
Tests pass?	yes
Fixed tickets	#634 - not really a fix but it's a start
License	MIT
Doc PR	/

Note:

~~I didn't implement the DataProvider yet! I'm just proposing those first changes to see if you have comments/improvements on the way I handled the nested behavior!~~

Related issues/PR:

#885
#634

What should this PR end up doing:

Allow /entity/{id}/association operations when a flag on a resource's property is set. For example:

/**
 * @ApiResource
 */
class Dummy {
    /**
     * @ApiProperty(subcollection=true) 
    */
    public $relatedDummies; 
}

curl http://localhost/api/dummy/1/related_dummies

You can chain those and have /api/dummy/1/related_dummies/2/third_levels.

Changes:

Add subcollection (boolean) on the PropertyMetadata
Initialize a new class ApiPlatform\Util\OperationTypes (maybe the path isn't the best one, suggestions?) to ease the handling of operation types
NestedCollectionDataProvider

TODO

path customization through path resolvers (new method no breaks)
it will help refactoring ApiLoader kinda messy right now
query review
tests tests tests

teohhanhui · 2017-01-11T07:53:01Z

src/Metadata/Property/PropertyMetadata.php

    private $identifier;
    private $childInherited;
    private $attributes;
+    private $nestedOperation;


I don't think nestedOperation is a good name, but then again readableLink / writableLink are very confusing names as well...

Perhaps $subcollection / hasSubcollection?

teohhanhui · 2017-01-11T07:53:24Z

src/Bridge/Symfony/Routing/ApiLoader.php

+                }
+
+                $operation = ['method' => 'GET', 'property' => $property];
+                $this->addRoute($routeCollection, $resourceClass, strtolower($property).'_get', $operation, $resourceShortName, OperationTypes::NESTED);


I can imagine the route name as such: api_products_get_images_subcollection

So $operationName should probably be 'get_'.strtolower($property)`.

teohhanhui · 2017-01-11T07:57:25Z

src/Bridge/Symfony/Routing/ApiLoader.php

                '_api_resource_class' => $resourceClass,
-                sprintf('_api_%s_operation_name', $collection ? 'collection' : 'item') => $operationName,
+                sprintf('_api_%s_operation_name', $operationType) => $operationName,
+                '_api_resource_property' => $operation['property'] ?? null,


_api_subcollection_property_name?

soyuka · 2017-01-11T16:53:30Z

Okay, now it does work, I still have to fix the tests but as I made many changes I'd like to know if those are okay / not breaking anything:

Moved normalizeIdentifiers to a new class IdentifiersHelper (ItemDataProvider constructor is impacted)
Changed the ResourceClassResolver->getResourceClass() signature as I needed more than only one class (may be a no-go)

The rest is pretty straightforward:

collection extensions should work on Subcollections
for now subcollections with composite identifiers are not possible
the query works fine for now, but I'd like to avoid a sub-select, I tried different options without success and I'll continue trying ref for myself
it's needed to have the subcollection resource class in the PropertyMetadata or we can't really get what it should be from the serializer/ApiLoader without Doctrine.

Please tell me what you think about the changes in general, is it too complex? Do you see things that can be simplified?

Thanks!

ping @api-platform/core-team

dunglas

I think that there is a flaw in the general design. We should find a way to handle as many level as we need. Introducing a subcollection operation type looks not scalable enough. We should probably try to tweak the current item and collection operations to make them able to be nested.

dunglas · 2017-01-11T18:26:36Z

src/Api/ResourceClassResolverInterface.php

     * @return string
     */
-    public function getResourceClass($value, string $resourceClass = null, bool $strict = false): string;
+    public function getResourceClass($value, array $context = null, bool $strict = false): string;


It's a BC break, maybe should we introduce a new method for that, but we can discuss this specific point later.

dunglas · 2017-01-11T18:28:57Z

src/Bridge/Doctrine/Orm/SubcollectionDataProvider.php

+            $placeholder = ':id_'.$identifier;
+            $expression = sprintf('parentResourceClass.%s = %s', $identifier, $placeholder);
+
+            $where = !$where ? $expression : $with.' AND '.$expression;


$where ? "$with AND $expression" : $expression
?

dunglas · 2017-01-11T18:30:20Z

src/Bridge/Doctrine/Orm/SubcollectionDataProvider.php

+        $where = null;
+        foreach ($identifiers as $identifier => $value) {
+            $placeholder = ':id_'.$identifier;
+            $expression = sprintf('parentResourceClass.%s = %s', $identifier, $placeholder);


I would use string concatenation here:

"parentResourceClass.$identifier = $placeholder"

dunglas · 2017-01-11T18:32:08Z

src/Bridge/Doctrine/Orm/Util/IdentifiersHelper.php

+/**
+ * Tools that helps managing Identifiers (composite or regular) and related where clauses.
+ */
+class IdentifiersHelper


The class should be final, and we probably need to introduce an interface for it too.

For the name, what about IdentifierGenerator or IdentifierManager?

dunglas · 2017-01-11T18:34:49Z

src/Bridge/Doctrine/Orm/Util/IdentifiersHelper.php

+        $identifierValues = [$id];
+        $doctrineMetadataIdentifier = $manager->getClassMetadata($resourceClass)->getIdentifier();
+
+        if (count($doctrineMetadataIdentifier) >= 2) {


2 <= count($doctrineMetadataIdentifier)

dunglas · 2017-01-11T18:39:34Z

src/Bridge/Symfony/Routing/ApiLoader.php

+                }
+
+                $operation = ['method' => 'GET', 'property' => $property, 'subcollection' => $subcollection];
+                $this->addRoute($routeCollection, $resourceClass, 'get_'.strtolower($property), $operation, $resourceShortName, OperationTypes::SUBCOLLECTION);


There is a high risk of conflicts here, the route name must include the name of the parent resource.

dunglas · 2017-01-11T18:41:08Z

src/Bridge/Symfony/Routing/ApiLoader.php

                '_api_resource_class' => $resourceClass,
-                sprintf('_api_%s_operation_name', $collection ? 'collection' : 'item') => $operationName,
+                sprintf('_api_%s_operation_name', $operationType) => $operationName,
+                '_api_subcollection_property_name' => $operation['property'] ?? null,


Wr should not add those keys at all if it's not a sub collection.

dunglas · 2017-01-11T18:43:20Z

src/DataProvider/SubcollectionDataProviderInterface.php

+ *
+ * @author Antoine Bluchet <soyuka@gmail.com>
+ */
+interface SubcollectionDataProviderInterface


I'm not sure that we need a new interface. Can't we refactor the current set of interfaces (and implementations) to allow a recursive pattern like /users/2/posts/1/comments?

We might but the dataprovides should understand that we "need the collection belonging to an item", and that's very different from both of the current implementations IMO.

dunglas · 2017-01-11T20:02:02Z

src/Util/OperationTypes.php

+ * file that was distributed with this source code.
+ */
+
+namespace ApiPlatform\Core\Util;


We should find a better name, maybe can we just move this class in ApiPlatform\Core.

soyuka · 2017-01-11T21:02:21Z

I think that there is a flaw in the general design. We should find a way to handle as many level as we need. Introducing a subcollection operation type looks not scalable enough. We should probably try to tweak the current item and collection operations to make them able to be nested.

Yes I kinda agree with you and you would like to end up with something like:

ItemOperation  CollectionOperation  ItemOperation etc.
/dummy/1       /relatedDummies        /1

I'll try to dig deeper into this but we definitely need a different DataProvider for exactly this kind of operation. Just chaining Item/Collection would be really bad in performances.
Though, I think the new operation type is required because it'll behave differently from the current one in many points (also thinking SRP, I would not want to add complexity to the current operations).

I'll do a separated Pull request tomorrow to get rid of that IdentifierGenerator / IdentifierManager or whatever.

Thoughts for later

loop resource properties
if property has subcollection
  keep track of property + subcollection entity
  loop subcollection properties //recurse
fi

declare operation with array of collections
declare every subcollection independently? Meaning

/foo/1/bar/1/baz

would declare

/foo/{id}/bar
/foo/{foo_id}/bar/{bar_id}/baz

teohhanhui · 2017-01-12T04:33:36Z

@dunglas:

We should find a way to handle as many level as we need.

Let's take your example:

/users/2/posts/1/comments

I think this is how it should be:
If User::$posts is marked as a subcollection and Post::$comments is marked as a subcollection, the above should automagically work.

soyuka · 2017-02-03T17:27:50Z

Please don't mind the last commit and this comment I'm just keeping some notes.

soyuka · 2017-02-06T18:58:12Z

src/Bridge/Doctrine/Orm/SubcollectionDataProvider.php

+
+        $queryBuilder->where(
+            $queryBuilder->expr()->in('o', $previousQueryBuilder->getDQL())
+        );


IMO this is the best way to do what we want because we force the SQL execution plan to go through indexes on doctrine identifiers. The other solution would be to use joins.

This transforms to (pseudo-dql):

SELECT thirdLevel WHERE thirdLevel IN ( SELECT thirdLevel FROM relatedDummies WHERE relatedDummies = ? AND relatedDummies IN ( SELECT relatedDummies FROM Dummy WHERE Dummy = ? ) )

Maybe can you add this as a comment in the code for future readers?

soyuka · 2017-02-06T18:58:18Z

src/Api/ResourceClassResolver.php

        return $resourceClass;
    }

+    /**


Instead of BC break I chose to duplicate the method, I think we should deprecate the old one, let me know if you've a better idea.

In fact I don't need this, back to the original method.

soyuka · 2017-02-06T19:16:02Z

~~@Simperfit any clue on how I broke the DocumentationNormalizer?~~

backbone87 · 2017-04-14T10:24:27Z

Either i am completely missunderstanding this feature, or there are some major flaws in it.

Consider the following classes:

/**
 * @API\ApiResource
 * @ORM\Entity
 */
class Container
{

    /**
     * @ORM\Id
     * @ORM\Column(name="id", type="guid")
     *
     * @var string UUID
     */
    private $id;

    /**
     * @API\ApiProperty(subresource=true)
     * @ORM\OneToMany(
     *      targetEntity="Node",
     *      mappedBy="container",
     *      indexBy="serial",
     *      fetch="LAZY",
     *      cascade={},
     *      orphanRemoval=false
     * )
     * @ORM\OrderBy({"serial"="ASC"})
     *
     * @var Collection|Node[]
     */
    private $nodes;

    /**
     * @return string
     */
    public function getId(): ?string
    {
        return $this->id;
    }

    /**
     * @return array|Node[]
     */
    public function getNodes(): array
    {
        return $this->nodes->toArray();
    }
}

/**
 * @API\ApiResource
 * @ORM\Entity
 */
class Node
{
    /**
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="Container", fetch="LAZY")
     * @ORM\JoinColumn(name="container_id", referencedColumnName="id", onDelete="RESTRICT")
     *
     * @var Container
     */
    private $container;

    /**
     * @ORM\Id
     * @ORM\Column(name="serial", type="integer")
     *
     * @var integer Node serial
     */
    private $serial;

    /**
     */
    public function __construct()
    {
    }

    public function setContainer(Container $container): void
    {
        $this->container= $container;
        $this->serial = $container->nextSerial();
    }

    /**
     *
     * @return integer
     */
    public function getSerial(): int
    {
        return $this->serial;
    }
}

invoking /containers/SOME_UUID/nodes fails because the generated query looks something like this:
SELECT o FROM Node o WHERE o IN(SELECT IDENTITY(id_a1.nodes) FROM Container id_a1 WHERE id_a1.id = :id_p1)

but i would expect a query more like this:
SELECT o FROM Node o WHERE o.container IN (SELECT id_a1.id FROM Container id_a1 WHERE id_a1.id = :id_p1)

any hints?

soyuka · 2017-04-14T12:30:04Z

@backbone87 thanks for your interest in this pull request. I'm currently reproducing your case in an intergration test. It's possible that there are uncovered parts here, because relations between resources can differ a lot according to the use cases.

Either i am completely missunderstanding this feature, or there are some major flaws in it.

Your choice of words is a bit strong here ^^. I don't think you misunderstood the feature:

/containers/SOME_UUID/nodes should get the nodes belonging to the container identified by SOME_UUID.

To me the following DQL looks correct:

SELECT o FROM Node o -- select nodes
WHERE o IN( -- where nodes are
  SELECT IDENTITY(id_a1.nodes) -- the nodes
  FROM Container id_a1
  WHERE id_a1.id = :id_p1  -- belonging to this container 
)

The second one is also valid, but it assumes that there is a two-way relationship (ie container is stored in node).

Anyway, I'm investigating this!

soyuka · 2017-04-14T14:01:32Z

Thanks @backbone87 this was a really interesting case it had:

a OneToMany relation which behavior is slightly different
a composite key

I've fixed your issue and it works great through behat. Could you give it a try? Thanks!

backbone87 · 2017-04-14T14:28:06Z

After digging deeper into the code, idk if this PR uses the right approach.

A configuration scheme i would like to see:

/**
 * @ApiProperty(
 *   resource=@ApiResource(
 *     collectionValued=true|false,
 *     collectionOperations={ ... }, // invalid when collectionValued = false
 *     itemOperations={ ... }
 *   )
 * )
 */
private $nodes;

soyuka · 2017-04-14T14:34:24Z

It's not really necessary to have to specify a subresource Resource because we already know the metadata (if it's a collection or not for example). Also, we would like to keep things simple for the end user.

What bother's you with the current implementation?

Keep in mind that for complex use cases, it'll always be easier to set up filters and custom operations (or a custom data provider).

backbone87 · 2017-04-14T14:37:15Z

Your changes do work with this use case.

Anyway, i think we should aim at getting full operation support (incl custom operations) on subresources and in combination with that: disable a resource being top lvl (or in terms of the given use case: remove /nodes)

backbone87 · 2017-04-14T14:40:27Z

our communication is somewhat deferred ;) i am on gitter, if you want to have chat more directly

another problem with current configuration scheme: i cant name the paths properly (its autogenerated for subresources). Since i have to use german resources names for my current project the english inflector and pluralizer generates completely unuseable routes.

@id

Hydra @id now matches subresource IRI Rebase was hard, added a feature (temp commit)

soyuka · 2017-04-26T14:41:43Z

ping @api-platform/core-team can someone review phpstan fixes in the last commit? Thanks!

dunglas · 2017-04-27T06:17:04Z

src/Api/OperationTypeDeprecationHelper.php

+     */
+    public static function getOperationType($operationType): string
+    {
+        if (is_bool($operationType)) {


To prevent the PHPStan error you may use if( !is_string(($operationType)) {.

hmm okay though it should be correct like this! I'll change it thx.

Okay so I did try !is_string but I get the same error :| I'll just ignore this one for now as this is a temporary class anyway.

dunglas · 2017-06-07T17:40:20Z

Don't forget the doc PR :)

cocciagialla · 2017-07-11T15:16:12Z

@dunglas I am using the annotation @ApiProperty(subresource=true) on a collection inside a Resource. It works well and I can call /myresource/<id>/mysubresourcecollection.
The problem is that in the swagger documentation this new endpoint is not exposed. Do you think it will be added in the future or there is a way to actually expose it? Thanks

soyuka · 2017-07-11T15:29:49Z

Hi @cocciagialla, be careful when you update to the next beta release it'll be @ApiSubresource!

Also, I'm currently working on this in #1245. I suggest that you subscribe to the issue to know when it'll be ready :).

cocciagialla · 2017-07-11T15:42:35Z

@soyuka thanks, I am subscribed to #1245

ghost · 2017-11-07T12:54:11Z

There is a way that we can change the sub-queries made them in SubResourceDataProvider!

Right now it's possbile to change ou well add other conditions to main queryBuilder but the previousBuilder made them into SubResourceDataProvider we can't hook into them with any type of extension?

thanks

…ation Subresource feature /dummy/1/relatedDummies

soyuka force-pushed the nested-data-implementation branch 3 times, most recently from dd79f35 to bc79541 Compare January 10, 2017 16:02

teohhanhui reviewed Jan 11, 2017

View reviewed changes

soyuka force-pushed the nested-data-implementation branch 2 times, most recently from 7279804 to 253e55e Compare January 11, 2017 16:45

dunglas requested changes Jan 11, 2017

View reviewed changes

soyuka force-pushed the nested-data-implementation branch from e94d28f to e381bd9 Compare February 3, 2017 17:24

soyuka force-pushed the nested-data-implementation branch 4 times, most recently from ec8312a to e721183 Compare February 6, 2017 18:55

soyuka commented Feb 6, 2017

View reviewed changes

soyuka force-pushed the nested-data-implementation branch 2 times, most recently from 1eb2e49 to 3a2147c Compare February 6, 2017 19:11

soyuka changed the title ~~[WIP] wip nested data implementation~~ [WIP] Nested data implementation /dummy/1/relatedDummies Feb 6, 2017

soyuka added the enhancement label Feb 6, 2017

soyuka force-pushed the nested-data-implementation branch 7 times, most recently from 4012a95 to c7c98fa Compare February 7, 2017 14:16

soyuka force-pushed the nested-data-implementation branch from a8764a5 to bef19e0 Compare April 14, 2017 14:02

soyuka force-pushed the nested-data-implementation branch from bef19e0 to e50f163 Compare April 14, 2017 14:34

soyuka force-pushed the nested-data-implementation branch 8 times, most recently from 2480c49 to dcf00bc Compare April 20, 2017 17:54

Subresource operations feature

959381b

Hydra @id now matches subresource IRI Rebase was hard, added a feature (temp commit)

soyuka force-pushed the nested-data-implementation branch from dcf00bc to 959381b Compare April 25, 2017 10:01

php stan errors

b78c274

dunglas reviewed Apr 27, 2017

View reviewed changes

soyuka merged commit 4d4b193 into api-platform:master Apr 27, 2017

soyuka deleted the nested-data-implementation branch November 7, 2017 13:56

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#904 from soyuka/nested-data-implement…

3005477

…ation Subresource feature /dummy/1/relatedDummies

Uh oh!

Comments

Conversation

soyuka commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note:

Related issues/PR:

What should this PR end up doing:

Changes:

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Jan 11, 2017

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented Jan 12, 2017

Uh oh!

soyuka commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

backbone87 commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Apr 14, 2017

Uh oh!

soyuka commented Apr 14, 2017

Uh oh!

backbone87 commented Apr 14, 2017

Uh oh!

soyuka commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

backbone87 commented Apr 14, 2017

Uh oh!

backbone87 commented Apr 14, 2017

Uh oh!

soyuka commented Apr 26, 2017

soyuka commented Jan 10, 2017 •

edited

Loading

soyuka commented Jan 11, 2017 •

edited

Loading

soyuka commented Feb 3, 2017 •

edited

Loading

soyuka commented Feb 6, 2017 •

edited

Loading

backbone87 commented Apr 14, 2017 •

edited

Loading

soyuka commented Apr 14, 2017 •

edited

Loading

ghost commented Nov 7, 2017 •

edited by ghost

Loading