Fixed #9284. Fixed #8813. BaseModelFormSet now calls ModelForm.save().

This is backwards-incompatible if you were doing things to 'initial' in BaseModelFormSet.__init__, or if you relied on the internal _total_form_count or _initial_form_count attributes of BaseFormSet. Those attributes are now public methods.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@10190 bcc190cf-cafb-0310-a4f2-bffc1f526a37

4 files changed, 203 insertions, 73 deletions

diff --git a/django/forms/formsets.py b/django/forms/formsets.py
index 7a5cb8a439..955b41a437 100644
--- a/django/forms/formsets.py
+++ b/django/forms/formsets.py
@@ -40,39 +40,51 @@ class BaseFormSet(StrAndUnicode):
         self.error_class = error_class
         self._errors = None
         self._non_form_errors = None
-        # initialization is different depending on whether we recieved data, initial, or nothing
-        if data or files:
-            self.management_form = ManagementForm(data, auto_id=self.auto_id, prefix=self.prefix)
-            if self.management_form.is_valid():
-                self._total_form_count = self.management_form.cleaned_data[TOTAL_FORM_COUNT]
-                self._initial_form_count = self.management_form.cleaned_data[INITIAL_FORM_COUNT]
-            else:
-                raise ValidationError('ManagementForm data is missing or has been tampered with')
-        else:
-            if initial:
-                self._initial_form_count = len(initial)
-                if self._initial_form_count > self.max_num and self.max_num > 0:
-                    self._initial_form_count = self.max_num
-                self._total_form_count = self._initial_form_count + self.extra
-            else:
-                self._initial_form_count = 0
-                self._total_form_count = self.extra
-            if self._total_form_count > self.max_num and self.max_num > 0:
-                self._total_form_count = self.max_num
-            initial = {TOTAL_FORM_COUNT: self._total_form_count,
-                       INITIAL_FORM_COUNT: self._initial_form_count}
-            self.management_form = ManagementForm(initial=initial, auto_id=self.auto_id, prefix=self.prefix)
-
         # construct the forms in the formset
         self._construct_forms()
 
     def __unicode__(self):
         return self.as_table()
 
+    def _management_form(self):
+        """Returns the ManagementForm instance for this FormSet."""
+        if self.data or self.files:
+            form = ManagementForm(self.data, auto_id=self.auto_id, prefix=self.prefix)
+            if not form.is_valid():
+                raise ValidationError('ManagementForm data is missing or has been tampered with')
+        else:
+            form = ManagementForm(auto_id=self.auto_id, prefix=self.prefix, initial={
+                TOTAL_FORM_COUNT: self.total_form_count(),
+                INITIAL_FORM_COUNT: self.initial_form_count()
+            })
+        return form
+    management_form = property(_management_form)
+
+    def total_form_count(self):
+        """Returns the total number of forms in this FormSet."""
+        if self.data or self.files:
+            return self.management_form.cleaned_data[TOTAL_FORM_COUNT]
+        else:
+            total_forms = self.initial_form_count() + self.extra
+            if total_forms > self.max_num > 0:
+                total_forms = self.max_num
+        return total_forms
+
+    def initial_form_count(self):
+        """Returns the number of forms that are required in this FormSet."""
+        if self.data or self.files:
+            return self.management_form.cleaned_data[INITIAL_FORM_COUNT]
+        else:
+            # Use the length of the inital data if it's there, 0 otherwise.
+            initial_forms = self.initial and len(self.initial) or 0
+            if initial_forms > self.max_num > 0:
+                initial_forms = self.max_num
+        return initial_forms
+
     def _construct_forms(self):
         # instantiate all the forms and put them in self.forms
         self.forms = []
-        for i in xrange(self._total_form_count):
+        for i in xrange(self.total_form_count()):
             self.forms.append(self._construct_form(i))
 
     def _construct_form(self, i, **kwargs):
@@ -89,7 +101,7 @@ class BaseFormSet(StrAndUnicode):
             except IndexError:
                 pass
         # Allow extra forms to be empty.
-        if i >= self._initial_form_count:
+        if i >= self.initial_form_count():
             defaults['empty_permitted'] = True
         defaults.update(kwargs)
         form = self.form(**defaults)
@@ -97,13 +109,13 @@ class BaseFormSet(StrAndUnicode):
         return form
 
     def _get_initial_forms(self):
