throw an exception if required filter is not set by jdeniau · Pull Request #1692 · api-platform/core

jdeniau · 2018-02-08T15:27:21Z

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

This PR is a start of query parameter validation.

It does only check required=true filter for now but I will open other PR to include the rest of OpenAPI specification on field validation.

There are some points that I am not really sure of:

namespace of the new QueryParameterValidateLister: I put it in Core\Validator\EventListener but maybe in Symfony\Bridge ?
I set the priority of the event listener to 16, this way, it is the first event in the queue (according to the documentation)

Otherwise, the code is quite simple and leverage Symfony validation system.

Thanks !

dunglas · 2018-02-08T15:32:44Z

I'm glad that you work on this! Thanks.

Do we really need a dependency to the Symfony validator for that? Can't we just use the filter extension? It will reusing API Platform in a non-Symfony context (PSR-7 and Laravel support is on our roadmap).

About the namespace, I would suggest to put it in the Filter namespace.

jdeniau · 2018-02-08T15:45:12Z

@dunglas I used Symfony validation for several reason:

api-platform automatically throw a 400 exception when a ConstraintViolationList is throwned
api-platform do already depends on a validator service (is it deprecated ?)
OpenApi declare several possible requirements that seems to work really great with Symfony constraints system

Or maybe I did not really understand your comment ?

dunglas · 2018-02-08T15:53:48Z

The integration with the validator is 100% optional (as with Doctrine etc), in fact it's only used in the bridge for convenience for the end user, but IIRC, not for any "core" feature like this one.

Regarding the 400, it's just one line to add to support a new exception: https://github.com/api-platform/core/blob/master/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php#L251

jdeniau · 2018-02-08T16:12:26Z

@dunglas OK for the ContraintViolationException, I can throw an api-platform specific exception, but don't you think it will be reinventing the wheel to re-implement every OpenApi validation ? (enum, pattern, min / max, empty value, type, etc.) ?

I think it will be a big copy/paste from symfony's constraints, don't you ?

About the FilterExtension, are you saying that I should drop the event listener and put the code in the FilterExtension ?

jdeniau · 2018-02-15T08:17:14Z

@dunglas I remove the dependency on Symfony validator service.

About using the FilterExtension, I did not, because FilterExtension is Doctrine-specific and will not work when using resources that are not Doctrine's.

I think it's good for merge. I will make other PR's implementing the rest of OpenApi parameters specification after that.

dunglas

Nice one :)

dunglas · 2018-02-15T08:44:04Z

+namespace ApiPlatform\Core\Exception;
+
+/**
+ * Deserialization exception.


Should be updated

dunglas · 2018-02-15T08:44:49Z

+    /**
+     * Gets constraint violations related to this exception.
+     *
+     * @return ConstraintViolationListInterface


You can use a return type instead.

dunglas · 2018-02-15T08:45:05Z

+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+
+/**
+ * Validate query parameters depending on filter description.


dunglas · 2018-02-15T08:46:13Z

+            }
+
+            foreach ($filter->getDescription($attributes['resource_class']) as $name => $data) {
+                if ($data['required'] ?? false) {


Can you invert this condition to reduce the complexity?

if (!($data['required'] ?? false)) { continue; }

I changed a litte this part to prepare the next step that will validate several parameter rules, but this should be more understandable though

dunglas · 2018-02-15T08:46:50Z

+                    $errorList = [];
+
+                    if (null === $request->query->get($name)) {
+                        $errorList[] = sprintf('Query parameter `%s` is required', $name);


Can you change the backticks with double quotes for consistency with other exceptions?

dunglas · 2018-02-15T08:47:15Z

+                        $errorList[] = sprintf('Query parameter `%s` is required', $name);
+                    }
+
+                    if (count($errorList) > 0) {


Can be simplified in if ($errorList) {

jdeniau · 2018-02-15T09:40:42Z

@dunglas done

meyerbaptiste · 2018-03-06T15:19:27Z

+            foreach ($filter->getDescription($attributes['resource_class']) as $name => $data) {
+                $errorList = [];
+
+                if (($data['required'] ?? false) && null === $request->query->get($name)) {


This condition will not work with serializer's filters (assuming they are required) for example. Indeed, property[] is not equal to property.

Sorry but I did not really understand what you mean here ?

Can you provide an example when this does not work ?

Let's take the description of the property filter:

core/src/Serializer/Filter/PropertyFilter.php

Lines 68 to 77 in 63d555a

public function getDescription(string $resourceClass): array

{

return [

"$this->parameterName[]" => [

'property' => null,

'type' => 'string',

'required' => false,

],

];

}

And let's say required is equal to true for our example.

So, the $name var will be equal to property[] here. But with ?property['relatedDummy'][]=name the name will be property in the parameter bag and $request->query->get('property[]') will return null...

meyerbaptiste · 2018-03-06T15:19:52Z

+ *
+ * @author Julien Deniau <julien.deniau@gmail.com>
+ */
+class QueryParameterValidateListener


