Add support for comma-separated filter values #236
Conversation
|
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. |
|
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 ***@***.***> 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>
.
|
|
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). Let me know what you think! |
| @@ -67,12 +67,20 @@ defmodule JSONAPI.QueryParserTest do | |||
| assert filter[:name] == "jason" | |||
| end | |||
|
|
|||
| test "parse_filter/2 handles multiple comma-separated filter values" do | |||
| test "parse_filter/2 handles multiple comma-separated filter values if configured" do | |||
| Application.put_env(:jsonapi, :filter_values_separator, ";") | |||
There was a problem hiding this comment.
Should this be cleaned up in an on_exit hook?
There was a problem hiding this comment.
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.
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 |
|
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 |
|
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. |
Implements #235