-        """Return a list of all the intial forms in this formset."""
-        return self.forms[:self._initial_form_count]
+        """Return a list of all the initial forms in this formset."""
+        return self.forms[:self.initial_form_count()]
     initial_forms = property(_get_initial_forms)
 
     def _get_extra_forms(self):
         """Return a list of all the extra forms in this formset."""
-        return self.forms[self._initial_form_count:]
+        return self.forms[self.initial_form_count():]
     extra_forms = property(_get_extra_forms)
 
     # Maybe this should just go away?
@@ -127,10 +139,10 @@ class BaseFormSet(StrAndUnicode):
         # that have had their deletion widget set to True
         if not hasattr(self, '_deleted_form_indexes'):
             self._deleted_form_indexes = []
-            for i in range(0, self._total_form_count):
+            for i in range(0, self.total_form_count()):
                 form = self.forms[i]
                 # if this is an extra form and hasn't changed, don't consider it
-                if i >= self._initial_form_count and not form.has_changed():
+                if i >= self.initial_form_count() and not form.has_changed():
                     continue
                 if form.cleaned_data[DELETION_FIELD_NAME]:
                     self._deleted_form_indexes.append(i)
@@ -150,10 +162,10 @@ class BaseFormSet(StrAndUnicode):
         # by the form data.
         if not hasattr(self, '_ordering'):
             self._ordering = []
-            for i in range(0, self._total_form_count):
+            for i in range(0, self.total_form_count()):
                 form = self.forms[i]
                 # if this is an extra form and hasn't changed, don't consider it
-                if i >= self._initial_form_count and not form.has_changed():
+                if i >= self.initial_form_count() and not form.has_changed():
                     continue
                 # don't add data marked for deletion to self.ordered_data
                 if self.can_delete and form.cleaned_data[DELETION_FIELD_NAME]:
@@ -221,7 +233,7 @@ class BaseFormSet(StrAndUnicode):
         self._errors = []
         if not self.is_bound: # Stop further processing.
             return
-        for i in range(0, self._total_form_count):
+        for i in range(0, self.total_form_count()):
             form = self.forms[i]
             self._errors.append(form.errors)
         # Give self.clean() a chance to do cross-form validation.
@@ -243,7 +255,7 @@ class BaseFormSet(StrAndUnicode):
         """A hook for adding extra fields on to each form instance."""
         if self.can_order:
             # Only pre-fill the ordering field for initial forms.
-            if index < self._initial_form_count:
+            if index < self.initial_form_count():
                 form.fields[ORDERING_FIELD_NAME] = IntegerField(label=_(u'Order'), initial=index+1, required=False)
             else:
                 form.fields[ORDERING_FIELD_NAME] = IntegerField(label=_(u'Order'), required=False)
diff --git a/django/forms/models.py b/django/forms/models.py
index d62a2ce713..4b697a8233 100644
--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -54,6 +54,10 @@ def save_instance(form, instance, fields=None, fail_message='saved',
         # callable upload_to can use the values from other fields.
         if isinstance(f, models.FileField):
             file_field_list.append(f)
+        # OneToOneField doesn't allow assignment of None. Guard against that
+        # instead of allowing it and throwing an error.
+        if isinstance(f, models.OneToOneField) and cleaned_data[f.name] is None:
+            pass
         else:
             f.save_form_data(instance, cleaned_data[f.name])
 
@@ -266,7 +270,13 @@ class BaseModelForm(BaseForm):
 
             lookup_kwargs = {}
             for field_name in unique_check:
-                lookup_kwargs[field_name] = self.cleaned_data[field_name]
+                lookup_value = self.cleaned_data[field_name]
+                # ModelChoiceField will return an object instance rather than
+                # a raw primary key value, so convert it to a pk value before
+                # using it in a lookup.
+                if isinstance(self.fields[field_name], ModelChoiceField):
+                    lookup_value =  lookup_value.pk
+                lookup_kwargs[field_name] = lookup_value
 
             qs = self.instance.__class__._default_manager.filter(**lookup_kwargs)
 
@@ -357,12 +367,17 @@ class BaseModelFormSet(BaseFormSet):
                  queryset=None, **kwargs):
         self.queryset = queryset
         defaults = {'data': data, 'files': files, 'auto_id': auto_id, 'prefix': prefix}
