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

isnull for ForeignObjectRel fields #882

Closed
ellamental opened this issue Mar 4, 2018 · 5 comments
Closed

isnull for ForeignObjectRel fields #882

ellamental opened this issue Mar 4, 2018 · 5 comments

Comments

@ellamental
Copy link

First some setup. I have the following models defined:

class Task(models.Model):
    # ...

class Domain(models.Model):
    # ...

class DomainTask(models.Model):
    task = models.ForeignKey('Task')
    domain = models.ForeignKey('Domain')
    # ...

To get all Tasks not referenced by a DomainTask, Django allows: Task.objects.filter(domaintask__isnull=True).

Using the 'django_filters.rest_framework.DjangoFilterBackend' as a default filter, I have the following view:

from rest_framework import mixins
from rest_framework import viewsets

class Task(mixins.ListModelMixin,
           # ...
           viewsets.GenericViewSet):
    queryset = models.Task.objects.all()
    filter_fields = {
        'domaintask': ['exact', 'isnull'],
        'domaintask__domain': ['exact'],
        # ...
    }

The following request doesn't seem to apply the isnull filter.

GET http://localhost:8000/api/tasks/?domaintask__isnull=true

I looked into the filter generation and it seems as though BaseFilterSet.get_filters doesn't create filters for the lookup expressions.

I hacked together a proof-of-concept for supporting isnull here. I'm curious if there's a reason isnull isn't supported, or if there is an existing way to achieve this that I'm simply overlooking?

@carltongibson
Copy link
Owner

Try this:

GET http://localhost:8000/api/tasks/?domaintask=None

Does this get converted to isnull under the hood? (It should I think.)

@ellamental
Copy link
Author

@carltongibson I don't believe it does. Both of these queries return no results for me (they should):

GET http://localhost:8000/api/tasks/?domaintask=None
GET http://localhost:8000/api/tasks/?domaintask=null

Though, I don't think I'd be able to do the opposite query (Task.objects.filter(domaintask__isnull=False)) using that method either.

@ellamental
Copy link
Author

ellamental commented Mar 4, 2018

After a bit more investigation, it looks like django.forms.models.ModelMultipleChoiceField._check_values is raising a ValidationError which is being swallowed somewhere (though I'm not sure where).

It appears that the code intends to "filter out the null value", but I don't think it's succeeding in my case. I added the following print statements to django_filters.fields.ModelMultipleChoiceField._check_values:

class ModelMultipleChoiceField(ChoiceIteratorMixin, forms.ModelMultipleChoiceField):
    iterator = ModelChoiceIterator

    def _check_values(self, value):
        print 'self.null_label', self.null_label
        print 'self.null_value', self.null_value
        print 'self.null_label is not None: ', self.null_label is not None
        print 'value: ', value
        print 'self.null_value in value: ', self.null_value in value

        null = self.null_label is not None and value and self.null_value in value
        if null:  # remove the null value and any potential duplicates

            print 'We never get here because self.null_label is None'
            value = [v for v in value if v != self.null_value]

        result = list(super(ModelMultipleChoiceField, self)._check_values(value))
        print 'We never get here because the superclass raises a ValidationError on `[u'null']`.'

        result += [self.null_value] if null else []
        return result

This is the output for GET http://localhost:8000/api/tasks/?domaintask=null:

self.null_label: None
self.null_value: null
self.null_label is not None:  False
value:  [u'null']
self.null_value in value:  True

And this is the output for GET http://localhost:8000/api/tasks/?domaintask=None:

self.null_label: None
self.null_value: null
self.null_label is not None:  False
value:  [u'None']
self.null_value in value:  False

EDIT: This does filter properly if I change the null check to: null = value and self.null_value in value

@ellamental
Copy link
Author

GET http://localhost:8000/api/tasks/?domaintask=None

Does this get converted to isnull under the hood? (It should I think.)

This works if settings.FILTERS_NULL_CHOICE_LABEL is set to a truthy value (like 'None'). So I can at least achieve the goal in my example of filtering for Tasks not referenced by a DomainTask.

Though there are at least 2 outstanding issues with this approach:

  1. I don't believe this solves for the case of filtering for Tasks.objects.filter(domaintask__isnull=False).

  2. FILTERS_NULL_CHOICE_LABEL seems kind-of orthogonal to whether a NULL value should be allowed in a multiple choice field. Though I can see why the presence of a label would be a good indicator that you want to accept NULL values, setting a label to enable NULL values isn't entirely intuitive (especially if you're not using any of the generated filter forms).

@carltongibson if you're interested, I'd be happy to whip up a real PR for:

  1. Allowing isnull (as demonstrated in https://github.com/jacktrades/django-filter/pull/1 but in a much less hacky way and with some actual tests).

and/or

  1. Removing gating on the presence of FILTERS_NULL_CHOICE_LABEL for allowing a NULL value to be passed to ModelMultipleChoiceField. Or possibly allowing an alternative setting that's unrelated to the label, (for a likely horrible name off-the-top-of-my-head, maybe FILTERS_ALLOW_NULL_CHOICE). Something like:

    null = (self.null_label is not None or settings.FILTER_ALLOW_NULL_CHOICE) and value and self.null_value in value

@rpkilby
Copy link
Collaborator

rpkilby commented May 21, 2018

Hi @jacktrades. #915 should address this issue by enabling non-exact (eg, isnull) lookups for reverse relationships. eg, the following should generate both domaintask and domaintask__isnull filters:

class Task(viewsets.ModelViewSet):
    queryset = models.Task.objects.all()
    filter_fields = {
        'domaintask': ['exact', 'isnull'],
    }

The PR basically folds filter_for_reverse_field into filer_for_field's machinery, so reverse relationships now obey the filterset's FILTER_DEFAULTS/Meta.filter_overrides.

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

3 participants