Can you add a unit test please?

Is it usefull ? It's fully tested with behat tests.

Yes, Behat is only for integration and acceptance tests. You also have to unit test your class.

jdeniau · 2018-03-09T08:59:04Z

@meyerbaptiste Done for the unit tests and the "array notation" (but I'm not really happy with the implementation though :| )

meyerbaptiste · 2018-03-13T10:47:20Z

+                    continue;
+                }
+
+                if (false !== strpos($name, '[')) { // array notation of filter


I have another dirty solution:

parse_str($name, $query); $name = array_keys($query)[0] ?? '';

It works with:

''

'foo'

'foo[]'

'foo[bar]'

'foo[bar][]'

'foo[bar][baz]'

etc.

😄

@meyerbaptiste done

meyerbaptiste · 2018-03-23T14:31:24Z

+    private function isRequiredFilterValid($name, $request): bool
+    {
+        $matches = [];
+        if (false === mb_parse_str($name, $matches)) {


Huum, this requires the mbstring extension... And the mb_parse_str() function is not implemented by symfony/polyfill-mbstring!

Oh yes, I though it was included since PHP7, my bad, I remove this

jdeniau · 2018-03-27T07:27:41Z

@meyerbaptiste changed regex → parse_str

soyuka · 2018-05-24T12:37:40Z

+use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
+use Doctrine\ORM\QueryBuilder;
+
+class ArrayRequiredFilter extends AbstractFilter


could be final

soyuka · 2018-05-24T12:37:47Z

+use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
+use Doctrine\ORM\QueryBuilder;
+
+class RequiredFilter extends AbstractFilter


could be final

soyuka · 2018-05-24T12:38:09Z

+
+    ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\ArrayRequiredFilter:
+        arguments: [ '@doctrine' ]
+        tags: [ 'api_platform.filter' ]


we're not using spaces after [ IIRC in this case?

teohhanhui · 2018-05-24T12:46:57Z

+    private function isRequiredFilterValid($name, $request): bool
+    {
+        $matches = [];
+        parse_str($name, $matches);


I've mentioned this a long time ago, but we really should stop using bad built-in PHP functions like parse_str. We should seriously think about using a proper URI parsing library instead: https://uri.thephpleague.com/5.0/

https://uri.thephpleague.com/5.0/components/parsers/#queryparserextract

Is this really necessary for this use case? Maybe can we do that at some point in another PR and replace all calls to parse_str in 1 time.

teohhanhui · 2018-05-24T13:01:28Z

@dunglas I think it's better if we avoid giving the filter implementations the responsibility of checking required filters. It's not their job and it doesn't need to be. But I agree with extracting all logic into a separate class, so as to keep the event listeners really thin / "dumb".

dunglas · 2018-05-24T13:04:03Z

Introducing a new service and using it in the listener looks good to me.

jdeniau · 2018-05-24T13:19:34Z

@soyuka about the "light" listener, the separation between the listener and the validation has been made in #1723 ( https://github.com/api-platform/core/pull/1723/files#diff-91ffc72a7db30ec816871e7141558def exactly ) : I prefer keeping code simple first, and re-factor it when needed.
With this modification, I can extract a FilterListValidator, but the only code it will contain is a foreach($validatorList as $validator) { doValidate(); }

@dunglas About the namespace, we talked about it previously, but I can move the listener in EventListener and move back the Filter validator in Filter namespace.

Can we keep the listener "as-is" for this PR, and make the refactoring in #1723, as the purpose of this PR is like a POC before #1723 ? (it's quite painful to rebase each time, and I think it will be more painful this time as the code is several months old).

dunglas · 2018-05-24T14:43:12Z

LGTM.

jdeniau · 2018-05-26T09:36:23Z

@soyuka done

jdeniau · 2018-06-04T07:47:46Z

@dunglas @meyerbaptiste @Simperfit @soyuka everything is OK on my part I think.

Waiting for a merge on master to work on the second PR that will check all documented fields

soyuka · 2018-06-04T08:09:03Z

Thanks @jdeniau !

…ation More query parameter validation (follow #1692)

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 653213d to e982f80 Compare February 8, 2018 15:27

jdeniau force-pushed the jd-feat-queryParameterValidation branch from e982f80 to af2baa6 Compare February 8, 2018 15:37

jdeniau force-pushed the jd-feat-queryParameterValidation branch from af2baa6 to 1930621 Compare February 8, 2018 15:51

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 1930621 to d43e385 Compare February 14, 2018 15:50

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 72845d9 to e2212dc Compare February 15, 2018 08:21

dunglas requested changes Feb 15, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-queryParameterValidation branch from e2212dc to 47877d6 Compare February 15, 2018 09:39

jdeniau force-pushed the jd-feat-queryParameterValidation branch 2 times, most recently from 3d05d37 to c76a40f Compare February 21, 2018 15:29

jdeniau mentioned this pull request Feb 22, 2018

More query parameter validation (follow #1692) #1723

Merged

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 7add831 to 40b6fe2 Compare February 22, 2018 07:29

meyerbaptiste requested changes Mar 6, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 40b6fe2 to 8a39942 Compare March 8, 2018 23:02

jdeniau force-pushed the jd-feat-queryParameterValidation branch 2 times, most recently from bb98949 to 0ddfa2b Compare March 12, 2018 13:57

meyerbaptiste reviewed Mar 13, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-queryParameterValidation branch 2 times, most recently from 6c5432c to e8375de Compare March 23, 2018 13:21

meyerbaptiste reviewed Mar 23, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-queryParameterValidation branch 2 times, most recently from 1388e44 to a04ee65 Compare March 23, 2018 15:37

jdeniau force-pushed the jd-feat-queryParameterValidation branch from a04ee65 to ffb46ea Compare March 28, 2018 14:55

soyuka reviewed May 24, 2018

View reviewed changes

teohhanhui reviewed May 24, 2018

View reviewed changes

dunglas approved these changes May 24, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-queryParameterValidation branch 3 times, most recently from c5cc04b to 462751b Compare May 26, 2018 09:35

soyuka approved these changes May 28, 2018

View reviewed changes

jdeniau added 8 commits June 4, 2018 09:45

throw an exception if required filter is not set

31e937a

remove dependency on validator

2367a47

fix issue with array notation filter name

da476c1

add unit test on QueryParameterValidateListener

261e522

use parse_str instead of preg_match

d624665

is_array -> \is_array

0186719

error list throw all erreors (not only the first one)

fe516ff

make classes final + consistency in service name

1e353af

jdeniau force-pushed the jd-feat-queryParameterValidation branch from 462751b to 1e353af Compare June 4, 2018 07:45

soyuka merged commit d2250c1 into api-platform:master Jun 4, 2018

jdeniau deleted the jd-feat-queryParameterValidation branch June 4, 2018 09:24

ismail1432 mentioned this pull request Aug 27, 2019

[Question] Required Filter and limit filter by user #1163

Closed

soyuka added a commit that referenced this pull request Dec 17, 2019

Merge pull request #1723 from jdeniau/jd-feat-moreQueryParameterValid…

2e491f7

…ation More query parameter validation (follow #1692)

jdeniau mentioned this pull request Apr 14, 2021

Filter\Validator\Require fails on nested property #4214

Closed

	public function getDescription(string $resourceClass): array
	{
	return [
	"$this->parameterName[]" => [
	'property' => null,
	'type' => 'string',
	'required' => false,
	],
	];
	}

Uh oh!

Uh oh!

Conversation

jdeniau commented Feb 8, 2018

Uh oh!

dunglas commented Feb 8, 2018 • edited by Simperfit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdeniau commented Feb 8, 2018

Uh oh!

dunglas commented Feb 8, 2018

Uh oh!

jdeniau commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdeniau commented Feb 15, 2018

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented Mar 9, 2018

Uh oh!

meyerbaptiste Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dunglas commented Feb 8, 2018 •

edited by Simperfit

Loading

jdeniau commented Feb 8, 2018 •

edited

Loading

meyerbaptiste Mar 13, 2018 •

edited

Loading

meyerbaptiste Mar 23, 2018 •

edited

Loading

soyuka May 24, 2018 •

edited

Loading

teohhanhui May 24, 2018 •

edited

Loading

teohhanhui commented May 24, 2018 •

edited

Loading

jdeniau commented Jun 4, 2018 •

edited

Loading