diff --git a/docs/superpowers/plans/2026-07-11-adapter-foundations.md b/docs/superpowers/plans/2026-07-11-adapter-foundations.md index c97440fb..664ff1b4 100644 --- a/docs/superpowers/plans/2026-07-11-adapter-foundations.md +++ b/docs/superpowers/plans/2026-07-11-adapter-foundations.md @@ -90,8 +90,15 @@ git commit -m "refactor(dwh): define adapter contract" - Create: `harness/tht/adapters/dwh/postgres.py` - Create: `harness/tht/adapters/dwh/thoth_rest.py` - Test: `harness/tests/test_dwh_adapters.py` +- Test: `harness/tests/test_dwh_port_contract.py` +- Test: `harness/tests/l0/test_db_sampling.py` +- Modify: `harness/tht/ports/__init__.py` +- Modify: `harness/tht/ports/dwh.py` +- Modify: `harness/tht/execute/__init__.py` - Modify: `harness/tht/db/execute.py` +- Modify: `harness/tht/db/sampling.py` - Modify: `harness/tht/rest/execute.py` +- Modify: `docs/superpowers/plans/2026-07-11-adapter-foundations.md` **Interfaces:** - Consumes: `DwhAdapter` from Task 1; existing `DatabaseConfig`, `RestConfig`, catalog, sampling, execute, and explain functions. @@ -125,8 +132,9 @@ class PostgresDwhAdapter: Implement the analogous REST wrapper by delegating to `tht.rest.*`; translate transport-specific errors only at the adapter boundary. Both wrappers delegate frequency-ranked, distinct sampling to -the paired implementations in `tht.db.sampling`. A non-positive query limit is rejected, and -`distinct_values` reports any cap through `DistinctValues.truncated`. +the paired implementations in `tht.db.sampling`. Query and sampling limits must be runtime-positive +integers (booleans and floats are rejected), and `distinct_values` reports any cap through +`DistinctValues.truncated`. - [ ] **Step 4: Run adapter, read-only, sampling, and REST tests** @@ -136,7 +144,11 @@ Expected: PASS; L0 may deselect when Docker is unavailable. - [ ] **Step 5: Commit** ```bash -git add harness/tht/adapters harness/tht/db/execute.py harness/tht/rest/execute.py harness/tests/test_dwh_adapters.py +git add docs/superpowers/plans/2026-07-11-adapter-foundations.md \ + harness/tht/ports harness/tht/adapters/dwh harness/tht/execute/__init__.py \ + harness/tht/db/execute.py harness/tht/db/sampling.py harness/tht/rest/execute.py \ + harness/tests/test_dwh_port_contract.py harness/tests/test_dwh_adapters.py \ + harness/tests/l0/test_db_sampling.py git commit -m "refactor(dwh): adapt direct and REST transports" ``` diff --git a/harness/tests/l0/test_db_sampling.py b/harness/tests/l0/test_db_sampling.py index ae386bf5..e15529d4 100644 --- a/harness/tests/l0/test_db_sampling.py +++ b/harness/tests/l0/test_db_sampling.py @@ -11,6 +11,22 @@ from tht.db.sampling import distinct_values, is_text_type, sample_column, unique pytestmark = [pytest.mark.l0] +@pytest.mark.parametrize("invalid_limit", [True, 1.5, 0, -1]) +def test_direct_sampling_rejects_non_positive_integer_limits(admin_engine, invalid_limit): + with pytest.raises(ValueError, match="positive integer"): + sample_column( + admin_engine, "dw", "fct_ricoveri", "reparto", limit=invalid_limit + ) + with pytest.raises(ValueError, match="positive integer"): + distinct_values( + admin_engine, + "dw", + "fct_ricoveri", + "reparto", + max_values=invalid_limit, + ) + + def test_is_text_type(): assert is_text_type("text") assert is_text_type("varchar(100)") diff --git a/harness/tests/test_dwh_adapters.py b/harness/tests/test_dwh_adapters.py index 4b9638a1..fdb88b62 100644 --- a/harness/tests/test_dwh_adapters.py +++ b/harness/tests/test_dwh_adapters.py @@ -2,6 +2,7 @@ import pytest from sqlalchemy.exc import OperationalError from tht.config import DatabaseConfig, RestConfig +from tht.db.sampling import distinct_values_rest, sample_column_rest from tht.execute import ExecutionError from tht.ports import DistinctValues, DwhAdapter from tht.rest.client import RestError @@ -45,12 +46,31 @@ def test_adapter_satisfies_dwh_protocol(factory): @pytest.mark.parametrize("factory", [postgres_factory, rest_factory]) -def test_run_query_requires_explicit_positive_limit(factory): +@pytest.mark.parametrize("invalid_limit", [True, 1.5, 0, -1]) +def test_run_query_rejects_non_positive_integer_limit(factory, invalid_limit): adapter = factory() + with pytest.raises(ValueError, match="positive integer"): + adapter.run_query("select 1", limit=invalid_limit) + + +@pytest.mark.parametrize("factory", [postgres_factory, rest_factory]) +def test_run_query_requires_explicit_limit(factory): with pytest.raises(TypeError): - adapter.run_query("select 1") - with pytest.raises(ValueError, match="positive"): - adapter.run_query("select 1", limit=0) + factory().run_query("select 1") + + +@pytest.mark.parametrize("invalid_limit", [True, 1.5, 0, -1]) +def test_rest_sampling_rejects_non_positive_integer_limit(invalid_limit): + class Client: + def top_values(self, *args): + raise AssertionError("transport must not be used") + + with pytest.raises(ValueError, match="positive integer"): + sample_column_rest(Client(), "dw", "sales", "region", limit=invalid_limit) + with pytest.raises(ValueError, match="positive integer"): + distinct_values_rest( + Client(), "dw", "sales", "region", max_values=invalid_limit + ) def test_postgres_sampling_delegates_to_paired_sampling_functions(monkeypatch): @@ -101,8 +121,6 @@ def test_rest_sampling_delegates_and_translates_transport_errors(monkeypatch): def test_rest_distinct_values_reports_transport_truncation(): - from tht.db.sampling import distinct_values_rest - class Client: def top_values(self, schema, table, column, limit): assert (schema, table, column, limit) == ("dw", "sales", "region", 3) diff --git a/harness/tht/db/execute.py b/harness/tht/db/execute.py index df084444..61e74c8f 100644 --- a/harness/tht/db/execute.py +++ b/harness/tht/db/execute.py @@ -2,14 +2,19 @@ from sqlalchemy import Engine -from tht.execute import ExecResult, PlanSummary, explain as _explain, run_controlled +from tht.execute import ( + ExecResult, + PlanSummary, + explain as _explain, + require_positive_int, + run_controlled, +) DEFAULT_TIMEOUT_MS = 30_000 def run_query(engine: Engine, sql: str, *, limit: int) -> ExecResult: - if limit <= 0: - raise ValueError("limit must be a positive integer") + limit = require_positive_int(limit, name="limit") return run_controlled( engine, sql, diff --git a/harness/tht/db/sampling.py b/harness/tht/db/sampling.py index 0029b501..ebc51bfb 100644 --- a/harness/tht/db/sampling.py +++ b/harness/tht/db/sampling.py @@ -4,6 +4,7 @@ from dataclasses import dataclass from sqlalchemy import Engine, text from tht.config import ExamplesConfig, LshConfig +from tht.execute import require_positive_int from tht.mschema.models import Annotations, PhysicalSchema from tht.ports.dwh import DistinctValues @@ -26,8 +27,7 @@ def _quoted_top_values_query(engine: Engine, schema: str, table: str, column: st def sample_column( engine: Engine, schema: str, table: str, column: str, *, limit: int ) -> list[object]: - if limit <= 0: - raise ValueError("limit must be a positive integer") + limit = require_positive_int(limit, name="limit") query = _quoted_top_values_query(engine, schema, table, column) with engine.connect() as conn: rows = conn.execute(query, {"lim": limit}).fetchall() @@ -37,8 +37,7 @@ def sample_column( def sample_column_rest( client, schema: str, table: str, column: str, *, limit: int ) -> list[object]: - if limit <= 0: - raise ValueError("limit must be a positive integer") + limit = require_positive_int(limit, name="limit") rows = client.top_values(schema, table, column, limit) return [row["value"] for row in rows if row.get("value") is not None] @@ -51,6 +50,7 @@ def distinct_values( *, max_values: int = DEFAULT_DISTINCT_VALUES_LIMIT, ) -> DistinctValues: + max_values = require_positive_int(max_values, name="max_values") values = sample_column(engine, schema, table, column, limit=max_values + 1) return DistinctValues(values=values[:max_values], truncated=len(values) > max_values) @@ -63,6 +63,7 @@ def distinct_values_rest( *, max_values: int = DEFAULT_DISTINCT_VALUES_LIMIT, ) -> DistinctValues: + max_values = require_positive_int(max_values, name="max_values") values = sample_column_rest(client, schema, table, column, limit=max_values + 1) return DistinctValues(values=values[:max_values], truncated=len(values) > max_values) diff --git a/harness/tht/execute/__init__.py b/harness/tht/execute/__init__.py index 6d3dd14c..57c17757 100644 --- a/harness/tht/execute/__init__.py +++ b/harness/tht/execute/__init__.py @@ -26,6 +26,13 @@ class PlanSummary: node_types: list[str] +def require_positive_int(value: object, *, name: str) -> int: + """Return a validated positive integer, excluding booleans and numeric lookalikes.""" + if type(value) is not int or value <= 0: + raise ValueError(f"{name} must be a positive integer") + return value + + def _inject_limit(sql: str, limit: int) -> tuple[str, bool]: """Aggiunge LIMIT limit+1 se assente (il +1 serve a rilevare il troncamento). Se la query ha gia' un suo LIMIT, lo si rispetta.""" diff --git a/harness/tht/rest/execute.py b/harness/tht/rest/execute.py index c86d4b59..f1a272c7 100644 --- a/harness/tht/rest/execute.py +++ b/harness/tht/rest/execute.py @@ -9,7 +9,14 @@ il client mantiene solo l'iniezione del LIMIT (per il rilevamento del troncament import time -from tht.execute import ExecResult, ExecutionError, PlanSummary, _inject_limit, assert_read_only +from tht.execute import ( + ExecResult, + ExecutionError, + PlanSummary, + _inject_limit, + assert_read_only, + require_positive_int, +) from tht.rest.client import RestError from tht.rest.explain import parse_text_plan @@ -17,8 +24,7 @@ from tht.rest.explain import parse_text_plan def run_controlled_rest(client, sql: str, *, limit: int) -> ExecResult: # Guard read-only client-side anche sul path REST (D7): non delegare l'unica verifica # al server. Stesso check strutturale del path diretto. - if limit <= 0: - raise ValueError("limit must be a positive integer") + limit = require_positive_int(limit, name="limit") assert_read_only(sql) final_sql, injected = _inject_limit(sql, limit) start = time.monotonic()