Add filter instance to `Filter.method` signature

[rpkilby]

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:

• [x] Try to implement deprecation path
• [x] Documentation updates
• [x] Migration entry

[codecov-io]

Codecov Report

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

@@            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%> (ø)	:arrow_up:
django_filters/filterset.py	100% <100%> (ø)	:arrow_up:

---

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]

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

[rpkilby]

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]

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]

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

• It would be nice to merge #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.

[codecov-commenter]

Codecov Report

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

@@           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]

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