Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29358 closed Cleanup/optimization (fixed)

Add a system check to prohibit models with more than one primary_key field

Reported by: zt_initech Owned by: nobody
Component: Core (System checks) Version: 2.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by zt_initech)

If a Django Model has two Fields with primary_key=True, Django will carry on without complaining at all, but silently corrupt data because the WHERE clause of UPDATE statement generated for a single model instance's save() will only use the first field that has primary_key=True. Same for refresh_from_db() and other things.

I know that the documentation at https://docs.djangoproject.com/en/dev/ref/models/fields/#primary-key says "Only one primary key is allowed on an object.".

I know about https://code.djangoproject.com/ticket/373 and this is not a duplicate of it.

I know that migrate will fail with django.db.utils.OperationalError: table "%s" has more than one primary key, but people use Django with pre-existing database tables.

This ticket is only about making Django fail when trying to load a Model with more than one primary key field, so that people won't try to use such a database table with Django.

If there is some need to let Django access such tables, at least add a warning.

Steps to reproduce:

mkdir compositepk
cd compositepk
rm -rf venv compositepk
virtualenv -p `which python3.6` venv
source venv/bin/activate
pip install Django
django-admin startproject compositepk
cd compositepk/
./manage.py startapp app
echo "INSTALLED_APPS = INSTALLED_APPS + ['app']" >> compositepk/settings.py
VAR1=$(cat <<EOF
from django.db import models

class M(models.Model):
    f1 = models.IntegerField(primary_key=True)
    f2 = models.IntegerField(primary_key=True)
    f3 = models.IntegerField()
EOF
)
echo "${VAR1}" > app/models.py
unset VAR1
./manage.py migrate contenttypes
./manage.py migrate auth
./manage.py migrate admin
./manage.py migrate sessions
echo "create table app_m (f1 integer not null, f2 integer not null, f3 integer, primary key (f1, f2));" | sqlite3 db.sqlite3
echo "insert into django_migrations (app, name, applied) values ('app', '0001_initial', datetime());" | sqlite3 db.sqlite3
./manage.py makemigrations
./manage.py migrate
VAR1=$(cat <<EOF
from app.models import M
m = M.objects.create(f1=1, f2=1)
m.refresh_from_db()
m.f3 = 42
m.save()
from django.db import connection
print(connection.queries[-3]['sql'])
print(connection.queries[-1]['sql'])
EOF
)
echo "${VAR1}" | ./manage.py shell
unset VAR1
deactivate
cd ..

You see it prints:

SELECT "app_m"."f1", "app_m"."f2", "app_m"."f3" FROM "app_m" WHERE "app_m"."f1" = 1
UPDATE "app_m" SET "f3" = 42 WHERE "app_m"."f1" = 1

Change History (5)

comment:1 by zt_initech, 6 years ago

Description: modified (diff)

comment:2 by Tim Graham, 6 years ago

Component: Database layer (models, ORM)Core (System checks)
Summary: loading a model with more than one primary_key field should failAdd a system check to prohibit models with more than one primary_key field
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:3 by Tim Graham, 6 years ago

Has patch: set

comment:4 by Carlton Gibson <carlton.gibson@…>, 6 years ago

Resolution: fixed
Status: newclosed

In 816b8d95:

Fixed #29358 -- Added a system check to prohibit models with more than one primary_key field.

comment:5 by GitHub <noreply@…>, 6 years ago

In 21fd8041:

Refs #29358 -- Corrected wording in primary key check message.

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