Subresource feature /dummy/1/relatedDummies #904

Merged
merged 2 commits into from Apr 27, 2017

Conversation

Projects
None yet
8 participants
@soyuka
Member

soyuka commented Jan 10, 2017

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
@@ -31,8 +31,9 @@
private $identifier;
private $childInherited;
private $attributes;
+ private $nestedOperation;

This comment has been minimized.

@teohhanhui

teohhanhui Jan 11, 2017

Member

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

Perhaps $subcollection / hasSubcollection?

@teohhanhui

teohhanhui Jan 11, 2017

Member

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

Perhaps $subcollection / hasSubcollection?

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

This comment has been minimized.

@teohhanhui

teohhanhui Jan 11, 2017

Member

I can imagine the route name as such: api_products_get_images_subcollection

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

@teohhanhui

teohhanhui Jan 11, 2017

Member

I can imagine the route name as such: api_products_get_images_subcollection

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

src/Bridge/Symfony/Routing/ApiLoader.php
@@ -156,7 +173,8 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas
'_controller' => $controller,
'_format' => null,
'_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,

This comment has been minimized.

@teohhanhui

teohhanhui Jan 11, 2017

Member

_api_subcollection_property_name?

@teohhanhui

teohhanhui Jan 11, 2017

Member

_api_subcollection_property_name?

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Jan 11, 2017

Member

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

Member

soyuka commented Jan 11, 2017

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.

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

I would use string concatenation here:

"parentResourceClass.$identifier = $placeholder"

@dunglas

dunglas Jan 11, 2017

Member

I would use string concatenation here:

"parentResourceClass.$identifier = $placeholder"

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

For the name, what about IdentifierGenerator or IdentifierManager?

@dunglas

dunglas Jan 11, 2017

Member

For the name, what about IdentifierGenerator or IdentifierManager?

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

2 <= count($doctrineMetadataIdentifier)

@dunglas

dunglas Jan 11, 2017

Member

2 <= count($doctrineMetadataIdentifier)

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

src/Bridge/Symfony/Routing/ApiLoader.php
@@ -156,7 +173,9 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas
'_controller' => $controller,
'_format' => null,
'_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,

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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?

@dunglas

dunglas Jan 11, 2017

Member

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?

This comment has been minimized.

@soyuka

soyuka Jan 11, 2017

Member

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.

@soyuka

soyuka Jan 11, 2017

Member

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.

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

This comment has been minimized.

@dunglas

dunglas Jan 11, 2017

Member

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

@dunglas

dunglas Jan 11, 2017

Member

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

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Jan 11, 2017

Member

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
Member

soyuka commented Jan 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Jan 12, 2017

Member

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

Member

teohhanhui commented Jan 12, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 3, 2017

Member

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

Member

soyuka commented Feb 3, 2017

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

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

This comment has been minimized.

@soyuka

soyuka Feb 6, 2017

Member

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

soyuka Feb 6, 2017

Member

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

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

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

@dunglas

dunglas Feb 15, 2017

Member

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

