[PropertyFilter] Fix whitelist comparison

[antograssiot]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	to come

I'm using the GroupFilter with whitelist since it got merged but never really tried/used the ProprertyFilter whitelist in an application and just relied on the tests I wrote initially. When trying to use it in a real application last week I noticed that the behavior was not the expected one.

An example to reproduce the issue is to whitelist a foo property. Then when requesting /resource?properties[]=bar, foowill be present in the serialized entity.

Another thing I've been trying to fix is the filter declaration and it might be considered as a BC break but given the actual implementation is unusable, I'm pretty sure that we would know if someone would have tried/implemented it already.
The thing is that the array containing properties has numeric and non-numeric keys like:

[
    'foo',
    'bar',
    'group' => ['baz, 'fuz'],
]

I'm no expert but I didn't found a way to generate an array of this kind in yml. The old given example was generating the following:

[
    'foo',
    'bar',
    ['group' => ['baz, 'fuz']],
]

So in order to ease the array intersection, I went to a whitelist definition of the following form

['foo', 'bar', 'group.baz', 'group.fuz']

Even if this is sort of a a BC break, the actual implementation has no chance to work, and using this format will be more inline with the other filter.

[antograssiot]

This function and partitionAssocAndNumericArrayKeys() may not really fit into this class, if you want me to extract them into some generic utils class, let me know...

[meyerbaptiste]

Huum, what about something easier (should be tested):

private function getProperties(array $properties, array $whitelist = null)
{
    $whitelist = $whitelist ?? $this->whitelist;
    $result = [];

    foreach ($properties as $key => $value) {
        if (is_numeric($key)) {
            if (in_array($value, $whitelist, true)) {
                $result[] = $value;
            }

            continue;
        }

        if (isset($whitelist[$key]) && is_array($value) && $recursiveResult = $this->getProperties($value, $whitelist[$key])) {
            $result[$key] = $recursiveResult;
        }
    }

    return $result;
}

[dunglas]

Class are not always used with YAML config, IMO it makes sense to support both the old and the new behavior.

[dunglas]

Instead of introducing a dotted notation and a lot of code to maintain, I propose to support those syntaxes:

# Use values (no nested properties)

arguments:
  - whitelisted_properties
  - false
  - [ foo, bar ]

# Use keys (support nested properties)

arguments:
  - whitelisted_properties
  - false
  - { foo: ~, bar: [ a, b ] }

This way, it will be in sync with how Symfony handles such cases. WDYT @antograssiot? Can you add the following examples to the test suite and remove the support for the dotted notation?

[meyerbaptiste]

I'm no expert but I didn't found a way to generate an array of this kind in yml.

With Yaml::dump():

foo:
    0: foo
    1: bar
    group: [baz, fuz]

[antograssiot]

Thanks @meyerbaptiste, even if I feel dumb now 😛

Ok @dunglas I'll remove the dot notation.
Just so I'm sure I don't miss a point here, if I'm not wrong, I will still need to transform { foo: ~, bar: [ a, b ] } to a php array like

[
'foo',
'bar' => ['a', 'b']
]

because that's this this how the parameters are retrieved from an url like /resources?properties[]=foo&properties['bar][]=a&properties['foo'][]=b

[dunglas]

@antograssiot maybe can you juste copy this snippet of mine: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php#L247-L254. It looks it deals with the exact same problem.

[antograssiot]

@dunglas I'll be on connected on slack in the next days, would you jump by and ping me maybe so we can figure this out ?

[meyerbaptiste]

This should be done in your ApiPlatform\Core\Bridge\Symfony\Bundle\DependencyInjection\Configuration class with the NodeDefinition::beforeNormalization() method.

[antograssiot]

Sorry @meyerbaptiste I don't get this one. From my understanding this is only when using configuration key no ? I mean here it is just services that can be defined multiple time without using any config parameter..

[meyerbaptiste]

My bad! I spoke too fast and in my head whitelist was a config key... 😛

[meyerbaptiste]

Huum, what about something easier (should be tested):

private function getProperties(array $properties, array $whitelist = null)
{
    $whitelist = $whitelist ?? $this->whitelist;
    $result = [];

    foreach ($properties as $key => $value) {
        if (is_numeric($key)) {
            if (in_array($value, $whitelist, true)) {
                $result[] = $value;
            }

            continue;
        }

        if (isset($whitelist[$key]) && is_array($value) && $recursiveResult = $this->getProperties($value, $whitelist[$key])) {
            $result[$key] = $recursiveResult;
        }
    }

    return $result;
}

[antograssiot]

Looks cleaner/simpler indeed.
I'll give it a try soon

[antograssiot]

@dunglas is it good to go now or you still want me to refractor some parts ?

[dunglas]

Looks good to me

[dunglas]

Thanks @antograssiot