Automatically run `session.commit()` in session dependency BEFORE returning request

[anguspmitchell]

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

# orm.py
def fastapi_session_scope() -> sqlalchemy.orm.session.Session:
    session = session_maker(bind=engine)
    try:
        yield session
        session.commit()
    except:
        session.rollback()
        raise
    finally:
        session.close()

# routers.py
@router.post("/v1/organizations", response_model=Organization)
def create_org(session: Session = Depends(orm.fastapi_session_scope)):
    org = sqlalc.Organization(name="test")
    session.add(org)
    return org

@router.get("/v1/organizations", response_model=List[Organization])
def get_orgs(session: Session = Depends(orm.fastapi_session_scope)):
    return session.query(sqlalc.Organization).all()

Description

(I realize this is diverging slightly from the SQLAlchemy example, which explicitly uses session.commit() in the body of the function and not in the session dependency.)

I'm creating a row with a POST and then immediately afterwards fetching all rows with a GET. I've got session.commit() in the session scope dependency, which is convenient because it runs a commit() at the end no matter what and doesn't require us to always explicitly commit at the end of an endpoint body.

As written, if an Organization is created via a POST and then all the organizations are immediately afterwards fetched via a GET, the GET will not include the recently created Organization. This is not a JS async issue. What's happening, as best I can tell, is that the POST starts a transaction, org is returned, and then, before the commit() actually happens, the GET is called and the list of organizations is returned without the organization that was just created (or so it seemed).

t  POST            GET

1  request
2  session.add
3  return
4                  request
5                  session.query
6                  return          # POSTed organization is missing!
7  session.commit
8                  session.commit

The behavior I'd like is for commit() to run BEFORE the value is returned, which would guarantee that the POST and GET could be run back-to-back safely. Is that possible using a FastAPI dependency? Has anyone else run into this issue?

t  POST            GET

1  request
2  session.add
3  session.commit
4  return
5                  request
6                  session.query
7                  session.commit
8                  return

Operating System

macOS

Operating System Details

No response

FastAPI Version

0.59.0

Python Version

3.8.10

Additional Context

No response

[waynerv]

As the documentation says, the statement after yield will be executed after the response is returned, so you should consider putting the commit statement in the function body

[anguspmitchell]

@waynerv Got it, thanks. It sounds like the options are either 1) to use Depends() and always specify commit() in the endpoint body or 2) to use a context manager, an explicit with statement and an implicit commit() (like the following)

# orm.py
@contextmanager
def session_scope(expire_on_commit: bool = True) -> sqlalchemy.orm.session.Session:
    orm_session = _orm_session(expire_on_commit=expire_on_commit)
    try:
        yield orm_session
        orm_session.commit()
    except:
        orm_session.rollback()
        raise
    finally:
        orm_session.close()

# routers.py
@router.post("/v1/organizations", response_model=Organization)
def create_org():
    with orm.session_scope() as session:
        org = sqlalc.Organization(name="test")
        session.add(org)
        return Organization.from_orm(org)

[AlexanderPodorov]

@anguspmitchell , I think you should avoid dependencies like this:

@contextmanager
def session_scope(expire_on_commit: bool = True) -> sqlalchemy.orm.session.Session:
    orm_session = _orm_session(expire_on_commit=expire_on_commit)
    try:
        yield orm_session
        orm_session.commit()
    except:
        orm_session.rollback()
        raise
    finally:
        orm_session.close()

For those reasons:

1. as @waynerv said, the statement after yield will be executed after the response is returned. So that means, in some circumstances you will send OK response, but commit will actually fail.
2. If you raise HTTPException from endpoint function body, then session will be commited too. This kind of exception won't be caught by try-except of session_scope function.

P.S. I also tried to use the same approach in the very beginning of my work with FastAPI, but finally I'm confident it's much better to commit explicitly at the end of endpoint function body.

[anguspmitchell]

@AlexanderPodorov Very good points.

I may not have been entirely clear on this part:

It sounds like the options are either 1) to use Depends() and always specify commit() in the endpoint body or 2) to use a context manager, an explicit with statement and an implicit commit()

What I'm proposing in (2) is just to not use a FastAPI dependency at all, and instead to use an explicit with block in the function body. (More verbose obviously, but, alas, tradeoffs.) I really do not want to run into the problems that you mentioned, though. To make sure those issues don't occur with this approach, I added two lines of code to test it. From what I can tell, neither of dangers you mentioned come up when using an explicit with block:

# orm.py
@contextmanager
def session_scope(expire_on_commit: bool = True) -> sqlalchemy.orm.session.Session:
    orm_session = _orm_session(expire_on_commit=expire_on_commit)
    try:
        yield orm_session
        raise   # <- this causes a 500 error to be returned, 
                #    so it doesn't appear to run the risk of returning OK
                #    but then silently failing after the yield statement
        orm_session.commit()
    except:
        orm_session.rollback()
        raise
    finally:
        orm_session.close()

# routers.py
@router.post("/v1/organizations", response_model=Organization)
def create_org():
    with orm.session_scope() as session:
        org = sqlalc.Organization(name="test")
        session.add(org)
        raise HTTPException(status_code=404, detail="Item not found") # <- this returns a 404
                    # and causes the rollback in the context manager to be executed, 
                    # avoiding the possibility of a commit after an HTTPException raise
        return Organization.from_orm(org)

[AlexanderPodorov]

Yes, I think this approach should work. And yes, you can create a session in the endpoint function. My 1. and 2. points are related to dependency approach. BTW, as of SQLAlchemy 1.4 we don't need to write our own context managers for sessions/transactions, it should work out of box. See: https://docs.sqlalchemy.org/en/14/orm/session_transaction.html Example for their site:

Session = sesssionmaker(engine)

with Session() as session:
    with session.begin():
        session.add(some_object)
    # commits

# closes the Session

[arsenron]

You can add database session to starlette.Request.state in get_db dependency and wrap all your requests with a middleware that checks if starlette.Request.state contains your database session and if so, commit it or whatever you want. I wrote for an async database but it can be applied to sync. For example:

import uvicorn
from fastapi import FastAPI, Request, Depends
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.orm import sessionmaker

from database import engine  # some engine

app = FastAPI()

@app.middleware("http")
async def commit_db_session(request: Request, call_next):
    """
    Commit db_session before response is returned.
    """
    response = await call_next(request)
    db_session = request.state._state.get("db_session")  # noqa
    if db_session:
        await db_session.commit()
    return response


async_session = sessionmaker(
    bind=engine,
    class_=AsyncSession,
    expire_on_commit=False,
)


async def get_db(request: Request) -> AsyncSession:
    session: AsyncSession = async_session()
    # add session to starlette.Request.state
    request.state.db_session = session
    yield session


@app.get("/")
async def check_db(db: AsyncSession = Depends(get_db)):
    return {"message": "db session is closed automatically before returning response"}


if __name__ == "__main__":
    uvicorn.run(app)

[skion]

I am aware this behaviour is documented, but not being able to raise exceptions after yielding in dependencies does seem like a fantastic gun to shoot yourself in the foot with. The risk is of course that the API (already) returns a positive response to the user, while the data is not yet written to the database, which might go unnoticed for a long time and lead to data sporadically disappearing on well-timed connection issues for instance.

The current behaviour also seems to be at friction with SQLAlchemy's documentation on session lifespan management.

Besides being unexpected (at least for some), there does not appear to be an obvious workaround. The workaround that @arsenron proposes is of course excellent (and I'm using that now), but also very asymmetric and brittle because of the logic-split.

The alternative of placing explicit session.commit()'s in your views is also risky in my opinion, since if you forget one then you risk losing already acknowledged data once again.

I believe that this is exactly where a framework should prevent the developer from doing something silly. The whole point of using yield in a dependency is so that you can do things after it, committing data (and handling its exceptions properly) being one of those things.

As an anecdotal proof-point of how unexpected this is, a quick search indicates that for example FastAPI-Restful appears to be at risk of data-loss because of this issue since they commit-after-yield as well.

I hope we can improve this situation somehow, perhaps by making background processing optional? Just thinking out loud here, there are probably better options.

[Lawouach]

Hi all,

I was made aware of that behavior through a bit of tracing when I realised my db sessions were committed after the response was returned. One of the other downside is that we keep a transaction opened longer than it needs when it is would be better to release db resources as early as possible. i'll go with the proposed workaround as well but I believe this is a bit of an issue that should at least be documented (I mean how the documented behavior may lead to potential risks).

[skion]

FWIW I resorted to using a middleware in the end, and picking up the session in a dependency through the application state:

@app.middleware("http")
async def db_session(request: Request, call_next):

    with _SessionLocal() as session, session.begin():

        # Make the session available to the FastAPI dependency.
        request.state.db_session = session

        return await call_next(request)


def get_database_session(request: Request) -> Session:
    """
    FastAPI dependency to obtain a database connection.
    """
    # This is set up in the database middleware.
    return request.state.db_session

This felt simpler than the solution proposed before, and keeps the session logic in one place so you can use the context managers again. Caveat is that database access in background tasks is probably completely broken without further work.