Marshmallow: Function fields break with Python 3 type annotations

[martinstein]

Let's take the following simple test case:

from marshmallow import fields, Schema

def get_split_words(value: str):
    return value.split(';')

class TestSchema(Schema):
    name = fields.String(required=True)
    friends = fields.Function(serialize=None, deserialize=get_split_words)

data = {'name': 'Bruce Wayne', 'friends': 'Clark;Alfred;Robin'}
result = TestSchema().load(data)

That results in an error:

Traceback (most recent call last):
  File "mmtest.py", line 14, in <module>
    result = TestSchema().load(data)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\schema.py", line 542, in load
    result, errors = self._do_load(data, many, partial=partial, postprocess=True)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\schema.py", line 610, in _do_load
    index_errors=self.opts.index_errors,
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\marshalling.py", line 294, in deserialize
    index=(index if index_errors else None)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\marshalling.py", line 67, in call_and_store
    value = getter_func(data)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\marshalling.py", line 287, in <lambda>
    data
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\fields.py", line 263, in deserialize
    output = self._deserialize(value, attr, data)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\fields.py", line 1199, in _deserialize
    return self._call_or_raise(self.deserialize_func, value, attr)
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\fields.py", line 1203, in _call_or_raise
    if len(utils.get_func_args(func)) > 1:
  File "C:\tools\py35-venv\lib\site-packages\marshmallow\utils.py", line 344, in get_func_args
    return inspect.getargspec(func).args
  File "C:\tools\Python35-32\lib\inspect.py", line 1045, in getargspec
    raise ValueError("Function has keyword-only arguments or annotations"
ValueError: Function has keyword-only arguments or annotations, use getfullargspec() API which can support them

The reason is: The marshmallow.utils module uses Python's inspect.getargspec for inspecting the functions used in Function fields. However, getargspec is deprecated since Python 3.0 (https://docs.python.org/3/library/inspect.html#inspect.getargspec) and throws a ValueError when called on a function with type annotations.

The error message suggests to use getfullargspec instead, but that has been deprecated since version 3.5, too (https://docs.python.org/3/library/inspect.html#inspect.getfullargspec). So the most future-proof way is probably to use inspect.signature instead (https://docs.python.org/3/library/inspect.html#inspect.signature).

[sabinem]

I would like to work on this, if that's okay with the community. I am a Newbie with contributing, so tell me if I am doing anything the wrong way. But I have been told that you should announce if you want to work on something. This seems like a bug and should be not too hard to fix.

[lafrech]

You don't have to announce each time you want to work on something, but it is advised to do so as soon as the task is not trivial, to avoid wasting your time on a feature bound to be rejected.

It would be a shame to come up with a nice well-coded, well-tested, pull-request that would be rejected because it does not follow the project's objectives/roadmap.

On the other hand, you'll generally reach more attention if you can propose a pull-request, so when the fix is easy, you might as well code something, even just a proof-of-concept, and send it as a PR for feedback. It does not have to be perfectly polished, as long as you introduce it as a beta version.

In this case, the comments in @martinstein about the deprecation of getargspec in Python3 seem wise, so that's a good start. Before getting your hands dirty in the code, you may want to get approval from @sloria and/or other contributors that you're on the right path. Unless this is a much more trivial change than I expect and you can quickly come up with something to show as an implementation example.

[sabinem]

@lafrech: Thank you so much, yes I will first figure out a solution and propose it here for approval, before I start implementing anything.

[sabinem]

Before I work on a pull request, I wanted to get approval for my solution. Feel free to correct me if I am getting anything wrong, since this will be my first real bug fix in open source:
I studied the code and this here is the function to be replaced in Python 3:
in marshmellow.utils:

def get_func_args2(func):
    """Given a callable, return a tuple of argument names. Handles
    `functools.partial` objects and class-based callables.
    """
    if isinstance(func, functools.partial):
        return inspect.getargspec(func.func).args
    if inspect.isfunction(func) or inspect.ismethod(func):
        return inspect.getargspec(func).args
    # Callable class
    return inspect.getargspec(func.__call__).args

It is called only once in marshmellow.fields:

def _call_or_raise(self, func, value, attr):
        if len(get_func_args(func)) > 1:
            if self.parent.context is None:
                msg = 'No context available for Function field {0!r}'.format(attr)
                raise ValidationError(msg)
            return func(value, self.parent.context)
        else:
            return func(value)

The problem

In Python 3 inspect.getargspec(func).args is replaced by inspect.signature(func).parameters. They work differently and signature is kind of smarter as it also can deal with partials.

Proposed solution

Because of these differences in the two functions, I would build a new get_func_arg for Python 3 and put it into marshmellow.utils and then I would have marshmellow.compat make the switch which function to use.
My new function looks like this:

def get_func_args3(func):
    """Given a callable, return a tuple of argument names. Handles
    `functools.partial` objects and class-based callables.
    """
    if inspect.isfunction(func) or inspect.ismethod(func) or isinstance(func, functools.partial):
        return inspect.signature(func).parameters
    elif inspect.isclass(func) and callable(func):
        return inspect.signature(func.__call__).parameters

I did not like the else branch in the old function and rather made that more specific, would that be okay? Or should there still be an else branch, that raises an error?

in marshmellow.compat I would add this branching:

if PY2:
    ... 
    from marshmallow.utils import get_func_args2 as get_func_args
else:
    ... 
    from marshmallow.utils import get_func_args3 as get_func_args

Approval or Feedback

What do you think: shall I go ahead with this, or can you please give me some feedback and tell me some other way to solve this?

[lafrech]

Looks like a good start to me.

I'm not sure the get_func_args3 implementation would be integrated as is, but now that you've got the code done, you may send it as a PR. It makes it easier to review/comment.

elif inspect.isclass(func) and callable(func):

No need for elif, here, since there's a return statement in the case before. if is enough.

I don't like the if/elif without default case. I don't know which is better, a default case returning an error or just assume the last case is default an let it crash. Or assume the last case if default and protect it in a try except. I'm pretty sure the maintainers here will have an educated opinion about this.

Also, you may be asked to add a test that fails with current code and passes with your fix.

Feel free to correct me if I am getting anything wrong, since this will be my first real bug fix in open source

You're doing well. Keep up the good work.

[sabinem]

@lafrech: Thanks so much! I will do it the way you proposed and make the PR. Thanks for your feedback and encouragement.

[sloria]

This is now fixed in the latest PyPI release (2.10.4).