smarie / python-makefun

Dynamically create python functions with a proper signature.
https://smarie.github.io/python-makefun/
BSD 3-Clause "New" or "Revised" License
119 stars 16 forks source link

[FIX] Prevent signature symbol to break on TypeError #67

Closed gcalmettes closed 3 years ago

gcalmettes commented 3 years ago

Hi,

First of all, thanks for makefun, very useful ! I came into the situation where I had to modify the signature of a FastAPI endpoint having Body(default="") as default value for one of the parameter. Any value other than the empty string for the default value of Body would work, but somehow having an empty string leads to a TypeError in the checking for proctection need of the symbol.

line 370, in _signature_symbol_needs_protection return eval(repr(symbol), evaldict) != symbol File "<string>", line 1, in <module> TypeError: Body() missing 1 required positional argument: 'default'

This PR fixes this by allowing the handling of TypeError in the _signature_symbol_needs_protection function.

Minimal example of breaking code before the PR:

from fastapi import Body
from makefun import wraps, add_signature_parameters
import inspect

def my_function(a: str, b: str = Body(default="")) -> str:
    return "hello"

fn_signature = inspect.signature(my_function)

modified_fn_sig = add_signature_parameters(
    fn_signature,
    last=inspect.Parameter(
        "new_param",
        kind=inspect.Parameter.POSITIONAL_OR_KEYWORD,
        default='abc',
    ),
)

@wraps(my_function, new_sig=modified_fn_sig)
def wrapped_function(
    *args,
    new_param= "abc",
    **kwargs,
):
    return True
smarie commented 3 years ago

Thanks @gcalmettes ! I am not very sure that this exception is a bug from makefun. It seems rather that the default value used is something that raises an exception when evaluated.

Does Body(default="") run correctly outside of the signature ?

gcalmettes commented 3 years ago

Hi @smarie ,

Yes, Body(default="") runs correctly if not applying the @wraps from makefun.

Actually, using @wraps from functools works without error. Below an even smaller exemple.

This won't raise a TypeError:

from fastapi import Body
from functools import wraps

def my_function(a: str, b: str = Body(default="")) -> str:
    return "hello"

@wraps(my_function)
def wrapped_function(
    *args,
    **kwargs,
):
    return True

This will raise a TypeError:

from fastapi import Body
from makefun import wraps

def my_function(a: str, b: str = Body(default="")) -> str:
    return "hello"

@wraps(my_function)
def wrapped_function(
    *args,
    **kwargs,
):
    return True
smarie commented 3 years ago

I do not have a local fastapi environment, can you please confirm that eval(repr(Body(default=""))) raises this same error ?

gcalmettes commented 3 years ago

I confirm indeed

Python 3.8.6 (default, Oct 25 2020, 09:07:46)
[Clang 12.0.0 (clang-1200.0.31.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from fastapi import Body
>>> eval(repr(Body(default="")))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
TypeError: Body() missing 1 required positional argument: 'default'
>>>
smarie commented 3 years ago

Ok then. I think that we need to extend the scope here to catching general Exception, so that other such issues do not appear in the future.

gcalmettes commented 3 years ago

Done, thanks !

smarie commented 3 years ago

Thanks @gcalmettes, I'll make a release now

smarie commented 3 years ago

I created a test for future reference

https://github.com/smarie/python-makefun/blob/5090d44288e1874a0c5b2caf1cc2c533feea417d/makefun/tests/test_issues.py#L189

This is now fixed in 1.11.3, released now. Thanks again @gcalmettes !

gcalmettes commented 3 years ago

Thanks for your reactivity @smarie !