Skip to content

Commit

Permalink
Squish repeated ShadowNode reconstruction bug
Browse files Browse the repository at this point in the history
- fixes cases when repeated calls to `ShadowNode::reconstructRealTree` would cause index collisions
  • Loading branch information
dakujem committed Mar 27, 2024
1 parent 4fe9d4f commit 04f5b5d
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 3 deletions.
1 change: 1 addition & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Please report any issues.

- Added `Seed::chain` method for iterable collection chaining: the new method takes over `Seed::merged` which becomes its alias for backward compatibility.
- Deprecated `Seed::merged`, use `Seed::chain` instead.
- Fixed repeated calls to `ShadowNode::reconstructRealTree` causing index collisions.


## v1.0
Expand Down
5 changes: 3 additions & 2 deletions src/MaterializedPath/Support/ShadowNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@ public function __construct(
public function reconstructRealTree(): ?TreeNodeContract
{
$realNode = $this->realNode();
$realNode?->removeChildren();
/** @var self $child */
foreach ($this->children() as $index => $child) {
$realChild = $child->realNode();
if (null !== $realNode && null !== $realChild) {
$realNode->addChild($realChild, $index);
if (null !== $realChild) {
$realNode?->addChild($realChild, $index);
$realChild->setParent($realNode);
}
$child->reconstructRealTree();
Expand Down
38 changes: 37 additions & 1 deletion tests/mptree.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ declare(strict_types=1);

namespace Dakujem\Test;

use Dakujem\Oliva\DataNodeContract;
use Dakujem\Oliva\Exceptions\ExtractorReturnValueIssue;
use Dakujem\Oliva\Exceptions\InternalLogicException;
use Dakujem\Oliva\Exceptions\InvalidInputData;
Expand All @@ -17,6 +18,7 @@ use Dakujem\Oliva\MovableNodeContract;
use Dakujem\Oliva\Node;
use Dakujem\Oliva\Seed;
use Dakujem\Oliva\Tree;
use Dakujem\Oliva\TreeNodeContract;
use Tester\Assert;

require_once __DIR__ . '/setup.php';
Expand Down Expand Up @@ -258,6 +260,12 @@ class Item
Assert::count(2, $node->children());
Assert::same('007000', $node->child(0)->data());
Assert::same('007001', $node->child(1)->data());

// The reconstruction works even when the tree contains nodes not connected to the root.
Assert::same(null, $almost->root());
Assert::same(null, $almost->shadow()->data());
Assert::same($node, $almost->shadow()->child(0)->child(0)->data()->parent());
Assert::same($node, $almost->shadow()->child(0)->child(1)->data()->parent());
})();

(function () {
Expand All @@ -283,4 +291,32 @@ class Item
Assert::throws(function () use ($shadow) {
$shadow->setParent(new Node('new parent'));
}, InternalLogicException::class, 'Invalid use of a shadow node. Only shadow nodes can be parents of shadow nodes.');
})();
})();

// This test checks an edge-case where reconstructing a shadow root led to an index collision.
(function () {
$builder = new TreeBuilder(
node: fn(?string $path) => new Node($path),
vector: Path::fixed(
3,
fn(?string $path) => $path,
),
);

$normalizer = function (TreeNodeContract $node) use (&$normalizer): array {
return [
'data' => $node instanceof DataNodeContract ? $node->data() : null,
'children' => array_map(fn($child) => $normalizer($child), $node->children()),
];
};

$almost = $builder->processInput(
input: ['', '007000', '006', '007', '007001', '005001', '004', '005001009',],
);

Assert::same(
$normalizer($almost->root()),
$normalizer($almost->shadow()->reconstructRealTree()),
);
})();

0 comments on commit 04f5b5d

Please sign in to comment.