Validation of optional nested Data attribute is impossible

[denjaland]

Consider a CreateAddressData object defined as follows:

class CreateAddressData extends Data {
  public function __construct(
    public readonly string $street,
    public readonly string $nbr,
    public readonly string $postal_code,
    public readonly string $city,
    public readonly string $cc
  ) {
  }

  public static function rules() {
    return [
      'street' => 'required|min:1|max:80',
      'nbr' => 'required|max:20',  
      'postal_code' => 'required|min:4|max:20',
      'city' => 'required|min:1|max:50',
      'cc' => 'required|size:2',
    ];
  }
}

And we have a CreatePersonData with the following definition:

class CreatePersonData extends Data {
  public function __construct(
    public readonly string $first_name,
    public readonly string $last_name,
    public readonly string $email,
    public readonly ?CreateAddressData $address // Please note the address is OPTIONAL
  ) {
  }

  public static function rules() {
    return [
      'first_name' => 'required|min:1|max:50',
      'last_name' => 'required|min:1|max:50',
      'email' => 'required|email:rfc',
      'address' => '', // No validation rules for the address, as it's considered optional!
    ];
  }

The idea being that a person can optionally have an address, but when an address is given, the properties are correctly being validated...

The following code however throws a validation exception saying The address.street field is required. (and 4 more errors):

  CreatePersonData::validate(
    [
      'first_name' => $value,
      'last_name' => fake()->lastName(),
      'email' => fake()->email(),
    ]
  );

Shouldn't the nested data object only be validated when it is required in the containing data object or when it's optional and provided as part of the containing object?

I haven't checked the code yet (will do asap though), but Im only guessing you are merging the validation rules from the nested objects into the rules array for the containing one, but we should probably loop-validate instead, no?

[stoic-mortal]

Try adding sometimes rule to the address so it's validated when present.

https://laravel.com/docs/9.x/validation#validating-when-present

[denjaland]

That doesn't work, because it would make the street optional; as soon as an address is provided, we need all address fields to be present.

I just went through the code, and just removing the required rule might fix it as it is automatically being added. Will do some further testing.

[denjaland]

hmm - removing the requiredrule on the address fields, now makes the following test fail of course:

test('it validates street', function (string $value) {
    CreateAddressData::validate(
        [
            'street' => $value,
            'nbr' => fake()->numberBetween(1, 200),
            'postal_code' => fake()->postcode(),
            'city' => fake()->city(),
            'cc' => fake()->countryCode(),
        ]
    );
})->with(
    ['', Str::random(81)]
)->throws(ValidationException::class);

The test with <empty string> now fails, as it is no longer required, and then the min:1 rule is no longer being applied. It's a bit weird behaviour, because in this case, the required rule is not automatically added by the DataClassValidationRulesResolver class.

I think we need to have some logic to remove the required rule from nested Data Objects when the nested object is optional and has no value provided. I need to dig into it, but don't have time to do that this year still ;-)

[rubenvanassche]

I'm currently working on a complete rewrite of the validation logic of the package which should hopefully fix all the issues existing in the current one. This will probably be a major release which can be released soon.

[denjaland]

@rubenvanassche As you are working on the validation logic, I'm not going to dive into it in detail, or will not try and fix it, but here is another case where we have a failure that I wanted to share with you as you can probably take it along with your current refactoring.

Our User model implements the SoftDeletes trait, so in order to validate that no two users can have the same email address, we test it like this:

class CreateUserData extends Data {
  public function __construct(
    public readonly string $email
  )

  public function rules() {
    return [
      'email' => Rule::unique('users', 'email')->whereNotNull('deleted_at')
    ]
}

Yet the following test is failing:

it('rejects an already existing email address', function() {
  $user = User::factory()->create();

  CreateUserData::validate([
    'email' => $user->email
  ]);
})->throws(ValidationException::class);

[bentleyo]

@denjaland in that last example, should it be ->whereNull('deleted_at') instead? Otherwise you'll be restricting it to only deleted records.

For convenience Unique has a $withoutTrashed property you can use too. E.g. Rule::unique('users', 'email', withoutTrashed: true)

[denjaland]

@bentleyo good catch! Fixed it :-). (sometimes I need my rubber duck)

[denjaland]

@bentleyo,

Just checked, but I don't see that withoutTrashed option. Is that by any chance something you added in your own project? It's definitely handy, but I don't see it...

[bentleyo]

@denjaland whoops I accidentally referenced Rule::unique, but meant the Unique validation class: https://github.com/spatie/laravel-data/blob/main/src/Attributes/Validation/Unique.php#L21

If you're using Rule::unique it will be like Rule::unique('users', 'email')->withoutTrashed() see: https://github.com/laravel/framework/blob/9.x/src/Illuminate/Validation/Rules/DatabaseRule.php#L178

Hope that helps!

[rubenvanassche]

For those who are interested, this is currently the implementation for laravel-data v3 with the new validation: https://github.com/spatie/laravel-data/pull/329. This is highly wip, so expect that thing are going to break and change 😄

[denjaland]

Thanks a lot for the effort, @rubenvanassche. Currently don't really have time to dive into it, but I'll definitely give it a go once I do!

[VladyslavChernyshov]

@rubenvanassche Will it be possible in the future to modify error messages as it is possible in requests?

[francoism90]

@smartmeerkat You can already do this, check the Data documentation about messages.

[rubenvanassche]

Data 3 is released 🎉, let's close this one.