Opened 13 months ago

Last modified 3 months ago

#22111 new Bug

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

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 (5)

comment:1 Changed 13 months ago by bmispelon

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.6 to master

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 Changed 13 months ago by ilya_pirogov

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 Changed 10 months ago by timo

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 10 months ago by aaugustin

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

comment:5 Changed 3 months ago by timgraham

  • Component changed from Python 3 to Core (Other)
Note: See TracTickets for help on using tickets.
Back to Top