jsonapi
jsonapi copied to clipboard
beam-community
Add support for comma-separated filter values
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
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?
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.
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?
You've convinced me: configuration is probably the way to go here.
On Tue, Sep 8, 2020 at 4:31 AM Matteo Costantini [email protected] 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 https://github.com/jeregrine/jsonapi/pull/236#issuecomment-688710633, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAZZCVBUFR4MFGDBAZYLLSEXTVDANCNFSM4QYOPF4Q .
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!
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!
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?
@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?
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.
You can't implement all possible user needs on this, so extensible is arguably better than any built in logic.
@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.
@doomspork @CostantiniMatteo as I said I think this would be better addressed with an extensible approach instead of configurable baked in logic.