More query parameter validation (follow #1692)

[jdeniau]

Q	A
Bug fix?	kind of
New feature?	yes
BC breaks?	if filter config was wrongly configured
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1627
License	MIT
Doc PR	Doc PR will follow if PR is approved

This follows #1692

It implements all other validation filters from OpenApi specification

This might break wrongly configured api though. I see two possible solutions here:

• make this a [Might break] as we can consider wrongly configured sites a "bug"
• add a new configuration key validate_filters, default to false, and deprecate it being to false, encouraging people to set it to true, and remove the configuration key in 3.0

[jdeniau]

I'm kind of stuck with the error here : I rebased on master, but 4cc6152 now throw an error "debugMessage": "Cannot return null for non-nullable field __InputValue.type.", in features/graphql/introspection.feature, apparently because of tests/Fixtures/TestBundle/Filter/BoundsFilter.php::getDescription return !

Can somebody knowing GraphQL help me ?

[jdeniau]

OK I found the issue, it come from the filters containing 'type' => 'number' like:

          'exclusiveMinimum' => [                                                                                                                                            
                  'property' => 'exclusiveMinimum',                                                                                                                              
                  'type' => 'number',                                                                                                                                            
                  'required' => false,                                                                                                                                           
                  'swagger' => [                                                                                                                                                 
                      'minimum' => 5,                                                                                                                                            
                      'exclusiveMinimum' => true,                                                                                                                                
                  ],                                                                                                                                                             
              ],            

If I set "string" or "int", the test does work (but the new tests does not work of course).

What is the real definition behind the type ? I mapped this PR upon the open-api specification, but apparently the GraphQL code use this to get a PHP type ?

I can "hack" and allow "int" and "float" inside this PR but I don't know if it's a good way to make this regarding api-platform point of view.
Another way to do this is to "override" the type in the swagger key, but it would be really redundant and sensible to developer error

Thanks

[soyuka]

I've put myself a reminder to review this, let's focus on #1692 first.

[soyuka]

@jdeniau could you rebase this? Thanks!

[jdeniau]

@dunglas @meyerbaptiste @soyuka Everything should be good for me, except the problem with "number" type that I mentionned earlier.

[jdeniau]

I finally got time to rework on that, and made the same choice as defined in

core/src/Swagger/Serializer/DocumentationNormalizer.php

Lines 658 to 660 in 5b5e6b7

	if (Type::BUILTIN_TYPE_FLOAT === $type) {
	return ['type' => 'number'];
	}

, thus if type is documented as float, it will be converted to number for openapi.

This way, it's more "PHP-friendly".

[jdeniau]

@TracKer @flug rebase done.
The only failing test is also failing on master branch

[soyuka]

Nice one, we should merge this as a new feature! WDYT @api-platform/core-team

[soyuka]

Let's merge to avoid further conflicts, we can always come back to some parts of the code if there's smth to change. Thanks a lot @jdeniau for the work!

[jdeniau]

@soyuka Thanks a lot 🙏

If there is anything to change, I will try to be as quick as possible !

[norkunas]

Does this allows to configure a SearchFilter with some property as required?

[jdeniau]

@norkunas Yes you can.

See https://github.com/jdeniau/api-platform-core/blob/3ec427e85ccaeff23aca620335bdd0b7c5da3ded/tests/Fixtures/TestBundle/Filter/RequiredFilter.php to do how.

For the record this, the required is operational this since 2.3.0. This PR adds required allowEmptyValue features and all swagger specifications (array items, maximum, minimum, enum, minLength, maxLength, multipleOf, pattern)

[norkunas]

Looking at example I'd need to create a custom filter class which would apply the "required" as true, yes? Instead I'd expect something like:

/**
 * @ApiFilter(
 *     SearchFilter::class,
 *     properties={
 *         "offer.id": { "strategy": "exact", "required": true }
 *    }
 * )
 */
class Entity {}

[soyuka]

I guess we need docs now :D

[jdeniau]

Totally! I will work on it! Le ven. 20 déc. 2019 à 10:55, Antoine Bluchet <notifications@github.com> a écrit :

…

I guess we need docs now :D — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1723?email_source=notifications&email_token=AAKVNRPXQQBZUGUQ3QVBC4TQZSJB7A5CNFSM4ER3MVMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHMPCCY#issuecomment-567865611>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKVNRPPBOX2MEMEPQNS7GTQZSJB7ANCNFSM4ER3MVMA> .

[kovlcode]

How to use it? Is there an example?
I need SearchFilter::class with required=true. I would also like to dynamically set required = true (dependent on user role). How can i do this?

my temporaty bad solution:

/**
 * @ApiResource(
 *     // ... rules
 * )
 * @ApiFilter(SearchRequiredFilter::class, properties={"foo": "exact", "bar": "exact"})
 * @ORM\Entity(repositoryClass="App\Repository\ProjectChatRepository")
 */
class User
{ ... }

and SearchRequiredFilter class:

//src/Filter
<?php

declare(strict_types=1);

namespace App\Filter;

// ... many use ...

class SearchRequiredFilter extends SearchFilter
{
    /**
     * @var Security
     */
    private $security;

    public function __construct(
        Security $security,
        // ... parent params
    ) {
        parent::__construct(
           // parents property
        );

        $this->security = $security;
    }

    public function getDescription(string $resourceClass): array
    {
        $description = parent::getDescription($resourceClass);

        foreach ($description as &$options) {
            if (!$options['is_collection']
                && $this->security->isGranted('ROLE_USER')
            ) {
                $options['required'] = true;
            }
        }

        return $description;
    }
}

I understand this is a very bad solution, but how to do my task correctly?

P.S.
I also noticed problem $this->security->isGranted('ROLE_USER') not work and $this->security->getUser() === null. Is this because the filter is triggered before authorization?

[jdeniau]

First point : I will work on documentation soon. It is on my to-do list. Second point : your solution seems good to me, as it is a complicated case, you will need to have a complex appproach for that. The only point may be to not extend SearchFilter but create a real custom filter. Le mar. 28 janv. 2020 à 15:50, winzza <notifications@github.com> a écrit :

…

How to use it? Is there an example? I need SearchFilter::class with required=true. I would also like to dynamically set required = true (dependent on user role). How can i do this? my temporaty bad solution: /** * @ApiResource( * // ... rules * ) * @ApiFilter(SearchRequiredFilter::class, properties={"foo": "exact", "bar": "exact"}) * @Orm\Entity(repositoryClass="App\Repository\ProjectChatRepository") */ class User { ... } and SearchRequiredFilter class: //src/Filter <?php declare(strict_types=1); namespace App\Filter; // ... many use ... class SearchRequiredFilter extends SearchFilter { /** * @var Security */ private $security; public function __construct( Security $security, // ... parent params ) { parent::__construct( // parents property ); $this->security = $security; } public function getDescription(string $resourceClass): array { $description = parent::getDescription($resourceClass); foreach ($description as &$options) { if (!$options['is_collection'] && $this->security->isGranted('ROLE_USER') ) { $options['required'] = true; } } return $description; } } I understand this is a very bad solution, but how to do my task correctly? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1723?email_source=notifications&email_token=AAKVNRIE46NN4BELFJAGD43RABA3BA5CNFSM4ER3MVMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKDSGFA#issuecomment-579281684>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKVNRNKJQQSYJ3ZFGSCZPTRABA3BANCNFSM4ER3MVMA> .

[efpapado]

Would it be possible to run a custom validation callback on a filter that is not applied to any entity property?
So, like binding the validator class to the filter itself, and not to any entity property.

[jdeniau]

@efpapado for the moment, there are only "swagger" filter validators. (See https://github.com/api-platform/core/blob/master/src/Filter/QueryParameterValidator.php#L35-L43 for more informations)

If you want to run a custom callback in a custom filter file, I think you can easily do something like that:

class FooFilter implements FilterInterface
{
      public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null)
    {
        $this->throwIfSomethingIsWrong();

        // normal filter behaviour
    }
}

[efpapado]

Thank you so much!!! I wasn't aware of the apply method because it's not in the FilterInterface :)

[jdeniau]

@efpapado You're welcome !

For the record, it is not available in the base ApiPlatform\Core\Api\FilterInterface, but it is available in the doctrine bridge ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\FilterInterface.

It is mentionned in the documentation about custom filter, but in the documentation, the easier AbstractContextAwareFilter class is prefered for it's simplicity.

Theoretically, you can hook into both apply or filterProperty to throw an error whenever you want.