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

Add support for using context managers as a dependency #10353

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

Conversation

notpushkin
Copy link

@notpushkin notpushkin commented Sep 29, 2023

Background

Currently, you can't pass a context manager to Depends(), as it would treat it as a function. This means that if you want to use a function both as a dependency in FastAPI and a context manager (e. g. in scripts or Alembic migrations etc), you need to define both versions:

# For FastAPI:
async def get_session():
    async with AsyncSession(engine) as session:
        yield session

# For scripts:
get_session_mgr = asynccontextmanager(get_session)

Unfortunately, as @a-recknagel pointed out in #8955 (comment), it's impossible to check whether given function is a context manager (defined with a decorator from contextlib) or not:

@asynccontextmanager
async def get_session():
    yield 42

get_session.__aenter__  # => AttributeError: 'function' object has no attribute '__aenter__'

It is however possible to check that after we call the function:

called = get_session()  # => <contextlib._AsyncGeneratorContextManager at ...>

if hasattr(called, "__aenter__"):  # => True
    solved = await stack.enter_async_context(called)
else:
    solved = await called

This PR implements that check (for async context managers only for now – sync version would require doing some threadpool magic I'm still looking into :-)

@tiangolo tiangolo added feature New feature or request investigate labels Oct 2, 2023
@iudeen
Copy link
Contributor

iudeen commented Oct 3, 2023

Interesting...

@notpushkin
Copy link
Author

notpushkin commented Oct 4, 2023

I think I've figured it out.

Both sync and async context manager functions are called synchronously, so both of them can be resolved with run_in_threadpool. We can then check if the result has either the __aenter__ or __enter__ method, and either pass it to enter_async_context directly or wrap it with contextmanager_in_threadpool first. If it has neither of those attributes, we can pass it down as a value.

I've updated my branch with the above logic implemented. Here's an example app: https://gist.github.com/notpushkin/cfa23e4ab8a73ef9505e2b6e6b2dca5c

Still need to figure out parameters and caching, and add some tests.

@notpushkin
Copy link
Author

notpushkin commented Oct 4, 2023

Parameters (sub-dependencies) seem to work! Caching shouldn't be an issue, too (given “generator” dependencies are handled in the same way).

Any ideas how to test this?

@a-recknagel
Copy link

Cool feature, thanks for tackling this.

It is however possible to check that after we call the function

Clever, I wasn't aware that there is a window where it's possible to disambiguate between functions and context managers without it already having been necessary to use context-manager-specifc syntax.

@notpushkin
Copy link
Author

I'm back! Let's get this sorted out. I've rebased on master, looking into tests now.

@notpushkin
Copy link
Author

notpushkin commented Dec 5, 2023

I've added a couple basic tests and updated docs to mention that you can now use context managers directly.

I'm not sure how to run doc tests though: when I run python ./scripts/docs.py live, I don't see any errors. How do I proceed from here? (I can solve the ruff errors for now though.)

@tiangolo
Copy link
Owner

tiangolo commented Dec 5, 2023

📝 Docs preview for commit d93c701 at: https://683c36c4.fastapitiangolo.pages.dev

@tiangolo
Copy link
Owner

📝 Docs preview for commit 736f544 at: https://87d81350.fastapitiangolo.pages.dev

@notpushkin
Copy link
Author

I've added an ignore rule for the docs, since DBSession is an example here and not something we actually use.

Coverage is now failing at 99%, but the lines that aren't covered are in a completely different part of the codebase: https://smokeshow.helpmanual.io/4z4c2y1i1r2j495l4a0i/d_6de190ce15502a91_models_py.html

@notpushkin
Copy link
Author

@iudeen Would you happen to know what are next steps to get this reviewed and (hopefully!) merged? Sorry if you're not the right person to ask, I just see that you're a Contributor on this repo so I was hoping that you can point me in the right direction :-)

@tiangolo
Copy link
Owner

📝 Docs preview for commit b9bb157 at: https://e99a602f.fastapitiangolo.pages.dev

@notpushkin
Copy link
Author

@tiangolo Would love to hear your thoughts about this! Let me know if you'd like any changes made (apart from resolving the conflicts). If this isn't something you think we should merge in at this time, please do let me know, too. 😊

@alejsdev alejsdev added p3 and removed investigate labels Jan 15, 2024
@Anton-Constructor
Copy link

Conflicts:
image

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

Successfully merging this pull request may close these issues.

None yet

6 participants