Opened 5 years ago

Closed 3 years ago

#22111 closed Bug (fixed)

Signal can throw ValueError in debug mode

Reported by: Ilya Pirogov Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no



// accepts keyword-only arguments 
@receiver(signals.post_save, sender=MyModel)
def my_handler(*, sender, instance, **kwargs):

// contains annotations
@receiver(signals.post_save, sender=MyModel)
def my_handler(sender, instance: MyModel, **kwargs):

In both cases the Signal can throw an error:
ValueError: Function has keyword-only arguments or annotations, use getfullargspec() API which can support them

Pull request:

Change History (6)

comment:1 Changed 5 years ago by Baptiste Mispelon

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Version: 1.6master


I can reproduce the issue indeed.

The approach taken in the pull request looks sane but needs some tests before it can be merged.

And while we're there, I think there's a few other places in Django where getargspec is used. Should those instances be fixed too?


comment:2 Changed 5 years ago by Ilya Pirogov


With tests there was a problem that syntax of annotations and keywords-only arguments is incorrect for Python 2.
I created tests where handlers for both Pythons are imported from different modules:

@skipIf(six.PY2, 'for Python 3 only')
class GetargspecPy3TestCase(SimpleTestCase):
    def test_getargspec(self):
        from .py3_handlers import (handler_with_kwargs_only,
        # ...

@skipIf(six.PY3, 'for Python 2 only')
class GetargspecPy2TestCase(SimpleTestCase):
    def test_getargspec(self):
        from .py2_handlers import handler_simple

        # ...

As far as this decision is correct in Django?

Yes, I think other cases are also affect this problem.
I wrote compatible implementation of getargspec and placed it in django.utils.inspect module. Then I replaced import inspect with this module.
This is an acceptable solution?

I committed all this on github.

comment:3 Changed 5 years ago by Tim Graham

I don't think there's a need to alias all of inspect under django.utils.inspect. Maybe the fix is too specific to Django, but I wonder if it wouldn't be better to try to include this in a Python 2/3 compatibility library like six.

comment:4 Changed 5 years ago by Aymeric Augustin

Yes, we avoid maintaining patched versions of stdlib features at all costs.

comment:5 Changed 4 years ago by Tim Graham

Component: Python 3Core (Other)

comment:6 Changed 3 years ago by vinnyrose

Resolution: fixed
Status: newclosed

Fixed in Django 1.9+ with #24979

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