-        defaults['initial'] = [model_to_dict(obj) for obj in self.get_queryset()]
         defaults.update(kwargs)
         super(BaseModelFormSet, self).__init__(**defaults)
 
+    def initial_form_count(self):
+        """Returns the number of forms that are required in this FormSet."""
+        if not (self.data or self.files):
+            return len(self.get_queryset())
+        return super(BaseModelFormSet, self).initial_form_count()
+
     def _construct_form(self, i, **kwargs):
-        if i < self._initial_form_count:
+        if i < self.initial_form_count():
             kwargs['instance'] = self.get_queryset()[i]
         return super(BaseModelFormSet, self)._construct_form(i, **kwargs)
 
@@ -380,11 +395,11 @@ class BaseModelFormSet(BaseFormSet):
 
     def save_new(self, form, commit=True):
         """Saves and returns a new model instance for the given form."""
-        return save_instance(form, self.model(), exclude=[self._pk_field.name], commit=commit)
+        return form.save(commit=commit)
 
     def save_existing(self, form, instance, commit=True):
         """Saves and returns an existing model instance for the given form."""
-        return save_instance(form, instance, exclude=[self._pk_field.name], commit=commit)
+        return form.save(commit=commit)
 
     def save(self, commit=True):
         """Saves model instances for every form, adding and changing instances
@@ -410,7 +425,7 @@ class BaseModelFormSet(BaseFormSet):
             existing_objects[obj.pk] = obj
         saved_instances = []
         for form in self.initial_forms:
-            obj = existing_objects[form.cleaned_data[self._pk_field.name]]
+            obj = existing_objects[form.cleaned_data[self._pk_field.name].pk]
             if self.can_delete and form.cleaned_data[DELETION_FIELD_NAME]:
                 self.deleted_objects.append(obj)
                 obj.delete()
@@ -438,10 +453,23 @@ class BaseModelFormSet(BaseFormSet):
 
     def add_fields(self, form, index):
         """Add a hidden field for the object's primary key."""
-        from django.db.models import AutoField
+        from django.db.models import AutoField, OneToOneField, ForeignKey
         self._pk_field = pk = self.model._meta.pk
