Skip to content

Comments

Fix #679: allow async class methods as dependencies#681

Merged
tiangolo merged 1 commit intofastapi:masterfrom
frankie567:issue-679
Nov 27, 2019
Merged

Fix #679: allow async class methods as dependencies#681
tiangolo merged 1 commit intofastapi:masterfrom
frankie567:issue-679

Conversation

@frankie567
Copy link
Contributor

This is the fix for issue #679: allow to use async class methods as dependencies.

Before, such methods were not correctly detected as coroutine and thus were not awaited but rather sent to run_in_threadpool. We ended up then injecting the coroutine instead of its result.

I've used inspect.isroutine, instead of doing inspect.isfunction(call) or inspect.ismethod(call). It seems to fulfill our need while saving us a check.

I also took this opportunity to add some unit tests to verify the different use cases regarding class dependencies (callable, async callable, sync methods and async methods).

It could also be interesting to mention those possibilities in the Classes as Dependencies section of the documentation. What do you think?

@frankie567
Copy link
Contributor Author

The CI fails because of linting issues (not related to my changes). Should I fix this or do you prefer a separate PR?

@euri10
Copy link
Contributor

euri10 commented Nov 5, 2019 via email

@frankie567
Copy link
Contributor Author

frankie567 commented Nov 5, 2019

Yeah, they fixed something regarding function parameters/trailing commas. Should be quite straightforward to fix, but maybe it should live in another PR. I'll do that quickly.

EDIT: See #682.

@euri10 euri10 mentioned this pull request Nov 19, 2019
@prostomarkeloff
Copy link
Contributor

prostomarkeloff commented Nov 23, 2019

@tiangolo what's about merging it?

@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #681 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #681    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         271    276     +5     
  Lines        7011   7871   +860     
======================================
+ Hits         7011   7871   +860
Impacted Files Coverage Δ
fastapi/dependencies/utils.py 100% <100%> (ø) ⬆️
tests/test_dependency_class.py 100% <100%> (ø)
...ration_advanced_configurations/test_tutorial003.py 100% <0%> (ø) ⬆️
tests/test_jsonable_encoder.py 100% <0%> (ø) ⬆️
fastapi/openapi/utils.py 100% <0%> (ø) ⬆️
fastapi/dependencies/models.py 100% <0%> (ø) ⬆️
docs/src/response_model/tutorial003.py 100% <0%> (ø) ⬆️
fastapi/concurrency.py 100% <0%> (ø) ⬆️
fastapi/applications.py 100% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5f5e63...bbd736f. Read the comment docs.

@frankie567
Copy link
Contributor Author

Rebased the branch with fixed linting to have the pipeline checked 👍

@tiangolo tiangolo merged commit f3ddc7b into fastapi:master Nov 27, 2019
@tiangolo
Copy link
Member

Great, thank you @frankie567 ! Thanks for implementing it, and adding extended tests. 🤓 🚀 🍰 🎉

And thanks everyone for the discussion.

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.

4 participants