From ac9a6ddc531a3d0be30211bcd9b6ec56ca83c9fe Mon Sep 17 00:00:00 2001 From: Atti Ur Rehman Date: Tue, 19 May 2026 10:08:53 +0300 Subject: [PATCH] =?UTF-8?q?fix(history):=20persist=20query=5Fhistory=20row?= =?UTF-8?q?s=20by=20encoding=20result=5Fjson=20as=20jsonb=20asyncpg=20cann?= =?UTF-8?q?ot=20encode=20a=20Python=20dict=20into=20a=20JSONB=20column,=20?= =?UTF-8?q?so=20the=20INSERT=20into=20query=5Fhistory=20raised=20DataError?= =?UTF-8?q?=20on=20every=20successful=20query.=20The=20blanket=20"non-fata?= =?UTF-8?q?l"=20except=20block=20hid=20the=20failure,=20but=20the=20rate-l?= =?UTF-8?q?imit=20counter=20on=20users.total=5Fqueries=20had=20already=20c?= =?UTF-8?q?ommitted=20=E2=80=94=20producing=20the=20user-facing=20bug=20wh?= =?UTF-8?q?ere=20the=20budget=20badge=20decremented=20("1=20of=202=20remai?= =?UTF-8?q?ning")=20yet=20the=20History=20panel=20stayed=20empty.=20Serial?= =?UTF-8?q?ise=20the=20result=20cache=20with=20json.dumps(...,=20default?= =?UTF-8?q?=3Dstr)=20and=20add=20an=20explicit=20::jsonb=20cast=20on=20the?= =?UTF-8?q?=20bind=20parameter.=20Keep=20the=20audit-log=20write=20best-ef?= =?UTF-8?q?fort=20(a=20successful=20query=20should=20not=20500=20because?= =?UTF-8?q?=20the=20audit=20log=20failed)=20but=20log=20structured=20conte?= =?UTF-8?q?xt=20so=20a=20regression=20is=20visible=20in=20Azure=20logs.=20?= =?UTF-8?q?Adds=20two=20regression=20tests:=20-=20asserts=20result=5Fjson?= =?UTF-8?q?=20is=20bound=20as=20a=20string=20and=20SQL=20contains=20$4::js?= =?UTF-8?q?onb=20-=20asserts=20a=20transient=20DB=20failure=20does=20not?= =?UTF-8?q?=20propagate=20to=20the=20user=20tests/conftest.py:=20setdefaul?= =?UTF-8?q?t=20JWT=5FSECRET=5FKEY=20and=20GOOGLE=5FCLIENT=5FID=20so=20unit?= =?UTF-8?q?=20tests=20load=20without=20a=20fully=20populated=20.env.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/app/agent/sql_agent.py | 15 ++++++-- tests/conftest.py | 2 + tests/test_sql_agent.py | 69 ++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/backend/app/agent/sql_agent.py b/backend/app/agent/sql_agent.py index b215dd1..223a402 100644 --- a/backend/app/agent/sql_agent.py +++ b/backend/app/agent/sql_agent.py @@ -224,14 +224,17 @@ async def _log_query_history( Persist query to audit log with full result cache. result_json stores rows + columns so history replays are free (no Claude call). """ + # asyncpg has no built-in dict→jsonb encoder. Serialise to JSON text and + # cast in SQL (`$4::jsonb`); otherwise the INSERT raises silently and + # history rows are lost (the rate-limit counter still increments). try: - result_json = {"rows": rows, "columns": columns} + result_json = json.dumps({"rows": rows, "columns": columns}, default=str) await database_pool.execute( """ INSERT INTO query_history (question, generated_sql, explanation, result_json, row_count, execution_ms, retries, user_id) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8) + VALUES ($1, $2, $3, $4::jsonb, $5, $6, $7, $8) """, question, sql, @@ -243,4 +246,10 @@ async def _log_query_history( user_id, ) except Exception: - logger.exception("Failed to write query_history — non-fatal") + # Best-effort: the user already saw their result; do not 500 a successful + # query because the audit-log write failed. Log loudly so we notice if + # the silent-loss regression returns. + logger.exception( + "Failed to write query_history", + extra={"user_id": user_id, "row_count": row_count}, + ) diff --git a/tests/conftest.py b/tests/conftest.py index e64fbc1..15c5453 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -40,3 +40,5 @@ def _load_dotenv_repo_root() -> None: os.environ.setdefault( "DATABASE_URL", "postgresql://test:test@127.0.0.1:5432/insightiq_test" ) +os.environ.setdefault("JWT_SECRET_KEY", "test-jwt-secret-not-used-in-unit-tests") +os.environ.setdefault("GOOGLE_CLIENT_ID", "test-google-client-id.apps.googleusercontent.com") diff --git a/tests/test_sql_agent.py b/tests/test_sql_agent.py index 73535a6..ec4f7b5 100644 --- a/tests/test_sql_agent.py +++ b/tests/test_sql_agent.py @@ -18,8 +18,11 @@ from datetime import date, datetime from pydantic import ValidationError +from unittest.mock import AsyncMock, patch + from app.agent.sql_agent import ( _assert_safe_sql, + _log_query_history, _serialise_row, ) from app.api.schemas import QueryRequest @@ -194,3 +197,69 @@ def test_too_long_raises(self): """Questions over 500 chars must fail validation.""" with pytest.raises(ValidationError): QueryRequest(question="x" * 501) + + +# ── _log_query_history (regression: silent JSONB encode failure) ────────── + + +class TestLogQueryHistoryJsonbEncoding: + """ + Regression tests for the bug where `query_history` rows were never + persisted because asyncpg cannot encode a Python dict to JSONB. + + Symptom in production: the per-user rate-limit counter incremented + (1 of 2 remaining) but History panel stayed empty. + """ + + @pytest.mark.asyncio + async def test_passes_json_string_and_jsonb_cast(self): + """result_json must be sent as a JSON string with explicit ::jsonb cast.""" + with patch("app.agent.sql_agent.database_pool.execute", new=AsyncMock()) as mock_exec: + await _log_query_history( + question="What is MRR?", + sql="SELECT 1", + explanation="demo", + rows=[{"mrr": 1.0}], + columns=["mrr"], + row_count=1, + execution_ms=12, + retries=0, + user_id=42, + ) + + assert mock_exec.await_count == 1 + sql_arg, *params = mock_exec.await_args.args + + assert "$4::jsonb" in sql_arg, "SQL must cast result_json parameter to jsonb" + + # result_json is the 4th positional bind value + result_json_param = params[3] + assert isinstance(result_json_param, str), ( + "result_json must be a JSON string — asyncpg has no dict→jsonb encoder" + ) + import json as _json + + decoded = _json.loads(result_json_param) + assert decoded == {"rows": [{"mrr": 1.0}], "columns": ["mrr"]} + + @pytest.mark.asyncio + async def test_db_failure_is_swallowed_not_propagated(self): + """ + Audit-log write is best-effort: a DB failure must not 500 a query + the user already received a successful response for. + """ + with patch( + "app.agent.sql_agent.database_pool.execute", + new=AsyncMock(side_effect=RuntimeError("transient")), + ): + await _log_query_history( + question="q", + sql="SELECT 1", + explanation="", + rows=[], + columns=[], + row_count=0, + execution_ms=0, + retries=0, + user_id=1, + )