#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 )
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 , 7 years ago
Description: | modified (diff) |
---|
comment:2 by , 7 years ago
Component: | Database layer (models, ORM) → Core (System checks) |
---|---|
Summary: | loading a model with more than one primary_key field should fail → Add a system check to prohibit models with more than one primary_key field |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
comment:3 by , 7 years ago
Has patch: | set |
---|
PR