Change in get_filtering_args_from_filterset on 2.4.0 breaks some filters

[kikeh]

Context

In the latest release there is a bugfix (#682) that intends to Ensure correct filter types for DjangoFilterConnectionFields, which is related to issue #678. The problem is that the change in the function get_filtering_args_from_filterset() has two possible issues (one minor and one major):

1. form_field not defined (minor):
   This is a simple one. Following the logic in this new implementation if the name of the filter is not in the filterset_class.declared_filters and field_name is not an attribute of the model, it will never get form_field defined, so when it gets to:

if not form_field:
    form_field = filter_field.field

We get a local variable 'form_field' referenced before assignment.

2. Filters that are not applied to fields of the model will crash (major):
   After fixing the previous issue, we still have the problem that the new implementation will only look for filters either in the declared_filters or for filter names that could match a field from a model. In our case, we have a mixin that overrides the get_filters method and updates the filters with some common ones that are not field filters. So we get a django.core.exceptions.FieldDoesNotExist: xxx has no field named 'xxxxx'

Steps to reproduce it

1. Create a filterset based on a mixin:

class FooMixin:

    @classmethod
    def get_filters(cls):
        filters = super().get_filters()
        filters.update({
                'partner__status': django_filters.ChoiceFilter(
                    method='filter_status',
                    field_name='users__status',
                    choices=users.field.model.STATUS,
                ),
            })

        return filters

class FooFilter(FooMixin, django_filters.FilterSet):
    . . .

2. Try any query that uses this filter.

What can we do?

I do have a proposal on how to fix this issue, but I wanted to hear your opinion first about this issue, as if this is an actual issue or if we are doing something wrong. So please, if you have any suggestions, let me know.

Proposal

My proposal to fix this issue would be changing the current implementation of the function mentioned before get_filtering_args_from_filterset:

def get_filtering_args_from_filterset(filterset_class, type):
    """ Inspect a FilterSet and produce the arguments to pass to
        a Graphene Field. These arguments will be available to
        filter against in the GraphQL
    """
    from ..forms.converter import convert_form_field

    args = {}
    model = filterset_class._meta.model
    for name, filter_field in six.iteritems(filterset_class.base_filters):
        form_field = None

        if name in filterset_class.declared_filters:
            form_field = filter_field.field
        else:
            field_name = name.split("__", 1)[0]

            if hasattr(model, field_name):
                model_field = model._meta.get_field(field_name)

                if hasattr(model_field, "formfield"):
                    form_field = model_field.formfield(
                        required=filter_field.extra.get("required", False)
                    )

        # Fallback to field defined on filter if we can't get it from the
        # model field
        if not form_field:
            form_field = filter_field.field

        field_type = convert_form_field(form_field).Argument()
        field_type.description = filter_field.label
        args[name] = field_type

    return args

Here we are defining form_field as None, in case it doesn't match any of the following conditions, and checking if the filter is related to a field of the model before calling get_field.

[jkimbo]

Thanks for the detailed write up @kikeh ! You're proposal looks great, could you raise a PR with the changes and the necessary regression tests?

I confess that I haven't had much experience with django_filters so it's not surprising that I missed something when implementing that bug fix.

[kikeh]

@jkimbo sure, I am going to create the PR.

And don't worry, this one was a quite specific issue but it was not so hard to spot :)

[dan-klasson]

This is completely broken for me (#709). Has anyone gotten class based filtering to work with graphene?