fastapi icon indicating copy to clipboard operation
fastapi copied to clipboard

Use __wrapped__ attribute

Open lucaswiman opened this issue 2 years ago • 14 comments

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 ==================================================================

lucaswiman avatar Jun 24 '22 17:06 lucaswiman

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

github-actions[bot] avatar Jun 24 '22 17:06 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 24 '22 17:06 codecov[bot]

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

github-actions[bot] avatar Jun 24 '22 18:06 github-actions[bot]

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

github-actions[bot] avatar Jun 24 '22 19:06 github-actions[bot]

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

github-actions[bot] avatar Jun 26 '22 18:06 github-actions[bot]

@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!

lucaswiman avatar Sep 07 '22 00:09 lucaswiman

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

github-actions[bot] avatar Sep 07 '22 00:09 github-actions[bot]

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

zachkirsch avatar Oct 13 '22 23:10 zachkirsch

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

dsinghvi avatar Nov 01 '22 05:11 dsinghvi

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

dsinghvi avatar Dec 05 '22 23:12 dsinghvi

@tiangolo Would it be possible to get this merged?

ashwin153 avatar Dec 07 '22 04:12 ashwin153

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

github-actions[bot] avatar Dec 07 '22 04:12 github-actions[bot]

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

zachkirsch avatar Dec 28 '22 16:12 zachkirsch

@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)

jochs avatar Apr 28 '23 17:04 jochs