Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#11702 closed Uncategorized (fixed)

ForeignKey validation should check that to_field is unique

Reported by: physicsnick Owned by: marcosmoyano
Component: Database layer (models, ORM) Version: 1.1
Severity: Normal Keywords: pycamp2010
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Currently, ForeignKey validation does not check that to_field is unique. Here is an example:

class Author(models.Model): 
    code = models.CharField(max_length=10, db_index=True) 
    first_name = models.CharField(max_length=30) 
    last_name = models.CharField(max_length=40) 
class Book(models.Model): 
    title = models.CharField(max_length=100) 
    author = models.ForeignKey(Author, to_field='code')

Django happily accepts these models. For PostgresSQL and Oracle, foreign keys must be unique, but MySQL and SQLite make no such requirement. MySQL requires only that the column be indexed, and SQLite has no requirements (it does not enforce foreign key constraints).

Django requires uniqueness so it should be enforcing this on its own. The documentation of to_field should also specify that it should be unique.

Here is the relevant MySQL documentation:

http://dev.mysql.com/doc/refman/5.1/en/innodb-foreign-key-constraints.html

...MySQL and InnoDB require that the referenced columns be indexed for performance. However, the system does not enforce a requirement that the referenced columns be UNIQUE or be declared NOT NULL.


Here is some extended discussion:
http://groups.google.com/group/django-users/browse_thread/thread/fcd3915a19ae333e

Attachments (2)

related.patch (1.8 KB) - added by marcosmoyano 5 years ago.
django.db.fields.related patch
new_patch.patch (2.7 KB) - added by marcosmoyano 5 years ago.
New patch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by marcosmoyano

  • Owner changed from nobody to marcosmoyano
  • Status changed from new to assigned

Changed 5 years ago by marcosmoyano

django.db.fields.related patch

comment:3 Changed 5 years ago by marcosmoyano

A humble approach at related field initialization. Don't know how to write a test suite for this since it's on initialization. I've ran the test suite just to make sure nothing breaks.

comment:4 Changed 5 years ago by ramiro

  • Has patch set
  • Keywords pycamp2010 added
  • Patch needs improvement set

Changed 5 years ago by marcosmoyano

New patch

comment:5 Changed 5 years ago by marcosmoyano

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

New patch. Moved validation code to django.core.management.validation and add test suite.

I think the validation could be clearear and simpler if we add a line like this:
self.to_field = to_field
in django.db.models.fields.related on ForeignKey init method.

comment:6 Changed 5 years ago by kmtracey

  • Resolution fixed deleted
  • Status changed from closed to reopened

It's not fixed until patch is committed.

comment:7 Changed 5 years ago by kmtracey

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

(In [12756]) Fixed #11702: Catch to_field specifying a non-unique target in validation. Thanks marcosmoyano.

comment:8 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

comment:9 Changed 3 years ago by anandjeyahar1

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Shouldn't this fix be specific based on the db being used? i.e syncdb should check the current db being used and raise an error only for postgresql or oracle. Am using mysql and i shouldn't be blocked from using a non_unique field as a foreign key.

comment:10 Changed 3 years ago by kmtracey

No. The validation is needed especially for databases that don't enforce this constraint. Django's ForeignKey is many-to-one, the Django code relies on that characteristic, and you can get odd results if the database does not enforce it, see for example this thread:

https://groups.google.com/forum/?fromgroups#!searchin/django-users/QuerySet.count%28%29$20inaccurate$20across$20ForeignKey$20relationships/django-users/fMmWVDr0Re8/overview

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