Unable to convert self-referring instances to resources #57

tobias-trozowski · 2019-04-12T08:51:23Z

Are you fixing a bug?
- Detail how the bug is invoked currently.
- Detail the original, incorrect behavior.
- Detail the new, expected behavior.
- Base your feature on the master branch, and submit against that branch.
- Add a regression test that demonstrates the bug, and proves the fix.
- Add a CHANGELOG.md entry for the fix.

Imagine the following example:

class Parent {
    /** @var Child[] */
    public $childs;
    public function addChild(Child $child) {
        $this->childs[] = $child;
        $child->parent = $this;
    }
}
class Child {
    /** @var Parent */
    public $parent;
}

$parent = new Parent();
$parent->addChild(new Child());
$parent->addChild(new Child());

$generator->fromObject($parent, $request);
// the call above will result in an error:
// Error : Maximum function nesting level of '256' reached, aborting!

Other examples:
One-To-One, Bidirectional
One-To-One, Self-referencing
One-To-Many, Bidirectional
One-To-Many, Self-referencing
Many-To-Many, Bidirectional
Many-To-Many, Self-referencing

The zfcampus/zf-hal component solved this issue by using a $maxDepth property in metadata which is then passed through here and here.

Using this approach would result in a change of the \Zend\Expressive\Hal\ResourceGenerator\StrategyInterface interface which would be a BC break.

I would love if someone comes up with an alternate solution which will not break BC.

weierophinney

I don't see a BC break, per se, but the fact that the trait has to vary logic based on concrete metadata type is definitely a code smell.

One way we've solved that in the past is to introduce an additional interface. In this case, it might look like this:

DepthAwareMetadataInterface
{
    public function hasReachedMaxDepth(int $currentDepth) : bool;
    public function extractBareInstance() : array;
}

The logic in ExtractInstanceTrait::extractInstance() would then become:

if ($metadata instanceof DepthAwareMetadataInterface
    && $metadata->hasReachedMaxDepth($depth)
) {
    return $metadata->extractBareInstance();
}

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

You would then implement the new interface in any resource that should be checking for circular references. This approach ensures that extensions or new implementations can signal to the strategy that they have hit a depth limit as well, and provide a way of providing a bare resource back to the resource generator.

Otherwise, there's no real BC break here - you're adding the $depth argument where necessary, but always with a default value, which means any extension will not break. We can message that extensions should honor the new argument, but if they do not need to worry about it, they can continue safely ignoring it.

tobias-trozowski · 2019-04-25T17:22:42Z

still smells. but should fit now.

weierophinney

The change to the StrategyInterface::createResource() is a BC break, as it makes a signature change; implementations MUST be updated in order to comply, basically.

We have two options:

Remove that signature change, but have the shipped implementations optionally accept the additional argument and use it internally. This would allow it to be backwards compatible.
Keep the change and target a new major version. We could then consider incorporating the hasReachedMaxDepth() method in the MetadataInterface. Implementations that do not care could always return false, while those that are aware of depth could implement logic.

I tend to lean towards the latter approach (new major version) as it means we won't need to clean-up later, and the logic in ExtractInstanceTrait becomes:

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

i.e., no switching based on types.

Thoughts? We can bump to a new major version immediately, so don't make a decision based on "I don't want to wait". 😄

weierophinney · 2019-04-25T17:26:45Z

src/ResourceGenerator/StrategyInterface.php

