laminas / laminas-inputfilter

Normalize and validate input sets from the web, APIs, the CLI, and more, including files
https://docs.laminas.dev/laminas-inputfilter/
BSD 3-Clause "New" or "Revised" License
42 stars 28 forks source link

[RFC]: Allow empty array validation group #71

Closed darckking closed 2 years ago

darckking commented 2 years ago

RFC

In my project I use laminas-inputfilter together with laminas-api-tools/api-tools-rest, laminas-api-tools/api-tools-content-validation. I implemented patchList(), to be able to patch multiple items at once. The challenge here is how to match change sets with items we need to patch. An obvious idea would be to add an 'id' input to change set, but

  1. This is wrong as we are not going to change an 'id' so it shouldn't be in change set.
  2. This solution is not generic - what if entity has property with different name as PK or entities with composite PKs.
  3. This solution is not reliable because laminas-api-tools/api-tools-content-validation ContentValidationListener::validatePatch() sets only posted properties as validation group. So even if we add an 'id' input and mark it 'required' - it won't be validated hence items without id will be allowed.

Then I looked at JSON Patch (RFC 6902), where the operation objects have "path" property which is subject for an operation. In case of array/collections path includes an index/position of an element. Example:

[
    { "op": "add", "path": "/items/2/name", "value": "New name" }
]

To integrate JSON Patch is big piece of work as it will require different validation and hydration layers.

So my idea is to use the same approach - match change set with entity by its position in the collection. So the example above can be changed to

[
    {},
    {},
    {
        "name": "New name"
    }
]

However, the empty change set is causing a problems for me.

Laminas\InputFilter\BaseInputFilter

  1. setValidationGroup() - won't set an empty array as a validation group
        if (! empty($inputs)) {
            $this->validateValidationGroup($inputs);
            $this->validationGroup = $inputs;
        }
  2. getValues() - will try to get values from all inputs when validation group is empty
    $inputs = $this->validationGroup ?: array_keys($this->inputs);
  3. isValid() - will validate all inputs when validation group is empty
    $inputs = $this->validationGroup ?: array_keys($this->inputs);

My main question - why empty array validation group is not allowed ? Or maybe you would suggest some other approach that would be compatible with Laminas components.

P.S. I extended the Laminas' InputFilter and overridden functions with Null Coalescing Operator (??) instead of Ternary Operator (?:) to solve second and third problems, to solve first problem, I removed the if statement. So far I don't see any problems.

Ocramius commented 2 years ago

won't set an empty array as a validation group

Changing this would lead to BC issues, no?

why empty array validation group is not allowed ?

I think only looking at tests related to this will give you a response to that: check which tests fail, then git blame on them to have an idea of where they came from.

darckking commented 2 years ago

The unit tests are passing, so it's not a BC (unless tests can't detect it).

However it is a behavior change. Let's look at case when you send an empty change set (no matter it is a PATCH of singular entity or list of entities), only in this case ContentValidationListener::validatePatch() will try to set an empty array validation group.

Scenario 1 - InputFilter has required inputs without fallback values Now: validation will fail as one of inputs is required Proposal: validation will be successful as there is nothing to validate and no values will be returned from InputFilter::getValues()

Scenario 2 - InputFilter has required inputs with fallback values Now: validation will be successful but InputFilter::getValues() will return defined fallback values for required inputs, hence you overwrite existing values Proposal: validation will be successful as there is nothing to validate and no values will be returned from InputFilter::getValues()

Scenario 3 - InputFilter doesn't have required inputs Now: validation will be successful but InputFilter::getValues() will return null for each input, hence you overwrite existing values Proposal: validation will be successful as there is nothing to validate and no values will be returned from InputFilter::getValues()

So it boils down to the question. Which behavior is correct ?

froschdesign commented 2 years ago

I don't know if this works in your use case, but have you checked the optional input filter?

https://docs.laminas.dev/laminas-inputfilter/optional-input-filters/

darckking commented 2 years ago

I thought about OptionalInputFilter. It kind of works in my case. As it won't try to validate inputs or return values when an empty data is set. The part I don't like here is that I'll have to create 2 InputFilters: 1) for POST / PUT which is a usual InputFilter (obviously I don't want to allow an empty data for these methods) 2) for PATCH which is OptionalInputFilter.

froschdesign commented 2 years ago

The problem is that a validation group is defined as a subset of the data to be validated so an empty makes no sense here. This means that all inputs should be validated if no validation group is set. Anything else complicates the usage of the method and also its explanation.

The method already allows two different data types – which is unbeautiful –, so an empty array with its own behaviour goes in the wrong direction.

darckking commented 2 years ago

This means that all inputs should be validated if no validation group is set.

Yes, this is correct. The BaseInputFilter::$validationGroup is null after object instantiation, so yes - all inputs should be validated. However I'm trying to set an empty subset, so when it's an empty array - it is set and should behave like usually.

Semantically the empty validation group is like "no-op" - nothing to validate and nothing to return from getValues(). That's my only argument in this discussion.

froschdesign commented 2 years ago

However I'm trying to set an empty subset, so when it's an empty array - it is set and should behave like usually.

Semantically the empty validation group is like "no-op" - nothing to validate and nothing to return from getValues(). That's my only argument in this discussion.

It's clear what you want, but an empty array is not a subset or subgroup therefore it does not make sense to use this as an optional validation.