Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15884 closed Bug (fixed)

Model validation allows nullable primary key field.

Reported by: JustinTArthur Owned by: JustinTArthur
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: julie@…, alexandrul Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If I have a primary key field that is also nullable, the model validation code is currently allowing it:

class MyModel(models.Model):
    other_model = OneToOneField(OtherModel, primary_key=True, null=True)

This behavior seems like a defect given two circumstances:

  • No SQL implementation I know of allows a primary key column to be NULL in the schema.
  • Django's CASCADE deletion will None/NULL out any null=True foreign keys (see source:/trunk/django/db/models/deletion.py@15927#L19),
    • While a SQL backend might correctly map the None to a 0 instead of NULL in the UPDATE statement, after a second deletion against the same table, you would have more than one row with primary key of 0, thus violating the uniqueness constraint intrinsic to a primary key field.

Attachments (2)

ticket_15884-patch_1.diff (948 bytes) - added by JustinTArthur 3 years ago.
Patch to add validation error when a model field is both a primary key and nullable.
15884.diff (2.7 KB) - added by jpichon 3 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by JustinTArthur

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

The correct link for the CASCADE interest point is actually source:/django/trunk/django/db/models/deletion.py@15927#L19

Changed 3 years ago by JustinTArthur

Patch to add validation error when a model field is both a primary key and nullable.

comment:2 Changed 3 years ago by JustinTArthur

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Has patch set

comment:3 Changed 3 years ago by jacob

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by jpichon

comment:4 Changed 3 years ago by jpichon

  • Cc julie@… added
  • Needs tests unset
  • UI/UX unset

I'm attaching a patch with a test for the new validation behaviour -- the patch also includes JustinTArthur's diff as there was a missing double-quote around the field name.

comment:5 Changed 3 years ago by alexandrul

  • Cc alexandrul added
  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 3 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16678]:

Teach "django-admin.py validate" to forbid nullable primary keys.

Fixes #15884, with thanks to JustinTArthur and Julie Pichon.

comment:7 Changed 3 years ago by aaugustin

The patch breaks with Oracle, see #16694.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.