Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#7888 closed (fixed)

BaseModelFormSet cannot resolve instances of inherited models when saving

Reported by: bpeschier Owned by: brosner
Component: Forms Version: master
Severity: Keywords: modelformset, inheritance
Cc: ville@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:


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 :-)

Attachments (1)

7888.diff (821 bytes) - added by Erwin 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by brosner

  • milestone set to 1.0 beta
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by brosner

  • Owner changed from nobody to brosner
  • Status changed from new to assigned

comment:3 Changed 7 years ago by brosner

Can you show an example of some code demonstrating this?

comment:4 Changed 7 years ago by bpeschier

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.

comment:5 Changed 7 years ago by brosner

  • milestone changed from 1.0 beta to 1.0

comment:6 Changed 7 years ago by 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.

Changed 7 years ago by Erwin

comment:7 Changed 7 years ago by Erwin

  • Has patch set

comment:8 Changed 7 years ago by Uninen

  • Cc ville@… added

comment:9 Changed 7 years ago by brosner

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

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

comment:10 Changed 7 years ago by semenov

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

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)
            data[] = f.value_from_object(instance)

That code only works until there is a Model with two fields f1 and f2 where f1.attname =, 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 without any mangling.

I'm trying to resolve the adjacent problem in [8241], and my suggestion so far is:
If you're working on that part, please review it and combine the best of the approaches.

comment:11 Changed 7 years ago by semenov

as a primary key => as a dict key

comment:12 Changed 7 years ago by brosner

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.

comment:13 Changed 7 years ago by brosner

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

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

comment:14 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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