Skip to content
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

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

Merged
merged 2 commits into from
Apr 8, 2021
Merged

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

merged 2 commits into from
Apr 8, 2021

Conversation

gcalmettes
Copy link
Contributor

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
Copy link
Owner

smarie commented Apr 8, 2021

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
Copy link
Contributor Author

gcalmettes commented Apr 8, 2021

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
Copy link
Owner

smarie commented Apr 8, 2021

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

@gcalmettes
Copy link
Contributor Author

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
Copy link
Owner

smarie commented Apr 8, 2021

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.

makefun/main.py Outdated Show resolved Hide resolved
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
@gcalmettes
Copy link
Contributor Author

Done, thanks !

@smarie smarie merged commit 3133a9d into smarie:main Apr 8, 2021
@smarie
Copy link
Owner

smarie commented Apr 8, 2021

Thanks @gcalmettes, I'll make a release now

smarie pushed a commit that referenced this pull request Apr 8, 2021
…l(repr(v))` raises an exception, created signatures would raise an exception instead of automatically protecting the symbol. PR #67
@smarie
Copy link
Owner

smarie commented Apr 8, 2021

I created a test for future reference

def test_issue_pr_67():

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

@gcalmettes
Copy link
Contributor Author

Thanks for your reactivity @smarie !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants