diff --git a/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx b/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx index a5c150471..9c604a8f8 100644 --- a/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx +++ b/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx @@ -15,6 +15,7 @@ import NativeSelect from "@mui/material/NativeSelect"; import type { HistogramDiffParams } from "../../api"; import { useModelColumns } from "../../hooks"; import type { RunFormProps } from "../run"; +import { classifyType } from "../ui/DataTypeIcon/classifyType"; // ============================================================================ // Type Utilities @@ -63,25 +64,13 @@ function isBooleanDataType(columnType: string) { } function isDateTimeType(columnType: string) { - const sql_datetime_types = [ - "DATE", - "DATETIME", - "TIMESTAMP", - "TIME", - "YEAR", // Specific to MySQL/MariaDB - "DATETIME2", - "SMALLDATETIME", - "DATETIMEOFFSET", // Specific to SQL Server - "INTERVAL", // Common in PostgreSQL and Oracle - "TIMESTAMPTZ", - "TIMETZ", // Specific to PostgreSQL - "TIMESTAMP WITH TIME ZONE", - "TIMESTAMP WITH LOCAL TIME ZONE", // Oracle - "TIMESTAMP_LTZ", - "TIMESTAMP_NTZ", - "TIMESTAMP_TZ", // Specific to Snowflake - ]; - return sql_datetime_types.includes(columnType.toUpperCase()); + const category = classifyType(columnType); + if (["date", "datetime", "time"].includes(category)) { + return true; + } + + const normalizedType = columnType.trim().toUpperCase(); + return normalizedType === "YEAR" || normalizedType === "INTERVAL"; } // ============================================================================ diff --git a/js/packages/ui/src/components/histogram/__tests__/HistogramDiffForm.test.ts b/js/packages/ui/src/components/histogram/__tests__/HistogramDiffForm.test.ts index 847bd7771..d04792cce 100644 --- a/js/packages/ui/src/components/histogram/__tests__/HistogramDiffForm.test.ts +++ b/js/packages/ui/src/components/histogram/__tests__/HistogramDiffForm.test.ts @@ -1,7 +1,29 @@ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; import { describe, expect, it } from "vitest"; import { supportsHistogramDiff } from "../HistogramDiffForm"; +interface HistogramTypePolicyCase { + type: string; + backend_supported: boolean; + picker_supported: boolean; +} + +const histogramTypePolicy = JSON.parse( + readFileSync( + resolve(process.cwd(), "../tests/fixtures/histogram_type_policy.json"), + "utf8", + ), +) as HistogramTypePolicyCase[]; + describe("supportsHistogramDiff", () => { + it.each(histogramTypePolicy)( + "matches the shared backend policy for '$type'", + ({ type, picker_supported: pickerSupported }) => { + expect(supportsHistogramDiff(type)).toBe(pickerSupported); + }, + ); + it("should return true for supported numeric types", () => { const supportedNumericTypes = [ "INT", diff --git a/js/packages/ui/src/components/ui/DataTypeIcon/classifyType.ts b/js/packages/ui/src/components/ui/DataTypeIcon/classifyType.ts index d27c964e8..b4471e809 100644 --- a/js/packages/ui/src/components/ui/DataTypeIcon/classifyType.ts +++ b/js/packages/ui/src/components/ui/DataTypeIcon/classifyType.ts @@ -144,7 +144,7 @@ const CATEGORY_MAP: Record = { * - Special case: TINYINT(1) maps to "boolean", plain TINYINT maps to "integer" */ export function classifyType(rawType: string): TypeCategory { - const trimmed = rawType.trim().toUpperCase(); + const trimmed = rawType.trim().toUpperCase().replace(/\s+/g, " "); if (!trimmed) { return "unknown"; diff --git a/recce/apis/check_api.py b/recce/apis/check_api.py index 73f421f96..6e97e2f19 100644 --- a/recce/apis/check_api.py +++ b/recce/apis/check_api.py @@ -6,6 +6,7 @@ from pydantic import BaseModel from recce.apis.check_func import ( + _validate_check, create_check_from_run, create_check_without_run, export_persistent_state, @@ -165,6 +166,8 @@ async def run_check_handler(check_id: UUID, input: RunCheckIn): raise HTTPException(status_code=404, detail=f"Check ID '{check_id}' not found") try: + if check.type == RunType.HISTOGRAM_DIFF: + _validate_check(check.type, check.params) log_api_event( "rerun_check", dict( @@ -173,7 +176,7 @@ async def run_check_handler(check_id: UUID, input: RunCheckIn): ), ) run, future = submit_run(check.type, check.params, check_id=check_id, triggered_by="user") - except RecceException as e: + except (RecceException, ValueError) as e: raise HTTPException(status_code=400, detail=str(e)) if input.nowait: @@ -237,11 +240,20 @@ class PatchCheckIn(BaseModel): response_model_exclude_none=True, ) async def update_check_handler(check_id: UUID, patch: PatchCheckIn, background_tasks: BackgroundTasks): + check_dao = CheckDAO() try: - check = CheckDAO().update_check_by_id(check_id, patch) + if patch.params is not None: + existing_check = check_dao.find_check_by_id(check_id) + if existing_check is None: + raise HTTPException(status_code=404, detail="Not Found") + if existing_check.type == RunType.HISTOGRAM_DIFF: + _validate_check(existing_check.type, patch.params) + check = check_dao.update_check_by_id(check_id, patch) except RecceCloudException as e: log_cloud_exception(f"Failed to update check {check_id}: {e}", e) raise HTTPException(status_code=e.status_code, detail=str(e.reason)) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) if check is None: raise HTTPException(status_code=404, detail="Not Found") diff --git a/recce/apis/check_func.py b/recce/apis/check_func.py index 1e7156a60..ecff44e97 100644 --- a/recce/apis/check_func.py +++ b/recce/apis/check_func.py @@ -28,7 +28,16 @@ def get_node_name_by_id(node_id): def _validate_check(check_type, params): if check_type == RunType.SCHEMA_DIFF: validate_schema_diff_check(params) - pass + elif check_type == RunType.HISTOGRAM_DIFF: + from recce.tasks.histogram import HistogramDiffCheckValidator + + HistogramDiffCheckValidator().validate( + { + "name": "Histogram diff", + "type": check_type, + "params": params, + } + ) def _get_ref_model(sql_template: str) -> Optional[str]: @@ -102,6 +111,8 @@ def create_check_from_run( def create_check_without_run( check_name, check_description, check_type, params, check_view_options, is_preset=False, is_checked=False ): + if check_type == RunType.HISTOGRAM_DIFF: + _validate_check(check_type, params) name = check_name if check_name is not None else _generate_check_name(check_type, params, check_view_options) # Get cloud user context for attribution if available diff --git a/recce/apis/run_api.py b/recce/apis/run_api.py index e3198bb5a..8cecdd31a 100644 --- a/recce/apis/run_api.py +++ b/recce/apis/run_api.py @@ -70,7 +70,7 @@ async def create_run_handler(input: CreateRunIn): ) try: run, future = submit_run(input.type, input.params, check_id=input.check_id, triggered_by="user") - except RecceException as e: + except (RecceException, ValueError) as e: raise HTTPException(status_code=400, detail=str(e)) if input.nowait: diff --git a/recce/mcp_server.py b/recce/mcp_server.py index 67bfd023f..d3c9c7073 100644 --- a/recce/mcp_server.py +++ b/recce/mcp_server.py @@ -2799,6 +2799,7 @@ async def _tool_create_check(self, arguments: Dict[str, Any]) -> Dict[str, Any]: """Create a persistent check from analysis findings.""" from recce.apis.check_api import PatchCheckIn from recce.apis.check_func import ( + _validate_check, create_check_without_run, export_persistent_state, ) @@ -2812,6 +2813,9 @@ async def _tool_create_check(self, arguments: Dict[str, Any]) -> Dict[str, Any]: description = arguments.get("description", "") approve = arguments.get("approve", True) + if check_type == RunType.HISTOGRAM_DIFF: + _validate_check(check_type, params) + # Idempotency: find existing check with same (type, params) check_dao = CheckDAO() existing_checks = check_dao.list() diff --git a/recce/tasks/histogram.py b/recce/tasks/histogram.py index dd9949032..21fa527c5 100644 --- a/recce/tasks/histogram.py +++ b/recce/tasks/histogram.py @@ -25,6 +25,7 @@ "INTERVAL", # Common in PostgreSQL and Oracle "TIMESTAMPTZ", "TIMESTAMP WITH TIME ZONE", + "TIMESTAMP WITHOUT TIME ZONE", "TIMESTAMP WITH LOCAL TIME ZONE", # Oracle "TIMESTAMP_LTZ", "TIMESTAMP_NTZ", @@ -81,13 +82,33 @@ r"^(CHAR|VARCHAR|NCHAR|NVARCHAR|VARCHAR2|NVARCHAR2)\(\d+\)$", # String types with lengths ] +sql_time_only_type_pattern = re.compile( + r"(?:TIMETZ\s*(?:\(\s*\d+\s*\))?" + r"|TIME\s*(?:\(\s*\d+\s*\))?" + r"(?:\s+(?:WITH|WITHOUT)\s+TIME\s+ZONE\s*(?:\(\s*\d+\s*\))?)?)" +) +sql_type_precision_pattern = re.compile(r"\s*\(\s*\d+\s*\)") + + +def _normalize_column_type(column_type: str) -> str: + return " ".join(column_type.strip().upper().split()) + + +def _is_datetime_histogram_type(column_type: str) -> bool: + normalized_type = sql_type_precision_pattern.sub("", _normalize_column_type(column_type), count=1) + return normalized_type in sql_datetime_types + def _is_histogram_supported(column_type): - if column_type.upper() in sql_not_supported_types: + normalized_type = _normalize_column_type(column_type) + if sql_time_only_type_pattern.fullmatch(normalized_type): + return False + + if normalized_type in sql_not_supported_types: return False for pattern in sql_not_supported_types_pattern: - if re.match(pattern, column_type.upper()): + if re.match(pattern, normalized_type): return False return True @@ -276,6 +297,11 @@ class HistogramDiffParams(BaseModel): num_bins: Optional[int] = 50 +def _validate_histogram_column_type(column_type: str) -> None: + if _is_histogram_supported(column_type) is False: + raise ValueError(f"Column type {column_type} is not supported for histogram analysis") + + def query_numeric_histogram(task, node, column, column_type, min_value, max_value, num_bins=50): is_integer = is_integral_histogram_type(column_type) geometry = numeric_histogram_geometry(min_value, max_value, num_bins, is_integer=is_integer) @@ -441,6 +467,7 @@ class HistogramDiffTask(Task, QueryMixin): def __init__(self, params): super().__init__() self.params = HistogramDiffParams(**params) + _validate_histogram_column_type(self.params.column_type) self.connection = None def execute(self): @@ -454,8 +481,8 @@ def execute(self): num_bins = self.params.num_bins or 50 column_type = self.params.column_type - if _is_histogram_supported(column_type) is False: - raise ValueError(f"Column type {column_type} is not supported for histogram analysis") + _validate_histogram_column_type(column_type) + is_datetime_type = _is_datetime_histogram_type(column_type) with dbt_adapter.connection_named("query"): self.connection = dbt_adapter.get_thread_connection() @@ -496,7 +523,7 @@ def get_min_max(fn, base, curr): } bin_edges = [] labels = [] - elif column_type.upper() in sql_datetime_types: + elif is_datetime_type: base_result, current_result, bin_edges = query_datetime_histogram( self, node, column, min_value, max_value ) @@ -510,7 +537,7 @@ def get_min_max(fn, base, curr): current_result["total"] = curr_total result["base"] = base_result result["current"] = current_result - if column_type.upper() in sql_datetime_types or min_value is None: + if is_datetime_type or min_value is None: result["min"] = min_value result["max"] = max_value else: @@ -539,5 +566,4 @@ def validate_check(self, check: Check): except Exception as e: raise ValueError(f"Invalid check: {str(e)}") - if _is_histogram_supported(params.column_type) is False: - raise ValueError(f"Column type {params.column_type} is not supported for histogram analysis") + _validate_histogram_column_type(params.column_type) diff --git a/tests/apis/test_histogram_validation.py b/tests/apis/test_histogram_validation.py new file mode 100644 index 000000000..96187a632 --- /dev/null +++ b/tests/apis/test_histogram_validation.py @@ -0,0 +1,126 @@ +from unittest.mock import MagicMock, patch + +import pytest +from fastapi import HTTPException + +from recce.apis.check_api import ( + CreateCheckIn, + PatchCheckIn, + RunCheckIn, + create_check, + run_check_handler, + update_check_handler, +) +from recce.apis.run_api import CreateRunIn, create_run_handler +from recce.models.types import Check, RunType + +INVALID_HISTOGRAM_PARAMS = { + "model": "customers", + "column_name": "created_time", + "column_type": "TIME(6) WITH TIME ZONE", +} +VALID_HISTOGRAM_PARAMS = { + "model": "customers", + "column_name": "customer_id", + "column_type": "INTEGER", +} + + +def _histogram_check() -> Check: + return Check( + name="time histogram", + type=RunType.HISTOGRAM_DIFF, + params=INVALID_HISTOGRAM_PARAMS, + ) + + +@pytest.mark.asyncio +async def test_saved_check_create_rejects_time_alias_before_persistence(): + """Catch POST /checks persisting a time-only histogram definition.""" + check_dao = MagicMock() + check_dao.create.return_value = _histogram_check() + + with ( + patch("recce.apis.check_func.CheckDAO", return_value=check_dao), + patch("recce.apis.check_api.CheckOut.from_check", return_value=MagicMock()), + ): + with pytest.raises(HTTPException) as error: + await create_check( + CreateCheckIn( + name="time histogram", + type=RunType.HISTOGRAM_DIFF, + params=INVALID_HISTOGRAM_PARAMS, + ), + MagicMock(), + ) + + assert error.value.status_code == 400 + assert "not supported for histogram analysis" in error.value.detail + check_dao.create.assert_not_called() + + +@pytest.mark.asyncio +async def test_saved_check_patch_rejects_time_alias_before_persistence(): + """Catch PATCH /checks replacing valid params with a time-only histogram.""" + check = Check( + name="numeric histogram", + type=RunType.HISTOGRAM_DIFF, + params=VALID_HISTOGRAM_PARAMS, + ) + check_dao = MagicMock() + check_dao.find_check_by_id.return_value = check + check_dao.update_check_by_id.return_value = check + + with ( + patch("recce.apis.check_api.CheckDAO", return_value=check_dao), + patch("recce.apis.check_api.CheckOut.from_check", return_value=MagicMock()), + ): + with pytest.raises(HTTPException) as error: + await update_check_handler( + check.check_id, + PatchCheckIn(params=INVALID_HISTOGRAM_PARAMS), + MagicMock(), + ) + + assert error.value.status_code == 400 + assert "not supported for histogram analysis" in error.value.detail + check_dao.update_check_by_id.assert_not_called() + + +@pytest.mark.asyncio +async def test_saved_check_rerun_rejects_time_alias_before_submission(): + """Catch a legacy invalid saved check being queued for warehouse execution.""" + check = _histogram_check() + check_dao = MagicMock() + check_dao.find_check_by_id.return_value = check + + with ( + patch("recce.apis.check_api.CheckDAO", return_value=check_dao), + patch("recce.apis.check_api.submit_run", return_value=(MagicMock(), None)) as submit_run, + ): + with pytest.raises(HTTPException) as error: + await run_check_handler(check.check_id, RunCheckIn(nowait=True)) + + assert error.value.status_code == 400 + assert "not supported for histogram analysis" in error.value.detail + submit_run.assert_not_called() + + +@pytest.mark.asyncio +async def test_direct_run_maps_histogram_validation_to_bad_request(): + """Catch direct invalid histogram parameters escaping the REST 400 boundary.""" + with patch( + "recce.apis.run_api.submit_run", + side_effect=ValueError("Column type TIME(6) is not supported for histogram analysis"), + ): + with pytest.raises(HTTPException) as error: + await create_run_handler( + CreateRunIn( + type="histogram_diff", + params=INVALID_HISTOGRAM_PARAMS, + nowait=True, + ) + ) + + assert error.value.status_code == 400 + assert "not supported for histogram analysis" in error.value.detail diff --git a/tests/fixtures/histogram_type_policy.json b/tests/fixtures/histogram_type_policy.json new file mode 100644 index 000000000..646ae2344 --- /dev/null +++ b/tests/fixtures/histogram_type_policy.json @@ -0,0 +1,27 @@ +[ + {"type": "TIME", "backend_supported": false, "picker_supported": false}, + {"type": "time", "backend_supported": false, "picker_supported": false}, + {"type": " TiMe ", "backend_supported": false, "picker_supported": false}, + {"type": "TIME(6)", "backend_supported": false, "picker_supported": false}, + {"type": " time ( 6 ) ", "backend_supported": false, "picker_supported": false}, + {"type": "TIMETZ", "backend_supported": false, "picker_supported": false}, + {"type": "timetz(6)", "backend_supported": false, "picker_supported": false}, + {"type": " TIMETZ ( 3 ) ", "backend_supported": false, "picker_supported": false}, + {"type": "TIME WITH TIME ZONE", "backend_supported": false, "picker_supported": false}, + {"type": "time without time zone", "backend_supported": false, "picker_supported": false}, + {"type": " time with time zone ", "backend_supported": false, "picker_supported": false}, + {"type": "TiMe ( 3 ) WiTh TiMe ZoNe", "backend_supported": false, "picker_supported": false}, + {"type": "TIME(6) WITHOUT TIME ZONE", "backend_supported": false, "picker_supported": false}, + {"type": "TIME WITH TIME ZONE(6)", "backend_supported": false, "picker_supported": false}, + {"type": "DATE", "backend_supported": true, "picker_supported": false}, + {"type": "date", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP(6)", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMPTZ", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMPTZ(6)", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP WITH TIME ZONE", "backend_supported": true, "picker_supported": false}, + {"type": "timestamp without time zone", "backend_supported": true, "picker_supported": false}, + {"type": " timestamp ( 6 ) with time zone ", "backend_supported": true, "picker_supported": false}, + {"type": "TIMEOUT", "backend_supported": true, "picker_supported": true}, + {"type": "MYSTERY_DECIMAL", "backend_supported": true, "picker_supported": true} +] diff --git a/tests/tasks/test_histogram.py b/tests/tasks/test_histogram.py index e86aa2d6c..75b8fb0ce 100644 --- a/tests/tasks/test_histogram.py +++ b/tests/tasks/test_histogram.py @@ -1,5 +1,9 @@ import json +from datetime import date from decimal import Decimal +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock import pytest @@ -10,6 +14,14 @@ _is_histogram_supported, ) +HISTOGRAM_TYPE_POLICY = json.loads( + (Path(__file__).parents[1] / "fixtures" / "histogram_type_policy.json").read_text(encoding="utf-8") +) +UNSUPPORTED_TIME_TYPES = [case["type"] for case in HISTOGRAM_TYPE_POLICY if not case["backend_supported"]] +SUPPORTED_TEMPORAL_TYPES = [ + case["type"] for case in HISTOGRAM_TYPE_POLICY if case["backend_supported"] and not case["picker_supported"] +] + @pytest.mark.parametrize( ("raw", "expected"), @@ -464,13 +476,95 @@ def test_is_column_type_supported_by_histogram(): assert _is_histogram_supported("TIMESTAMPTZ") is True +@pytest.mark.parametrize("case", HISTOGRAM_TYPE_POLICY, ids=lambda case: case["type"].strip()) +def test_histogram_backend_matches_shared_type_policy(case): + """Catch backend policy drift for time-only, temporal, and unknown adapter types.""" + assert _is_histogram_supported(case["type"]) is case["backend_supported"] + + +@pytest.mark.parametrize("column_type", SUPPORTED_TEMPORAL_TYPES) +def test_supported_temporal_aliases_use_datetime_execution_and_result_shape(column_type, monkeypatch): + """Catch supported temporal aliases falling through to numeric SQL or numeric result conversion.""" + minimum = date(2026, 1, 1) + maximum = date(2026, 1, 2) + task = HistogramDiffTask( + { + "model": "customers", + "column_name": "created_at", + "column_type": column_type, + } + ) + task.execute_sql = MagicMock(return_value=[(minimum, maximum, 1)]) + adapter = MagicMock() + monkeypatch.setattr(histogram, "default_context", lambda: SimpleNamespace(adapter=adapter)) + monkeypatch.setattr( + histogram, + "query_datetime_histogram", + MagicMock( + return_value=( + {"counts": [1]}, + {"counts": [1]}, + [minimum, maximum], + ) + ), + ) + monkeypatch.setattr( + histogram, + "query_numeric_histogram", + MagicMock(side_effect=AssertionError("temporal alias entered numeric histogram execution")), + ) + + assert task.execute() == { + "base": {"counts": [1], "total": 1}, + "current": {"counts": [1], "total": 1}, + "min": minimum, + "max": maximum, + "bin_edges": [minimum, maximum], + "labels": None, + } + + +@pytest.mark.parametrize("column_type", UNSUPPORTED_TIME_TYPES) +def test_histogram_task_rejects_every_time_alias_before_context_or_sql(column_type, monkeypatch): + """Catch time-only aliases reaching adapter setup or a warehouse query.""" + + def fail_if_context_is_opened(): + raise AssertionError("unsupported histogram type reached adapter setup") + + monkeypatch.setattr(histogram, "default_context", fail_if_context_is_opened) + + with pytest.raises(ValueError, match="not supported for histogram analysis"): + HistogramDiffTask( + { + "model": "customers", + "column_name": "time_col", + "column_type": column_type, + } + ) + + +@pytest.mark.parametrize("column_type", UNSUPPORTED_TIME_TYPES) +def test_histogram_check_validator_rejects_every_time_alias(column_type): + """Catch saved checks accepting an alias that direct histogram tasks reject.""" + with pytest.raises(ValueError, match="not supported for histogram analysis"): + HistogramDiffCheckValidator().validate( + { + "name": "time histogram", + "type": "histogram_diff", + "params": { + "model": "customers", + "column_name": "time_col", + "column_type": column_type, + }, + } + ) + + def test_histogram_rejects_time_columns_before_sql(dbt_test_helper): params_time = {"model": "customers", "column_name": "log_time", "column_type": "TIME"} - task = HistogramDiffTask(params_time) with pytest.raises(ValueError, match="Column type TIME is not supported for histogram analysis"): - task.execute() + HistogramDiffTask(params_time) params_timetz = {"model": "customers", "column_name": "log_time_tz", "column_type": "TIMETZ"} - task_tz = HistogramDiffTask(params_timetz) with pytest.raises(ValueError, match="Column type TIMETZ is not supported for histogram analysis"): - task_tz.execute() + HistogramDiffTask(params_timetz) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 0b747e82a..52281bf76 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -1488,6 +1488,121 @@ async def test_tool_histogram_diff_unknown_column(self, mcp_server): with pytest.raises(ValueError, match="Cannot determine column type"): await server._tool_histogram_diff({"model": "my_model", "column_name": "unknown"}) + @pytest.mark.asyncio + async def test_tool_histogram_diff_rejects_time_alias_before_sql(self, mcp_server): + """Catch MCP direct histogram analysis sending a time-only alias to SQL.""" + server, mock_context = mcp_server + mock_context.build_name_to_unique_id_index.return_value = {"my_model": "model.project.my_model"} + mock_context.get_model.return_value = { + "columns": {"event_time": {"name": "event_time", "type": "TIME(6) WITH TIME ZONE"}}, + } + + task_context = MagicMock() + task_context.adapter = MagicMock() + with ( + patch("recce.tasks.histogram.default_context", return_value=task_context), + patch.object( + HistogramDiffTask, + "execute_sql", + side_effect=AssertionError("time-only type reached SQL"), + ), + ): + with pytest.raises(ValueError, match="not supported for histogram analysis"): + await server._tool_histogram_diff({"model": "my_model", "column_name": "event_time"}) + + @pytest.mark.asyncio + async def test_tool_create_check_rejects_time_histogram_before_persistence(self, mcp_server): + """Catch MCP create_check persisting or running a time-only histogram.""" + server, _ = mcp_server + check_dao = MagicMock() + check_dao.list.return_value = [] + check_dao.create.return_value = Check( + name="time histogram", + type=RunType.HISTOGRAM_DIFF, + params={ + "model": "my_model", + "column_name": "event_time", + "column_type": "TIME(6) WITH TIME ZONE", + }, + ) + + with ( + patch("recce.models.CheckDAO", return_value=check_dao), + patch("recce.apis.check_func.CheckDAO", return_value=check_dao), + ): + with pytest.raises(ValueError, match="not supported for histogram analysis"): + await server._tool_create_check( + { + "type": "histogram_diff", + "name": "time histogram", + "params": { + "model": "my_model", + "column_name": "event_time", + "column_type": "TIME(6) WITH TIME ZONE", + }, + } + ) + + check_dao.create.assert_not_called() + + @pytest.mark.asyncio + async def test_tool_create_check_validates_existing_time_histogram_before_update(self, mcp_server): + """Catch idempotent MCP create mutating metadata before legacy params are rejected.""" + server, _ = mcp_server + params = { + "model": "my_model", + "column_name": "event_time", + "column_type": "TIME(6) WITH TIME ZONE", + } + existing_check = Check( + name="original name", + description="original description", + type=RunType.HISTOGRAM_DIFF, + params=params, + ) + check_dao = MagicMock() + check_dao.list.return_value = [existing_check] + + with patch("recce.models.CheckDAO", return_value=check_dao): + with pytest.raises(ValueError, match="not supported for histogram analysis"): + await server._tool_create_check( + { + "type": "histogram_diff", + "name": "mutated name", + "description": "mutated description", + "params": params, + } + ) + + check_dao.update_check_by_id.assert_not_called() + + @pytest.mark.asyncio + async def test_tool_run_check_rejects_time_histogram_before_submission(self, mcp_server): + """Catch MCP run_check queuing a legacy time-only histogram.""" + server, _ = mcp_server + check = Check( + name="time histogram", + type=RunType.HISTOGRAM_DIFF, + params={ + "model": "my_model", + "column_name": "event_time", + "column_type": "TIME(6) WITH TIME ZONE", + }, + ) + check_dao = MagicMock() + check_dao.find_check_by_id.return_value = check + + run_context = MagicMock() + run_context.adapter_type = "dbt" + run_context.review_mode = False + with ( + patch("recce.models.CheckDAO", return_value=check_dao), + patch("recce.apis.run_func.default_context", return_value=run_context), + patch("recce.apis.run_func.RunDAO", return_value=MagicMock()), + ): + with pytest.raises(ValueError, match="not supported for histogram analysis"): + await server._tool_run_check({"check_id": str(check.check_id)}) + @pytest.mark.asyncio async def test_tool_get_model(self, mcp_server): """Test the get_model tool""" @@ -3883,7 +3998,7 @@ async def test_non_run_backed_tools_have_no_run_id(self, server, tool_method, ta patch.object( RecceContext, "get_model", - return_value={"columns": {"status": {"name": "status", "type": "VARCHAR"}}}, + return_value={"columns": {"status": {"name": "status", "type": "INTEGER"}}}, ), ): result = await getattr(server, tool_method)(args)