Skip to content
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.

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

[9.x] Precognitive validation with nested arrays doesn't throw validation error #45405

Merged

Conversation

morloderex
Copy link
Contributor

@morloderex morloderex commented Dec 23, 2022

Hello everybody,

I belive i have found a bug on the way the filterPrecognitiveRules method on the CanBePrecognitive trait works. The reason is that the string nested.*.name have not being parsed to the corisponding nested.0.name yet because the validator instance only looks after nested.*.name and builds the rules array based on the incoming data turning nested.*.name into n number of elements. This means that when we try to filter for only keys containing nested and nested.0.name within filterPrecognitiveRules our rules haven't been parsed yet which mean that the rule for key nested.0.name just get dropped. And never actually validated.

I have attached a test case for this bug.

@taylorotwell taylorotwell marked this pull request as draft December 23, 2022 14:27
@timacdonald timacdonald changed the title Precognitive validation with nested arrays doesn't throw validation error [9.x] Precognitive validation with nested arrays doesn't throw validation error Dec 28, 2022
@timacdonald
Copy link
Member

timacdonald commented Dec 28, 2022

Thanks so much for providing a failing test @morloderex. I've addressed the issue and will summarise things here.

Issue 1

It is possible with Precognition to specify the validation rules to run from the client, via a Precognition-Validate-Only header.

POST /posts HTTP/1.1
Precognition true
Precognition-Validate-Only members,members.0.email

{ /* ... */ }

The backend then runs a filter over the validation rules.

$initialRules = $formRequest->rules();

$only = explode(',', $request->header('Precognition-Validate-Only'));

$rules = collect($initialRules)->only($only)->all();

The problem is that $initialRules is populated directly from the form request's rules() method. If a rule contains a *, it has not yet been expanded by the validator.

public function rules()
{
    return [
        'members.*.name' => 'required|string',
        'members.*.email' => 'required|email',
    ];
}

So we are unable, from the client, to get validation errors for members.1.email, when using the Precognition-Validate-Only header.

The proposed fix for this is:

  1. Retrieve the rules from the validator after they have been parsed and expanded.
  2. Run the precognition filter.
  3. Set the rules back on the validator after filtering.

It is unfortunate to have to double set the validation rules. I could look at expanding them manually, but I feel it is best to just let the validator handle things internally - especially because then we don't need to worry about handling $implicitAttributes.

Issue 2

When using the ValidatesRequests::validateWith($validator) method and passing through a Validator instance, rules were retrieved from the validator instance and passed to the precognition filter method. We would then re-set the rules on the validator with the filter result.

This will sound like the solution we have used for issue 1, however we are using Validator::getRules() which does not cleanup any placeholders.

Validator::make([], ['escaped\.dot' => 'required'])->getRules()

//  [
//     "escaped9LQiXghrbE8XL1Z8dot" => [
//       "required",
//     ],
//   ]

The introduction of getRulesWithoutPlaceholders will allow developers to get the validation rules without any placeholder values in the keys.

Validator::make([], ['escaped\.dot' => 'required'])->getRulesWithoutPlaceholders()

//  [
//     "escaped\.dot" => [
//       "required",
//     ],
//   ]

So now when using ValidatesRequests::validateWith($validator) it is possible to filter escaped dots in rules.

POST /posts HTTP/1.1
Precognition true
Precognition-Validate-Only escaped\.dot

{ /* ... */ }

The ability to filter escaped dots was already possible via all the other means of using Precognition, such as FormRequests, $request->validate(), etc. So this fix standardises the behaviour across everything.

A note on escaped keys

Using escaped keys with Precognition does mean that you need to escape your key in:

  1. The rules array passed the the validator.
  2. The Precognition header.

However, the unescaped key is to be used in:

  1. The data payload.
  2. The error bag.

This is not a change, just some context I'm putting here for others.

{
return collect($this->rules)
->mapWithKeys(fn ($value, $key) => [
str_replace($this->dotPlaceholder, '\\.', $key) => $value,
Copy link
Member

Choose a reason for hiding this comment

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

This is not using the existing method for replacing placeholders, as it does not restore the backslash.

Copy link
Member

@timacdonald timacdonald left a comment

Choose a reason for hiding this comment

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

Ready for review.

@timacdonald timacdonald marked this pull request as ready for review December 29, 2022 01:06
@morloderex
Copy link
Contributor Author

@timacdonald thank a lot for taking a look at this. This bug has bitten me a bit.

I hope it can be fixed with the next release.

@taylorotwell taylorotwell merged commit 7797de8 into laravel:9.x Dec 31, 2022
@taylorotwell
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants