Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#8241 closed (fixed)

Primary ForeignKeys don't work with FormSets

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

Description

Using this UserProfile as an example:

class UserProfile(models.Model):
    user = models.OneToOneField(primary_key=True)
    nickname = models.CharField(max_length=30)

from django.contrib.auth.models import User
from django.contrib.auth.admin import UserAdmin
from django.contrib.admin import site, StackedInline

class UserProfileAdmin(StackedInline)
    model = UserProfile
    max_num = 1

UserAdmin.inlines = UserAdmin.inlines + [UserProfileAdmin]

site.unregister(User)
site.register(User, UserAdmin)

Trying to save a User from the admin causes a KeyError: 'user_id' because the primary key doesn't get rendered in the form.

Attachments (3)

8241.patch (2.2 KB) - added by sciyoshi 6 years ago.
onetoone_admin.8461.diff (2.7 KB) - added by semenov 6 years ago.
onetoone_admin.8541.diff (3.2 KB) - added by semenov 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by sciyoshi

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

That should be

user = models.OneToOneField(User, primary_key=True)

comment:2 Changed 6 years ago by jacob

  • Component changed from Uncategorized to Admin interface
  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:3 follow-up: Changed 6 years ago by mremolt

Isn't this a duplicate for or at least related to #7947 ?

comment:4 in reply to: ↑ 3 Changed 6 years ago by sciyoshi

Replying to mremolt:

Isn't this a duplicate for or at least related to #7947 ?

I don't think so; applying the patch from that ticket doesn't fix the bug, and the same behavior also occurs if we have

user = models.ForeignKey(User, primary_key=True)

I think the problem could be fixed somewhere around add_fields in BaseModelFormSet:

    def add_fields(self, form, index):
        """Add a hidden field for the object's primary key."""
        if self.model._meta.has_auto_field:
            self._pk_field_name = self.model._meta.pk.attname
            form.fields[self._pk_field_name] = IntegerField(required=False, widget=HiddenInput)
        super(BaseModelFormSet, self).add_fields(form, index)

If the primary key isn't an AutoField it doesn't get added to the fields here...

comment:5 Changed 6 years ago by sciyoshi

The following patch fixes it for me (for OneToOneFields the patch from #7949 needs to be applied first), but probably breaks other things; namely, model_formsets fails. If anybody has ideas on how to better implement this let me know...

Changed 6 years ago by sciyoshi

comment:6 Changed 6 years ago by sciyoshi

  • Has patch set
  • Needs tests set
  • Patch needs improvement set

comment:7 Changed 6 years ago by semenov

  • Patch needs improvement unset

sciyoshi, first of all, thanks for your patch. It inspired me for further analysis of the problem. :)

From what I've discovered, your patch suffers from the following:

  1. It curcumvents into the naming convention of form fields and adds a form field named user_id (i.e., taken as field.attname, not as field.name). At first glance, that seems to be ok: the field is filled by model_to_dict(), passed thru browser POST, but then ignored by save_instance(). Alas, that would break things if a model had TWO fields like user=OneToOneField() and user_id=CharField(). We shouldn't be messing with the naming convention: all form fields should always use field.name as their names.
  1. The patch breaks BaseModelFormSet.add_fields() contract, which currently adds a new field if (and only if) a primary key is an AutoField -- what I consider to be a desired behaviour.

I'm attaching the updated patch (vs [8461]), which resolves the both problems.

Changed 6 years ago by semenov

comment:8 Changed 6 years ago by semenov

Also, I didn't get the idea behind parent_link, which was in your patch, and also was discussed in #7947. I replaced that with:

if not kwargs.get('primary_key', False): 
    kwargs['editable'] = False 

which I admit might be wrong for some obscure reason...

comment:9 Changed 6 years ago by semenov

If anybody cares, I'm attaching the updated patch against [8541], since [8528] broke things again (but in a different way).

Changed 6 years ago by semenov

comment:10 Changed 6 years ago by semenov

Note: I really don't like my drill down until we find a "real" pk approach; but I don't have enough deep knowledge of django/db internals, so that was the only way that seemed correct to me (more or less).

comment:11 Changed 6 years ago by Uninen

  • Cc ville@… added

comment:12 Changed 6 years ago by brosner

  • Resolution set to invalid
  • Status changed from new to closed

It appears this ticket has been caught in the middle of some commit rapid fire. In the fire the issue reported here is fixed. The issue that semenov brings up is not related, but I think should be fixed. Can you please open a ticket talking about the namespace stomping. Please reopen if the issue reported has not been fixed on trunk with detailed and concise use cases.

comment:13 Changed 6 years ago by brosner

  • Resolution invalid deleted
  • Status changed from closed to reopened

Ok. I take back that this is fixed. I had a bad test case. Reopening.

comment:14 Changed 6 years ago by brosner

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

comment:15 Changed 6 years ago by brosner

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

Fixed in [8756].

comment:16 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.