From fdf5053acedae711fc8b1f874e6057141fd4cd6d Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:41:38 +0000 Subject: [PATCH 1/6] test: add error path tests for API token revocation - Enhanced existing tests for 400 (already revoked) and 404 (not found) - Added test_revoke_token_unauthenticated (401) - Added test_revoke_token_database_error (500 + rollback check) - Added test_revoke_token_invalid_id_format (422) Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- tests/test_api_tokens.py | 60 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_api_tokens.py b/tests/test_api_tokens.py index f9e95875..cf2732c7 100644 --- a/tests/test_api_tokens.py +++ b/tests/test_api_tokens.py @@ -221,6 +221,7 @@ class TestTokenRevoke: resp = client.delete(f"/api/api-tokens/{token_id}") assert resp.status_code == 400 + assert resp.json()["detail"] == "Token is already revoked" finally: _cleanup(app) @@ -233,6 +234,65 @@ class TestTokenRevoke: try: resp = client.delete("/api/api-tokens/99999") assert resp.status_code == 404 + assert resp.json()["detail"] == "Token not found" + finally: + _cleanup(app) + + @pytest.mark.unit + def test_revoke_token_unauthenticated(self, tok_engine): + """Revoking a token without authentication should return 401.""" + from fastapi import HTTPException, status + + from app.api.api_tokens import _get_owner_id + from app.main import app + + client = _make_client(tok_engine) + + # Simulating unauthenticated by forcing the dependency to raise 401 + def _override_owner_fail(): + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Not authenticated") + + app.dependency_overrides[_get_owner_id] = _override_owner_fail + + try: + resp = client.delete("/api/api-tokens/1") + assert resp.status_code == 401 + assert resp.json()["detail"] == "Not authenticated" + finally: + _cleanup(app) + + @pytest.mark.unit + def test_revoke_token_database_error(self, tok_engine, tok_session): + """Revoking a token should rollback and raise 500 if database commit fails.""" + from unittest.mock import patch + + from app.main import app + + client = _make_client(tok_engine) + try: + create_resp = client.post("/api/api-tokens/", json={"name": "DB Error Test"}) + token_id = create_resp.json()["id"] + + # Mock commit to raise an exception + with patch("sqlalchemy.orm.Session.commit", side_effect=Exception("DB Failure")): + resp = client.delete(f"/api/api-tokens/{token_id}") + assert resp.status_code == 500 + + # Verify token is still active in DB because of rollback + db_token = tok_session.query(ApiToken).filter(ApiToken.id == token_id).first() + assert db_token.is_active is True + finally: + _cleanup(app) + + @pytest.mark.unit + def test_revoke_token_invalid_id_format(self, tok_engine): + """Revoking a token with a non-integer ID should return 422 Unprocessable Entity.""" + from app.main import app + + client = _make_client(tok_engine) + try: + resp = client.delete("/api/api-tokens/abc") + assert resp.status_code == 422 finally: _cleanup(app) From 8656434f510ef1cc999d8077c9f3feb0d1c37670 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:49:43 +0000 Subject: [PATCH 2/6] Initial plan From 3f8841a58920c95b9dcdd171eb71c53fb81b1854 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:49:49 +0000 Subject: [PATCH 3/6] Initial plan From 96ef7d4769fc5cdfbd552cd49a13778f8717a4d9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:54:20 +0000 Subject: [PATCH 4/6] test: exercise real auth path in test_revoke_token_unauthenticated Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- tests/test_api_tokens.py | 43 ++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/tests/test_api_tokens.py b/tests/test_api_tokens.py index cf2732c7..fa439462 100644 --- a/tests/test_api_tokens.py +++ b/tests/test_api_tokens.py @@ -68,6 +68,31 @@ def _make_client(tok_engine, owner_id: str = _OWNER) -> TestClient: return client +def _make_unauthenticated_client(tok_engine) -> TestClient: + """Return a TestClient that only overrides ``get_db`` (no auth injection). + + This exercises the real ``_get_owner_id`` → ``get_current_owner_id`` + authentication path. Any request made with this client that does not + carry a valid session or Bearer token will receive a 401 from the + actual auth code, not from a mocked dependency. + """ + from app.main import app + + Session = sessionmaker(bind=tok_engine) + + def _override_get_db(): + session = Session() + try: + yield session + finally: + session.close() + + app.dependency_overrides[get_db] = _override_get_db + + client = TestClient(app, base_url="http://localhost", raise_server_exceptions=False) + return client + + def _cleanup(app): """Remove dependency overrides after test.""" app.dependency_overrides.clear() @@ -240,20 +265,16 @@ class TestTokenRevoke: @pytest.mark.unit def test_revoke_token_unauthenticated(self, tok_engine): - """Revoking a token without authentication should return 401.""" - from fastapi import HTTPException, status + """Revoking a token without authentication should return 401. - from app.api.api_tokens import _get_owner_id + Uses a client that only overrides ``get_db`` so that the real + ``_get_owner_id`` → ``get_current_owner_id`` path is exercised. + Sending no session or Bearer credentials means ``get_current_owner_id`` + returns ``None``, and ``_get_owner_id`` raises a 401. + """ from app.main import app - client = _make_client(tok_engine) - - # Simulating unauthenticated by forcing the dependency to raise 401 - def _override_owner_fail(): - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Not authenticated") - - app.dependency_overrides[_get_owner_id] = _override_owner_fail - + client = _make_unauthenticated_client(tok_engine) try: resp = client.delete("/api/api-tokens/1") assert resp.status_code == 401 From 751b16d804b0659514ac682b6b5d6c66a4e83e94 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:55:33 +0000 Subject: [PATCH 5/6] test: clarify caller cleanup responsibility in _make_unauthenticated_client docstring Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- tests/test_api_tokens.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_api_tokens.py b/tests/test_api_tokens.py index fa439462..710a961c 100644 --- a/tests/test_api_tokens.py +++ b/tests/test_api_tokens.py @@ -75,6 +75,9 @@ def _make_unauthenticated_client(tok_engine) -> TestClient: authentication path. Any request made with this client that does not carry a valid session or Bearer token will receive a 401 from the actual auth code, not from a mocked dependency. + + The caller is responsible for clearing overrides via ``_cleanup(app)`` + after the test completes (typically in a ``finally`` block). """ from app.main import app From 9e9b1fb158cf906de47b61997436104d924b45d6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 09:58:46 +0000 Subject: [PATCH 6/6] test: strengthen test_revoke_token_database_error to verify rollback is called Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- tests/test_api_tokens.py | 42 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/test_api_tokens.py b/tests/test_api_tokens.py index cf2732c7..23e97d2c 100644 --- a/tests/test_api_tokens.py +++ b/tests/test_api_tokens.py @@ -263,9 +263,23 @@ class TestTokenRevoke: @pytest.mark.unit def test_revoke_token_database_error(self, tok_engine, tok_session): - """Revoking a token should rollback and raise 500 if database commit fails.""" + """Revoking a token should rollback and raise 500 if database commit fails. + + The test verifies two properties: + 1. ``db.rollback()`` is actually called when commit raises (not just that + the endpoint returns 500). + 2. After the rollback the token remains active in the database. + + To ensure the assertions are meaningful, the patched ``commit`` first + flushes the session (so the changes *are* staged inside the transaction) + before raising. Without a subsequent ``rollback()`` the flushed state + would still be visible to other sessions, so the ``is_active`` check + would catch a missing rollback call. + """ from unittest.mock import patch + from sqlalchemy.orm import Session as SASession + from app.main import app client = _make_client(tok_engine) @@ -273,12 +287,32 @@ class TestTokenRevoke: create_resp = client.post("/api/api-tokens/", json={"name": "DB Error Test"}) token_id = create_resp.json()["id"] - # Mock commit to raise an exception - with patch("sqlalchemy.orm.Session.commit", side_effect=Exception("DB Failure")): + # Wrap commit: flush first so changes are staged in the transaction, + # then raise to simulate a commit failure after data has been written. + def _fail_after_flush(self): + self.flush() # stage changes inside the open transaction + raise Exception("DB Failure") + + # Spy on rollback so we can assert it is called. + rollback_called = False + real_rollback = SASession.rollback + + def _spy_rollback(self): + nonlocal rollback_called + rollback_called = True + real_rollback(self) + + with ( + patch.object(SASession, "commit", _fail_after_flush), + patch.object(SASession, "rollback", _spy_rollback), + ): resp = client.delete(f"/api/api-tokens/{token_id}") assert resp.status_code == 500 - # Verify token is still active in DB because of rollback + # rollback() must have been called to undo the flushed changes. + assert rollback_called, "db.rollback() was not called after commit failure" + + # After rollback the token must still be active in the database. db_token = tok_session.query(ApiToken).filter(ApiToken.id == token_id).first() assert db_token.is_active is True finally: