Expand API of `CuyZ\Valinor\Mapper\Tree\Message\Message` to allow for i18n and API customization?

[Ocramius]

Currently, Message is only an instance of Stringable: https://github.com/CuyZ/Valinor/blob/396f64a5246ccfe3f6f6d3211bac7f542a9c7fc6/src/Mapper/Tree/Message/Message.php#L9-L11

When using this library, you can expect the most common usage to be (pseudo-code):

try {
    parse_validate_and_map($request);
} catch (MappingError $failed) {
    response_code(422);
    mapping_error_to_failure_response($failed);
}

In order to effectively write mapping_error_to_failure_response(), especially considering concerns like translation, i18n, and general re-shaping of the response, it would be helpful to have:

• the internal message (like now), valid for loggers and such
• a declared code (currently, Exception#getCode() is used, but unsure if it is stable long-term?)
• the original value (which failed mapping - at the level we're at) - this is useful for interpolating in error messages
• the wished type (to which we failed to cast/map towards) - this is useful for interpolating in error messages
• the logical path of the value, not the string as key in MappingError, useful for processing/filtering errors later on

In practice, the use-case is mostly around i18n of error messages, plus providing a bit more information on each Message.

A good example of such serialization is https://datatracker.ietf.org/doc/html/rfc7807

[romm]

Hi @Ocramius, thanks for the suggestion.

Instead of modifying the Message signature, I'd suggest something else, let me know what you think about it.

Currently, when the mapping fails, an instance of MappingError is thrown, the only existing implementation by now is CannotMapObject. This is the entry point for users when something goes wrong, and I assumed that giving access to the describe method like below makes sense:

https://github.com/CuyZ/Valinor/blob/ea842e2efb94ded9f9f2ebce1484f8d4e7e29237/src/Mapper/Exception/CannotMapObject.php#L33-L56

My suggestion is this: removing the describe method and give access to a new method:

interface MappingError extends Throwable
{
    public function rootNode(): Node;
}

This way, something like this could be done:

try {
    parse_validate_and_map($request);
} catch (MappingError $failed) {
    $printError = static function(Node $node) use (&$printError): void {
        if ($node->isValid()) {
            return;
        }

        foreach ($node->messages() as $message) {
            if ($message instanceof Throwable) {
                echo "path: {$node->path()}" . PHP_EOL;
                echo "expected type: {$node->type()}" . PHP_EOL;
                echo "message: {$message->getMessage()}" . PHP_EOL;
                if ($message instanceof HasCode) {
                    echo "code: {$message->code()}" . PHP_EOL;
                }
            }
        }

        foreach ($node->children() as $child) {
            $printError($child);
        }
    };

    $printError($failed->rootNode());
}

The missing part in the code above is the original value, which is currently not accessible from outside the Node — but it could be accessed from Node::$shell->value() with a method that gives access to it somehow.

WDYT?

[romm]

Hi @Ocramius, I've done some work on this topic, could you check #38?

More information in the chapter "Validation" of the README.

[aurimasniekis]

I think for the validation side of things it would be nice to have something similar to symfony/validator with ConstraintViolation and ConstraintViolationList and then MappingError::getViolations() or something similar could return a flat list of errors, with messageTemplate and similar thingies would make whole i18n side of things really nice and friendly.

[romm]

Hi @aurimasniekis, being able to get a flat list of errors might be useful indeed; I already kept an open door for this feature — see https://github.com/CuyZ/Valinor/pull/38#issue-1081593482. Whether it should be part of the library or not is still to discuss, please tell me what you think of it.

Concerning the messageTemplate feature, there's probably still some work to do. With the current status of the API, how would you expect to be able to extend it and add i18n features? TBH I was waiting for @Ocramius's feedback to have a more precise thought of the direction to take; I know he's been very busy lately, we can surely keep up working on this topic in the meantime. 🙂

[aurimasniekis]

The messageTemplate would basically provide the needed i18n support because it would give you an error message template with variables, so then u can translate them and pass variables to any translation library.

[romm]

Hi @Ocramius / @aurimasniekis, I've made major work on this topic, can you check #52 and tell me what you think about it? I think this is a good approach, but you may have feedback on it. 🙂

[ro0NL]

Hi :wave: looks neat here.

I'm on a journey of deserializing/validating/mapping payloads to domain models, and found this repo.

My main concern is exactly this topic, and i think https://github.com/CuyZ/Valinor/issues/2#issuecomment-1001774650 gets it right.

symfony/validator brings full i18n compatibility: https://symfony-translations.nyholm.tech/, an object constraint mapping system to read from (let it be PHP8 attributes soley), as well as a ConstraintViolationList to help further response serialization.

My main question would be: can we read the constraint attributes from the target object, while applying them on the source input? Thus achieving "validation before hydration" with i18n friendly error handling. And are you willing to build/maintain such a bridge? Or first-class support maybe? :angel:

Note, under the hood it can transform real type info to a Type constraint (if not already present), to leverage its i18n translation message.

I tend to believe if we have SF community on our side, this lib could sky-rocket :)

[romm]

Hi @ro0NL, thanks for your interest!

To answer your main question: yes, the library is able to read the attributes from the nodes (as I call them in the library) — they can come from class properties or construct parameters. Now the question whether these attributes should be used for validating an input or not is another topic, let me explain my thoughts on this.

One of the main aspect of this library that I try to stand to, is that it should be as much independent to the objects it is mapping as possible. These objects are supposed to be DTO/VO that could in theory be mapped manually; in the end this library is just here to do the recursive type casting automatically.

PHP attributes are metadata; adding a "validation attribute" (a constraint in Symfony's vocabulary) to an object does not guarantee it will be used in every case — for instance if the object is instantiated manually.

Take this example:

final class SomeClass
{
    #[\Symfony\Component\Validator\Constraints\Email]
    public readonly string $email;
}

(new Foo())->email = 'not an email';

The email property is hydrated with a value that is not "supposed" to be correct, yet this example makes sense and the state of the object is perfectly valid, yet the business rule associated to it is not fulfilled.

In my opinion, this should be done like this:

final class SomeClass
{
    public function __construct(
        public readonly string $email
    ) {
        if (! some_email_check($email)) {
            throw new Exception('invalid email');
        }
    }
}

new Foo('not an email'); // error

Now I understand the concern you have with the full i18n support from Symfony, and I'm sure we can work on finding a way to have a bridge that can help, without having to use attributes.

There are two things: the first one it being able to translate the messages. Well, that's kind of easy and not really related to the symfony/validator package, for instance:

try {
    return $this->mapperBuilder
        ->mapper()
        ->map(SomeClass::class, [/* … */]);
} catch (MappingError $exception) {
    $node = $exception->node();
    $messages = new \CuyZ\Valinor\Mapper\Tree\Message\MessagesFlattener($node);

    $translator = new \Symfony\Component\Translation\Translator('fr_FR');
    $translator->addLoader('xliff', new XliffFileLoader());
    $translator->addResource('xliff', 'vendor/symfony/validator/Resources/translations/validators.fr.xlf', 'fr_FR');

    foreach ($messages as $message) {
        echo $translator->trans((string)$message);
    }
}

Like this we can use any string that is part of the translated strings from Symfony's libraries.

Now the second part is more tricky, it would be using the constraints API without the attributes.

final class SomeClass
{
    public function __construct(
        public readonly string $email
    ) {
        $validator = \Symfony\Component\Validator\Validation::createValidator();
        $violations = $validator->validate($foo, new \Symfony\Component\Validator\Constraints\Email());

        if (count($violations) === 0) {
            return;
        }

        throw new ThrowableMessage($violations->get(0)->getMessage());
    }
}

Note that with the current API it is possible to "throw" only one constraint, but we could imagine that the library could support aggregate message that would be flattened by the mapper.

What are your thoughts? 🙂

[ro0NL]

I'd say (business) validation is a service-layer concern. A complex constraint may require eg. a DB connection.

While i strive for "data models in valid state", im not nessecarily fan of them having runtime assertions all over. In that sense i prefer the type system to do it's work (let it be enhanced by psalm/phpdoc).

As such, to me new Foo('not an email'); // error is a business rule violation, that should bubble up somewhere. Or maybe it's intended to set this value ... that's an implementation detail we don't nessecarily know. Ultimately the validator can still be applied on the mapped object, if sources are uncertain.

However, to satisy any potential system assertion i tend to prefer pre-validating the input, rather than catching some hydration error (and then have this issue of how to derive a i18n error from it). In that sense, after validating any mapping exception can bubble IMHO (or caught for some logging/special handling purpose).

To make this stupidly-simple from a Symfony app POV, i'd aim for a minimal user API:

try {
  $data = map(Some::class, $reques->getContent());
}
catch (InputError $e) {
  return serialize($e->getViolations()); // i18n errors
}
// catch (MappingError $e) {
//   $logger->error(...);
// }

Thus given

class Some {
  #[\Symfony\Component\Validator\Constraints\Email]
  public string $email;
}

Expected valid: {"email": "foo@bar.baz"} Expected invalid {}, {"email": true}, {"email": "foo"}

The invalid cases should have i18n errors.

Note i have no super strong opionion about pre-validating the input vs. post-validating the mapped object technically (i think semantically it's more pure though). But post-validating requires another solution for i18n type errors, for this library to solve.

And maybe this library should already consider i18n decoding errors, for which SF has no translations available AFAIK. But that's least of my concern :)

[ro0NL]

Btw see also https://symfony.com/blog/new-in-symfony-5-4-serializer-improvements#collect-denormalization-type-errors for how Symfony, eh, does it.

IMHO it suffers the same design issue ... that is; a ton of boilerplate and still no i18n type-errors nor i18n value-errors out of the box :joy:

It can also leak a partial (mapped) object .. let's not go there :)

I understand this is maybe a lot of hassle, just to have i18n type-errors :) (assuming we'd post-validate the mapped object still using symfony/validator for the remaining i18n value-errors)

IMHO ideally we get the i18n type- and value-errors in one go ... otherwise i'll take things for granted and have system type-errors facing end-users :shrug: it's not the end of world :grin:

I've no practical experience yet with your library, so to not hijack this issue ... if the current API works for OP (@Ocramius) ... feel free to ignore me :)

[romm]

I'd say (business) validation is a service-layer concern. A complex constraint may require eg. a DB connection. … However, to satisy any potential system assertion i tend to prefer pre-validating the input, rather than catching some hydration error

The issue with pre-validating the input is that you have no idea what's inside, you'd have to check the types for every parameter, and worst if the value you seek is deeply nested. The goal of this library is to help with formatting any input to a known nested structure — which can then be used to do the business logic associated to it — while aggregating low-level errors.

As you said complex constraint is a service-layer concern — I agree, and this is out of the scope of this library and shall be handled correctly by the domain of an application, let it be "inside" a command/query bus for instance.

And maybe this library should already consider i18n decoding errors, for which SF has no translations available AFAIK. But that's least of my concern :)

Well we could of course imagine providing i18n for existing errors of course, I'll think about providing a basic list for it.

Btw see also https://symfony.com/blog/new-in-symfony-5-4-serializer-improvements#collect-denormalization-type-errors for how Symfony, eh, does it.

IMHO it suffers the same design issue ... that is; a ton of boilerplate and still no i18n type-errors nor i18n value-errors out of the box 😂

While it looks a bit different from how you'd use Valinor to flatten messages, I don't think that's too much to do. Plus a helper could probably be used to help any controller/service to do it with only one line (IMO).

It can also leak a partial (mapped) object .. let's not go there :)

I fully agree on this one. 😁

IMHO ideally we get the i18n type- and value-errors in one go

For the reasons I listed above, I really think that "complex value errors" can not and will not be somehow handled by this library, although this may change if people really see a benefit to it and we find a proper way to do it.

I've no practical experience yet with your library, so to not hijack this issue ... if the current API works for OP (Ocramius) ... feel free to ignore me :)

I think Ocramius can unsubscribe from this thread if he wants not to be disturbed 😅 — in the meantime I'm very happy to have feedback so don't feel forced to stop the discussion! I think we are still in the scope of the original topic so we can continue there.

[ro0NL]

The goal of this library is to help with formatting any input to a known nested structure

fair :) then my main concern is still the boilerplate involved to achieve i18n type-errors out-of-the-box-ish.

I really think that "complex value errors" can not and will not be somehow handled by this library

also fair :) for the record, all im suggesting is to invoke symfony/validator either on input, or the mapped object, to activate "its" constraint validation. If we decide to validate the mapped object, then perhaps this is out-of-scope here i agree. And i'll give up on pursuing validation-before-hydration :')

[ro0NL]

I don't think that's too much to do

i dont need the boilerplate in every controller for sure ... so im curious who will provide the ultimate abstraction eventually. Time will tell :)

last but not least, maybe this should be considered from a ValinorBundle POV

1 service for trusted inputs 1 service for untrusted inputs (that integrates symfony/validator, and thus symfony/translation)

[romm]

fair :) then my main concern is still the boilerplate involved to achieve i18n type-errors out-of-the-box-ish.

Right, I'll think about it.

In the meantime, please note that @framjet began a Symfony bundle to integrate this library in the framework, see https://github.com/framjet/php-valinor-bundle — maybe that's where we could have some helper for automatic i18n.

[romm]

Hi there, I recently merged #106 which improves a lot the way the messages can be handled by the library.

You can check out the new documentation chapter about it; I'd love to have some feedback on it. 🙂

[Ocramius]

Awesome! Closing here, since the initial proposal is very much fulfilled. Thanks @romm!