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

2 files changed, 100 insertions, 57 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):