ChoiceFilter: don't call callable choices immediately

[dmytrokyrychuk]

When choices is a callable and it fetches data from DB, an error will be raised during first migration (relation does not exist for postgres) if filterset is imported when running manage.py, because ChoiceFilter will run choices at the time of importing the file in which the filterset is defined, which may happen before any tables are created. This may also be the case when choices callable references fields that will be created when later migrations are applied, but are not available at the moment, but I haven't tested this, so I can't be sure.

Django itself wraps ChoiceField choices in an iterator, presumably to avoid evaluating choices at the moment the field is instantiated, which would help to prevent the error I am describing. See https://github.com/django/django/blob/8714403614c4dfa37e806db4a6708b8a91a827a4/django/forms/fields.py#L838
Would anyone be interested in a PR that employs a similar approach to ChoiceFilter?

[carltongibson]

My first inclination it to think you're imports are badly structured. You should be able to run your migrations without importing your filtersets. (Happy to see why that inclination is wrong...)

[dmytrokyrychuk]

Django loads urls config when running manage.py -> urls.py: from app.views import SomeView -> views.py: from app.filters import SomeFilter. Are these imports considered badly structured?

[carltongibson]

No. That seems fine. But I've not seen this particular problem, so I'm wondering why you are.

(You're evaluating a fixed set of choices at import time... — that can't be right — I add a new item, my choices change — they MUST be evaluated per instantiation... — your filter set definition can't be right.)

[dmytrokyrychuk]

I do not intend to evaluate the choices at import time, this is exactly why I made a separate function that generates a list of choices:

def get_choices():
    return [(...) for x in SomeModel.objects.filter(...).order_by(...)]

class SomeFilterSet(django_filters.FilterSet):
    field = django_filters.ChoiceField(
        choices=get_choices,  # not calling it here
    )
    class Meta:
        ...

I'd expect choices to be evaluated when the filterset instance is created, or when filter itself is called, but instead it gets evaluated on filter definition, right there in __init__:

django-filter/django_filters/filters.py

Line 199 in 7e94907

choices = choices()

I haven't noticed this issue myself, until I moved a project to production server with an empty DB. get_choices gets evaluated before I even run the first migration, so there isn't a table for SomeModel in the DB yet.

[carltongibson]

Right, OK, I see. Yep, happy to consider solutions. 👍

— I guess the initial idea here wasn't considering you'd evaluate a QuerySet in your callable. (This being introduced as an alternative to a QuerySet.) But yes, evaluation should probably be deferred.

[dmytrokyrychuk]

This worked for me:

import django_filters
from django.forms.fields import CallableChoiceIterator
from django_filters.conf import settings as django_filters_settingss

class DeferredChoiceFilter(django_filters.ChoiceFilter):
    def __init__(self, *args, **kwargs):
        empty_label = kwargs.pop(
            'empty_label', django_filters_settingss.EMPTY_CHOICE_LABEL)
        null_label = kwargs.pop(
            'null_label', django_filters_settingss.NULL_CHOICE_LABEL)
        null_value = kwargs.pop(
            'null_value', django_filters_settingss.NULL_CHOICE_VALUE)

        self.null_value = null_value

        if 'choices' in kwargs:
            choices = kwargs.get('choices')

            # coerce choices to list
            if callable(choices):
                choices = CallableChoiceIterator(choices)
            else:
                choices = list(choices)

            # create the empty/null choices that prepend the original choices
            prepend = []
            if empty_label is not None:
                prepend.append(('', empty_label))
            if null_label is not None:
                prepend.append((null_value, null_label))

            kwargs['choices'] = chain(prepend, choices)

        # Skipping ChoiceFilter init
        django_filters.Filter.__init__(self, *args, **kwargs)

CallableChoiceIterator is not a part of Django's public API, so I'm not sure if it's safe to use it here. It was ok for me right now, because my project uses a specific Django version that is know to have CallableChoiceIterator.

Any thoughts?

[carltongibson]

Well the obvious one is just, why not return an generator or such from get_choices? (We'd at least add a note to the docs for that.)

Other than that, "Not sure, right now".

I'd need to have a look into CallableChoiceIterator to begin.

[dmytrokyrychuk]

Well the obvious one is just, why not return an generator or such from get_choices? (We'd at least add a note to the docs for that.)

Because of

django-filter/django_filters/filters.py

Line 200 in 7e94907

choices = list(choices)

It gets evaluated there too.

[carltongibson]

Fine. Yes. Of course it does.

OK. Your use-case seems legitimate. I'd take a PR on this. The starting point for that it a failing test case.

I'll take a look at CallableChoiceIterator but first guess would be we'll go with that. (The test matrix will give us some idea as to its stability.)

[dmytrokyrychuk]

OK, I'll start working on a PR when I have more time, probably on the next weekend.

I'll take a look at CallableChoiceIterator but first guess would be we'll go with that. (The test matrix will give us some idea as to its stability.)

Since it's not a part of Django's public API, it might be reasonable to include a version of CallableChoiceIterator into django_filter package itself.

[rpkilby]

By the way, I believe this is a duplicate of #710. @orgkhnargh - could you install the branch from PR 680 and see if that fixes the issue?

[carltongibson]

Yep. #680 looks good for this. @orgkhnargh care to give it a run? 👍

[dmytrokyrychuk]

#680 does make relation does not exist error go away. Closing this issue.