Add support for comma-separated filter values

[CostantiniMatteo]

Implements #235

[jherdman]

LGTM. I need to wrap my head around how to release this. I think it's a breaking change despite being what is pretty obviously a bug in the initial implementation. I'd be curious to hear what other maintainers think.

cc @jeregrine @doomspork

[CostantiniMatteo]

I've re-read the documentation and I've made a mistake. The part I quoted in the issue is from the recommendations page so it's not supposed to be the default jsonapi behaviour.

Maybe this could be a configurable behaviour?

[jherdman]

I'm pretty certain this is the kind of behaviour users of this library would expect. My inclination is to merge this as a bug fix and call it out in the changelog.

[CostantiniMatteo]

I do agree with you, I would expect this behaviour, however this is not jsonapi standard.

I was discussing this with @lucacorti, which pointed out that this is only a recommendation's example, and one may want to support a different a strategy for filtering, or may want to use a comma in the filter value.

Isn't it better to at least offer a way to prevent this behaviour via configuration?

[jherdman]

You've convinced me: configuration is probably the way to go here.

…

On Tue, Sep 8, 2020 at 4:31 AM Matteo Costantini ***@***.***> wrote: I do agree with you, I would expect this behaviour, however this is not jsonapi standard. I was discussing this with @lucacorti <https://github.com/lucacorti>, which pointed out that this is only a recommendation's example, and one may want to support a different a strategy for filtering, or may want to use a comma in the filter value. Isn't it better to at least offer a way to prevent this behaviour via configuration? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAZZCVBUFR4MFGDBAZYLLSEXTVDANCNFSM4QYOPF4Q> .

[CostantiniMatteo]

Hey @jherdman, sorry I was off the grid.

I've updated the PR to make the parsing configurable.

I don't particularly like the config name, and how I handled the tests (without sacrificing concurrency).
Since they run asynchronously, tests at line 70 and 77 may change the same configuration and one of the two may fail.
The problem may still arise with the current implementation, although much less likely.

Let me know what you think!

[jherdman]

Should this be cleaned up in an on_exit hook?

[CostantiniMatteo]

Absolutely! I've restored a previous commit where the whole env is backed up before tests and restored on exit.
This way we are sure we won't forget to clean up other variables.

[jherdman]

Hey @jherdman, sorry I was off the grid.

Never sweat about this. F/OSS is done on a volunteer basis for the vast majority of people. Any and all contributions are much appreciated!

[CostantiniMatteo]

Could we take the chance to also merge this one among #245 and make a new release? We ended up not using this feature so I forgot about it, but it would still be useful. What do you think?

[lucacorti]

@CostantiniMatteo I think merging this would be a mistake as this is really constraining what the spec allows in term of filtering: https://jsonapi.org/format/#fetching-filtering

The spec specifies that the filter parameter is used for filtering and its content are entirely up to implementation specific strategy. Processing the filter query param with some predefined logic is really limiting in terms of what can be implemented. Maybe we could explore some delegation to a behaviour like what I implemented earlier for pagination, or leave it entirely to the library user to parse its content?

[CostantiniMatteo]

The processing is optional and needs to be configured so it's not limiting and does not break the existing behaviour.

Maybe it can be refactored to accept a module used to parse the filter content, and include a parser with support for comma-separated values, instead of the current hard-coded solution.

[lucacorti]

You can't implement all possible user needs on this, so extensible is arguably better than any built in logic.

[doomspork]

@jeregrine / @jherdman / @lucacorti / @CostantiniMatteo where do we stand with this PR? It looks like it was approved but there's been some discussion about whether this is the change we want.

[lucacorti]

@doomspork @CostantiniMatteo as I said I think this would be better addressed with an extensible approach instead of configurable baked in logic.

[github-actions]

This pull request has been automatically marked as "stale:discard". If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated!.

[github-actions]

Closing this pull request after a prolonged period of inactivity. If this issue is still relevant, please ask for this pull request to be reopened. Thank you!