Opened 16 years ago

Closed 16 years ago

Last modified 13 years ago

#7888 closed (fixed)

BaseModelFormSet cannot resolve instances of inherited models when saving

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

Description

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 16 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Brian Rosner, 16 years ago

milestone: 1.0 beta
Triage Stage: UnreviewedAccepted

comment:2 by Brian Rosner, 16 years ago

Owner: changed from nobody to Brian Rosner
Status: newassigned

comment:3 by Brian Rosner, 16 years ago

Can you show an example of some code demonstrating this?

comment:4 by Bas Peschier, 16 years ago

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 by Brian Rosner, 16 years ago

milestone: 1.0 beta1.0

comment:6 by Erwin, 16 years ago

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.

by Erwin, 16 years ago

Attachment: 7888.diff added

comment:7 by Erwin, 16 years ago

Has patch: set

comment:8 by Ville Säävuori, 16 years ago

Cc: ville@… added

comment:9 by Brian Rosner, 16 years ago

Resolution: fixed
Status: assignedclosed

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

comment:10 by Ilya Semenov, 16 years ago

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

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.

comment:11 by Ilya Semenov, 16 years ago

as a primary key => as a dict key

comment:12 by Brian Rosner, 16 years ago

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 by Brian Rosner, 16 years ago

Resolution: fixed
Status: reopenedclosed

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 by Jacob, 13 years ago

milestone: 1.0

Milestone 1.0 deleted

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