Skip to content

Make ValidatorBuilder::isValid fail fast implicitly #1633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alganet wants to merge 1 commit into from
Closed

Make ValidatorBuilder::isValid fail fast implicitly #1633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/IsValid.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

/*
* SPDX-License-Identifier: MIT
* SPDX-FileCopyrightText: (c) Respect Project Contributors
*/

declare(strict_types=1);

namespace Respect\Validation;

interface IsValid extends Validator
{
public function isValid(mixed $input): bool;
}
25 changes: 17 additions & 8 deletions src/ValidatorBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,7 @@ public static function init(Validator ...$validators): self

public function evaluate(mixed $input): Result
{
$validator = match (count($this->validators)) {
0 => throw new ComponentException('No validators have been added.'),
1 => current($this->validators),
default => new AllOf(...$this->validators),
};

return $validator->evaluate($input);
return $this->getEvaluationTarget()->evaluate($input);
}

/** @param array<string|int, mixed>|string|null $template */
Expand All @@ -73,7 +67,13 @@ public function validate(mixed $input, array|string|null $template = null): Resu

public function isValid(mixed $input): bool
{
return $this->evaluate($input)->hasPassed;
$validator = $this->getEvaluationTarget();

if ($validator instanceof IsValid) {
return $validator->isValid($input);
}

return $validator->evaluate($input)->hasPassed;
}

/** @param array<string|int, mixed>|callable(ValidationException): Throwable|string|Throwable|null $template */
Expand Down Expand Up @@ -118,6 +118,15 @@ public function getName(): Name|null
return null;
}

