search

thevahidal / soul

🕉 A SQLite REST and realtime server
https://thevahidal.github.io/soul/
MIT License
1.45k stars 50 forks source link

added feature to also include array as filter in GET /:name/rows route #132

Closed TahaKhanAbdalli closed 11 months ago

TahaKhanAbdalli commented 11 months ago

After this merge the filters in GET /:name/rows can also include array e.g. _filters=id:[1,2,3],firstName:John,lastName:Doe

This fix has been sponsored by @IanMayo

fixes #131

IanMayo commented 11 months ago

Hello @thevahidal , any chance of review/merge on this PR please?

It's the last feature required in order for Soul to be compliant as a REST backend for react-admin

thevahidal commented 11 months ago

@TahaKhanAbdalli Thanks for the PR, I think it's better if we use a syntax like what we have for greater than, less than, and ... queries, in those cases, we use a gt, lt lookup appended to the end of the field name, e.g. id_gt=..., id_lt=.... I think it's a good pattern to also use in for multiple lookup values, e.g. id__in=.... @IanMayo What's your opinion on this?

thevahidal commented 11 months ago

@TahaKhanAbdalli There's also some conflicts needed to be handled. Would you please bump of the minor version of Soul Core to v0.6.0

IanMayo commented 11 months ago

@thevahidal - sure, that strategy separates how API supports single vs multiple key values, which keeps things simple.

IanMayo commented 11 months ago

Hello @TahaKhanAbdalli - I support @thevahidal suggestion to support multiple matching keys using the __in operator.

IanMayo commented 11 months ago

I think it's a good pattern to also use in for multiple lookup values, e.g. id__in=.... @IanMayo What's your opinion on this?

Aaah, @thevahidal - @TahaKhanAbdalli has pointed out two issues with this:

  1. we're extending Soul API to match the REST standards employed by the react-admin front-end library, and react-admin uses id:[2,3,4]
  2. with the array of items we can split the URL by , except for commas inside [] brackets. Without the brackets it's not possible to split by ,. Aaah, hold on. This would match your recommendations: id__in=[2,3,4]

So,

  1. Are you willing to relax your API recommendation if we're already following an existing standard?
  2. If not, are you ok with the multiple elements being provided in array brackets?
thevahidal commented 11 months ago

@IanMayo, the __in= is more of a nice-to-have feature and if react-admin is already using it the way @TahaKhanAbdalli implemented it, I'm ok to move on with this.

thevahidal commented 11 months ago

@all-contributors please add @thevahidal for review.

allcontributors[bot] commented 11 months ago

@thevahidal

I've put up a pull request to add @thevahidal! :tada: