Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10672 closed (fixed)

Proxy Model does not send save signal

Reported by: zbyte64 Owned by: Nobody
Component: Database layer (models, ORM) Version: 1.1-beta
Severity: Keywords:
Cc: zbyte64@…, gnuk0001@…, valera.grishin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

from django.test import TestCase
from django.db import models

class Test1Model(models.Model):
    field1 = models.CharField(max_length=10)

class TestProxyModel(Test1Model):
    class Meta:
        proxy = True

class ProxySignalTest(TestCase):
    def test_proxy_signal(self):
        from django.db.models import signals
        def save_signal():
            raise Exception()
        signals.post_save.connect(save_signal, sender=TestProxyModel)
        tp = TestProxyModel(field1="test1")
        self.assertRaises(Exception, tp.save)

This test case fails as the save signal never gets called.

Attachments (2)

10672.diff (5.2 KB ) - added by k0001 15 years ago.
10672.2.diff (3.8 KB ) - added by Ulrich Petri 15 years ago.
Updated to trunk (r10836)

Download all attachments as: .zip

Change History (27)

comment:1 by Alex Gaynor, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:2 by medhat, 15 years ago

the last line should be: self.assertRaises(Exception, tp.save())

in reply to:  2 comment:3 by zbyte64, 15 years ago

Replying to medhat:

the last line should be: self.assertRaises(Exception, tp.save())

You're not suppose to call the function in the parameter, or else the function won't catch the exception, here's a simple example:

import unittest

def raise_error():
	assert False

class AssertErrorTestCase(unittest.TestCase):
	def test_if_callable(self):
		self.assertRaises(AssertionError, raise_error)

	def test_if_inline_call(self):
		self.assertRaises(AssertionError, raise_error())

if __name__ == '__main__':
	unittest.main()

The result is "test_if_inline_call" fails while "test_if_callable" passes.

by k0001, 15 years ago

Attachment: 10672.diff added

comment:4 by k0001, 15 years ago

Cc: gnuk0001@… added
Has patch: set
Triage Stage: AcceptedDesign decision needed

I've attached a patch that fixes this issue. Notice we must carry around the value for the "created" argument of the signal: I thought the cleanest (and probably useful) way to do this could be to add a return value to save_base as the patch shows.

Thoughts:

Shouldn't we send both sender=Test1Model and sender=TestProxyModel post_save signals in a situation like the one on the given example? If we take this approach, in what order should we send the signals?

comment:5 by Valera_Grishin, 15 years ago

Cc: valera.grishin@… added

comment:6 by Valera_Grishin, 15 years ago

I have a similar problem though with simpler code (w/o proxy):

from django.db import models
from django.db.models.signals import post_save

class MyModel(models.Model):
    pass

class ChangeLinstener:
    def __init__(self):
        def listener(sender, **kwargs):
            print "MyModel changed"
        post_save.connect(listener, sender=MyModel)

The listener never called. And patch doesn't help.
Is it the same problem OR should I open another ticket?

comment:7 by Alex Gaynor, 15 years ago

That's not a bug, your signal never gets connected unless you instantiate the ChangeLitener.

comment:8 by Valera_Grishin, 15 years ago

Well, I though so too unless following happened to work:

from django.db import models
from django.db.models.signals import post_save

class MyModel(models.Model):
    pass

def listener(sender, **kwargs):
    print "MyModel changed"

class ChangeLinstener:
   def __init__(self):
       post_save.connect(listener, sender=MyModel)

Which looks like hackish design (the "listener" function should be inside ChangeListener).

comment:9 by Alex Gaynor, 15 years ago

That will not work either, a signal will not be called unless it is connected which doesn't happen unless that code is excecuted. Please don't use trac for asking for these support questions, use either #django on freenode or django-users mailing list.

comment:10 by Valera_Grishin, 15 years ago

Ah, sorry. I forgot to add last line of code in both examples above:

ChangeListener = ChangeListener()

That is, I immediately instantiate ChangeListener after it has been defined. But, first example doesn't work while second one does work fine. This seems quite misterious to me since the difference is only the location of "listener" definition (inside or outside of ChangeListener).

P.S. Alex, this is not a support question but a question whether this is the same bug or separate ticket needs to be added. Thanks.

comment:11 by Alex Gaynor, 15 years ago

My guess is the issue is that by default a weak reference is used, and since you overwrite the class references to the inner closure ceases to exist.

comment:12 by Valera_Grishin, 15 years ago

Unfortunatelly, even following:

change_listener = ChangeListener()

works in the same way.

Btw, using "lambda" for defining "listener" function instead of "def" doesn't fix the issue.

comment:13 by Alex Gaynor, 15 years ago

It doens't have anything to do with the name change, it's just that the local function isn't in scope so it ceases to exist, you just need to pass weak=False when you connect the signal.

comment:14 by Valera_Grishin, 15 years ago

It works now! Thanks a lot. Sorry for taking much attention.

comment:15 by Jacob, 15 years ago

Triage Stage: Design decision neededAccepted

I very much don't like the semantics of the save_base return type: usually True/False in a method like save would indicate success or failure, but here it indicates two different types of success. That's an ugly API; I think we need to find another one.

comment:16 by Ulrich Petri, 15 years ago

Owner: changed from nobody to Ulrich Petri

I'll give this a shot.

comment:17 by Ulrich Petri, 15 years ago

Owner: changed from Ulrich Petri to Nobody

I have to leave the sprint pretty soon, so I'm reassigning to nobdy since I didn't come up with any sensible way to imporove this patch

by Ulrich Petri, 15 years ago

Attachment: 10672.2.diff added

Updated to trunk (r10836)

comment:18 by Ulrich Petri, 15 years ago

I updated the patch for current trunk and modified the way the the recursion result is returned so that it doesn't leak into the "public" API of the method (IIRC this was one of the main concerns Jacob expressed at the EuroDjangoCon sprint)

comment:19 by Karen Tracey, 15 years ago

Jacob's objection a few comments up is he very much does not like save base returning True/False to indicate two types of success. I do not see that the new patch changes that behavior?

comment:20 by Ulrich Petri, 15 years ago

The change is that now this behaviour only occurs internally to the method in recursive calls. The outermost (initial) call behaves as before the patch and does not return anything.

comment:21 by Karen Tracey, 15 years ago

I wasn't at the sprint so don't know if that is sufficient to overcome Jacob's objection, all I have to go on is the comment in the ticket, which sounds a bit broader. Personally, having an internal-recursive use of the function that expects/returns a return value while the "public" use does not makes me a bit uneasy, but this is not code I know well enough to really kibitz on.

comment:22 by Ulrich Petri, 15 years ago

Yeah, I agree that its not very nice code, but I see no better way except completely rewriting save_base.

comment:23 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: newclosed

(In [10954]) Fixed #10672 -- Altered save_base to ensure that proxy models send a post_save signal.

comment:24 by ccahoon, 15 years ago

(In [11001]) Fixed #10672 -- Altered save_base to ensure that proxy models send a post_save signal.

comment:25 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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