private function getEvaluationTarget(): Validator
{
return match (count($this->validators)) {
0 => throw new ComponentException('No validators have been added.'),
1 => current($this->validators),
default => new AllOf(...$this->validators),
Copy link
Member

@henriquemoody henriquemoody Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal is improving the performance (or simplify) of the isValid() method, we could do this simply by using Circuit instead of AllOf here.

I like what you're doing with Generative, and I see that with it we would still have performance improvements for other "composite" rules (like AllOf, AnyOf), but then I would argue that what we're having in Generative:::isValid() should be something we use in Circuit uses

We could just have a different Rule interface (FailFastRule) what would have another method (evaluateWithFailFast()) so validators would implement that method and Circuit would use them. That way we make Circuit a true fail-fast validator.

PS.: The names I suggested are just illustrative, I don't think they're good.

Copy link
Member Author

@alganet alganet Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circuit does not apply itself recursively through all its children. If there's an AllOf inside it, that AllOf will be fully evaluated.

Generative goes deep.

Copy link
Member

@henriquemoody henriquemoody Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, that's why I'm suggesting this solution. Something like this:

// FailFastRule.php
interface FailFastRule
{
    public function evaluateWithFailFast(mixed $input): Result;
}

// AllOf.php
final class AllOf extends Composite implements FailFastRule
{
    // ...
    public function evaluateWithFailFast(mixed $input): Result
    {
        foreach ($this->validators as $validator) {
            $result = $validator instanceof Validator
                ? $validator->evaluateWithFailFast($input)
                : $validator->evaluate($input);

            if (!$result->hasPassed) {
                return $result;
            }
        }

        return $result;
    }
}

// Circuit.php
final class Circuit extends Composite
{
    public function evaluate(mixed $input): Result
    {
        foreach ($this->validators as $validator) {
            $result = $validator instanceof Validator
                ? $validator->evaluateWithFailFast($input)
                : $validator->evaluate($input);

            if (!$result->hasPassed) {
                return $result;
            }
        }

        return $result;
    }
}


// ValidatorBuilder.php

final readonly class ValidatorBuilder implements Validator, Nameable
{
    // ...

    public function isValid(mixed $input): bool
    {
        if ($this->validators === []) {
            throw new ComponentException('No validators have been added.');
        }

        $validator = new Circuit(...$this->validators);

        return $validator->evaluate($input)->hasPassed;
    }
    // ...

Copy link
Member

@henriquemoody henriquemoody Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major gain is that Circuit becomes a REAL fail fast validator, and it's not so different from what you're doing.

Copy link
Member Author

@alganet alganet Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function evaluateWithFailFast(mixed $input): Result;

I see your suggestion returns Result. What is the goal here? Build a partial Result instance?

I solved this by interfacing to boolean, so I don't have to deal with partials.

Circuit: ? $validator->evaluateWithFailFast($input)
[...]
ValidtorBuilder: $validator = new Circuit(...$this->validators);

Seems like you're only wrapping the first level of chains in this solution. Circuit can see other Circuits, but it's up to the user to wrap every single of the inner ones, or make sure they're at the beginning of the chain.

Consider this:

v::arrayVal()->allOf(...

In this case, the composite is not the first node in the chain. It would be wrapped by Circuit, but then, how can you fail fast the second allOf node?

I'm not sure we need an extra wrapping around Circuit every time we find a FailFastRule. I just decided to make that mechanism part of the interface itself.

This way, Generative becomes an inner path for getting booleans fast. Circuit remains as an useful tool for managing individual fast-fail reports with messages. There is no way to combine both (getting true fail fast and full Report instances at the same time).

Maybe I misunderstood your suggestion. My main blocker here is dealing with potentially partial Result instances. That is what I'm most curious about your suggestion, how to deal with that.

Copy link
Member Author

@alganet alganet Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finished the implementation and tests, and the PR is not an incomplete draft anymore. I added you as a reviewer.

Copy link
Member

@henriquemoody henriquemoody Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suggesting to use Circuit as a default only on isValid(), not on assert(), because indeed, it's up to the user to define how they'd like the results to be shown.

If we consider the following chain:

v::arrayVal()->allOf(...)

when we would use isValid() (not assert()), it would be translated into something like:

new Circuit(new ArrayVal(), new AllOf(...))

BUT, because Circuit calls FailFastValidator:: evaluateWithFailFast(), AllOf would fail-fast if one of its validators fail. Admittedly, that works best with AllOf and NoneOf than with the other ones, because returning partial results in AnyOf and OneOf can confusing for the user, because we would indeed return "partial results", and the message might not make sense with AnyOf and OneOf.

I meant "partial" because in the context of AllOf and NoneOf each child result makes sense on its own, not so much for AnyOf and OneOf

What is the goal here? Build a partial Result instance?

Your changes sparkle some ideas, and I thought we could indeed return "partial" Result instances. A better way of saying it is to return the child result directly, not the parent (in some cases)

With the changes I'm suggesting Circuit can fail-fast not only with the validators we give it, but those validators themselves could also fail-fast (even the Each validator). If someone has a v::circuit(v::intVal(), v::allOf(v::positive(), v::greaterThan(3)))->assert(-1), the AllOf validator would never evaluate v::greaterThan(3), and instead return the result of Positive(), but only because Circuit is the main wrapper.

Copy link
Member

@henriquemoody henriquemoody Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fair if we don't want to have a Circuit as a wrapper the whole time, we could call evaluateWithFailFast() directly on isValid()

Copy link
Member Author

@alganet alganet Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented locally with partial Result instances. It's a deep rabbit hole.

Ultimately, we could make Result completely lazy (it already carries the Validator instance it was obtained from), using property hooks for $hasPassed as I mentioned previously. This PR advances the internal engine towards that goal. The immutable list of children is a design issue though. It only goes as far as partials.

The truth is that getting a boolean will always be faster, even than partial or lazy Result instances. And it's simpler to implement, so it makes a lot of sense (to me) to start with a small measurable deliverable that does that. No changes to public interface, no complications, clear use case for all sorts of scenarios.

even the Each validator

That is a separate refactor, improving FilteredNonEmptyArray to deal with iterables (basically, making it FilteredNonEmptyIterable). There are challenges with no-rewind style iterables though.

Copy link
Member Author

@alganet alganet Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henriquemoody

I have updated the PR to only feature the boolean IsValid interface, and not deal with any generators, leaving space for us to decide on how to deal with result streaming later.

This still offers a significant performance improvement for obtaining boolean only results,

if (v::foo()->bar()->baz()->validate() is very a common usage pattern seeing in our users across GitHub. Examples:

https://github.com/search?q=%22Respect%5CValidation%22+AND+%22v%3A%3A%22+AND+%22validate%28%22&type=code

This also opens up possibilities for modes, such as --verbose/--quiet in (but not limited to) CLI apps, getting both performance in the quiet mode and throroughness in the verbose mode.

With this, you are free to implement your ideas around Circuit, as long as the benchmarks for single boolean do not display any performance regression (I would accept a small performance regression actually).

};
}

/** @param array<string|int, mixed>|string|null $template */
private function toResultQuery(Result $result, array|string|null $template): ResultQuery
{
Expand Down
18 changes: 17 additions & 1 deletion src/Validators/AllOf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace Respect\Validation\Validators;

use Attribute;
use Respect\Validation\IsValid;
use Respect\Validation\Message\Template;
use Respect\Validation\Result;
use Respect\Validation\Validator;
Expand All @@ -36,7 +37,7 @@
'{{subject}} must pass all the rules',
self::TEMPLATE_ALL,
)]
final class AllOf extends Composite
final class AllOf extends Composite implements IsValid
{
public const string TEMPLATE_ALL = '__all__';
public const string TEMPLATE_SOME = '__some__';
Expand All @@ -53,4 +54,19 @@ public function evaluate(mixed $input): Result

return Result::of($valid, $input, $this, [], $template)->withChildren(...$children);
}

public function isValid(mixed $input): bool
{
foreach ($this->validators as $validator) {
if ($validator instanceof IsValid) {
return $validator->isValid($input);
}

if (!$validator->evaluate($input)->hasPassed) {
return false;
}
}

return true;
}
}
22 changes: 21 additions & 1 deletion src/Validators/AnyOf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace Respect\Validation\Validators;

use Attribute;
use Respect\Validation\IsValid;
use Respect\Validation\Message\Template;
use Respect\Validation\Result;
use Respect\Validation\Validator;
Expand All @@ -28,7 +29,7 @@
'{{subject}} must pass at least one of the rules',
'{{subject}} must pass at least one of the rules',
)]
final class AnyOf extends Composite
final class AnyOf extends Composite implements IsValid
{
public function evaluate(mixed $input): Result
{
Expand All @@ -41,4 +42,23 @@ public function evaluate(mixed $input): Result

return Result::of($valid, $input, $this)->withChildren(...$children);
}

public function isValid(mixed $input): bool
{
foreach ($this->validators as $validator) {
if ($validator instanceof IsValid) {
if ($validator->isValid($input)) {
return true;
}

continue;
}

if ($validator->evaluate($input)->hasPassed) {
return true;
}
}

return false;
}
}
22 changes: 19 additions & 3 deletions src/Validators/NoneOf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace Respect\Validation\Validators;

