Respect / Validation

The most awesome validation engine ever created for PHP
https://respect-validation.readthedocs.io
MIT License
5.79k stars 776 forks source link

ValidationException::getMessages behaves inconsistently when validation rules are instances of Validator #1347

Closed dakujem closed 1 year ago

dakujem commented 3 years ago

Hello,

I found a strange behaviour when creating nested and in general more complex validators.

Whenever I use v::any_rule_here static shortcut, which is quite comfortable, I encounter problems with validation messages. This happens, because internally, v::__callStatic calls v::create() and creates a new instance of v (Validator).

See the code and output below.

Problem no. 1\ While the error output (using Exception::getFullMessage()) of $validator1 and $validator2 is the same, the key in the returned array (using Exception::getMessages()) is different.

Problem no. 2\ It gets much worse with nested rules, where the messages array does not even contain messages for the failed rules that are Validator instances themselves, compare the error output of $validator3 and $validator4 .

What I need is to get all the failed rule names (data keys) with all the failed error messages and pass them as a response to an API request, so taht the consumer can map the error mesages to respective form inputs or whatever. It is becomming overly too difficult task for such a trivial feature.

In the code below, the $validator1 and $validator2 should be equivalent, and $validator3 and $validator4 as well.\ They validate the rules just fine, except for the error messages.

Full PHP code ```php // a single rule $validator1 = (new Validator()) ->addRule(Validator::key('missing1')); $validator2 = (new Validator()) ->addRule(Factory::getDefaultInstance()->rule('key', ['missing1'])); // a pair of rules $validator3 = (new Validator()) ->addRule(Validator::key('missing1')) ->addRule(Validator::key('foo', Validator::number())); $validator4 = (new Validator()) ->addRule(Factory::getDefaultInstance()->rule('key', ['missing1'])) ->addRule(Factory::getDefaultInstance()->rule('key', ['foo', Validator::number()])); $data = ['foo' => 'bar']; $export = function (ValidationException $e): void { echo "\n\n"; echo (string)$e->getFullMessage(); echo "\n\n"; var_export($e->getMessages()); echo "\n"; echo "\n\n-------"; }; try { $validator1->assert($data); } catch (ValidationException $e) { $export($e); } try { $validator2->assert($data); } catch (ValidationException $e) { $export($e); } try { $validator3->assert($data); } catch (ValidationException $e) { $export($e); } try { $validator4->assert($data); } catch (ValidationException $e) { $export($e); } ```
Full output ``` ------- $validator1 - All of the required rules must pass for `{ "foo": "bar" }` - missing1 must be present array ( 'validator' => 'missing1 must be present', ) ------- $validator2 - All of the required rules must pass for `{ "foo": "bar" }` - missing1 must be present array ( 'missing1' => 'missing1 must be present', ) ------- $validator3 - All of the required rules must pass for `{ "foo": "bar" }` - All of the required rules must pass for `{ "foo": "bar" }` - missing1 must be present - All of the required rules must pass for `{ "foo": "bar" }` - foo must be a number array ( 'validator' => 'foo must be a number', ) ------- $validator4 - All of the required rules must pass for `{ "foo": "bar" }` - missing1 must be present - foo must be a number array ( 'missing1' => 'missing1 must be present', 'foo' => 'foo must be a number', ) ```

Thanks for any info. If this is a bug, I could provide a PR, but it would take time...

dakujem commented 3 years ago

I tried to create my own iterator, that would produce a nested array of exceptions and then convert it to messages and flatten using the dot notation.

I failed, because the exception IDs do not reflect the data structure being validated, rather, they reflect the structure of the validators. Instead of

[
    'dimensions.width' => "Message here...."
]

I'm getting:

[
    'dimensions.dimensions.width.width' => "Message here...."
]

This is the validator:

        $positiveNumber = fn() => v::allOf(v::number(), v::positive());

        $rules = [
            // ID must be numeric, if present
            v::key('id', $positiveNumber(), false),

            // If any of the dimension values is present, all 3 dimensions and a unit must be present.
            v::key(
                'dimensions',
                v::allOf(
                    v::key('width', $positiveNumber()),
                    v::key('length', $positiveNumber()),
                    v::key('height', $positiveNumber()),
                    v::key('unit', $notEmptyMax255())->setTemplate('The product\'s dimension unit must be present.'),
                )->setTemplate('Product\'s {{name}} must be a positive integer.'),
                false, // optional rule
            ),

            // If weight is present, both amount and unit must be specified.
            v::key(
                'weight',
                v::allOf(
                    v::key('amount', $positiveNumber()),
                    v::key('unit', $notEmptyMax255()),
                ),
                false,
            ),

        ];
        return new Validator(...$rules);

