Opened 10 years ago

Closed 8 years ago

#22111 closed Bug (fixed)

Signal can throw ValueError in debug mode

Reported by: Ilya Pirogov Owned by: nobody
Component: Core (Other) Version: dev
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

Description

Examples:

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


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

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: https://github.com/django/django/pull/2336

Change History (6)

comment:1 by Baptiste Mispelon, 10 years ago

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

Hi,

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?

Thanks.

comment:2 by Ilya Pirogov, 10 years ago

Hello.

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,
                                   handler_with_annotations)
        # ...

@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 by Tim Graham, 10 years ago

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 by Aymeric Augustin, 10 years ago

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

comment:5 by Tim Graham, 9 years ago

Component: Python 3Core (Other)

comment:6 by vinnyrose, 8 years ago

Resolution: fixed
Status: newclosed

Fixed in Django 1.9+ with #24979

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