BaseModelFormSet cannot resolve instances of inherited models when saving

[Bas Peschier]

When you create a BaseModelFormSet for a Model which inherits from a user-defined Model, the save operation fails with a KeyError (key is None). This is because the primary key's attribute name is not what it expects it to be. As far as i can see the following is supposed to take place:

• creates a BaseModelFormSet, loads initials from model_to_dict for all forms from a QuerySet, including the 'id'-field
• ModelFormSet adds a hidden field via add_fields named pk.attname to each form which will contain the primary key. In normal (most?) cases, this attname is 'id'
• The form uses initial data when cleaning, outputing the pk nicely in the hidden field.
• When saving, use the hidden field to retrieve the object it refers to and save it.

The fact that the attributename of a pk for an inherited model is "<parentmodel>_ptr_id" screws things up. This can be fixed in three ways:

• Patch model_to_dict to include the actual pk.attrname: pk key/value-pair or...
• Setup the hiddenfield in add_fields of BaseModelFormSet to include a initial value
• Some clever way I did not think of yet.

I really love these formsets, they reduce coding time with large factors instead of reducing it by a few minutes :-)

[Brian Rosner]

Can you show an example of some code demonstrating this?

[Bas Peschier]

Sure, example which I'm using is a inlineformset

class Calendar(models.Model):
   # class def
class Event(models.Model):
   # class def
class Course(Calendar):
   # class def
class College(Event)
   course = models.ForeignKey(Course)
   # class def

InlineCourseFormSet = inlineformset_factory(Course, College, extra=2)
formset = InlineCourseFormSet(instance=obj) # obj is an instance of Course

when you have colleges in your database, they do not show a value within their hidden pk-field when you render it, causing the error when you load the resulting QueryDict into it and save.

[Erwin]

With the current trunk I get a 'event_ptr_id' running that example and not a 'key is None'; the hidden field is not displayed at all. The reason is that College._meta.has_auto_field returns false for a child class/object. I don't know if that is correct or not, but I fixed it by adding some code to add_fields in BaseModelFormSet.

[Brian Rosner]

(In [8528]) Fixed #7888 -- Handle model inheritance with model formsets correctly. Thanks bpeschier for the report.

[Ilya Semenov]

The code in [8528] is obviously incorrect.

Consider the following excerpt from the patch:

    for f in opts.fields + opts.many_to_many:
        # ...
        elif isinstance(f, OneToOneField):
            data[f.attname] = f.value_from_object(instance)
        else:
            data[f.name] = f.value_from_object(instance)

That code only works until there is a Model with two fields f1 and f2 where f1.attname = f2.name, which is easily achievable:

class BetterAuthor(Author):
    write_speed = models.IntegerField()
    author_ptr_id = models.CharField() # Why not, after all? I was never told that I couldn't use xxxx_ptr_id as a field name.

You see, that is semantically incorrect to ever use field.attname in the forms processing. attname should be considered as a private attribute, only visible to the ORM and low-level database query builders. In any case, one should never rely on both name and attname as a primary key, as they can (and will!) overlap.

Instead, we need to modify save_instance() or whoever processes the data prepared by model_to_dict(), to properly process OneToOneFields, even if they were prepared using f.name without any mangling.

I'm trying to resolve the adjacent problem in [8241], and my suggestion so far is:
​http://code.djangoproject.com/attachment/ticket/8241/onetoone_admin.8541.diff
If you're working on that part, please review it and combine the best of the approaches.

[Ilya Semenov]

as a primary key => as a dict key

[Brian Rosner]

Fair enough. Thanks for catching my mistake here. I must have overlooked #8241 because it really should have been taken into consideration in fixes this correctly. I will rethink this.

[Brian Rosner]

However, this ticket itself has been fixed. I will mark as such. Open a new ticket describing the missing bits here. Thanks semenov.

[Jacob]

Milestone 1.0 deleted