Opened 12 years ago

Closed 8 years ago

#17761 closed Bug (fixed)

save_base() does not properly detect when MTI parent key is unset

Reported by: Aron Grififs Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: aron@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Consider the following MTI scenario, where the primary key of the parent is a CharField:

from django.db import models

class Foo(models.Model):
    id = models.CharField(max_length=6, primary_key=True)

class Bar(Foo):
    data = models.TextField()

Attempting to instantiate and save the child model fails:

>>> from bar.models import Bar
>>> b=Bar()
>>> b.pk='abcdef'
>>> b.save()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/base.py", line 460, in save
    self.save_base(using=using, force_insert=force_insert, force_update=force_update)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/base.py", line 553, in save_base
    result = manager._insert(values, return_id=update_pk, using=using)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/manager.py", line 195, in _insert
    return insert_query(self.model, values, **kwargs)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/query.py", line 1436, in insert_query
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 791, in execute_sql
    cursor = super(SQLInsertCompiler, self).execute_sql(None)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 735, in execute_sql
    cursor.execute(sql, params)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/backends/util.py", line 34, in execute
    return self.cursor.execute(sql, params)
  File "/home/aron/.virtualenvs/pp/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py", line 234, in execute
    return Database.Cursor.execute(self, query, params)
IntegrityError: bar_bar.foo_ptr_id may not be NULL

The reason this happens is the following code around line 500 in
[source:django/trunk/django/db/models/base.py db/models/base.py]:

class Model(object):
    ...

    def save_base(...):
        ...

        for parent, field in meta.parents.items():
            # At this point, parent's primary key field may be unknown
            # (for example, from administration form which doesn't fill
            # this field). If so, fill it.
            if field and getattr(self, parent._meta.pk.attname) is None and getattr(self, field.attname) is not None:
                setattr(self, parent._meta.pk.attname, getattr(self, field.attname))

            self.save_base(cls=parent, origin=org, using=using)

This fails because the default value for a CharField is the emtpy string
rather than None. This code works for an IntegerField primary key (which
defaults to None until set). It also works for a CharField(null=True) but
it's not clear to me that's the right answer, because it means null is
valid for the DB column, which isn't right.

I think the right answer is to change the test from "is None" to "in
[None, '']" as shown in the attached patch.

You might ask, "What about the test on the child model's pk? Does that need
to change too?" The answer is no, because the child model's pk is a
OneToOneField which defaults to None, so only the first None-test on the
line needs to change.

Attachments (2)

django-17761.patch (824 bytes ) - added by Aron Grififs 12 years ago.
django-17761-tests.patch (2.0 KB ) - added by Aron Grififs 12 years ago.

Download all attachments as: .zip

Change History (8)

by Aron Grififs, 12 years ago

Attachment: django-17761.patch added

comment:1 by Aron Grififs, 12 years ago

Cc: aron@… added
Has patch: set

comment:2 by Ramiro Morales, 12 years ago

Needs tests: set

comment:3 by Ramiro Morales, 12 years ago

Triage Stage: UnreviewedAccepted

by Aron Grififs, 12 years ago

Attachment: django-17761-tests.patch added

comment:4 by Aron Grififs, 12 years ago

Needs tests: unset

I've attached a regression test patch, so I'm unchecking "Needs tests" (though the meaning of that field is unclear, does it mean "issue needs a test in django" or "ticket needs a test attached?" I'm interpreting it as the latter)

comment:5 by Tim Graham, 10 years ago

Patch needs improvement: set

I haven't verified if this is still an issue, but the patch no longer applies cleanly.

comment:6 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top