New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError swallowed in decorated handlers #1530

Closed
lbolla opened this Issue Dec 16, 2016 · 7 comments

Comments

3 participants
@lbolla
Contributor

lbolla commented Dec 16, 2016

If a TypeError is raised within a handler, which has been decorated by a "wrapper" function whose first argument is not called self, the TypeError is swallowed and a 404 HTTP Error is returned instead.

from functools import wraps
import cherrypy


def dec(f):
    @wraps(f)
    def wrapper(handler, *args, **kwargs):
        return f(handler, *args, **kwargs)
    return wrapper


class HelloWorld(object):

    @dec
    def index(self, *args, **kwargs):
        raise TypeError('OOPS')
        return "Hello world!"

    index.exposed = True


if __name__ == '__main__':
    cherrypy.quickstart(HelloWorld())

Response is a 404:

> curl -sD - 'http://localhost:8080/' -o /dev/null
HTTP/1.1 404 Not Found
Server: CherryPy/8.1.2
Content-Length: 1267
Content-Type: text/html;charset=utf-8
Date: Fri, 16 Dec 2016 16:05:18 GMT

Without the decorator @dec, one correctly gets a 500.

The problem is here: https://github.com/cherrypy/cherrypy/blob/master/cherrypy/_cpdispatch.py#L105
When inspecting the "callable", CherryPy checks for the name of the first argument and strips it out if it's called self. It then goes on inspecting the other arguments and raises a 404 if some arguments are missing or unknown.
But if the "callable"'s first argument is not called "self", as in the decorated handler above (where it's called "handler"), then CherryPy mistakenly returns a "404 argument 'handler' is unknown".

@lbolla

This comment has been minimized.

Show comment
Hide comment
@lbolla

lbolla Dec 16, 2016

Contributor

This is the full stacktrace returned in the response body.

<body>
    <h2>404 Not Found</h2>
    <p>Missing parameters: handler</p>
    <pre id="traceback">Traceback (most recent call last):
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 670, in respond
            response.body = self.handler()
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 220, in __call__
            self.body = self.oldhandler(*args, **kwargs)
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 66, in __call__
            raise sys.exc_info()[1]
        HTTPError: (404, 'Missing parameters: handler')
    </pre>
<div id="powered_by">
  <span>
    Powered by <a href="http://www.cherrypy.org">CherryPy 8.1.2</a>
  </span>
</div>
</body>
Contributor

lbolla commented Dec 16, 2016

This is the full stacktrace returned in the response body.

<body>
    <h2>404 Not Found</h2>
    <p>Missing parameters: handler</p>
    <pre id="traceback">Traceback (most recent call last):
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 670, in respond
            response.body = self.handler()
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 220, in __call__
            self.body = self.oldhandler(*args, **kwargs)
        File "/home/lbolla/.virtualenvs/exp/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 66, in __call__
            raise sys.exc_info()[1]
        HTTPError: (404, 'Missing parameters: handler')
    </pre>
<div id="powered_by">
  <span>
    Powered by <a href="http://www.cherrypy.org">CherryPy 8.1.2</a>
  </span>
</div>
</body>
@lbolla

This comment has been minimized.

Show comment
Hide comment
@lbolla

lbolla Dec 16, 2016

Contributor

One option, that would only work on Python3, though, is to check if the "callable" has a __wrapped__ attribute when checking if it's been wrapped or not and call getargspec on __wrapped__ if so:

>>> getargspec(callable.__wrapped__)
(['self'], 'args', 'kwargs', None)

See https://docs.python.org/3/library/functools.html#functools.update_wrapper

For Python2, at leasts the docs should suggest to set the __wrapped__ attribute manually to the original wrapper.

Contributor

lbolla commented Dec 16, 2016

One option, that would only work on Python3, though, is to check if the "callable" has a __wrapped__ attribute when checking if it's been wrapped or not and call getargspec on __wrapped__ if so:

>>> getargspec(callable.__wrapped__)
(['self'], 'args', 'kwargs', None)

See https://docs.python.org/3/library/functools.html#functools.update_wrapper

For Python2, at leasts the docs should suggest to set the __wrapped__ attribute manually to the original wrapper.

@bdeeney

This comment has been minimized.

Show comment
Hide comment
@bdeeney

bdeeney Dec 16, 2016

Contributor

An option that would work for both Python 2/3 would be to use wrapt to create a decorator that preserves the wrapped method's signature.

from functools import wraps
from inspect import getargspec

import wrapt


def dec(f):
    @wraps(f)
    def wrapper(handler, *args, **kwargs):
        return f(handler, *args, **kwargs)
    return wrapper


@wrapt.decorator
def wrapt_dec(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)


class HelloWorld(object):

    @dec
    def index(self, *args, **kwargs):
        raise TypeError('OOPS')
        return "Hello world!"

    @wrapt_dec
    def index2(self, *args, **kwargs):
        raise TypeError('OOPS')
        return "Hello world!"

    index.exposed = True


handler = HelloWorld()
>>> getargspec(handler.index)
ArgSpec(args=['handler'], varargs='args', keywords='kwargs', defaults=None)
>>> getargspec(handler.index2)
ArgSpec(args=['self'], varargs='args', keywords='kwargs', defaults=None)
>>>
Contributor

bdeeney commented Dec 16, 2016

An option that would work for both Python 2/3 would be to use wrapt to create a decorator that preserves the wrapped method's signature.

from functools import wraps
from inspect import getargspec

import wrapt


def dec(f):
    @wraps(f)
    def wrapper(handler, *args, **kwargs):
        return f(handler, *args, **kwargs)
    return wrapper


@wrapt.decorator
def wrapt_dec(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)


class HelloWorld(object):

    @dec
    def index(self, *args, **kwargs):
        raise TypeError('OOPS')
        return "Hello world!"

    @wrapt_dec
    def index2(self, *args, **kwargs):
        raise TypeError('OOPS')
        return "Hello world!"

    index.exposed = True


handler = HelloWorld()
>>> getargspec(handler.index)
ArgSpec(args=['handler'], varargs='args', keywords='kwargs', defaults=None)
>>> getargspec(handler.index2)
ArgSpec(args=['self'], varargs='args', keywords='kwargs', defaults=None)
>>>
@lbolla

This comment has been minimized.

Show comment
Hide comment
@lbolla

lbolla Dec 16, 2016

Contributor

@bdeeney Sure, applications aware of this issue could write their decorators using wrapt and get the correct behavior. But unaware applications won't know that they have to use wrapt... So I still think that CherryPy should try a bit harder to return the correct error.
Probably, rather than the argument name, CherryPy should check the actual first argument value and try to deduct if it's self.

Contributor

lbolla commented Dec 16, 2016

@bdeeney Sure, applications aware of this issue could write their decorators using wrapt and get the correct behavior. But unaware applications won't know that they have to use wrapt... So I still think that CherryPy should try a bit harder to return the correct error.
Probably, rather than the argument name, CherryPy should check the actual first argument value and try to deduct if it's self.

@bdeeney

This comment has been minimized.

Show comment
Hide comment
@bdeeney

bdeeney Dec 16, 2016

Contributor

@lbolla Agreed. A more accurate check might focus on, e.g.:

inspect.ismethod(callable)

ref: https://docs.python.org/3/library/inspect.html#inspect.ismethod

Contributor

bdeeney commented Dec 16, 2016

@lbolla Agreed. A more accurate check might focus on, e.g.:

inspect.ismethod(callable)

ref: https://docs.python.org/3/library/inspect.html#inspect.ismethod

@lbolla

This comment has been minimized.

Show comment
Hide comment
@lbolla

lbolla Dec 16, 2016

Contributor

@bdeeney Thanks for the suggestion, it's a great idea. I just tested it and it does indeed solves the issue elegantly. I will submit a PR soon.

Contributor

lbolla commented Dec 16, 2016

@bdeeney Thanks for the suggestion, it's a great idea. I just tested it and it does indeed solves the issue elegantly. I will submit a PR soon.

lbolla added a commit to lbolla/cherrypy that referenced this issue Dec 16, 2016

@lbolla

This comment has been minimized.

Show comment
Hide comment
@lbolla

lbolla Dec 16, 2016

Contributor

PR available here: #1531

Contributor

lbolla commented Dec 16, 2016

PR available here: #1531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment