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

Use __wrapped__ attribute #5077

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lucaswiman
Copy link

Fixes #5065 where the globalns computation in get_typed_signature misses an edge case handled by typing.get_type_hints. This PR makes the logic similar to get_type_hints.

Background

functools.wraps updates the annotations and type signature to match the wrapped function. However, it copies the __annotations__ dict verbatim, including forward references (strings). get_type_hints handles this by dereferencing the __wrapped__ attribute until it gets to the original function, then uses the __globals__ of that function.

Testing

I added a test case which reproduced the NameError seen in #5065. The test passes after my commit updating the implementation of get_typed_signature:

tests/test_wrapped_method_forward_reference.py:24:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
fastapi/routing.py:617: in decorator
    self.add_api_route(
fastapi/routing.py:556: in add_api_route
    route = route_class(
fastapi/routing.py:425: in __init__
    self.dependant = get_dependant(path=self.path_format, call=self.endpoint)
fastapi/dependencies/utils.py:279: in get_dependant
    endpoint_signature = get_typed_signature(call)
fastapi/dependencies/utils.py:249: in get_typed_signature
    typed_params = [
fastapi/dependencies/utils.py:254: in <listcomp>
    annotation=get_typed_annotation(param, globalns),
fastapi/dependencies/utils.py:266: in get_typed_annotation
    annotation = evaluate_forwardref(annotation, globalns, globalns)
pydantic/typing.py:76: in pydantic.typing.evaluate_forwardref
    ???
../../.pyenv/versions/3.9.1/lib/python3.9/typing.py:533: in _evaluate
    eval(self.__forward_code__, globalns, localns),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   NameError: name 'ForwardRef' is not defined

<string>:1: NameError
============================================================== short test summary info ===============================================================
FAILED tests/test_wrapped_method_forward_reference.py::test_wrapped_method_type_inference - NameError: name 'ForwardRef' is not defined
================================================================= 1 failed in 0.59s ==================================================================

@github-actions
Copy link
Contributor

📝 Docs preview for commit cd17a8c at: https://62b5f83b3b5ee400907caa09--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf73051) 100.00% compared to head (d293bb3) 100.00%.
Report is 1038 commits behind head on master.

❗ Current head d293bb3 differs from pull request most recent head c1f1585. Consider uploading reports for the commit c1f1585 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #5077   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       541    +1     
  Lines        13969     13930   -39     
=========================================
- Hits         13969     13930   -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ped__ attribute.

It is guarded by a hasattr check, so it is safe to ignore this error.
@github-actions
Copy link
Contributor

📝 Docs preview for commit c1691f5 at: https://62b5fdb2c1e6e8008a527e31--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit c3591b3 at: https://62b60a4cf0eaae00569dd8dd--fastapi.netlify.app

Copy link
Contributor

@JarroVGIT JarroVGIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, agree with the solution and good work on providing a testcase with proper docstring explaining the issue at hand.

Copy link
Contributor

@uriyyo uriyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have one small suggestion

Copy link
Contributor

@yezz123 yezz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚡️

Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
@github-actions
Copy link
Contributor

📝 Docs preview for commit f412d36 at: https://62b8a0e96d2aa16e090cb5d7--fastapi.netlify.app

Copy link
Contributor

@uriyyo uriyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucaswiman
Copy link
Author

@tiangolo This PR has 3 approvals. Could you please merge this PR or comment about what additional steps I should take prior to merge? Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

📝 Docs preview for commit d293bb3 at: https://6317df3295a60f20eaf20f2e--fastapi.netlify.app

@zachkirsch
Copy link

+1 I'd love for this to merge in!

@dsinghvi
Copy link

dsinghvi commented Nov 1, 2022

@tiangolo what would it take to get this merged in?

@dsinghvi
Copy link

dsinghvi commented Dec 5, 2022

@tiangolo following up again -- pr has 3 approvals, what can we do to get this in?

@ashwin153
Copy link

@tiangolo Would it be possible to get this merged?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

📝 Docs preview for commit c1f1585 at: https://63901889b9f3e837ecb3eca9--fastapi.netlify.app

@zachkirsch
Copy link

@tiangolo is there anything is particular that is blocking this PR from merging?

@jochs
Copy link

jochs commented Apr 28, 2023

@tiangolo hey! is there any way for me to help prioritize this getting in?

FWIW post 0.89 you also need to update get_typed_return_annotation to look like this:

    def get_typed_return_annotation(call: Callable[..., Any]) -> Any:
        signature = inspect.signature(call)
        annotation = signature.return_annotation

        if annotation is inspect.Signature.empty:
            return None

        nsobj = inspect.unwrap(call)
        globalns = getattr(nsobj, "__globals__", {})
        return get_typed_annotation(annotation, globalns)

@tiangolo tiangolo added feature New feature or request p2 and removed investigate labels Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request p2
Projects
None yet
10 participants