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

Allow access to the path operation's Response from within Dependencies using yield #3500

Open
9 tasks done
adriangb opened this issue Jul 7, 2021 · 8 comments
Open
9 tasks done
Labels
confirmed feature New feature or request reviewed

Comments

@adriangb
Copy link
Contributor

adriangb commented Jul 7, 2021

Checks

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but to Pydantic.
  • I already checked if it is not related to FastAPI but to Swagger UI.
  • I already checked if it is not related to FastAPI but to ReDoc.
  • After submitting this, I commit to:
    • Read open issues with questions until I find 2 issues where I can help someone and add a comment to help there.
    • Or, I already hit the "watch" button in this repository to receive notifications and I commit to help at least 2 people that ask questions in the future.
    • Implement a Pull Request for a confirmed bug.

Problem

FastAPI's dependency injection system supports generators as dependencies (docs).

But there is currently no way to access a response generated by the path function in dependencies.
Well, you can access a response object, but it doesn't get merged with the response from the path operation.
Here's a minimal example:

from fastapi import Depends, FastAPI, Response
from fastapi.testclient import TestClient


def dependency(response: Response):
    yield
    assert response.status_code is None  # so not the same response object!


app = FastAPI(dependencies=[Depends(dependency)])

@app.get("/")
def root():
    return Response(status_code=400)


client = TestClient(app)
res = client.get("/")

If your path operation accepts a Response object and modifies and returns the same object,
then you can access data from the path operation (like status code) in the dependency.
Otherwise, you cannot.

Use case

The main use case I have for accessing this data is logging, but I imagine there may be others.

Proposed solution

I think that access to the final Response object could be enable via the generator's send functionality:

def dependency():
    response = yield
    assert response.status_code == 400

I think the main complication with this will be how FastAPI is using these generators internally.
I believe they are being wrapped in context managers, which means the ability to use .send() is lost.

Alternatives

Changing how responses are merged

I know somewhere within FastAPI Response objects get merged together.
I'm not sure if it's possible, but an alternative might be to merge into the dependency's response object instead of creating a new object or merging into the path operation's response (I am not sure which is the current behavior).
This may or may not be a breaking change, I'm not sure.

Middleware

For the particular use case of logging, it is possible to use a middleware.
But middleware's are in some ways not as flexible as dependencies.
For example, you can't apply a middleware to just some paths (for example, to avoid logging data on a path that might handle sensitive information) unless you resort to "give me a list of path regexes as a constructor parameter" in your middleware.

@adriangb adriangb added the feature New feature or request label Jul 7, 2021
@rotten
Copy link

rotten commented Jul 7, 2021

Could you use a decorator? Maybe stick it between the @app.get("/") decorator and the def root() function call?

@adriangb
Copy link
Contributor Author

adriangb commented Jul 7, 2021

Maybe? But then you can't use it with routers or apps (you can have router or app level dependencies)

Using a random decorator would be a system outside of FastAPI DI, and I think there'd be a lot of integrations (like the above) that the DI system has that would be lost.

@adriangb
Copy link
Contributor Author

Since this does not seem to have gained much traction, I am going to close this to let maintainers focus on long standing bugs

@tiangolo
Copy link
Member

I have been thinking about this recently, and I think it makes sense.

The main problem is that the current implementation runs the exit code of the dependencies after the background tasks, which means it's all done after the response is sent.

The main idea was to allow using the same DB session in background tasks. But now I think that might not be as common, and might actually be even problematic, as the session would be held even while waiting for the network to return the response. For the specific use case of background tasks, it would probably make more sense to create a new DB session.

And then dependencies could do a lot more things, modifying the response, logging, catching exceptions, setting headers and cookies, etc.

This would be a somewhat strong change, but I currently think it makes sense (even though before I thought the contrary).

Given that, I'm gonna open this again. Because unless I see something that would obviously break and be problematic for a lot of use cases, I think this should actually be the way to go.

Sorry for the long delay! 🙈 I wanted to personally address each issue/PR and they piled up through time, but now I'm checking each one in order.

@elonzh
Copy link

elonzh commented Apr 20, 2023

Currently, if the endpoint returns a Response object, the Response changes made by dependence functions will be ignored silently. It's obscure and may cause problems.

See Custom response does not include headers set in dependencies · Issue #5498 · tiangolo/fastapi

For example, if we want to implement an endpoint level rate limiter, it depends on some sub-dependencies like user, request and it checks the request limit in the function body and sets the X-RateLimit-Limit header to the Response. The header should be mandatory but it can be ignored by accident.

@k0t3n
Copy link
Contributor

k0t3n commented Jan 25, 2024

It could be really useful for cache implementation.

@richarddevers
Copy link

+1 , would be nice to have it. Use case here is to generate a transaction id, log the incoming request+transaction id, use the transaction_id in the path code, then log response+transaction_id

@MarcBresson
Copy link

My use case is also about logging (and I quite like the idea from @richarddevers). We have liveliness and readiness endpoints that flood our logs and I would like to exclude them from the logging middleware.

I'll check https://www.starlette.io/middleware/#applying-middleware-to-groups-of-routes in the meantime.

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