Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#28345 closed Bug (fixed)

limit_choices_to callable is no longer applied during ModelForm instantiation which blocks some use cases

Reported by: drunkard Owned by: Jon Dufresne
Component: Forms Version: 1.11
Severity: Release blocker Keywords: ModelForm
Cc: Jon Dufresne Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

I'm upgrading to Django 1.11.2 and found that limit_choices_to is applied to forms too early. It's caused by commit 6abd6c598ea23e0a962c87b0075aa2f79f9ead36 (ticket #27563).

I'm using dynamic limit_choices_to function to get user specific filter expr. It just called once during develop server init, and won't be call while open a USER EDIT FORM, which is inherited from ModelForm, but it work in django admin edit page, by reading the code, django admin build form using modelform_factory(), with this commit reverted, my customized form works just fine. So, I think it's a regression.

Below is key part of test project showing the problem:

proj/middleware.py

from threading import current_thread


class CUserMiddleware(object):
    """Get current operating request and user"""
    _user = {}

    def process_request(self, request):
        self._user[current_thread()] = request.user

    def process_response(self, request, response):
        # TODO cleanup by alive session, if user session expired, remove all
        # items related to the user (dict value).
        # self._user.pop(current_thread(), None)  # clean to avoid it grows
        if len(self._user) > 1000:
            self._user = {}
        return response

    @classmethod
    def get_user(cls, default=None):
        # debug('当前用户: {}'.format(cls._user.get(current_thread(), default)))
        return cls._user.get(current_thread(), default)


def current_user():
    return CUserMiddleware.get_user()

testapp/models.py

from django.db import models
from django.db.models import Q

from proj.middleware import current_user


def limit_export_branch():
    u = current_user()
    if u:
        print('\tlimit_export_branch worked the right way')
        return Q(name__endswith='something')
    print('\tlimit_export_branch works in wrong way')
    import traceback
    traceback.print_stack()
    return Q(pk=-1)  # deny to avoid info leak


class Branch(models.Model):
    name = models.CharField(max_length=16)

    def __str__(self):
        return self.name


class Export(models.Model):
    name = models.CharField(max_length=16)
    branch = models.ForeignKey(Branch, limit_choices_to=limit_export_branch)

    def __str__(self):
        return self.name

testapp/forms.py

from django import forms
from .models import Export


class ExportEditForm(forms.ModelForm):
    class Meta:
        model = Export
        fields = ['name', 'branch']

testapp/views.py

from django.views.generic import UpdateView

from .forms import ExportEditForm
from .models import Export


class ExportEditView(UpdateView):
    form_class = ExportEditForm
    model = Export
    template_name = 'edit.html'
    pk_url_kwarg = 'export_pk'

and the ugly template: testapp/templates/edit.html

{{ form.as_table }}

Attachments (1)

limit_choices_to-bug.tgz (14.5 KB) - added by drunkard 3 years ago.
Simple test project of this bug

Download all attachments as: .zip

Change History (7)

Changed 3 years ago by drunkard

Attachment: limit_choices_to-bug.tgz added

Simple test project of this bug

comment:1 Changed 3 years ago by Tim Graham

Cc: Jon Dufresne added
Description: modified (diff)
Severity: NormalRelease blocker
Summary: #27563 caused regression to ModelFormlimit_choices_to callable is no longer applied during ModelForm instantiation which blocks some use cases
Triage Stage: UnreviewedAccepted

Without much investigation, I'm not sure if we should revert the original change or if some other workaround might restore allowing this use case.

comment:2 Changed 3 years ago by Jon Dufresne

Owner: changed from nobody to Jon Dufresne
Status: newassigned

I agree, this is a regression. The docs say:

If a callable is used for limit_choices_to, it will be invoked every time a new form is instantiated.

I have a fix in mind that will restore previous, correct behavior will also facilitating my use case motivating the original change.

comment:3 Changed 3 years ago by Jon Dufresne

PR

The change adds an additional keyword argument, apply_limit_choices_to, to field_for_model(). This arugment allows field_for_model() to continue to be used to create form fields dynamically after ModelForm.__init__() is called.

comment:4 Changed 3 years ago by Jon Dufresne

Has patch: set

comment:5 Changed 3 years ago by Jon Dufresne <jon.dufresne@…>

Resolution: fixed
Status: assignedclosed

In a1be12fe:

Fixed #28345 -- Applied limit_choices_to during ModelForm.init().

field_for_model() now has an additional keyword argument,
apply_limit_choices_to, allowing it to continue to be used to create
form fields dynamically after ModelForm.init() is called.

Thanks Tim Graham for the review.

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

In 8641489:

[1.11.x] Fixed #28345 -- Applied limit_choices_to during ModelForm.init().

field_for_model() now has an additional keyword argument,
apply_limit_choices_to, allowing it to continue to be used to create
form fields dynamically after ModelForm.init() is called.

Thanks Tim Graham for the review.

Backport of a1be12fe193c8f3de8a0b0820f460a302472375f from master

Note: See TracTickets for help on using tickets.
Back to Top