fix(dwh): validate adapter limits strictly
This commit is contained in:
@@ -90,8 +90,15 @@ git commit -m "refactor(dwh): define adapter contract"
|
|||||||
- Create: `harness/tht/adapters/dwh/postgres.py`
|
- Create: `harness/tht/adapters/dwh/postgres.py`
|
||||||
- Create: `harness/tht/adapters/dwh/thoth_rest.py`
|
- Create: `harness/tht/adapters/dwh/thoth_rest.py`
|
||||||
- Test: `harness/tests/test_dwh_adapters.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/execute.py`
|
||||||
|
- Modify: `harness/tht/db/sampling.py`
|
||||||
- Modify: `harness/tht/rest/execute.py`
|
- Modify: `harness/tht/rest/execute.py`
|
||||||
|
- Modify: `docs/superpowers/plans/2026-07-11-adapter-foundations.md`
|
||||||
|
|
||||||
**Interfaces:**
|
**Interfaces:**
|
||||||
- Consumes: `DwhAdapter` from Task 1; existing `DatabaseConfig`, `RestConfig`, catalog, sampling, execute, and explain functions.
|
- 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
|
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
|
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
|
the paired implementations in `tht.db.sampling`. Query and sampling limits must be runtime-positive
|
||||||
`distinct_values` reports any cap through `DistinctValues.truncated`.
|
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**
|
- [ ] **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**
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
```bash
|
```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"
|
git commit -m "refactor(dwh): adapt direct and REST transports"
|
||||||
```
|
```
|
||||||
|
|
||||||
|
|||||||
@@ -11,6 +11,22 @@ from tht.db.sampling import distinct_values, is_text_type, sample_column, unique
|
|||||||
pytestmark = [pytest.mark.l0]
|
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():
|
def test_is_text_type():
|
||||||
assert is_text_type("text")
|
assert is_text_type("text")
|
||||||
assert is_text_type("varchar(100)")
|
assert is_text_type("varchar(100)")
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import pytest
|
|||||||
from sqlalchemy.exc import OperationalError
|
from sqlalchemy.exc import OperationalError
|
||||||
|
|
||||||
from tht.config import DatabaseConfig, RestConfig
|
from tht.config import DatabaseConfig, RestConfig
|
||||||
|
from tht.db.sampling import distinct_values_rest, sample_column_rest
|
||||||
from tht.execute import ExecutionError
|
from tht.execute import ExecutionError
|
||||||
from tht.ports import DistinctValues, DwhAdapter
|
from tht.ports import DistinctValues, DwhAdapter
|
||||||
from tht.rest.client import RestError
|
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])
|
@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()
|
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):
|
with pytest.raises(TypeError):
|
||||||
adapter.run_query("select 1")
|
factory().run_query("select 1")
|
||||||
with pytest.raises(ValueError, match="positive"):
|
|
||||||
adapter.run_query("select 1", limit=0)
|
|
||||||
|
@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):
|
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():
|
def test_rest_distinct_values_reports_transport_truncation():
|
||||||
from tht.db.sampling import distinct_values_rest
|
|
||||||
|
|
||||||
class Client:
|
class Client:
|
||||||
def top_values(self, schema, table, column, limit):
|
def top_values(self, schema, table, column, limit):
|
||||||
assert (schema, table, column, limit) == ("dw", "sales", "region", 3)
|
assert (schema, table, column, limit) == ("dw", "sales", "region", 3)
|
||||||
|
|||||||
@@ -2,14 +2,19 @@
|
|||||||
|
|
||||||
from sqlalchemy import Engine
|
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
|
DEFAULT_TIMEOUT_MS = 30_000
|
||||||
|
|
||||||
|
|
||||||
def run_query(engine: Engine, sql: str, *, limit: int) -> ExecResult:
|
def run_query(engine: Engine, sql: str, *, limit: int) -> ExecResult:
|
||||||
if limit <= 0:
|
limit = require_positive_int(limit, name="limit")
|
||||||
raise ValueError("limit must be a positive integer")
|
|
||||||
return run_controlled(
|
return run_controlled(
|
||||||
engine,
|
engine,
|
||||||
sql,
|
sql,
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ from dataclasses import dataclass
|
|||||||
from sqlalchemy import Engine, text
|
from sqlalchemy import Engine, text
|
||||||
|
|
||||||
from tht.config import ExamplesConfig, LshConfig
|
from tht.config import ExamplesConfig, LshConfig
|
||||||
|
from tht.execute import require_positive_int
|
||||||
from tht.mschema.models import Annotations, PhysicalSchema
|
from tht.mschema.models import Annotations, PhysicalSchema
|
||||||
from tht.ports.dwh import DistinctValues
|
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(
|
def sample_column(
|
||||||
engine: Engine, schema: str, table: str, column: str, *, limit: int
|
engine: Engine, schema: str, table: str, column: str, *, limit: int
|
||||||
) -> list[object]:
|
) -> list[object]:
|
||||||
if limit <= 0:
|
limit = require_positive_int(limit, name="limit")
|
||||||
raise ValueError("limit must be a positive integer")
|
|
||||||
query = _quoted_top_values_query(engine, schema, table, column)
|
query = _quoted_top_values_query(engine, schema, table, column)
|
||||||
with engine.connect() as conn:
|
with engine.connect() as conn:
|
||||||
rows = conn.execute(query, {"lim": limit}).fetchall()
|
rows = conn.execute(query, {"lim": limit}).fetchall()
|
||||||
@@ -37,8 +37,7 @@ def sample_column(
|
|||||||
def sample_column_rest(
|
def sample_column_rest(
|
||||||
client, schema: str, table: str, column: str, *, limit: int
|
client, schema: str, table: str, column: str, *, limit: int
|
||||||
) -> list[object]:
|
) -> list[object]:
|
||||||
if limit <= 0:
|
limit = require_positive_int(limit, name="limit")
|
||||||
raise ValueError("limit must be a positive integer")
|
|
||||||
rows = client.top_values(schema, table, column, limit)
|
rows = client.top_values(schema, table, column, limit)
|
||||||
return [row["value"] for row in rows if row.get("value") is not None]
|
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,
|
max_values: int = DEFAULT_DISTINCT_VALUES_LIMIT,
|
||||||
) -> DistinctValues:
|
) -> DistinctValues:
|
||||||
|
max_values = require_positive_int(max_values, name="max_values")
|
||||||
values = sample_column(engine, schema, table, column, limit=max_values + 1)
|
values = sample_column(engine, schema, table, column, limit=max_values + 1)
|
||||||
return DistinctValues(values=values[:max_values], truncated=len(values) > max_values)
|
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,
|
max_values: int = DEFAULT_DISTINCT_VALUES_LIMIT,
|
||||||
) -> DistinctValues:
|
) -> DistinctValues:
|
||||||
|
max_values = require_positive_int(max_values, name="max_values")
|
||||||
values = sample_column_rest(client, schema, table, column, limit=max_values + 1)
|
values = sample_column_rest(client, schema, table, column, limit=max_values + 1)
|
||||||
return DistinctValues(values=values[:max_values], truncated=len(values) > max_values)
|
return DistinctValues(values=values[:max_values], truncated=len(values) > max_values)
|
||||||
|
|
||||||
|
|||||||
@@ -26,6 +26,13 @@ class PlanSummary:
|
|||||||
node_types: list[str]
|
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]:
|
def _inject_limit(sql: str, limit: int) -> tuple[str, bool]:
|
||||||
"""Aggiunge LIMIT limit+1 se assente (il +1 serve a rilevare il troncamento).
|
"""Aggiunge LIMIT limit+1 se assente (il +1 serve a rilevare il troncamento).
|
||||||
Se la query ha gia' un suo LIMIT, lo si rispetta."""
|
Se la query ha gia' un suo LIMIT, lo si rispetta."""
|
||||||
|
|||||||
@@ -9,7 +9,14 @@ il client mantiene solo l'iniezione del LIMIT (per il rilevamento del troncament
|
|||||||
|
|
||||||
import time
|
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.client import RestError
|
||||||
from tht.rest.explain import parse_text_plan
|
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:
|
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
|
# Guard read-only client-side anche sul path REST (D7): non delegare l'unica verifica
|
||||||
# al server. Stesso check strutturale del path diretto.
|
# al server. Stesso check strutturale del path diretto.
|
||||||
if limit <= 0:
|
limit = require_positive_int(limit, name="limit")
|
||||||
raise ValueError("limit must be a positive integer")
|
|
||||||
assert_read_only(sql)
|
assert_read_only(sql)
|
||||||
final_sql, injected = _inject_limit(sql, limit)
|
final_sql, injected = _inject_limit(sql, limit)
|
||||||
start = time.monotonic()
|
start = time.monotonic()
|
||||||
|
|||||||
Reference in New Issue
Block a user