Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#17519 closed Bug (fixed)

Missing SQL constraint to proxy models

Reported by: thibaultj Owned by: jenh
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: sql constraint proxy
Cc: anssi.kaariainen@…, jenh, charette.s@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


In a model class, when you add a ForeignKey to a proxy model, the sql reference will no be created in Mysql. This is a severe issue, since the integrity structure of the database cannot be ensured.

Moreover, as stated in #16128, the django admin will not gather related proxy model objects when deleting something. This is a separate issue though, since we are talking of the sql generation here.

# file constraint_test/
class Parent(models.Model):
    name = models.CharField(max_length=25)

class Father(Parent):
    class Meta:
        proxy = True

    def __unicode__(self):
        return "I'm your father, luke"

class Child(models.Model):
    name = models.CharField(max_length=25)
    parent = models.ForeignKey(Parent, related_name='child_parent_set')
    father = models.ForeignKey(Father, related_name='child_father_set')
python sql constaint_test
CREATE TABLE `constraint_test_parent` (
    `name` varchar(25) NOT NULL
CREATE TABLE `constraint_test_child` (
    `name` varchar(25) NOT NULL,
    `parent_id` integer NOT NULL,
    `father_id` integer NOT NULL
ALTER TABLE `constraint_test_child` ADD CONSTRAINT `parent_id_refs_id_875b6ff2` FOREIGN KEY (`parent_id`) REFERENCES `constraint_test_parent` (`id`);
-- Missing constraint to the father table

Attachments (2)

17519.diff (501 bytes) - added by jenh 7 years ago.
Patch to remove exclusion of proxy models from creation of pending references.
17519.1.diff (4.1 KB) - added by Simon Charette 7 years ago.
Added tests

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by anonymous

Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added

Sorry for the stupid question, but what is the point of allowing foreign keys to proxy models at all? In the DB the relation is to the base class anyways. The only point I can see is you want to return instances of the proxy class instead of the base class, but I hope that could be fixed by something like Parent.objects.as_model(Father). This would have uses beyond just ForeignKeys.

My point is that is it worth fixing this by complicating the ForeignKey implementation? You don't get that much from the work after all. I haven't actually checked what it would need to fix this. One possibility is to just make the ForeignKey in reality to the base class and when constructing the related query set, just start from the Father model.

If it is easy to fix the current implementation, by all means lets do that. But I am not too sure that is the case here...

comment:3 Changed 7 years ago by thibaultj

I don't think this is directly related to the ForeignKey implementation. No matter how the python model and class inheritance is handled, data integrity must be respected on the db level.

Consider the following exemple:

random_id = 666
child = Child() = 'random name
child.parent_id = random_id # Throws IntegrityError
child.father_id = random_id # No exception thrown

That means that you can have crappy data in db, which breaks the admin modules for those models.

comment:4 Changed 7 years ago by jenh

Cc: jenh added

I am getting the same behaviour with SQLite and PostgreSQL, only with a slight twist. If the ForeignKey is not given a related_name, the SQL will include a reference to the parent model. It is only when a related_name is present that the SQL will not include a reference. This strikes me as rather odd, but I haven't ever delved into the database code.

As for the use of specifying the proxy model in the model definition, I see it as a matter of clarity. The ORM provides useful abstractions, and this is one - the reference really is, in the example, to a Father and not to a Parent, and the fact that this distinction does not exist at the database level is immaterial at the ORM level.

comment:5 Changed 7 years ago by jenh

After further investigation, it appears to be merely coincidence that in my cases the presence of related_name corresponds exactly with whether the constraint is created. Sorry for that red herring.

The reason some of my ForeignKeys get constraints is that they are handled in db/backends/ BaseDatabaseCreation.sql_for_inline_foreign_key_references, when the proxy model being referenced happens to be in the list of the app's models that have been processed at the point it reaches this non-proxy model. The SQL generator has no problem with the fact that the referenced model is a proxy model, since it uses, which matches the parent model.

The reason the other ForeignKeys don't get constraints is that if the model containing the references is processed before the proxy models it is referencing, the references get deferred and are handled in BaseDatabaseCreation.sql_for_pending_references. Crucially, this method operates on the model being referenced, and thus referenced proxy models fall afoul of its ditching of unmanaged and proxy models.

Now, it seems to me that there is no reason for sql_for_pending_references to not do anything with proxy models. The constraints it creates use the underlying database table and are perfectly fine, and if there are no pending references to the proxy model, the method does nothing. However, being completely new to this part of Django's code, I may well be missing something important.

I'm attaching a patch that just removes the requirement that a model not be a proxy model in order to create pending references to it. I don't have a test that this fixes the problem; if someone can give me some pointers on where that test should go and how it should be formulated, I'll do my best to create one. It does at least not cause any of the existing tests to fail (checked under PostgreSQL and SQLite (which doesn't use the code that's changed)).

Changed 7 years ago by jenh

Attachment: 17519.diff added

Patch to remove exclusion of proxy models from creation of pending references.

comment:6 Changed 7 years ago by jenh

Has patch: set
Keywords: postgresql added
Needs tests: set

Changed 7 years ago by Simon Charette

Attachment: 17519.1.diff added

Added tests

comment:7 Changed 7 years ago by Simon Charette

Cc: charette.s@… added
Keywords: proxy added; mysql postgresql removed
Needs tests: unset
Version: 1.3master

Added the missing tests. They both pass on sqlite (tests are skipped because the backend doesn't enforce constraints) and postgresql.

Last edited 7 years ago by Simon Charette (previous) (diff)

comment:8 Changed 7 years ago by jenh

Is there anything else that I can do to progress this, or are we just needing a reviewer to mark as ready for checkin (or state why it isn't)? I'm not sure what if any documentation should be added.

comment:9 Changed 7 years ago by jenh

Owner: changed from nobody to jenh
Status: newassigned

comment:10 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Verified that the tests fail on MySQL before the fix and pass on all backends after (didn't try Oracle). I'm not confident enough to check this in myself, but am marking it as such so someone else can verify. Here's a PR that merges cleanly.

comment:11 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 196cc875b26f266e8f8dfd5be9daa0b8f246b9cd:

[1.6.x] Fixed #17519 -- Fixed missing SQL constraints to proxy models.

Thanks thibaultj for the report, jenh for the patch,
and charettes for the tests.

Backport of aa830009de from master

comment:12 Changed 6 years ago by Tim Graham

In aa830009de940e31711b4858501becd7f34a1972:

Fixed #17519 -- Fixed missing SQL constraints to proxy models.

Thanks thibaultj for the report, jenh for the patch,
and charettes for the tests.

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