@@ -23,6 +23,7 @@ public function createResource(
        $instance,
        Metadata\AbstractMetadata $metadata,
        ResourceGenerator $resourceGenerator,
-        ServerRequestInterface $request
+        ServerRequestInterface $request,
+        int $depth = 0


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

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

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.

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?

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.

weierophinney · 2019-04-25T17:27:44Z

src/Metadata/DepthAwareMetadataInterface.php

+     *
+     * @return int
+     */
+    public function getMaxDepth(): int;


Is this method even necessary? It's never called internally.

yes, never called internally. but as we introduce a mechanism which handles depth / maxDepth we should be able to retrieve the maxDepth from the interface.

For what use case, though?

The hasReachedMaxDepth() method provides behavior, which we consume internally. What is the use case for retrieving the max depth setting? What should implementations that ignore depth return in such cases?

good point. i guess that method is really useless :O

froschdesign

This pull request has a defined topic, please do not included changes which does not belong to this one topic. Revert all changes like static, return types, Doc-Blocks etc and please create smaller commits. The commit "added new MetadataInterface" breaks the history of the repository and does not allow any cherry-pick.

Thanks in advance!

weierophinney · 2019-05-13T21:06:32Z

@tobias-trozowski I've rebased your branch to squash several commits and remove both the revert commits and the commits they were reverting; the diff ends up the same as you had committed previously.

In the future, feel free to use git rebase -i + git push -f liberally when working on a patch; these help ensure the history is easier to follow, particularly when removing patches.

I'll review again now to see where we are in terms of ability to merge.

weierophinney

Just a few minor changes at this point, mostly around ensuring we have good docs once this is in place.

weierophinney · 2019-05-13T21:16:58Z

src/ResourceGenerator.php

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


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!

Metadata\RouteBasedResourceMetadata::__construct

ResourceGenerator::fromObject

ResourceGenerator\StrategyInterface::createResource

ResourceGenerator\RouteBasedCollectionStrategy::createResource

ResourceGenerator\RouteBasedResourceStrategy::createResource

ResourceGenerator\UrlBasedCollectionStrategy::createResource

ResourceGenerator\UrlBasedResourceStrategy::createResource

weierophinney · 2019-05-13T21:19:01Z

src/Metadata/RouteBasedResourceMetadataFactory.php

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


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

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

ping @weierophinney
any thoughts on this?

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.

weierophinney · 2019-12-31T20:56:28Z

This repository has been closed and moved to mezzio/mezzio-hal; a new issue has been opened at mezzio/mezzio-hal#1.

weierophinney · 2019-12-31T20:57:00Z

This repository has been moved to mezzio/mezzio-hal. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone mezzio/mezzio-hal to another directory.
Copy the files from the second bullet point to the clone of mezzio/mezzio-hal.
In your clone of mezzio/mezzio-hal, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

added test to test self referring instances

abdcefb

weierophinney suggested changes Apr 23, 2019

View reviewed changes

weierophinney suggested changes Apr 25, 2019

View reviewed changes

froschdesign suggested changes May 6, 2019

View reviewed changes

tobias-trozowski added 2 commits May 13, 2019 16:03

added maxDepth for RouteBasedResource to prevent endless nesting

43772a4

added MetadataInterface

c985716

weierophinney force-pushed the self-referring-instance branch from 87378c1 to c985716 Compare May 13, 2019 21:03

weierophinney suggested changes May 13, 2019

View reviewed changes

weierophinney mentioned this pull request Dec 31, 2019

Unable to convert self-referring instances to resources mezzio/mezzio-hal#1

Closed

7 tasks

tobias-trozowski mentioned this pull request Dec 5, 2020

Unable to convert self-referring instances to resources mezzio/mezzio-hal#22

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unable to convert self-referring instances to resources #57

Unable to convert self-referring instances to resources #57

tobias-trozowski commented Apr 12, 2019 •

edited

Loading

weierophinney left a comment

tobias-trozowski commented Apr 25, 2019

weierophinney left a comment

weierophinney Apr 25, 2019

tobias-trozowski Apr 25, 2019

weierophinney May 1, 2019

tobias-trozowski May 6, 2019

weierophinney May 13, 2019

weierophinney Apr 25, 2019

tobias-trozowski Apr 25, 2019

weierophinney Apr 25, 2019

tobias-trozowski Apr 25, 2019

froschdesign left a comment

weierophinney commented May 13, 2019

weierophinney left a comment

weierophinney May 13, 2019

tobias-trozowski May 15, 2019

weierophinney May 13, 2019

tobias-trozowski May 15, 2019

tobias-trozowski May 27, 2019

weierophinney Oct 16, 2019

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

Unable to convert self-referring instances to resources #57

Are you sure you want to change the base?

Unable to convert self-referring instances to resources #57

Conversation

tobias-trozowski commented Apr 12, 2019 • edited Loading

weierophinney left a comment

Choose a reason for hiding this comment

tobias-trozowski commented Apr 25, 2019

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign left a comment

Choose a reason for hiding this comment

weierophinney commented May 13, 2019

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

tobias-trozowski commented Apr 12, 2019 •

edited

Loading