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

AllLookupsFilter does not work w/ FilterSet mixins #82

Closed
NotSqrt opened this issue Apr 19, 2016 · 6 comments
Closed

AllLookupsFilter does not work w/ FilterSet mixins #82

NotSqrt opened this issue Apr 19, 2016 · 6 comments
Milestone

Comments

@NotSqrt
Copy link

NotSqrt commented Apr 19, 2016

Hi !

cf https://docs.djangoproject.com/en/1.9/topics/db/models/#abstract-base-classes

Is there an equivalent for FilterSets ?

When I do:

class FilterMixin(FilterSet):
    common_field = AllLookupsFilter('field_name')

   # no possibility to do:
   # class Meta:
   #    abstract = True

class ActualFilter(FilterMixin, FilterSet):
    # other filters
    class Meta:
        model = MyModel

I get an error :

  File "rest_framework_filters/filterset.py", line 57, in __new__
    field = filterset.get_model_field(model, filter_.name)

If I do class FilterMixin(object): .., the common filters are not detected.

I would really like not having to repeat the fields multiple times !

Thanks !

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 19, 2016

Hi @NotSqrt

cf https://docs.djangoproject.com/en/1.9/topics/db/models/#abstract-base-classes

Is there an equivalent for FilterSets ?

No, django-filter is a very similar abstraction to standard django forms and is in fact built on top of them. There are no abstract Forms, no abstract FilterSets.

Could you provide a more complete stack trace? That's not enough information to go off of.

@rpkilby rpkilby modified the milestone: v0.8.1 Apr 20, 2016
@NotSqrt
Copy link
Author

NotSqrt commented Apr 20, 2016

Here is a longer trace:

Traceback (most recent call last):
  File "mycode.py", line 34, in <module>
    class MessageMixin(FilterSet):
  File "env/rest_framework_filters/filterset.py", line 57, in __new__
    field = filterset.get_model_field(model, filter_.name)
  File "env/django_filters/utils.py", line 43, in get_model_field
    opts = model._meta
AttributeError: 'NoneType' object has no attribute '_meta'

when my code looks like:

class MessageMixin(FilterSet):
    common_field = AllLookupsFilter('field_name')

    # no model in Meta class, this is a mixin

class ActualFilter1(MessageMixin, FilterSet):
    # other filters
    class Meta:
        model = MyModel1

class ActualFilter2(MessageMixin, FilterSet):
    # other filters
    class Meta:
        model = MyModel2

The exception is raised at import time, so runserver or anythong else fails immediately.

If I simply do:

class MessageMixin(object):
    common_field = AllLookupsFilter('field_name')

class ActualFilter1(MessageMixin, FilterSet):
    # other filters
    class Meta:
        model = MyModel1

class ActualFilter2(MessageMixin, FilterSet):
    # other filters
    class Meta:
        model = MyModel2

the common_field filter is not usable.

Whereas for modelforms, it works perfectly to do:

class FormMixin(ModelForm):
    common_field = AllLookupsFilter('field_name')

    # no model in Meta

class ActualForm1(FormMixin):
    # other filters
    class Meta:
        model = MyModel1

class ActualForm2(FormMixin):
    # other filters
    class Meta:
        model = MyModel2

@NotSqrt
Copy link
Author

NotSqrt commented Apr 20, 2016

In django's ModelFormMetaclass.__new__, there's a if opts.model: # If a model is defined, extract form fields from it. code branch.

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 20, 2016

Ah, that makes more sense. Checking for the presence of the model is the right thing to do here.

@rpkilby rpkilby changed the title Equivalent of django's abstract models for filters AllLookupsFilter does not work w/ FilterSet mixins Apr 20, 2016
rpkilby pushed a commit that referenced this issue Apr 20, 2016
Fix #82, make all lookups work w/ FilterSet mixins
@rpkilby
Copy link
Collaborator

rpkilby commented Apr 20, 2016

Alright, this should be fixed on master - it was a straightforward change.

@NotSqrt
Copy link
Author

NotSqrt commented Apr 20, 2016

Great ! Thanks !

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

No branches or pull requests

2 participants