Code

Opened 5 months ago

Last modified 5 weeks ago

#22111 new Bug

Signal can throw ValueError in debug mode

Reported by: ilya_pirogov Owned by: nobody
Component: Python 3 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

Attachments (0)

Change History (4)

comment:1 Changed 5 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 5 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 8 weeks 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 5 weeks ago by aaugustin

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.