diff --git a/tests/test_cli_error_render.py b/tests/test_cli_error_render.py new file mode 100644 index 0000000..b519750 --- /dev/null +++ b/tests/test_cli_error_render.py @@ -0,0 +1,102 @@ +"""CLI structured error renderer for typed BigQuery error responses. + +The server side maps BQ Forbidden/auth/badrequest errors into typed +BqAccessError dicts that the FastAPI handler returns as `detail` JSON. +Today the CLI side flattens them via `f"HTTP {code}: {body[:200]}"`, +truncating the structured shape and hiding the operator-facing hints. + +`cli.error_render.render_error` recognizes a few canonical shapes and +pretty-prints them; falls back to truncated form for anything else. + +Closes part of #160 §4.7. +""" +from __future__ import annotations + +import pytest + + +@pytest.fixture +def render_error(): + from cli.error_render import render_error + return render_error + + +def test_renders_typed_bq_access_error(render_error): + """`{detail: {kind, hint, billing_project, data_project}}` from + BqAccessError surfaces as a multi-line block with the kind line, the + key/value pairs, and the hint word-wrapped at 80 cols.""" + body = {"detail": { + "kind": "cross_project_forbidden", + "message": "USER_PROJECT_DENIED on bigquery.googleapis.com", + "billing_project": "", + "data_project": "prj-example-data-001", + "hint": ( + "Set data_source.bigquery.billing_project in /admin/server-config " + "to a project where the SA has serviceusage.services.use, or " + "grant the SA that role on the data project." + ), + }} + out = render_error(502, body) + # Single-line `f"HTTP {code}: ..."` style is the OLD form. The new + # renderer must produce multi-line output. + assert "\n" in out + # Kind appears prominently in the output. + assert "cross_project_forbidden" in out + # Key/value pairs visible. + assert "billing_project" in out + assert "prj-example-data-001" in out + # Hint text included. + assert "serviceusage.services.use" in out + + +def test_renders_remote_scan_too_large(render_error): + """`{detail: {reason: 'remote_scan_too_large', scan_bytes, limit_bytes, + tables, suggestion}}` from the new /api/query guardrail formats with + the bytes + tables + suggestion clearly visible.""" + body = {"detail": { + "reason": "remote_scan_too_large", + "scan_bytes": 10737418240, # 10 GiB + "limit_bytes": 5368709120, # 5 GiB + "tables": ["finance.unit_economics"], + "suggestion": ( + "Use `da fetch --select --where " + "--estimate` to materialize a filtered subset, then query " + "the snapshot locally." + ), + }} + out = render_error(400, body) + assert "remote_scan_too_large" in out + assert "10737418240" in out or "10 GiB" in out or "10737418240" in str(out) + assert "finance.unit_economics" in out + assert "da fetch" in out + + +def test_renders_bq_path_not_registered(render_error): + """`{detail: {reason: 'bq_path_not_registered', path, hint}}` from the + RBAC patch formats path + hint clearly.""" + body = {"detail": { + "reason": "bq_path_not_registered", + "path": 'bq."secret_ds"."secret_tbl"', + "hint": "Direct bq.* references must point to a registered table.", + }} + out = render_error(403, body) + assert "bq_path_not_registered" in out + assert 'secret_ds' in out + assert "registered table" in out + + +def test_falls_back_to_truncated_for_unrecognized_shape(render_error): + """Body without recognizable typed shape falls back to truncated form.""" + body = "Internal Server Error: Something went wrong" * 20 # 800+ chars + out = render_error(500, body) + # Old-style truncation kicks in; output is single-line and short. + assert len(out) < 600 + assert "500" in out + + +def test_falls_back_when_detail_is_string(render_error): + """Many old endpoints return `detail: ""` — render that + as-is without trying to walk it as a structured error dict.""" + body = {"detail": "Only single SELECT queries are allowed"} + out = render_error(400, body) + assert "Only single SELECT" in out diff --git a/tests/test_cli_query_render.py b/tests/test_cli_query_render.py new file mode 100644 index 0000000..fdfe9bc --- /dev/null +++ b/tests/test_cli_query_render.py @@ -0,0 +1,66 @@ +"""CLI commands route BQ-typed errors through the shared renderer. + +Three CLI paths surface BQ errors today: +- `da query --remote` (cli/commands/query.py:_query_remote → /api/query) +- `da query --register-bq` (cli/commands/query.py:_query_hybrid via + RemoteQueryError, which wraps server-side BqAccessError) +- `da fetch` / `da schema` / etc. (cli/v2_client.V2ClientError → v2 endpoints) + +After the refactor they all call cli.error_render.render_error so analyst +output is consistent and structured. Closes part of #160 §4.7.3. +""" +from __future__ import annotations + +import pytest + + +def test_v2_client_error_uses_renderer(): + """`V2ClientError.__str__` calls render_error so any caller of the v2 + HTTP helpers (api_get_json/api_post_json/api_post_arrow) automatically + surfaces typed BQ errors as structured output.""" + from cli.v2_client import V2ClientError + + body = {"detail": { + "kind": "cross_project_forbidden", + "message": "USER_PROJECT_DENIED", + "hint": "Set data_source.bigquery.billing_project", + }} + err = V2ClientError(status_code=502, body=body) + out = str(err) + # Old form: `HTTP 502: {'detail': {'kind': ...}}` (single line). + # New form: multi-line structured. + assert "\n" in out, f"V2ClientError must use multi-line renderer; got {out!r}" + assert "cross_project_forbidden" in out + assert "billing_project" in out + + +def test_v2_client_error_drops_truncation_for_dicts(): + """The OLD `message=str(body)[:200]` truncation hid the structured + `hint` field for any reasonably-sized error dict. The new renderer + must NOT pre-truncate dict bodies.""" + from cli.v2_client import V2ClientError + + body = {"detail": { + "kind": "bq_forbidden", + "billing_project": "x" * 200, # padding to push past old 200-char limit + "data_project": "y" * 200, + "hint": "MUST_REACH_THIS_HINT_IN_OUTPUT", + }} + err = V2ClientError(status_code=502, body=body) + out = str(err) + assert "MUST_REACH_THIS_HINT_IN_OUTPUT" in out, \ + "renderer must not pre-truncate dict bodies past the hint field" + + +def test_remote_query_error_carries_details(): + """`RemoteQueryError` already has a `details` field. Verify the type's + surface so cli/commands/query.py:_query_hybrid can rely on it.""" + from src.remote_query import RemoteQueryError + + err = RemoteQueryError( + error_type="cross_project_forbidden", + message="USER_PROJECT_DENIED", + details={"billing_project": "", "data_project": "prj"}, + ) + assert err.details == {"billing_project": "", "data_project": "prj"} + assert err.error_type == "cross_project_forbidden" diff --git a/tests/test_remote_query_error_details.py b/tests/test_remote_query_error_details.py new file mode 100644 index 0000000..006190b --- /dev/null +++ b/tests/test_remote_query_error_details.py @@ -0,0 +1,57 @@ +"""`src/remote_query.py:RemoteQueryError` carries a `details` dict +(verified to already exist) populated for raise sites that wrap an +upstream BqAccessError. + +Currently only the BqAccessError-wrap path (lines 422 + 432) populates +it. The other 11 raise sites (lines 134, 142, 167, 173, 259, 264, 282, +289, 313, 322, 375) need an audit — for sites that wrap an external +exception, populate `details` so the CLI renderer has the structured +context it needs. + +Closes the audit half of #160 §4.7.3. +""" +from __future__ import annotations + + +def test_blocked_keyword_carries_keyword_in_details(): + """`raise RemoteQueryError("blocked_keyword", ..., details={"blocked_keyword": kw})` + already populates the keyword. Lock it in via assertion so a future + refactor doesn't drop the field.""" + from src.remote_query import RemoteQueryEngine, RemoteQueryError + import duckdb + + conn = duckdb.connect(":memory:") + engine = RemoteQueryEngine(conn) + try: + engine.execute("DROP TABLE foo") + except RemoteQueryError as exc: + assert exc.error_type == "blocked_keyword", exc.error_type + # 'drop' is the keyword that triggers the block. + assert exc.details, "blocked_keyword raise must populate details" + assert "blocked_keyword" in exc.details + else: + raise AssertionError("expected blocked_keyword RemoteQueryError") + + +def test_query_must_be_select_carries_no_unnecessary_details(): + """A pure local-validation raise (SQL doesn't start with SELECT) + has nothing structured to surface. details=None is the right shape; + test locks that in so a future contributor doesn't add noise.""" + from src.remote_query import RemoteQueryEngine, RemoteQueryError + import duckdb + + conn = duckdb.connect(":memory:") + engine = RemoteQueryEngine(conn) + try: + engine.execute("WITH x AS (SELECT 1) SELECT * FROM x") + except RemoteQueryError as exc: + # `WITH ...` is treated as not-starting-with-SELECT today; this is + # the shape we want to assert: details may be empty/None for pure + # local-validation errors. + assert exc.error_type in ("query_must_be_select", "blocked_keyword"), exc.error_type + # Either empty dict or dict with explanatory keys is fine. + assert isinstance(exc.details, dict) + except Exception: + # If the engine accepts WITH, that's also fine — this test is + # primarily about details shape, not what gets blocked. + pass