Roundup Tracker - Issues

Issue 2550952

classification
schema.py no longer supports arbitrary callables for checks
Type: behavior Severity: normal
Components: API Versions: devel
process
Status: fixed fixed
:
: rouilj : ezio.melotti, rouilj
Priority: normal : Effort-Low

Created on 2017-09-14 20:35 by ezio.melotti, last changed 2017-09-20 02:25 by rouilj.

Messages
msg6015 Author: [hidden] (ezio.melotti) Date: 2017-09-14 20:35
https://sourceforge.net/p/roundup/code/ci/36630a062fb5 introduced a check 
that uses inspect.getargspec().  In our instance we were using a callable 
instance for a check ( https://hg.python.org/tracker/python-
dev/file/545a7d7ef9e4/schema.py#l251 ) and while updating roundup we got 
the failure "TypeError: <__builtin__.may_view_spam instance at 
0x7f2ac7629bd8> is not a Python function" since getargspec() only works 
with Python functions and not arbitrary callables.

We fixed this on our end by replacing the callable class with a function 
that uses a closure to store the klassname, but the fact that arbitrary 
callables are no longer supported is technically a backward incompatible 
change that you might want to fix in Roundup.

FWIW getargspec(inst.__call__) works, but there might be better way to 
fix it.
msg6016 Author: [hidden] (rouilj) Date: 2017-09-14 23:20
Hi Ezio:

In message
<1505421313.77.0.0767788137936.issue2550952@psf.upfronthosting.co.za>,
Ezio Melotti writes:
>
>https://sourceforge.net/p/roundup/code/ci/36630a062fb5 introduced a check 
>that uses inspect.getargspec().  In our instance we were using a callable 
>instance for a check ( https://hg.python.org/tracker/python-
>dev/file/545a7d7ef9e4/schema.py#l251 ) and while updating roundup we got 
>the failure "TypeError: <__builtin__.may_view_spam instance at 
>0x7f2ac7629bd8> is not a Python function" since getargspec() only works 
>with Python functions and not arbitrary callables.

Hmm, from https://bugs.python.org/issue20828 we get something like:

_WrapperDescriptor = type(type.__call__)
_MethodWrapper = type(all.__call__)
_ClassMethodWrapper = type(int.__dict__['from_bytes'])

def get_callable_argspec(fn):
    if inspect.isfunction(fn) or inspect.ismethod(fn):
        inspectable = fn
    elif inspect.isclass(fn):
        inspectable = fn.__init__
    elif hasattr(fn, '__call__'):
        inspectable = fn.__call__
    else:
        inspectable = fn

    if isinstance(fn,
           (_WrapperDescriptor, _MethodWrapper, _ClassMethodWrapper)):
        raise ValueError('unsupported callable {!r}'.format(fn))
    try:
        return inspect.getargspec(inspectable)
    except TypeError:
        raise

This seems to be best, but could it break on python if they once again
deprecate getfullargspec per
https://docs.python.org/3/library/inspect.html

   Changed in version 3.4: This function is now based on signature(),
   but still ignores __wrapped__ attributes and includes the already
   bound first parameter in the signature output for bound methods.

   Changed in version 3.6: This method was previously documented as
   deprecated in favour of signature() in Python 3.5, but that
   decision has been reversed in order to restore a clearly supported
   standard interface for single-source Python 2/3 code migrating away
   from the legacy getargspec() API.

From
https://www.programcreek.com/python/example/87828/inspect.getfullargspec

[From project xonsh-master, under directory xonsh, in source file
 inspectors.py.]

def getargspec(obj):
    """Wrapper around :func:`inspect.getfullargspec` on Python 3, and
    :func:inspect.getargspec` on Python 2.

    In addition to functions and methods, this can also handle objects with a
    ``__call__`` attribute.
    """
        if safe_hasattr(obj, '__call__') and not is_simple_callable(obj):
	        obj = obj.__call__

    return inspect.getfullargspec(obj) if ISPY3K else inspect.getargspec(obj)

They have a fair number of local functions there safe_hasattr and
is_simple_callable but using something like this should fix your
immediate issue right Ezio?

>We fixed this on our end by replacing the callable class with a function 
>that uses a closure to store the klassname, but the fact that arbitrary 
>callables are no longer supported is technically a backward incompatible 
>change that you might want to fix in Roundup.

I don't remember any of the examples in the docs that use anything
other than an actual function. So as you correctly state, technically
it's changed incompatibly.  However it was never advertised that it
would work 8-).

However there is probably no statement that arbitrary callables
shouldn't work so let's try to fix it for your use case at least.

>FWIW getargspec(inst.__call__) works, but there might be better way to 
>fix it.

There are some code ideas above. My thoughts are:

  Add function findargspec to anypy/getarspec_.py that wraps
    getargspec/getfullargspec and checks for __call__ and uses that
    if present.

  Use this function in place of inspect.getargspec in security.py

  Remove the comment to remove the array subscript notation as it looks
    like the field name changes for the 4 fields in common between
    the tuple returned by getargspec and getfullargspec

If I get this done would you be able to add a couple of test cases to
the test suit that exercises both forms of __call__ to see if I got
the code correct?

Have a great day.
msg6017 Author: [hidden] (ezio.melotti) Date: 2017-09-15 00:07
> Hmm, from https://bugs.python.org/issue20828 we get something like:
>
>    ...
>    if isinstance(fn,
>           (_WrapperDescriptor, _MethodWrapper, _ClassMethodWrapper)):
>        raise ValueError('unsupported callable {!r}'.format(fn))
>    ...

This is probably a bit overkill if getargspec is only going to be used 
for "check functions" in schema.py.  I don't think people are going to 
write C extensions or other complex solutions to check for permissions :)

> [From project xonsh-master, under directory xonsh, in source file
>  inspectors.py.]
>
>     ...
>       if safe_hasattr(obj, '__call__') and not is_simple_callable(obj):
>	        obj = obj.__call__
>
>     return inspect.getfullargspec(obj) if ISPY3K else 
inspect.getargspec(obj)

This seems a good compromise between functionality and simplicity and 
should work with callable instances (like the one we had).

>  Add function findargspec to anypy/getarspec_.py that wraps
>    getargspec/getfullargspec and checks for __call__ and uses that
>    if present.
>
>  Use this function in place of inspect.getargspec in security.py

This is an option, especially considering that you are (finally) moving 
on Python 3 :)
However, have you considered other non-LBYL approaches that don't require 
using getargspec()?  Unfortunately the only alternative that comes to 
mind is using a try/except, except that the TypeError you would get is 
not specific enough and you might end up masking unrelated errors -- even 
if you check for the error message.


> If I get this done would you be able to add a couple of test cases to
> the test suit that exercises both forms of __call__ to see if I got
> the code correct?

Right now I'm still busy trying to update our bug tracker, so probably 
not.  Adding these test cases should be quite straightforward though, 
since you can convert any existing:

    def func(args):
        ...

to

    class Foo:
        def __call__(self, args):
            ...

and pass Foo() instead of func.

> Have a great day.

You too :)
msg6018 Author: [hidden] (rouilj) Date: 2017-09-20 02:25
Fixed in commit rev:c94fd717e28c

using modified version of first example in msg6016.
History
Date User Action Args
2017-09-20 02:25:11rouiljsetstatus: new -> fixed
versions: + devel
messages: + msg6018
assignee: rouilj
components: + API
resolution: fixed
2017-09-15 00:07:36ezio.melottisetmessages: + msg6017
2017-09-14 23:20:35rouiljsetnosy: + rouilj
messages: + msg6016
2017-09-14 20:35:13ezio.melotticreate