And this is how I iterate:

    public static function extractMessages(array $exceptions): array
    {
        $messages = [];
        foreach ($exceptions as $exception) {
            $id = $exception->getId();
            if ($exception instanceof NestedValidationException) {
                $children = $exception->getChildren();
                if ($children !== []) {
                    $messages[$id] = static::extractMessages($children);
                }
            }
            if (!isset($messages[$id])) {
                $messages[$id] = static::renderMessage($exception);
            }
        }
        return $messages;
    }

Any idea how I could reflect the data structue and not the validator structure ?

alganet commented 1 year ago

In your case, you can use setName to inform the validator of the structure:

$notEmptyMax255 = fn() => v::allOf(v::notEmpty(), v::max(255));
$positiveNumber = fn() => v::allOf(v::number(), v::positive());

$rules = [
    // ID must be numeric, if present
    v::key('id', $positiveNumber(), false)->setName('id'),

    // If any of the dimension values is present, all 3 dimensions and a unit must be present.
    v::key(
        'dimensions',
        v::allOf(
            v::key('width', $positiveNumber())->setName('width'),
            v::key('length', $positiveNumber())->setName('length'),
            v::key('height', $positiveNumber())->setName('height'),
            v::key('unit', $notEmptyMax255())->setTemplate('The product\'s dimension unit must be present.'),
        )->setTemplate('Product\'s {{name}} must be a positive integer.'),
        false, // optional rule
    )->setName('dimensions'),

    // If weight is present, both amount and unit must be specified.
    v::key(
        'weight',
        v::allOf(
            v::key('amount', $positiveNumber())->setName('amount'),
            v::key('unit', $notEmptyMax255())->setName('unit'),
        ),
        false,
    )->setName('weight'),

];

try {
    (new v(...$rules))->assert(['id' => null, 'dimensions' => [], 'weight' => null]);
} catch (NestedValidationException $e) {
    print_r($e->getMessages());
}

This outputs:

Array
(
    [id] => id must be positive
    [dimensions] => Array
        (
            [width] => Product's width must be a positive integer.
            [length] => Product's length must be a positive integer.
            [height] => Product's height must be a positive integer.
        )

    [weight] => Array
        (
            [amount] => amount must be present
            [unit] => unit must be present
        )

)

I hope that helps!

dakujem commented 1 year ago

Yes, this seems like what I wanted. Except it is unnecessarily verbose, compared to the otherwise concise interface.

alganet commented 1 year ago

For a less verbose declararion, you can use keySet instead of allOf:

$notEmptyMax255 = fn() => v::allOf(v::notEmpty(), v::max(255));
$positiveNumber = fn() => v::allOf(v::number(), v::positive());

$rules = [
    // ID must be numeric, if present
    v::key('id', $positiveNumber(), false),

    // If any of the dimension values is present, all 3 dimensions and a unit must be present.
    v::key(
        'dimensions',
        v::keySet(
            v::key('width', $positiveNumber()),
            v::key('length', $positiveNumber()),
            v::key('height', $positiveNumber()),
            v::key('unit', $notEmptyMax255())->setTemplate('The product\'s dimension unit must be present.'),
        )->setTemplate('Product\'s {{name}} must be a positive integer.'),
        false, // optional rule
    ),

    // If weight is present, both amount and unit must be specified.
    v::key(
        'weight',
        v::keySet(
            v::key('amount', $positiveNumber()),
            v::key('unit', $notEmptyMax255()),
        ),
        false,
    ),

];

try {
    v::keySet(...$rules)->setName('Input Object')->assert(['id' => null, 'dimensions' => [], 'weight' => ['amount' => null, 'unit' => null]]);
} catch (NestedValidationException $e) {
    print_r($e->getMessages());
}

Which outputs:

Array
(
    [Input Object] => Array
        (
            [id] => id must be positive
            [dimensions] => Product's dimensions must be a positive integer.
            [weight] => Array
                (
                    [amount] => amount must be positive
                    [unit] => unit must be less than or equal to 255
                )

        )

)
dakujem commented 1 year ago

Yes, this looks promising. I might give it a try in the future. Thanks for the tips.