laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

Rule::unique doesn't handle a nested where condition properly #44448

Closed tom-skilltech closed 1 year ago

tom-skilltech commented 2 years ago

Description:

When using a nested where on a Rule::unique rule the given restrictions are not resolved to the correct string, for example, the following behave differently but unless I've misunderstood the documentation these should still result in similar behaviour.

This is from the docs "Adding Additional Where Clauses:" section of the validation docs:

'email' => Rule::unique('users')->where(fn ($query) => $query->where('account_id', 1))
// or
'email' => [
    Rule::unique('users')->where(fn ($query) => $query->where('account_id', 1))
]

results in the compiled rule string of unique:users,NULL,NULL,id

Which doesn't appear to actually take into account the account_id condition. If I adjust this to not be a callback:

Rule::unique('users')->where('account_id', 1)

results in the compiled rule string of unique:users,NULL,NULL,id,account_id,"1" and does then appear to take into account the extra condition.

I believe, after little code dive, this is related to the ->where(callback) version being added to the rule's internal usings array as a callback. Which is then, from what I can see, used within the validateUnique function call on the following line (around line 870 for me):

if ($this->currentRule instanceof Unique) {
     $extra = array_merge($extra, $this->currentRule->queryCallbacks());
}

However, this, if condition I believe will always be false as the rule has been converted to a string before this point and so I do not believe there's an instance where currentRule would still be an instance of unique.

I'm not sure which version changed this, however, this worked fine for us on version 6 but we have recently become aware of this in version 9.

Steps To Reproduce:

  1. Create a rule with the Unique rule and a where restraint as per the documentation
  2. Try and submit a request which should succeed due to the where restraint
driesvints commented 1 year ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

tom-skilltech commented 1 year ago

Hey @driesvints ,

Thanks for your reply, annoyingly I'm struggling to get this reproduced, but I do remember reliably doing it when I raised the bug. I'll close this for now and reopen it with a repo if I manage to come across it again.

Thanks!

haakym commented 1 year ago

@tom-skilltech I was taking a look at your issue recently, if you pull down laravel/framework take a look at tests/Validation/ValidationUniqueRuleTest.php and you will find the method testItCorrectlyFormatsAStringVersionOfTheRule, I believe you can write a failing test with something like this:

        $rule = new Unique('table');
        $rule->where(function ($query) {
            $query->where('foo', 'bar');
        });
        $this->assertSame('unique:table,NULL,NULL,id,foo,"bar"', (string) $rule);

and that's about as far as I've got. I'm currently trying to get my head round the related classes and methods, for example:

Not sure if any of this helps, but will update if I make any progress.

tom-skilltech commented 1 year ago

From my attempt yesterday the string version of the rule does indeed miss the nested where conditions. However, the validator stores the non-string version and processes it, which does take into account the nested where.

When I was hitting this issue the other week, the validator was not storing the non-string version for some reason, causing it to not be able to use the nested wheres against the unique rule.

if ($this->currentRule instanceof Unique) {
     $extra = array_merge($extra, $this->currentRule->queryCallbacks());
}

That line is what parses the nested conditions, but for me $this->currentRule was not an instance of Unique but rather was always the string version, if I removed the nesting the conditions would be in the string if I had the nesting they would not.

Unfortunately, I couldn't reproduce that behaviour yesterday, but will keep an eye out for it as I thought I was reliably reproducing it 😞

Would be interested if you're able to reproduce it so that it causes a failed validation, as I believe the string not matching is expected in this instance due to how the nested conditions are stored on the class and parsed when validating.