#17519 closed Bug (fixed)
Missing SQL constraint to proxy models
Reported by: | thibaultj | Owned by: | jenh |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
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)
Change History (14)
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 13 years ago
Cc: | added |
---|
comment:3 by , 12 years ago
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 by , 12 years ago
Cc: | 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 by , 12 years ago
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)).
by , 12 years ago
Attachment: | 17519.diff added |
---|
Patch to remove exclusion of proxy models from creation of pending references.
comment:6 by , 12 years ago
Has patch: | set |
---|---|
Keywords: | postgresql added |
Needs tests: | set |
comment:7 by , 12 years ago
Cc: | added |
---|---|
Keywords: | proxy added; mysql postgresql removed |
Needs tests: | unset |
Version: | 1.3 → master |
Added the missing tests. They both pass on sqlite (tests are skipped because the backend doesn't enforce constraints) and postgresql.
comment:8 by , 12 years ago
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 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 11 years ago
Triage Stage: | Accepted → 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 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:12 by , 11 years ago
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.
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 theForeignKey
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...