philipn / django-rest-framework-filters

Better filtering for Django REST Framework
Other
848 stars 130 forks source link

Thoughts about supporting an AllLookupsFilter for a JSONField #96

Closed jsmedmar closed 8 years ago

jsmedmar commented 8 years ago

The JSONField is now supported by django restframework after version 3.3.3. I realized that when trying to use an AllLookupsFilter on such a field I get:

AttributeError: 'NoneType' object has no attribute 'model'

Would be interesting to hear insights from the authors regarding a filter for this field and the additional lookups supported by django.

I'm happy to help if possible!

jsmedmar commented 8 years ago

BTW: I found this discussion in django-filter. Which @rpkilby is already involved in. You may want to close this issue...

rpkilby commented 8 years ago

Yep - some thought has already been put into this. The short version is that regular lookups (including the additional JSONField lookups) should be compatible with AllLookupsFilter. This is because those lookups are made available through the lookup registration API. The additional lookups are registered here.

What is not possible at the moment is support for arbitrary key/index/path transforms, which is made possible here and discussed here. Because the key transforms aren't registered (which wouldn't make sense anyway), we're unable to introspect them and generate filters. There are theoretically an infinite number of valid key transforms anyway, which would introduce other problems.

I realized that when trying to use an AllLookupsFilter on such a field I get:

AttributeError: 'NoneType' object has no attribute 'model'

Could you post example code or a fork w/ working code? As far as I can tell, JSONField should be compatible with `AllLookupsFilter, including the additional lookups.

jsmedmar commented 8 years ago

Hey @rpkilby I forked the project and pushed this commit replicating the JSONField error.

I changed the database configuration to use postgres with a database called drff. To see the error just do python manage.py runserver and go to http://127.0.0.1:8000/notes/.

rpkilby commented 8 years ago

Hi @jsmedmar - I figured it out. The problem is that JSONField is not a field type recognized by the automatic filter generation. AllLookupsFilter uses the same code paths internally, which is what's causing the error.

I've filed carltongibson/django-filter#450, which is related to this. Specifically:

The implementation to support Meta.fields = None requires ignoring unrecognized field types, such as tests.models.SubnetMaskField. This prevents us from providing meaningful errors to ...

Fixing that issue would provide better error reporting, but you still need to use filter_overrides. Depending on what lookups you want to use, you may also need to extend filter_for_lookup (might be necessary for has_keys).

class NoteFilter(FilterSet):
    filter_overrides = {
        JSONField: {
            'filter_class': CharFilter,  # Presumably, CharFilter is correct?
        }
    }
    jsons = lookups.AllLookupsFilter()

    class Meta:
        model = Note
rpkilby commented 8 years ago

Removing the milestone, since there isn't anything to fix inside of drf-filters for an 0.8.1 release

rpkilby commented 8 years ago

Hi @jsmedmar - I've submitted carltongibson/django-filter#451 which should raise a more informative error message in future releases.

I'm also going to close this, as I don't think there's anything to be accomplished within the scope of drf-filters. If anything, JSONField support should be added to the core django-filters library.