feat!: rework mapper node and messages handling #593

romm · 2025-01-17T15:03:23Z

This commit brings a huge rework of how the recursive node system works under the hood during the mapping; it aims to simplify the architecture of this part of the library for several reasons:

Better memory management and overall performance:

—> The TreeNode class has been reworked and will no longer maintain a reference to the Shell instance.

—> The Node class has been removed and will no longer be part of the library's public API. This class used to give access to a lot of information to the user only when the mapping was failing.

It was one of the worst architectural decisions made at the
beginning of the project, because it would keep the entire tree of
nodes created during the mapping (even nodes that were correct).
This extra data was in the vast majority of cases (always?) not
needed and led to a more complex codebase, as well as bad memory
management.
API simplification — instead of working with instances of Node when the mapping fails, the user can now just call the newly introduced method MappingError::messages() and work directly with the messages.
Maintenance simplification — because the API has become simpler, it will be easier to maintain as well as easier to understand for newcomers.
Prepare for the future — this work was necessary for an upcoming project for this library: ✨ Mapper compilation ✨ which will bring huge performance gains.

List of changes:

❗ Removed \CuyZ\Valinor\Mapper\MappingError::node()
❗ Removed \CuyZ\Valinor\Mapper\Tree\Node
❗ Removed \CuyZ\Valinor\Mapper\Tree\NodeTraverser
❗ Removed \CuyZ\Valinor\Mapper\Tree\Message\Messages::flattenFromNode()
❗ Removed \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::node()
❗ Changed \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::__construct()

➕ Added: \CuyZ\Valinor\Mapper\MappingError::messages
➕ Added: \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::name()
➕ Added: \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::path()
➕ Added: \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::type()
➕ Added: \CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::sourceValue()

Note: this work was done in collaboration with Baptiste Langlade, who I thank a lot for his help and support 😊

Baptouuuu · 2025-01-18T14:50:53Z

src/Mapper/ArgumentsMapperError.php


-    public function __construct(FunctionDefinition $function, Node $node)
+    /**
+     * @param list<NodeMessage> $errors


Small detail but shouldn't this be non-empty-list<NodeMessage> ? Otherwise this mean it's possible to raise an exception when there's no mapping error.

Good catch, I forgot we can use @phpstan-assert-if-true to fix this kind of issues. Thanks!

Baptouuuu · 2025-01-18T14:52:39Z

src/Mapper/ArgumentsMapperError.php

                ->withBody("Could not map arguments of `$function->signature`. An error occurred at path {node_path}: {original_message}")
                ->toString();
        } else {
-            $source = ValueDumper::dump($node->sourceValue());
+            $source = ValueDumper::dump($source);
            $body = "Could not map arguments of `$function->signature` with value $source. A total of $errorsCount errors were encountered.";
        }

        parent::__construct($body, 1671115362);


Out of curiosity what does this number mean ?

This is the timestamp at which the exception class is created in the editor, every exception has it. The idea is to give a "unique" code for each exception of the library, this can help when debugging or other purposes.

FYI I borrowed this from the TYPO3 CMS, where this is (was?) a common practice.

Baptouuuu · 2025-01-18T14:56:35Z

src/Mapper/Tree/Message/NodeMessage.php

-    {
-        return $this->node;
-    }
+    public function __construct(


To ease future maintenance can this constructor be declared @internal ?

As discussed IRL I really like the idea, however I'll do it in a separate commit/PR. 😊

Baptouuuu · 2025-01-18T15:10:04Z

src/Mapper/Tree/Builder/ArrayNodeBuilder.php

    {
        $type = $shell->type();
        $value = $shell->value();

        assert($type instanceof ArrayType || $type instanceof NonEmptyArrayType || $type instanceof IterableType);

        if ($shell->enableFlexibleCasting() && $value === null) {
-            return TreeNode::branch($shell, [], []);
+            return Node::branch([]);


Why is it a ::branch() and not a ::leaf() ?

Baptouuuu · 2025-01-18T15:19:30Z

src/Mapper/Tree/Builder/ArrayNodeBuilder.php

-            if (! $child->isValid()) {
-                return null;
+            if (! $children[$key]->isValid()) {
+                $errors[] = $children[$key];


Maybe this is a micro optimisation but $children could be overwritten here with an empty array since any valid value will never be used afterward.

I don't really see much value in doing this. This method is already ~50 lines long, let's not add another line. 😛

Thanks for noticing it, though!

Baptouuuu · 2025-01-18T15:46:39Z

src/Mapper/Tree/Builder/Node.php

+    public static function leaf(mixed $value): self
+    {
+        return new self(value: $value, messages: []);
+    }
+
+    /**
+     * @param iterable<mixed>|object $value
+     * @param non-negative-int $childrenCount
+     */
+    public static function branch(iterable|object $value, int $childrenCount = 0): self