-        if pk.auto_created or isinstance(pk, AutoField):
-            form.fields[self._pk_field.name] = IntegerField(required=False, widget=HiddenInput)
+        # If a pk isn't editable, then it won't be on the form, so we need to
+        # add it here so we can tell which object is which when we get the
+        # data back. Generally, pk.editable should be false, but for some
+        # reason, auto_created pk fields and AutoField's editable attribute is
+        # True, so check for that as well.
+        if (not pk.editable) or (pk.auto_created or isinstance(pk, AutoField)):
+            try:
+                pk_value = self.get_queryset()[index].pk
+            except IndexError:
+                pk_value = None
+            if isinstance(pk, OneToOneField) or isinstance(pk, ForeignKey):
+                qs = pk.rel.to._default_manager.get_query_set()
+            else:
+                qs = self.model._default_manager.get_query_set()
+            form.fields[self._pk_field.name] = ModelChoiceField(qs, initial=pk_value, required=False, widget=HiddenInput)
         super(BaseModelFormSet, self).add_fields(form, index)
 
 def modelformset_factory(model, form=ModelForm, formfield_callback=lambda f: f.formfield(),
@@ -477,11 +505,15 @@ class BaseInlineFormSet(BaseModelFormSet):
         super(BaseInlineFormSet, self).__init__(data, files, prefix=prefix,
                                                 queryset=qs)
 
-    def _construct_forms(self):
+    def initial_form_count(self):
         if self.save_as_new:
-            self._total_form_count = self._initial_form_count
-            self._initial_form_count = 0
-        super(BaseInlineFormSet, self)._construct_forms()
+            return 0
+        return super(BaseInlineFormSet, self).initial_form_count()
+
+    def total_form_count(self):
+        if self.save_as_new:
+            return super(BaseInlineFormSet, self).initial_form_count()
+        return super(BaseInlineFormSet, self).total_form_count()
 
     def _construct_form(self, i, **kwargs):
         form = super(BaseInlineFormSet, self)._construct_form(i, **kwargs)
@@ -498,14 +530,15 @@ class BaseInlineFormSet(BaseModelFormSet):
     get_default_prefix = classmethod(get_default_prefix)
 
     def save_new(self, form, commit=True):
-        fk_attname = self.fk.get_attname()
-        kwargs = {fk_attname: self.instance.pk}
-        new_obj = self.model(**kwargs)
-        if fk_attname == self._pk_field.attname or self._pk_field.auto_created:
-            exclude =  [self._pk_field.name]
-        else:
-            exclude = []
-        return save_instance(form, new_obj, exclude=exclude, commit=commit)
+        # Use commit=False so we can assign the parent key afterwards, then
+        # save the object.
+        obj = form.save(commit=False)
+        setattr(obj, self.fk.get_attname(), self.instance.pk)
+        obj.save()
+        # form.save_m2m() can be called via the formset later on if commit=False
+        if commit and hasattr(form, 'save_m2m'):
+            form.save_m2m()
+        return obj
 
     def add_fields(self, form, index):
         super(BaseInlineFormSet, self).add_fields(form, index)
@@ -620,8 +653,6 @@ class InlineForeignKeyField(Field):
         # ensure the we compare the values as equal types.
         if force_unicode(value) != force_unicode(self.parent_instance.pk):
             raise ValidationError(self.error_messages['invalid_choice'])
-        if self.pk_field:
-            return self.parent_instance.pk
         return self.parent_instance
 
 class ModelChoiceIterator(object):
diff --git a/docs/topics/forms/formsets.txt b/docs/topics/forms/formsets.txt
index c72b0f3c63..45b03029d3 100644
--- a/docs/topics/forms/formsets.txt
+++ b/docs/topics/forms/formsets.txt
@@ -133,6 +133,20 @@ It is used to keep track of how many form instances are being displayed. If
 you are adding new forms via JavaScript, you should increment the count fields
 in this form as well.
 
+.. versionadded:: 1.1
+
+``total_form_count`` and ``initial_form_count``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+``BaseModelFormSet`` has a couple of methods that are closely related to the
+``ManagementForm``, ``total_form_count`` and ``initial_form_count``.
+
+``total_form_count`` returns the total number of forms in this formset.
+``initial_form_count`` returns the number of forms in the formset that were
+pre-filled, and is also used to determine how many forms are required. You
+will probably never need to override either of these methods, so please be
+sure you understand what they do before doing so.
+
 Custom formset validation
 ~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/tests/modeltests/model_formsets/models.py b/tests/modeltests/model_formsets/models.py
index 0503ea2a84..d8cbe34b94 100644
--- a/tests/modeltests/model_formsets/models.py
+++ b/tests/modeltests/model_formsets/models.py
@@ -1,6 +1,4 @@
-
 import datetime
-
 from django import forms
 from django.db import models
 
@@ -27,7 +25,7 @@ class Book(models.Model):
 
     def __unicode__(self):
         return self.title
-    
+
 class BookWithCustomPK(models.Model):
     my_pk = models.DecimalField(max_digits=5, decimal_places=0, primary_key=True)
     author = models.ForeignKey(Author)
@@ -35,13 +33,13 @@ class BookWithCustomPK(models.Model):
 
     def __unicode__(self):
         return u'%s: %s' % (self.my_pk, self.title)
-    
+
 class AlternateBook(Book):
     notes = models.CharField(max_length=100)
-    
+
     def __unicode__(self):
         return u'%s - %s' % (self.title, self.notes)
-    
+
 class AuthorMeeting(models.Model):
     name = models.CharField(max_length=100)
     authors = models.ManyToManyField(Author)
@@ -68,7 +66,7 @@ class Owner(models.Model):
     auto_id = models.AutoField(primary_key=True)
     name = models.CharField(max_length=100)
     place = models.ForeignKey(Place)
-    
+
     def __unicode__(self):
         return "%s at %s" % (self.name, self.place)
 
@@ -81,7 +79,7 @@ class Location(models.Model):
 class OwnerProfile(models.Model):
     owner = models.OneToOneField(Owner, primary_key=True)
     age = models.PositiveIntegerField()
-    
+
     def __unicode__(self):
         return "%s is %d" % (self.owner.name, self.age)
 
@@ -114,17 +112,17 @@ class MexicanRestaurant(Restaurant):
 # using inlineformset_factory.
 class Repository(models.Model):
     name = models.CharField(max_length=25)
-    
+
     def __unicode__(self):
         return self.name
 
 class Revision(models.Model):
     repository = models.ForeignKey(Repository)
     revision = models.CharField(max_length=40)
-    
+
     class Meta:
         unique_together = (("repository", "revision"),)
-    
+
     def __unicode__(self):
         return u"%s (%s)" % (self.revision, unicode(self.repository))
 
@@ -146,7 +144,21 @@ class Team(models.Model):
 class Player(models.Model):
     team = models.ForeignKey(Team, null=True)
     name = models.CharField(max_length=100)
-    
+
+    def __unicode__(self):
+        return self.name
+
+# Models for testing custom ModelForm save methods in formsets and inline formsets
+class Poet(models.Model):
+    name = models.CharField(max_length=100)
+
+    def __unicode__(self):
+        return self.name
+
+class Poem(models.Model):
+    poet = models.ForeignKey(Poet)
+    name = models.CharField(max_length=100)
+
     def __unicode__(self):
         return self.name
 
@@ -337,13 +349,44 @@ used.
 
 >>> AuthorFormSet = modelformset_factory(Author, max_num=2)
 >>> formset = AuthorFormSet(queryset=qs)
->>> [sorted(x.items()) for x in formset.initial]
-[[('id', 1), ('name', u'Charles Baudelaire')], [('id', 3), ('name', u'Paul Verlaine')]]
+>>> [x.name for x in formset.get_queryset()]
+[u'Charles Baudelaire', u'Paul Verlaine']
 
 >>> AuthorFormSet = modelformset_factory(Author, max_num=3)
 >>> formset = AuthorFormSet(queryset=qs)
->>> [sorted(x.items()) for x in formset.initial]
-[[('id', 1), ('name', u'Charles Baudelaire')], [('id', 3), ('name', u'Paul Verlaine')], [('id', 2), ('name', u'Walt Whitman')]]
+>>> [x.name for x in formset.get_queryset()]
+[u'Charles Baudelaire', u'Paul Verlaine', u'Walt Whitman']
+
+
+# ModelForm with a custom save method in a formset ###########################
+
+>>> class PoetForm(forms.ModelForm):
+...     def save(self, commit=True):
+...         # change the name to "Vladimir Mayakovsky" just to be a jerk.
+...         author = super(PoetForm, self).save(commit=False)
+...         author.name = u"Vladimir Mayakovsky"
+...         if commit:
+...             author.save()
+...         return author
+
+>>> PoetFormSet = modelformset_factory(Poet, form=PoetForm)
+
+>>> data = {
+...     'form-TOTAL_FORMS': '3', # the number of forms rendered
+...     'form-INITIAL_FORMS': '0', # the number of forms with initial data
+...     'form-0-name': 'Walt Whitman',
+...     'form-1-name': 'Charles Baudelaire',
+...     'form-2-name': '',
+... }
+
+>>> qs = Poet.objects.all()
+>>> formset = PoetFormSet(data=data, queryset=qs)
+>>> formset.is_valid()
+True
+
+>>> formset.save()
+[<Poet: Vladimir Mayakovsky>, <Poet: Vladimir Mayakovsky>]
+
 
 # Model inheritance in model formsets ########################################
 
@@ -553,6 +596,36 @@ True
 [<AlternateBook: Flowers of Evil - English translation of Les Fleurs du Mal>]
 
 
+# ModelForm with a custom save method in an inline formset ###################
+
+>>> class PoemForm(forms.ModelForm):
+...     def save(self, commit=True):
+...         # change the name to "Brooklyn Bridge" just to be a jerk.
+...         poem = super(PoemForm, self).save(commit=False)
+...         poem.name = u"Brooklyn Bridge"
+...         if commit:
+...             poem.save()
+...         return poem
+
+>>> PoemFormSet = inlineformset_factory(Poet, Poem, form=PoemForm)
+
+>>> data = {
+...     'poem_set-TOTAL_FORMS': '3', # the number of forms rendered
+...     'poem_set-INITIAL_FORMS': '0', # the number of forms with initial data
+...     'poem_set-0-name': 'The Cloud in Trousers',
+...     'poem_set-1-name': 'I',
+...     'poem_set-2-name': '',
+... }
+
+>>> poet = Poet.objects.create(name='Vladimir Mayakovsky')
+>>> formset = PoemFormSet(data=data, instance=poet)
+>>> formset.is_valid()
+True
+
+>>> formset.save()
+[<Poem: Brooklyn Bridge>, <Poem: Brooklyn Bridge>]
+
+
 # Test a custom primary key ###################################################
 
 We need to ensure that it is displayed