Core: RFC: Filters do not respect the configured name converter.

[bendavies]

Hey all,

Here's an issue I discovered today.
If your API has a configured name converter, (let's say CamelCaseToSnakeCaseNameConverter), filtering on fields is unintuitive for the consumer.

Given an ApiResource with a field called createdAt, createdAt will be serialized/deserialized to/from the consumer as created_at.

However, if the consumer wants to then filter on createdAt, they must use ?createdAt..., which is very unintuitive as that is not how the field is represented.

The fix seems simple - make all filters respect the configured name converter.
However, this would be a big BC break, so will need to go in 3.0?

Cheers

[dunglas]

I would be in favor of fixing this. It looks totally unexpected on my side. If someone complains... we'll provide a fix like a flag or something like that to restore the buggy behavior. WDYT?

[bendavies]

Cool, while obviously a bug, I didn't expect a fix to be prioritized over the BC break. happy with that!

[soyuka]

I didn't expect a fix to be prioritized over the BC break

Most of us are using the default name converter, therefore nothing will change.

[teohhanhui]

Was it ever claimed that the filters would use the name converter? :stuck_out_tongue:

I don't know if it could be considered a bug if it's a missing feature...

[slebrequier]

Sorry to have opened a duplicate of this issue.
I, for one, think of it as a bug since the client have to filter from what he knows : the response body.
If the field is called created_at in the response body, then it must be created_at that will filter or sort a list of results.

[soullivaneuh]

@dunglas To avoid BC break, we could provide a api_platform.name_generator parameter and deprecate the api_platform.path_segment_name_generator one.

[soullivaneuh]

I really need this fix and would like to help making it real, but I don't even know where to start. Could you please give us some clues? Thanks!

[dunglas]

To me it’s a bug fix, so don’t bother with BC because the current behavior is not correct (this case has just been forgotten).

A PR would be very appreciated, just target master instead of 2.3 to limit the consequences in case someone was relying on the buggy behavior.

[soullivaneuh]

@dunglas Do you have any clues to give about how to fix it?

[antograssiot]

@Soullivaneuh Quick and dirty but I guess a start would be https://github.com/api-platform/core/compare/master...antograssiot:nameconverter-filter

But it would be a BC break I guess to add an argument in the middle of the constructor, and adding it in the last position, would break definitions like

    app.my_dummy_resource.date_filter:
        parent:    'api_platform.doctrine.orm.date_filter'
        arguments: [ { 'dummyDate': ~, 'relatedDummy.dummyDate': ~, 'embeddedDummy.dummyDate': ~ } ]
        tags:      [ { name: 'api_platform.filter', id: 'my_dummy.date' } ]

as we rely on properties being the last arg

It needs to be tested more deeply and I havent put effort in the SearchFilter yet, if it is considered as BC break.
I'm also unsure of what would be the impact o the admin part etc...
ping @api-platform/core

[soyuka]

@antograssiot I think that your changes will do the trick because we use parent in the filter service declaration and we use abstract=true when defining the base services.

To me it’s a bug fix, so don’t bother with BC because the current behavior is not correct (this case has just been forgotten).

To me it's just a missing feature, filter properties are not documented as converted but the same as the property names. I definitely agree that this is inconsistent though.

[soullivaneuh]

@antograssiot on your POC, you use the name converter on each filter getDescription method.

Why not leave this like that and use the name converter only on the documentation normalizer and the filter request extraction?

This will be also better for eventually other supports than Doctrine ORM.

[antograssiot]

I didn't think a lot about the issue @Soullivaneuh , I just quickly update with the first (not best) idea I had to see if it would work.. The discalmer was Quick and dirty :wink:

[soullivaneuh]

@antograssiot Ha ha right ! :wink:

I tried an another approach: https://github.com/api-platform/core/pull/2377

[bendavies]

think this is done!