Subresource feature /dummy/1/relatedDummies

[soyuka]

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]

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

Perhaps $subcollection / hasSubcollection?

[teohhanhui]

I can imagine the route name as such: api_products_get_images_subcollection

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

[teohhanhui]

_api_subcollection_property_name?

[soyuka]

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:

1. Moved normalizeIdentifiers to a new class IdentifiersHelper (ItemDataProvider constructor is impacted)
2. 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]

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

[dunglas]

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

[dunglas]

I would use string concatenation here:

"parentResourceClass.$identifier = $placeholder"

[dunglas]

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

[dunglas]

For the name, what about IdentifierGenerator or IdentifierManager?

[dunglas]

2 <= count($doctrineMetadataIdentifier)

[dunglas]

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

[dunglas]

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

[dunglas]

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?

[soyuka]

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]

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

[soyuka]

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]

@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]

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

[soyuka]

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 = ?
  )
 )

[dunglas]

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

[soyuka]

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.

[soyuka]

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

[soyuka]

@Simperfit any clue on how I broke the DocumentationNormalizer?

[backbone87]

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]

@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]

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]

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]

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]

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]

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.

[soyuka]

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

[dunglas]

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

[soyuka]

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

[soyuka]

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]

Don't forget the doc PR :)

[cocciagialla]

@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]

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]

@soyuka thanks, I am subscribed to #1245

[ghost]

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