test: add nested property test for uuidfilter

[aaa2000]

Q	A
Branch?	main
License	MIT

See #7628 (comment)

[soyuka]

looks nice thanks!

[VincentLanglet]

You give the property here while you didn't have the need for

'myDevice' => new QueryParameter(
      filter: new UuidFilter(),
),

for instance ; is it needed ?

We could have a test with something like

'myDevice.externalId' => new QueryParameter(
      filter: new UuidFilter(),
),
'myDeviceExternalIdAlias' => new QueryParameter(
     filter: new UuidFilter(),
     property: 'myDevice.externalId',
)

no ?

Same for nameConverted like

'nameConverted.nameConverted' => new QueryParameter(
     filter: new UuidBinaryFilter(),
),
'nameConvertedAlias' => new QueryParameter(
     filter: new UuidBinaryFilter(),
     property: 'nameConverted.nameConverted',
),

[aaa2000]

Without the property, I have the error

{"@context":"\/contexts\/Error","@id":"\/errors\/500","@type":"hydra:Error","detail":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter::filterProperty(): Argument #1 ($property) must be of type string, null given, called in xxxxsrc\/Doctrine\/Orm\/Filter\/AbstractUuidFilter.php on line 62","status":500,"type":"\/errors\/500","trace":[{"file":"xxxxsrc\/Doctrine\/Orm\/Filter\/AbstractUuidFilter.php","line":62,"function":"filterProperty","class":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/Extension\/ParameterExtension.php","line":72,"function":"apply","class":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/Extension\/ParameterExtension.php","line":83,"function":"applyFilter","class":"ApiPlatform\\Doctrine\\Orm\\Extension\\ParameterExtension","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/State\/CollectionProvider.php","line":73,"function":"applyToCollection","class":"ApiPlatform\\Doctrine\\Orm\\Extension\\ParameterExtension","type":"-\u003E"},{"file":"xxxxsrc\/State\/CallableProvider.php","line":43,"function":"provide","class":"ApiPlatform\\Doctrine\\Orm\\State\\CollectionProvider","type":"-\u003E"},{"file":"xxxxsrc\/State\/Provider\/ObjectMapperProvider.php","line":41,"function":"provide","class":"ApiPlatform\\State\\CallableProvider","type":"-\u003E"},{"file":"xxxxsrc\/State\/Provider\/ReadProvider.php","line":84,"function":"provide","class":"ApiPlatform\\State\\Provider\\ObjectMapperProvider","type":"-\u003E"},{"file":"xxxxsrc\/Symfony\/Security\/State\/AccessCheckerProvider.php","line":68,"

I put it in the draft because of that, I'd like to investigate why?

Yes, I can add a test for

'myDeviceExternalIdAlias' => new QueryParameter(
     filter: new UuidFilter(),
     property: 'myDevice.externalId',
)

and

'nameConverted.nameConverted' => new QueryParameter(
     filter: new UuidBinaryFilter(),
),
nameConvertedAlias' => new QueryParameter(
     filter: new UuidBinaryFilter(),
     property: 'nameConverted.nameConverted',
),

But, I think that with nameConverted.nameConverted, the query parameter will be nameConverted.nameConverted and not name_converted.name_converted....

[VincentLanglet]

I put it in the draft because of that, I'd like to investigate why?

This is is maybe the same issue than the one I found in my draft or a related one #7667 ?

You could try removing these lines
https://github.com/api-platform/core/pull/7667/files#diff-5e7788e120b00f67edb2447978c868aac8c117f505e150dbf859f603581a9c55L262-L264
to see if it works better for you without the property (?)

But sure your investigation will be interesting ^^

[VincentLanglet]

My bad, after a night of sleep I remember where does come from your issue.

It's because of

core/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php

Lines 258 to 260 in c8493bb

	if (null === $parameter->getProperty() && isset($properties[$key])) {
	$parameter = $parameter->withProperty($key);
	}

Properties are just the initial list of properties [myDevice, nameConverted, ...] and since the key is not in the array, it's not added as a property.
So I think this is ok for now.

[soyuka]

its not supported to magically find out the nested property and I don't get how it would work. I'm just fine with having to specify it for now. When (and if) we support nested properties everywhere, we might add some magic but I'm quite unsure of the benefits. A parameter's key is different then its property (and this is also why the current filtering system is hard to customize).

[aaa2000]

Ok thanks, new tests were added; to avoid duplication, I refactored the tests using a data provider.

Shouldn't we throw an exception if the property is null ?

--- a/src/Doctrine/Orm/Filter/AbstractUuidFilter.php
+++ b/src/Doctrine/Orm/Filter/AbstractUuidFilter.php
@@ -59,6 +59,10 @@ class AbstractUuidFilter implements FilterInterface, ManagerRegistryAwareInterfa
             return;
         }

+        if (null === $parameter->getProperty()) {
+            throw new InvalidArgumentException(\sprintf('The filter parameter with key "%s" must specify a property. Nested properties are not automatically resolved. Please provide the property explicitly.', $parameter->getKey()));
+        }
+
         $this->filterProperty($parameter->getProperty(), $parameter->getValue(), $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);
     }

[VincentLanglet]

I think it's a great idea. And the issue exists for some other filters like

core/src/Doctrine/Orm/Filter/ExactFilter.php

Line 35 in c8493bb

$property = $parameter->getProperty();

If @soyuka agree with the suggestion, a PR could be done to add the Exception to all the filter which requires the property to be either set or automatically resolved to help the developer.

[soyuka]

Shouldn't we throw an exception if the property is null ?

Good point but I suggest we fix this in the ParameterExtension (I think its the case) + we should add a typehint or documentation, for now I didn't wanted to change the filter interfaces but ideally (at least to me), filtering without a property makes no sense, I wish the interface for developers would state this clearly.

tldr: no need yet on this pr

[VincentLanglet]

I suggest we fix this in the ParameterExtension (I think its the case) + we should add a typehint or documentation, for now I didn't wanted to change the filter interfaces but ideally (at least to me), filtering without a property makes no sense, I wish the interface for developers would state this clearly.

This would be a breaking change for at least some of my filter where the property is hard coded in the filter (so I never call getProperty for instance and therefor I don't have to pass the property.

For the generic filters it doesn't make sens, but for custom specific filter it can make sens.

That's why I think it's better to let the Filter throw this exception.

[VincentLanglet]

Can we add a third filter

            'nameConverted.nameConverted' => new QueryParameter(
                filter: new UuidBinaryFilter(),
                property: 'nameConverted.nameConverted',
            ),

?

Cause the key here is just a "name" for the filter no ? It shouldn't have any impact on the query and therefor a camelCase key should be allowed too no ?)

(In the same idea than #7667)

[soyuka]

I'd have just a test for a nested property (ie the myDeviceExternalIdAlias one) the rest has no real point as no converting is done anywhere (nor the parameter's key change nor the properties). This is was I was a bit relunctant at having this functionality in the UuidFilter (as its a new one) as for now new filter's didn't really had this possibility designed.