fastapi icon indicating copy to clipboard operation
fastapi copied to clipboard

[Doc][Bug] SQL DB session with middleware: Non effective code

Open jpotyka opened this issue 2 years ago • 1 comments

First Check

  • [X] I added a very descriptive title to this issue.
  • [X] I used the GitHub search to find a similar issue and didn't find it.
  • [X] I searched the FastAPI documentation, with the integrated search.
  • [X] I already searched in Google "How to X in FastAPI" and didn't find any information.
  • [X] I already read and followed all the tutorial in the docs and didn't find an answer.
  • [X] I already checked if it is not related to FastAPI but to Pydantic.
  • [X] I already checked if it is not related to FastAPI but to Swagger UI.
  • [X] I already checked if it is not related to FastAPI but to ReDoc.

Commit to Help

  • [X] I commit to help with one of those options 👆

Example Code

# middleware from the docs db example
# https://fastapi.tiangolo.com/tutorial/sql-databases/#alternative-db-session-with-middleware
# paths in repo:
# docs_src/sql_databases/sql_app/alt_main.py
# docs_src/sql_databases/sql_app_py39/alt_main.py
# docs_src/sql_databases/sql_app_py310/alt_main.py

@app.middleware("http")
async def db_session_middleware(request: Request, call_next):
    response = Response("Internal server error", status_code=500)
    try:
        request.state.db = SessionLocal()
        response = await call_next(request)
    finally:
        request.state.db.close()
    return response

Description

In the docs is an example of how to create a DB session via middleware. In this example, an error 500 response is generated at the beginning. However, this response is never used, because if an exception occurs, this response object is never returned.

One possibility would be to move the return statement into the finally block. However, the problem is that the text in the response text is not displayed in swagger because the content type still has to be defined.

Generally speaking, it is not necessary to return an error 500, as far as I understand. Because this is already handled in Starlette by the ServerErrorMiddleware.

So my suggestion would be to remove the error 500 from this piece of code, as it does nothing at the moment and everything works as it should:

@app.middleware("http")
async def db_session_middleware(request: Request, call_next):
    try:
        request.state.db = SessionLocal()
        return await call_next(request)
    finally:
        request.state.db.close()

Resources

  • https://fastapi.tiangolo.com/tutorial/sql-databases/#alternative-db-session-with-middleware
  • docs_src/sql_databases/sql_app/alt_main.py
  • docs_src/sql_databases/sql_app_py39/alt_main.py
  • docs_src/sql_databases/sql_app_py310/alt_main.py

Operating System

Linux

Operating System Details

OS independent

FastAPI Version

0.81.0

Python Version

3.10.6

Additional Context

Just a small bug in the example code in the doc. If it is desired I can open a PR to fix this bug.

jpotyka avatar Sep 01 '22 14:09 jpotyka

I have created a pull-request to fix that issue: #5872

r0b2g1t avatar Jan 13 '23 09:01 r0b2g1t