src/Api/ResourceClassResolver.php
@@ -63,6 +63,39 @@ public function getResourceClass($value, string $resourceClass = null, bool $str
+ /**
+ * {@inheritdoc}
+ */
+ public function getResourceClassFromContext($value, array $context = null, bool $strict = false): string

This comment has been minimized.

@soyuka

soyuka Feb 6, 2017

Member

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

soyuka Feb 6, 2017

Member

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.

This comment has been minimized.

@soyuka

soyuka Feb 7, 2017

Member

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

@soyuka

soyuka Feb 7, 2017

Member

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

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 6, 2017

Member

@Simperfit any clue on how I broke the DocumentationNormalizer?

Member

soyuka commented Feb 6, 2017

@Simperfit any clue on how I broke the DocumentationNormalizer?

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

@soyuka soyuka added the enhancement label Feb 6, 2017

@soyuka soyuka changed the title from [WIP] Nested data implementation /dummy/1/relatedDummies to Nested data implementation /dummy/1/relatedDummies Feb 7, 2017

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 7, 2017

Member

All green RFR ping @api-platform/core-team

Member

soyuka commented Feb 7, 2017

All green RFR ping @api-platform/core-team

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Feb 7, 2017

Member

@soyuka I will take some time to review this and thanks, that's a great improvement IMO, could be very useful ;).

Member

Simperfit commented Feb 7, 2017

@soyuka I will take some time to review this and thanks, that's a great improvement IMO, could be very useful ;).

features/main/embedded.feature
+ I need to be able to retrieve embedded resources only
+
+ @dropSchema
+ Scenario: Just delete previous stuff pls

This comment has been minimized.

@Simperfit

Simperfit Feb 7, 2017

Member

Do you really need that ?

@Simperfit

Simperfit Feb 7, 2017

Member

Do you really need that ?

This comment has been minimized.

@soyuka

soyuka Feb 7, 2017

Member

Forgot to remove it it's easier when you want to launch just that feature

@soyuka

soyuka Feb 7, 2017

Member

Forgot to remove it it's easier when you want to launch just that feature

+ private $managerRegistry;
+ private $collectionExtensions;
+
+ /**

This comment has been minimized.

@Simperfit

Simperfit Feb 7, 2017

Member

You may not need phpdoc in this case

@Simperfit

Simperfit Feb 7, 2017

Member

You may not need phpdoc in this case

@Simperfit

This will need a big Pr on the documentation IMO.

+
+ $num = count($context['identifiers']);
+
+ while ($num--) {

This comment has been minimized.

@Simperfit

Simperfit Feb 9, 2017

Member

It feels like a bit complicated, could it be split ?

@Simperfit

Simperfit Feb 9, 2017

Member

It feels like a bit complicated, could it be split ?

This comment has been minimized.

@soyuka

soyuka Feb 9, 2017

Member

Because of the while ? 😄

It is complicated because we're doing a recursive query where each previous part is included in the next one:

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

and

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

The rest is pretty straightforward, we first select the data we need, than we're adding identifiers to the where clause.

@soyuka

soyuka Feb 9, 2017

Member

Because of the while ? 😄

It is complicated because we're doing a recursive query where each previous part is included in the next one:

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

and

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

The rest is pretty straightforward, we first select the data we need, than we're adding identifiers to the where clause.

This comment has been minimized.

@Simperfit

Simperfit Feb 16, 2017

Member

👍

src/Bridge/Symfony/Routing/ApiLoader.php
+ $rootShortname = $rootResourceMetadata->getShortName();
+ $resourceRouteName = Inflector::pluralize(Inflector::tableize($rootShortname));
+
+ $operation['identifiers'] = [['id', $rootResourceClass]];

This comment has been minimized.

@Simperfit

Simperfit Feb 9, 2017

Member
$operation['identifiers'][] = ['id', $rootResourceClass];
@Simperfit

Simperfit Feb 9, 2017

Member
$operation['identifiers'][] = ['id', $rootResourceClass];

This comment has been minimized.

@soyuka

soyuka Feb 9, 2017

Member

At this point $operation['identifiers'] is not declared.

@soyuka

soyuka Feb 9, 2017

Member

At this point $operation['identifiers'] is not declared.

+/**
+ * Tries each configured data provider and returns the result of the first able to handle the resource class.
+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>

This comment has been minimized.

@Simperfit

Simperfit Feb 9, 2017

Member

Not you :p ?

@Simperfit

Simperfit Feb 9, 2017

Member

Not you :p ?

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 9, 2017

Member

Not really, look at the first post in this pull request, the feature is easy to use:


/**
 * @ApiResource
 */
class Dummy {
    /**
     * @ApiProperty(subcollection=true) 
    */
    public $relatedDummies; 
}
curl http://localhost/api/dummy/1/related_dummies
Member

soyuka commented Feb 9, 2017

Not really, look at the first post in this pull request, the feature is easy to use:


/**
 * @ApiResource
 */
class Dummy {
    /**
     * @ApiProperty(subcollection=true) 
    */
    public $relatedDummies; 
}
curl http://localhost/api/dummy/1/related_dummies
@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Feb 13, 2017

Member

@soyuka

subcollection="RelatedDummy"

Why? Isn't the collection value type already available from the PropertyMetadata? I haven't gone through the code yet, but this feels unintuitive...

Member

teohhanhui commented Feb 13, 2017

@soyuka

subcollection="RelatedDummy"

Why? Isn't the collection value type already available from the PropertyMetadata? I haven't gone through the code yet, but this feels unintuitive...

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 13, 2017

Member

Wow your're right. I should also check that isCollection is true. Let me change this, it'll simplify things. Thanks.

Member

soyuka commented Feb 13, 2017

Wow your're right. I should also check that isCollection is true. Let me change this, it'll simplify things. Thanks.

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Feb 13, 2017

Member

Do filters work on the subcollection? 😄 Or is that out of scope?

Member

teohhanhui commented Feb 13, 2017

Do filters work on the subcollection? 😄 Or is that out of scope?

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 13, 2017

Member

I've added the collection extensions, it should work out of the box. I'll add a test though.

For now only the get operation is supported.

Member

soyuka commented Feb 13, 2017

I've added the collection extensions, it should work out of the box. I'll add a test though.

For now only the get operation is supported.

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 13, 2017

Member

@teohhanhui filters did work, and now it's tested.

Note:

On subcollections, filters are the one from the retrieved sub collection, not those from the original entity. For example on /dummies/1/relatedDummies, the filters must be declared on the RelatedDummies resource.

Member

soyuka commented Feb 13, 2017

@teohhanhui filters did work, and now it's tested.

Note:

On subcollections, filters are the one from the retrieved sub collection, not those from the original entity. For example on /dummies/1/relatedDummies, the filters must be declared on the RelatedDummies resource.

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Feb 13, 2017

Member
Member

teohhanhui commented Feb 13, 2017

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 13, 2017

Member

@dunglas you know what's wrong with appveyor? :| Seems it can't install php anymore

Member

soyuka commented Feb 13, 2017

@dunglas you know what's wrong with appveyor? :| Seems it can't install php anymore

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Feb 15, 2017

Member

Is it possible to disable the generic collection GET operation (/related_dummies) while still having /dummies/1/related_dummies work?

Member

teohhanhui commented Feb 15, 2017

Is it possible to disable the generic collection GET operation (/related_dummies) while still having /dummies/1/related_dummies work?

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 15, 2017

Member

@teohhanhui I don't see why it would not be possible. The computeSubCollectionOperations is done independently from other operations in ApiLoader.
So yes, here it's possible.

Member

soyuka commented Feb 15, 2017

@teohhanhui I don't see why it would not be possible. The computeSubCollectionOperations is done independently from other operations in ApiLoader.
So yes, here it's possible.

src/Util/OperationTypes.php
+
+namespace ApiPlatform\Core\Util;
+
+final class OperationTypes

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

@teohhanhui WDYT about the name? Do you agree we should put this in ApiPlatform\Core?

@soyuka

soyuka Feb 15, 2017

Member

@teohhanhui WDYT about the name? Do you agree we should put this in ApiPlatform\Core?

This comment has been minimized.

@teohhanhui

teohhanhui Feb 15, 2017

Member

Perhaps ApiPlatform\Core\Api\OperationType?

Singular, like https://github.com/giggsey/libphonenumber-for-php/blob/master/src/PhoneNumberType.php

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

ApiPlatform\Core\Api\OperationType looks nice to me! @dunglas do you approve?

@soyuka

soyuka Feb 15, 2017

Member

ApiPlatform\Core\Api\OperationType looks nice to me! @dunglas do you approve?

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Yep!

+
+ $manager = $this->managerRegistry->getManagerForClass($identifierResourceClass);
+
+ if (!($manager instanceof EntityManagerInterface)) {

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Useless parenthesis

@dunglas

dunglas Feb 15, 2017

Member

Useless parenthesis

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

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

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

@dunglas

dunglas Feb 15, 2017

Member

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

src/Bridge/Symfony/Routing/ApiLoader.php
@@ -34,19 +37,24 @@
{
const ROUTE_NAME_PREFIX = 'api_';
const DEFAULT_ACTION_PATTERN = 'api_platform.action.';
+ const SUBCOLLECTION_SUFFIX = '_get_subcollection';

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Maybe should it not include the _get part (in the case we want to support other operations in the future)?

@dunglas

dunglas Feb 15, 2017

Member

Maybe should it not include the _get part (in the case we want to support other operations in the future)?

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

Indeed I did that on purpose because we only support get for now. It's better hardcoded here then below IMO.

@soyuka

soyuka Feb 15, 2017

Member

Indeed I did that on purpose because we only support get for now. It's better hardcoded here then below IMO.

src/Bridge/Symfony/Routing/ApiLoader.php
private $resourceNameCollectionFactory;
private $resourceMetadataFactory;
private $operationPathResolver;
private $container;
private $formats;
- public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats)
+ public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats)

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

It's a BC break, it should be the last arguments and optional (an error can be thrown if using subcollections without having the proper dependencies).

@dunglas

dunglas Feb 15, 2017

Member

It's a BC break, it should be the last arguments and optional (an error can be thrown if using subcollections without having the proper dependencies).

src/Bridge/Symfony/Routing/ApiLoader.php
+
+ $subcollection = $propertyMetadata->getType()->isCollection() ? $propertyMetadata->getType()->getCollectionValueType()->getClassName() : $propertyMetadata->getType()->getClassName();
+
+ $propertyName = Inflector::pluralize(Inflector::tableize($property));

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Shouldn't we use the configured strategy?

@dunglas

dunglas Feb 15, 2017

Member

Shouldn't we use the configured strategy?

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

Was was the strategy you had in mind?

@soyuka

soyuka Feb 15, 2017

Member

Was was the strategy you had in mind?

src/Util/OperationTypes.php
+
+namespace ApiPlatform\Core\Util;
+
+final class OperationTypes

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Yep!

src/Util/OperationTypes.php
+
+final class OperationTypes
+{
+ const ITEM = 'item';

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Why not using int as values (less memory usage)? Do we use those const in YAML?

@dunglas

dunglas Feb 15, 2017

Member

Why not using int as values (less memory usage)? Do we use those const in YAML?

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

We might do this with int values indeed. No need for those in YAML.

@soyuka

soyuka Feb 15, 2017

Member

We might do this with int values indeed. No need for those in YAML.

This comment has been minimized.

@teohhanhui

teohhanhui Feb 15, 2017

Member

Is it really necessary to optimize this? Is it a tangible difference?

Doesn't it make stack traces harder to read?

@teohhanhui

teohhanhui Feb 15, 2017

Member

Is it really necessary to optimize this? Is it a tangible difference?

Doesn't it make stack traces harder to read?

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Tangible I don't think so. But it's common convention to use numbers for constants no? Probably my C background...

@dunglas

dunglas Feb 15, 2017

Member

Tangible I don't think so. But it's common convention to use numbers for constants no? Probably my C background...

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

Strings are making making things easier too do:

foreach (OperationTypes::TYPES as $operationType) {
+            $attribute = "_api_{$operationType}_operation_name";

I think it's fine like this but if we want integers we need to change this now or it might be painful later \o/.

/edit: I mean if TYPES is a dictionary for the declared types constants.

@soyuka

soyuka Feb 15, 2017

Member

Strings are making making things easier too do:

foreach (OperationTypes::TYPES as $operationType) {
+            $attribute = "_api_{$operationType}_operation_name";

I think it's fine like this but if we want integers we need to change this now or it might be painful later \o/.

/edit: I mean if TYPES is a dictionary for the declared types constants.

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Ok keep the string.

@dunglas

dunglas Feb 15, 2017

Member

Ok keep the string.

src/Util/RequestAttributesExtractor.php
- $itemOperationName = $request->attributes->get('_api_item_operation_name');
+ $hasRequestAttributeKey = false;
+ foreach (OperationTypes::TYPES as $operationType) {
+ $attribute = "_api_{$operationType}_operation_name";

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

src/Util/RequestAttributesExtractor.php
+ foreach (OperationTypes::TYPES as $operationType) {
+ $attribute = "_api_{$operationType}_operation_name";
+ if ($request->attributes->has($attribute)) {
+ $result["{$operationType}_operation_name"] = $request->attributes->get($attribute);

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

src/Util/RequestAttributesExtractor.php
- throw new RuntimeException('One of the request attribute "_api_collection_operation_name" or "_api_item_operation_name" must be defined.');
+ if (false === $hasRequestAttributeKey) {
+ throw new RuntimeException('One of the following request attribute must be defined: '.implode(', ', array_map(function ($operationType) {
+ return "_api_{$operationType}_operation_name";

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

@dunglas

dunglas Feb 15, 2017

Member

Brackets can be removed.

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 15, 2017

Member

Fixed the comments except for the brackets part. May you check that the deprecation warnings are correct? Thanks!

Member

soyuka commented Feb 15, 2017

Fixed the comments except for the brackets part. May you check that the deprecation warnings are correct? Thanks!

src/Bridge/Symfony/Routing/ApiLoader.php
+ $this->propertyMetadataFactory = $propertyMetadataFactory;
+
+ if (null === $this->propertyNameCollectionFactory || null === $this->propertyMetadataFactory) {
+ @trigger_error('Missing dependencies to compute subcollection operations.', E_USER_DEPRECATED);

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

I would say Not injecting "PropertyNameCollectionFactoryInterface" and "PropertyMetadataFactoryInterface" instances in __CLASS__ is deprecrated since API Platform 2.3 and will not be possible anymore in API Platform 3.

@dunglas

dunglas Feb 15, 2017

Member

I would say Not injecting "PropertyNameCollectionFactoryInterface" and "PropertyMetadataFactoryInterface" instances in __CLASS__ is deprecrated since API Platform 2.3 and will not be possible anymore in API Platform 3.

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

Okay thanks!

@soyuka

soyuka Feb 15, 2017

Member

Okay thanks!

src/EventListener/ReadListener.php
+ $this->subcollectionDataProvider = $subcollectionDataProvider;
+
+ if (null === $subcollectionDataProvider) {
+ @trigger_error('No subcollection data provider given', E_USER_DEPRECATED);

This comment has been minimized.

@dunglas

dunglas Feb 15, 2017

Member

Same here.

@dunglas

dunglas Feb 15, 2017

Member

Same here.

src/EventListener/ReadListener.php
+ {
+ if (null === $this->subcollectionDataProvider) {
+ throw new RuntimeException('No subcollection data provider.');
+ }

This comment has been minimized.

@soyuka

soyuka Feb 15, 2017

Member

@dunglas should I throw this here?

In ApiLoader I just fail silently but here I wasn't sure.

@soyuka

soyuka Feb 15, 2017

Member

@dunglas should I throw this here?

In ApiLoader I just fail silently but here I wasn't sure.

@lbarulski

This comment has been minimized.

Show comment
Hide comment
@lbarulski

lbarulski Feb 15, 2017

@soyuka will it support one-to-one relation?

/question/{question.id}/answer - one answer for one question

lbarulski commented Feb 15, 2017

@soyuka will it support one-to-one relation?

/question/{question.id}/answer - one answer for one question

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 15, 2017

Member

@lbarulski they should yes. It's fairly rare to have one to one relations though, if you've an example I could add another test.

By example I mean some entity code, we've none in our current features \o/

Member

soyuka commented Feb 15, 2017

@lbarulski they should yes. It's fairly rare to have one to one relations though, if you've an example I could add another test.

By example I mean some entity code, we've none in our current features \o/

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 15, 2017

Member

Okay so OneToOne relations do work. But:

  1. I had to inverse the relation you gave me to have /questions/1/answers. Indeed, we can not identify the question.answer if it's not this side that has the identity column.
  2. Because I had subcollections in mind, the returning result will be a collection. Though on a OneToOne relation we know that we expect only one result. Therefore it should be an Item. This is wrong IMO in terms of metadata.
  3. The url, as shown above has answers plural, which is again wrong for this particular use case.

I didn't wanted to support a kinda SubItem thing because I don't think it's worth it. With filters you could have almost the same result through the origin item, here Question, really easily.

So, what should we do about this?

  • Option 1: Prevent subcollections on OneToOne relations (ie throw an exception).
  • Option 2: Support subitem retrieval to have a clean url and proper Item metadata (eg hydra item vs collection) + Option 1
  • Option 3 : Do nothing, OneToOne works (only one way and throws a weird error that will make beginners panic the other way) but returns a collection.

I'm not publishing the behat test yet but it's done thanks for the example classes @lbarulski .

Member

soyuka commented Feb 15, 2017

Okay so OneToOne relations do work. But:

  1. I had to inverse the relation you gave me to have /questions/1/answers. Indeed, we can not identify the question.answer if it's not this side that has the identity column.
  2. Because I had subcollections in mind, the returning result will be a collection. Though on a OneToOne relation we know that we expect only one result. Therefore it should be an Item. This is wrong IMO in terms of metadata.
  3. The url, as shown above has answers plural, which is again wrong for this particular use case.

I didn't wanted to support a kinda SubItem thing because I don't think it's worth it. With filters you could have almost the same result through the origin item, here Question, really easily.

So, what should we do about this?

  • Option 1: Prevent subcollections on OneToOne relations (ie throw an exception).
  • Option 2: Support subitem retrieval to have a clean url and proper Item metadata (eg hydra item vs collection) + Option 1
  • Option 3 : Do nothing, OneToOne works (only one way and throws a weird error that will make beginners panic the other way) but returns a collection.

I'm not publishing the behat test yet but it's done thanks for the example classes @lbarulski .

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Feb 15, 2017

Member

1-1 relations can already be handled thru embedding. I'll be for explicitly excluding them (throw).

Member

dunglas commented Feb 15, 2017

1-1 relations can already be handled thru embedding. I'll be for explicitly excluding them (throw).

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Feb 16, 2017

Member

Don't throw. The subcollection attribute shall have no effect when set on a to-one relation. 😄

Member

teohhanhui commented Feb 16, 2017

Don't throw. The subcollection attribute shall have no effect when set on a to-one relation. 😄

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 16, 2017

Member

@teohhanhui So you'd say that they should not be allowed when creating routes in ApiLoader for example? How can I detect the OneToOne relation? I wanted to use PropertyMetadata->isCollection(), but if I do so I also need to do something about ManyToOne relations.

Member

soyuka commented Feb 16, 2017

@teohhanhui So you'd say that they should not be allowed when creating routes in ApiLoader for example? How can I detect the OneToOne relation? I wanted to use PropertyMetadata->isCollection(), but if I do so I also need to do something about ManyToOne relations.

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Feb 24, 2017

Member

@lbarulski
@samusenkoiv

It would be lovely if you could try this feature and tell me if it fits your needs or if you've issues/improvements to add :).

Member

soyuka commented Feb 24, 2017

@lbarulski
@samusenkoiv

It would be lovely if you could try this feature and tell me if it fits your needs or if you've issues/improvements to add :).

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Mar 21, 2017

Member

@dunglas Constructor argument have been covered for BC breaks in this pull request. However, as I named $operationType (and added constants etc.), I've changed a lot of function that were using isCollection with a boolean. Now it's a string representing the type being used.

Member

soyuka commented Mar 21, 2017

@dunglas Constructor argument have been covered for BC breaks in this pull request. However, as I named $operationType (and added constants etc.), I've changed a lot of function that were using isCollection with a boolean. Now it's a string representing the type being used.

@soyuka soyuka added this to the 2.1.0 milestone Mar 22, 2017

@dunglas

Awesome piece of code! I left some minor changes, but it's almost ready to be merged.

features/main/embedded.feature
+ """
+ {
+ "@context": "/contexts/RelatedDummy",
+ "@id": "/related_dummies",

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

IIUC, the @id key should be /dummies/1/related_dummies

@dunglas

dunglas Apr 3, 2017

Member

IIUC, the @id key should be /dummies/1/related_dummies

This comment has been minimized.

@soyuka

soyuka Apr 3, 2017

Member

Hmm not sure because this is still a related_dummy. Anyway I really don't know how I can implement this because getIriFromItem isn't aware that we have requested this resource from a parent resource.

@soyuka

soyuka Apr 3, 2017

Member

Hmm not sure because this is still a related_dummy. Anyway I really don't know how I can implement this because getIriFromItem isn't aware that we have requested this resource from a parent resource.

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

It's the ID of this collection, not of the general collection, it must reference the IRI to derefence to get this specific collection. Maybe can it be passed in the context in the SerializerListener?

@dunglas

dunglas Apr 3, 2017

Member

It's the ID of this collection, not of the general collection, it must reference the IRI to derefence to get this specific collection. Maybe can it be passed in the context in the SerializerListener?

This comment has been minimized.

@soyuka

soyuka Apr 3, 2017

Member

Oh wait this is set in hydra, my bad. Yeah I just need to add some data to the SerializerContextBuilder :).

@soyuka

soyuka Apr 3, 2017

Member

Oh wait this is set in hydra, my bad. Yeah I just need to add some data to the SerializerContextBuilder :).

+ *
+ * @author Antoine Bluchet <soyuka@gmail.com>
+ */
+class SubresourceDataProvider implements SubresourceDataProviderInterface

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Can you made this class final?

@dunglas

dunglas Apr 3, 2017

Member

Can you made this class final?

+ $alias = $queryNameGenerator->generateJoinAlias($identifier);
+
+ //MANY_TO_MANY relations needs an explicit join so that the identifier part can be retrieved
+ if ($classMetadata->getAssociationMapping($previousIdentifier)['type'] === ClassMetadataInfo::MANY_TO_MANY) {

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Can you invert this test (yoda style)?

@dunglas

dunglas Apr 3, 2017

Member

Can you invert this test (yoda style)?

src/Bridge/Symfony/Routing/ApiLoader.php
+ $this->propertyMetadataFactory = $propertyMetadataFactory;
+
+ if (null === $this->propertyNameCollectionFactory || null === $this->propertyMetadataFactory) {
+ @trigger_error('Not injecting "PropertyNameCollectionFactoryInterface" and "PropertyMetadataFactoryInterface" instances in '.__CLASS__.' is deprecrated since API Platform 2.1 and will not be possible anymore in API Platform 3', E_USER_DEPRECATED);

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Is this really necessary? To ease using API Platform as a standalone lib, it would be nice to be able to not provide those instance (and then loose subresource support)

@dunglas

dunglas Apr 3, 2017

Member

Is this really necessary? To ease using API Platform as a standalone lib, it would be nice to be able to not provide those instance (and then loose subresource support)

src/Bridge/Symfony/Routing/ApiLoader.php
+ * @param string $rootResourceClass null on the first iteration, it then keeps track of the origin resource class
+ * @param string $rootResourceClass null on the first iteration, it then keeps track of the parent operation
+ */
+ private function computeSubresourceOperations(RouteCollection $routeCollection, string $resourceClass, $rootResourceClass = null, $parentOperation = null)

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Missing typehints for $rootResourceClass and $parentOperation

@dunglas

dunglas Apr 3, 2017

Member

Missing typehints for $rootResourceClass and $parentOperation

@@ -23,16 +24,31 @@
/**
* {@inheritdoc}
*/
- public function resolveOperationPath(string $resourceShortName, array $operation, bool $collection): string
+ public function resolveOperationPath(string $resourceShortName, array $operation, string $operationType): string

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Same here, BC break that can fixed with the same trick.

@dunglas

dunglas Apr 3, 2017

Member

Same here, BC break that can fixed with the same trick.

* @param array $operation The operation metadata
- * @param bool $collection
+ * @param bool $operationType One of the constants defined in ApiPlatform\Core\Api\OperationType

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Bad documented type.

@dunglas

dunglas Apr 3, 2017

Member

Bad documented type.

*
* @return string
*/
- public function resolveOperationPath(string $resourceShortName, array $operation, bool $collection): string;
+ public function resolveOperationPath(string $resourceShortName, array $operation, string $operationType): string;

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

BC break, can be fixed the same way too.

@dunglas

dunglas Apr 3, 2017

Member

BC break, can be fixed the same way too.

@@ -23,14 +24,24 @@
/**
* {@inheritdoc}
*/
- public function resolveOperationPath(string $resourceShortName, array $operation, bool $collection): string
+ public function resolveOperationPath(string $resourceShortName, array $operation, string $operationType): string

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

Same here :)

@dunglas

dunglas Apr 3, 2017

Member

Same here :)

- if (isset($attributes['collection_operation_name'])) {
- $context = $resourceMetadata->getCollectionOperationAttribute($attributes['collection_operation_name'], $key, [], true);
- $context['collection_operation_name'] = $attributes['collection_operation_name'];
+ if (isset($attributes['collection_operation_name']) || isset($attributes['subresource_operation_name'])) {

This comment has been minimized.

@dunglas

dunglas Apr 3, 2017

Member

WDYT of avoiding an extra call to isset?

$operationKey = null;
if (isset($attributes['collection_operation_name'])) {
    $operationKey = 'collection_operation_name' ;
} elseif (isset($attributes['subresource_operation_name'])) {
    $operationKey = 'subresource_operation_name';
}

if (null !== $operationKey) // ...
@dunglas

dunglas Apr 3, 2017

Member

WDYT of avoiding an extra call to isset?

$operationKey = null;
if (isset($attributes['collection_operation_name'])) {
    $operationKey = 'collection_operation_name' ;
} elseif (isset($attributes['subresource_operation_name'])) {
    $operationKey = 'subresource_operation_name';
}

if (null !== $operationKey) // ...
@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 3, 2017

Member

@dunglas May you check the last commit (about corrected Hydra @id)? Let me know what you think about the implementation, if it looks fine to you I need to add some tests to the IriConverter :).

Member

soyuka commented Apr 3, 2017

@dunglas May you check the last commit (about corrected Hydra @id)? Let me know what you think about the implementation, if it looks fine to you I need to add some tests to the IriConverter :).

+ *
+ * @return string
+ */
+ public static function getOperationType($operationType)

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

: string

@dunglas

dunglas Apr 7, 2017

Member

: string

+ * - OperationType::ITEM
+ * - OperationType::COLLECTION
+ * - OperationType::SUBCOLLECTION
+ */

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

@internal so we can drop it in the next major version.

@dunglas

dunglas Apr 7, 2017

Member

@internal so we can drop it in the next major version.

+ * - OperationType::COLLECTION
+ * - OperationType::SUBCOLLECTION
+ */
+final class OperationTypeDeprecationHelper

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

Why not a trait?

@dunglas

dunglas Apr 7, 2017

Member

Why not a trait?

This comment has been minimized.

@soyuka

soyuka Apr 7, 2017

Member

Not really useful to have a trait for this, static methods are fine enough IMO but I can change this.

@soyuka

soyuka Apr 7, 2017

Member

Not really useful to have a trait for this, static methods are fine enough IMO but I can change this.

$path .= '.{_format}';
return $path;
}
+
+ private function dashize(string $string)

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

: string

@dunglas

dunglas Apr 7, 2017

Member

: string

@dunglas

dunglas approved these changes Apr 7, 2017

@soyuka soyuka requested a review from teohhanhui Apr 7, 2017

@soyuka soyuka referenced this pull request in api-platform/api-platform Apr 7, 2017

Closed

Allow collection/item paths with wildcard parameters #276

@backbone87

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 9, 2017

Is the statement from earlier in this ticket still true, that composite PKs do not work? If so, what needs to be done to support them? I would really like that feature for my next project and can offer some manpower to it. though, if it is too complicated, i could redesign my use-case to use an artificial single column PK.

Is it possible to define different operation/configuration depending on the "scope" a resource is used in?
Example: 2 related top-level entities (in UML terms: association)

  • /books/{book_id}/authors/{author_id} only allow collection get and item get
  • /authors/{author_id} allow all default operations
    Or is that another use case not covered by this PR (and only covers the UML constructs "aggregation" and "composition")?

What about self-referencing associations? From my use-case:

  • Top-level (aggregate root) entity container: contains mostly meta-data for authorization and concurrency-control and privately owns (UML composition) a set of nodes that form a DAG
  • Subentity node: Insert-only data-point; has an id that is local to the container and is "auto-incremented" (not rly, it is just the version of the container at insertation time); has a type (handled outside of doctrine, no inheritance mapping; strategy pattern); has a data column (arbitrary json data, structure depends on the type); has some more domain-specific meta-data; and of course a (possibly empty) to-many association "derivedFrom" to other nodes of the same container (this association reflects the DAG).
  • Some example filter queries:
    • /containers/1/nodes?subgraph-of-most-recent-node-of-type=result (this should also be the embedded node set, when getting a container /container/1)
    • /containers/1/nodes?subgraph-of-nodes-derived-from=<NODE_ID>
    • /containers/1/nodes?subgraph-of-nodes-used-to-derive-node=<NODE_ID>

Is the statement from earlier in this ticket still true, that composite PKs do not work? If so, what needs to be done to support them? I would really like that feature for my next project and can offer some manpower to it. though, if it is too complicated, i could redesign my use-case to use an artificial single column PK.

Is it possible to define different operation/configuration depending on the "scope" a resource is used in?
Example: 2 related top-level entities (in UML terms: association)

  • /books/{book_id}/authors/{author_id} only allow collection get and item get
  • /authors/{author_id} allow all default operations
    Or is that another use case not covered by this PR (and only covers the UML constructs "aggregation" and "composition")?

What about self-referencing associations? From my use-case:

  • Top-level (aggregate root) entity container: contains mostly meta-data for authorization and concurrency-control and privately owns (UML composition) a set of nodes that form a DAG
  • Subentity node: Insert-only data-point; has an id that is local to the container and is "auto-incremented" (not rly, it is just the version of the container at insertation time); has a type (handled outside of doctrine, no inheritance mapping; strategy pattern); has a data column (arbitrary json data, structure depends on the type); has some more domain-specific meta-data; and of course a (possibly empty) to-many association "derivedFrom" to other nodes of the same container (this association reflects the DAG).
  • Some example filter queries:
    • /containers/1/nodes?subgraph-of-most-recent-node-of-type=result (this should also be the embedded node set, when getting a container /container/1)
    • /containers/1/nodes?subgraph-of-nodes-derived-from=<NODE_ID>
    • /containers/1/nodes?subgraph-of-nodes-used-to-derive-node=<NODE_ID>
@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 9, 2017

Member

Is the statement from earlier in this ticket still true, that composite PKs do not work?

I think they do.

Is it possible to define different operation/configuration depending on the "scope" a resource is used in?

A subresource has only GET operations. No plan on implementing PUT/POST yet.

What about self-referencing associations?

Just try the branch :) seams it should work well with your use-cases.

Member

soyuka commented Apr 9, 2017

Is the statement from earlier in this ticket still true, that composite PKs do not work?

I think they do.

Is it possible to define different operation/configuration depending on the "scope" a resource is used in?

A subresource has only GET operations. No plan on implementing PUT/POST yet.

What about self-referencing associations?

Just try the branch :) seams it should work well with your use-cases.

@backbone87

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 9, 2017

Just try the branch :) seams it should work well with your use-cases.

I am doin that right now ;)

A subresource has only GET operations. No plan on implementing PUT/POST yet.

So i cant POST to /container/{container_id}/nodes to add a new node? (i dont need update, just create)

Just try the branch :) seams it should work well with your use-cases.

I am doin that right now ;)

A subresource has only GET operations. No plan on implementing PUT/POST yet.

So i cant POST to /container/{container_id}/nodes to add a new node? (i dont need update, just create)

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 10, 2017

Member

So i cant POST to /container/{container_id}/nodes to add a new node? (i dont need update, just create)

Nope, you'd need to POST /nodes.

Member

soyuka commented Apr 10, 2017

So i cant POST to /container/{container_id}/nodes to add a new node? (i dont need update, just create)

Nope, you'd need to POST /nodes.

@backbone87

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 10, 2017

There is no /nodes in my current approach, nodes are privately owned by a container entity. a node has composite PK consisting of the containers UUID and an index

There is no /nodes in my current approach, nodes are privately owned by a container entity. a node has composite PK consisting of the containers UUID and an index

@soyuka

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 10, 2017

Member

Hmm, you might be able to get what you want by manually declaring a new operation for posts on your /container/{uuid}/nodes. Though this is a bit out of scope as nodes should be a Resource, so that it can be a SubResource of containers:

/**
 * @ApiResource
 */
class Nodes
{
}

/**
 * @ApiResource
 */
class Container
{
    /**
     * @ApiProperty(subresource=true)
     */
    private $node;

   public function getNode(): Node {
       return $this->node;
   }
}
Member

soyuka commented Apr 10, 2017

Hmm, you might be able to get what you want by manually declaring a new operation for posts on your /container/{uuid}/nodes. Though this is a bit out of scope as nodes should be a Resource, so that it can be a SubResource of containers:

/**
 * @ApiResource
 */
class Nodes
{
}

/**
 * @ApiResource
 */
class Container
{
    /**
     * @ApiProperty(subresource=true)
     */
    private $node;

   public function getNode(): Node {
       return $this->node;
   }
}
@backbone87

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 14, 2017

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?

backbone87 commented Apr 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 14, 2017

Member

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

Member

soyuka commented Apr 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 14, 2017

Member

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!

Member

soyuka commented Apr 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 14, 2017

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;

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

This comment has been minimized.

Show comment
Hide comment
@soyuka

soyuka Apr 14, 2017

Member

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).

Member

soyuka commented Apr 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 14, 2017

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)

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

This comment has been minimized.

Show comment
Hide comment
@backbone87

backbone87 Apr 14, 2017

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.

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 added some commits Feb 21, 2017