From 5d62b60c9b9bac7ba738e6178d21ab622238f329 Mon Sep 17 00:00:00 2001 From: Andrej Rypo Date: Sun, 4 Feb 2024 19:02:54 +0100 Subject: [PATCH] some edge-case tests and prevention --- src/Exceptions/CallableIssue.php | 18 ++++++ src/MaterializedPath/Path.php | 3 + src/Simple/TreeWrapper.php | 34 ++++++++--- tests/nodes.phpt | 58 +++++++++++++++++- tests/path.phpt | 101 +++++++++++++++++++++++++++++++ 5 files changed, 204 insertions(+), 10 deletions(-) create mode 100644 src/Exceptions/CallableIssue.php create mode 100644 tests/path.phpt diff --git a/src/Exceptions/CallableIssue.php b/src/Exceptions/CallableIssue.php new file mode 100644 index 0000000..0901fa9 --- /dev/null +++ b/src/Exceptions/CallableIssue.php @@ -0,0 +1,18 @@ + + */ +final class CallableIssue extends LogicException implements FailsIntegrity, AcceptsDebugContext +{ + use AcceptDebugContext; +} diff --git a/src/MaterializedPath/Path.php b/src/MaterializedPath/Path.php index fc744d3..f5c1039 100644 --- a/src/MaterializedPath/Path.php +++ b/src/MaterializedPath/Path.php @@ -61,6 +61,9 @@ public static function delimited(string $delimiter, callable $accessor): callabl */ public static function fixed(int $levelWidth, callable $accessor): callable { + if ($levelWidth < 1) { + throw new ConfigurationIssue('The level width must be a positive integer.'); + } return function (mixed $data, mixed $inputIndex = null, ?TreeNodeContract $node = null) use ( $levelWidth, $accessor, diff --git a/src/Simple/TreeWrapper.php b/src/Simple/TreeWrapper.php index 6b38333..edddfe7 100644 --- a/src/Simple/TreeWrapper.php +++ b/src/Simple/TreeWrapper.php @@ -5,10 +5,12 @@ namespace Dakujem\Oliva\Simple; use Dakujem\Oliva\Exceptions\AcceptsDebugContext; +use Dakujem\Oliva\Exceptions\CallableIssue; use Dakujem\Oliva\Exceptions\ExtractorReturnValueIssue; use Dakujem\Oliva\Exceptions\InvalidNodeFactoryReturnValue; use Dakujem\Oliva\MovableNodeContract; use Dakujem\Oliva\TreeNodeContract; +use Throwable; /** * Simple tree builder. @@ -55,22 +57,36 @@ private function wrapNode( callable $nodeFactory, callable $childrenExtractor, ): MovableNodeContract { - // Create a node using the provided factory. - $node = $nodeFactory($data); + try { + // Create a node using the provided factory. + $node = $nodeFactory($data); - // Check for consistency. - if (!$node instanceof MovableNodeContract) { - throw (new InvalidNodeFactoryReturnValue()) - ->tag('node', $node) - ->tag('data', $data); + // Check for consistency. + if (!$node instanceof MovableNodeContract) { + throw (new InvalidNodeFactoryReturnValue()) + ->tag('node', $node) + ->tag('data', $data); + } + + $childrenData = $childrenExtractor($data, $node); + } catch (Throwable $e) { + if (!$e instanceof AcceptsDebugContext) { + // wrap the exception so that it supports context decoration + $e = (new CallableIssue( + message: $e->getMessage(), + code: $e->getCode(), + previous: $e, + )); + } + throw $e->push('nodes', $node ?? null); } - $childrenData = $childrenExtractor($data, $node); if (null !== $childrenData && !is_iterable($childrenData)) { throw (new ExtractorReturnValueIssue('Children data extractor must return an iterable collection containing children data.')) ->tag('children', $childrenData) ->tag('parent', $node) - ->tag('data', $data); + ->tag('data', $data) + ->push('nodes', $node); } foreach ($childrenData ?? [] as $key => $childData) { try { diff --git a/tests/nodes.phpt b/tests/nodes.phpt index fb7d3c1..55c60ec 100644 --- a/tests/nodes.phpt +++ b/tests/nodes.phpt @@ -4,6 +4,8 @@ declare(strict_types=1); namespace Dakujem\Test; +use Dakujem\Oliva\Exceptions\AcceptsDebugContext; +use Dakujem\Oliva\Exceptions\Context; use Dakujem\Oliva\Node; use Dakujem\Oliva\Simple\NodeBuilder; use Dakujem\Oliva\Simple\TreeWrapper; @@ -78,11 +80,65 @@ require_once __DIR__ . '/setup.php'; ], ], ]; - $wrapper = new TreeWrapper(fn(array $raw) => new Node($raw['data']), fn(array $raw) => $raw['children'] ?? null); + $wrapper = new TreeWrapper( + fn(array $raw) => new Node($raw['data']), + fn(array $raw): ?iterable => $raw['children'] ?? null, + ); $root = $wrapper->wrap($data); // pre-order Assert::same('FBADCEGIH', TreeTesterTool::flatten($root)); + + $faulty = $data; + $thrown = false; + try { + $faulty['children'][1]['children'][] = ['data' => 'this will cause type error inside the extractor', 'children' => 42]; + $wrapper->wrap($faulty); + } catch (AcceptsDebugContext $e) { + /** @var Context $context */ + $context = $e->context; + Assert::count(3, $context->bag['nodes'] ?? []); + Assert::same( + ['this will cause type error inside the extractor', 'G', 'F'], + array_map(fn(Node $node) => $node->data(), $context->bag['nodes']), + ); + $thrown = true; + } + Assert::true($thrown, 'The catch block has to run'); + + $faultyWrapper = new TreeWrapper( + fn(array $raw) => new Node($raw['data']), + function (array $raw): mixed { + if (isset($raw['children']) && !is_iterable($raw['children'] ?? null)) { + return 'fault'; + } + return $raw['children'] ?? null; + }, + ); + + $thrown = false; + try { + $faultyWrapper->wrap($faulty); + } catch (AcceptsDebugContext $e) { + /** @var Context $context */ + $context = $e->context; + Assert::count(3, $context->bag['nodes'] ?? []); + Assert::same( + ['this will cause type error inside the extractor', 'G', 'F'], + array_map(fn(Node $node) => $node->data(), $context->bag['nodes']), + ); + // the "children" extracted by the extractor + Assert::same('fault', $context->bag['children'] ?? null); + // the data that was present + Assert::same([ + 'data' => 'this will cause type error inside the extractor', + 'children' => 42, + ], $context->bag['data'] ?? null); + // ref to the parent node + Assert::type(Node::class, $context->bag['parent'] ?? null); + $thrown = true; + } + Assert::true($thrown, 'The catch block has to run'); })(); diff --git a/tests/path.phpt b/tests/path.phpt new file mode 100644 index 0000000..0328688 --- /dev/null +++ b/tests/path.phpt @@ -0,0 +1,101 @@ + null); + }, ConfigurationIssue::class, 'The delimiter must be a single character.'); + Assert::throws(function () { + Path::delimited('42', fn() => null); + }, ConfigurationIssue::class, 'The delimiter must be a single character.'); + + Assert::throws(function () { + $extractor = Path::delimited('.', fn() => 42); + $extractor(null); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + + $extractor = Path::delimited('.', fn($data) => $data); + Assert::same([], $extractor(null)); + Assert::same([], $extractor('')); + Assert::same(['a string'], $extractor('a string')); + Assert::same(['a', 'string'], $extractor('a.string')); + Assert::same([' a', ' string '], $extractor(' a. string ')); // no trim implemented + + Assert::throws(function () use ($extractor) { + $extractor(42); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor([]); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor(4.2); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor(new Node(null)); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + + $extractor = Path::delimited('.', fn() => 42); + $thrown = false; + $node = new Node('empty'); + try { + $extractor('foo', 'bar', $node); + } catch (InvalidTreePath $e) { + /** @var Context $context */ + $context = $e->context; + Assert::type(Context::class, $context); + Assert::same([ + 'path' => 42, + 'data' => 'foo', + 'index' => 'bar', + 'node' => $node, + ], $context->bag); + $thrown = true; + } + Assert::true($thrown, 'The catch block has not been executed.'); +})(); + +// Test some edge cases of Path::fixed +(function () { + Assert::throws(function () { + Path::fixed(0, fn() => null); + }, ConfigurationIssue::class, 'The level width must be a positive integer.'); + Assert::throws(function () { + Path::fixed(-1, fn() => null); + }, ConfigurationIssue::class, 'The level width must be a positive integer.'); + + $extractor = Path::fixed(1, fn($data) => $data); + Assert::same(['a', 'b', 'c'], $extractor('abc')); + Assert::same([], $extractor('')); + + $extractor = Path::fixed(2, fn($data) => $data); + Assert::same(['ab', 'cd'], $extractor('abcd')); + Assert::same([' ', 'ab', 'cd', ' '], $extractor(' abcd ')); // no trim implemented + Assert::same([], $extractor('')); + + Assert::throws(function () use ($extractor) { + $extractor(42); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor([]); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor(4.2); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); + Assert::throws(function () use ($extractor) { + $extractor(new Node(null)); + }, InvalidTreePath::class, 'Invalid tree path returned by the accessor. A string is required.'); +})(); +