Code

Opened 3 years ago

Closed 11 months ago

Last modified 11 months 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

Description

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/models.py
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 manage.py sql constaint_test
BEGIN;
CREATE TABLE `constraint_test_parent` (
    `id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY,
    `name` varchar(25) NOT NULL
)
;
CREATE TABLE `constraint_test_child` (
    `id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY,
    `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
COMMIT;

Attachments (2)

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

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by anonymous

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

comment:2 Changed 2 years ago by akaariai

  • 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 2 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()
child.name = 'random name
child.parent_id = random_id
child.save() # Throws IntegrityError
child.father_id = random_id
child.save() # No exception thrown

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

comment:4 Changed 2 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 2 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/creation.py 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 field.rel.to._meta.db_table, 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 2 years ago by jenh

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

comment:6 Changed 2 years ago by jenh

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

Changed 2 years ago by charettes

Added tests

comment:7 Changed 2 years ago by charettes

  • Cc charette.s@… added
  • Keywords proxy added; mysql postgresql removed
  • Needs tests unset
  • Version changed from 1.3 to master

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

Last edited 2 years ago by charettes (previous) (diff)

comment:8 Changed 2 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 23 months ago by jenh

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

comment:10 Changed 11 months ago by timo

  • Triage Stage changed from Accepted to Ready 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 11 months ago by Tim Graham <timograham@…>

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

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 11 months ago by timo

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.

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.