Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16128 closed Bug (fixed)

cascade delete does not work for proxy models

Reported by: xkennyx@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: cascade delete proxy meta
Cc: Jarek, anssi.kaariainen@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi all :), there is problem with cascade delete, when proxy model is used.
example:

class ExUser(User):                  
    class Meta:                      
        proxy = True                 
              
# work:                     
class Profile(models.Model):         
    user = models.ForeignKey(User) 
  
# does not work:
class Profile(models.Model):         
    user = models.ForeignKey(ExUser) 

when I delete user, then in first scenario is profile also deleted, but when I want use proxy as foreign key, then profile is not deleted, and it lead to integrity problem in DB

Attachments (10)

16128.patch (2.3 KB) - added by aaugustin 4 years ago.
16128.2.patch (2.3 KB) - added by aaugustin 4 years ago.
mysite.tar.gz (3.2 KB) - added by xkennyx@… 4 years ago.
16128.3.patch (2.1 KB) - added by aaugustin 4 years ago.
16128.4.patch (2.8 KB) - added by claudep 4 years ago.
Patch with test and fix
16128.5.patch (2.9 KB) - added by adamnelson 4 years ago.
Update to make previous patch merge
proxy_bug.zip (1.9 KB) - added by jaap3 4 years ago.
Testcase that deletes models, proxies and proxies of proxies.
16128.6.diff (13.4 KB) - added by akaariai 4 years ago.
16128.diff (13.8 KB) - added by akaariai 3 years ago.
#16128-proxy_model_objects_deletion.diff (8.1 KB) - added by Kronuz 3 years ago.
Not assume all object models are the same as the first one on the set

Download all attachments as: .zip

Change History (43)

Changed 4 years ago by aaugustin

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I have written a test case for the issue describe above, and that test passes. In other words, I'm unable to reproduce the problem at this point.

Changed 4 years ago by aaugustin

comment:2 Changed 4 years ago by aaugustin

  • Resolution set to worksforme
  • Status changed from new to closed

In case it would be specific to django.contrib.auth.models.User, I wrote a second test case. It passes too.

xkennyx, could you provide a complete test case that exhibits the bug?

Changed 4 years ago by xkennyx@…

comment:3 Changed 4 years ago by xkennyx@…

  • Resolution worksforme deleted
  • Status changed from closed to reopened

please try this example code,
But when I look at your testcase I probably know the problem.

You are creating and deleting AuthUserProxy. It probably works , but when I delete the base of AuthUserProxy the (AuthUser) then it fail.
And because AuthUser and AuthUserProxy are the same for database, then it lead to integrity problems.

I'm not now sure if it is bug of feature ;-) I will leave it to you. But it is annoying

And thanks for fast response :)

comment:4 Changed 4 years ago by xkennyx@…

(this was not posted)
Try this attached test.
and these steps:

1) go to admin page
2) create new user "test"
3) create new profile for user "test"
4) try delete user "test"
and there starts problem, only user is deleted and not profile.
(It can be deleted in sqlite3 but in postgresql it fail because of constrain)

when you change in model the foreign key to User
then delete ask to delete also profile

comment:5 Changed 4 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

OK, here's a test case that exhibits the problem. Indeed, I think it's a bug.

Changed 4 years ago by aaugustin

comment:6 Changed 4 years ago by xkennyx@…

  • UI/UX unset

There is one funny thing, I need also a way to invocate the bug :) Eg. delete child but do not delete parent. This is now possible with inheriting from proxy model, but after fixing .. is there way ?

comment:7 Changed 4 years ago by Jarek

  • Cc Jarek added

sorry, previous comment was mistage (I cannot delete it)

Changed 4 years ago by claudep

Patch with test and fix

comment:8 Changed 4 years ago by claudep

  • Has patch set

I've been bitten by this bug. In fact, aaugustin's patch (16128.3.patch) is not exactly related to this issue. It might be a bug (not sure), but in that case, it's the problem of the non-proxy class not seeing at all the related proxy objects.

In this bug, the issue is about sorting objects before deletion, based on the dependencies structure. The keys of dependencies dict are constructed by accessing the rel.to attribute which is not a proxy class.