I have difficulties to understand what's a leaf and what's a branch since both can be used inside a same builder (like ObjectNodeBuilder).

Are these concepts applied on the expected structure or the input value ? Or both ?

It seems like it's both.

Yeah that's not obvious, and maybe that could be improved indeed (I'll check it after posting this message).

Done, it should now be easier to understand.

Baptouuuu · 2025-01-18T15:58:36Z

src/Mapper/Tree/Message/Messages.php

@@ -58,7 +50,7 @@
 */
 final class Messages implements IteratorAggregate, Countable
 {
-    /** @var list<NodeMessage> */
+    /** @var array<NodeMessage> */


Why the change from list to array ? It doesn't seem it can be anything else than a list.

I did it because array_filter usage breaks the "list". This property is internal anyway, and when looping over messages it will always be a list, see \CuyZ\Valinor\Mapper\Tree\Message\Messages::getIterator().

mastir · 2025-01-23T16:34:55Z

Nice! This is a great feature. I had some troubles understending error messages. Sometimes i have structures with 5+ levels depth and in most cases its typo in some field name or extra field returned from api. So i made my debugger to generate readable error trace:

} catch (MappingError $e) {
            $messages = [];
            foreach(new RecursiveIterableAggregate([$e->node()], function($value){
                return $value->children();
            }) as $item){
                if (!$item->isValid()) {
                    $messages[$item->path()] = ['type'=>$item->type(), 'name'=>$item->name(), 'source'=>$item->sourceFilled()?$item->sourceValue():'undefined'];
                }
            }
            throw new HttpException(400, $e->getMessage().json_encode($messages), previous: $e);
        }

Sounds like this part will be depricated soon. Thanks!

This commit brings a huge rework of how the recursive node system works under the hood during the mapping; it aims to simplify the architecture of this part of the library for several reasons: - Better memory management and overall performance: —> The `TreeNode` class has been reworked and will no longer maintain a reference to the `Shell` instance. —> The `Node` class has been removed and will no longer be part of the library's public API. This class used to give access to a lot of information to the user only when the mapping was failing. It was one of the worst architectural decisions made at the beginning of the project, because it would keep the entire tree of nodes created during the mapping (even nodes that were correct). This extra data was in the vast majority of cases (always?) not needed and led to a more complex codebase, as well as bad memory management. - API simplification — instead of working with instances of `Node` when the mapping fails, the user can now just call the newly introduced method `MappingError::messages()` and work directly with the messages. - Maintenance simplification — because the API has become simpler, it will be easier to maintain as well as easier to understand for newcomers. - Prepare for the future — this work was necessary for an upcoming project for this library: ✨ Mapper compilation ✨ which will bring huge performance gains. List of changes: ❗ Removed `\CuyZ\Valinor\Mapper\MappingError::node()` ❗ Removed `\CuyZ\Valinor\Mapper\Tree\Node` ❗ Removed `\CuyZ\Valinor\Mapper\Tree\NodeTraverser` ❗ Removed `\CuyZ\Valinor\Mapper\Tree\Message\Messages::flattenFromNode()` ❗ Removed `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::node()` ❗ Changed `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::__construct()` ➕ Added: `\CuyZ\Valinor\Mapper\MappingError::messages()` ➕ Added: `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::name()` ➕ Added: `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::path()` ➕ Added: `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::type()` ➕ Added: `\CuyZ\Valinor\Mapper\Tree\Message\NodeMessage::sourceValue()` Note: this work was done in collaboration with Baptiste Langlade, who I thank a lot for his help and support 😊

Baptouuuu reviewed Jan 18, 2025

View reviewed changes

romm added 2 commits January 24, 2025 18:45

qa: improve MappingError types

f098a0b

romm force-pushed the feat/node-rework branch from 818f82c to f098a0b Compare January 24, 2025 17:46

misc: simplify Node API

85f7ab5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: rework mapper node and messages handling #593

feat!: rework mapper node and messages handling #593

romm commented Jan 17, 2025 •

edited

Loading

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

Baptouuuu Jan 18, 2025

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

romm Jan 24, 2025

Baptouuuu Jan 18, 2025

romm Jan 24, 2025

mastir commented Jan 23, 2025

feat!: rework mapper node and messages handling #593

Are you sure you want to change the base?

feat!: rework mapper node and messages handling #593

Conversation

romm commented Jan 17, 2025 • edited Loading

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

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

mastir commented Jan 23, 2025

romm commented Jan 17, 2025 •

edited

Loading