Opened 6 years ago

Closed 6 years ago

Last modified 4 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: UI/UX:

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 6 years ago.
10672.2.diff (3.8 KB) - added by UloPe 6 years ago.
Updated to trunk (r10836)

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by Alex

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

comment:2 follow-up: Changed 6 years ago by medhat

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

comment:3 in reply to: ↑ 2 Changed 6 years ago by zbyte64

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.

Changed 6 years ago by k0001

comment:4 Changed 6 years ago by k0001

  • Cc gnuk0001@… added
  • Has patch set
  • Triage Stage changed from Accepted to Design 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 Changed 6 years ago by Valera_Grishin

  • Cc valera.grishin@… added

comment:6 Changed 6 years ago by Valera_Grishin

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 Changed 6 years ago by Alex

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

comment:8 Changed 6 years ago by Valera_Grishin

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 Changed 6 years ago by Alex

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 Changed 6 years ago by Valera_Grishin

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 Changed 6 years ago by Alex

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 Changed 6 years ago by Valera_Grishin

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 Changed 6 years ago by Alex

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 Changed 6 years ago by Valera_Grishin

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

comment:15 Changed 6 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 6 years ago by UloPe

  • Owner changed from nobody to UloPe

I'll give this a shot.

comment:17 Changed 6 years ago by UloPe

  • Owner changed from UloPe 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

Changed 6 years ago by UloPe

Updated to trunk (r10836)

comment:18 Changed 6 years ago by UloPe

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 Changed 6 years ago by kmtracey

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 Changed 6 years ago by UloPe

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 Changed 6 years ago by kmtracey

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 Changed 6 years ago by UloPe

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

comment:23 Changed 6 years ago by russellm

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

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

comment:24 Changed 6 years ago by ccahoon

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

comment:25 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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