Fix nameConverter usage in Metadata

[VincentLanglet]

See failure https://github.com/api-platform/core/actions/runs/20894952430/job/60032003544?pr=7667

[VincentLanglet]

Related issue for the property path
symfony/symfony#63019

I wonder if ApiPlatform shouldnt decorate the nameconverter into a PropertyPathNameConverter and use it instead.

[VincentLanglet]

I feel like nameConverterInterface shouldn't be applied to filter since it's a Serializer tool.

I would expect

'nameConverted' => new QueryParameter(filter: new ExactFilter()),

to work ; currently, only does

'name_converted' => new QueryParameter(filter: new ExactFilter()),

And same, I would expect

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

to word and I have to write

'nameConvertedAlias' => new QueryParameter(filter: new ExactFilter(), property: 'name_converted'),

This seems to be because of

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

Line 154 in c8493bb

$converted = $this->nameConverter?->denormalize($property) ?? $property;

[VincentLanglet]

This seems to be normalized here

core/src/Doctrine/Common/Filter/OrderFilterTrait.php

Lines 51 to 53 in c8493bb

	$propertyName = $this->normalizePropertyName($property);
	$description[\sprintf('%s[%s]', $this->orderParameterName, $propertyName)] = [
	'property' => $propertyName,

Or here

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

Line 263 in c8493bb

$parameter = $parameter->withProperty($this->nameConverter->normalize($property));

[soyuka]

https://github.com/api-platform/core/blob/main/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php#L123-L127

I think we should just have another name converter that has nothing to do with the one for serialization, can this be fixed only by changing dependency injection on there? We should do the least transformations in the filters.

[VincentLanglet]

I think we should just have another name converter that has nothing to do with the one for serialization, can this be fixed only by changing dependency injection on there? We should do the least transformations in the filters.

Sure, I agree two different name converter should be use. I'm not even sure why one is used at the first place... Unless this is really useful in some case (which one ?), I would

• Split the conf about name converter into serializerNameConverter and filterNameConverter
• Change the default filterNameConverter to null in next major and maybe even deprecate using one (?)

Still, if the syntax 'nameConverted' => new QueryParameter(filter: new OrderFilter()), the NameConverter is currently not injected while it is with the syntax #ApiFilter[OrderFilter::class].

[VincentLanglet]

It's tricky because some tests seems to rely on this behavior:

• Laravel test which are supposed to support sort[isActive] while the property is is_active
• Behat test which are supposed to support order[name_converted] filter when the property is nameConverted.

If the property is normalized in ParameterResourceMetadataCollectionFactory
https://github.com/api-platform/core/pull/7667/files#diff-5e7788e120b00f67edb2447978c868aac8c117f505e150dbf859f603581a9c55L263, it has to be denormalized somewhere else !

I feel like while the Abstract legacy filters are correctly handling this case by denormalizing the property

• core/src/Doctrine/Orm/Filter/AbstractFilter.php

  Line 50 in c8493bb

  $this->filterProperty($this->denormalizePropertyName($property), $value, $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);

All the new filters developed by @vinceAmstoutz are missing this transformation

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

  Line 35 in c8493bb

  $property = $parameter->getProperty();

Should the normalization be conditional ?

[soyuka]

I don't understand, if the property is is_active then it should be is_active inside the metadata (Parameter::property). We should do no converting after metadata is computed.

$this->filterProperty($this->denormalizePropertyName($property), $value, $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);

This makes this harder as then the responsability of converting is everywhere, we now have a way to cache these transformations lets use it.

Laravel test which are supposed to support sort[isActive] while the property is is_active
Behat test which are supposed to support order[name_converted] filter when the property is nameConverted.

yes and there's an important difference between a parameter key (and a property), if you use the placeholder :property there'll be some magic happening and if the name converting is set to snakeCase to camelCase (for parameters at least) it should get sort[isActive] for an is_active property.

[VincentLanglet]

I don't understand, if the property is is_active then it should be is_active inside the metadata (Parameter::property). We should do no converting after metadata is computed.

$this->filterProperty($this->denormalizePropertyName($property), $value, $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);

This makes this harder as then the responsability of converting is everywhere, we now have a way to cache these transformations lets use it.

Given the fact that's not how it works at the moment, I also have some trouble to understand what's expected and what's buggy.

In my case I use the CamelCaseToSnakeCaseNameConverter which means that snake_case is used in for item normalization, but camelCase should be used in database.
Maybe the idea is that since the api returns foo_bar the user of the API only knows foo_bar and therefor

• the metadata should use foo_bar
• the filter should be order[foo_bar]
• the transformation should be later in the filter to use fooBar for the query

yes and there's an important difference between a parameter key (and a property), if you use the placeholder :property there'll be some magic happening and if the name converting is set to snakeCase to camelCase (for parameters at least) it should get sort[isActive] for an is_active property.

I agree.
I currently found an implementation which solve all my issues and regress only one Laravel case.
But I'm unsure how to fix it...

[soyuka]

The problem is using this class ^^

We should really get rid of it it shows that the nameConverter responsibility is at the wrong place, a filter should only be like this:

    public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, ?Operation $operation = null, array $context = []): void
    {
        $parameter = $context['parameter'];
        $value = $parameter->getValue();
        $property = $parameter->getProperty();
        $alias = $queryBuilder->getRootAliases()[0];
        $parameterName = $queryNameGenerator->generateParameterName($property);

        $queryBuilder
            ->{$context['whereClause'] ?? 'andWhere'}(\sprintf('%s.%s = :%s', $alias, $property, $parameterName))
			->setParameter($parameterName, $value);
    }

It should not have to transform property BUT if we change the AbstractFilter we break every filter that exists for 10 years.

[soyuka]

I'd rather not include this at runtime

[VincentLanglet]

I thought this was needed for AbstractFilter only. Because some "non-deprecated but legacy" filters are still using it (like the OrderFilter).

But after so much debugging, I'll try without.

[soyuka]

Maybe that we miss something, what is the value of $parameter->getProperty()? If we can't set the property directly from metadata maybe we need to add something in between? Don't worry about Laravel I'll check but please just send a failing test I'll take a look.

[VincentLanglet]

Maybe that we miss something, what is the value of $parameter->getProperty()? If we can't set the property directly from metadata maybe we need to add something in between? Don't worry about Laravel I'll check but please just send a failing test I'll take a look.

@soyuka Seems like the NameConverterAwareInterface is not needed anymore (?)
I reverted all related stuff.

All the tests added are green by removing the normalize call. There is only the Laravel one I'm not good with (I cannot even run the test locally). If you're ok checking it, that would be great !