I used your test case, but simplified it. It fails only with MySql-Innodb, but might also fail on Postgres (if somebody knows how to really commit the objects in the DB inside the test case to trigger the IntegrityError, before the delete call).

Changed 4 years ago by adamnelson

Update to make previous patch merge

comment:9 Changed 4 years ago by adamnelson

  • Triage Stage changed from Accepted to Ready for checkin

Slightly modified patch to make merge work after recent commits. Passes tests.

comment:10 Changed 4 years ago by Alex

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

This patch isn't sufficient, it doesn't handle the case of a proxy-of-a-proxy.

comment:11 Changed 4 years ago by thibault@…

I noticed that in the case of a ForeignKey to a proxy model, the constraint key is not created in MySql. I think it could be related, but tell me if anyone thinks I should open a new bug.

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')
mysql> SHOW CREATE TABLE tata_child;
| tata_child | CREATE TABLE `tata_child` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `name` varchar(25) NOT NULL,
  `parent_id` int(11) NOT NULL,
  `father_id` int(11) NOT NULL,
  PRIMARY KEY (`id`),
  KEY `tata_child_63f17a16` (`parent_id`),
  KEY `tata_child_656bfb9c` (`father_id`),
  CONSTRAINT `parent_id_refs_id_875b6ff2` FOREIGN KEY (`parent_id`) REFERENCES `tata_parent` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

comment:12 Changed 4 years ago by akaariai

I wonder if the right way would be to fix Model __eq__ and Model __hash__ to make proxy models equal to their "concrete" target? There is discussion about the equality operator in #16458.

comment:13 Changed 4 years ago by carljm

#17140 was a duplicate.

comment:14 Changed 4 years ago by carljm

To be clear, there are two varieties of this problem: if the FK points to the proxy subclass and you delete the concrete superclass instance, or if the FK points to the concrete superclass and you delete a proxy subclass instance. So the tests should cover both of these cases, with both one level of proxying and multiple levels of proxying.

Also, contrib.auth models shouldn't be reused in non-auth tests. In general a new test for a new bug should get new test models, unless it's closely related to existing tests (i.e. just adding a new test method or two to an existing TestCase), in which case it can share the models already used in that TestCase.

comment:15 Changed 4 years ago by carljm

(Also, to thibault: I'm not sure what constraints Django does or does not normally create on MySQL, but if it behaves differently with an FK to a proxy model than with an FK to its concrete parent, then that certainly does sound like a bug. It's a different one from this bug, however.)

Thanks to everyone for the work on this patch thus far!

Changed 4 years ago by jaap3

Testcase that deletes models, proxies and proxies of proxies.

comment:16 Changed 4 years ago by jaap3

I've added an expanded testcase of the one I submitted to ticket #17140. This one also shows the proxy-of-proxy issue.

comment:17 Changed 4 years ago by akaariai

  • Patch needs improvement unset

I did some work for this. The main fix is that when deleting a model and collecting related objects for it, check all the proxy children of the models' concrete class (that is, check the "whole proxy equivalence class"). For this, I added two new _meta fields:

  • proxying_models -> a list of models proxying this class
  • concrete_class -> For every non-abstract model, this will be the concrete class of the model. For proxy models it is the concrete parent, for concrete classes, this will be the class itself. This cleans the code in some other places, too.

I also fixed something that I think is a bug. proxy_for_model was set up in a way that it was actually always the concrete parent of the class. There were many places which clearly assumed that proxy_for_model was the direct proxy parent of the class, so that proxy_for_model would create a proxy-chain to the concrete parent of the class. One such place was actually the place where proxy_for_model setup was done. If I am not mistaken, this should fix one minor bug in admin, where deferred models would get its concrete parents name, not it proxy parents name (see response_change() in django/contrib/admin/options.py).

The tests in proxy_bug.zip are now passing, as is the whole test suite on sqlite. The tests from proxy_bug are included in regressiontests/proxy_bug. This is probably not the correct place for the new tests... Maybe regressiontests/delete_regress?

