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 filter instance to Filter.method signature #1150

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Nov 15, 2019

Filter.method enables ad hoc filtering behavior, however the current implementation does not sufficiently support writing more generic methods, as you cannot easily access the associated filter instance or its properties. With the filter instance, the method could alter its behavior, like whether it should exclude or use a specific lookup_expr. This PR proposes changing the method callback signature to provide the filter instance in lieu of the model field name.

Context:
Let's work with the following example:

class MyFilter(FilterSet):
    date = IsoDateFilter(field_name='f', lookup_expr='exact', method='date_filter')
    not_date = IsoDateFilter(field_name='f', lookup_expr='exact', exclude=True, method='date_filter')
    date_before = IsoDateFilter(field_name='f', lookup_expr='lte', method='date_filter')

    def date_filter(self, qs, field_name, value):
        ...

With the current implementation, it's not possible for date_filter to differentiate between the three filters, as only the field_name is provided as context, and all three filters reference the same field.

Current workarounds involve either abusing the field_name to provide both the attr/field names (e.g., field_name="<attr name>-<field name>") or to use functools.partialmethod. e.g.,

class MyFilter(FilterSet):
    date = IsoDateFilter(field_name='f', lookup_expr='exact', method='date_filter')
    not_date = IsoDateFilter(field_name='f', lookup_expr='exact', exclude=True, method='not_date_filter')

    def generic_date_filter(self, qs, field_name, value, attr):
        f = self.filters[attr]
    
    date_filter = partialmethod(generic_date_filter, attr='date')
    not_date_filter = partialmethod(generic_date_filter, attr='not_date')

Proposal:
Change the signature to include the filter instance instead of the field_name. e.g.,

    def filter_method(self, f, qs, value):
        ...

def filter_func(f, qs, value):
    ...

Because the first positional argument has changed from the QuerySet to a Filter instance, this should raise a loud, but easy to fix AttributeError. There might be a possible deprecation path here (check for AttributeError and object type, then retry with old signature call), or at a minimum, we can catch all AttributeErrors and reraise with an explanatory message.

TODO:

  • Try to implement deprecation path
  • Documentation updates
  • Migration entry

@codecov-io
Copy link

codecov-io commented Nov 15, 2019

Codecov Report

Merging #1150 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
+ Coverage   99.43%   99.43%   +<.01%     
==========================================
  Files          15       15              
  Lines        1235     1237       +2     
==========================================
+ Hits         1228     1230       +2     
  Misses          7        7
Impacted Files Coverage Δ
django_filters/filters.py 100% <100%> (ø) ⬆️
django_filters/filterset.py 100% <100%> (ø) ⬆️

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 9a11b0b...2876594. Read the comment docs.

@carltongibson
Copy link
Owner

carltongibson commented Nov 27, 2019

Just a question: Does method not take the actual callable, so making the partial approach look more appealing?

@rpkilby
Copy link
Collaborator Author

rpkilby commented Dec 2, 2019

The method accepts either a callable or a method name on the class. But since a given callable wouldn't be a method on the class, it wouldn't have any way of accessing the filterset/filter instances.

@gustabot42
Copy link

This feature is very useful, I am using part of the code to define negations in custom methods.
I would like to see it mergued, but I see that what is missing is the deprecation path.
Is there any progress in the deployment?

@rpkilby rpkilby marked this pull request as ready for review May 9, 2020 00:23
@rpkilby rpkilby force-pushed the filter-method-name branch from 66d4f67 to 9f1a64b Compare May 9, 2020 00:31
@rpkilby
Copy link
Collaborator Author

rpkilby commented May 9, 2020

Okay, this should be ready for review. A few thoughts:

  • It would be nice to merge Formalize filter binding to parent #1182 first, as the filter binding is unrelated to this PR.

  • Added a deprecation path that will call the old signature if the first attempt fails. Note that it wasn't as clear cut as I first thought:

    Because the first positional argument has changed from the QuerySet to a Filter instance, this should raise a loud, but easy to fix AttributeError.

    Both the queryset and filter have a filter method, so you'd actually get a TypeError here due to the difference in signature. Although, accessing any other queryset methods would likely cause an AttributeError.

  • Either way, the deprecation path falls back to re-calling the method with the old signature. There is a potential downside of side-effects being triggered twice, but I don't think this is a huge concern? Querying does not typically cause state changes. The alternative is to not have a deprecation path and raise an error.

  • Should determine when we'd want to remove a deprecation path. right now, TBD.

@rpkilby rpkilby marked this pull request as ready for review May 11, 2020 20:47
@carltongibson carltongibson self-requested a review May 17, 2020 10:29
@codecov-commenter
Copy link

Codecov Report

Merging #1150 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1150   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files          15       15           
  Lines        1255     1269   +14     
=======================================
+ Hits         1248     1262   +14     
  Misses          7        7           
Impacted Files Coverage Δ
django_filters/filters.py 100.00% <100.00%> (ø)
django_filters/filterset.py 100.00% <100.00%> (ø)
django_filters/utils.py 100.00% <100.00%> (ø)

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 1880430...9f1a64b. Read the comment docs.

@yakimka
Copy link

yakimka commented Oct 29, 2020

Any progress? This feature is very useful, I would like to see it merged.

Base automatically changed from master to main March 3, 2021 08:48
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.

6 participants