cascade delete does not work for proxy models

[xkennyx@…]

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

[Aymeric Augustin]

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.

[Aymeric Augustin]

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?

[xkennyx@…]

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 :)

[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

[Aymeric Augustin]

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

[xkennyx@…]

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 ?

[Jarek]

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

[Claude Paroz]

Patch with test and fix

[Claude Paroz]

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).

[Adam Nelson]

Update to make previous patch merge

[Adam Nelson]

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

[Alex Gaynor]

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

[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

[Anssi Kääriäinen]

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.

[Carl Meyer]

#17140 was a duplicate.

[Carl Meyer]

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.

[Carl Meyer]

(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!

[Jaap Roes]

Testcase that deletes models, proxies and proxies of proxies.

[Jaap Roes]

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

[Anssi Kääriäinen]

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

[Carl Meyer]

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.

[Anssi Kääriäinen]

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...

[Carl Meyer]

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).

[Carl Meyer]

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.

[Anssi Kääriäinen]

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.

[anonymous]

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

[Carl Meyer]

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.

[Anssi Kääriäinen]

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... :)

[anler86@…]

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

[Anssi Kääriäinen]

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.

[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.

[anler <anler86@…>]

Replying to anonymous:

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

[Carl Meyer]

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.

[Carl Meyer]

In [17755]:

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

[German M. Bravo]

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.

[German M. Bravo]

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

[Preston Holmes]

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.