use Attribute;
use Respect\Validation\IsValid;
use Respect\Validation\Message\Template;
use Respect\Validation\Result;
use Respect\Validation\Validators\Core\Composite;
Expand All @@ -32,7 +33,7 @@
'{{subject}} must pass all the rules',
self::TEMPLATE_ALL,
)]
final class NoneOf extends Composite
final class NoneOf extends Composite implements IsValid
{
public const string TEMPLATE_ALL = '__all__';
public const string TEMPLATE_SOME = '__some__';
Expand All @@ -41,8 +42,8 @@ public function evaluate(mixed $input): Result
{
$failedCount = 0;
$children = [];
foreach ($this->validators as $validator) {
$child = $validator->evaluate($input)->withToggledModeAndValidation();
foreach ($this->validators as $child) {
$child = $child->evaluate($input)->withToggledModeAndValidation();
$children[] = $child;
if ($child->hasPassed) {
continue;
Expand All @@ -59,4 +60,19 @@ public function evaluate(mixed $input): Result
count($children) === $failedCount ? self::TEMPLATE_ALL : self::TEMPLATE_SOME,
)->withChildren(...$children);
}

public function isValid(mixed $input): bool
{
foreach ($this->validators as $validator) {
if ($validator instanceof IsValid) {
return !$validator->isValid($input);
}

if ($validator->evaluate($input)->hasPassed) {
return false;
}
}

return true;
}
}
29 changes: 28 additions & 1 deletion src/Validators/OneOf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
namespace Respect\Validation\Validators;

use Attribute;
use Respect\Validation\IsValid;
use Respect\Validation\Message\Template;
use Respect\Validation\Result;
use Respect\Validation\Validator;
Expand All @@ -38,7 +39,7 @@
'{{subject}} must pass only one of the rules',
self::TEMPLATE_MORE_THAN_ONE,
)]
final class OneOf extends Composite
final class OneOf extends Composite implements IsValid
{
public const string TEMPLATE_NONE = '__none__';
public const string TEMPLATE_MORE_THAN_ONE = '__more_than_one__';
Expand Down Expand Up @@ -66,4 +67,30 @@ public function evaluate(mixed $input): Result

return Result::of($valid, $input, $this, [], $template)->withChildren(...$children);
}

public function isValid(mixed $input): bool
{
$passed = 0;
foreach ($this->validators as $validator) {
if ($passed > 1) {
return false;
}

if ($validator instanceof IsValid) {
if ($validator->isValid($input)) {
$passed++;
}

continue;
}

if (!$validator->evaluate($input)->hasPassed) {
continue;
}

$passed++;
}

return $passed === 1;
}
}
78 changes: 78 additions & 0 deletions tests/benchmark/CompositeValidatorsBench.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

/*
* SPDX-License-Identifier: MIT
* SPDX-FileCopyrightText: (c) Respect Project Contributors
*/

declare(strict_types=1);

namespace Respect\Validation\Benchmarks;

use Generator;
use PhpBench\Attributes as Bench;
use Respect\Validation\Validator;
use Respect\Validation\ValidatorBuilder;
use Respect\Validation\Validators\Alnum;
use Respect\Validation\Validators\Alpha;
use Respect\Validation\Validators\BoolType;
use Respect\Validation\Validators\Digit;
use Respect\Validation\Validators\Even;
use Respect\Validation\Validators\FloatType;
use Respect\Validation\Validators\IntType;
use Respect\Validation\Validators\Negative;
use Respect\Validation\Validators\Positive;
use Respect\Validation\Validators\StringType;

final class CompositeValidatorsBench
{
/** @param array{string, array<Validator>} $params */
#[Bench\ParamProviders(['provideValidatorBuilder'])]
#[Bench\Iterations(5)]
#[Bench\Revs(50)]
#[Bench\Warmup(1)]
#[Bench\Subject]
public function isValid(array $params): void
{
ValidatorBuilder::__callStatic(...$params)->isValid(42);
}

public function provideValidatorBuilder(): Generator
{
yield 'allOf(10)' => ['allOf', $this->buildValidators(10)];
yield 'oneOf(10)' => ['oneOf', $this->buildValidators(10)];
yield 'anyOf(10)' => ['anyOf', $this->buildValidators(10)];
yield 'noneOf(10)' => ['noneOf', $this->buildValidators(10)];
yield 'allOf(100)' => ['allOf', $this->buildValidators(100)];
yield 'oneOf(100)' => ['oneOf', $this->buildValidators(100)];
yield 'anyOf(100)' => ['anyOf', $this->buildValidators(100)];
yield 'noneOf(100)' => ['noneOf', $this->buildValidators(100)];
}

/** @return array<Validator> */
private function buildValidators(int $count): array
{
$validators = [];
for ($i = 0; $i < $count; $i++) {
$validators[] = $this->makeValidator($i);
}

return $validators;
}

private function makeValidator(int $index): Validator
{
return match ($index % 10) {
0 => new IntType(),
1 => new Positive(),
2 => new Negative(),
3 => new Even(),
4 => new FloatType(),
5 => new StringType(),
6 => new Alpha(),
7 => new Alnum(),
8 => new Digit(),
default => new BoolType(),
};
}
}
52 changes: 52 additions & 0 deletions tests/unit/ValidatorBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

/*
* SPDX-License-Identifier: MIT
* SPDX-FileCopyrightText: (c) Respect Project Contributors
*/

declare(strict_types=1);

namespace Respect\Validation;

use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\Attributes\Test;
use Respect\Validation\Exceptions\ComponentException;
use Respect\Validation\Test\TestCase;
use Respect\Validation\Test\Validators\Stub;
use Respect\Validation\Validators\AllOf;

#[Group('validator')]
#[CoversClass(ValidatorBuilder::class)]
final class ValidatorBuilderTest extends TestCase
{
#[Test]
public function shouldDelegateToIsValidWhenSingleValidatorInBuilder(): void
{
$builder = ValidatorBuilder::init(new AllOf(Stub::pass(1), Stub::pass(1)));

self::assertTrue($builder->isValid([]));
}

#[Test]
public function shouldCallIsValidOnCombinedIsValidWhenMultipleValidatorsExist(): void
{
$builder = ValidatorBuilder::init(
Stub::pass(1),
Stub::pass(1),
Stub::pass(1),
Stub::fail(1),
);

self::assertFalse($builder->isValid([]));
}

#[Test]
public function shouldThrowComponentExceptionWhenNoValidatorsExist(): void
{
$this->expectException(ComponentException::class);

ValidatorBuilder::init()->isValid([]);
}
}
Loading
Loading