Opened 3 years ago

Closed 22 months ago

#17478 closed Bug (fixed)

Overridding model formset queryset in BaseModelFormSet

Reported by: contact@… Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In https://docs.djangoproject.com/en/1.3/topics/forms/modelforms/#changing-the-queryset there are two methods described to override default queryset. Second method won't work in Django 1.3:

from django.forms.models import BaseModelFormSet

class BaseAuthorFormSet(BaseModelFormSet):
    def __init__(self, *args, **kwargs):
        super(BaseAuthorFormSet, self).__init__(*args, **kwargs)
        self.queryset = Author.objects.filter(name__startswith='O')

Calling BaseModelFormSet __init__ will call BaseFormSet's __init__ and it will in turn call self._construct_forms(). self._construct_forms use queryset we try to override but it will be to late for this when the program reach our self.queryset = Author.objects.filter(name__startswith='O') line.

BaseFormSet's _construct_forms is only called once to populate forms attribute. We can defer it a bit using properties to allow queryset to be overridden. So instead for:

    def _construct_forms(self):
        # instantiate all the forms and put them in self.forms
        self.forms = []
        for i in xrange(self.total_form_count()):
            self.forms.append(self._construct_form(i))

we can do:

    def _construct_forms(self):
        if hasattr(self, "_forms"):
            return self._forms
        # instantiate all the forms and put them in self.forms
        self._forms = []
        for i in xrange(self.total_form_count()):
            self._forms.append(self._construct_form(i))

        return self._forms

    forms = property(_construct_forms)

This change will also require _construct_forms call to be removed from BaseFormSet's __init__.

Deferring forms creation should give us some time to override queryset attribute in BaseAuthorFormSet's __init__.

Attachments (1)

17478-1.diff (3.7 KB) - added by claudep 22 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Note that the order of the two calls in __init__ was switched as a resolution of ticket:11735. It seems this was not the correct fix.

What about documenting the subclass this way:

from django.forms.models import BaseModelFormSet

class BaseAuthorFormSet(BaseModelFormSet):
    def __init__(self, *args, **kwargs):
        kwargs['queryset'] = Author.objects.filter(name__startswith='O')
        super(BaseAuthorFormSet, self).__init__(*args, **kwargs)

comment:2 Changed 3 years ago by contact@…

Please mind that queryset can also be passed as positional argument, so it can be in either args or kwargs. The correct way would be:

from django.forms.models import BaseModelFormSet

class BaseAuthorFormSet(BaseModelFormSet):
    def __init__(self, *args, **kwargs):
        queryset = Author.objects.filter(name__startswith='O')
        if len(args) > 4:
            args[4] = queryset
        else:
            kwargs['queryset'] = queryset

        super(BaseAuthorFormSet, self).__init__(*args, **kwargs)

But it's a bit ugly in my opinion. For example we rely on arguments ordering, etc.

I think my solution (moving queryset evaluation out of __init___) is better, forms attribute is read-only anyway. We should not force child classes to worry about what parent class does in its __init__ and create ugly hacks like args/kwargs modifications. Well, at least in my opinion ;)

comment:3 Changed 22 months ago by alej0

This bug is still there :(
@claudep work around works, Thanks!
Maybe a note or change in docs is enough

comment:4 Changed 22 months ago by claudep

There was a concrete proposal by the original poster, now we need someone to turn the proposal into a real patch, including tests. I might give a try...

comment:5 Changed 22 months ago by claudep

  • Has patch set
  • Version changed from 1.3 to master

The attached patch does fix the issue (hopefully), but also bring a little backwards incompatibility.

Previously, initializing a formset with an empty dict would raise a ValidationError at instanciation time. Now it is raised only when formally validating the formset (hence the TestIsBoundBehavior modification) or for any operation that triggers the forms creation. I don't think it should matter much, but I'd like to hear some other committer.

Changed 22 months ago by claudep

comment:6 Changed 22 months ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

I would have preferred to document setting kwargs.setdefault('queryset', Author.objects.filter(name__startswith='Charles')) but, as pointed out by @tomaszswiderski, this wouldn't work if queryset is passed as an arg.

I always thought it was a bit odd that a ValidationError raised on instantiation while all the other form API classes wait for validation before sanitizing data anyway.

All tests pass on Python 2.7.4/3.3.1 SQLite3. Marking as RFC.

comment:7 Changed 22 months ago by claudep

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [ef79582e8630cb3c119caed52130c9671188addd] (sorry for the missing hash in the commit message).

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