Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ComplexFilterBackend #198

Merged
merged 7 commits into from
Dec 30, 2017
Merged

Add ComplexFilterBackend #198

merged 7 commits into from
Dec 30, 2017

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Oct 25, 2017

This is a first pass at #54, which should enable filtering the UNION, INTERSECTION, and DIFFERENCE of multiple invocations of the same filteset. For example, let's say you want to filter for users whose first_name=bob or last_name=jones. The desired query is something like:

q1 = User.objects.filter(first_name='bob')
q2 = User.objects.filter(last_name='jones')

return q1.union(q2)

The above is not currently possible, without creating a custom Filter.method. Even with a custom method, this solution is not flexible and does not easily allow for arbitrary queries.

Ultimately, the problem is at a higher order than the individual filterset, which is focused on producing a single query. The solution is to perform set operations on the results of multiple filterset queries. eg,

q1 = UserFilter({'first_name': 'bob'}).qs
q2 = UserFilter({'last_name': 'jones'}).qs

return q1.union(q2)

However, the problem is that querystrings only represent a single set of key=value pairs. This PR circumvents this by providing a custom MultiFilterBackend that expects the various groups of query params to be encoded as a single value in the querysting. The basic procedure for the client is to encode the sets of params, wrap those in parentheses, apply the set operations to the groups, then encode the overall result and assign to an actual query param name. The encoding steps should look something like:

  • (first_name=bob) | (last_name=jones)
  • (first_name%3Dbob) | (last_name%3Djones)
  • %28first_name%253Dbob%29%20%7C%20%28last_name%253Djones%29
  • filters=%28first_name%253Dbob%29%20%7C%20%28last_name%253Djones%29

@rpkilby rpkilby added this to the v1.0.0 milestone Oct 25, 2017
@rpkilby rpkilby mentioned this pull request Oct 25, 2017
@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #198 into master will increase coverage by 0.06%.
The diff coverage is 98.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   98.67%   98.74%   +0.06%     
==========================================
  Files           3        4       +1     
  Lines         151      239      +88     
  Branches       35       50      +15     
==========================================
+ Hits          149      236      +87     
- Partials        2        3       +1
Impacted Files Coverage Δ
rest_framework_filters/backends.py 100% <100%> (ø) ⬆️
rest_framework_filters/utils.py 100% <100%> (ø) ⬆️
rest_framework_filters/complex_ops.py 97.77% <97.77%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 454e7d8...4f52885. Read the comment docs.

@rpkilby rpkilby changed the title Add MultiFilterBackend [wip] Add MultiFilterBackend Oct 26, 2017
@gatensj
Copy link

gatensj commented Nov 22, 2017

Hey there. To start, I really appreciate the effort you put into the Multifiltering. Currently I am trying to pair up your multifiltering branch with the jQuery Query Builder. Reason for the message, is the MulitFilteringBackend breaks the pagination of DRF. I’ve been looking at your filter_queryset method and think the issue might be inside there. For whatever reason, the MultiFilteringBackend is not changing the position of the results based on the offset query parameter. Wanted to send you a line, because your branch has added excellent functionality to the DRFF and thought you should know about this issue. Please let me know if I can be of any assistance and I’ll respond accordingly. Thanks again for your effort on this repo. Take care and have a good day.

Sincerely,

JG

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 22, 2017

Hi @gatensj, thanks for raising the issue. Can you give an example querystring here? I'm wondering if the pagination params have been added to the filtering param, instead of remaining separate. eg, you should have something like:

/api/endpoint?page=1&filters=<escaped filters>

not

/api/endpoint?filters=<escaped filters + pagination>

@gatensj
Copy link

gatensj commented Nov 27, 2017

Hey @rpkilby. Thanks for responding to my message. Listed below is the query I used against the API, which breaks the pagination. I tried adding the pagination variables before the multi_filter_param, to no avail. Please let me know if there’s anything I can do to help, and I’ll respond accordingly. Thanks again for your work on this project. Take care and have a good day.

http://API/?filters=%28hml%253D-0.0028%29&offset=10

