MultiValueDictKeyError when saving Inlines without an AutoField

[nickname123]

Saving an Inline that makes use of an autogenerated primary key that is not an AutoField causes a MultiValueDictKeyError.

Here is a gist with an admin.py and model.py to reproduce the issue: ​https://gist.github.com/jacobian/5017514

Steps:
create Author
save
add a Book to Author
save
add another Book to Author
save --> MultiValueDictKeyError

Here is an IRC chat log about the issue:

[18:22] <gp> I posted a message to the django-dev group earlier about #12235.  I need to use a uuid primary key.  I've added the solution to my field, but django is trying to populate it with an integer because the suggested solution in the ticket is to imitate an autofield.  Is that the right way to fix the issue?
[18:22] <ticketbot> https://code.djangoproject.com/ticket/12235
[18:23] <gp> I get a ValueError when I try to add the instance because of this
[18:23] <akaariai_> gp: sorry for stupid question, but why do you need to mark the field as autofield?
[18:24] <akaariai_> and what DB are you using?
[18:26] <akaariai_> if mysql I can see the reason, Django will helpfully issue a select last_insert_id() and populate the field using that value...
[18:26] <gp> akaariai_: sqlite3 and mysql
[18:26] <akaariai_> yeah, likely similar problem on sqlite3 too
[18:27] <akaariai_> so, what happens if the field isn't an autofield?
[18:27] <gp> akaariai_: MultiValueDict error
[18:27] <gp> it has to do with inlines
[18:28] <gp> The summary of the ticket was that everyone's UUIDField implementation is wrong.  The suggested fix was to mark the field as an AutoField in the contribute_to_class method
[18:30] <jacobkm> I have to say I still think it's an implementation problem. I have plenty of models with non-integer primary keys (slug fields, often) and those seem to work fine.
[18:30] <jacobkm> Unless there's some reason why a uuid, specifically, causes Django to fail when it doesn't have an autofield primary key I can't really see that this is a bug in Django.
[18:31] <gp> jacobkm: are those fields automatically filled in?
[18:31] <jacobkm> gp: I'd have to check specifically where, but yeah they kinda have to be.
[18:32] <gp> jacobkm: are any of them open?  I would like to plug on into my model and see if it works.  Would help me narrow the issue down
[18:32] <jacobkm> gp: can you reproduce the problem with a field built into Django? i.e. char field or seomthing?
[18:33] <jacobkm> gp: if so, then it's certainly a bug in Django. If not, it still might be a bug in Django, but it also implies there's more going on.
[18:34] <gp> jacobkm: I will copy over the relavent code to a charfield subclass.  The field is basically a charfield on everything but postgres
[18:34] <gp> will report back
[18:35] <akaariai_> jacobkm: are you setting the field as autofield?
[18:35] <jacobkm> akaariai_: I doubt it, that seems wrong to me.
[18:36] <akaariai_> agreed
[18:36] <gp> jacobkm: the suggested fix to that issue was to use cls._meta.has_auto_field = True and cls._meta.auto_field = self in the fields contribute_to_class method
[18:36] <jacobkm> gp: yeah i don't think that should be needed.
[18:36] <gp> jacobkm: okay good.
[18:53] <gp> here is the field: http://dpaste.com/971742/
[18:53] <gp> I still get the MultiValueDictKeyError
[18:55] <gp> Have a model and an inline model.  Create an instance of the model.  Save it.  Add one child... save it.  Add another child... save = MultiValueDictKeyError
[18:56] <jacobkm> gp: beat me to it :)
[18:56] <jacobkm> It doesn't actually require a custom field: https://gist.github.com/5017514
[18:56] <jacobkm> So yeah, bug.
[19:00] <akaariai_> hmmh, so the problem is that the has_auto_field has dual meaning - first, it is used to determine if the field is autogenerated, but also if it is autogenerated in the DB
[19:01] <gp> think it is a difficult fix?
[19:05] <gp> I would be happy to put it together if someone gave me some guidance on how it should be done
[19:06] <gp> akaariai_: I think that is the issue
[19:06] <akaariai_> gp: A possible idea is to add field.autogenerated, meaning is that the field's value will be automatically generated on first save, and isn't user editable.
[19:07] <akaariai_> then the meta.has_auto_field would point to true AutoFields only. Go through all instances where has_auto_field is used and determine if has_auto_field or field.autogenerated should be used.
[19:07] <akaariai_> might work, but I wouldn't be surprised if it doesn't.
[19:07] <akaariai_> all instances -> all places in django code...
[19:08] <gp> akaariai_: that makes sense
[19:10] <gp> Is this something that could get into the 1.5 or will it have to wait?
[19:10] <akaariai_> gp: 1.5 is out of the question, release hopefully really soon now.
[19:11] <akaariai_> so, no new features
[19:11] <gp> I figured so.  But if it could I was going to start right now lol
[19:12] <gp> Just didn't know if since it was a bug it could sneak in
[19:12] <gp> Should I reopen that bug report or create a new one?
[19:13] <gp> I mean ticket
[19:14] <jacobkm> gp: yeah please do, sorry I meant to do it but I'm getting distracted
[19:15] <gp> jacobkm: no problem.  which one? reopen or new?
[19:15] <jacobkm> gp: um... new one might be better, get rid of the distraction of uuidfield.
[19:16] <jacobkm> I got thrown by that and I think everyone else followed along.
[19:16] <akaariai_> gp: If it turns out there is a simple bugfix for the inline issue only, then fixing just that might be a good way forward, too.
[19:18] <gp> akaariai_: I think the value error issue i've run into is on Model.save_base where the pk is set to the return value of manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw)
[19:19] <gp> akaariai_: actually I guess that doesn't apply now
[19:19] <gp> since imitaiting autofield caused that
[19:20] <akaariai_> yes, if you fix that, then users will still need to imitate autofield, and that isn't good. Internal api, and relies on a hack.
[19:23] <akaariai_> btw the reason why the autogenerated flag change isn't a good idea to 1.5 even if a perfect patch was available just now is that there are users who surely rely on the has_auto_field hack.
[19:24] <akaariai_> and breaking their code just before release isn't a good idea. And then there is of course the possibility of regressions...
[19:24] <gp> akaariai_: yep.  very true

Here is a copy of the field definition I referenced:

class UUIDField(CharField):
    def __init__(self, version=4, node=None, clock_seq=None,
            namespace=None, name=None, auto=False, *args, **kwargs):
        # We store UUIDs in hex format, which is fixed at 32 characters.
        kwargs['max_length'] = 32
        self.auto = auto
        if auto:
            # Do not let the user edit UUIDs if they are auto-assigned.
            kwargs['editable'] = False
            kwargs['blank'] = True
            kwargs['unique'] = True
        
        super(UUIDField, self).__init__(*args, **kwargs)
    
    def pre_save(self, model_instance, add):
        """
        This is used to ensure that we auto-set values if required.
        See CharField.pre_save
        """
        value = getattr(model_instance, self.attname, None)
        if self.auto and add and not value:
            # Assign a new value for this attribute if required.
            uuid = uuid4()
            value = uuid.hex
            setattr(model_instance, self.attname, value)
        return value

[Carl Meyer]

It's not clear at this point if this should be categorized as an ORM bug or an admin bug; it seems like it might have its roots in the ORM, but so far the only failing example is in the admin.

[nickname123]

simple test project setup to test issue. uses selenium

[nickname123]

current patch does not work if the pk field is excluded from the admin

[Karen Tracey]

I believe this is #13696