[Django] #29358: loading a model with more than one primary_key field should fail

[Django]

#29358: loading a model with more than one primary_key field should fail
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
zt_initech |
Type: Bug | Status: new
Component: Database | Version: 2.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:

{{{#!bash
mkdir 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
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29358>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#29358: loading a model with more than one primary_key field should fail
-------------------------------------+-------------------------------------

Reporter: zt_initech | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by zt_initech:

Old description:

> 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.

New description:

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:

{{{#!bash
mkdir compositepk
cd compositepk

You see it prints:

--

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:1>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
--------------------------------------+------------------------------------
Reporter: zt_initech | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Tim Graham):

* component: Database layer (models, ORM) => Core (System checks)
* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:2>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
-------------------------------------+-------------------------------------
Reporter: zt_initech | Owner: Nicolas
Type: | Noé
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 2.0
checks) |

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nicolas Noé):

* status: new => assigned
* owner: nobody => Nicolas Noé

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:3>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
--------------------------------------+------------------------------------
Reporter: zt_initech | Owner: (none)

Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: 2.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Nicolas Noé):

* owner: Nicolas Noé => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:4>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
--------------------------------------+------------------------------------
Reporter: zt_initech | Owner: nobody

Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Tim Graham):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/9911 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:3>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
--------------------------------------+------------------------------------
Reporter: zt_initech | Owner: nobody

Type: Cleanup/optimization | Status: closed

Component: Core (System checks) | Version: 2.0

Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Carlton Gibson <carlton.gibson@…>):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"816b8d9518c41f034dcbacfd1f1826f2366975e5" 816b8d95]:
{{{
#!CommitTicketReference repository=""
revision="816b8d9518c41f034dcbacfd1f1826f2366975e5"
Fixed #29358 -- Added a system check to prohibit models with more than one
primary_key field.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:4>

[Django]

#29358: Add a system check to prohibit models with more than one primary_key field
--------------------------------------+------------------------------------
Reporter: zt_initech | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Core (System checks) | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------

Comment (by GitHub <noreply@…>):

In [changeset:"21fd8041c163eafc0a6bfad0efc8e98693580622" 21fd8041]:
{{{
#!CommitTicketReference repository=""
revision="21fd8041c163eafc0a6bfad0efc8e98693580622"
Refs #29358 -- Corrected wording in primary key check message.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29358#comment:5>