http://API/?offset=10&filters=%28hml%253D-0.0028%29

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 28, 2017

Thanks @gatensj, I'll take a look.

@gatensj
Copy link

gatensj commented Dec 13, 2017

Hey @rpkilby - I wanted to message you to see if you had a chance to look at the pagination issue? Odds are you have gotten busy with the end of the year bustle. Anyway my apologies for all these messages.

I am using your multifiltering branch, and other than the pagination, it is working really well. Being able to use an OR clause in an API call is a big win. I am looking to move my project to prod, however the pagination issue is holding up the process.

So long story long, please let me know if there’s anything I can do to help. My friend Tim just submitted a PR to fix a different issue. We are here, if there’s any testing you need completed. Thank you again for all your effort on this project. This is some bleeding edge tech and you are doing some great work. Take care and have a good day.

Sincerely,

JG

@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 13, 2017

Odds are you have gotten busy with the end of the year bustle.

Yep - a lot of my open PRs have gotten a bit side tracked.

Anyway my apologies for all these messages.

Absolutely not a problem.

So long story long, please let me know if there’s anything I can do to help.

At this point, the most helpful thing would be to provide test cases for issues you run into. I'd either open a PR against my fork (as your friend Tim did), or open an issue on my fork and post the code there.

@rpkilby rpkilby force-pushed the multi-filtering branch 2 times, most recently from 70ea757 to 2b1270e Compare December 14, 2017 00:45
@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 14, 2017

Hi @gatensj. The PR should be compatible w/ pagination now.

@gatensj
Copy link

gatensj commented Dec 21, 2017

Hey @rpkilby . Just wanted to take a moment to thank you for all your work on this project. The mulifiltering with pagination is working great! Your work helped me out big time with a project. Thanks again for the help. Take care and have a great day.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 22, 2017

Just as a heads up, I'm changing the name to reflect the complex queryset operations. Something along the lines of ComplexFilterOpBackend or ComplexOperationsBackend.

@rpkilby rpkilby changed the title [wip] Add MultiFilterBackend Add ComplexFilterBackend Dec 22, 2017
@rpkilby rpkilby force-pushed the multi-filtering branch 2 times, most recently from d44f7f2 to fbeb181 Compare December 28, 2017 00:12
@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 28, 2017

I've taken a second stab at this, removing the difference op and adding negation. eg, syntax looks like:

(param1=value1) & (param2=value2) | ~(param3=value3)

Negation can be disabled by setting negation = False on the backend.
Supported operators are & and | and can be overridden by setting your own operators map on the backend. For example, users may wish to use the queryset union, intersection, and difference methods to combine the querysets instead of & and |.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 28, 2017

A few other notes:

  • negation may not be correct - it simply calls qs.query.where.negate(). This seems to work for some simple cases, but may not be sufficient for others (such as aggregations and annotations).
  • filterset errors are now collected and raised as a single validation error. for example, with a complex query like (a=1&b=2) | (c=3&d=4), errors would be collected and raised like so:
    {
        "filters": {
            "a=1&b=2": {
                "a": [...]
            },
            "c=3&d=4": {
                "c": [...]
            }
        }
    {
    

@rpkilby rpkilby merged commit 551b680 into philipn:master Dec 30, 2017
@rpkilby rpkilby deleted the multi-filtering branch December 30, 2017 06:00
@FlipperPA
Copy link

Awesome work, thank you. In building the tests around the bug fix I was working on, I discovered we had errantly placed ComplexFilterBackend into DEFAULT_FILTER_BACKENDS. By not specifying any DEFAULT_FILTER_BACKENDS, we are working as expected, and just including filter_backends explicitly on the views we need. Thank again!

@warvariuc
Copy link

Do I understand correctly that nested parens are not supported?

response = self.client.get('/api/entries/', {
            'filters': "(created_at>'2020-02-03T06:00:00')&((distance<15000)|(distance>25000))"
        })

Response

{'filters': ["Ending querystring must not have trailing characters. Matched: ')'."]}

@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 16, 2019

That is correct. The current implementation does not support nested groups, although this is something that's being looked into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants