flugg / laravel-responder

A Laravel Fractal package for building API responses, giving you the power of Fractal with Laravel's elegancy.
MIT License
865 stars 86 forks source link

Resource filtering #98

Open IlCallo opened 6 years ago

IlCallo commented 6 years ago

I see that it's possible to filter out relations, but it is not possible to apply a "filter" rule on a Transformer in a way that the resource itself will be filtered.

My use case is the following:

I can imagine that the filter will be stored as a filter() function in the transformer and that it must take place before transforming a resource, either if it is the base resource or an included relations, either if the model is a single object (maybe? not sure of this) or a collection. Not really proposing a solution here, more like "exploring the possibilities" and checking if the idea can be interesting.

Maybe it could be more interesting to abstract the concept into an ad-hoc entity category Filters (like there could be Decorators, Serializers or Transformers) and make them interact in some way with the others components of the library. Adding a "defaultFilters" or somethink like that and giving the possibility to apply filters manually.

flugg commented 6 years ago

I like this idea. The data could be filtered before the transformation takes place, but sometimes you may want to filter based on relationships or fields calculated in a transformer.

I'm not sure I can justify adding an entirely new concept of a Filter class, but a filter method in the transformers sound like a valid option. Are you imagining a method taking a transformed array item as the parameter? Like:

public function filter(array $item): array 
{
    return $item['transformed_field'] > 9000;
}

This could also be expanded to a method on the response builder, allowing for simple filtering per response:

return responder()->success($pies, PieTransformer::class)->filter(function ($transformedPie) {
    return $transformedPie->isHealthy;
})->respond();

I'll look into this soon!

IlCallo commented 6 years ago

I would keep it before the transformation. Virtually all data is accessible before transformation for you to decide, but not the contrary. Also, I think that it's always better to work with model classes instead of arrays, because it can help prevent errors and enhance code readability.

I would love having the ability to chain it to the response builder too

flugg commented 6 years ago

Just so I'm understanding this correctly; if this happens before the transformation takes place, how would this differ from just filtering the data before sending it to the responder?

IlCallo commented 6 years ago

It doesn't, at a techical level. It does at a logical level.

The problem can be addressed in various places of course:

The case which I found myself in was about the fact that the filter should be applied only when data exits the backend, so the more logical place to put it is the responder system

flugg commented 6 years ago

Makes sense, thanks!

I've been wanting some functionality for filtering and ordering the data automatically using query string parameters in the past. I know it's not exactly what you're asking for, but I feel like both scenarios could be solved the same way. I have considering creating a separate package for this, as I'm afraid this package might be too bloated with features. However, thinking about it, filtering and sorting the data might be a perfect match for a responder and complement the package's other features.

I'll tinker some with this and see if I get some good ideas!

IlCallo commented 6 years ago

Funny, I'll have to implement the same sorting thing myself in a not so distant future, if you're gonna add it to the package, that would be a lot easier to do xD Are you thinking about a "catch-for-all" method on which applying data manipulation before the transformation or about two distinct features?

flugg commented 6 years ago

Haha, great. I'm not sure yet to be honest, but I believe a dedicated Filterconcept or similar might make sense for this. Just throwing some ideas in the air, but for instance:

responder()->success($orders)->filter([RemoveOutdated::class, OrderByName::class])->respond();

The filters could be simple classes to perform the filtering. Although I don't have a clear vision of exactly how the implementation details would be.

Then perhaps some configuration option to map those filters to query string parameters automatically?

'filter_parameters' => [
    'remove_outdated' => RemoveOutdated::class
],
flugg commented 6 years ago

Thinking about it some more and looking at some other packages handling similar things (https://github.com/spatie/laravel-query-builder, https://github.com/cerbero90/query-filters), I believe this should be handled more generally. Filtering, ordering and searching is so common I believe these could be default Filter classes. For instance QueryFilter, SortFilter and SearchFilter where you can apply them by instantiating them and sending in parameters:

responder()->success($orders)->filter([
    new QueryFilter('outdated', true), 
    new SortFilter('name')
])->respond();

The configuration may then include some default filters:

'filters' => [
    'filter' => QueryFilter::class,
    'sort' => SortFilter::class
],

Allowing for dynamic query string parsing:

GET /orders?filter[outdated]=true&sort=name

This consequently means the filter method also will allow for the alternative syntax of:

responder()->success($orders)->filter([
    'filter' => [
        'outdated' => true
    ], 
    'sort' => 'name'
])->respond();

I would also like to support other where-clauses, like != or >:

new QueryFilter('created_at', '>', now())

Not sure how this would work yet with the query string parsing, but it seems doable to parse the operator used in the string and keep it in store for later somewhere.

I'm thinking the transformers also could specify a list of allowed filters as to not blindly allowing filtering and sorting for all resources.

Lastly, a valid filter could also be a closure, to support for:

responder()->success($orders)->filter([
    function ($order) {
        return ! $order->outdated;
    }
])->respond();

You could also omit the array brackets if only sending in one filter and we've ended up with allowing the same syntax that I originally proposed earlier in the thread. So, this way it seems to cover all bases. What do you think?

flugg commented 6 years ago

I'm trying to decide how to best handle query string parameters. The format I mentioned above...

?filter[outdated]=true&sort=name

...is basically based on the JSON API spec. While this format is the easiest to parse, I feel like it has some weaknesses. Like, you wouldn't be able to do a "not equal to" expression:

?filter[outdated]!=true

Since the ! would be swallowed by the request parser. This could instead be extended to a multidimensional array:

?filter[outdated][!=]=true

I like this idea, but then again, this format is completely different to the one provided through Fractal on relations:

?include=comments:limit(5|1):order(created_at|desc)

This would require a bit of parsing and since Fractal's parsing capabilities is baked into the transformation layer I would practically have to recreate the parsing logic in order to achieve the same effect. However, this might be yet another step in the direction of becoming more decoupled from Fractal. Using this style we could do something like this:

?filters=where(foo|>|10):sort(created_at|desc)

I'm not sure why Fractal chose to go for a delimiter instead of a comma to separate the arguments, but I think it looks way better with commas:

?filters=where(foo,>,10):sort(created_at,desc)

Just a few ideas 💡

IlCallo commented 6 years ago

So, this way it seems to cover all bases. What do you think?

Seems a good addition to the package and it actually covers almost everything, it's actually even more than what I expected xD

you wouldn't be able to do a "not equal to" [!=] expression since the ! would be swallowed by the request parser.

I'm not sure about which request parser are you talking about, does it also swallows brackets characters? If not, <> can be used to represent "not equal to". Maybe also ^=. There are a lot of possible notations meaning "not equal to" we can find as replacement to != if that's the problem.

This would require a bit of parsing and since Fractal's parsing capabilities is baked into the transformation layer I would practically have to recreate the parsing logic in order to achieve the same effect.

I'm not sure this is actually bad at all. The fact that parameters parsing is that coupled to the transformation layer seems odd to me, I think separating it could be a good thing. But if you have to rewrite all the parsing mechanism, at this point it's pretty pointless to keep the Fractal notation. When in doubt, I usually stick to the standard. And the standard in this case is the JSON API, not the Fractal style.

I'm not sure why Fractal chose to go for a delimiter instead of a comma to separate the arguments, but I think it looks way better with commas

This may be to allow usage of commas as decimal separator, which is not so uncommon (JSON API landing page shows exactly an example where commas are used in that way). If you decide to stick to Fractal style, in which case I highly suggest you to find a workaround to avoid to recreate the parsing logic, I'd stick to the | separator, which is far less common than commas for... eveything I guess :)

Sorry for not responding in these days but I've a lot of things to do both at work and out. Be prepared: today I upgraded my API to use version 3.0.0 and tomorrow I'll be starting trying to break things and opening issues :P I already found a problem when both an includeXxx and a whitelisted relation with it's mapping is present, eheheh

flugg commented 6 years ago

Seems a good addition to the package and it actually covers almost everything, it's actually even more than what I expected xD

Great!

I'm not sure about which request parser are you talking about, does it also swallows brackets characters? If not, <> can be used to represent "not equal to". Maybe also ^=. There are a lot of possible notations meaning "not equal to" we can find as replacement to != if that's the problem.¨¨

I'm talking about Laravel's parser, or just the general PHP parser. For instance, when using the following query string:

?name!=foo

request()->all() will return:

[
  "name!" => "foo"
]

In this case we could parse the ! from the key, however, in cases with arrays:

?where[name]!=foo

It will swallow the ! and output the following:

"where" => [
    "name" => "foo"
  ]

It seems that the only valid character after the array brackets is an equal sign, so neither <> or ^= would work either. Which is why I was thinking of using a nested array:

?where[name][!=]=foo

However, looking at it, it seems that != is not a valid array index either:

"where" => [
    "foo" => "]=10"
  ]

When in doubt, I usually stick to the standard. And the standard in this case is the JSON API, not the Fractal style.

Considering the points I made above I'm not sure the JSON API style is flexible enough to handle advanced expressions and another problem with this style has to do with relationships. To apply parameters to a relationship with the JSON API style would look like this:

?include[comments][limit]=5,1

This collides with the format we use to include relationships now as you can't mix arrays and scalars:

?include=comments

It would also be hard to know if limit was a nested relationship or a parameter. I think this might be why Fractal uses a different format altogether.

I think we should look into how to combine the features of Filters and relationship parameters though.

This may be to allow usage of commas as decimal separator, which is not so uncommon (JSON API landing page shows exactly an example where commas are used in that way). If you decide to stick to Fractal style, in which case I highly suggest you to find a workaround to avoid to recreate the parsing logic, I'd stick to the | separator, which is far less common than commas for... eveything I guess :)

Hmm, wouldn't periods be more common for decimal separators as that's what PHP (and most other languages) uses? Also, I couldn't find the example on the landing page, could you paste it here?

The parsing logic is actually not that bad, Fractal handles it all in one method (https://github.com/thephpleague/fractal/blob/master/src/Manager.php#L163) - so I wouldn't have a big problem with a recreating this in a more modular way.

There could be a QueryStringParser interface or something similar which you potentially could rebind to allow for other query string parsing strategies.

Sorry for not responding in these days but I've a lot of things to do both at work and out. Be prepared: today I upgraded my API to use version 3.0.0 and tomorrow I'll be starting trying to break things and opening issues :P I already found a problem when both an includeXxx and a whitelisted relation with it's mapping is present, eheheh

No problems at all. Hope the upgrade goes smoothly! If you have time, please check the new test suite added and see if it's possible to replicate your last issue through a test.

IlCallo commented 6 years ago

It would also be hard to know if limit was a nested relationship or a parameter. I think this might be why Fractal uses a different format altogether.

Yep, I guess it could be, maybe we should ask them. Maybe even open an issue on fractal package referencing this issue, but someone could take is as "not appropriate".

Maybe dropping signs and moving into usage of words (eq, gt, not, etc) can solve the problem with the != operator? A not can be used in the array form you proposed instead of !=.

IBM solves the problem by using the encoded values of the signs and putting the expression in more natural-language-style way. Microsoft follows that idea too.

Drupal seems to use a way like the array form you proposed.

This collides with the format we use to include relationships now as you can't mix arrays and scalars

Well, that's because we are tightly coupled to Fractal current syntax. Not a problem if you prefer to stick to it, just noting that there are a few around and switching to a different notation removes also some limitations.

Hmm, wouldn't periods be more common for decimal separators as that's what PHP (and most other languages) uses? Also, I couldn't find the example on the landing page, could you paste it here?

Yes they are more common, mine was an hypotesis. I currently cannot find where I saw the example... Given the hour it was when I wrote that answer, I probably imagined it, at this point...