I am not sure if the route taken in the patch is the correct one. It might be better to handle the proxy-relation collection in ._meta.get_all_related_objects(include_proxy_chain=True) where the include_proxy_chain would be a new kwarg. There might be some point in also doing the deletes on the concrete_class internally, even when doing ProxyClass.objects.all().delete(). This would cause signaling changes and it might break other stuff, so I didn't check that route further. Still one possibility is to register the proxy-relations as hidden relations to the concrete class, so that we would not need _meta.proxying_models at all.

For those preferring git, there is a git branch at https://github.com/akaariai/django/tree/proxy_delete

Changed 4 years ago by akaariai

comment:18 Changed 3 years ago by carljm

  • Patch needs improvement set

Did a brief review here, I'd like to get this fix in before 1.4. I think the route taken is basically correct; we don't want to signal the delete on the concrete class instead of the proxy, that would be a breaking change.

I do think that the proxy-equivalence-class-relations logic belongs in Options, not in the deletion code. And the new tests belong in delete_regress. So basically all your instincts were correct :-)

Thanks for the patch - if you have time to update it that would be great, if not I'll try to find time soon.

comment:19 follow-up: Changed 3 years ago by akaariai

  • Cc anssi.kaariainen@… added

I will not have time before Monday. I don't know if that is soon enough for 1.4.

A couple of notes:

  • If a model has a large number of proxy models, does that cause performance problems?
  • The proxy_for_model is actually the same thing as concrete_model. proxy_for_model is supposed to build a chain of proxies to the concrete model, but that is not happening correctly. I don't think fixing this is this ticket's problem, but it is good to note this as this can cause problems when fixing this ticket.
  • I hope the auto-created deferred model classes are not included in the proxy eq class. That could cause some problems...

comment:20 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by carljm

Replying to akaariai:

I will not have time before Monday. I don't know if that is soon enough for 1.4.

This is a bugfix, it can go in after the beta is released.

  • If a model has a large number of proxy models, does that cause performance problems?

I'm not particularly concerned about that, as long as the patch doesn't do anything egregiously silly. If the proxy equivalence class is built up in Options at class-creation time, and we can move the get-relations-with-proxy-equivalence logic into Options so we aren't looking through the same relations over and over again, I think we should be fine.

  • The proxy_for_model is actually the same thing as concrete_model. proxy_for_model is supposed to build a chain of proxies to the concrete model, but that is not happening correctly. I don't think fixing this is this ticket's problem, but it is good to note this as this can cause problems when fixing this ticket.

Is there a separate bug filed for that problem? If we fix that first, would it make the fix here clearer/simpler?

  • I hope the auto-created deferred model classes are not included in the proxy eq class. That could cause some problems...

I'm not sure what you mean by "auto-created deferred model classes". The only auto-created model classes I'm aware of are those for many-to-many relations, and I don't see how those would cause a problem; they should never be nor have proxies.

I think the whole concept of the "proxy equivalence class", if represented clearly in Options, will be useful in fixing other issues that make proxy models less useful than they could be. For instance, assigning any member of a proxy equivalence class to a ForeignKey ought to satisfy that FK's "is this object of the right type?" check. (Obviously that should be a separate ticket, just mentioning that I think this is adding something generally useful to Options).

comment:21 in reply to: ↑ 20 Changed 3 years ago by carljm

Replying to carljm:

Replying to akaariai:

  • I hope the auto-created deferred model classes are not included in the proxy eq class. That could cause some problems...

I'm not sure what you mean by "auto-created deferred model classes". The only auto-created model classes I'm aware of are those for many-to-many relations, and I don't see how those would cause a problem; they should never be nor have proxies.

Whoops, sorry, charettes set me straight here on IRC. I'd forgotten that the .defer() implementation auto-creates proxy model classes automatically. On quick inspection I can't see why these wouldn't be included, but I'm also not clear what problem this would cause. You're right that it may need attention/tests, in any case.

comment:22 Changed 3 years ago by akaariai

I created a ticket for the proxy_for_model bug (#17678).

My performance concern is about doing the same queries again and again, so I think we are on the same page about that. This could become ridiculously slow if this would happen for each deferred class instance.

I don't have time to work on this today, all time went to 17678 (I didn't realize that there was already an implementation available in the latest patch to this ticket). Busy week and all that, but I hope I can work on this ticket sometime during this week. But, there is one problem which I would like to solve before working on this.

