Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Unable to convert self-referring instances to resources #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Unable to convert self-referring instances to resources #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abdcefb
added test to test self referring instances
tobias-trozowski Apr 12, 2019
43772a4
added maxDepth for RouteBasedResource to prevent endless nesting
tobias-trozowski Apr 23, 2019
c985716
added MetadataInterface
tobias-trozowski Apr 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/book/cookbook/generating-custom-resources.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ interface StrategyInterface
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource;
}
```
Expand Down
1 change: 1 addition & 0 deletions docs/book/factories.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ The additional pairs are as follows:
represents the resource identifier; defaults to "id".
- `route_params`: an array of additional routing parameters to use when
generating the self relational link for the resource.
- `max_depth`: the number of nesting levels processed. Defaults to 10.
- For `RouteBasedCollectionMetadata`:
- `collection_class`: the collection class the metadata describes.
- `collection_relation`: the embedded relation for the collection in the
Expand Down
1 change: 1 addition & 0 deletions docs/book/resource-generator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ following information:
that maps to the resource identifier)
- array `$routeParams = []` (associative array of additional routing
parameters to substitute when generating the URI)
- int `$maxDepth = 10` max allowed nesting levels.
- `Zend\Expressive\Hal\Metadata\UrlBasedCollectionMetadata`:
- string `$class`
- string `$collectionRelation`
Expand Down
7 changes: 6 additions & 1 deletion src/Metadata/AbstractMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

use Zend\Expressive\Hal\LinkCollection;

abstract class AbstractMetadata
abstract class AbstractMetadata implements MetadataInterface
{
use LinkCollection;

Expand All @@ -22,4 +22,9 @@ public function getClass() : string
{
return $this->class;
}

public function hasReachedMaxDepth(int $currentDepth): bool
{
return false;
}
}
23 changes: 23 additions & 0 deletions src/Metadata/MetadataInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
declare(strict_types=1);

namespace Zend\Expressive\Hal\Metadata;

interface MetadataInterface
{
/**
* Returns the configured metadata class name
*
* @return string
*/
public function getClass() : string;

/**
* Determines whenever the current depth level exceeds the allowed max depth
*
* @param int $currentDepth
*
* @return bool
*/
public function hasReachedMaxDepth(int $currentDepth): bool;
}
12 changes: 11 additions & 1 deletion src/Metadata/RouteBasedResourceMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,25 @@ class RouteBasedResourceMetadata extends AbstractResourceMetadata
/** @var array */
private $routeParams;

/** @var int */
private $maxDepth;

public function __construct(
string $class,
string $route,
string $extractor,
string $resourceIdentifier = 'id',
string $routeIdentifierPlaceholder = 'id',
array $routeParams = []
array $routeParams = [],
int $maxDepth = 10
) {
$this->class = $class;
$this->route = $route;
$this->extractor = $extractor;
$this->resourceIdentifier = $resourceIdentifier;
$this->routeIdentifierPlaceholder = $routeIdentifierPlaceholder;
$this->routeParams = $routeParams;
$this->maxDepth = $maxDepth;
}

public function getRoute() : string
Expand All @@ -61,4 +66,9 @@ public function setRouteParams(array $routeParams) : void
{
$this->routeParams = $routeParams;
}

public function hasReachedMaxDepth(int $currentDepth): bool
{
return $currentDepth > $this->maxDepth;
}
}
7 changes: 6 additions & 1 deletion src/Metadata/RouteBasedResourceMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ class RouteBasedResourceMetadataFactory implements MetadataFactoryInterface
* // generating the self relational link for the collection
* // resource. Defaults to an empty array.
* 'route_params' => [],
*
* // Max depth to render
* // Defaults to 10.
* 'max_depth' => 10,
* ]
* </code>
* @return AbstractMetadata
Expand All @@ -72,7 +76,8 @@ public function createMetadata(string $requestedName, array $metadata) : Abstrac
$metadata['extractor'],
$metadata['resource_identifier'] ?? 'id',
$metadata['route_identifier_placeholder'] ?? 'id',
$metadata['route_params'] ?? []
$metadata['route_params'] ?? [],
$metadata['max_depth'] ?? 10
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't know exactly where the config is coming from, it may be a good idea to do something like:

$maxDepth = $metadata['max_depth'] ?? 10;

somewhere before this, and then, here:

(int) $maxDepth

to ensure that there are not type issues.

(Many config formats will return everything as strings by default.)

Copy link
Author

Choose a reason for hiding this comment

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

not sure if silently supporting a wrong type is the right way to go here.
i'd rather see an TypeError to inform me that i use a wrong type instead of continuing using the wrong but silently supported type.
IMHO this is just hiding a some kind of complexity (using string in config which is then "magically" (from an outer perspective) casted to int).

Copy link
Author

Choose a reason for hiding this comment

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

ping @weierophinney
any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Leave it as-is. Most of the config libraries we support will cast values as they are processed, before they are pushed into the container. And the primary config type is pure PHP.

);
}
}
5 changes: 3 additions & 2 deletions src/ResourceGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public function fromArray(array $data, string $uri = null) : HalResource
* against types registered in the metadata map.
* @param ServerRequestInterface $request
*/
public function fromObject($instance, ServerRequestInterface $request) : HalResource
public function fromObject($instance, ServerRequestInterface $request, int $depth = 0) : HalResource
Copy link
Member

Choose a reason for hiding this comment

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

To help me as I create the changelog: could you please collate everywhere there is an additional optional argument passed to a public method? If I have that list, it will be far easier to communicate to users what may have changed in any extensions or custom implementations they have created.

Thanks in advance!

Copy link
Author

Choose a reason for hiding this comment

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

{
if (! is_object($instance)) {
throw Exception\InvalidObjectException::forNonObject($instance);
Expand All @@ -146,7 +146,8 @@ public function fromObject($instance, ServerRequestInterface $request) : HalReso
$instance,
$metadata,
$this,
$request
$request,
$depth
);
}
}
40 changes: 25 additions & 15 deletions src/ResourceGenerator/ExtractCollectionTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,22 @@ private function extractCollection(
Traversable $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
if (! $metadata instanceof AbstractCollectionMetadata) {
throw Exception\UnexpectedMetadataTypeException::forCollection($metadata, get_class($this));
}

if ($collection instanceof Paginator) {
return $this->extractPaginator($collection, $metadata, $resourceGenerator, $request);
return $this->extractPaginator($collection, $metadata, $resourceGenerator, $request, $depth);
}

if ($collection instanceof DoctrinePaginator) {
return $this->extractDoctrinePaginator($collection, $metadata, $resourceGenerator, $request);
return $this->extractDoctrinePaginator($collection, $metadata, $resourceGenerator, $request, $depth);
}

return $this->extractIterator($collection, $metadata, $resourceGenerator, $request);
return $this->extractIterator($collection, $metadata, $resourceGenerator, $request, $depth);
}

/**
Expand All @@ -77,7 +78,8 @@ private function extractPaginator(
Paginator $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
$data = ['_total_items' => $collection->getTotalItemCount()];
$pageCount = $collection->count();
Expand All @@ -91,7 +93,8 @@ function (int $page) use ($collection) {
$collection,
$metadata,
$resourceGenerator,
$request
$request,
$depth
);
}

Expand All @@ -106,7 +109,8 @@ private function extractDoctrinePaginator(
DoctrinePaginator $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
$query = $collection->getQuery();
$totalItems = count($collection);
Expand All @@ -124,22 +128,24 @@ function (int $page) use ($query, $perPage) {
$collection,
$metadata,
$resourceGenerator,
$request
$request,
$depth
);
}

private function extractIterator(
Traversable $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
$isCountable = $collection instanceof Countable;
$count = $isCountable ? $collection->count() : 0;

$resources = [];
foreach ($collection as $item) {
$resources[] = $resourceGenerator->fromObject($item, $request);
$resources[] = $resourceGenerator->fromObject($item, $request, $depth + 1);
$count = $isCountable ? $count : $count + 1;
}

Expand Down Expand Up @@ -184,7 +190,8 @@ private function createPaginatedCollectionResource(
iterable $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
$links = [];
$paginationParamType = $metadata->getPaginationParamType();
Expand All @@ -197,7 +204,8 @@ private function createPaginatedCollectionResource(
$collection,
$metadata,
$resourceGenerator,
$request
$request,
$depth
);
}

Expand Down Expand Up @@ -236,7 +244,8 @@ private function createPaginatedCollectionResource(
$collection,
$metadata,
$resourceGenerator,
$request
$request,
$depth
);
}

Expand All @@ -255,11 +264,12 @@ private function createCollectionResource(
iterable $collection,
AbstractCollectionMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
$resources = [];
foreach ($collection as $item) {
$resources[] = $resourceGenerator->fromObject($item, $request);
$resources[] = $resourceGenerator->fromObject($item, $request, $depth + 1);
}

return new HalResource($data, $links, [
Expand Down
9 changes: 7 additions & 2 deletions src/ResourceGenerator/ExtractInstanceTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ private function extractInstance(
$instance,
AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : array {
$hydrators = $resourceGenerator->getHydrators();
$extractor = $hydrators->get($metadata->getExtractor());
Expand All @@ -37,6 +38,10 @@ private function extractInstance(

$array = $extractor->extract($instance);

if ($metadata->hasReachedMaxDepth($depth)) {
return $array;
}

// Extract nested resources if present in metadata map
$metadataMap = $resourceGenerator->getMetadataMap();
foreach ($array as $key => $value) {
Expand All @@ -49,7 +54,7 @@ private function extractInstance(
continue;
}

$childData = $resourceGenerator->fromObject($value, $request);
$childData = $resourceGenerator->fromObject($value, $request, $depth + 1);

// Nested collections need to be merged.
$childMetadata = $metadataMap->get($childClass);
Expand Down
3 changes: 2 additions & 1 deletion src/ResourceGenerator/RouteBasedCollectionStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ public function createResource(
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
if (! $metadata instanceof Metadata\RouteBasedCollectionMetadata) {
throw Exception\UnexpectedMetadataTypeException::forMetadata(
Expand Down
10 changes: 8 additions & 2 deletions src/ResourceGenerator/RouteBasedResourceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ public function createResource(
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
if (! $metadata instanceof Metadata\RouteBasedResourceMetadata) {
throw Exception\UnexpectedMetadataTypeException::forMetadata(
Expand All @@ -34,7 +35,8 @@ public function createResource(
$instance,
$metadata,
$resourceGenerator,
$request
$request,
$depth
);

$routeParams = $metadata->getRouteParams();
Expand All @@ -45,6 +47,10 @@ public function createResource(
$routeParams[$routeIdentifier] = $data[$resourceIdentifier];
}

if ($metadata->hasReachedMaxDepth($depth)) {
$data = [];
}

return new HalResource($data, [
$resourceGenerator->getLinkGenerator()->fromRoute(
'self',
Expand Down
3 changes: 2 additions & 1 deletion src/ResourceGenerator/StrategyInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public function createResource(
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
Copy link
Member

Choose a reason for hiding this comment

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

Oof- this is a BC break, as it changes a signature in an interface.

Copy link
Author

Choose a reason for hiding this comment

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

yep :/ this was the thing i mentioned as i opened the PR. still don't know how to resolve this without breaking BC.

Not changing signature could end up in szenarios like this:

$a = new stdClass;
$b = new stdClass;
$a->b = $b;
$b->a = $a;
// using custom strategy for `$b` without passing the depth param would cause the issue i try to solve here

Copy link
Member

Choose a reason for hiding this comment

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

That was my point with one of my previous comments — this may be an excellent reason to break BC and have an immediate major release. As such, let's figure out what all we should change to make this as robust as possible for that release.

Copy link
Author

Choose a reason for hiding this comment

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

sounds good to me. should i just create a new PR with a few ideas or did you want to discuss it in issues first?

Copy link
Member

Choose a reason for hiding this comment

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

The main thing here is to ensure that this is the only change necessary to fix the depth issues.

If you know of other fixes that need to be done to address other issues, and which require BC breaks, submit those as separate patches.

In all likelihood, I won't do the major release immediately; I'm going to be looking at #42 as well, as I understand it represents a common Doctrine scenario, which means the component fails for it.

) : HalResource;
}
3 changes: 2 additions & 1 deletion src/ResourceGenerator/UrlBasedCollectionStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public function createResource(
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
if (! $metadata instanceof Metadata\UrlBasedCollectionMetadata) {
throw Exception\UnexpectedMetadataTypeException::forMetadata(
Expand Down
3 changes: 2 additions & 1 deletion src/ResourceGenerator/UrlBasedResourceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ public function createResource(
$instance,
Metadata\AbstractMetadata $metadata,
ResourceGenerator $resourceGenerator,
ServerRequestInterface $request
ServerRequestInterface $request,
int $depth = 0
) : HalResource {
if (! $metadata instanceof Metadata\UrlBasedResourceMetadata) {
throw Exception\UnexpectedMetadataTypeException::forMetadata(
Expand Down
Loading