From b1c1b8d96eef3855e972110206dec56a47a058dd Mon Sep 17 00:00:00 2001 From: Victor Repkow Date: Tue, 12 Apr 2022 13:43:24 -0400 Subject: [PATCH 1/3] fix: rollback warning on clean sessions --- fastapi_sqla/__init__.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fastapi_sqla/__init__.py b/fastapi_sqla/__init__.py index b5eba7c5..2e1e669e 100644 --- a/fastapi_sqla/__init__.py +++ b/fastapi_sqla/__init__.py @@ -183,7 +183,13 @@ def get_users(session: fastapi_sqla.Session = Depends()): if response.status_code >= 400: # If ever a route handler returns an http exception, we do not want the # session opened by current context manager to commit anything in db. - logger.warning("http error, rolling back", status_code=response.status_code) + if session.dirty: + # optimistically only log if there's uncommited changes + logger.warning( + "http error, rolling back possibly uncommitted changes", + status_code=response.status_code, + ) + # since this is no-op if session is not dirty, we can always call it await loop.run_in_executor(None, session.rollback) return response From 7837f2701cd88932edf653445e708f17c19d9697 Mon Sep 17 00:00:00 2001 From: Victor Repkow Date: Tue, 12 Apr 2022 13:57:20 -0400 Subject: [PATCH 2/3] chore: tests --- fastapi_sqla/__init__.py | 7 ++++--- tests/test_middleware.py | 9 ++++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/fastapi_sqla/__init__.py b/fastapi_sqla/__init__.py index 2e1e669e..22c543be 100644 --- a/fastapi_sqla/__init__.py +++ b/fastapi_sqla/__init__.py @@ -166,12 +166,13 @@ def get_users(session: fastapi_sqla.Session = Depends()): response = await call_next(request) + is_dirty = bool(session.dirty) + loop = asyncio.get_running_loop() # try to commit after response, so that we can return a proper 500 response # and not raise a true internal server error if response.status_code < 400: - try: await loop.run_in_executor(None, session.commit) except Exception: @@ -183,8 +184,8 @@ def get_users(session: fastapi_sqla.Session = Depends()): if response.status_code >= 400: # If ever a route handler returns an http exception, we do not want the # session opened by current context manager to commit anything in db. - if session.dirty: - # optimistically only log if there's uncommited changes + if is_dirty: + # optimistically only log if there were uncommitted changes logger.warning( "http error, rolling back possibly uncommitted changes", status_code=response.status_code, diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 4a6a7fce..684c928f 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -171,7 +171,14 @@ async def test_rollback_on_http_exception(client, mock_middleware): with patch("fastapi_sqla.open_session") as open_session: session = open_session.return_value.__enter__.return_value - await client.get("/404") + with capture_logs() as caplog: + await client.get("/404") session.rollback.assert_called_once_with() mock_middleware.assert_called_once() + + assert { + "event": "http error, rolling back", + "log_level": "warning", + "status_code": 500, + } not in caplog From 15adb4dd58cc3bda2df5a515a86b4b1b54ff4754 Mon Sep 17 00:00:00 2001 From: Victor Repkow Date: Tue, 12 Apr 2022 14:13:25 -0400 Subject: [PATCH 3/3] chore: improve tests --- fastapi_sqla/__init__.py | 2 +- tests/test_middleware.py | 22 ++++++++++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/fastapi_sqla/__init__.py b/fastapi_sqla/__init__.py index 22c543be..90a598d7 100644 --- a/fastapi_sqla/__init__.py +++ b/fastapi_sqla/__init__.py @@ -166,7 +166,7 @@ def get_users(session: fastapi_sqla.Session = Depends()): response = await call_next(request) - is_dirty = bool(session.dirty) + is_dirty = bool(session.dirty or session.deleted or session.new) loop = asyncio.get_running_loop() diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 684c928f..e6e9f4b7 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -159,7 +159,7 @@ async def test_commit_error_returns_500(client, user_1, mock_middleware): } in caplog assert { - "event": "http error, rolling back", + "event": "http error, rolling back possibly uncommitted changes", "log_level": "warning", "status_code": 500, } in caplog @@ -171,14 +171,20 @@ async def test_rollback_on_http_exception(client, mock_middleware): with patch("fastapi_sqla.open_session") as open_session: session = open_session.return_value.__enter__.return_value - with capture_logs() as caplog: - await client.get("/404") + await client.get("/404") session.rollback.assert_called_once_with() mock_middleware.assert_called_once() - assert { - "event": "http error, rolling back", - "log_level": "warning", - "status_code": 500, - } not in caplog + +async def test_rollback_on_http_exception_silent(client, mock_middleware): + with capture_logs() as caplog: + await client.get("/404") + + mock_middleware.assert_called_once() + + assert { + "event": "http error, rolling back possibly uncommitted changes", + "log_level": "warning", + "status_code": 404, + } not in caplog