I think I will go for an implementation where the proxy_equivalence_class is a property, and the real set is contained in model._meta.concrete_model. This should be pretty clean implementation.

One problem I would like to solve still: Should the auto-created deferred model classes be added to the proxy-equivalence set? I think yes, as this is easier to implement, and it is possible to get only the real classes by doing some filtering on the equivalence class. Going into the other direction isn't possible, of course.

comment:23 follow-up: Changed 3 years ago by anonymous

Any chance this, assuming you will come to a satisfactory patch, will also go into 1.3?

comment:24 in reply to: ↑ 23 Changed 3 years ago by carljm

Replying to anonymous:

Any chance this, assuming you will come to a satisfactory patch, will also go into 1.3?

No. It is not a security, data-loss, or crash bug, so it does not meet the criteria for backport.

comment:25 Changed 3 years ago by akaariai

  • Patch needs improvement unset

Ok, another try for this ticket. The patch includes the patch of #17678.

There is no meta proxy_equivalence_class. The reason is, it is very easy to test if this model is in proxy-equivalence with the other: if self._meta.concrete_class == other._meta.concrete_class. This should work for the ForeignKey example.

I think there might be similar bug still waiting for generic foreign key case, but I would like to deal with that separately.

I hope the patch is OK, too long day to actually think it through... :)

Changed 3 years ago by akaariai

comment:26 Changed 3 years ago by anler86@…

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

Just as a notice. I'm still having this issue in my setup: osx Lion - mysql 5.5.19.
Sometimes when I run my tests they all pass and sometimes they don't, is pretty rare and can drive you crazy but, certainly it have something to do with mysql or mysql-osxlion and not with Django itself because if I switch to using sqlite3 or postgresql as my database engine, everything works fine.

Here is a screenshot where I'm running some test that tries to delete an instance of a proxy model( which have related objects attached to it) and you can see that in the first run it deletes two models (related and proxy) and succeed, but in the second only deletes one (proxy) and fails
http://cl.ly/3t2X0y1v0l2t292z3t3s

comment:27 Changed 3 years ago by akaariai

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, why did you resolve this as fixed? I am going to reopen this. If the tests in the patch do not pass, this is not fixed.

BTW I just noticed there is some leftover debugging stuff in the patch. Nothing breaking, but it needs a minor cleanup.

comment:28 follow-up: Changed 3 years ago by anonymous

Sorry, I resolve it as fixed accidentally :S Regarding the tests, I repeat what I said before, the tests pass...sometimes and sometimes doesn't, without change nothing! It's pretty rare, but I'm posting it here just in case it happens to someone else.

comment:29 in reply to: ↑ 28 Changed 3 years ago by anler <anler86@…>

Replying to anonymous:

That was me, I submitted it as anonymous again, accidentally

comment:30 Changed 3 years ago by carljm

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

In [17664]:

Fixed #16128 - Correctly cascade-delete proxy models as if they were the concrete model class. Thanks xkennyx for the report, and Aymeric Augustin, Claude Paroz, Adam Nelson, jaap3, and Anssi Kääriäinen for work on the patch.

comment:31 Changed 3 years ago by carljm

In [17755]:

Reorganized proxy-delete tests for easier addition of new tests. Refs #16128.

comment:32 follow-up: Changed 3 years ago by Kronuz

I'm still having issues deleting objects that come from a collection of objects of different types... This happens (particularly) with polymorphic (django-polymorphic) objects. I've added a patch to fix this and be able to delete sets of polymorphic objects. The patch makes the collector not assume all objects are of the same type.

Last edited 3 years ago by Kronuz (previous) (diff)

Changed 3 years ago by Kronuz

Not assume all object models are the same as the first one on the set

comment:33 in reply to: ↑ 32 Changed 3 years ago by ptone

Replying to Kronuz:

I'm still having issues deleting objects that come from a collection of objects of different types... This happens (particularly) with polymorphic (django-polymorphic) objects. I've added a patch to fix this and be able to delete sets of polymorphic objects. The patch makes the collector not assume all objects are of the same type.

Because this ticket has been closed a while - it would be best to open a new ticket for this case, reference this one from the description, and include a test that demonstrates the failure.

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