Don't validate fields that were not passed

[joesaunderson]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes/no
Documented?	no
Fixed tickets	#1182
License	MIT

This fixes the validation rules so that when passing a partial input object, only the fields passed are validated.

[Vincz]

Hi @joesaunderson!
Do you mind to add a bunch of tests please?

[joesaunderson]

Hi @joesaunderson! Do you mind to add a bunch of tests please?

I can try, there aren't many existing tests though, so not sure where I'll be starting from.

[joesaunderson]

Hi @joesaunderson! Do you mind to add a bunch of tests please?

I can try, there aren't many existing tests though, so not sure where I'll be starting from.

Yep @Vincz this is a bit beyond my knowledge of this bundle, there's nothing to extend and I'm getting into the internals of the system.

[grossmannmartin]

Hi @Vincz, @joesaunderson

I don't understand the need for this change.
Currently we're experiencing a changed and unexpected behavior in our application.

I our use-case we're creating an order and filling customer data based on the input.
Customer may choose a different delivery address.

differentDeliveryAddress:
    type: "Boolean!"
deliveryFirstName:
    type: "String"
    validation:
        -   NotBlank:
                message: "Please enter first name of contact person"
                groups: "differentDeliveryAddressWithoutPreselected"

The validation group is then computed based on the value of the differentDeliveryAddress field - so when different delivery address is true (customer wishes to delivery goods to different address), the delivery fields become mandatory (regardless the input data).

But after this change, the deliveryFirstName field is never validated if not passed in the mutation. This leads to the different behavior to data if passed and not, which is error-prone approach.

Can you explain the use-case benefiting from this change? And do you have an idea how to solve the case I described?

[joesaunderson]

The use case benefitting this change. Imagine a mutation like so:

edit_entity($entity)

Where entity is all the editable fields on that entity.

If you only want to edit some properties of the entity, you can pass only the properties you want to edit.

Previously, if you did this, it ran the validation rules for every properly of that entity (or input object in this case) regardless of whether they were passed on not.

[grossmannmartin]

But that way you have to count on the data you're passing down the application. So it's really easy to make a mistake and set data that should not pass the validation.

[joesaunderson]

Not sure I fully understand your use case so forgive me...

But isn't it correct that when not passing the deliveryFirstName, it shouldn't be validated?

It should only be validated when it's passed (which is when differentDeliveryAddress is true?).

[joesaunderson]

But that way you have to count on the data you're passing down the application. So it's really easy to make a mistake and set data that should not pass the validation.

Where does the counting element come in to it? This way the data you pass in is validated, the data you don't pass in isn't validated?

[grossmannmartin]

If I set the differentDeliveryAddress = true, the I want to validate the deliveryFirstName regardless if passed in mutation.
Because differentDeliveryAddress means deliveryFirstName field has to be set.

Now I have to be sure I pass all field even though the schema do not require them, otherwise invalid data may be set

[joesaunderson]

If I set the differentDeliveryAddress = true, the I want to validate the deliveryFirstName regardless if passed in mutation.

Because differentDeliveryAddress means deliveryFirstName field has to be set.

Now I have to be sure I pass all field even though the schema do not require them, otherwise invalid data may be set

I understand it better now. Devils advocate... if you're essentially requiring that field when differentDeliveryAddress = true, shouldn't your application ensure it's always passed too?

@Vincz suggested this behaviour could be a config setting, so maybe that could be an option?

validationMode: full | partial

[Vincz]

Hi @grossmannmartin, @joesaunderson
Yes, in the end both of yours uses cases make sense. So, we should definitely add an option.
What do you think, should be the default behavior?
I think we should be able to set the default behavior in the bundle config as suggested but we should also be able to override this in the input config itself.
Does any of you want to take care of this PR?

[joesaunderson]

I should have some time to look at it Tuesday. Can you point me in the direction of where config is used in other places?

[grossmannmartin]

Hi, thanks for understanding and the dialog :)

From my point of view, the original behavior is better as a default, because otherwise the rest of the application has to know what was passed as arguments and it's a behavior that already was.
But I am completely ok with the opposite as default as long as it's configurable :)

If necessary, I can make some time at the end of the week... sooner is not possible for me, sorry

[joesaunderson]

@Vincz @grossmannmartin I had a brief go at this today. It adds a new config option validation_mode to the default config.

I tried to add it to the field level too (under validation.mode) but got lost in the internals of this repository that I don't understand. Feel free to extend it, or merge it as is (as part 1).

I believe it puts us in a good place for full / partial validation.

#1185