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	https://github.com/overblog/GraphQLBundle/issues/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.

https://github.com/overblog/GraphQLBundle/pull/1185

[ITernovtsii]

Hi @Vincz @joesaunderson

I discovered that this modification causes 2 validation issues in certain scenarios: 1) Invalid data to bypass validation rules 2) Internal server Error for valid data

Prerequisites: schema:

CreatePageElementInput:
    type: input-object
    config:
        fields:
            translations:
                type: '[PageElementTranslationInput!]'
                validation: cascade

PageElementTranslationInput:
    type: input-object
    config:
        fields:
            placeholder:
                type: String!
            tooltip:
                type: String
                validation:
                    -   Symfony\Component\Validator\Constraints\Length:
                            max: 5
            language:
                type: String!

valid input data related to schema (no issues):

{
  translations: [
    {
      placeholder: "enter",
      language: "english",
      tooltip: "text"
    },
    {
      placeholder: "введіть",
      language: "ukrainian",
      tooltip: "текст"
    }
  ]
}

Scenario 1) Invalid data to bypass validation rules

{
  translations: [
    {
      placeholder: "enter",
      language: "english",
      tooltip: "invalid text with length > 5"
    },
    {
      placeholder: "введіть",
      language: "ukrainian"
    }
  ]
}

Reason: While processing second object (without "tooltip"), $metadata will not contain validation for "tooltip" field. Taking into account addMetadata implementation $this->metadataPool[$metadata->getClassName()] = $metadata;, this call $this->metadataFactory->addMetadata($metadata); will replace validation rules.

Scenario 2) Internal server Error for valid data

{
  translations: [
    {
      placeholder: "enter",
      language: "english"
    },
    {
      placeholder: "введіть",
      language: "ukrainian",
      tooltip: "норм"
    }
  ]
}

Reason: While processing first object (with "tooltip"), $metadata will contain validation for "tooltip" field. And, it will try to validate first object. Taking into account that this code will not be reached due to continue introduced in this PR.

} else {
    $rootObject->$property = $inputData[$property] ?? null;
}

It gives us error: Property Overblog\GraphQLBundle\Validator\ValidationNode::$tooltip does not exist

[joesaunderson]

@ITernovtsii see https://github.com/overblog/GraphQLBundle/issues/1185

I allowed the behaviour to be configurable, but I haven't had time to write tests / take it forward.

Feel free to take it on if this is pressing

[ITernovtsii]

@joesaunderson I think this change should be removed from the released version first, as "partial" mode will not work properly anyway, There is no reason to build the switch between 2 modes while only one mode works properly. I reviewed the core a bit and don't see any simple solution for how this can be implemented without the validation break. It looks like we may need metadata for each object, which will slow down request processing, and require significant changes in the code.

[joesaunderson]

@joesaunderson I think this change should be removed from the released version first, as "partial" mode will not work properly anyway,

There is no reason to build the switch between 2 modes while only one mode works properly.

I reviewed the core a bit and don't see any simple solution for how this can be implemented without the validation break.

It looks like we may need metadata for each object, which will slow down request processing, and require significant changes in the code.

Partial mode does work, and is working for our use case in a production app for millions of users 😀

[joesaunderson]

I think both your scenarios are when an input object is passed as an array, so maybe we need to adjust the check for that (does the field exist on any object in the array).

[ITernovtsii]

Sorry, but I disagree that it makes sense to keep it because it works in some cases. Especially taking into account that bypass validation can be considered as a security issue. Potentially, it can be easily solved on application level by ignoring null fields.

Issue is reproducible not only with arrays, but with any schema where same input type used twice in the request.

For example, if you have address input type, and passing it as person's deliveryAddress plus as homeAddress. User who knows about this issue will be able to bypass validation rules, and set invalid county code to delivery address. And those two types may exist in different depth levels of input data structure. So, it will require to iterate through the whole data tree to identify if there's object of the same type.

And I don't think it will be as simple as adjust this check, as it breaks consistency. Same object will be validated differently based on another object in the request.

[joesaunderson]

There's was also another suggestion from @Vincz so allow the validation mode to be set at the config level (PR above) or at a mutation level.

Maybe you could extend my PR to add that too, allowing it to be set at a mutation by mutation basis?