From 0753ed7bc570f32f3402f09dec7c60e588813115 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 21 Oct 2026 22:54:35 +0200 Subject: [PATCH 01/14] fix(tests): the overlay CLI tests drop the debug level tests/cli/conftest.py leaks tests/cli/conftest.py sets FLUID_LOG_LEVEL=DEBUG, and some xdist workers keep it. The CLI subprocesses here inherited it, so the loader's DEBUG copy of a warning the command reports was printed too. The tests that count the warning once then failed with `assert 2 == 1`, as #734's TestValidateStrict did on main's CI run for 1320619b. The subprocesses now drop FLUID_LOG_LEVEL, FLUID_DEBUG and FLUID_SHOW_REGISTRATION, as tests/output_ports already do, with API keys as before. Same change as #735's 57abff3d, so #735 rebases onto it cleanly. --- tests/test_overlay_keyed_merge.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/test_overlay_keyed_merge.py b/tests/test_overlay_keyed_merge.py index 5c11e0dc..c8d823b9 100644 --- a/tests/test_overlay_keyed_merge.py +++ b/tests/test_overlay_keyed_merge.py @@ -427,6 +427,20 @@ def _write_product(tmp_path: Path, version: str, overlay: Dict[str, Any]) -> Non #: Written against ``[ingest, transform]``: its second entry patches transform. PROD_BY_POSITION = {"builds": [{}, {"id": "transform", "description": "8 executors"}]} +#: Environment variables a CLI subprocess here must not inherit: API keys, and +#: the debug level ``tests/cli/conftest.py`` sets, which some xdist workers keep. +#: At DEBUG the loader also logs a warning the command reports, and the tests +#: below count that the terminal shows it once. +_NOT_INHERITED = ("FLUID_LOG_LEVEL", "FLUID_DEBUG", "FLUID_SHOW_REGISTRATION") + + +def _cli_env() -> Dict[str, str]: + return { + k: v + for k, v in os.environ.items() + if not k.endswith("_API_KEY") and k not in _NOT_INHERITED + } + def _validate(tmp_path: Path, *argv: str) -> "subprocess.CompletedProcess[str]": return subprocess.run( @@ -435,7 +449,7 @@ def _validate(tmp_path: Path, *argv: str) -> "subprocess.CompletedProcess[str]": cwd=str(tmp_path), capture_output=True, text=True, - env={k: v for k, v in os.environ.items() if not k.endswith("_API_KEY")}, + env=_cli_env(), ) @@ -515,7 +529,7 @@ def test_workspace_validate_strict_fails_on_it(tmp_path: Path) -> None: product.mkdir(parents=True) _write_product(product, "0.7.5", PROD_BY_POSITION) run = [sys.executable, "-m", "fluid_build.cli", "validate", "--env", "prod", "--offline"] - env = {k: v for k, v in os.environ.items() if not k.endswith("_API_KEY")} + env = _cli_env() lenient = subprocess.run(run, cwd=tmp_path, capture_output=True, text=True, env=env) strict = subprocess.run( run + ["--strict"], cwd=tmp_path, capture_output=True, text=True, env=env From 6cb97481ee15e37797929de719c6278bd8fdbfd2 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 21 Oct 2026 22:54:48 +0200 Subject: [PATCH 02/14] fix(duckdb): run on DuckDB 2.0 as well as 1.5.x DuckDB 2.0.0 (2026-10-21) turned main red: 51 tests across Test 3.11-3.14, Coverage and the DuckDB e2e legs. Python 3.10 stayed green because DuckDB 2.0 needs 3.11, so pip resolves 1.5.6 there. The floor stays duckdb>=1.5.0 and no ceiling is added; one small seam, providers/_duckdb_compat.py (the shape of fluid_build/_mcp_compat.py), holds what differs. - Relations have no fetchone() in 2.0, and a con.execute() result streams and owns its connection until read to the end: after fetchone() on a one-row COUNT(*), the next relation read, register or create_function raises ResourceInUseException. first_row() reads to the end on both versions; all 18 DuckDB fetchone sites use it (verify, local provider, contract_tests, duckdb runner, late-arrival split, embedded-SQL parse, local validation, diff live, verify masking). An AST fence in tests/providers/test_duckdb_compat.py refuses fetchone/fetchmany/fetch_df_chunk/fetch_arrow_table/fetch_record_batch anywhere in fluid_build except modules of other database clients. - A database file 2.0 creates uses storage version 69, which 1.5.6 cannot open. secure_duckdb_connect now passes storage_compatibility_version='v1.5.0' at connect on 2.x only (a SET after open is too late for the main file), so the files forge-cli creates stay readable by every supported DuckDB; a file that exists keeps its format, a caller's own setting wins, and the setting is locked with the rest. 1.5.x behaviour is unchanged. - 2.0's sqlite scanner goes through DuckDB's file system, so the sandbox allowlist now bounds it (outside file, ./.., symlink and ATTACH refused). The pin that documents 1.5's unbounded scanner runs on 1.5 only, and a new one pins 2.0's bound. The engine still keeps contract SQL off any connection that loads a scanner (postgres stays unbounded). Sources: duckdb-python v2.0.0 release notes ("Upgrading from 1.x"); https://duckdb.org/docs/2.0/clients/python/migration.html; https://duckdb.org/docs/2.0/operations_manual/migration.html; narwhals-dev/narwhals#4078 (the fetchall pattern). --- CHANGELOG.md | 18 ++ fluid_build/build_runners/_embedded_sql_io.py | 3 +- fluid_build/build_runners/_late_arrival.py | 15 +- fluid_build/build_runners/duckdb/runner.py | 19 +- fluid_build/cli/_diff_live.py | 13 +- fluid_build/cli/_verify_masking.py | 4 +- fluid_build/cli/verify.py | 3 +- fluid_build/contract_tests.py | 5 +- fluid_build/providers/_duckdb_compat.py | 72 +++++ fluid_build/providers/_duckdb_sandbox.py | 33 ++- fluid_build/providers/local/local.py | 7 +- fluid_build/providers/local_validation.py | 5 +- .../build_runners/test_consumes_resolution.py | 2 +- .../test_plan_apply_mode_agreement.py | 10 +- tests/providers/test_duckdb_compat.py | 246 ++++++++++++++++++ tests/providers/test_duckdb_sandbox.py | 51 +++- tests/test_e2e_local_apply.py | 2 +- tests/test_fix_a4_bugs.py | 2 +- 18 files changed, 455 insertions(+), 55 deletions(-) create mode 100644 fluid_build/providers/_duckdb_compat.py create mode 100644 tests/providers/test_duckdb_compat.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b5643c62..45094123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Upgrade notes +- forge-cli runs on DuckDB 2.x as well as 1.5.x (`duckdb>=1.5.0`, no ceiling; DuckDB 2.0 + needs Python 3.11, so a Python 3.10 install stays on 1.5.x). On 2.x, a DuckDB database + file forge-cli creates (`fluid init`'s `.fluid/db.duckdb`, the local provider's + session and `persist` databases, an acquisition's DuckDB destination) is written in + DuckDB 1.5's storage format, so a 1.5 install can still open it; a file that already + exists keeps its format. DuckDB 2.x's own default format cannot be read by 1.5: a file + another 2.x tool created needs every process that opens it on 2.x, or an `EXPORT + DATABASE` on 2.x and an `IMPORT DATABASE` on 1.5. - `fluid validate --env --strict` (on one contract or a whole workspace), `fluid contract-validation --env --strict` and `fluid test --env --strict` (the native engine; `--engine soda` does not read `--strict`) now fail when an overlay patch @@ -52,6 +60,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 here`, or the rule's own `description`, instead of quoting the whole object and the subschema it must not match. +### Fixed + +- **DuckDB 2.0.** `fluid verify` reported every local file as `error` ("Relations no + longer have fetchone()"); `fluid apply`'s local SQL and materialize steps and + `contract_tests` actions lost their row counts (a materialize step logged + `local_materialize_fallback`, a `contract_tests` action reported `-1` rows). DuckDB 2.0 + relations have no `fetchone()`, and a query result keeps its connection busy until it + is read to the end, which fails the next step on that connection. Every DuckDB result + is now read to the end, the same way on 1.5.x and 2.x. + ## [0.21.0] — 2026-10-19 ### Upgrade notes diff --git a/fluid_build/build_runners/_embedded_sql_io.py b/fluid_build/build_runners/_embedded_sql_io.py index 6db965aa..95d78ec6 100644 --- a/fluid_build/build_runners/_embedded_sql_io.py +++ b/fluid_build/build_runners/_embedded_sql_io.py @@ -471,12 +471,13 @@ def relations_read(sql: str) -> Optional[FrozenSet[str]]: if not str(sql or "").strip(): return None try: + from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import DuckDBAllowlist, secure_duckdb_connect # Parsed only, never bound or run: no file, URL or extension at all. con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none()) try: - row = con.execute("SELECT json_serialize_sql(?)", [str(sql)]).fetchone() + row = first_row(con.execute("SELECT json_serialize_sql(?)", [str(sql)])) finally: con.close() doc = json.loads(row[0]) if row and row[0] else {} diff --git a/fluid_build/build_runners/_late_arrival.py b/fluid_build/build_runners/_late_arrival.py index 95209bd6..bdead4ff 100644 --- a/fluid_build/build_runners/_late_arrival.py +++ b/fluid_build/build_runners/_late_arrival.py @@ -370,6 +370,7 @@ def split_late_events_in_duckdb( can call this too. """ # ── input validation ──────────────────────────────────────────── + from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._sql_safety import validate_ident validate_ident(source_relation) @@ -380,9 +381,9 @@ def split_late_events_in_duckdb( return {"on_time": 0, "late": 0} # ── compute watermark ──────────────────────────────────────────── - watermark_row = con.execute( - f"SELECT max({event_time_column}) FROM {source_relation}" - ).fetchone() + watermark_row = first_row( + con.execute(f"SELECT max({event_time_column}) FROM {source_relation}") + ) watermark = watermark_row[0] if watermark_row else None if watermark is None: # Empty source — no enforcement possible. @@ -401,9 +402,9 @@ def split_late_events_in_duckdb( ) # ── append late rows to side-output ────────────────────────────── - late_count = con.execute( - f"SELECT count(*) FROM {source_relation} WHERE {threshold_expr}" - ).fetchone()[0] + late_count = first_row( + con.execute(f"SELECT count(*) FROM {source_relation} WHERE {threshold_expr}") + )[0] if late_count > 0: con.execute( f"INSERT INTO {side_output_relation} " @@ -411,7 +412,7 @@ def split_late_events_in_duckdb( ) con.execute(f"DELETE FROM {source_relation} WHERE {threshold_expr}") - on_time_count = con.execute(f"SELECT count(*) FROM {source_relation}").fetchone()[0] + on_time_count = first_row(con.execute(f"SELECT count(*) FROM {source_relation}"))[0] return {"on_time": int(on_time_count), "late": int(late_count)} diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 7dd7d0df..56b26c80 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -57,6 +57,7 @@ ) from fluid_build.api.schema import SchemaFingerprint from fluid_build.api.source import AcquisitionMode +from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, confine_declared, @@ -1253,9 +1254,9 @@ def _persist_cursor_after_run( quoted_path = quote_ansi_string_literal(out_path) con = _connect_for_destination(ctx) try: - row = con.execute( - f"SELECT MAX({cursor_field_safe}) FROM {reader}({quoted_path})" - ).fetchone() + row = first_row( + con.execute(f"SELECT MAX({cursor_field_safe}) FROM {reader}({quoted_path})") + ) finally: con.close() new_value = row[0] if row is not None else None @@ -1409,14 +1410,14 @@ def _execute(ctx: RunContext, runner: DuckdbRunner) -> RunResult: # select re-reads the source, which a live database may have # changed since, and a table holding exactly what the COPY # wrote then failed verify against the larger number. - written = con.execute(copy_sql).fetchone() + written = first_row(con.execute(copy_sql)) if written is not None and isinstance(written[0], int): n = written[0] else: counted_at_write = False # Best-effort via COUNT on the same select. try: - row = con.execute(f"SELECT COUNT(*) FROM ({landed_sel})").fetchone() + row = first_row(con.execute(f"SELECT COUNT(*) FROM ({landed_sel})")) n = row[0] if row is not None else 0 except Exception: n = 0 @@ -1857,7 +1858,7 @@ def _artifact_exists(con: Any, path: str, reader: str) -> bool: if not _is_remote_uri(path): return Path(path).exists() try: - con.execute(f"SELECT 1 FROM {reader}({quote_ansi_string_literal(path)}) LIMIT 1").fetchone() + first_row(con.execute(f"SELECT 1 FROM {reader}({quote_ansi_string_literal(path)}) LIMIT 1")) return True except Exception: # noqa: BLE001 return False @@ -1944,9 +1945,9 @@ def _count_file_rows(path: str, sink_format: str) -> int: ] con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_paths(path)) try: - row = con.execute( - f"SELECT COUNT(*) FROM {reader}({quote_ansi_string_literal(path)})" - ).fetchone() + row = first_row( + con.execute(f"SELECT COUNT(*) FROM {reader}({quote_ansi_string_literal(path)})") + ) finally: con.close() return int(row[0]) if row else 0 diff --git a/fluid_build/cli/_diff_live.py b/fluid_build/cli/_diff_live.py index eca88b6d..3b117b62 100644 --- a/fluid_build/cli/_diff_live.py +++ b/fluid_build/cli/_diff_live.py @@ -644,6 +644,7 @@ def _duckdb_table_exists(path: Path, schema_name: str, table: str) -> bool: checked by the provider's own allowlist first and are bound as parameters, never spliced into the SQL. """ + from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import DuckDBAllowlist, secure_duckdb_connect from fluid_build.providers.local_validation import _build_duckdb_table_ref @@ -652,11 +653,13 @@ def _duckdb_table_exists(path: Path, schema_name: str, table: str) -> bool: # access beyond the database file itself. con = secure_duckdb_connect(path, allow=DuckDBAllowlist.none(), read_only=True) try: - row = con.execute( - "SELECT count(*) FROM information_schema.tables " - "WHERE lower(table_schema) = lower(?) AND lower(table_name) = lower(?)", - [schema_name, table], - ).fetchone() + row = first_row( + con.execute( + "SELECT count(*) FROM information_schema.tables " + "WHERE lower(table_schema) = lower(?) AND lower(table_name) = lower(?)", + [schema_name, table], + ) + ) finally: con.close() return bool(row and row[0]) diff --git a/fluid_build/cli/_verify_masking.py b/fluid_build/cli/_verify_masking.py index 61985753..6f162ec7 100644 --- a/fluid_build/cli/_verify_masking.py +++ b/fluid_build/cli/_verify_masking.py @@ -215,7 +215,9 @@ def local_dimension( ) values: List[int] = [] if selects: - row = con.execute(f"SELECT {', '.join(selects)} FROM ({relation_sql})").fetchone() + from fluid_build.providers._duckdb_compat import first_row + + row = first_row(con.execute(f"SELECT {', '.join(selects)} FROM ({relation_sql})")) values = [int(v or 0) for v in row] return finish(values) diff --git a/fluid_build/cli/verify.py b/fluid_build/cli/verify.py index 8cec840e..2b5da6e1 100644 --- a/fluid_build/cli/verify.py +++ b/fluid_build/cli/verify.py @@ -960,6 +960,7 @@ def _verify_local_file( # Introspect with DuckDB. try: + from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, secure_duckdb_connect, @@ -979,7 +980,7 @@ def _verify_local_file( relation_sql = f"SELECT * FROM read_csv_auto({literal})" rel = con.sql(relation_sql) - row_count = rel.aggregate("count(*)").fetchone()[0] + row_count = first_row(rel.aggregate("count(*)"))[0] actual_columns = [col for col in rel.columns] # Did the columns policy.privacy.masking names land treated? Counts only: # a value that fails the check is cleartext and stays out of the report. diff --git a/fluid_build/contract_tests.py b/fluid_build/contract_tests.py index f36759c6..a500625b 100644 --- a/fluid_build/contract_tests.py +++ b/fluid_build/contract_tests.py @@ -53,6 +53,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple, Union +from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, DuckDBSandboxError, @@ -243,7 +244,7 @@ def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Opt if dry_run: # Estimate count without writing try: - cnt = rel.count("*").fetchone()[0] + cnt = first_row(rel.count("*"))[0] except Exception as e: LOGGER.warning(f"Failed to count rows in dry run: {e}") cnt = -1 @@ -309,7 +310,7 @@ def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Opt # Count rows try: - cnt = rel.count("*").fetchone()[0] + cnt = first_row(rel.count("*"))[0] except Exception as e: LOGGER.warning(f"Failed to count output rows: {e}") cnt = -1 diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py new file mode 100644 index 00000000..799c16fa --- /dev/null +++ b/fluid_build/providers/_duckdb_compat.py @@ -0,0 +1,72 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Version-compat seam for the DuckDB Python client (1.5.x and 2.x). + +forge-cli takes ``duckdb>=1.5.0`` with no ceiling (the floor is the sandbox's, +:data:`fluid_build.providers._duckdb_sandbox.MIN_DUCKDB_VERSION`). DuckDB 2.0 +needs Python 3.11, so a Python 3.10 install stays on 1.5.x while a newer one +gets 2.x: the engine runs on both, and what differs lives here. + +Two 2.0 changes reach the engine (duckdb-python v2.0.0 release notes, +"Upgrading from 1.x"; https://duckdb.org/docs/2.0/clients/python/migration.html): + +* A ``con.execute()`` result streams, and owns its connection until it is read + to the end. ``fetchone()`` on a one-row ``COUNT(*)`` leaves it open, and the + next relation read, ``register`` or ``create_function`` on that connection + raises ``ResourceInUseException``. Relations lost ``fetchone()`` and + ``fetchmany()`` outright. :func:`first_row` reads to the end on both. +* A database file 2.0 creates uses storage version 2.0, which no 1.5 process can + open ("can only read versions between 64 and 68"). :func:`storage_compat_config` + keeps the files forge-cli creates readable by every DuckDB it supports. + +Nothing here imports duckdb: callers pass what they already hold. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional, Tuple + +#: The storage format of the oldest DuckDB forge-cli supports. Database files +#: forge-cli creates on DuckDB 2.x are written in it, so a 1.5 install (Python +#: 3.10, or a tool pinned below 2) can still open them. +STORAGE_COMPATIBILITY_VERSION = "v1.5.0" + + +def first_row(result: Any) -> Optional[Tuple[Any, ...]]: + """The first row of ``result`` (a ``con.execute()`` result or a relation), or ``None``. + + Reads the whole result, so give it a query of one row (``COUNT(*)``, + ``LIMIT 1``). ``fetchone()`` does not do: on DuckDB 2.0 a result read only + partway keeps its connection busy, and a relation has no ``fetchone()``. + """ + rows = result.fetchall() + return rows[0] if rows else None + + +def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: + """``duckdb.connect`` config that keeps files DuckDB ``version`` creates readable by 1.5. + + Empty before 2.0, where every file is readable by 1.5 already and the + client's own default stays. From 2.0, ``storage_compatibility_version`` + set at connect (a ``SET`` once the file is open is too late) makes each + database file the connection creates, ``ATTACH`` included, use the 1.5 + format. A file that exists keeps the format it has. + """ + if version >= (2, 0, 0): + return {"storage_compatibility_version": STORAGE_COMPATIBILITY_VERSION} + return {} + + +__all__ = ["STORAGE_COMPATIBILITY_VERSION", "first_row", "storage_compat_config"] diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index c23ac606..ce597a5b 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -100,13 +100,15 @@ load (``sqlite_scan``, ``read_xlsx``, ``ST_Read``, ``delta_scan``, ``iceberg_scan``) is not in the catalog. :func:`is_sandbox_refusal` treats that error as a refusal. -* The ``sqlite``, ``postgres`` and ``mysql`` scanners open files and sockets - through their own client libraries, not through DuckDB's file system, so - neither ``allowed_directories`` nor ``enable_external_access`` bounds them: - once ``sqlite`` is loaded, ``sqlite_scan`` / ``ATTACH ... (TYPE sqlite)`` - open any SQLite file the process can read, and once ``postgres`` is loaded, - ``postgres_scan`` reaches any host the process can. Only the engine's own - SQL runs on a connection that loads them; contract SQL never does. +* The ``postgres`` and ``mysql`` scanners open sockets through their own + client libraries, not through DuckDB's file system, so neither + ``allowed_directories`` nor ``enable_external_access`` bounds them: once + ``postgres`` is loaded, ``postgres_scan`` reaches any host the process can. + Through DuckDB 1.5 the ``sqlite`` scanner opens files that way too, so + ``sqlite_scan`` / ``ATTACH ... (TYPE sqlite)`` open any SQLite file the + process can read; from 2.0 it goes through DuckDB's file system and the + allowlist bounds it. Only the engine's own SQL runs on a connection that + loads a scanner; contract SQL never does. * DuckDB shares one database instance per file per process, and the lock is instance-wide: a second connection to a file database that is already open cannot be sandboxed, and is refused with :class:`DuckDBSandboxError`. Each @@ -145,6 +147,7 @@ from fluid_build._errors import FluidUserError, doc_url +from ._duckdb_compat import storage_compat_config from ._sql_safety import quote_ansi_string_literal, validate_ident if TYPE_CHECKING: # pragma: no cover - typing only @@ -796,7 +799,8 @@ def duckdb_version(module: Any) -> Tuple[int, int, int]: return int(match.group(1)), int(match.group(2)), int(match.group(3)) -def _require_supported(module: Any) -> None: +def _require_supported(module: Any) -> Tuple[int, int, int]: + """``module``'s version, once it is one the sandbox can confine.""" version = duckdb_version(module) if version < MIN_DUCKDB_VERSION: want = ".".join(str(n) for n in MIN_DUCKDB_VERSION) @@ -810,6 +814,7 @@ def _require_supported(module: Any) -> None: ), fix=f"pip install 'duckdb>={want}'", ) + return version def _sql_list(values: Iterable[str]) -> str: @@ -843,7 +848,10 @@ def secure_duckdb_connect( in, for set-up that needs access the SQL must not have, such as attaching a declared source database or creating an object-store secret. ``config`` takes scalar start-up options (``threads``, ``TimeZone``), which cannot be - changed once the configuration is locked. + changed once the configuration is locked. On DuckDB 2.x a database file the + connection creates is written in DuckDB 1.5's storage format + (:func:`~fluid_build.providers._duckdb_compat.storage_compat_config`), unless + ``config`` names ``storage_compatibility_version`` itself. Raises :class:`DuckDBSandboxError` for an older DuckDB, a bad allowlist entry, or a file ``database`` this process already has open (its one @@ -853,10 +861,13 @@ def secure_duckdb_connect( """ import duckdb - _require_supported(duckdb) + version = _require_supported(duckdb) target = os.fspath(database) + # A database file this creates stays readable by the oldest supported + # DuckDB (see _duckdb_compat); a caller's own setting wins. + options = {**storage_compat_config(version), **dict(config or {})} try: - con = duckdb.connect(target, read_only=read_only, config=dict(config or {})) + con = duckdb.connect(target, read_only=read_only, config=options) except duckdb.Error as exc: if "same database file with a different configuration" in str(exc): raise _already_open(target) from exc diff --git a/fluid_build/providers/local/local.py b/fluid_build/providers/local/local.py index 52e1bee5..6dce3f5c 100644 --- a/fluid_build/providers/local/local.py +++ b/fluid_build/providers/local/local.py @@ -39,6 +39,7 @@ from typing import Any, Dict, List, Optional, Tuple, Union from fluid_build.observability.secret_redactor import redact_secret_text, redact_value +from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_read import build_register_view_sql from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -845,7 +846,7 @@ def _register_with_retry(): with_retry(_register_with_retry, logger=self.logger, max_attempts=3) # Get row count for reporting - rowcount = con.execute(f"SELECT COUNT(*) FROM {validate_ident(table_name)}").fetchone()[ + rowcount = first_row(con.execute(f"SELECT COUNT(*) FROM {validate_ident(table_name)}"))[ 0 ] @@ -987,7 +988,7 @@ def _write_preview(): rowcount = None try: - rowcount = rel.aggregate("count(*)").fetchone()[0] + rowcount = first_row(rel.aggregate("count(*)"))[0] except Exception: pass @@ -1163,7 +1164,7 @@ def _run_copy_action(self, idx: int, action: Dict[str, Any]) -> Dict[str, Any]: try: rel = con.sql(f"SELECT * FROM {validate_ident(source_table)}") self._write_relation(rel, dst, fmt) - rowcount = rel.count("*").fetchone()[0] if hasattr(rel, "count") else -1 + rowcount = first_row(rel.count("*"))[0] if hasattr(rel, "count") else -1 del rel finally: # Closed on every path (see _run_load_data_action). diff --git a/fluid_build/providers/local_validation.py b/fluid_build/providers/local_validation.py index ce176596..aa38ad89 100644 --- a/fluid_build/providers/local_validation.py +++ b/fluid_build/providers/local_validation.py @@ -26,6 +26,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional +from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import DuckDBAllowlist, secure_duckdb_connect from fluid_build.providers._sql_safety import quote_ansi_string_literal, validate_ident from fluid_build.providers.quality_engine import ( @@ -175,7 +176,7 @@ def get_resource_schema(self, resource_spec: Dict[str, Any]) -> Optional[Resourc fields: List[FieldSchema] = [] for row in rows: fields.append(FieldSchema(name=row[0], type=row[1], mode="NULLABLE")) - count_row = conn.execute(f"SELECT COUNT(*) FROM {table_ref}").fetchone() + count_row = first_row(conn.execute(f"SELECT COUNT(*) FROM {table_ref}")) row_count = count_row[0] if count_row else None size_bytes = path.stat().st_size conn.close() @@ -219,7 +220,7 @@ def get_resource_schema(self, resource_spec: Dict[str, Any]) -> Optional[Resourc count_sql = "SELECT COUNT(*) FROM {fn}({p})".format( fn=read_fn, p=quote_ansi_string_literal(abs_path) ) - count_row = conn.execute(count_sql).fetchone() + count_row = first_row(conn.execute(count_sql)) row_count = count_row[0] if count_row else None # File size diff --git a/tests/build_runners/test_consumes_resolution.py b/tests/build_runners/test_consumes_resolution.py index 91e9ffc9..7b6157a4 100644 --- a/tests/build_runners/test_consumes_resolution.py +++ b/tests/build_runners/test_consumes_resolution.py @@ -1131,7 +1131,7 @@ def test_the_high_value_churn_example_builds_as_it_did_before_consumes_resolved( ) assert run_builds_from_args(args, logging.getLogger("t")) == 0, printed out = copy / "output" / "high_value_churn.parquet" - assert duckdb.sql(f"SELECT COUNT(*) FROM '{out}'").fetchone()[0] > 0 + assert duckdb.sql(f"SELECT COUNT(*) FROM '{out}'").fetchall()[0][0] > 0 assert any( "consumes bronze.local.high_value_customers_v1/hv_customers" in line and "lineage only" in line diff --git a/tests/forge/core/pipeline_systems/test_plan_apply_mode_agreement.py b/tests/forge/core/pipeline_systems/test_plan_apply_mode_agreement.py index 52d808d9..df306213 100644 --- a/tests/forge/core/pipeline_systems/test_plan_apply_mode_agreement.py +++ b/tests/forge/core/pipeline_systems/test_plan_apply_mode_agreement.py @@ -228,7 +228,7 @@ def test_generated_plan_and_apply_agree_on_the_mode( assert _flag(apply_argv, "--mode") == apply_mode landed = ws / "contracts" / "p" / "out" / "rows.parquet" if apply_mode in _BUILD_MODES: - assert duckdb.sql(f"SELECT count(*) FROM '{landed}'").fetchone() == (1,) + assert duckdb.sql(f"SELECT count(*) FROM '{landed}'").fetchall() == [(1,)] # A build id reaches ``fluid apply`` only with a build mode. expected_build_id = build_id if (build_id and apply_mode in _BUILD_MODES) else None assert _flag(apply_argv, "--build-id") == expected_build_id @@ -257,7 +257,7 @@ def test_a_parameterless_run_uses_the_declared_defaults( assert _flag(apply_argv, "--build-id") == expected_build_id landed = ws / "contracts" / "p" / "out" / "rows.parquet" if expected_mode in _BUILD_MODES: - assert duckdb.sql(f"SELECT count(*) FROM '{landed}'").fetchone() == (1,) + assert duckdb.sql(f"SELECT count(*) FROM '{landed}'").fetchall() == [(1,)] if expected_mode == "dry-run": assert not landed.exists() # a dry run writes nothing @@ -301,7 +301,7 @@ def test_a_run_on_defaults_applies_the_fluid_env_default( apply_argv = _run_chain(ws, stages, exported, default_env="staging") assert _flag(apply_argv, "--env") == "staging" out = ws / "contracts" / "p" / "out" - assert duckdb.sql(f"SELECT count(*) FROM '{out / 'staging' / 'rows.parquet'}'").fetchone() == ( - 1, - ) + assert duckdb.sql(f"SELECT count(*) FROM '{out / 'staging' / 'rows.parquet'}'").fetchall() == [ + (1,) + ] assert not (out / "rows.parquet").exists() # the base binding was not applied diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py new file mode 100644 index 00000000..a521f964 --- /dev/null +++ b/tests/providers/test_duckdb_compat.py @@ -0,0 +1,246 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""One engine on DuckDB 1.5.x and 2.x: ``providers/_duckdb_compat.py``. + +Each test runs on whichever DuckDB is installed and states what must hold on +both. CI runs the suite on 1.5.x (Python 3.10, which DuckDB 2.0 does not +support) and on 2.x (every newer Python). What 2.0 changed is in the module's +docstring; the cases here are the ones that broke forge-cli's own code. +""" + +from __future__ import annotations + +import ast +from pathlib import Path +from typing import List + +import pytest + +duckdb = pytest.importorskip("duckdb") + +from fluid_build.providers._duckdb_compat import ( # noqa: E402 + STORAGE_COMPATIBILITY_VERSION, + first_row, + storage_compat_config, +) +from fluid_build.providers._duckdb_sandbox import ( # noqa: E402 + DuckDBAllowlist, + duckdb_version, + secure_duckdb_connect, +) + +REPO_ROOT = Path(__file__).resolve().parents[2] +PACKAGE = REPO_ROOT / "fluid_build" +DUCKDB_2 = duckdb_version(duckdb) >= (2, 0, 0) + + +# ── first_row ──────────────────────────────────────────────────────────── + + +def test_first_row_of_an_execute_result(): + assert first_row(duckdb.connect().execute("SELECT 41 + 1, 'x'")) == (42, "x") + + +def test_first_row_of_a_relation(): + """DuckDB 2.0 removed ``fetchone()`` from relations.""" + con = duckdb.connect() + rel = con.sql("SELECT range AS a FROM range(3)") + assert first_row(rel.aggregate("count(*)")) == (3,) + assert first_row(rel.count("*")) == (3,) + + +def test_first_row_of_no_rows_is_none(): + con = duckdb.connect() + assert first_row(con.execute("SELECT 1 WHERE false")) is None + assert first_row(con.sql("SELECT 1 WHERE false")) is None + + +def test_the_connection_is_free_after_first_row(): + """On DuckDB 2.0 a ``con.execute()`` result read only partway (``fetchone()`` + on a one-row ``COUNT(*)``) keeps the connection busy, and the next relation + read, ``register`` or ``create_function`` on it raises + ``ResourceInUseException``. ``first_row`` reads to the end.""" + pd = pytest.importorskip("pandas") + pytest.importorskip("numpy") # DuckDB's create_function needs it + con = duckdb.connect() + con.execute("CREATE TABLE t AS SELECT range AS a FROM range(5)") + + assert first_row(con.execute("SELECT count(*) FROM t")) == (5,) + assert con.sql("SELECT sum(a) FROM t").fetchall() == [(10,)] + assert first_row(con.execute("SELECT a FROM t ORDER BY a LIMIT 1")) == (0,) + con.register("frame", pd.DataFrame({"b": [1, 2]})) + assert first_row(con.execute("SELECT max(a) FROM t")) == (4,) + con.create_function("twice", lambda x: x * 2, ["BIGINT"], "BIGINT") + assert con.sql("SELECT twice(b) FROM frame ORDER BY b").fetchall() == [(2,), (4,)] + + +@pytest.mark.skipif(not DUCKDB_2, reason="the hazard first_row avoids is DuckDB 2.0's") +def test_on_duckdb_2_a_partial_read_keeps_the_connection_busy(): + """What first_row exists for, pinned: if a later DuckDB relaxes this, the + module docstring and this test go together.""" + con = duckdb.connect() + con.execute("CREATE TABLE t AS SELECT range AS a FROM range(5)") + assert con.execute("SELECT count(*) FROM t").fetchone() == (5,) + with pytest.raises(duckdb.Error, match="(?i)resource in use|busy|open result"): + con.sql("SELECT 1").fetchall() + + +# ── storage format ─────────────────────────────────────────────────────── + + +def _storage(con, name: str) -> str: + tags = con.execute( + "SELECT tags FROM duckdb_databases() WHERE database_name = ?", [name] + ).fetchall()[0][0] + return dict(tags)["storage_version"] + + +def test_storage_compat_config_pins_only_from_duckdb_2(): + assert storage_compat_config((1, 5, 0)) == {} + assert storage_compat_config((1, 99, 99)) == {} + for version in ((2, 0, 0), (2, 3, 1), (3, 0, 0)): + assert storage_compat_config(version) == { + "storage_compatibility_version": STORAGE_COMPATIBILITY_VERSION + } + assert STORAGE_COMPATIBILITY_VERSION == "v1.5.0" + + +def test_a_database_file_the_engine_creates_is_readable_by_duckdb_1_5(tmp_path): + """A 2.x install must not write files a 1.5 one (Python 3.10, or a tool still + pinned below 2) cannot open: 1.5.6 reads storage versions 64 to 68 only.""" + db = tmp_path / "new.duckdb" + con = secure_duckdb_connect(db, allow=DuckDBAllowlist.none()) + con.execute("CREATE TABLE t AS SELECT 1 AS a") + if DUCKDB_2: + assert _storage(con, "new") == "v1.5.0+" + else: # 1.5's own default, left alone + assert _storage(con, "new") == "v1.0.0+" + assert con.execute("SELECT current_setting('storage_compatibility_version')").fetchall() == [ + (STORAGE_COMPATIBILITY_VERSION if DUCKDB_2 else "v0.10.2",) + ] + + +def test_an_attached_new_database_follows_the_same_format(tmp_path): + con = secure_duckdb_connect( + allow=DuckDBAllowlist.none(), + before_lock=lambda c: c.execute(f"ATTACH '{tmp_path / 'att.duckdb'}' AS att"), + ) + con.execute("CREATE TABLE att.t AS SELECT 1 AS a") + assert _storage(con, "att") == ("v1.5.0+" if DUCKDB_2 else "v1.0.0+") + + +def test_a_database_file_that_exists_keeps_its_format(tmp_path): + db = tmp_path / "theirs.duckdb" + raw = duckdb.connect(str(db)) + raw.execute("CREATE TABLE t AS SELECT 1 AS a") + before = _storage(raw, "theirs") + raw.close() + + con = secure_duckdb_connect(db, allow=DuckDBAllowlist.none()) + con.execute("INSERT INTO t VALUES (2)") + assert _storage(con, "theirs") == before + assert con.execute("SELECT count(*) FROM t").fetchall() == [(2,)] + con.close() + con = secure_duckdb_connect(db, allow=DuckDBAllowlist.none(), read_only=True) + assert con.execute("SELECT count(*) FROM t").fetchall() == [(2,)] + + +@pytest.mark.skipif(not DUCKDB_2, reason="only 2.x has a newer format to choose") +def test_a_callers_own_storage_setting_wins(tmp_path): + db = tmp_path / "latest.duckdb" + con = secure_duckdb_connect( + db, allow=DuckDBAllowlist.none(), config={"storage_compatibility_version": "latest"} + ) + con.execute("CREATE TABLE t AS SELECT 1 AS a") + assert _storage(con, "latest") != "v1.5.0+" + + +def test_the_storage_setting_is_locked_with_the_rest(tmp_path): + con = secure_duckdb_connect(allow=DuckDBAllowlist.none()) + with pytest.raises(duckdb.Error): + con.execute("SET storage_compatibility_version = 'latest'") + + +# ── no partial reads in DuckDB code ────────────────────────────────────── + +#: Calls DuckDB 2.0 removed from relations or from ``con.execute()`` results, or +#: that leave a result open. ``first_row`` replaces the first two. +_PARTIAL_OR_GONE = frozenset( + {"fetchone", "fetchmany", "fetch_df_chunk", "fetch_arrow_table", "fetch_record_batch"} +) + +#: Modules whose ``fetchone`` belongs to another database's client, never DuckDB's. +_OTHER_CLIENTS = { + "fluid_build/cli/doctor.py": "sqlite3 / Snowflake connector cursors", + "fluid_build/cli/workspace.py": "sqlite3 workspace registry", + "fluid_build/copilot/catalog/snowflake.py": "Snowflake connector cursor", + "fluid_build/copilot/store/backends/postgres.py": "psycopg cursor", + "fluid_build/copilot/store/backends/sqlite.py": "sqlite3 connection", + "fluid_build/copilot/store/backends/vector.py": "sqlite3 + sqlite-vec probe", + "fluid_build/forge_datamodel/from_ddl/snowflake_dumper.py": "Snowflake connector cursor", +} + + +def _partial_reads(path: Path) -> List[str]: + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + where = path.relative_to(REPO_ROOT).as_posix() if REPO_ROOT in path.parents else path.name + return [ + f"{where}:{node.lineno} .{node.func.attr}()" + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr in _PARTIAL_OR_GONE + ] + + +@pytest.mark.parametrize( + "shape", + [ + "con.execute('select 1').fetchone()\n", + "rel.aggregate('count(*)').fetchone()[0]\n", + "con.execute('select 1').fetchmany(2)\n", + "con.execute('select 1').fetch_arrow_table()\n", + "rel.fetch_df_chunk()\n", + "con.execute('select 1').fetch_record_batch(10)\n", + ], +) +def test_the_partial_read_detector_catches_each_shape(shape, tmp_path): + sample = tmp_path / "probe.py" + sample.write_text(shape, encoding="utf-8") + assert _partial_reads(sample), shape + + +def test_no_partial_reads_in_duckdb_code(): + offenders = [] + for path in sorted(PACKAGE.rglob("*.py")): + if path.relative_to(REPO_ROOT).as_posix() in _OTHER_CLIENTS: + continue + offenders.extend(_partial_reads(path)) + assert not offenders, ( + "On DuckDB 2.0 a relation has no fetchone/fetchmany, and a con.execute() " + "result read partway keeps its connection busy (the next relation read, " + "register or create_function raises ResourceInUseException). Use " + "fluid_build.providers._duckdb_compat.first_row with a one-row query; if the " + "call is another database's client, list the module in _OTHER_CLIENTS:\n " + + "\n ".join(offenders) + ) + + +def test_every_other_client_module_still_needs_its_exemption(): + """An exemption outlives its reason silently; a stale one would hide DuckDB code.""" + for module in _OTHER_CLIENTS: + path = REPO_ROOT / module + assert path.is_file(), module + assert _partial_reads(path), f"{module} no longer calls fetchone; drop its exemption" diff --git a/tests/providers/test_duckdb_sandbox.py b/tests/providers/test_duckdb_sandbox.py index f5796ecb..d6eb3e0a 100644 --- a/tests/providers/test_duckdb_sandbox.py +++ b/tests/providers/test_duckdb_sandbox.py @@ -1154,11 +1154,10 @@ def _with_extension(name: str) -> Any: ) from exc -def test_a_loaded_sqlite_scanner_is_not_bounded_by_the_allowlist(tmp_path): - """Pins a documented limit: once ``sqlite`` is loaded, any SQLite file the - process can read is reachable, whatever the allowlist says. A call site that - loads it must keep contract SQL off the connection.""" - db = tmp_path / "elsewhere.sqlite" +_DUCKDB_2 = sandbox.duckdb_version(duckdb) >= (2, 0, 0) + + +def _seed_sqlite(db: Path) -> None: seed = duckdb.connect() try: seed.execute("INSTALL sqlite; LOAD sqlite") @@ -1168,10 +1167,52 @@ def test_a_loaded_sqlite_scanner_is_not_bounded_by_the_allowlist(tmp_path): seed.execute(f"CREATE TABLE s.t AS SELECT '{SECRET}' AS v") seed.close() + +@pytest.mark.skipif(_DUCKDB_2, reason="DuckDB 2.0 bounds the sqlite scanner (next test)") +def test_a_loaded_sqlite_scanner_is_not_bounded_by_the_allowlist(tmp_path): + """Pins a documented limit through DuckDB 1.5: once ``sqlite`` is loaded, + any SQLite file the process can read is reachable, whatever the allowlist + says. A call site that loads it must keep contract SQL off the connection.""" + db = tmp_path / "elsewhere.sqlite" + _seed_sqlite(db) + con = _with_extension("sqlite") assert con.execute(f"SELECT v FROM sqlite_scan('{db}', 't')").fetchall() == [(SECRET,)] +@pytest.mark.skipif(not _DUCKDB_2, reason="through DuckDB 1.5 the scanner is unbounded") +def test_from_duckdb_2_a_loaded_sqlite_scanner_is_bounded_by_the_allowlist(tmp_path): + """DuckDB 2.0's sqlite scanner opens files through DuckDB's file system, so + the allowlist bounds it as it bounds ``read_csv``: a file outside, ``./..`` + out of an allowed directory and a symlink out of one are each refused, + while a file inside is read. The engine still keeps contract SQL off a + connection that loads a scanner (``postgres`` stays unbounded).""" + inside, outside = tmp_path / "allowed", tmp_path / "outside" + inside.mkdir() + outside.mkdir() + _seed_sqlite(outside / "x.sqlite") + _seed_sqlite(inside / "x.sqlite") + (inside / "link.sqlite").symlink_to(outside / "x.sqlite") + + try: + con = secure_duckdb_connect( + allow=DuckDBAllowlist.none().with_dirs(inside), extensions=["sqlite"] + ) + except duckdb.Error as exc: + raise pytest.skip.Exception("the sqlite extension is not installable here") from exc + assert con.execute(f"SELECT v FROM sqlite_scan('{inside}/x.sqlite', 't')").fetchall() == [ + (SECRET,) + ] + for refused in ( + f"SELECT v FROM sqlite_scan('{outside}/x.sqlite', 't')", + f"SELECT v FROM sqlite_scan('{inside}/./../outside/x.sqlite', 't')", + f"SELECT v FROM sqlite_scan('{inside}/link.sqlite', 't')", + f"ATTACH '{outside}/x.sqlite' AS o (TYPE sqlite)", + ): + with pytest.raises(duckdb.PermissionException): + con.execute(refused).fetchall() + + def test_a_loaded_postgres_scanner_is_not_bounded_by_the_allowlist(): """Pins a documented limit: ``postgres_scan`` opens its own socket, so the failure is the server's (nothing listens), not the sandbox's refusal.""" diff --git a/tests/test_e2e_local_apply.py b/tests/test_e2e_local_apply.py index e2cb09a8..e2d30016 100644 --- a/tests/test_e2e_local_apply.py +++ b/tests/test_e2e_local_apply.py @@ -209,7 +209,7 @@ def test_two_apply_runs_produce_consistent_row_count(self, hello_world_workspace output_csv = hello_world_workspace / "runtime" / "out" / "hello-world-v1.csv" # ``all_varchar=True`` for the same reason as the schema test — # avoids materialising TIMESTAMPTZ which needs pytz at runtime. - row_count = duckdb.read_csv(str(output_csv), all_varchar=True).count("*").fetchone()[0] + row_count = duckdb.read_csv(str(output_csv), all_varchar=True).count("*").fetchall()[0][0] assert row_count == 1, f"expected 1 row after two applies; got {row_count}" diff --git a/tests/test_fix_a4_bugs.py b/tests/test_fix_a4_bugs.py index 3672c635..54347818 100644 --- a/tests/test_fix_a4_bugs.py +++ b/tests/test_fix_a4_bugs.py @@ -356,7 +356,7 @@ def test_parquet_apply_creates_parquet_file(self, tmp_path: Path, monkeypatch): assert Path(out_path).exists(), "parquet file must exist after apply" # Confirm it's readable parquet con = duckdb.connect(":memory:") - rows = con.sql(f"SELECT COUNT(*) FROM read_parquet({out_path!r})").fetchone()[0] + rows = con.sql(f"SELECT COUNT(*) FROM read_parquet({out_path!r})").fetchall()[0][0] assert rows >= 1 From aa6b63f9fd8e85f9ccf0b5ffff7f1f4032121b91 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:24:57 +0200 Subject: [PATCH 03/14] fix(duckdb): SQL that ends in DDL fails the same on 1.5 and 2.0 On DuckDB 2.0, con.sql() of SQL whose last statement returns no rows (CREATE TABLE ... AS, an INSERT without RETURNING) returns a relation over the row count where 1.5 returned None. A local SQL action or contract_tests action with such SQL then wrote a one-row "Count" file as its output and reported ok; on 1.5 it failed on 'NoneType' object has no attribute 'write_csv'. query_relation() in providers/_duckdb_compat.py restores 1.5's answer on 2.x (by the last statement's type, then by the Count/Success shape for DML), and both call sites now fail with a message saying to end the SQL with a SELECT. Found by two read-only audits of fluid_build against the 2.0 change list. Also from those audits: - providers/local/mocks.py: DuckDB 2.0 refuses CREATE SCHEMA information_schema (a system catalog); on 1.5 it was a no-op. Dropped. - cli/verify.py closes its DuckDB connection when reading the file fails. - confine_declared's docstring no longer says the sqlite scanner is unbounded on every version. - New tests pin what the fetchone fix restored but nothing asserted: a local SQL action's row count and a materialize step that stays "materialize" (on 2.0 it fell back to a skipped noop after writing its file). - tests/test_contract_tests_branches.py's mock pinned the 1.x fetchone call. --- CHANGELOG.md | 5 + fluid_build/cli/verify.py | 41 +++--- fluid_build/contract_tests.py | 15 ++- fluid_build/providers/_duckdb_compat.py | 54 +++++++- fluid_build/providers/_duckdb_sandbox.py | 6 +- fluid_build/providers/local/local.py | 13 +- fluid_build/providers/local/mocks.py | 4 +- tests/providers/test_duckdb_compat.py | 153 ++++++++++++++++++++++- tests/test_contract_tests_branches.py | 2 +- 9 files changed, 258 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45094123..bc77f939 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 relations have no `fetchone()`, and a query result keeps its connection busy until it is read to the end, which fails the next step on that connection. Every DuckDB result is now read to the end, the same way on 1.5.x and 2.x. +- A local SQL action or `contract_tests` action whose SQL ends in a statement that + returns no rows (`CREATE TABLE ... AS`, an `INSERT` without `RETURNING`) now fails with + a message saying to end the SQL with a `SELECT` of the result. DuckDB 1.5 failed there + on `'NoneType' object has no attribute 'write_csv'`, and DuckDB 2.0 wrote a one-row + `Count` file as the action's output and reported success. ## [0.21.0] — 2026-10-19 diff --git a/fluid_build/cli/verify.py b/fluid_build/cli/verify.py index 2b5da6e1..7c23d7cb 100644 --- a/fluid_build/cli/verify.py +++ b/fluid_build/cli/verify.py @@ -969,25 +969,28 @@ def _verify_local_file( # The masking check below runs SQL built from the contract: it reads # the one file being verified and nothing else. con = secure_duckdb_connect(allow=DuckDBAllowlist.none().with_paths(file_path)) - # A SQL string literal, not Python ``repr``: the path now carries the - # contract's directory, and ``repr`` switches to double quotes (a - # DuckDB identifier) or backslash escapes (literal in DuckDB) the - # moment a directory name holds a quote. - literal = quote_ansi_string_literal(file_path.as_posix()) - if actual_fmt == "parquet": - relation_sql = f"SELECT * FROM read_parquet({literal})" - else: - relation_sql = f"SELECT * FROM read_csv_auto({literal})" - rel = con.sql(relation_sql) - - row_count = first_row(rel.aggregate("count(*)"))[0] - actual_columns = [col for col in rel.columns] - # Did the columns policy.privacy.masking names land treated? Counts only: - # a value that fails the check is cleartext and stays out of the report. - from fluid_build.cli._verify_masking import local_dimension - - masking = local_dimension(con, relation_sql, actual_columns, expose_config) - con.close() + try: + # A SQL string literal, not Python ``repr``: the path now carries the + # contract's directory, and ``repr`` switches to double quotes (a + # DuckDB identifier) or backslash escapes (literal in DuckDB) the + # moment a directory name holds a quote. + literal = quote_ansi_string_literal(file_path.as_posix()) + if actual_fmt == "parquet": + relation_sql = f"SELECT * FROM read_parquet({literal})" + else: + relation_sql = f"SELECT * FROM read_csv_auto({literal})" + rel = con.sql(relation_sql) + + row_count = first_row(rel.aggregate("count(*)"))[0] + actual_columns = [col for col in rel.columns] + # Did the columns policy.privacy.masking names land treated? Counts + # only: a value that fails the check is cleartext and stays out of + # the report. + from fluid_build.cli._verify_masking import local_dimension + + masking = local_dimension(con, relation_sql, actual_columns, expose_config) + finally: + con.close() except Exception as exc: return { "status": "error", diff --git a/fluid_build/contract_tests.py b/fluid_build/contract_tests.py index a500625b..e69536cb 100644 --- a/fluid_build/contract_tests.py +++ b/fluid_build/contract_tests.py @@ -53,7 +53,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple, Union -from fluid_build.providers._duckdb_compat import first_row +from fluid_build.providers._duckdb_compat import first_row, query_relation from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, DuckDBSandboxError, @@ -209,10 +209,9 @@ def _execute_sql(con, sql: str): if not sql_stripped: raise LocalProviderError("No SQL provided for action") try: - # DuckDB returns a Relation for SELECT; for CREATE TABLE AS, we can - # run an additional SELECT if needed. We assume a SELECT for portability. - rel = con.sql(sql_stripped) - return rel + # A relation for a query; None when the last statement returns no rows + # (CREATE TABLE AS, INSERT), on DuckDB 1.5 and 2.x alike. + rel = query_relation(con, sql_stripped) except Exception as e: # Try to be helpful if the error hints at missing tables msg = str(e) @@ -222,6 +221,12 @@ def _execute_sql(con, sql: str): "the SQL references the correct table/view names. Error: " + msg ) from e raise LocalProviderError("SQL execution failed: " + msg) from e + if rel is None: + raise LocalProviderError( + "The action's SQL ends in a statement that returns no rows (CREATE, INSERT, " + "UPDATE, ...), so it has no result to write. End it with a SELECT of the result." + ) + return rel def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Optional[str]]: diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index 799c16fa..1cf0ea41 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -19,7 +19,7 @@ needs Python 3.11, so a Python 3.10 install stays on 1.5.x while a newer one gets 2.x: the engine runs on both, and what differs lives here. -Two 2.0 changes reach the engine (duckdb-python v2.0.0 release notes, +Three 2.0 changes reach the engine (duckdb-python v2.0.0 release notes, "Upgrading from 1.x"; https://duckdb.org/docs/2.0/clients/python/migration.html): * A ``con.execute()`` result streams, and owns its connection until it is read @@ -27,6 +27,11 @@ next relation read, ``register`` or ``create_function`` on that connection raises ``ResourceInUseException``. Relations lost ``fetchone()`` and ``fetchmany()`` outright. :func:`first_row` reads to the end on both. +* ``con.sql()`` of SQL whose last statement returns no rows (``CREATE TABLE + ... AS``, an ``INSERT`` without ``RETURNING``) returns a relation over the + statement's row count where 1.5 returned ``None``. Written out as a + contract's result, that is a one-row ``Count`` file. :func:`query_relation` + answers ``None`` for it on both. * A database file 2.0 creates uses storage version 2.0, which no 1.5 process can open ("can only read versions between 64 and 68"). :func:`storage_compat_config` keeps the files forge-cli creates readable by every DuckDB it supports. @@ -55,6 +60,46 @@ def first_row(result: Any) -> Optional[Tuple[Any, ...]]: return rows[0] if rows else None +#: Statement types whose result is a query result (rows), whatever they hold. +#: ``MULTI`` is 2.0's name for ``PIVOT``, which 1.5 called ``SELECT``. +_QUERY_STATEMENTS = frozenset({"SELECT", "EXPLAIN", "CALL", "PRAGMA", "MULTI"}) +#: Statement types that never return rows: 1.5 answered ``None`` for SQL ending +#: in one even after a ``SELECT`` (2.0 then returns that ``SELECT``'s rows). +_NO_ROWS_STATEMENTS = frozenset( + {"CREATE", "DROP", "ALTER", "SET", "VARIABLE_SET", "ATTACH", "DETACH", "TRANSACTION"} + | {"LOAD", "VACUUM", "ANALYZE", "EXPORT", "COPY_DATABASE", "PREPARE"} +) +#: What 2.0 holds for a statement with no query result: the changed-row count +#: (``INSERT``, ``UPDATE``, ``DELETE``, ``MERGE``, ``COPY``), or a success flag. +_NO_RESULT_SHAPES = frozenset({(("Count",), ("BIGINT",)), (("Success",), ("BOOLEAN",))}) + + +def query_relation(con: Any, sql: str) -> Any: + """``con.sql(sql)``, or ``None`` when the SQL's last statement returns no rows. + + What DuckDB 1.5 answered: a relation for a query (``SELECT``, ``CALL``, + ``EXPLAIN``, ``PRAGMA``, a ``RETURNING`` clause), ``None`` for a statement + that only changes things. DuckDB 2.0 returns a relation for both, the + second over a row count, so this restores ``None`` there. The statements + run either way, once. One case stays apart: a ``RETURNING`` clause whose + only column is a ``BIGINT`` named ``Count`` reads as no rows on 2.0. + """ + rel = con.sql(sql) + if rel is None: # DuckDB 1.5 decided already + return None + try: + statements = con.extract_statements(sql) + except Exception: # noqa: BLE001 - it ran, so it parses; keep its answer + return rel + last = statements[-1].type.name if statements else "SELECT" + if last in _QUERY_STATEMENTS: + return rel + if last in _NO_ROWS_STATEMENTS: + return None + shape = (tuple(rel.columns), tuple(str(t) for t in rel.types)) + return None if shape in _NO_RESULT_SHAPES else rel + + def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: """``duckdb.connect`` config that keeps files DuckDB ``version`` creates readable by 1.5. @@ -69,4 +114,9 @@ def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: return {} -__all__ = ["STORAGE_COMPATIBILITY_VERSION", "first_row", "storage_compat_config"] +__all__ = [ + "STORAGE_COMPATIBILITY_VERSION", + "first_row", + "query_relation", + "storage_compat_config", +] diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index ce597a5b..ebf1dfe3 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -454,9 +454,9 @@ def confine_declared( """The ``realpath`` of a declared local ``location``, refused outside ``within``. For a location the engine opens itself rather than grants to the SQL, such - as a SQLite source it ``ATTACH``es before the lock: the sqlite scanner opens - files through its own client library, so ``allowed_directories`` never - bounds it, and this check is the only one. Confined as + as a SQLite source it ``ATTACH``es before the lock: nothing bounds that open + (and through DuckDB 1.5 the sqlite scanner's own client library is never + bounded), so this check is the only one. Confined as :meth:`DuckDBAllowlist.with_declared` confines (``within`` plus ``FLUID_DUCKDB_ALLOWED_DIRS``); a relative ``location`` is resolved against ``base``, else the working directory. Open the returned path, the one that diff --git a/fluid_build/providers/local/local.py b/fluid_build/providers/local/local.py index 6dce3f5c..995a74c3 100644 --- a/fluid_build/providers/local/local.py +++ b/fluid_build/providers/local/local.py @@ -39,7 +39,7 @@ from typing import Any, Dict, List, Optional, Tuple, Union from fluid_build.observability.secret_redactor import redact_secret_text, redact_value -from fluid_build.providers._duckdb_compat import first_row +from fluid_build.providers._duckdb_compat import first_row, query_relation from fluid_build.providers._duckdb_read import build_register_view_sql from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -937,7 +937,7 @@ def _run_sql_on( try: # Execute SQL with retry logic (handles transient DuckDB errors) def _execute_sql(): - return con.sql(sql) + return query_relation(con, sql) rel = with_retry(_execute_sql, logger=self.logger, max_attempts=3) @@ -958,6 +958,15 @@ def _execute_sql(): # error (an error -> traceback -> frame -> error cycle would keep # the connection alive until the cyclic GC runs). raise self._sandbox_refusal(e) from e + if rel is None: + # The same on DuckDB 1.5 and 2.x (see query_relation); 1.5 used to + # fail below on ``None.write_csv``. + raise ValueError( + f"SQL action {idx} ends in a statement that returns no rows (CREATE, " + "INSERT, UPDATE, ...), so it has no result to write. End the SQL with a " + "SELECT of the result; to keep it as a table for later actions, also set " + "output_table." + ) # If an output_table is specified, persist the result as a DuckDB table # so downstream materialize/copy steps can reference it. diff --git a/fluid_build/providers/local/mocks.py b/fluid_build/providers/local/mocks.py index 614747d1..c2957d81 100644 --- a/fluid_build/providers/local/mocks.py +++ b/fluid_build/providers/local/mocks.py @@ -212,8 +212,8 @@ def connect(self) -> Any: self.db_path, allow=DuckDBAllowlist.none(), config={"TimeZone": "UTC"} ) - # Create standard Snowflake schemas if they don't exist - self._con.execute("CREATE SCHEMA IF NOT EXISTS INFORMATION_SCHEMA") + # Snowflake's PUBLIC schema. DuckDB has its own information_schema, + # and DuckDB 2.0 refuses to create one ("system catalog"). self._con.execute("CREATE SCHEMA IF NOT EXISTS PUBLIC") self.logger.info(f"Connected to Snowflake mock at {self.db_path}") diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index a521f964..8bc17495 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -24,7 +24,7 @@ import ast from pathlib import Path -from typing import List +from typing import Any, Dict, List import pytest @@ -33,6 +33,7 @@ from fluid_build.providers._duckdb_compat import ( # noqa: E402 STORAGE_COMPATIBILITY_VERSION, first_row, + query_relation, storage_compat_config, ) from fluid_build.providers._duckdb_sandbox import ( # noqa: E402 @@ -97,6 +98,156 @@ def test_on_duckdb_2_a_partial_read_keeps_the_connection_busy(): con.sql("SELECT 1").fetchall() +# ── query_relation ─────────────────────────────────────────────────────── + + +def _table_t() -> Any: + con = duckdb.connect() + con.execute("CREATE TABLE t AS SELECT range AS a FROM range(3)") + return con + + +@pytest.mark.parametrize( + "sql, columns", + [ + ("SELECT 1 AS a", ["a"]), + ("WITH x AS (SELECT 1 AS a) SELECT * FROM x", ["a"]), + ("FROM t", ["a"]), + ("CREATE TABLE made AS SELECT 1 AS q; SELECT * FROM made", ["q"]), + ("INSERT INTO t VALUES (7) RETURNING a", ["a"]), + ("UPDATE t SET a = a RETURNING a", ["a"]), + ('SELECT count(*) AS "Count" FROM t', ["Count"]), + ("PIVOT t ON a USING count(*)", ["0", "1", "2"]), + ("DESCRIBE t", None), + ("CALL pragma_version()", None), + ("EXPLAIN SELECT 1", None), + ], +) +def test_query_relation_keeps_a_query(sql, columns): + rel = query_relation(_table_t(), sql) + assert rel is not None + if columns is not None: + assert rel.columns == columns + + +@pytest.mark.parametrize( + "sql", + [ + "CREATE TABLE made AS SELECT 1 AS q", + "CREATE OR REPLACE VIEW v AS SELECT 1 AS q", + "INSERT INTO t VALUES (7)", + "UPDATE t SET a = a", + "DELETE FROM t WHERE a = 100", + "MERGE INTO t USING (SELECT 1 AS a) s ON t.a = s.a " + "WHEN NOT MATCHED THEN INSERT VALUES (s.a)", + "DROP TABLE t", + "SET threads = 2", + "SELECT 1 AS x; CREATE TABLE made AS SELECT 1 AS q", + ], +) +def test_query_relation_is_none_for_a_statement_that_returns_no_rows(sql): + """DuckDB 1.5 returned None here; 2.0 returns a relation over a row count, + which an engine writing "the result" would write as a one-row Count file.""" + assert query_relation(_table_t(), sql) is None + + +def test_query_relation_of_a_copy_is_none(tmp_path): + con = _table_t() + assert query_relation(con, f"COPY t TO '{tmp_path / 'x.csv'}'") is None + assert (tmp_path / "x.csv").read_text(encoding="utf-8").splitlines() == ["a", "0", "1", "2"] + + +def test_query_relation_runs_each_statement_once(): + con = _table_t() + assert query_relation(con, "INSERT INTO t VALUES (7)") is None + assert first_row(con.execute("SELECT count(*) FROM t WHERE a = 7")) == (1,) + assert query_relation(con, "CREATE TABLE made AS SELECT 1 AS q") is None + assert con.execute("SELECT q FROM made").fetchall() == [(1,)] + + +# ── the engine, end to end ─────────────────────────────────────────────── + + +@pytest.fixture +def product(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + contract_dir = tmp_path / "product" + (contract_dir / "data").mkdir(parents=True) + (contract_dir / "data" / "orders.csv").write_text("id,amount\n1,10\n2,20\n", encoding="utf-8") + (tmp_path / "home").mkdir() + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.chdir(contract_dir) + monkeypatch.delenv("FLUID_UPSTREAM_CONTRACTS", raising=False) + return contract_dir + + +def _local_apply(product: Path, *actions: Dict[str, Any]) -> List[Dict[str, Any]]: + from fluid_build.providers.local.local import LocalProvider + + provider = LocalProvider(project="local", region="local", anchor_dir=product) + load = {"op": "load_data", "path": str(product / "data" / "orders.csv"), "table_name": "orders"} + return provider.apply(actions=[load, *actions])["results"] + + +def test_local_apply_counts_rows_and_materializes(product): + """On DuckDB 2.0 the SQL action's row count was lost (``rows: None``) and a + materialize step fell back to a skipped ``noop`` after writing its file.""" + out = product / "out" + results = _local_apply( + product, + { + "op": "sql", + "sql": "SELECT * FROM orders", + "output_table": "t1", + "outputs": [str(out / "t1.parquet")], + }, + {"op": "materialize", "source_table": "t1", "dst": str(out / "copy.csv")}, + ) + assert [r["status"] for r in results] == ["ok", "ok", "ok"], results + assert results[0]["rows"] == 2 + assert results[1]["rows"] == 2 + assert results[2]["op"] == "materialize" + assert (out / "copy.csv").read_text(encoding="utf-8").splitlines() == [ + "id,amount", + "1,10", + "2,20", + ] + + +def test_local_sql_that_returns_no_rows_fails_the_same_on_both_versions(product): + """1.5 failed on ``None.write_csv``; 2.0 wrote a one-row ``Count`` file and + reported ok. Both now refuse it and say what to write instead.""" + out = product / "out" / "made.csv" + results = _local_apply( + product, + {"op": "sql", "sql": "CREATE TABLE made AS SELECT * FROM orders", "outputs": [str(out)]}, + ) + assert results[1]["status"] == "error" + assert "returns no rows" in results[1]["error"] + assert "End the SQL with a SELECT" in results[1]["error"] + assert not out.exists() + + +def test_contract_tests_sql_that_returns_no_rows_fails_clearly(): + from fluid_build.contract_tests import LocalProviderError, _execute_sql + + with pytest.raises(LocalProviderError, match="returns no rows"): + _execute_sql(duckdb.connect(), "CREATE TABLE made AS SELECT 1 AS q") + assert _execute_sql(duckdb.connect(), "SELECT 1 AS q").fetchall() == [(1,)] + + +def test_the_snowflake_mock_connects(tmp_path): + """DuckDB 2.0 refuses ``CREATE SCHEMA information_schema`` (a system catalog).""" + from fluid_build.providers.local.mocks import SnowflakeDuckDBMock + + mock = SnowflakeDuckDBMock(db_path=str(tmp_path / "sf.duckdb")) + con = mock.connect() + assert con.execute("SELECT count(*) FROM information_schema.schemata").fetchall()[0][0] > 0 + assert con.execute( + "SELECT count(*) FROM information_schema.schemata WHERE schema_name = 'PUBLIC'" + ).fetchall() == [(1,)] + con.close() + + # ── storage format ─────────────────────────────────────────────────────── diff --git a/tests/test_contract_tests_branches.py b/tests/test_contract_tests_branches.py index ac3efb86..14cbd922 100644 --- a/tests/test_contract_tests_branches.py +++ b/tests/test_contract_tests_branches.py @@ -233,7 +233,7 @@ def test_invalid_format(self): def test_csv_dry_run(self): rel = MagicMock() count_rel = MagicMock() - count_rel.fetchone.return_value = (42,) + count_rel.fetchall.return_value = [(42,)] rel.count.return_value = count_rel cnt, path = _write_output(rel, {"path": "/tmp/out.csv", "format": "csv"}, dry_run=True) assert cnt == 42 From 9c29f7d183fd1e35e79ebc12633436e04b27a111 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 05:37:53 +0200 Subject: [PATCH 04/14] fix(duckdb): what the 2.0 verification sweep found A verification sweep ran the port against duckdb 1.5.6 and 2.0.0, on Python 3.10, 3.12 and 3.14: sandbox attack probes, about 900 live fluid CLI runs on the 13 quickstart templates and the local examples, a CI-job replay, mutation testing, a drift diff and concurrency probes. Its findings that the port or DuckDB 2.0 caused, fixed: - is_unloaded_function: DuckDB 2.0 no longer names postgres_scan's extension ("Table Function with name postgres_scan does not exist!"), so contract SQL calling it got a bare catalog error instead of the sandbox hint; mysql_scan never had one. Both read as a refusal now. Access was never affected. - MCP DuckDBDriver.health_check ran on the shared connection; on 2.0 a second thread there refuses ("connection is busy") instead of waiting. It takes a cursor, as execute() already does. A concurrency test pins both. - The partial-read fence also catches rel.arrow(), fetch_arrow_reader(), record_batch() and a relation's removed execute(), and fails when it scans nothing (a mutant pointing it at an empty tree survived). - Tests for what mutants showed unpinned: first_row returns the first of several rows; contract_tests' dry run counts a real relation; the duckdb runner's remote-landing probe leaves its connection usable. - CHANGELOG: `fluid demo` (not `fluid init`) creates .fluid/db.duckdb; the pinned format opens on DuckDB 1.5.0 and later; dbt-duckdb and dlt files follow the DuckDB that writes them; contract SQL changed in 2.0 (the `->` lambda, integer // and % by zero). --- CHANGELOG.md | 14 ++-- .../output_ports/mcp/drivers/duckdb.py | 9 +- fluid_build/providers/_duckdb_sandbox.py | 16 +++- tests/output_ports/test_drivers_duckdb.py | 28 +++++++ tests/providers/test_duckdb_compat.py | 83 +++++++++++++++++-- 5 files changed, 135 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc77f939..55bf2068 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,12 +11,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - forge-cli runs on DuckDB 2.x as well as 1.5.x (`duckdb>=1.5.0`, no ceiling; DuckDB 2.0 needs Python 3.11, so a Python 3.10 install stays on 1.5.x). On 2.x, a DuckDB database - file forge-cli creates (`fluid init`'s `.fluid/db.duckdb`, the local provider's - session and `persist` databases, an acquisition's DuckDB destination) is written in - DuckDB 1.5's storage format, so a 1.5 install can still open it; a file that already + file forge-cli creates (`fluid demo`'s `.fluid/db.duckdb`, the local provider's session + and `persist` databases, a Meltano acquisition's DuckDB destination) is written in + DuckDB 1.5's storage format, which DuckDB 1.5.0 and later open; a file that already exists keeps its format. DuckDB 2.x's own default format cannot be read by 1.5: a file - another 2.x tool created needs every process that opens it on 2.x, or an `EXPORT - DATABASE` on 2.x and an `IMPORT DATABASE` on 1.5. + another 2.x tool created (dbt-duckdb and dlt included) needs every process that opens + it on 2.x, or an `EXPORT DATABASE` on 2.x and an `IMPORT DATABASE` on 1.5. +- Contract SQL runs on the DuckDB that is installed, and DuckDB 2.0 changed some SQL: the + `x -> x + 1` lambda is an error (write `lambda x: x + 1`), and integer `//` and `%` by + zero raise an error instead of returning NULL (wrap them in `TRY(...)`). See + https://duckdb.org/docs/2.0/operations_manual/migration.html. - `fluid validate --env --strict` (on one contract or a whole workspace), `fluid contract-validation --env --strict` and `fluid test --env --strict` (the native engine; `--engine soda` does not read `--strict`) now fail when an overlay patch diff --git a/fluid_build/output_ports/mcp/drivers/duckdb.py b/fluid_build/output_ports/mcp/drivers/duckdb.py index 6b97c82d..8b767f4c 100644 --- a/fluid_build/output_ports/mcp/drivers/duckdb.py +++ b/fluid_build/output_ports/mcp/drivers/duckdb.py @@ -193,8 +193,13 @@ def execute( def health_check(self) -> Dict[str, Any]: started = time.monotonic() try: - connection = self._get_connection() - connection.execute("SELECT 1").fetchall() + # A cursor of its own, as execute() uses: on DuckDB 2.0 a call on + # the shared connection refuses while another thread is on it. + cursor = self._get_connection().cursor() + try: + cursor.execute("SELECT 1").fetchall() + finally: + cursor.close() except Exception as exc: # noqa: BLE001 return { "status": "unavailable", diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index ebf1dfe3..e1624102 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -924,14 +924,26 @@ def _already_open(target: str) -> DuckDBSandboxError: ) +#: Scanner functions DuckDB does not name with their extension when it is not +#: loaded: ``mysql_scan`` on every version, ``postgres_scan`` from 2.0. +_UNADVERTISED_EXTENSION_FUNCTIONS = ("postgres_scan", "mysql_scan") + + def is_unloaded_function(exc: BaseException) -> bool: """Whether ``exc`` is a function whose extension autoloading would have loaded. DuckDB's catalog error: ``Table Function with name "sqlite_scan" is not in - the catalog, but it exists in the sqlite_scanner extension``. + the catalog, but it exists in the sqlite_scanner extension``, or, for a + scanner it does not advertise, ``Table Function with name postgres_scan + does not exist!``. """ text = str(exc) - return "but it exists in the" in text and "extension" in text + if "but it exists in the" in text and "extension" in text: + return True + return any( + f"Table Function with name {name} does not exist" in text + for name in _UNADVERTISED_EXTENSION_FUNCTIONS + ) def is_sandbox_refusal(exc: BaseException) -> bool: diff --git a/tests/output_ports/test_drivers_duckdb.py b/tests/output_ports/test_drivers_duckdb.py index 5f329c49..9aa6a0aa 100644 --- a/tests/output_ports/test_drivers_duckdb.py +++ b/tests/output_ports/test_drivers_duckdb.py @@ -82,6 +82,34 @@ def test_health_check_succeeds(tmp_path): assert "latency_ms" in health +def test_concurrent_calls_on_one_driver_all_succeed(tmp_path): + """DuckDB 2.0 refuses a second thread on one connection where 1.5 waited. + health_check and execute each take a cursor of their own, so concurrent + calls on one driver succeed on both versions.""" + import threading + + driver, _ = _build_duckdb_driver(tmp_path) + assert driver.health_check()["status"] == "ok" # the connection now exists + errors = [] + + def work(): + try: + for _ in range(20): + health = driver.health_check() + if health["status"] != "ok": + errors.append(health["detail"]) + driver.sample(limit=1) + except Exception as exc: # noqa: BLE001 - collected for the assert + errors.append(repr(exc)) + + threads = [threading.Thread(target=work) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + assert errors == [] + + def test_sample_returns_three_rows(tmp_path): driver, _ = _build_duckdb_driver(tmp_path) result = driver.sample(limit=10) diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 8bc17495..442fa223 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -62,6 +62,12 @@ def test_first_row_of_a_relation(): assert first_row(rel.count("*")) == (3,) +def test_first_row_is_the_first_of_several_rows(): + con = duckdb.connect() + assert first_row(con.execute("SELECT range FROM range(3) ORDER BY 1")) == (0,) + assert first_row(con.sql("SELECT range FROM range(3) ORDER BY 1")) == (0,) + + def test_first_row_of_no_rows_is_none(): con = duckdb.connect() assert first_row(con.execute("SELECT 1 WHERE false")) is None @@ -235,6 +241,55 @@ def test_contract_tests_sql_that_returns_no_rows_fails_clearly(): assert _execute_sql(duckdb.connect(), "SELECT 1 AS q").fetchall() == [(1,)] +def test_contract_tests_dry_run_counts_a_real_relation(tmp_path): + from fluid_build.contract_tests import _write_output + + rel = duckdb.connect().sql("SELECT range AS a FROM range(42)") + out = tmp_path / "out.csv" + assert _write_output(rel, {"path": str(out), "format": "csv"}, dry_run=True) == (42, None) + assert not out.exists() + + +def test_a_remote_landing_is_probed_without_keeping_the_connection_busy(tmp_path, monkeypatch): + """``_artifact_exists`` probes a remote landing with ``SELECT 1 ... LIMIT 1`` + on the run's own connection; read partway on DuckDB 2.0, that kept the + connection busy for the late-arrival split that reads the landing next.""" + from fluid_build.build_runners.duckdb import runner + + landed = tmp_path / "landed.parquet" + duckdb.connect().execute(f"COPY (SELECT 1 AS a) TO '{landed}' (FORMAT parquet)") + monkeypatch.setattr(runner, "_is_remote_uri", lambda _path: True) + con = duckdb.connect() + assert runner._artifact_exists(con, str(landed), "read_parquet") is True + assert con.sql(f"SELECT a FROM read_parquet('{landed}')").fetchall() == [(1,)] + assert runner._artifact_exists(con, str(tmp_path / "missing.parquet"), "read_parquet") is False + + +@pytest.mark.parametrize( + "function", ["postgres_scan", "postgres_query", "mysql_scan", "sqlite_scan", "read_xlsx"] +) +def test_a_scanner_the_sandbox_did_not_load_reads_as_a_refusal(function): + """DuckDB 2.0 stopped naming ``postgres_scan``'s extension in its error + ("does not exist!"), and no version names ``mysql_scan``'s; contract SQL + calling one still gets the sandbox's hint instead of a bare catalog error.""" + from fluid_build.providers._duckdb_sandbox import is_sandbox_refusal, is_unloaded_function + + con = secure_duckdb_connect(allow=DuckDBAllowlist.none()) + with pytest.raises(duckdb.Error) as refused: + con.execute(f"SELECT * FROM {function}('x', 'y')") + assert is_unloaded_function(refused.value), refused.value + assert is_sandbox_refusal(refused.value) + + +def test_an_unknown_function_is_not_a_sandbox_refusal(): + from fluid_build.providers._duckdb_sandbox import is_sandbox_refusal + + con = secure_duckdb_connect(allow=DuckDBAllowlist.none()) + with pytest.raises(duckdb.Error) as missing: + con.execute("SELECT * FROM no_such_function_anywhere()") + assert not is_sandbox_refusal(missing.value) + + def test_the_snowflake_mock_connects(tmp_path): """DuckDB 2.0 refuses ``CREATE SCHEMA information_schema`` (a system catalog).""" from fluid_build.providers.local.mocks import SnowflakeDuckDBMock @@ -326,13 +381,18 @@ def test_the_storage_setting_is_locked_with_the_rest(tmp_path): # ── no partial reads in DuckDB code ────────────────────────────────────── -#: Calls DuckDB 2.0 removed from relations or from ``con.execute()`` results, or -#: that leave a result open. ``first_row`` replaces the first two. +#: Reads that DuckDB 2.0 removed from relations or ``con.execute()`` results, or +#: that leave a result open (the connection stays busy until it is drained): +#: ``fetchone``/``fetchmany`` (``first_row`` replaces them), the Arrow batch +#: readers, ``rel.arrow()``. A relation's argument-less ``execute()``, also gone, +#: is caught separately. _PARTIAL_OR_GONE = frozenset( {"fetchone", "fetchmany", "fetch_df_chunk", "fetch_arrow_table", "fetch_record_batch"} + | {"fetch_arrow_reader", "record_batch", "arrow"} ) -#: Modules whose ``fetchone`` belongs to another database's client, never DuckDB's. +#: Modules whose ``fetchone`` (or argument-less ``execute``) belongs to another +#: client, never DuckDB's. _OTHER_CLIENTS = { "fluid_build/cli/doctor.py": "sqlite3 / Snowflake connector cursors", "fluid_build/cli/workspace.py": "sqlite3 workspace registry", @@ -341,6 +401,7 @@ def test_the_storage_setting_is_locked_with_the_rest(tmp_path): "fluid_build/copilot/store/backends/sqlite.py": "sqlite3 connection", "fluid_build/copilot/store/backends/vector.py": "sqlite3 + sqlite-vec probe", "fluid_build/forge_datamodel/from_ddl/snowflake_dumper.py": "Snowflake connector cursor", + "fluid_build/providers/gcp/util/auth.py": "googleapiclient request .execute()", } @@ -352,7 +413,10 @@ def _partial_reads(path: Path) -> List[str]: for node in ast.walk(tree) if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) - and node.func.attr in _PARTIAL_OR_GONE + and ( + node.func.attr in _PARTIAL_OR_GONE + or (node.func.attr == "execute" and not node.args and not node.keywords) + ) ] @@ -365,6 +429,10 @@ def _partial_reads(path: Path) -> List[str]: "con.execute('select 1').fetch_arrow_table()\n", "rel.fetch_df_chunk()\n", "con.execute('select 1').fetch_record_batch(10)\n", + "rel.fetch_arrow_reader(1000)\n", + "rel.record_batch()\n", + "rel.arrow()\n", + "rel.execute()\n", ], ) def test_the_partial_read_detector_catches_each_shape(shape, tmp_path): @@ -374,11 +442,14 @@ def test_the_partial_read_detector_catches_each_shape(shape, tmp_path): def test_no_partial_reads_in_duckdb_code(): - offenders = [] + offenders, scanned = [], 0 for path in sorted(PACKAGE.rglob("*.py")): if path.relative_to(REPO_ROOT).as_posix() in _OTHER_CLIENTS: continue + scanned += 1 offenders.extend(_partial_reads(path)) + # A fence that scans nothing passes; it must see the DuckDB modules. + assert scanned > 500, scanned assert not offenders, ( "On DuckDB 2.0 a relation has no fetchone/fetchmany, and a con.execute() " "result read partway keeps its connection busy (the next relation read, " @@ -394,4 +465,4 @@ def test_every_other_client_module_still_needs_its_exemption(): for module in _OTHER_CLIENTS: path = REPO_ROOT / module assert path.is_file(), module - assert _partial_reads(path), f"{module} no longer calls fetchone; drop its exemption" + assert _partial_reads(path), f"{module} no longer needs its exemption; drop it" From 5459776c22095772986d79a49f873ece776b5165 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:02:36 +0200 Subject: [PATCH 05/14] fix(duckdb): the no-rows error names the fix a contract author can make Seen in a live `fluid apply --mode amend-and-build` of a quickstart whose build SQL was a CREATE TABLE ... AS: under the build's heading the error said "SQL action 0 ... also set output_table". A contract author writes neither actions nor output_table. Both call sites now print one message from _duckdb_compat.NO_ROWS_TO_WRITE that ends with a working example (CREATE TABLE t AS SELECT ...; SELECT * FROM t), and a test runs that example through local apply on both DuckDB versions. --- fluid_build/contract_tests.py | 7 ++----- fluid_build/providers/_duckdb_compat.py | 10 ++++++++++ fluid_build/providers/local/local.py | 9 ++------- tests/providers/test_duckdb_compat.py | 17 ++++++++++++++++- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/fluid_build/contract_tests.py b/fluid_build/contract_tests.py index e69536cb..cd921183 100644 --- a/fluid_build/contract_tests.py +++ b/fluid_build/contract_tests.py @@ -53,7 +53,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple, Union -from fluid_build.providers._duckdb_compat import first_row, query_relation +from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, first_row, query_relation from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, DuckDBSandboxError, @@ -222,10 +222,7 @@ def _execute_sql(con, sql: str): ) from e raise LocalProviderError("SQL execution failed: " + msg) from e if rel is None: - raise LocalProviderError( - "The action's SQL ends in a statement that returns no rows (CREATE, INSERT, " - "UPDATE, ...), so it has no result to write. End it with a SELECT of the result." - ) + raise LocalProviderError(NO_ROWS_TO_WRITE) return rel diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index 1cf0ea41..af680293 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -74,6 +74,15 @@ def first_row(result: Any) -> Optional[Tuple[Any, ...]]: _NO_RESULT_SHAPES = frozenset({(("Count",), ("BIGINT",)), (("Success",), ("BOOLEAN",))}) +#: What the engine says when SQL meant to produce a result ends in a statement +#: that returns no rows (``query_relation`` answered ``None``). +NO_ROWS_TO_WRITE = ( + "The SQL ends in a statement that returns no rows (CREATE, INSERT, UPDATE, ...), so " + "there is no result to write. End it with a SELECT of the result, for example: " + "CREATE TABLE t AS SELECT ...; SELECT * FROM t" +) + + def query_relation(con: Any, sql: str) -> Any: """``con.sql(sql)``, or ``None`` when the SQL's last statement returns no rows. @@ -115,6 +124,7 @@ def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: __all__ = [ + "NO_ROWS_TO_WRITE", "STORAGE_COMPATIBILITY_VERSION", "first_row", "query_relation", diff --git a/fluid_build/providers/local/local.py b/fluid_build/providers/local/local.py index 995a74c3..3612f5d1 100644 --- a/fluid_build/providers/local/local.py +++ b/fluid_build/providers/local/local.py @@ -39,7 +39,7 @@ from typing import Any, Dict, List, Optional, Tuple, Union from fluid_build.observability.secret_redactor import redact_secret_text, redact_value -from fluid_build.providers._duckdb_compat import first_row, query_relation +from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, first_row, query_relation from fluid_build.providers._duckdb_read import build_register_view_sql from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -961,12 +961,7 @@ def _execute_sql(): if rel is None: # The same on DuckDB 1.5 and 2.x (see query_relation); 1.5 used to # fail below on ``None.write_csv``. - raise ValueError( - f"SQL action {idx} ends in a statement that returns no rows (CREATE, " - "INSERT, UPDATE, ...), so it has no result to write. End the SQL with a " - "SELECT of the result; to keep it as a table for later actions, also set " - "output_table." - ) + raise ValueError(NO_ROWS_TO_WRITE) # If an output_table is specified, persist the result as a DuckDB table # so downstream materialize/copy steps can reference it. diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 442fa223..32f48ed3 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -229,10 +229,25 @@ def test_local_sql_that_returns_no_rows_fails_the_same_on_both_versions(product) ) assert results[1]["status"] == "error" assert "returns no rows" in results[1]["error"] - assert "End the SQL with a SELECT" in results[1]["error"] + assert "End it with a SELECT of the result" in results[1]["error"] assert not out.exists() +def test_the_fix_the_no_rows_error_suggests_works(product): + """The error's own example (CREATE, then SELECT what it made) lands the rows.""" + out = product / "out" / "made.csv" + results = _local_apply( + product, + { + "op": "sql", + "sql": "CREATE TABLE made AS SELECT * FROM orders; SELECT * FROM made", + "outputs": [str(out)], + }, + ) + assert results[1]["status"] == "ok", results + assert out.read_text(encoding="utf-8").splitlines() == ["id,amount", "1,10", "2,20"] + + def test_contract_tests_sql_that_returns_no_rows_fails_clearly(): from fluid_build.contract_tests import LocalProviderError, _execute_sql From 850dc38ab4b772656c51d5bd32cafd6280f33b1b Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:59:17 +0200 Subject: [PATCH 06/14] ci: a DuckDB-floor job runs the version-sensitive tests on duckdb 1.5.0 pyproject.toml takes duckdb>=1.5.0 with no ceiling, and DuckDB 2.0 needs Python 3.11, so 1.5.x ran only on the Python 3.10 test leg, which resolves the newest 1.5 patch (1.5.6), never the declared floor; and Python 3.10 reached end of life in October 2026. The job installs exactly the floor the sandbox enforces (MIN_DUCKDB_VERSION, read from the code, so the two cannot drift) on Python 3.12, asserts it is the version that runs, and runs the compat seam, sandbox, MCP DuckDB driver and local end-to-end tests. --- .github/workflows/ci.yml | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6ab6d257..21fb8c8e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -623,6 +623,42 @@ jobs: if: always() run: python scripts/cleanup_local_test_artifacts.py + # ────────────────────────────────────────────────────────────── + # DuckDB floor. pyproject.toml takes duckdb>=1.5.0 with no ceiling, and + # DuckDB 2.0 needs Python 3.11, so the only other job on 1.5.x is the + # Python 3.10 test leg, which gets the newest 1.5 patch, never the floor. + # This installs exactly the floor the sandbox enforces (MIN_DUCKDB_VERSION, + # read from the code, so the two cannot drift) on a supported Python and + # runs what differs between DuckDB versions: the compat seam + # (providers/_duckdb_compat.py), the sandbox, the MCP DuckDB driver and the + # local end-to-end suite. + # ────────────────────────────────────────────────────────────── + duckdb-floor: + name: DuckDB floor (Python 3.12) + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + cache: pip + - name: Install with local extras, then the DuckDB floor + run: | + python -m pip install --upgrade pip + python -m pip install -e ".[dev,local]" + floor=$(python -c "from fluid_build.providers._duckdb_sandbox import MIN_DUCKDB_VERSION as v; print('.'.join(map(str, v)))") + python -m pip install "duckdb==${floor}" + python -c "import duckdb, sys; print('duckdb', duckdb.__version__); sys.exit(duckdb.__version__ != '${floor}')" + - name: Run the DuckDB-version-sensitive tests on the floor + env: + FLUID_REQUIRE_DUCKDB: "1" + run: | + python -m pytest -q tests/providers/test_duckdb_compat.py tests/providers/test_duckdb_sandbox.py tests/providers/test_duckdb_sandbox_workspace_root.py tests/output_ports/test_drivers_duckdb.py tests/test_e2e_local.py tests/test_e2e_local_apply.py tests/test_e2e_local_negative.py + - name: Cleanup local artifacts (defensive) + if: always() + run: python scripts/cleanup_local_test_artifacts.py + # ────────────────────────────────────────────────────────────── # dbt v2 canary. The `test` and `coverage-and-audit` jobs install # `dbt-core<2` + dbt-duckdb, so they exercise the 1.x line only. Every From bd71a015c8b9b6f2c2a73f1eb1a7be54c3522ab9 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:59:46 +0200 Subject: [PATCH 07/14] fix(duckdb): a Postgres source is attached read-only, so it runs on DuckDB 2.0 DuckDB 2.0's postgres extension dropped postgres_scan(dsn, schema, table), so a DuckDB acquisition build reading Postgres failed on every Python 3.11+ install ("Table Function with name postgres_scan does not exist!"); main fails the same. Found by the documentation dimension of the world-class inspection, then reproduced live against a local Postgres 16 container. The runner now attaches the source before the sandbox's lock, as it attaches MySQL and SQLite sources, and reads ..: - ATTACH '' AS postgres_ (TYPE postgres, SECRET postgres_, READ_ONLY). READ_ONLY: postgres_execute and INSERT on the alias are refused (probed on 1.5.6 and 2.0.0), so nothing on the connection can write to the source. - An empty CREATE TEMPORARY SECRET named in the ATTACH: without one, the extension looks for a default secret in every storage, and the persistent storage the sandbox turns off answers "Unknown secret storage found: 'local_file'" on 1.5.0, 1.5.6 and 2.0.0 alike. The DSN carries the credentials and stays authoritative (a wrong password is refused). - connection.secretRef is resolved into the password, as the MySQL branch already did; the shared DSN builder still refuses control characters and a non-numeric port. - A connect error is raised as _SourceAttachError with the password masked (one helper for the postgres and mysql attaches), and _execute turns it into a FAILED run with a run record, so `fluid apply` reports "duckdb build ... failed: source: ..." instead of "Unexpected error during execution". Live proof, examples/source-aligned-postgres-duckdb through the fluid CLI against postgres:16-alpine: validate, apply --mode amend-and-build (5 rows) and verify --strict pass on duckdb 2.0.0, 1.5.6 and 1.5.0; on main 2.0.0 the apply fails as above. TestDuckdbPostgresLive passes on 3.10/1.5.6, 3.12/1.5.0, 3.12/1.5.6, 3.12/2.0.0 and 3.14/2.0.0. A wrong password fails the build with the password masked in the output, logs and run record. A new offline test runs the runner against an unreachable server and asserts a connection failure (it fails on 2.0 with the old postgres_scan read). The example's README now uses `fluid apply --mode amend-and-build --build-id`, which replaced `--build`. Reference: https://duckdb.org/docs/2.0/core_extensions/postgres/overview.html (ATTACH, postgres_query and postgres_execute; no postgres_scan). --- .../source-aligned-postgres-duckdb/README.md | 8 +- .../contract.fluid.yaml | 2 +- fluid_build/build_runners/duckdb/runner.py | 187 ++++++++++++------ fluid_build/cli/discover/__init__.py | 6 +- fluid_build/providers/_sql_safety.py | 4 +- tests/build_runners/test_duckdb_runner.py | 69 ++++++- .../test_duckdb_runner_mysql_attach.py | 63 +++++- tests/cli/discover/test_jdbc_attach_safety.py | 4 +- tests/security/test_security_regressions.py | 26 +-- 9 files changed, 273 insertions(+), 96 deletions(-) diff --git a/examples/source-aligned-postgres-duckdb/README.md b/examples/source-aligned-postgres-duckdb/README.md index 189d7448..40f8dffc 100644 --- a/examples/source-aligned-postgres-duckdb/README.md +++ b/examples/source-aligned-postgres-duckdb/README.md @@ -1,8 +1,8 @@ # Source-aligned Postgres → DuckDB → Parquet A minimal source-aligned data product (`Bronze` / `SDP`) that acquires -`public.orders` from a Postgres source via DuckDB's `postgres_scan` and -lands it as Parquet. +`public.orders` from a Postgres source through DuckDB's postgres extension +(the source is attached read-only) and lands it as Parquet. **Vocabulary note.** v0.7.3 carries two interchangeable classifications: the medallion `metadata.layer` (`Bronze`/`Silver`/`Gold`) and the Data @@ -34,7 +34,7 @@ The pipeline: ``` fluid validate contract.fluid.yaml -fluid apply --build ingest_orders contract.fluid.yaml +fluid apply contract.fluid.yaml --mode amend-and-build --build-id ingest_orders ``` `apply` detects `pattern: acquisition` + `engine: duckdb`, dispatches to @@ -45,7 +45,7 @@ table to `out/orders.parquet`, and persists a run record under ## What this exercises - v0.7.3 `acquisition` build pattern + `duckdb` engine -- Postgres source via `postgres_scan` (no Airbyte server needed) +- Postgres source attached by DuckDB's postgres extension (no Airbyte server needed) - Parquet sink via `COPY (...) TO ... (FORMAT 'parquet')` - Run-record persistence + lock acquisition - Pre-land hook chain (`dlp_scan`, `quality_gate`) — non-blocking on this diff --git a/examples/source-aligned-postgres-duckdb/contract.fluid.yaml b/examples/source-aligned-postgres-duckdb/contract.fluid.yaml index a848c18b..0ac03e33 100644 --- a/examples/source-aligned-postgres-duckdb/contract.fluid.yaml +++ b/examples/source-aligned-postgres-duckdb/contract.fluid.yaml @@ -4,7 +4,7 @@ id: bronze.crm_orders name: CRM Orders Bronze description: | Source-aligned Bronze data product. Acquires raw orders from a Postgres - source via DuckDB's postgres_scan and lands them as Parquet for downstream + source through DuckDB's postgres extension and lands them as Parquet for downstream Silver/Gold consumption. domain: sales diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 56b26c80..0faece5d 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -21,9 +21,9 @@ Reader dispatch by ``source.kind``: - ``filesystem`` -> ``read_csv`` / ``read_parquet`` / ``read_json`` - - ``postgres`` -> ``postgres_scan(...)`` - - ``mysql`` -> ``mysql_scan(...)`` - - ``sqlite`` -> ``sqlite_scan(...)`` + - ``postgres`` -> ``ATTACH ... (TYPE postgres, READ_ONLY)``, then ``..
`` + - ``mysql`` -> ``ATTACH ... (TYPE mysql)``, then ``..
`` + - ``sqlite`` -> ``ATTACH ... (TYPE sqlite)``, then ``.
`` - ``http`` -> ``read_csv_auto('https://...')`` Loads required extensions on demand (``httpfs``, ``postgres``, ``mysql``, @@ -397,29 +397,51 @@ def _build_select_for_filesystem(uri: str, fmt: str, opts: Dict[str, Any]) -> st raise ValueError(f"unsupported reader.format for filesystem: {fmt}") -def _build_select_for_postgres(connection: Dict[str, Any], stream: str) -> str: - """``postgres_scan('host=… port=… dbname=… user=… password=…', schema, table)``. +def _postgres_attach_sql(connection: Dict[str, Any], *, alias: str) -> str: + """``ATTACH '' AS (TYPE postgres, SECRET , READ_ONLY)``. - Schema/table are validated as identifiers (so ``stream='x; DROP TABLE y'`` - is rejected at build time, not executed). DSN values are passed through - ``quote_ansi_string_literal`` so embedded single quotes can't escape the - outer SQL string. The DSN itself uses libpq's ``key=value`` whitespace - format; the entire DSN is a single SQL literal at the boundary. + DuckDB 1.5's ``postgres_scan(dsn, schema, table)`` is gone from DuckDB 2.0's + postgres extension; ``ATTACH`` works on both, and attached ``READ_ONLY`` the + source cannot be written through it (``postgres_execute`` and ``INSERT`` are + refused). The DSN is the shared :func:`build_libpq_dsn` one (control + characters and a non-numeric port refused), one SQL literal at the boundary. + ``SECRET `` names the empty secret :func:`_postgres_secret_sql` makes. + """ + alias = validate_ident(alias) + dsn = build_libpq_dsn(connection, database_key="dbname") + return ( + f"ATTACH {quote_ansi_string_literal(dsn)} AS {alias} " + f"(TYPE postgres, SECRET {alias}, READ_ONLY)" + ) + + +def _postgres_secret_sql(*, alias: str) -> str: + """``CREATE TEMPORARY SECRET (TYPE postgres)``: empty, the DSN carries the credentials. + + Without a secret named in the ``ATTACH`` the postgres extension looks for a + default one in every secret storage, and the persistent storage the sandbox + turns off answers ``Unknown secret storage found: 'local_file'`` (DuckDB 1.5 + and 2.0 alike). A temporary secret lives in memory, on this connection only. + """ + return f"CREATE TEMPORARY SECRET {validate_ident(alias)} (TYPE postgres)" - Delegates DSN construction to the shared :func:`build_libpq_dsn` so - postgres and mysql don't drift over time. + +def _build_select_for_postgres(stream: str, *, alias: str) -> str: + """``SELECT * FROM ..
`` over the ``ATTACH``-ed postgres source. + + ``DuckdbRunner._attach_external_databases`` attaches it first + (:func:`_postgres_attach_sql`). Schema and table are validated as + identifiers, so ``stream='x; DROP TABLE y'`` is rejected at build time; a + bare ``"table"`` is read from ``public``. """ if "." in stream: schema, table = stream.split(".", 1) else: schema, table = "public", stream + alias = validate_ident(alias) schema = validate_ident(schema) table = validate_ident(table) - dsn = build_libpq_dsn(connection, database_key="dbname") - return ( - f"SELECT * FROM postgres_scan({quote_ansi_string_literal(dsn)}, " - f"{quote_ansi_string_literal(schema)}, {quote_ansi_string_literal(table)})" - ) + return f"SELECT * FROM {alias}.{schema}.{table}" def _build_select_for_mysql(connection: Dict[str, Any], stream: str, *, alias: str) -> str: @@ -487,6 +509,13 @@ def _mysql_alias_for_build(build_id: str) -> str: return validate_ident(candidate) +def _postgres_alias_for_build(build_id: str) -> str: + """Per-build ATTACH alias for postgres — same pattern as mysql.""" + safe = re.sub(r"[^A-Za-z0-9_]", "_", build_id) or "default" + candidate = f"postgres_{safe}" + return validate_ident(candidate) + + def _sqlite_alias_for_build(build_id: str) -> str: """Per-build ATTACH alias for sqlite — same pattern as mysql.""" safe = re.sub(r"[^A-Za-z0-9_]", "_", build_id) or "default" @@ -607,9 +636,9 @@ def _warn_workspace_not_granted(message: str) -> None: def _grant_source(allow: DuckDBAllowlist, ctx: RunContext, streams: List[str]) -> DuckDBAllowlist: """``allow`` plus where a filesystem / http source reads from. - A database source needs no grant: postgres is scanned through its DSN, and - mysql / sqlite are attached before the lock (``_attach_external_databases``), - a sqlite file only inside :func:`_declared_roots` (``confine_declared``). + A database source needs no grant: postgres / mysql / sqlite are attached + before the lock (``_attach_external_databases``), a sqlite file only inside + :func:`_declared_roots` (``confine_declared``). A local source is granted only inside :func:`_declared_roots` (a glob as the directory above its first wildcard): the contract names it, so naming ``~/.ssh/*`` must not make it readable. @@ -751,31 +780,38 @@ def _load_extensions(self, con: Any, ctx: RunContext) -> None: self._attach_external_databases(con, ctx) def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: - """ATTACH any source kind that requires a named database alias. + """ATTACH the source database a postgres / mysql / sqlite build reads. - Postgres uses ``postgres_scan(dsn, schema, table)`` directly with - the DSN inline, so no ATTACH is needed. The mysql / mariadb / - sqlite extensions only expose ATTACH-then-reference semantics — - there's no ``mysql_scan(dsn, …)`` function — so we ATTACH here - at extension-load time and ``_build_select_for_`` helpers - reference ``..
`` (or ``.
`` - for sqlite). + These extensions read through ATTACH-then-reference: the attach runs + here, at extension-load time (before the sandbox's lock, after which an + ATTACH is refused), and ``_build_select_for_`` references + ``..
`` (or ``.
`` for sqlite). + Postgres used ``postgres_scan(dsn, schema, table)`` until DuckDB 2.0 + dropped it; it is attached ``READ_ONLY``. - The alias is per-build (``mysql_`` / ``sqlite_``) - so two sources in the same contract / interpreter don't collide - on a hardcoded name. + The alias is per-build (``postgres_`` / ``mysql_`` + / ``sqlite_``) so two sources in the same contract / + interpreter don't collide on a hardcoded name. Best-effort: if the connection DSN is malformed or the upstream is unreachable, the ATTACH raises and the runner surfaces the - error in the per-stream try/except. + error in the per-stream try/except, with the password masked. A sqlite file is confined to :func:`_declared_roots` plus the operator's ``FLUID_DUCKDB_ALLOWED_DIRS`` first, and refused with - ``DuckDBSandboxError`` outside them: the sqlite scanner is not bounded - by the sandbox's allowlist, so nothing else would stop it. + ``DuckDBSandboxError`` outside them: nothing else bounds an ATTACH + made before the lock. """ kind = ctx.source.kind - if kind in ("mysql", "mariadb"): + if kind == "postgres": + # Resolve secretRef → password, as the mysql branch does; an inline + # literal still wins. + conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) + alias = _postgres_alias_for_build(ctx.build_id) + attach_sql = _postgres_attach_sql(conn, alias=alias) # validates first + con.execute(_postgres_secret_sql(alias=alias)) + _attach_with_password_masked(con, attach_sql, conn.get("password")) + elif kind in ("mysql", "mariadb"): # Resolve secretRef → password before build_mysql_dsn reads # connection.get("password"). Inline literal values still win. conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) @@ -785,22 +821,13 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: # and refuses a '?', a '/' in the host and a control character. dsn = build_mysql_dsn(conn, database_key="database") alias = _mysql_alias_for_build(ctx.build_id) - password = conn.get("password") - masked: Optional[str] = None - try: - # mysql extension supports both mysql:// and mariadb:// upstreams; - # the duckdb extension type literal is always ``mysql``. - con.execute(f"ATTACH {quote_ansi_string_literal(dsn)} AS {alias} (TYPE mysql)") - except Exception as exc: - if not password: - raise - # mysql_scanner quotes the whole DSN in a connect error, and - # ``fluid apply`` prints the error: keep it, without the password. - masked = mask_dsn_password(str(exc), str(password)) - if masked is not None: - # Raised after the handler has ended, so the unmasked error is - # neither the cause nor the context of this one (PEP 415). - raise RuntimeError(masked) + # mysql extension supports both mysql:// and mariadb:// upstreams; + # the duckdb extension type literal is always ``mysql``. + _attach_with_password_masked( + con, + f"ATTACH {quote_ansi_string_literal(dsn)} AS {alias} (TYPE mysql)", + conn.get("password"), + ) elif kind == "sqlite": conn = dict(ctx.source.connection.raw) # SQLite uses a path, not a libpq-style DSN. The path comes @@ -809,16 +836,42 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: path = conn.get("uri") or conn.get("path") or conn.get("database") or "" if not path: raise ValueError("sqlite source requires connection.uri, .path, or .database") - # The sqlite scanner opens the file through its own client library, - # which allowed_directories never bounds: this confinement is the - # only check on it. Without it a contract could land any SQLite - # file on the host (a browser's cookie store, another app's data). - # The checked realpath is what is attached. + # The attach runs before the sandbox's lock, when nothing bounds the + # open (and through DuckDB 1.5 the sqlite scanner's own client + # library is never bounded): this confinement is the only check on + # it. Without it a contract could land any SQLite file on the host + # (a browser's cookie store, another app's data). The checked + # realpath is what is attached. real = confine_declared(str(path), within=_declared_roots(ctx)) alias = _sqlite_alias_for_build(ctx.build_id) con.execute(f"ATTACH {quote_ansi_string_literal(real)} AS {alias} (TYPE sqlite)") +class _SourceAttachError(RuntimeError): + """The source database could not be attached; the message holds no password.""" + + +def _attach_with_password_masked(con: Any, statement: str, password: Any) -> None: + """Run an ``ATTACH`` that carries a DSN; its connect error loses the password. + + The postgres and mysql scanners quote the whole DSN they were given in a + connect error, and ``fluid apply`` prints the error: keep it, without the + password (:func:`mask_dsn_password`), as a :class:`_SourceAttachError` the + run reports as the build's failure. + """ + masked: Optional[str] = None + try: + con.execute(statement) + except Exception as exc: + if not password: + raise _SourceAttachError(str(exc)) from exc + masked = mask_dsn_password(str(exc), str(password)) + if masked is not None: + # Raised after the handler has ended, so the unmasked error is + # neither the cause nor the context of this one (PEP 415). + raise _SourceAttachError(masked) + + def _select_for_first_stream(ctx: RunContext) -> str: return _select_for_stream(ctx, ctx.source.streams[0] if ctx.source.streams else "data") @@ -837,7 +890,7 @@ def _select_for_stream(ctx: RunContext, stream: str) -> str: opts = dict(ctx.source.reader.options) if ctx.source.reader else {} return _build_select_for_http(uri, fmt or "csv", opts) if kind == "postgres": - return _build_select_for_postgres(conn, stream) + return _build_select_for_postgres(stream, alias=_postgres_alias_for_build(ctx.build_id)) if kind in ("mysql", "mariadb"): # MariaDB uses the same duckdb mysql extension; the discoverer # already accepts both schemes (cli/discover/mysql.py), keep @@ -1360,11 +1413,17 @@ def _execute(ctx: RunContext, runner: DuckdbRunner) -> RunResult: dlq_writer: Optional[DLQWriter] = None dlq_total_records = 0 - con = secure_duckdb_connect( - ":memory:", - allow=_run_allowlist(ctx, streams_to_run, sink_format), - before_lock=lambda c: runner._load_extensions(c, ctx), - ) + try: + con = secure_duckdb_connect( + ":memory:", + allow=_run_allowlist(ctx, streams_to_run, sink_format), + before_lock=lambda c: runner._load_extensions(c, ctx), + ) + except _SourceAttachError as exc: + # The source database is attached as the connection opens. Its connect + # error fails the run as a stream's error does, so the run record and + # the build's summary carry it (without the password). + return _refused_run(ctx, streams_to_run, started_at, t_start, f"source: {exc}") try: if masker is not None: masker.install(con) @@ -1490,8 +1549,8 @@ def _execute(ctx: RunContext, runner: DuckdbRunner) -> RunResult: on_error, ) except Exception as exc: # noqa: BLE001 - # The COPY SQL embeds DSN strings (postgres_scan dsn=…), - # which DuckDB echoes in its error messages. Don't pass + # DuckDB echoes the statement, and a source database's own + # error, in its error messages. Don't pass # `exc` directly and don't dump the full traceback via # exc_info — both can carry password/key data. LOG.error( diff --git a/fluid_build/cli/discover/__init__.py b/fluid_build/cli/discover/__init__.py index ccd7e426..1f557c93 100644 --- a/fluid_build/cli/discover/__init__.py +++ b/fluid_build/cli/discover/__init__.py @@ -17,9 +17,9 @@ Introspects a live source and emits a deterministic Bronze contract for every discoverable stream. Supported source kinds: - - ``postgres://...`` — full schema/table introspection via DuckDB postgres_scan - - ``mysql://...`` — same via mysql_scan - - ``sqlite:///...`` — via sqlite_scan + - ``postgres://...`` — full schema/table introspection via DuckDB's postgres extension (ATTACH) + - ``mysql://...`` — same via the mysql extension + - ``sqlite:///...`` — via the sqlite extension - ``s3://`` / ``file://`` / ``https://`` — filesystem (CSV / Parquet / JSONL) The output is a list of ``contract..fluid.yaml`` files plus an optional diff --git a/fluid_build/providers/_sql_safety.py b/fluid_build/providers/_sql_safety.py index 7c14fe8d..6722aac8 100644 --- a/fluid_build/providers/_sql_safety.py +++ b/fluid_build/providers/_sql_safety.py @@ -225,8 +225,8 @@ def libpq_escape(value: Any) -> str: Used by: - * The duckdb acquisition runner when building ``postgres_scan(...)`` - DSNs. + * The duckdb acquisition runner when building its postgres ``ATTACH`` + DSN. * The JDBC attach builder (``cli/discover/_jdbc_introspect.py``) for postgres. """ diff --git a/tests/build_runners/test_duckdb_runner.py b/tests/build_runners/test_duckdb_runner.py index d54de613..e0caf939 100644 --- a/tests/build_runners/test_duckdb_runner.py +++ b/tests/build_runners/test_duckdb_runner.py @@ -209,6 +209,73 @@ def test_missing_uri_fails(self, tmp_path): assert rc != 0 +# ── postgres, no server ─────────────────────────────────────────────────── + + +def test_an_unreachable_postgres_source_fails_the_build_not_the_process(tmp_path, caplog): + """The source is attached as the run's connection opens (DuckDB 2.0 has no + ``postgres_scan``), so its connect error is reported as the build's failure, + with the password masked, as a failing stream was before.""" + duckdb = pytest.importorskip("duckdb") + try: + probe = duckdb.connect() + probe.execute("INSTALL postgres; LOAD postgres") + probe.close() + except duckdb.Error: + pytest.skip("the postgres extension is not installable here (offline)") + password = "runner-unreachable-pg-pw-6T" # pragma: allowlist secret + out_path = tmp_path / "orders.parquet" + contract = { + "fluidVersion": "0.7.3", + "id": "bronze.pg_orders", + "builds": [ + { + "id": "ingest_pg", + "pattern": "acquisition", + "engine": "duckdb", + "properties": { + "source": { + "kind": "postgres", + "connection": { + "host": "127.0.0.1", + "port": 9, # discard: nothing listens + "user": "fluid", + "password": password, + "database": "app", + }, + "mode": "full_refresh", + "streams": ["public.orders"], + }, + "sink": {"format": "parquet"}, + }, + "outputs": ["orders"], + } + ], + "exposes": [ + { + "exposeId": "orders", + "kind": "table", + "binding": { + "platform": "local", + "format": "parquet", + "location": {"path": str(out_path)}, + }, + "contract": {"schema": []}, + } + ], + } + caplog.set_level("DEBUG") + rc = execute_duckdb_build(contract["builds"][0], contract, tmp_path, dry_run=False) + assert rc == 1 + assert not out_path.exists() + # A connection failure, the source reached: not a function DuckDB 2.0 lacks. + assert "Unable to connect to Postgres" in caplog.text + assert password not in caplog.text + for written in tmp_path.rglob("*"): + if written.is_file(): + assert password not in written.read_text(encoding="utf-8", errors="replace"), written + + # ── postgres (docker-required) ──────────────────────────────────────────── @@ -218,7 +285,7 @@ def test_missing_uri_fails(self, tmp_path): reason="Set FLUID_TEST_POSTGRES_HOST/PORT/USER/PASSWORD/DB to run; CI uses docker-compose.", ) class TestDuckdbPostgresLive: - def test_postgres_scan_round_trip(self, tmp_path): + def test_postgres_round_trip(self, tmp_path): host = os.environ["FLUID_TEST_POSTGRES_HOST"] port = int(os.environ.get("FLUID_TEST_POSTGRES_PORT", "5432")) user = os.environ.get("FLUID_TEST_POSTGRES_USER", "postgres") diff --git a/tests/build_runners/test_duckdb_runner_mysql_attach.py b/tests/build_runners/test_duckdb_runner_mysql_attach.py index cf679f3c..194800cb 100644 --- a/tests/build_runners/test_duckdb_runner_mysql_attach.py +++ b/tests/build_runners/test_duckdb_runner_mysql_attach.py @@ -187,22 +187,69 @@ def test_other_non_ascii_text_is_accepted(self, value): assert dict(_parse_mysql_dsn(_dsn_of(sql)))["password"] == value -class TestPostgresScanDsnRefusesControlCharacters: - """The runner's postgres source is read through ``postgres_scan(dsn, ...)``, - whose DSN the same shared builder makes. libpq ends a bare value at any C - ``isspace()`` character, and libpq_escape quotes only a value with a space - or a quote, so a tab or newline there started a second keyword.""" +class TestPostgresAttachDsnRefusesControlCharacters: + """The runner's postgres source is attached with ``ATTACH '' ... (TYPE + postgres)``, whose DSN the same shared builder makes. libpq ends a bare + value at any C ``isspace()`` character, and libpq_escape quotes only a value + with a space or a quote, so a tab or newline there started a second keyword.""" @pytest.mark.parametrize("field", ["host", "user", "password", "database"]) @pytest.mark.parametrize("ch", _CONTROLS, ids=lambda c: f"U+{ord(c):04X}") def test_refused(self, field, ch): - from fluid_build.build_runners.duckdb.runner import _build_select_for_postgres - conn = {"host": "db.test", "port": "5432", "user": "u", "password": FAKE_PW} conn["database"] = "app" conn[field] = f"ab{ch}sslmode=disable" with pytest.raises(ValueError, match=f"{field}.*control characters"): - _build_select_for_postgres(conn, "public.orders") + _attach(conn, kind="postgres") + + +class TestPostgresAttach: + """DuckDB 2.0's postgres extension has no ``postgres_scan``, so the runner + attaches a postgres source before the sandbox's lock, read-only, as it + attaches mysql and sqlite.""" + + def test_the_source_is_attached_read_only_under_a_per_build_alias(self): + secret, sql = _attach(dict(_BASE, port="5432"), kind="postgres") + # An empty temporary secret: the sandbox's disabled persistent storage + # otherwise fails the ATTACH's default-secret lookup. + assert secret == "CREATE TEMPORARY SECRET postgres_b1 (TYPE postgres)" + assert sql.startswith("ATTACH '") + assert sql.endswith(" AS postgres_b1 (TYPE postgres, SECRET postgres_b1, READ_ONLY)") + assert "dbname=app" in _dsn_of(sql) + + def test_the_select_reads_the_attached_alias(self): + from fluid_build.build_runners.duckdb.runner import _build_select_for_postgres + + assert ( + _build_select_for_postgres("sales.orders", alias="postgres_b1") + == "SELECT * FROM postgres_b1.sales.orders" + ) + assert ( + _build_select_for_postgres("orders", alias="postgres_b1") + == "SELECT * FROM postgres_b1.public.orders" + ) + + def test_a_secret_ref_password_is_resolved(self, monkeypatch): + monkeypatch.setenv("RUNNER_FAKE_PG_SECRET", "runner-pg-ref-pw-3J") + raw = {k: v for k, v in _BASE.items() if k != "password"} + raw["secretRef"] = "env://RUNNER_FAKE_PG_SECRET" # pragma: allowlist secret + _secret, sql = _attach(raw, kind="postgres") + assert "password=runner-pg-ref-pw-3J" in _dsn_of(sql) + + def test_a_connect_error_has_the_password_masked(self): + class _Con: + def execute(self, sql: str, *_a: Any, **_k: Any) -> None: + if sql.startswith("ATTACH"): + # What postgres_scanner says: it quotes the DSN it was given. + raise RuntimeError( + f'IO Error: Unable to connect to Postgres at "{_dsn_of(sql)}"' + ) + + with pytest.raises(RuntimeError) as caught: + _attach_on(_Con(), dict(_BASE, port="5432"), kind="postgres") + assert FAKE_PW not in str(caught.value) + assert "password=***" in str(caught.value) + assert caught.value.__context__ is None and caught.value.__cause__ is None class TestConnectErrorHasThePasswordMasked: diff --git a/tests/cli/discover/test_jdbc_attach_safety.py b/tests/cli/discover/test_jdbc_attach_safety.py index f799b418..8d150c7a 100644 --- a/tests/cli/discover/test_jdbc_attach_safety.py +++ b/tests/cli/discover/test_jdbc_attach_safety.py @@ -137,7 +137,7 @@ def test_other_non_ascii_text_is_accepted(self, value, postgres): class TestSharedBuildersRejectControlChars: """The check lives in the shared DSN builders, so every caller gets it: the - JDBC attach, and the duckdb build runner's mysql ATTACH and postgres_scan DSN, + JDBC attach, and the duckdb build runner's mysql and postgres ATTACH DSNs, which call the builders directly.""" @pytest.mark.parametrize("ch", ["\x00", "\t", "\n", "\x1b", "\x7f", "\x85"], ids=repr) @@ -182,7 +182,7 @@ def test_build_attach_dsn_refuses_a_non_ascii_digit_port(self, name): @pytest.mark.parametrize("name", sorted(_PORTS)) def test_build_libpq_dsn_refuses_a_non_ascii_digit_port(self, name): - # The shared builder: the duckdb runner's postgres_scan DSN uses it too. + # The shared builder: the duckdb runner's postgres ATTACH DSN uses it too. from fluid_build.providers._sql_safety import build_libpq_dsn with pytest.raises(ValueError, match="port"): diff --git a/tests/security/test_security_regressions.py b/tests/security/test_security_regressions.py index 0165d704..d06e7df7 100644 --- a/tests/security/test_security_regressions.py +++ b/tests/security/test_security_regressions.py @@ -39,6 +39,7 @@ _build_select_for_filesystem, _build_select_for_postgres, _csv_options_clause, + _postgres_attach_sql, ) from fluid_build.build_runners.hooks.tokenize_pii import ( _TOKEN_HEX_LEN, @@ -78,15 +79,18 @@ def test_filesystem_options_key_must_be_identifier(self): def test_postgres_stream_must_be_identifier(self): with pytest.raises(ValueError): - _build_select_for_postgres( - {"host": "h", "port": 5432, "user": "u", "password": "p", "database": "d"}, - "public.orders; DROP TABLE secrets; --", - ) + _build_select_for_postgres("public.orders; DROP TABLE secrets; --", alias="pg") + + def test_postgres_alias_must_be_identifier(self): + with pytest.raises(ValueError): + _build_select_for_postgres("orders", alias="pg; DROP TABLE secrets") + with pytest.raises(ValueError): + _postgres_attach_sql({"host": "h"}, alias="pg; DROP TABLE secrets") def test_postgres_password_with_quote_neutralized(self): # Password contains a single quote; the runner must escape it twice - # (libpq + SQL boundary) so the resulting SQL is still well-formed. - sql = _build_select_for_postgres( + # (libpq + SQL boundary) so the ATTACH is still one well-formed statement. + sql = _postgres_attach_sql( { "host": "h", "port": 5432, @@ -94,18 +98,18 @@ def test_postgres_password_with_quote_neutralized(self): "password": "p'; DROP TABLE secrets; --", "database": "d", }, - "orders", + alias="pg", ) # Single quote was doubled at the SQL boundary. assert "''" in sql - # Exactly one postgres_scan call is emitted. - assert sql.count("postgres_scan(") == 1 + assert sql.count("ATTACH ") == 1 + assert sql.endswith(" AS pg (TYPE postgres, SECRET pg, READ_ONLY)") def test_postgres_port_must_be_numeric(self): with pytest.raises(ValueError): - _build_select_for_postgres( + _postgres_attach_sql( {"host": "h", "port": "5432; DROP", "user": "u", "password": "p", "database": "d"}, - "orders", + alias="pg", ) def test_copy_destination_path_with_quote_is_quoted(self): From c6530aad4f3be3423b486196ef19731b0b909be4 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 07:00:02 +0200 Subject: [PATCH 08/14] fix(duckdb): what the world-class inspection found in the port - Strict mypy (CI's typecheck-strict job) failed on cli/_verify_masking.py: first_row() is Optional. New one_row() raises on an empty result and is used where an aggregate's row is indexed straight away (verify, verify masking, local provider, contract_tests, late-arrival counts). - query_relation() still let two DuckDB 2.0 shapes through as results: a CREATE TABLE ... AS PIVOT (typed MULTI, it returns a Success flag) and SQL ending in DML after a query (2.0's con.sql() returns the earlier query's rows). On 2.x every statement but the last now runs first and the last one alone decides; MULTI is judged by its shape. On 1.5 it is con.sql itself, the reference. Parametrized cases for DML, ALTER, SET, DROP and BEGIN...COMMIT endings, and the local apply refuses all three shapes. - The no-rows message said "CREATE TABLE t AS ...; SELECT * FROM t", which fails silently when the planner also sets output_table (its CTAS wrap cannot hold several statements); it now says to write just the SELECT. - The storage pin is v1.0.0, the format DuckDB 1.5 itself writes (storage version 64, which DuckDB 1.0 and later open), not v1.5.0 (version 68, which 1.4 refuses and which accepts 2.0-only types such as VARIANT that 1.5 refuses). A byte-level test pins header version 64 on both DuckDB versions. - Reverted the is_unloaded_function broadening of 9c29f7d1: postgres_scan is gone from DuckDB 2.0's extension, not unadvertised, and mysql_scan never existed, so "does not exist" there is not a refusal. A 2.0 test pins that postgres_scan is absent even with the extension loaded. - The sandbox's scanner bullet is version-qualified with what was replayed on 1.5.0, 1.5.6 and 2.0.0: through 1.5 postgres_scan reaches any host and the sqlite scanner any file; 2.0 has no postgres_scan and bounds sqlite; on both an ATTACH of postgres or mysql after the lock is refused, so postgres_query / mysql_query reach only what the engine attached. The postgres pin runs on 1.5 only, and a new pin covers the refused ATTACH. - The partial-read fence also flags an execute() of a query whose result is thrown away (local_validation's SELECT 1 probe now reads it), and fails if an exempt module starts using DuckDB. The MagicMock dry-run test that mirrored first_row's internals is gone; a real-relation test covers it. - contract_tests' docstring no longer says CREATE TABLE AS SQL is accepted. - CHANGELOG: the legacy module is named fluid_build.contract_tests; the Postgres fix; the conversion recipe (COPY FROM DATABASE on 2.x, verified readable by 1.5.6); files forge-cli 0.21.0 wrote on DuckDB 2.0. --- CHANGELOG.md | 44 +++-- fluid_build/build_runners/_late_arrival.py | 6 +- fluid_build/cli/_verify_masking.py | 4 +- fluid_build/cli/verify.py | 4 +- fluid_build/contract_tests.py | 9 +- fluid_build/providers/_duckdb_compat.py | 100 +++++++---- fluid_build/providers/_duckdb_sandbox.py | 38 ++-- fluid_build/providers/local/local.py | 10 +- fluid_build/providers/local_validation.py | 2 +- tests/providers/test_duckdb_compat.py | 198 ++++++++++++++++----- tests/providers/test_duckdb_sandbox.py | 20 ++- tests/test_contract_tests_branches.py | 9 - 12 files changed, 294 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55bf2068..7225a2dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,11 +12,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - forge-cli runs on DuckDB 2.x as well as 1.5.x (`duckdb>=1.5.0`, no ceiling; DuckDB 2.0 needs Python 3.11, so a Python 3.10 install stays on 1.5.x). On 2.x, a DuckDB database file forge-cli creates (`fluid demo`'s `.fluid/db.duckdb`, the local provider's session - and `persist` databases, a Meltano acquisition's DuckDB destination) is written in - DuckDB 1.5's storage format, which DuckDB 1.5.0 and later open; a file that already - exists keeps its format. DuckDB 2.x's own default format cannot be read by 1.5: a file - another 2.x tool created (dbt-duckdb and dlt included) needs every process that opens - it on 2.x, or an `EXPORT DATABASE` on 2.x and an `IMPORT DATABASE` on 1.5. + and `persist` databases, a Meltano acquisition's DuckDB destination) is written in the + format DuckDB 1.5 writes, which DuckDB 1.0 and later open, so a 2.0-only type such as + `VARIANT` is refused in it as on 1.5; a file that already exists keeps its format. + DuckDB 2.x's own default format cannot be read by 1.5: a file another 2.x tool created + (dbt-duckdb and dlt included), or forge-cli 0.21.0 or older on DuckDB 2.0, opens only + on 2.x. To convert one, run on 2.x: `ATTACH 'old.duckdb' AS src (READ_ONLY); ATTACH + 'new.duckdb' AS dst (STORAGE_VERSION 'v1.0.0'); COPY FROM DATABASE src TO dst;`. - Contract SQL runs on the DuckDB that is installed, and DuckDB 2.0 changed some SQL: the `x -> x + 1` lambda is an error (write `lambda x: x + 1`), and integer `//` and `%` by zero raise an error instead of returning NULL (wrap them in `TRY(...)`). See @@ -67,17 +69,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **DuckDB 2.0.** `fluid verify` reported every local file as `error` ("Relations no - longer have fetchone()"); `fluid apply`'s local SQL and materialize steps and - `contract_tests` actions lost their row counts (a materialize step logged - `local_materialize_fallback`, a `contract_tests` action reported `-1` rows). DuckDB 2.0 - relations have no `fetchone()`, and a query result keeps its connection busy until it - is read to the end, which fails the next step on that connection. Every DuckDB result - is now read to the end, the same way on 1.5.x and 2.x. -- A local SQL action or `contract_tests` action whose SQL ends in a statement that - returns no rows (`CREATE TABLE ... AS`, an `INSERT` without `RETURNING`) now fails with - a message saying to end the SQL with a `SELECT` of the result. DuckDB 1.5 failed there - on `'NoneType' object has no attribute 'write_csv'`, and DuckDB 2.0 wrote a one-row - `Count` file as the action's output and reported success. + longer have fetchone()"); `fluid apply`'s local SQL and materialize steps, and the + actions of the legacy `fluid_build.contract_tests` module, lost their row counts (a + materialize step logged `local_materialize_fallback`, a `contract_tests` action + reported `-1` rows). DuckDB 2.0 relations have no `fetchone()`, and a query result + keeps its connection busy until it is read to the end, which fails the next step on + that connection. Every DuckDB result is now read to the end, the same way on 1.5.x and + 2.x. +- **DuckDB 2.0: a Postgres source.** A DuckDB acquisition build reading Postgres failed + on DuckDB 2.0 (`Table Function with name postgres_scan does not exist!`): the postgres + extension dropped `postgres_scan`. The source is now attached read-only before the + sandbox locks, as MySQL and SQLite sources are, and read as `..
` + on 1.5.x and 2.x; its `connection.secretRef` is resolved into the password, as a MySQL + source's already was. A Postgres or MySQL source the runner cannot connect to now fails + the build with the connect error, its password masked, and writes a run record; it + used to end `fluid apply` with `Unexpected error during execution`. +- A local SQL action, or a legacy `fluid_build.contract_tests` action, whose SQL ends in + a statement that returns no rows (`CREATE TABLE ... AS`, an `INSERT` without + `RETURNING`) now fails with a message saying to write just the `SELECT`. DuckDB 1.5 + failed there on `'NoneType' object has no attribute 'write_csv'`, and DuckDB 2.0 + reported success with a one-row `Count` file, an empty `Success` file (a `CREATE TABLE + ... AS PIVOT`) or the rows of an earlier `SELECT` in the same SQL as the output. ## [0.21.0] — 2026-10-19 diff --git a/fluid_build/build_runners/_late_arrival.py b/fluid_build/build_runners/_late_arrival.py index bdead4ff..151cd347 100644 --- a/fluid_build/build_runners/_late_arrival.py +++ b/fluid_build/build_runners/_late_arrival.py @@ -370,7 +370,7 @@ def split_late_events_in_duckdb( can call this too. """ # ── input validation ──────────────────────────────────────────── - from fluid_build.providers._duckdb_compat import first_row + from fluid_build.providers._duckdb_compat import first_row, one_row from fluid_build.providers._sql_safety import validate_ident validate_ident(source_relation) @@ -402,7 +402,7 @@ def split_late_events_in_duckdb( ) # ── append late rows to side-output ────────────────────────────── - late_count = first_row( + late_count = one_row( con.execute(f"SELECT count(*) FROM {source_relation} WHERE {threshold_expr}") )[0] if late_count > 0: @@ -412,7 +412,7 @@ def split_late_events_in_duckdb( ) con.execute(f"DELETE FROM {source_relation} WHERE {threshold_expr}") - on_time_count = first_row(con.execute(f"SELECT count(*) FROM {source_relation}"))[0] + on_time_count = one_row(con.execute(f"SELECT count(*) FROM {source_relation}"))[0] return {"on_time": int(on_time_count), "late": int(late_count)} diff --git a/fluid_build/cli/_verify_masking.py b/fluid_build/cli/_verify_masking.py index 6f162ec7..702ace4c 100644 --- a/fluid_build/cli/_verify_masking.py +++ b/fluid_build/cli/_verify_masking.py @@ -215,9 +215,9 @@ def local_dimension( ) values: List[int] = [] if selects: - from fluid_build.providers._duckdb_compat import first_row + from fluid_build.providers._duckdb_compat import one_row - row = first_row(con.execute(f"SELECT {', '.join(selects)} FROM ({relation_sql})")) + row = one_row(con.execute(f"SELECT {', '.join(selects)} FROM ({relation_sql})")) values = [int(v or 0) for v in row] return finish(values) diff --git a/fluid_build/cli/verify.py b/fluid_build/cli/verify.py index 7c23d7cb..6c9c2aeb 100644 --- a/fluid_build/cli/verify.py +++ b/fluid_build/cli/verify.py @@ -960,7 +960,7 @@ def _verify_local_file( # Introspect with DuckDB. try: - from fluid_build.providers._duckdb_compat import first_row + from fluid_build.providers._duckdb_compat import one_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, secure_duckdb_connect, @@ -981,7 +981,7 @@ def _verify_local_file( relation_sql = f"SELECT * FROM read_csv_auto({literal})" rel = con.sql(relation_sql) - row_count = first_row(rel.aggregate("count(*)"))[0] + row_count = one_row(rel.aggregate("count(*)"))[0] actual_columns = [col for col in rel.columns] # Did the columns policy.privacy.masking names land treated? Counts # only: a value that fails the check is cleartext and stays out of diff --git a/fluid_build/contract_tests.py b/fluid_build/contract_tests.py index cd921183..00295956 100644 --- a/fluid_build/contract_tests.py +++ b/fluid_build/contract_tests.py @@ -27,7 +27,8 @@ - The key is an alias; referenced as table/view name in SQL. - `path` can be a string (supports globs) or list of strings. - If `name` is set, it overrides the alias for the table/view name. -- sql: the SQL string to execute (must be a SELECT or CREATE TABLE AS ... style) +- sql: the SQL string to execute; it must end in a SELECT of the result + (for CREATE TABLE t AS SELECT ..., write just the SELECT ...) - outputs: dict with one or more outputs. Supported: { "path": "runtime/out/high_value_churn_risk.csv", @@ -53,7 +54,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple, Union -from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, first_row, query_relation +from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, one_row, query_relation from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, DuckDBSandboxError, @@ -246,7 +247,7 @@ def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Opt if dry_run: # Estimate count without writing try: - cnt = first_row(rel.count("*"))[0] + cnt = one_row(rel.count("*"))[0] except Exception as e: LOGGER.warning(f"Failed to count rows in dry run: {e}") cnt = -1 @@ -312,7 +313,7 @@ def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Opt # Count rows try: - cnt = first_row(rel.count("*"))[0] + cnt = one_row(rel.count("*"))[0] except Exception as e: LOGGER.warning(f"Failed to count output rows: {e}") cnt = -1 diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index af680293..dd31347b 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -28,25 +28,30 @@ raises ``ResourceInUseException``. Relations lost ``fetchone()`` and ``fetchmany()`` outright. :func:`first_row` reads to the end on both. * ``con.sql()`` of SQL whose last statement returns no rows (``CREATE TABLE - ... AS``, an ``INSERT`` without ``RETURNING``) returns a relation over the - statement's row count where 1.5 returned ``None``. Written out as a - contract's result, that is a one-row ``Count`` file. :func:`query_relation` + ... AS``, an ``INSERT`` without ``RETURNING``) returns a relation where 1.5 + returned ``None``: over the statement's row count, or, after an earlier + query in the same SQL, over that query's rows. Written out as a contract's + result, that is a one-row ``Count`` file or stale rows. :func:`query_relation` answers ``None`` for it on both. * A database file 2.0 creates uses storage version 2.0, which no 1.5 process can open ("can only read versions between 64 and 68"). :func:`storage_compat_config` - keeps the files forge-cli creates readable by every DuckDB it supports. + has 2.x write the format 1.5 itself writes, so a file forge-cli creates is the + same whichever DuckDB created it. -Nothing here imports duckdb: callers pass what they already hold. +Nothing here imports duckdb at module level: callers pass what they hold, and +:func:`query_relation` reads the installed version when it runs. """ from __future__ import annotations from typing import Any, Dict, Optional, Tuple -#: The storage format of the oldest DuckDB forge-cli supports. Database files -#: forge-cli creates on DuckDB 2.x are written in it, so a 1.5 install (Python -#: 3.10, or a tool pinned below 2) can still open them. -STORAGE_COMPATIBILITY_VERSION = "v1.5.0" +#: The storage format DuckDB 1.5 writes by default (storage version 64, which +#: DuckDB 1.0 and later open). Database files forge-cli creates on DuckDB 2.x +#: are written in it, so a 1.5 install (Python 3.10, or a tool pinned below 2) +#: opens them, and a 2.x connection accepts the same SQL on them as 1.5 does +#: (no 2.0-only types such as VARIANT). +STORAGE_COMPATIBILITY_VERSION = "v1.0.0" def first_row(result: Any) -> Optional[Tuple[Any, ...]]: @@ -60,11 +65,25 @@ def first_row(result: Any) -> Optional[Tuple[Any, ...]]: return rows[0] if rows else None +def one_row(result: Any) -> Tuple[Any, ...]: + """The row of a query that always returns exactly one (an aggregate, ``COUNT(*)``). + + :func:`first_row`, for the callers that index the row straight away: an + aggregate without ``GROUP BY`` returns one row even over no input, so an + empty result is a bug and raises ``ValueError`` rather than passing ``None``. + """ + row = first_row(result) + if row is None: + raise ValueError("an aggregate query returned no row") + return row + + #: Statement types whose result is a query result (rows), whatever they hold. -#: ``MULTI`` is 2.0's name for ``PIVOT``, which 1.5 called ``SELECT``. -_QUERY_STATEMENTS = frozenset({"SELECT", "EXPLAIN", "CALL", "PRAGMA", "MULTI"}) -#: Statement types that never return rows: 1.5 answered ``None`` for SQL ending -#: in one even after a ``SELECT`` (2.0 then returns that ``SELECT``'s rows). +#: Not ``MULTI``, 2.0's type for a ``PIVOT`` with columns it must look up first: +#: a bare ``PIVOT`` returns rows, a ``CREATE TABLE ... AS PIVOT`` does not, so +#: that one is judged by the result's shape. +_QUERY_STATEMENTS = frozenset({"SELECT", "EXPLAIN", "CALL", "PRAGMA"}) +#: Statement types that never return rows. _NO_ROWS_STATEMENTS = frozenset( {"CREATE", "DROP", "ALTER", "SET", "VARIABLE_SET", "ATTACH", "DETACH", "TRANSACTION"} | {"LOAD", "VACUUM", "ANALYZE", "EXPORT", "COPY_DATABASE", "PREPARE"} @@ -78,32 +97,44 @@ def first_row(result: Any) -> Optional[Tuple[Any, ...]]: #: that returns no rows (``query_relation`` answered ``None``). NO_ROWS_TO_WRITE = ( "The SQL ends in a statement that returns no rows (CREATE, INSERT, UPDATE, ...), so " - "there is no result to write. End it with a SELECT of the result, for example: " - "CREATE TABLE t AS SELECT ...; SELECT * FROM t" + "there is no result to write. End it with the SELECT of the result: for " + "CREATE TABLE t AS SELECT ..., write just the SELECT ..." ) def query_relation(con: Any, sql: str) -> Any: """``con.sql(sql)``, or ``None`` when the SQL's last statement returns no rows. - What DuckDB 1.5 answered: a relation for a query (``SELECT``, ``CALL``, - ``EXPLAIN``, ``PRAGMA``, a ``RETURNING`` clause), ``None`` for a statement - that only changes things. DuckDB 2.0 returns a relation for both, the - second over a row count, so this restores ``None`` there. The statements - run either way, once. One case stays apart: a ``RETURNING`` clause whose - only column is a ``BIGINT`` named ``Count`` reads as no rows on 2.0. + What DuckDB 1.5 answers itself: a relation for a query (``SELECT``, + ``CALL``, ``EXPLAIN``, ``PRAGMA``, a ``RETURNING`` clause), ``None`` for a + statement that only changes things, so on 1.5 this is ``con.sql``. DuckDB + 2.0 returns a relation for both, and for SQL of several statements the + relation of the last one that returned rows, which need not be the last + statement. On 2.x every statement but the last therefore runs first, and + the last one alone decides: by its type, then by the shape 2.0 gives a + statement without a query result. Each statement runs once. One case stays + apart: a ``RETURNING`` clause whose only column is a ``BIGINT`` named + ``Count`` reads as no rows on 2.x. """ - rel = con.sql(sql) - if rel is None: # DuckDB 1.5 decided already - return None + import duckdb + + from ._duckdb_sandbox import duckdb_version # the sandbox imports this module + + if duckdb_version(duckdb) < (2, 0, 0): + return con.sql(sql) try: statements = con.extract_statements(sql) - except Exception: # noqa: BLE001 - it ran, so it parses; keep its answer - return rel - last = statements[-1].type.name if statements else "SELECT" - if last in _QUERY_STATEMENTS: + except Exception: # noqa: BLE001 - DuckDB raises its own error from con.sql + statements = [] + if not statements: + return con.sql(sql) + for statement in statements[:-1]: + con.execute(statement.query).fetchall() + last = statements[-1] + rel = con.sql(last.query) + if last.type.name in _QUERY_STATEMENTS: return rel - if last in _NO_ROWS_STATEMENTS: + if last.type.name in _NO_ROWS_STATEMENTS: return None shape = (tuple(rel.columns), tuple(str(t) for t in rel.types)) return None if shape in _NO_RESULT_SHAPES else rel @@ -112,11 +143,11 @@ def query_relation(con: Any, sql: str) -> Any: def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: """``duckdb.connect`` config that keeps files DuckDB ``version`` creates readable by 1.5. - Empty before 2.0, where every file is readable by 1.5 already and the - client's own default stays. From 2.0, ``storage_compatibility_version`` - set at connect (a ``SET`` once the file is open is too late) makes each - database file the connection creates, ``ATTACH`` included, use the 1.5 - format. A file that exists keeps the format it has. + Empty before 2.0, where the client's own default is the format 1.5 writes. + From 2.0, ``storage_compatibility_version`` set at connect (a ``SET`` once + the file is open is too late) makes each database file the connection + creates, ``ATTACH`` included, use that format. A file that exists keeps the + format it has. """ if version >= (2, 0, 0): return {"storage_compatibility_version": STORAGE_COMPATIBILITY_VERSION} @@ -127,6 +158,7 @@ def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: "NO_ROWS_TO_WRITE", "STORAGE_COMPATIBILITY_VERSION", "first_row", + "one_row", "query_relation", "storage_compat_config", ] diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index e1624102..8bbe73ab 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -100,15 +100,17 @@ load (``sqlite_scan``, ``read_xlsx``, ``ST_Read``, ``delta_scan``, ``iceberg_scan``) is not in the catalog. :func:`is_sandbox_refusal` treats that error as a refusal. -* The ``postgres`` and ``mysql`` scanners open sockets through their own - client libraries, not through DuckDB's file system, so neither - ``allowed_directories`` nor ``enable_external_access`` bounds them: once - ``postgres`` is loaded, ``postgres_scan`` reaches any host the process can. - Through DuckDB 1.5 the ``sqlite`` scanner opens files that way too, so - ``sqlite_scan`` / ``ATTACH ... (TYPE sqlite)`` open any SQLite file the - process can read; from 2.0 it goes through DuckDB's file system and the - allowlist bounds it. Only the engine's own SQL runs on a connection that - loads a scanner; contract SQL never does. +* A database scanner the call site loads widens what SQL on that connection + reaches, and how far depends on the DuckDB version (each case replayed on + 1.5.0, 1.5.6 and 2.0.0). Through 1.5, once ``postgres`` is loaded, + ``postgres_scan(dsn, ...)`` connects to any host the process can reach, and + once ``sqlite`` is loaded, ``sqlite_scan`` / ``ATTACH ... (TYPE sqlite)`` open + any SQLite file it can read, whatever the allowlist says. DuckDB 2.0 has no + ``postgres_scan``, and refuses a SQLite file outside the allowlist as it + refuses any other read. On both, an ``ATTACH`` of a postgres or mysql + database after the lock is refused, so ``postgres_query`` / ``mysql_query`` + reach only what the engine attached before it. Only the engine's own SQL runs + on a connection that loads a scanner; contract SQL never does. * DuckDB shares one database instance per file per process, and the lock is instance-wide: a second connection to a file database that is already open cannot be sandboxed, and is refused with :class:`DuckDBSandboxError`. Each @@ -849,7 +851,7 @@ def secure_duckdb_connect( declared source database or creating an object-store secret. ``config`` takes scalar start-up options (``threads``, ``TimeZone``), which cannot be changed once the configuration is locked. On DuckDB 2.x a database file the - connection creates is written in DuckDB 1.5's storage format + connection creates is written in the storage format DuckDB 1.5 writes (:func:`~fluid_build.providers._duckdb_compat.storage_compat_config`), unless ``config`` names ``storage_compatibility_version`` itself. @@ -924,26 +926,14 @@ def _already_open(target: str) -> DuckDBSandboxError: ) -#: Scanner functions DuckDB does not name with their extension when it is not -#: loaded: ``mysql_scan`` on every version, ``postgres_scan`` from 2.0. -_UNADVERTISED_EXTENSION_FUNCTIONS = ("postgres_scan", "mysql_scan") - - def is_unloaded_function(exc: BaseException) -> bool: """Whether ``exc`` is a function whose extension autoloading would have loaded. DuckDB's catalog error: ``Table Function with name "sqlite_scan" is not in - the catalog, but it exists in the sqlite_scanner extension``, or, for a - scanner it does not advertise, ``Table Function with name postgres_scan - does not exist!``. + the catalog, but it exists in the sqlite_scanner extension``. """ text = str(exc) - if "but it exists in the" in text and "extension" in text: - return True - return any( - f"Table Function with name {name} does not exist" in text - for name in _UNADVERTISED_EXTENSION_FUNCTIONS - ) + return "but it exists in the" in text and "extension" in text def is_sandbox_refusal(exc: BaseException) -> bool: diff --git a/fluid_build/providers/local/local.py b/fluid_build/providers/local/local.py index 3612f5d1..93d9157b 100644 --- a/fluid_build/providers/local/local.py +++ b/fluid_build/providers/local/local.py @@ -39,7 +39,7 @@ from typing import Any, Dict, List, Optional, Tuple, Union from fluid_build.observability.secret_redactor import redact_secret_text, redact_value -from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, first_row, query_relation +from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, one_row, query_relation from fluid_build.providers._duckdb_read import build_register_view_sql from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -846,9 +846,7 @@ def _register_with_retry(): with_retry(_register_with_retry, logger=self.logger, max_attempts=3) # Get row count for reporting - rowcount = first_row(con.execute(f"SELECT COUNT(*) FROM {validate_ident(table_name)}"))[ - 0 - ] + rowcount = one_row(con.execute(f"SELECT COUNT(*) FROM {validate_ident(table_name)}"))[0] self._log_info( "local_load_data_complete", @@ -992,7 +990,7 @@ def _write_preview(): rowcount = None try: - rowcount = first_row(rel.aggregate("count(*)"))[0] + rowcount = one_row(rel.aggregate("count(*)"))[0] except Exception: pass @@ -1168,7 +1166,7 @@ def _run_copy_action(self, idx: int, action: Dict[str, Any]) -> Dict[str, Any]: try: rel = con.sql(f"SELECT * FROM {validate_ident(source_table)}") self._write_relation(rel, dst, fmt) - rowcount = first_row(rel.count("*"))[0] if hasattr(rel, "count") else -1 + rowcount = one_row(rel.count("*"))[0] if hasattr(rel, "count") else -1 del rel finally: # Closed on every path (see _run_load_data_action). diff --git a/fluid_build/providers/local_validation.py b/fluid_build/providers/local_validation.py index aa38ad89..39bc48b0 100644 --- a/fluid_build/providers/local_validation.py +++ b/fluid_build/providers/local_validation.py @@ -133,7 +133,7 @@ def validate_connection(self) -> bool: """Local provider is always reachable.""" try: conn = self._connect(allow=DuckDBAllowlist.none()) - conn.execute("SELECT 1") + conn.execute("SELECT 1").fetchall() conn.close() return True except Exception: diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 32f48ed3..6ad46960 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -23,6 +23,7 @@ from __future__ import annotations import ast +import re from pathlib import Path from typing import Any, Dict, List @@ -33,6 +34,7 @@ from fluid_build.providers._duckdb_compat import ( # noqa: E402 STORAGE_COMPATIBILITY_VERSION, first_row, + one_row, query_relation, storage_compat_config, ) @@ -74,6 +76,18 @@ def test_first_row_of_no_rows_is_none(): assert first_row(con.sql("SELECT 1 WHERE false")) is None +def test_one_row_is_the_row_of_an_aggregate(): + con = duckdb.connect() + assert one_row(con.execute("SELECT count(*) FROM range(0)")) == (0,) + assert one_row(con.sql("SELECT max(range) FROM range(4)")) == (3,) + + +def test_one_row_refuses_an_empty_result(): + """An aggregate cannot return no row; a caller indexing [0] must not get None.""" + with pytest.raises(ValueError, match="no row"): + one_row(duckdb.connect().execute("SELECT 1 WHERE false")) + + def test_the_connection_is_free_after_first_row(): """On DuckDB 2.0 a ``con.execute()`` result read only partway (``fetchone()`` on a one-row ``COUNT(*)``) keeps the connection busy, and the next relation @@ -149,6 +163,17 @@ def test_query_relation_keeps_a_query(sql, columns): "DROP TABLE t", "SET threads = 2", "SELECT 1 AS x; CREATE TABLE made AS SELECT 1 AS q", + # DuckDB 2.0 types a PIVOT whose columns it looks up as MULTI, a query + # or not: as DDL it returns a Success flag. + "CREATE TABLE p AS PIVOT t ON a USING count(*)", + # SQL ending in DML after a query: 2.0's con.sql() returns the query's rows. + "CREATE TABLE o2 AS SELECT 1 AS id; SELECT * FROM o2; INSERT INTO o2 VALUES (3)", + "SELECT a FROM t; DELETE FROM t WHERE a = 1", + "SELECT a FROM t; UPDATE t SET a = a", + "SELECT a FROM t; ALTER TABLE t ADD COLUMN b INTEGER", + "SELECT 1 AS x; SET threads = 2", + "SELECT 1 AS x; DROP TABLE t", + "BEGIN; SELECT * FROM t; COMMIT", ], ) def test_query_relation_is_none_for_a_statement_that_returns_no_rows(sql): @@ -169,6 +194,10 @@ def test_query_relation_runs_each_statement_once(): assert first_row(con.execute("SELECT count(*) FROM t WHERE a = 7")) == (1,) assert query_relation(con, "CREATE TABLE made AS SELECT 1 AS q") is None assert con.execute("SELECT q FROM made").fetchall() == [(1,)] + rel = query_relation(con, "INSERT INTO t VALUES (8); SELECT count(*) AS n FROM t WHERE a = 8") + assert rel.fetchall() == [(1,)] + assert query_relation(con, "SELECT a FROM t; DELETE FROM t WHERE a = 8") is None + assert first_row(con.execute("SELECT count(*) FROM t WHERE a = 8")) == (0,) # ── the engine, end to end ─────────────────────────────────────────────── @@ -219,35 +248,28 @@ def test_local_apply_counts_rows_and_materializes(product): ] -def test_local_sql_that_returns_no_rows_fails_the_same_on_both_versions(product): - """1.5 failed on ``None.write_csv``; 2.0 wrote a one-row ``Count`` file and - reported ok. Both now refuse it and say what to write instead.""" +@pytest.mark.parametrize( + "sql", + [ + "CREATE TABLE made AS SELECT * FROM orders", + "CREATE TABLE made AS PIVOT orders ON id USING sum(amount)", + "CREATE TABLE made AS SELECT * FROM orders; SELECT * FROM made; " + "INSERT INTO made VALUES (3, 30)", + ], + ids=["ctas", "pivot-ctas", "dml-after-a-query"], +) +def test_local_sql_that_returns_no_rows_fails_the_same_on_both_versions(product, sql): + """1.5 failed on ``None.write_csv``; 2.0 wrote a one-row ``Count`` file (or + a ``Success`` header, or the rows before the INSERT) and reported ok. Both + now refuse it and say what to write instead.""" out = product / "out" / "made.csv" - results = _local_apply( - product, - {"op": "sql", "sql": "CREATE TABLE made AS SELECT * FROM orders", "outputs": [str(out)]}, - ) - assert results[1]["status"] == "error" + results = _local_apply(product, {"op": "sql", "sql": sql, "outputs": [str(out)]}) + assert results[1]["status"] == "error", results assert "returns no rows" in results[1]["error"] - assert "End it with a SELECT of the result" in results[1]["error"] + assert "write just the SELECT" in results[1]["error"] assert not out.exists() -def test_the_fix_the_no_rows_error_suggests_works(product): - """The error's own example (CREATE, then SELECT what it made) lands the rows.""" - out = product / "out" / "made.csv" - results = _local_apply( - product, - { - "op": "sql", - "sql": "CREATE TABLE made AS SELECT * FROM orders; SELECT * FROM made", - "outputs": [str(out)], - }, - ) - assert results[1]["status"] == "ok", results - assert out.read_text(encoding="utf-8").splitlines() == ["id,amount", "1,10", "2,20"] - - def test_contract_tests_sql_that_returns_no_rows_fails_clearly(): from fluid_build.contract_tests import LocalProviderError, _execute_sql @@ -267,8 +289,8 @@ def test_contract_tests_dry_run_counts_a_real_relation(tmp_path): def test_a_remote_landing_is_probed_without_keeping_the_connection_busy(tmp_path, monkeypatch): """``_artifact_exists`` probes a remote landing with ``SELECT 1 ... LIMIT 1`` - on the run's own connection; read partway on DuckDB 2.0, that kept the - connection busy for the late-arrival split that reads the landing next.""" + on the run's own connection; on DuckDB 2.0 a result read only partway keeps + the connection busy for the next relation read on it.""" from fluid_build.build_runners.duckdb import runner landed = tmp_path / "landed.parquet" @@ -280,13 +302,8 @@ def test_a_remote_landing_is_probed_without_keeping_the_connection_busy(tmp_path assert runner._artifact_exists(con, str(tmp_path / "missing.parquet"), "read_parquet") is False -@pytest.mark.parametrize( - "function", ["postgres_scan", "postgres_query", "mysql_scan", "sqlite_scan", "read_xlsx"] -) -def test_a_scanner_the_sandbox_did_not_load_reads_as_a_refusal(function): - """DuckDB 2.0 stopped naming ``postgres_scan``'s extension in its error - ("does not exist!"), and no version names ``mysql_scan``'s; contract SQL - calling one still gets the sandbox's hint instead of a bare catalog error.""" +@pytest.mark.parametrize("function", ["postgres_query", "mysql_query", "sqlite_scan", "read_xlsx"]) +def test_a_function_of_an_extension_the_sandbox_did_not_load_reads_as_a_refusal(function): from fluid_build.providers._duckdb_sandbox import is_sandbox_refusal, is_unloaded_function con = secure_duckdb_connect(allow=DuckDBAllowlist.none()) @@ -296,6 +313,23 @@ def test_a_scanner_the_sandbox_did_not_load_reads_as_a_refusal(function): assert is_sandbox_refusal(refused.value) +@pytest.mark.skipif(not DUCKDB_2, reason="postgres_scan exists through DuckDB 1.5") +def test_duckdb_2_has_no_postgres_scan_even_with_the_extension_loaded(): + """Why the duckdb runner reads a Postgres source through ATTACH: DuckDB 2.0's + postgres extension dropped ``postgres_scan``. Not a sandbox refusal either: + loading the extension would not bring it back.""" + from fluid_build.providers._duckdb_sandbox import is_sandbox_refusal + + try: + con = secure_duckdb_connect(allow=DuckDBAllowlist.none(), extensions=["postgres"]) + except duckdb.Error as exc: + raise pytest.skip.Exception("the postgres extension is not installable here") from exc + with pytest.raises(duckdb.CatalogException) as missing: + con.execute("SELECT * FROM postgres_scan('host=127.0.0.1 port=9', 'public', 't')") + assert "does not exist" in str(missing.value) + assert not is_sandbox_refusal(missing.value) + + def test_an_unknown_function_is_not_a_sandbox_refusal(): from fluid_build.providers._duckdb_sandbox import is_sandbox_refusal @@ -335,7 +369,8 @@ def test_storage_compat_config_pins_only_from_duckdb_2(): assert storage_compat_config(version) == { "storage_compatibility_version": STORAGE_COMPATIBILITY_VERSION } - assert STORAGE_COMPATIBILITY_VERSION == "v1.5.0" + # What DuckDB 1.5 writes by default: storage version 64, "v1.0.0+". + assert STORAGE_COMPATIBILITY_VERSION == "v1.0.0" def test_a_database_file_the_engine_creates_is_readable_by_duckdb_1_5(tmp_path): @@ -344,22 +379,32 @@ def test_a_database_file_the_engine_creates_is_readable_by_duckdb_1_5(tmp_path): db = tmp_path / "new.duckdb" con = secure_duckdb_connect(db, allow=DuckDBAllowlist.none()) con.execute("CREATE TABLE t AS SELECT 1 AS a") - if DUCKDB_2: - assert _storage(con, "new") == "v1.5.0+" - else: # 1.5's own default, left alone - assert _storage(con, "new") == "v1.0.0+" + # The same format on both: 1.5's own default, which 2.x is pinned to. + assert _storage(con, "new") == "v1.0.0+" assert con.execute("SELECT current_setting('storage_compatibility_version')").fetchall() == [ (STORAGE_COMPATIBILITY_VERSION if DUCKDB_2 else "v0.10.2",) ] +def test_a_new_database_file_has_the_header_duckdb_1_5_writes(tmp_path): + """Byte-level: storage version 64 in the main header, which DuckDB 1.0 and + later read; 2.0's own default writes 999 there, which no 1.5 opens.""" + db = tmp_path / "header.duckdb" + con = secure_duckdb_connect(db, allow=DuckDBAllowlist.none()) + con.execute("CREATE TABLE t AS SELECT 1 AS a") + con.close() + header = db.read_bytes()[:20] + assert header[8:12] == b"DUCK" + assert int.from_bytes(header[12:20], "little") == 64 + + def test_an_attached_new_database_follows_the_same_format(tmp_path): con = secure_duckdb_connect( allow=DuckDBAllowlist.none(), before_lock=lambda c: c.execute(f"ATTACH '{tmp_path / 'att.duckdb'}' AS att"), ) con.execute("CREATE TABLE att.t AS SELECT 1 AS a") - assert _storage(con, "att") == ("v1.5.0+" if DUCKDB_2 else "v1.0.0+") + assert _storage(con, "att") == "v1.0.0+" def test_a_database_file_that_exists_keeps_its_format(tmp_path): @@ -385,7 +430,7 @@ def test_a_callers_own_storage_setting_wins(tmp_path): db, allow=DuckDBAllowlist.none(), config={"storage_compatibility_version": "latest"} ) con.execute("CREATE TABLE t AS SELECT 1 AS a") - assert _storage(con, "latest") != "v1.5.0+" + assert _storage(con, "latest").startswith("v2.") def test_the_storage_setting_is_locked_with_the_rest(tmp_path): @@ -399,15 +444,17 @@ def test_the_storage_setting_is_locked_with_the_rest(tmp_path): #: Reads that DuckDB 2.0 removed from relations or ``con.execute()`` results, or #: that leave a result open (the connection stays busy until it is drained): #: ``fetchone``/``fetchmany`` (``first_row`` replaces them), the Arrow batch -#: readers, ``rel.arrow()``. A relation's argument-less ``execute()``, also gone, -#: is caught separately. +#: readers, ``rel.arrow()``. Caught separately: a relation's argument-less +#: ``execute()`` (gone), and an ``execute()`` of a query whose result is thrown +#: away unread (open until the next ``execute()``). ``rel.pl()`` is not listed: +#: relations keep it in 2.0. _PARTIAL_OR_GONE = frozenset( {"fetchone", "fetchmany", "fetch_df_chunk", "fetch_arrow_table", "fetch_record_batch"} | {"fetch_arrow_reader", "record_batch", "arrow"} ) -#: Modules whose ``fetchone`` (or argument-less ``execute``) belongs to another -#: client, never DuckDB's. +#: Modules whose ``fetchone`` (or ``execute``) belongs to another client, never +#: DuckDB's; each must keep importing no DuckDB. _OTHER_CLIENTS = { "fluid_build/cli/doctor.py": "sqlite3 / Snowflake connector cursors", "fluid_build/cli/workspace.py": "sqlite3 workspace registry", @@ -417,13 +464,46 @@ def test_the_storage_setting_is_locked_with_the_rest(tmp_path): "fluid_build/copilot/store/backends/vector.py": "sqlite3 + sqlite-vec probe", "fluid_build/forge_datamodel/from_ddl/snowflake_dumper.py": "Snowflake connector cursor", "fluid_build/providers/gcp/util/auth.py": "googleapiclient request .execute()", + "fluid_build/output_ports/mcp/drivers/postgres.py": "psycopg cursor health probe", + "fluid_build/output_ports/mcp/drivers/snowflake.py": "Snowflake cursor health probe", } +#: The start of a statement that returns rows, for an ``execute()`` whose result +#: is thrown away. +_QUERY_START = re.compile( + r"\s*(SELECT|WITH|FROM|TABLE|VALUES|DESCRIBE|SHOW|SUMMARIZE|PRAGMA|CALL|EXPLAIN|PIVOT)\b", + re.IGNORECASE, +) + + +def _literal_start(node: ast.expr) -> str: + """The leading literal text of a ``str`` or f-string argument, else ``""``.""" + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + if isinstance(node, ast.JoinedStr) and node.values: + first = node.values[0] + if isinstance(first, ast.Constant) and isinstance(first.value, str): + return first.value + return "" + + +def _unread_query(node: ast.AST) -> bool: + """``.execute("SELECT ...")`` as a statement of its own: its rows are never read.""" + if not (isinstance(node, ast.Expr) and isinstance(node.value, ast.Call)): + return False + call = node.value + return ( + isinstance(call.func, ast.Attribute) + and call.func.attr == "execute" + and bool(call.args) + and bool(_QUERY_START.match(_literal_start(call.args[0]))) + ) + def _partial_reads(path: Path) -> List[str]: tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) where = path.relative_to(REPO_ROOT).as_posix() if REPO_ROOT in path.parents else path.name - return [ + found = [ f"{where}:{node.lineno} .{node.func.attr}()" for node in ast.walk(tree) if isinstance(node, ast.Call) @@ -433,6 +513,10 @@ def _partial_reads(path: Path) -> List[str]: or (node.func.attr == "execute" and not node.args and not node.keywords) ) ] + found += [ + f"{where}:{node.lineno} unread .execute()" for node in ast.walk(tree) if _unread_query(node) + ] + return found @pytest.mark.parametrize( @@ -448,6 +532,9 @@ def _partial_reads(path: Path) -> List[str]: "rel.record_batch()\n", "rel.arrow()\n", "rel.execute()\n", + "con.execute('SELECT 1')\n", + "con.execute(f'select count(*) FROM {table}')\n", + "con.execute('DESCRIBE t')\n", ], ) def test_the_partial_read_detector_catches_each_shape(shape, tmp_path): @@ -456,6 +543,22 @@ def test_the_partial_read_detector_catches_each_shape(shape, tmp_path): assert _partial_reads(sample), shape +@pytest.mark.parametrize( + "shape", + [ + "con.execute('SELECT 1').fetchall()\n", + "rows = con.execute('SELECT 1')\n", + "con.execute('CREATE TABLE t AS SELECT 1')\n", + "con.execute(f'COPY ({select}) TO {path}')\n", + "con.execute(sql)\n", + ], +) +def test_the_partial_read_detector_leaves_a_read_or_a_write_alone(shape, tmp_path): + sample = tmp_path / "probe.py" + sample.write_text(shape, encoding="utf-8") + assert not _partial_reads(sample), shape + + def test_no_partial_reads_in_duckdb_code(): offenders, scanned = [], 0 for path in sorted(PACKAGE.rglob("*.py")): @@ -481,3 +584,8 @@ def test_every_other_client_module_still_needs_its_exemption(): path = REPO_ROOT / module assert path.is_file(), module assert _partial_reads(path), f"{module} no longer needs its exemption; drop it" + # The exemption is for another client's calls: DuckDB code there would + # go unchecked. + assert not re.search( + r"import duckdb|from duckdb|secure_duckdb_connect", path.read_text(encoding="utf-8") + ), f"{module} now uses DuckDB; it cannot stay exempt" diff --git a/tests/providers/test_duckdb_sandbox.py b/tests/providers/test_duckdb_sandbox.py index d6eb3e0a..26f8bb10 100644 --- a/tests/providers/test_duckdb_sandbox.py +++ b/tests/providers/test_duckdb_sandbox.py @@ -1133,8 +1133,8 @@ def test_a_function_from_an_unloaded_extension_reads_as_a_sandbox_refusal(layout def test_contract_sql_runs_on_a_connection_with_no_database_scanner(layout, printed): - """sqlite/postgres/mysql scanners are not bounded by the allowlist (next - test), so the connection contract SQL runs on must never have one loaded.""" + """A loaded database scanner widens what SQL on the connection reaches (the + tests below), so the connection contract SQL runs on must never have one.""" rc = _build( "SELECT count(*) AS n FROM duckdb_functions() WHERE function_name IN " "('sqlite_scan', 'sqlite_attach', 'postgres_scan', 'postgres_query', 'mysql_query')", @@ -1213,9 +1213,11 @@ def test_from_duckdb_2_a_loaded_sqlite_scanner_is_bounded_by_the_allowlist(tmp_p con.execute(refused).fetchall() +@pytest.mark.skipif(_DUCKDB_2, reason="DuckDB 2.0's postgres extension has no postgres_scan") def test_a_loaded_postgres_scanner_is_not_bounded_by_the_allowlist(): - """Pins a documented limit: ``postgres_scan`` opens its own socket, so the - failure is the server's (nothing listens), not the sandbox's refusal.""" + """Pins a documented limit through DuckDB 1.5: ``postgres_scan`` opens its + own socket, so the failure is the server's (nothing listens), not the + sandbox's refusal.""" con = _with_extension("postgres") with pytest.raises(duckdb.Error) as failed: con.execute( @@ -1225,6 +1227,16 @@ def test_a_loaded_postgres_scanner_is_not_bounded_by_the_allowlist(): assert "Permission Error" not in str(failed.value) +@pytest.mark.parametrize("extension", ["postgres", "mysql"]) +def test_after_the_lock_no_database_can_be_attached_through_a_loaded_scanner(extension): + """On every version: ``postgres_query`` / ``mysql_query`` reach only the + databases the engine attached before the lock, which is how the duckdb + runner reads a postgres or mysql source.""" + con = _with_extension(extension) + with pytest.raises(duckdb.PermissionException, match="disabled through configuration"): + con.execute(f"ATTACH 'host=127.0.0.1 port=9 connect_timeout=1' AS x (TYPE {extension})") + + # ── the helper's own guarantees ────────────────────────────────────────── diff --git a/tests/test_contract_tests_branches.py b/tests/test_contract_tests_branches.py index 14cbd922..944f8587 100644 --- a/tests/test_contract_tests_branches.py +++ b/tests/test_contract_tests_branches.py @@ -230,15 +230,6 @@ def test_invalid_format(self): with pytest.raises(LocalProviderError, match="format must be"): _write_output(rel, {"path": "/tmp/out.txt", "format": "json"}, dry_run=False) - def test_csv_dry_run(self): - rel = MagicMock() - count_rel = MagicMock() - count_rel.fetchall.return_value = [(42,)] - rel.count.return_value = count_rel - cnt, path = _write_output(rel, {"path": "/tmp/out.csv", "format": "csv"}, dry_run=True) - assert cnt == 42 - assert path is None - def test_csv_dry_run_count_error(self): rel = MagicMock() rel.count.side_effect = RuntimeError("count fail") From ef820b7530ad488a15b082e4d0ffd0e935fab14d Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 07:14:14 +0200 Subject: [PATCH 09/14] fix(duckdb): query_relation lets the result's shape decide on 2.x A mutation pass on the round-2 code showed _NO_ROWS_STATEMENTS redundant: once the last statement runs alone, every DDL, SET, ATTACH, LOAD and transaction statement returns a Count or Success shape on DuckDB 2.0 (probed for 27 statement kinds), so the set never changed an answer. It is gone; a query type (SELECT, EXPLAIN, CALL, PRAGMA) keeps its relation, as DuckDB 1.5 does even for CHECKPOINT's Success flag (now a test case), and anything else is judged by its shape. The same pass showed no test of a postgres source that cannot connect and has no password: the unreachable-source test now runs with and without one, so an attach error that escapes _execute as a raw exception fails it. --- fluid_build/providers/_duckdb_compat.py | 27 ++++++++++------------- tests/build_runners/test_duckdb_runner.py | 7 ++++-- tests/providers/test_duckdb_compat.py | 2 ++ 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index dd31347b..b43125e0 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -78,16 +78,12 @@ def one_row(result: Any) -> Tuple[Any, ...]: return row -#: Statement types whose result is a query result (rows), whatever they hold. -#: Not ``MULTI``, 2.0's type for a ``PIVOT`` with columns it must look up first: -#: a bare ``PIVOT`` returns rows, a ``CREATE TABLE ... AS PIVOT`` does not, so -#: that one is judged by the result's shape. +#: Statement types whose result is a query result, whatever it holds: DuckDB 1.5 +#: returns a relation for these even when it is a ``Success`` flag +#: (``CHECKPOINT`` is a ``CALL``). Not ``MULTI``, 2.0's type for a ``PIVOT`` +#: with columns it must look up first: a bare ``PIVOT`` returns rows, a +#: ``CREATE TABLE ... AS PIVOT`` does not, so its shape decides. _QUERY_STATEMENTS = frozenset({"SELECT", "EXPLAIN", "CALL", "PRAGMA"}) -#: Statement types that never return rows. -_NO_ROWS_STATEMENTS = frozenset( - {"CREATE", "DROP", "ALTER", "SET", "VARIABLE_SET", "ATTACH", "DETACH", "TRANSACTION"} - | {"LOAD", "VACUUM", "ANALYZE", "EXPORT", "COPY_DATABASE", "PREPARE"} -) #: What 2.0 holds for a statement with no query result: the changed-row count #: (``INSERT``, ``UPDATE``, ``DELETE``, ``MERGE``, ``COPY``), or a success flag. _NO_RESULT_SHAPES = frozenset({(("Count",), ("BIGINT",)), (("Success",), ("BOOLEAN",))}) @@ -111,10 +107,13 @@ def query_relation(con: Any, sql: str) -> Any: 2.0 returns a relation for both, and for SQL of several statements the relation of the last one that returned rows, which need not be the last statement. On 2.x every statement but the last therefore runs first, and - the last one alone decides: by its type, then by the shape 2.0 gives a - statement without a query result. Each statement runs once. One case stays - apart: a ``RETURNING`` clause whose only column is a ``BIGINT`` named - ``Count`` reads as no rows on 2.x. + the last one alone decides: a query type keeps its relation, and any other + statement is judged by the shape 2.0 gives one without a query result (a + ``Count`` of changed rows, or a ``Success`` flag; every DDL, ``SET``, + ``ATTACH``, ``LOAD`` and transaction statement returns one of the two). + Each statement runs once. One case stays apart: a ``RETURNING`` clause + whose only column is a ``BIGINT`` named ``Count``, or a ``BOOLEAN`` named + ``Success``, reads as no rows on 2.x. """ import duckdb @@ -134,8 +133,6 @@ def query_relation(con: Any, sql: str) -> Any: rel = con.sql(last.query) if last.type.name in _QUERY_STATEMENTS: return rel - if last.type.name in _NO_ROWS_STATEMENTS: - return None shape = (tuple(rel.columns), tuple(str(t) for t in rel.types)) return None if shape in _NO_RESULT_SHAPES else rel diff --git a/tests/build_runners/test_duckdb_runner.py b/tests/build_runners/test_duckdb_runner.py index e0caf939..96722e78 100644 --- a/tests/build_runners/test_duckdb_runner.py +++ b/tests/build_runners/test_duckdb_runner.py @@ -212,7 +212,10 @@ def test_missing_uri_fails(self, tmp_path): # ── postgres, no server ─────────────────────────────────────────────────── -def test_an_unreachable_postgres_source_fails_the_build_not_the_process(tmp_path, caplog): +@pytest.mark.parametrize("with_password", [True, False], ids=["password", "no-password"]) +def test_an_unreachable_postgres_source_fails_the_build_not_the_process( + tmp_path, caplog, with_password +): """The source is attached as the run's connection opens (DuckDB 2.0 has no ``postgres_scan``), so its connect error is reported as the build's failure, with the password masked, as a failing stream was before.""" @@ -240,7 +243,7 @@ def test_an_unreachable_postgres_source_fails_the_build_not_the_process(tmp_path "host": "127.0.0.1", "port": 9, # discard: nothing listens "user": "fluid", - "password": password, + **({"password": password} if with_password else {}), "database": "app", }, "mode": "full_refresh", diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 6ad46960..81c10789 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -141,6 +141,8 @@ def _table_t() -> Any: ("DESCRIBE t", None), ("CALL pragma_version()", None), ("EXPLAIN SELECT 1", None), + # A CALL whose result is a Success flag: 1.5 returns it as a relation. + ("CHECKPOINT", ["Success"]), ], ) def test_query_relation_keeps_a_query(sql, columns): From 9217f6ec360cdb36a20a152bc2e2cf51dc453776 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 07:49:28 +0200 Subject: [PATCH 10/14] fix(duckdb): a MySQL source attaches under the sandbox; setup errors fail the run Found by the defect scan's recon, then reproduced live against MySQL 8.4: every DuckDB acquisition build reading MySQL or MariaDB failed before it connected, on DuckDB 1.5.6 and 2.0.0 alike (main too), with "Unknown secret storage found: 'local_file'". Without a secret named in the ATTACH, the mysql extension looks for a default one in every secret storage, and the sandbox turns the persistent one off; the postgres attach hit the same and got an empty temporary secret in bd71a015. The mysql attach now gets one too (_empty_secret_sql, shared), and is READ_ONLY like the postgres one: CREATE and mysql_execute on the alias are refused (probed live). A connection block the DSN builder refuses, or a secretRef that does not resolve, raised a bare ValueError out of the connect, which `fluid apply` printed as "Unexpected error during execution" with no run record. It is now a _SourceSetupError (a _SourceAttachError and still a ValueError), so the run fails with the reason; an extension that did not load raises _SourceAttachError too. The messages name the field or the reference, never a value. TestDuckdbMysqlLive and TestDuckdbPostgresLive pass on 3.10/1.5.6, 3.12/1.5.0, 3.12/1.5.6, 3.12/2.0.0 and 3.14/2.0.0 (local mysql:8 and postgres:16-alpine). The offline unreachable-source test now runs for postgres and mysql, with and without a password, and asserts each extension's own connection failure: against main's code it fails 4/4 on 2.0.0 and the 2 mysql cases on 1.5.6. --- CHANGELOG.md | 20 ++-- fluid_build/build_runners/duckdb/runner.py | 95 ++++++++++++------- tests/build_runners/test_duckdb_runner.py | 32 ++++--- .../test_duckdb_runner_mysql_attach.py | 37 +++++++- 4 files changed, 124 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7225a2dc..d15833ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,14 +76,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 keeps its connection busy until it is read to the end, which fails the next step on that connection. Every DuckDB result is now read to the end, the same way on 1.5.x and 2.x. -- **DuckDB 2.0: a Postgres source.** A DuckDB acquisition build reading Postgres failed - on DuckDB 2.0 (`Table Function with name postgres_scan does not exist!`): the postgres - extension dropped `postgres_scan`. The source is now attached read-only before the - sandbox locks, as MySQL and SQLite sources are, and read as `..
` - on 1.5.x and 2.x; its `connection.secretRef` is resolved into the password, as a MySQL - source's already was. A Postgres or MySQL source the runner cannot connect to now fails - the build with the connect error, its password masked, and writes a run record; it - used to end `fluid apply` with `Unexpected error during execution`. +- **DuckDB 2.0: Postgres and MySQL sources.** A DuckDB acquisition build reading + Postgres failed on DuckDB 2.0 (`Table Function with name postgres_scan does not + exist!`): the postgres extension dropped `postgres_scan`. A MySQL or MariaDB source + failed on every DuckDB version, before connecting (`Unknown secret storage found: + 'local_file'`): its `ATTACH` looked for a default secret in the persistent storage the + sandbox turns off. Both are now attached read-only before the sandbox locks, each with + an empty temporary secret named in the `ATTACH` (the DSN carries the credentials), and + read as `..
` on 1.5.x and 2.x; a Postgres source's + `connection.secretRef` is resolved into the password, as a MySQL source's already was. + A source the runner cannot attach (unreachable, refused, a `secretRef` that does not + resolve) now fails the build with the reason, its password masked, and writes a run + record; it used to end `fluid apply` with `Unexpected error during execution`. - A local SQL action, or a legacy `fluid_build.contract_tests` action, whose SQL ends in a statement that returns no rows (`CREATE TABLE ... AS`, an `INSERT` without `RETURNING`) now fails with a message saying to write just the `SELECT`. DuckDB 1.5 diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 0faece5d..6b957540 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -405,7 +405,7 @@ def _postgres_attach_sql(connection: Dict[str, Any], *, alias: str) -> str: source cannot be written through it (``postgres_execute`` and ``INSERT`` are refused). The DSN is the shared :func:`build_libpq_dsn` one (control characters and a non-numeric port refused), one SQL literal at the boundary. - ``SECRET `` names the empty secret :func:`_postgres_secret_sql` makes. + ``SECRET `` names the empty secret :func:`_empty_secret_sql` makes. """ alias = validate_ident(alias) dsn = build_libpq_dsn(connection, database_key="dbname") @@ -415,15 +415,37 @@ def _postgres_attach_sql(connection: Dict[str, Any], *, alias: str) -> str: ) -def _postgres_secret_sql(*, alias: str) -> str: - """``CREATE TEMPORARY SECRET (TYPE postgres)``: empty, the DSN carries the credentials. +def _empty_secret_sql(secret_type: str, *, alias: str) -> str: + """``CREATE TEMPORARY SECRET (TYPE postgres|mysql)``: empty, the DSN carries the credentials. - Without a secret named in the ``ATTACH`` the postgres extension looks for a - default one in every secret storage, and the persistent storage the sandbox - turns off answers ``Unknown secret storage found: 'local_file'`` (DuckDB 1.5 - and 2.0 alike). A temporary secret lives in memory, on this connection only. + Without a secret named in the ``ATTACH``, the postgres and mysql extensions + look for a default one in every secret storage, and the persistent storage + the sandbox turns off answers ``Unknown secret storage found: 'local_file'`` + (DuckDB 1.5 and 2.0 alike), so no postgres or mysql source could attach. A + temporary secret lives in memory, on this connection only. """ - return f"CREATE TEMPORARY SECRET {validate_ident(alias)} (TYPE postgres)" + if secret_type not in ("postgres", "mysql"): + raise ValueError(f"no attach secret for a {secret_type!r} source") + return f"CREATE TEMPORARY SECRET {validate_ident(alias)} (TYPE {secret_type})" + + +def _mysql_attach_sql(connection: Dict[str, Any], *, alias: str) -> str: + """``ATTACH '' AS (TYPE mysql, SECRET , READ_ONLY)``. + + mysql_scanner's own DSN syntax, not libpq's: libpq quoting let a value with + a space add ``socket=`` or ``ssl_key=``, and a ``?`` turn the rest into URI + options. :func:`build_mysql_dsn` double-quotes each value and refuses a + ``?``, a ``/`` in the host and a control character. The extension type is + ``mysql`` for a MariaDB upstream too. ``SECRET `` names the empty + secret :func:`_empty_secret_sql` makes; ``READ_ONLY``, as a postgres source + is attached, since acquisition only reads. + """ + alias = validate_ident(alias) + dsn = build_mysql_dsn(connection, database_key="database") + return ( + f"ATTACH {quote_ansi_string_literal(dsn)} AS {alias} " + f"(TYPE mysql, SECRET {alias}, READ_ONLY)" + ) def _build_select_for_postgres(stream: str, *, alias: str) -> str: @@ -793,9 +815,10 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: / ``sqlite_``) so two sources in the same contract / interpreter don't collide on a hardcoded name. - Best-effort: if the connection DSN is malformed or the upstream - is unreachable, the ATTACH raises and the runner surfaces the - error in the per-stream try/except, with the password masked. + A connection block the DSN builder refuses, a ``secretRef`` that does + not resolve, or an upstream that cannot be reached raises a + :class:`_SourceAttachError` (the password masked), which fails the run + with that reason and a run record (``_execute``). A sqlite file is confined to :func:`_declared_roots` plus the operator's ``FLUID_DUCKDB_ALLOWED_DIRS`` first, and refused with @@ -803,31 +826,29 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: made before the lock. """ kind = ctx.source.kind - if kind == "postgres": - # Resolve secretRef → password, as the mysql branch does; an inline - # literal still wins. - conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) - alias = _postgres_alias_for_build(ctx.build_id) - attach_sql = _postgres_attach_sql(conn, alias=alias) # validates first - con.execute(_postgres_secret_sql(alias=alias)) + if kind in ("postgres", "mysql", "mariadb"): + import duckdb + + postgres = kind == "postgres" + try: + # Resolve secretRef → password before the DSN is built; an + # inline literal still wins. + conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) + if postgres: + alias = _postgres_alias_for_build(ctx.build_id) + attach_sql = _postgres_attach_sql(conn, alias=alias) # validates first + else: + alias = _mysql_alias_for_build(ctx.build_id) + attach_sql = _mysql_attach_sql(conn, alias=alias) # validates first + con.execute(_empty_secret_sql("postgres" if postgres else "mysql", alias=alias)) + except ValueError as exc: + # A DSN value the builder refuses, or a secretRef that does not + # resolve: the message names the field or the reference, never + # a value of the connection's. + raise _SourceSetupError(str(exc)) from exc + except duckdb.Error as exc: # the extension did not load + raise _SourceAttachError(str(exc)) from exc _attach_with_password_masked(con, attach_sql, conn.get("password")) - elif kind in ("mysql", "mariadb"): - # Resolve secretRef → password before build_mysql_dsn reads - # connection.get("password"). Inline literal values still win. - conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) - # mysql_scanner's own DSN syntax, not libpq's: libpq quoting let a - # value with a space add socket= or ssl_key=, and a '?' turn the - # rest into URI options. build_mysql_dsn double-quotes each value - # and refuses a '?', a '/' in the host and a control character. - dsn = build_mysql_dsn(conn, database_key="database") - alias = _mysql_alias_for_build(ctx.build_id) - # mysql extension supports both mysql:// and mariadb:// upstreams; - # the duckdb extension type literal is always ``mysql``. - _attach_with_password_masked( - con, - f"ATTACH {quote_ansi_string_literal(dsn)} AS {alias} (TYPE mysql)", - conn.get("password"), - ) elif kind == "sqlite": conn = dict(ctx.source.connection.raw) # SQLite uses a path, not a libpq-style DSN. The path comes @@ -851,6 +872,10 @@ class _SourceAttachError(RuntimeError): """The source database could not be attached; the message holds no password.""" +class _SourceSetupError(_SourceAttachError, ValueError): + """The source's connection block was refused before any attach was tried.""" + + def _attach_with_password_masked(con: Any, statement: str, password: Any) -> None: """Run an ``ATTACH`` that carries a DSN; its connect error loses the password. diff --git a/tests/build_runners/test_duckdb_runner.py b/tests/build_runners/test_duckdb_runner.py index 96722e78..7e7d2434 100644 --- a/tests/build_runners/test_duckdb_runner.py +++ b/tests/build_runners/test_duckdb_runner.py @@ -212,33 +212,42 @@ def test_missing_uri_fails(self, tmp_path): # ── postgres, no server ─────────────────────────────────────────────────── +_CONNECT_FAILED = { + "postgres": "Unable to connect to Postgres", + "mysql": "Failed to connect to MySQL", +} + + @pytest.mark.parametrize("with_password", [True, False], ids=["password", "no-password"]) -def test_an_unreachable_postgres_source_fails_the_build_not_the_process( - tmp_path, caplog, with_password +@pytest.mark.parametrize("kind", ["postgres", "mysql"]) +def test_an_unreachable_database_source_fails_the_build_not_the_process( + tmp_path, caplog, kind, with_password ): """The source is attached as the run's connection opens (DuckDB 2.0 has no ``postgres_scan``), so its connect error is reported as the build's failure, - with the password masked, as a failing stream was before.""" + with the password masked. The error is a connection failure: not a function + DuckDB 2.0 lacks, nor the "Unknown secret storage found: 'local_file'" that + an ATTACH without a named secret hits under the sandbox.""" duckdb = pytest.importorskip("duckdb") try: probe = duckdb.connect() - probe.execute("INSTALL postgres; LOAD postgres") + probe.execute(f"INSTALL {kind}; LOAD {kind}") probe.close() except duckdb.Error: - pytest.skip("the postgres extension is not installable here (offline)") - password = "runner-unreachable-pg-pw-6T" # pragma: allowlist secret + pytest.skip(f"the {kind} extension is not installable here (offline)") + password = "runner-unreachable-db-pw-6T" # pragma: allowlist secret out_path = tmp_path / "orders.parquet" contract = { "fluidVersion": "0.7.3", - "id": "bronze.pg_orders", + "id": "bronze.db_orders", "builds": [ { - "id": "ingest_pg", + "id": "ingest_db", "pattern": "acquisition", "engine": "duckdb", "properties": { "source": { - "kind": "postgres", + "kind": kind, "connection": { "host": "127.0.0.1", "port": 9, # discard: nothing listens @@ -247,7 +256,7 @@ def test_an_unreachable_postgres_source_fails_the_build_not_the_process( "database": "app", }, "mode": "full_refresh", - "streams": ["public.orders"], + "streams": ["app.orders"], }, "sink": {"format": "parquet"}, }, @@ -271,8 +280,7 @@ def test_an_unreachable_postgres_source_fails_the_build_not_the_process( rc = execute_duckdb_build(contract["builds"][0], contract, tmp_path, dry_run=False) assert rc == 1 assert not out_path.exists() - # A connection failure, the source reached: not a function DuckDB 2.0 lacks. - assert "Unable to connect to Postgres" in caplog.text + assert _CONNECT_FAILED[kind] in caplog.text, caplog.text[-2000:] assert password not in caplog.text for written in tmp_path.rglob("*"): if written.is_file(): diff --git a/tests/build_runners/test_duckdb_runner_mysql_attach.py b/tests/build_runners/test_duckdb_runner_mysql_attach.py index 194800cb..3e3d3dc1 100644 --- a/tests/build_runners/test_duckdb_runner_mysql_attach.py +++ b/tests/build_runners/test_duckdb_runner_mysql_attach.py @@ -50,8 +50,8 @@ def _attach_on(con: Any, raw: Dict[str, Any], kind: str = "mysql") -> None: runner._attach_external_databases(con, ctx) # type: ignore[arg-type] -def _attach(raw: Dict[str, Any], kind: str = "mysql") -> List[str]: - """Run the runner's mysql attach on a recording connection; return the SQL run.""" +def _executed(raw: Dict[str, Any], kind: str = "mysql") -> List[str]: + """Run the runner's attach on a recording connection; return every statement run.""" seen: List[str] = [] class _Con: @@ -62,6 +62,11 @@ def execute(self, sql: str, *_a: Any, **_k: Any) -> None: return seen +def _attach(raw: Dict[str, Any], kind: str = "mysql") -> List[str]: + """The ``ATTACH`` statements :func:`_executed` saw (the empty secret left out).""" + return [sql for sql in _executed(raw, kind) if not sql.startswith("CREATE TEMPORARY SECRET")] + + def _dsn_of(attach_sql: str) -> str: """The DSN inside ``ATTACH '' AS ...`` with the SQL literal unquoted.""" assert attach_sql.startswith("ATTACH '"), attach_sql @@ -124,7 +129,15 @@ def test_one_keyword_per_field(self, field, value): def test_a_plain_connection(self): (sql,) = _attach(_BASE) assert dict(_parse_mysql_dsn(_dsn_of(sql))) == _BASE - assert sql.endswith(" AS mysql_b1 (TYPE mysql)") + assert sql.endswith(" AS mysql_b1 (TYPE mysql, SECRET mysql_b1, READ_ONLY)") + + def test_an_empty_temporary_secret_is_named_in_the_attach(self): + """Without it the extension looks for a default secret in the persistent + storage the sandbox turned off: ``Unknown secret storage found: + 'local_file'``, and no mysql source attached at all.""" + secret, sql = _executed(_BASE) + assert secret == "CREATE TEMPORARY SECRET mysql_b1 (TYPE mysql)" + assert sql.startswith("ATTACH '") class TestRefusedBeforeTheAttach: @@ -209,7 +222,7 @@ class TestPostgresAttach: attaches mysql and sqlite.""" def test_the_source_is_attached_read_only_under_a_per_build_alias(self): - secret, sql = _attach(dict(_BASE, port="5432"), kind="postgres") + secret, sql = _executed(dict(_BASE, port="5432"), kind="postgres") # An empty temporary secret: the sandbox's disabled persistent storage # otherwise fails the ATTACH's default-secret lookup. assert secret == "CREATE TEMPORARY SECRET postgres_b1 (TYPE postgres)" @@ -233,9 +246,21 @@ def test_a_secret_ref_password_is_resolved(self, monkeypatch): monkeypatch.setenv("RUNNER_FAKE_PG_SECRET", "runner-pg-ref-pw-3J") raw = {k: v for k, v in _BASE.items() if k != "password"} raw["secretRef"] = "env://RUNNER_FAKE_PG_SECRET" # pragma: allowlist secret - _secret, sql = _attach(raw, kind="postgres") + (sql,) = _attach(raw, kind="postgres") assert "password=runner-pg-ref-pw-3J" in _dsn_of(sql) + @pytest.mark.parametrize("kind", ["postgres", "mysql"]) + def test_a_secret_ref_that_does_not_resolve_fails_the_source_setup(self, kind, monkeypatch): + from fluid_build.build_runners.duckdb.runner import _SourceAttachError + + monkeypatch.delenv("RUNNER_FAKE_UNSET_SECRET", raising=False) + raw = {k: v for k, v in _BASE.items() if k != "password"} + raw["secretRef"] = "env://RUNNER_FAKE_UNSET_SECRET" # pragma: allowlist secret + with pytest.raises(_SourceAttachError, match="RUNNER_FAKE_UNSET_SECRET") as caught: + _executed(raw, kind=kind) + # Still a ValueError for callers that catch the builder's refusal. + assert isinstance(caught.value, ValueError) + def test_a_connect_error_has_the_password_masked(self): class _Con: def execute(self, sql: str, *_a: Any, **_k: Any) -> None: @@ -262,6 +287,8 @@ class TestConnectErrorHasThePasswordMasked: def _failing_con() -> Any: class _Con: def execute(self, sql: str, *_a: Any, **_k: Any) -> None: + if not sql.startswith("ATTACH"): + return # What mysql_scanner 1.5.6 says, quoting the DSN and the SQL. raise RuntimeError( "IO Error: Failed to connect to MySQL database with parameters " From 084f5b8703d56b4ab7b729042d934916c4393d14 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 08:12:29 +0200 Subject: [PATCH 11/14] fix(duckdb): what the second review round found - A secretRef whose backend fails (vault, aws, gcp, azure or file raise ConfigurationError, not ValueError) escaped the runner, so `fluid apply` ended with no run record and skipped its later builds. Any error resolving the secretRef now fails the source setup like a bad port or an unset variable does. A new end-to-end test checks exit 1 and a failed run record for a bad port, an unset env:// variable and a file-backend refusal, on postgres and mysql; narrowing the catch back to ValueError fails 4 tests. - _postgres_attach_sql's docstring said READ_ONLY refuses postgres_execute. It does not: inside an explicit transaction, postgres_execute('COMMIT') and then a CREATE TABLE wrote to a live Postgres 16 on DuckDB 1.5.6 and 2.0.0. It now says READ_ONLY refuses writes through the attached catalog, and that only the engine's own SQL runs on that connection. - The duckdb-floor job retries its installs as the other jobs do, and also runs the DuckDB runner's tests and every suite the DuckDB end-to-end job runs (quickstarts, examples, the local happy path). The new tests/test_ci_duckdb_floor.py fails when the end-to-end job runs a file the floor job does not, or when a test that reads the DuckDB version is missing from it. On duckdb 1.5.0 the list gives 455 passed; the 2 failures are the macOS-only /var tests that fail the same on main. - VARIANT is not a 2.0-only type (DuckDB 1.5.0 has it): the CHANGELOG and the storage-pin comment now say a VARIANT column needs storage v1.5.0 and is refused in the pinned format on 1.5 and 2.x alike. - CHANGELOG: only a MySQL or MariaDB source used to end `fluid apply` with "Unexpected error during execution"; a Postgres source already failed the build. - Two sandbox test docstrings no longer say the sqlite scanner is never bounded and that postgres stays unbounded. --- .github/workflows/ci.yml | 49 ++++++++-- CHANGELOG.md | 10 ++- fluid_build/build_runners/duckdb/runner.py | 21 +++-- fluid_build/providers/_duckdb_compat.py | 4 +- tests/build_runners/test_duckdb_runner.py | 89 ++++++++++++++----- .../test_duckdb_runner_mysql_attach.py | 15 +++- tests/providers/test_duckdb_sandbox.py | 10 ++- tests/test_ci_duckdb_floor.py | 71 +++++++++++++++ 8 files changed, 225 insertions(+), 44 deletions(-) create mode 100644 tests/test_ci_duckdb_floor.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 21fb8c8e..1d84f546 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -630,13 +630,16 @@ jobs: # This installs exactly the floor the sandbox enforces (MIN_DUCKDB_VERSION, # read from the code, so the two cannot drift) on a supported Python and # runs what differs between DuckDB versions: the compat seam - # (providers/_duckdb_compat.py), the sandbox, the MCP DuckDB driver and the - # local end-to-end suite. + # (providers/_duckdb_compat.py), the sandbox, the MCP DuckDB driver, the + # DuckDB acquisition runner (its database-source attaches) and every suite + # the DuckDB end-to-end job runs. tests/test_ci_duckdb_floor.py fails when a + # file the end-to-end job runs, or a test that branches on the DuckDB + # version, is missing here. # ────────────────────────────────────────────────────────────── duckdb-floor: name: DuckDB floor (Python 3.12) runs-on: ubuntu-latest - timeout-minutes: 15 + timeout-minutes: 20 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 @@ -644,17 +647,51 @@ jobs: python-version: "3.12" cache: pip - name: Install with local extras, then the DuckDB floor + env: + # Same pip timeout and retried install as the test matrix (see the + # comments there). + PIP_TIMEOUT: "30" run: | python -m pip install --upgrade pip - python -m pip install -e ".[dev,local]" + for attempt in 1 2 3; do + python -m pip install -e ".[dev,local]" && break + if [ "$attempt" -eq 3 ]; then + echo "::error::pip install -e .[dev,local] failed 3 times; this is an install failure, not a test failure." + exit 1 + fi + echo "::warning::pip install -e .[dev,local] failed (attempt ${attempt}/3); retrying in 30s." + sleep 30 + done floor=$(python -c "from fluid_build.providers._duckdb_sandbox import MIN_DUCKDB_VERSION as v; print('.'.join(map(str, v)))") - python -m pip install "duckdb==${floor}" + for attempt in 1 2 3; do + python -m pip install "duckdb==${floor}" && break + if [ "$attempt" -eq 3 ]; then + echo "::error::pip install duckdb==${floor} failed 3 times; this is an install failure, not a test failure." + exit 1 + fi + echo "::warning::pip install duckdb==${floor} failed (attempt ${attempt}/3); retrying in 30s." + sleep 30 + done python -c "import duckdb, sys; print('duckdb', duckdb.__version__); sys.exit(duckdb.__version__ != '${floor}')" - name: Run the DuckDB-version-sensitive tests on the floor env: + # The quickstart and example suites fail, rather than skip, without + # duckdb (as in the DuckDB end-to-end job). FLUID_REQUIRE_DUCKDB: "1" run: | - python -m pytest -q tests/providers/test_duckdb_compat.py tests/providers/test_duckdb_sandbox.py tests/providers/test_duckdb_sandbox_workspace_root.py tests/output_ports/test_drivers_duckdb.py tests/test_e2e_local.py tests/test_e2e_local_apply.py tests/test_e2e_local_negative.py + python -m pytest -q \ + tests/providers/test_duckdb_compat.py \ + tests/providers/test_duckdb_sandbox.py \ + tests/providers/test_duckdb_sandbox_workspace_root.py \ + tests/output_ports/test_drivers_duckdb.py \ + tests/build_runners/test_duckdb_runner.py \ + tests/build_runners/test_duckdb_runner_mysql_attach.py \ + tests/test_e2e_local.py \ + tests/test_e2e_local_apply.py \ + tests/test_e2e_local_negative.py \ + tests/providers/test_local_live_happy_path.py \ + tests/test_quickstart_templates_execute.py \ + tests/test_examples_execute.py - name: Cleanup local artifacts (defensive) if: always() run: python scripts/cleanup_local_test_artifacts.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d15833ed..f625a113 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 needs Python 3.11, so a Python 3.10 install stays on 1.5.x). On 2.x, a DuckDB database file forge-cli creates (`fluid demo`'s `.fluid/db.duckdb`, the local provider's session and `persist` databases, a Meltano acquisition's DuckDB destination) is written in the - format DuckDB 1.5 writes, which DuckDB 1.0 and later open, so a 2.0-only type such as - `VARIANT` is refused in it as on 1.5; a file that already exists keeps its format. + format DuckDB 1.5 writes, which DuckDB 1.0 and later open, so a type that needs a newer + format, such as a `VARIANT` column, is refused in it on 2.x as it is on 1.5; a file + that already exists keeps its format. DuckDB 2.x's own default format cannot be read by 1.5: a file another 2.x tool created (dbt-duckdb and dlt included), or forge-cli 0.21.0 or older on DuckDB 2.0, opens only on 2.x. To convert one, run on 2.x: `ATTACH 'old.duckdb' AS src (READ_ONLY); ATTACH @@ -86,8 +87,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 read as `..
` on 1.5.x and 2.x; a Postgres source's `connection.secretRef` is resolved into the password, as a MySQL source's already was. A source the runner cannot attach (unreachable, refused, a `secretRef` that does not - resolve) now fails the build with the reason, its password masked, and writes a run - record; it used to end `fluid apply` with `Unexpected error during execution`. + resolve) fails the build with the reason, its password masked, and writes a run + record; a MySQL or MariaDB source used to end `fluid apply` with `Unexpected error + during execution` instead. - A local SQL action, or a legacy `fluid_build.contract_tests` action, whose SQL ends in a statement that returns no rows (`CREATE TABLE ... AS`, an `INSERT` without `RETURNING`) now fails with a message saying to write just the `SELECT`. DuckDB 1.5 diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 6b957540..9ac2b1ab 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -401,9 +401,12 @@ def _postgres_attach_sql(connection: Dict[str, Any], *, alias: str) -> str: """``ATTACH '' AS (TYPE postgres, SECRET , READ_ONLY)``. DuckDB 1.5's ``postgres_scan(dsn, schema, table)`` is gone from DuckDB 2.0's - postgres extension; ``ATTACH`` works on both, and attached ``READ_ONLY`` the - source cannot be written through it (``postgres_execute`` and ``INSERT`` are - refused). The DSN is the shared :func:`build_libpq_dsn` one (control + postgres extension; ``ATTACH`` works on both. ``READ_ONLY`` refuses a write + through the attached catalog (``INSERT``, ``CREATE``) and runs the + connection in a read-only transaction, but it is no write barrier: + ``postgres_execute`` sends its SQL to the server, which can end that + transaction (a ``COMMIT``) and then write. Only the engine's own SQL runs on + this connection. The DSN is the shared :func:`build_libpq_dsn` one (control characters and a non-numeric port refused), one SQL literal at the boundary. ``SECRET `` names the empty secret :func:`_empty_secret_sql` makes. """ @@ -834,6 +837,13 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: # Resolve secretRef → password before the DSN is built; an # inline literal still wins. conn = resolve_connection_secrets(dict(ctx.source.connection.raw)) + except Exception as exc: + # An unset variable or an unknown scheme (ValueError), or the + # secret backend's own error (ConfigurationError from vault, + # aws, gcp, azure or file): each names the reference, never + # the secret. + raise _SourceSetupError(str(exc)) from exc + try: if postgres: alias = _postgres_alias_for_build(ctx.build_id) attach_sql = _postgres_attach_sql(conn, alias=alias) # validates first @@ -842,9 +852,8 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: attach_sql = _mysql_attach_sql(conn, alias=alias) # validates first con.execute(_empty_secret_sql("postgres" if postgres else "mysql", alias=alias)) except ValueError as exc: - # A DSN value the builder refuses, or a secretRef that does not - # resolve: the message names the field or the reference, never - # a value of the connection's. + # A DSN value the builder refuses: the message names the field + # (and quotes a refused port), never a credential. raise _SourceSetupError(str(exc)) from exc except duckdb.Error as exc: # the extension did not load raise _SourceAttachError(str(exc)) from exc diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index b43125e0..5cdf91c9 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -49,8 +49,8 @@ #: The storage format DuckDB 1.5 writes by default (storage version 64, which #: DuckDB 1.0 and later open). Database files forge-cli creates on DuckDB 2.x #: are written in it, so a 1.5 install (Python 3.10, or a tool pinned below 2) -#: opens them, and a 2.x connection accepts the same SQL on them as 1.5 does -#: (no 2.0-only types such as VARIANT). +#: opens them, and a 2.x connection accepts the same SQL on them as 1.5 does (a +#: VARIANT column, which needs storage v1.5.0, is refused on both). STORAGE_COMPATIBILITY_VERSION = "v1.0.0" diff --git a/tests/build_runners/test_duckdb_runner.py b/tests/build_runners/test_duckdb_runner.py index 7e7d2434..d96dc91a 100644 --- a/tests/build_runners/test_duckdb_runner.py +++ b/tests/build_runners/test_duckdb_runner.py @@ -218,16 +218,7 @@ def test_missing_uri_fails(self, tmp_path): } -@pytest.mark.parametrize("with_password", [True, False], ids=["password", "no-password"]) -@pytest.mark.parametrize("kind", ["postgres", "mysql"]) -def test_an_unreachable_database_source_fails_the_build_not_the_process( - tmp_path, caplog, kind, with_password -): - """The source is attached as the run's connection opens (DuckDB 2.0 has no - ``postgres_scan``), so its connect error is reported as the build's failure, - with the password masked. The error is a connection failure: not a function - DuckDB 2.0 lacks, nor the "Unknown secret storage found: 'local_file'" that - an ATTACH without a named secret hits under the sandbox.""" +def _require_extension(kind: str) -> None: duckdb = pytest.importorskip("duckdb") try: probe = duckdb.connect() @@ -235,9 +226,11 @@ def test_an_unreachable_database_source_fails_the_build_not_the_process( probe.close() except duckdb.Error: pytest.skip(f"the {kind} extension is not installable here (offline)") - password = "runner-unreachable-db-pw-6T" # pragma: allowlist secret - out_path = tmp_path / "orders.parquet" - contract = { + + +def _database_source_contract(kind: str, connection: Dict[str, Any], out_path: Path) -> dict: + """One acquisition build reading ``app.orders`` from a ``kind`` database.""" + return { "fluidVersion": "0.7.3", "id": "bronze.db_orders", "builds": [ @@ -248,13 +241,7 @@ def test_an_unreachable_database_source_fails_the_build_not_the_process( "properties": { "source": { "kind": kind, - "connection": { - "host": "127.0.0.1", - "port": 9, # discard: nothing listens - "user": "fluid", - **({"password": password} if with_password else {}), - "database": "app", - }, + "connection": connection, "mode": "full_refresh", "streams": ["app.orders"], }, @@ -276,6 +263,29 @@ def test_an_unreachable_database_source_fails_the_build_not_the_process( } ], } + + +@pytest.mark.parametrize("with_password", [True, False], ids=["password", "no-password"]) +@pytest.mark.parametrize("kind", ["postgres", "mysql"]) +def test_an_unreachable_database_source_fails_the_build_not_the_process( + tmp_path, caplog, kind, with_password +): + """The source is attached as the run's connection opens (DuckDB 2.0 has no + ``postgres_scan``), so its connect error is reported as the build's failure, + with the password masked. The error is a connection failure: not a function + DuckDB 2.0 lacks, nor the "Unknown secret storage found: 'local_file'" that + an ATTACH without a named secret hits under the sandbox.""" + _require_extension(kind) + password = "runner-unreachable-db-pw-6T" # pragma: allowlist secret + out_path = tmp_path / "orders.parquet" + connection = { + "host": "127.0.0.1", + "port": 9, # discard: nothing listens + "user": "fluid", + **({"password": password} if with_password else {}), + "database": "app", + } + contract = _database_source_contract(kind, connection, out_path) caplog.set_level("DEBUG") rc = execute_duckdb_build(contract["builds"][0], contract, tmp_path, dry_run=False) assert rc == 1 @@ -287,6 +297,45 @@ def test_an_unreachable_database_source_fails_the_build_not_the_process( assert password not in written.read_text(encoding="utf-8", errors="replace"), written +@pytest.mark.parametrize( + "refused, reason", + [ + ({"port": "abc"}, "connection.port must be numeric"), + # The run record's redactor reads "_SECRET: environment" as an + # assignment and masks its value, so the record says "variable not set". + ({"secretRef": "env://FLUID_TEST_UNSET_SOURCE_SECRET"}, "variable not set"), + # The file backend refuses a name with a separator: a ConfigurationError, + # as every secret backend but env:// raises, not a ValueError. + ({"secretRef": "file://a/b"}, "Invalid secret name"), + ], + ids=["port", "unset-secret-ref", "secret-backend-error"], +) +@pytest.mark.parametrize("kind", ["postgres", "mysql"]) +def test_a_source_the_runner_cannot_set_up_fails_the_build_with_a_run_record( + tmp_path, monkeypatch, kind, refused, reason +): + """A connection block refused before any attach (a port that is not a + number, a secretRef that does not resolve) fails the build as a connect + error does: exit 1 and a failed run record, not an exception out of the + runner that ends `fluid apply` before its other builds run.""" + import json + + _require_extension(kind) + monkeypatch.delenv("FLUID_TEST_UNSET_SOURCE_SECRET", raising=False) + out_path = tmp_path / "orders.parquet" + connection = {"host": "127.0.0.1", "port": 9, "user": "fluid", "database": "app", **refused} + contract = _database_source_contract(kind, connection, out_path) + rc = execute_duckdb_build(contract["builds"][0], contract, tmp_path, dry_run=False) + assert rc == 1 + assert not out_path.exists() + runs = tmp_path / ".fluid" / "runs" / "bronze.db_orders" / "ingest_db" / "runs" + (record,) = [json.loads(p.read_text(encoding="utf-8")) for p in runs.glob("*.json")] + assert record["state"] == "failed" + (stream,) = record["streams"] + assert stream["state"] == "failed" + assert stream["error"].startswith("source: ") and reason in stream["error"], stream + + # ── postgres (docker-required) ──────────────────────────────────────────── diff --git a/tests/build_runners/test_duckdb_runner_mysql_attach.py b/tests/build_runners/test_duckdb_runner_mysql_attach.py index 3e3d3dc1..564fbf9c 100644 --- a/tests/build_runners/test_duckdb_runner_mysql_attach.py +++ b/tests/build_runners/test_duckdb_runner_mysql_attach.py @@ -218,8 +218,8 @@ def test_refused(self, field, ch): class TestPostgresAttach: """DuckDB 2.0's postgres extension has no ``postgres_scan``, so the runner - attaches a postgres source before the sandbox's lock, read-only, as it - attaches mysql and sqlite.""" + attaches a postgres source before the sandbox's lock, as it attaches mysql + and sqlite sources, and attaches it read-only.""" def test_the_source_is_attached_read_only_under_a_per_build_alias(self): secret, sql = _executed(dict(_BASE, port="5432"), kind="postgres") @@ -261,6 +261,17 @@ def test_a_secret_ref_that_does_not_resolve_fails_the_source_setup(self, kind, m # Still a ValueError for callers that catch the builder's refusal. assert isinstance(caught.value, ValueError) + @pytest.mark.parametrize("kind", ["postgres", "mysql"]) + def test_a_secret_backend_error_fails_the_source_setup(self, kind): + """Every backend but env:// raises ConfigurationError, not ValueError; + the file backend refuses a name with a separator before reading.""" + from fluid_build.build_runners.duckdb.runner import _SourceSetupError + + raw = {k: v for k, v in _BASE.items() if k != "password"} + raw["secretRef"] = "file://a/b" # pragma: allowlist secret + with pytest.raises(_SourceSetupError, match="Invalid secret name"): + _executed(raw, kind=kind) + def test_a_connect_error_has_the_password_masked(self): class _Con: def execute(self, sql: str, *_a: Any, **_k: Any) -> None: diff --git a/tests/providers/test_duckdb_sandbox.py b/tests/providers/test_duckdb_sandbox.py index 26f8bb10..11fc8121 100644 --- a/tests/providers/test_duckdb_sandbox.py +++ b/tests/providers/test_duckdb_sandbox.py @@ -522,9 +522,10 @@ def test_the_copy_sibling_is_one_more_file_confined_like_the_landing(layout, mon def test_a_sqlite_source_outside_the_allowed_directories_is_refused(layout, monkeypatch): - """The sqlite scanner opens files through its own library, which DuckDB's - allowlist never bounds, so the declared source path was attached wherever - it pointed: any SQLite file on the host could be landed.""" + """The engine attaches a SQLite source before the sandbox's lock, when no + allowlist applies yet (and through DuckDB 1.5 the sqlite scanner did not + honour one), so the declared source path was attached wherever it pointed: + any SQLite file on the host could be landed.""" import sqlite3 from fluid_build.build_runners.duckdb.runner import execute_duckdb_build @@ -1186,7 +1187,8 @@ def test_from_duckdb_2_a_loaded_sqlite_scanner_is_bounded_by_the_allowlist(tmp_p the allowlist bounds it as it bounds ``read_csv``: a file outside, ``./..`` out of an allowed directory and a symlink out of one are each refused, while a file inside is read. The engine still keeps contract SQL off a - connection that loads a scanner (``postgres`` stays unbounded).""" + connection that loads a scanner (``postgres_query`` reaches what the engine + attached before the lock).""" inside, outside = tmp_path / "allowed", tmp_path / "outside" inside.mkdir() outside.mkdir() diff --git a/tests/test_ci_duckdb_floor.py b/tests/test_ci_duckdb_floor.py new file mode 100644 index 00000000..2cde2afc --- /dev/null +++ b/tests/test_ci_duckdb_floor.py @@ -0,0 +1,71 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""The ``duckdb-floor`` CI job runs what differs between DuckDB versions. + +pyproject.toml takes ``duckdb>=1.5.0`` with no ceiling, and that job is the +only one that installs the floor itself. Its test list is written out, and a +written-out list drifts: the MCP SDK job's had come to name 3 of the 12 MCP +test files when it was replaced by ``-k mcp``. So the list must hold every +file the DuckDB end-to-end job runs, and every test file that branches on the +installed DuckDB version. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +import yaml + +pytestmark = pytest.mark.unit + +REPO_ROOT = Path(__file__).resolve().parents[1] +CI = REPO_ROOT / ".github" / "workflows" / "ci.yml" + +_TEST_FILE = re.compile(r"\btests/[\w/]+\.py\b") +#: How a test reads the installed DuckDB version (the compat and sandbox tests +#: derive their ``_DUCKDB_2`` flag from one of these). +_READS_THE_VERSION = re.compile(r"duckdb\.__version__|\bduckdb_version\(|\bMIN_DUCKDB_VERSION\b") + + +def _files_run_by(job: str) -> set[str]: + steps = yaml.safe_load(CI.read_text(encoding="utf-8"))["jobs"][job]["steps"] + scripts = [step.get("run", "") for step in steps] + return {path for script in scripts if "pytest" in script for path in _TEST_FILE.findall(script)} + + +def test_the_floor_runs_every_suite_the_duckdb_end_to_end_job_runs(): + end_to_end = _files_run_by("duckdb-integration") + assert end_to_end, "found no test file in the DuckDB end-to-end job's pytest step" + missing = sorted(end_to_end - _files_run_by("duckdb-floor")) + assert not missing, f"add to the duckdb-floor job in ci.yml: {missing}" + + +def test_the_floor_runs_every_test_that_branches_on_the_duckdb_version(): + this_file = Path(__file__).resolve() + branching = { + path.relative_to(REPO_ROOT).as_posix() + for path in (REPO_ROOT / "tests").rglob("test_*.py") + if path.resolve() != this_file + and _READS_THE_VERSION.search(path.read_text(encoding="utf-8")) + } + assert branching, "found no test that reads the DuckDB version; the scan is broken" + missing = sorted(branching - _files_run_by("duckdb-floor")) + assert not missing, f"add to the duckdb-floor job in ci.yml: {missing}" + + +def test_every_file_the_floor_names_exists(): + absent = sorted(p for p in _files_run_by("duckdb-floor") if not (REPO_ROOT / p).is_file()) + assert not absent, f"the duckdb-floor job names files that do not exist: {absent}" From a2506ac44742c79fe52f08fbda68aca25f0f9d58 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 08:43:41 +0200 Subject: [PATCH 12/14] fix(duckdb): what the third review round found Postgres sources (regressions of bd71a015's ATTACH): - An enum column: postgres_scan typed it VARCHAR, the ATTACH-ed catalog types it ENUM(...), so a contract declaring the column VARCHAR failed the schema gate (mood/type_changed->fail) on DuckDB 1.5.6 and 2.0.0 where 0.21.0 passed. The Postgres SELECT now casts enum columns to VARCHAR (an enum array to VARCHAR[]), as the masking projection replaces columns. Live, the cross-file hunter's contract fails on 084f5b87 and passes here on both versions, landing mood as VARCHAR. - The schema gate fingerprints on a connection of its own and swallows a failed fingerprint, so a fingerprint that could not attach the source would land drifted data; nothing pinned it. A sqlite source with a strict schema it drifts from now has a test (SchemaDriftError, nothing landed). - CHANGELOG: an upgrade note that a Postgres secretRef is now resolved (0.21.0 ignored it and libpq's PGPASSWORD supplied the password), that an enum column still reads as text, and that stream names match without regard to case. query_relation on DuckDB 2.x, against 1.5's answers: - SQL whose later statement does not parse ran the statements ahead of it (2.0's con.sql() runs them before it raises); 1.5 runs none. It now raises from extract_statements before anything runs. - A comment after the last ';' broke the relation 2.0 returns: aggregate, limit and write_csv re-read the statement's text and fail ("syntax error at or near ';'"). The trailing comments are cut, found with duckdb.tokenize (byte offsets, so a multibyte string is cut right). - Statements ahead of the last are run with 2.0's complete() instead of fetchall(), so a large SELECT among them is not read into Python. relation_view: 2.0 refuses a view over rows it already holds (a PIVOT without an IN list, nextval(), RETURNING), which failed a local SQL action's s3:// output and contract_tests' Parquet output. relation_view falls back to a table; contract_tests' view, which only its unreachable COPY used, may fail and its pandas fallback writes the file. Also: detect-secrets flagged `secret == "CREATE TEMPORARY SECRET ..."` in two tests and the shifted baseline line of test_security_regressions.py (CI's Lint job would have failed); MySQL docstrings name the SECRET and READ_ONLY attach; scripts/verify_engines.sh read a relation with fetchone(); the live round trips' skip reason no longer says CI runs them; the example README says READ_ONLY is no write barrier and suggests a read-only role; the new CI guard test has the blank line the license writer expects. Each fix has a test that fails without it (7 mutants, all killed). --- .secrets.baseline | 4 +- CHANGELOG.md | 14 ++ .../source-aligned-postgres-duckdb/README.md | 5 +- fluid_build/build_runners/duckdb/runner.py | 49 +++++-- fluid_build/contract_tests.py | 8 +- fluid_build/providers/_duckdb_compat.py | 96 ++++++++++--- fluid_build/providers/local/local.py | 12 +- scripts/verify_engines.sh | 2 +- tests/build_runners/test_duckdb_runner.py | 119 +++++++++++++++- .../test_duckdb_runner_mysql_attach.py | 8 +- tests/providers/test_duckdb_compat.py | 132 ++++++++++++++++++ tests/test_ci_duckdb_floor.py | 1 + 12 files changed, 402 insertions(+), 48 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index e8381aa8..5e153ce6 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -1350,7 +1350,7 @@ "filename": "tests/security/test_security_regressions.py", "hashed_secret": "7c9ce88af5f76ecad340e5e6a6e40bca11c3d234", "is_verified": false, - "line_number": 213 + "line_number": 217 } ], "tests/security/test_security_regressions_round2.py": [ @@ -2154,5 +2154,5 @@ } ] }, - "generated_at": "2026-10-13T20:49:43Z" + "generated_at": "2026-10-22T06:15:34Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index f625a113..543d2ea5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `x -> x + 1` lambda is an error (write `lambda x: x + 1`), and integer `//` and `%` by zero raise an error instead of returning NULL (wrap them in `TRY(...)`). See https://duckdb.org/docs/2.0/operations_manual/migration.html. +- A DuckDB acquisition build now resolves a Postgres source's `connection.secretRef`. It + ignored it, and libpq's `PGPASSWORD` or `~/.pgpass` supplied the password, so a + `secretRef` that does not resolve where `fluid apply` runs now fails the build. To keep + using `PGPASSWORD`, remove the `secretRef`; an inline `password` still wins over it. - `fluid validate --env --strict` (on one contract or a whole workspace), `fluid contract-validation --env --strict` and `fluid test --env --strict` (the native engine; `--engine soda` does not read `--strict`) now fail when an overlay patch @@ -86,6 +90,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 an empty temporary secret named in the `ATTACH` (the DSN carries the credentials), and read as `..
` on 1.5.x and 2.x; a Postgres source's `connection.secretRef` is resolved into the password, as a MySQL source's already was. + A Postgres enum column still reads, and lands, as text. A stream name is looked up as + DuckDB looks up names, ignoring case: where two tables' names differ only in case, the + build can read the other one. A source the runner cannot attach (unreachable, refused, a `secretRef` that does not resolve) fails the build with the reason, its password masked, and writes a run record; a MySQL or MariaDB source used to end `fluid apply` with `Unexpected error @@ -96,6 +103,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 failed there on `'NoneType' object has no attribute 'write_csv'`, and DuckDB 2.0 reported success with a one-row `Count` file, an empty `Success` file (a `CREATE TABLE ... AS PIVOT`) or the rows of an earlier `SELECT` in the same SQL as the output. +- **DuckDB 2.0: contract SQL.** A local SQL action whose SQL ends in a comment after its + last `;` failed to write its output (`syntax error at or near ";"`); SQL with a + statement that does not parse ran the statements ahead of it before failing, where + DuckDB 1.5 runs none; and the result of a `PIVOT` without an `IN` list, or of a query + calling `nextval()`, could not be written to an `s3://` output or, in + `fluid_build.contract_tests`, to Parquet (`Cannot create the view ...: it reads rows + held in this process`). Each now behaves as on 1.5. ## [0.21.0] — 2026-10-19 diff --git a/examples/source-aligned-postgres-duckdb/README.md b/examples/source-aligned-postgres-duckdb/README.md index 40f8dffc..5121ea93 100644 --- a/examples/source-aligned-postgres-duckdb/README.md +++ b/examples/source-aligned-postgres-duckdb/README.md @@ -45,7 +45,10 @@ table to `out/orders.parquet`, and persists a run record under ## What this exercises - v0.7.3 `acquisition` build pattern + `duckdb` engine -- Postgres source attached by DuckDB's postgres extension (no Airbyte server needed) +- Postgres source attached by DuckDB's postgres extension (no Airbyte server needed). + The attach is `READ_ONLY`, which refuses writes through DuckDB's catalog but + is no write barrier on the server; outside this demo, connect as a role that + can only read (for example one granted `pg_read_all_data`) - Parquet sink via `COPY (...) TO ... (FORMAT 'parquet')` - Run-record persistence + lock acquisition - Pre-land hook chain (`dlp_scan`, `quality_gate`) — non-blocking on this diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 9ac2b1ab..1bc7e203 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -22,7 +22,7 @@ - ``filesystem`` -> ``read_csv`` / ``read_parquet`` / ``read_json`` - ``postgres`` -> ``ATTACH ... (TYPE postgres, READ_ONLY)``, then ``..
`` - - ``mysql`` -> ``ATTACH ... (TYPE mysql)``, then ``..
`` + - ``mysql`` -> ``ATTACH ... (TYPE mysql, READ_ONLY)``, then ``..
`` - ``sqlite`` -> ``ATTACH ... (TYPE sqlite)``, then ``.
`` - ``http`` -> ``read_csv_auto('https://...')`` @@ -469,12 +469,37 @@ def _build_select_for_postgres(stream: str, *, alias: str) -> str: return f"SELECT * FROM {alias}.{schema}.{table}" +#: How an ``ATTACH``-ed Postgres catalog types an enum column, ``ENUM('a', 'b')``, +#: or an array of one, ``ENUM('a', 'b')[]``. Group 1 is the array suffix. +_ENUM_TYPE = re.compile(r"ENUM\(.*\)((?:\[\])*)", re.DOTALL) + + +def _postgres_enums_as_varchar(con: Any, select_sql: str) -> str: + """``select_sql`` with each enum column read as ``VARCHAR`` (an enum array as ``VARCHAR[]``). + + ``postgres_scan``, which read a Postgres source until DuckDB 2.0 dropped + it, typed an enum column ``VARCHAR``; the ``ATTACH``-ed catalog types it + ``ENUM(...)``. Read as before, an enum column is no type change to the + declared schema the gate compares, a ``range`` gate compares it as text + rather than in declaration order, and it lands as the text it always did. + The same ``SELECT * REPLACE`` the masking projection uses. + """ + replaced = [] + for row in con.execute(f"DESCRIBE {select_sql}").fetchall(): + match = _ENUM_TYPE.fullmatch(str(row[1])) + if match: + ident = '"' + str(row[0]).replace('"', '""') + '"' + replaced.append(f"CAST({ident} AS VARCHAR{match.group(1)}) AS {ident}") + if not replaced: + return select_sql + return f"SELECT * REPLACE ({', '.join(replaced)}) FROM ({select_sql})" + + def _build_select_for_mysql(connection: Dict[str, Any], stream: str, *, alias: str) -> str: """Read from MySQL via the duckdb ``mysql`` extension. - Requires that ``ATTACH '' AS (TYPE mysql)`` has already - been issued in the same connection (handled by - ``DuckdbRunner._attach_external_databases``). + Requires the ``ATTACH`` of :func:`_mysql_attach_sql` on the same + connection (``DuckdbRunner._attach_external_databases`` makes it). The ``alias`` is per-build (derived from the build_id) so a contract with two mysql sources doesn't collide on a hardcoded ``mysql_db`` @@ -762,7 +787,7 @@ def fingerprint(self, ctx: RunContext) -> SchemaFingerprint: before_lock=lambda c: self._load_extensions(c, ctx), ) try: - select_sql = _select_for_first_stream(ctx) + select_sql = _select_for_first_stream(con, ctx) con.execute(f"CREATE TEMP VIEW _fp AS {select_sql}") rows = con.execute("DESCRIBE _fp").fetchall() # What lands, not what the source holds: a masked column lands as @@ -906,11 +931,12 @@ def _attach_with_password_masked(con: Any, statement: str, password: Any) -> Non raise _SourceAttachError(masked) -def _select_for_first_stream(ctx: RunContext) -> str: - return _select_for_stream(ctx, ctx.source.streams[0] if ctx.source.streams else "data") +def _select_for_first_stream(con: Any, ctx: RunContext) -> str: + return _select_for_stream(con, ctx, ctx.source.streams[0] if ctx.source.streams else "data") -def _select_for_stream(ctx: RunContext, stream: str) -> str: +def _select_for_stream(con: Any, ctx: RunContext, stream: str) -> str: + """The ``SELECT`` of ``stream`` on ``con``, the run's connection with the source attached.""" kind = ctx.source.kind conn = dict(ctx.source.connection.raw) if kind == "filesystem": @@ -924,7 +950,10 @@ def _select_for_stream(ctx: RunContext, stream: str) -> str: opts = dict(ctx.source.reader.options) if ctx.source.reader else {} return _build_select_for_http(uri, fmt or "csv", opts) if kind == "postgres": - return _build_select_for_postgres(stream, alias=_postgres_alias_for_build(ctx.build_id)) + select_sql = _build_select_for_postgres( + stream, alias=_postgres_alias_for_build(ctx.build_id) + ) + return _postgres_enums_as_varchar(con, select_sql) if kind in ("mysql", "mariadb"): # MariaDB uses the same duckdb mysql extension; the discoverer # already accepts both schemes (cli/discover/mysql.py), keep @@ -1464,7 +1493,7 @@ def _execute(ctx: RunContext, runner: DuckdbRunner) -> RunResult: for stream in streams_to_run: t_stream = time.time() try: - base_sel = _select_for_stream(ctx, stream) + base_sel = _select_for_stream(con, ctx, stream) # Apply incremental cursor filter if the contract asked for it. # ``last_cursor_value`` is the value we filtered on (None for # full refresh OR first incremental run); kept for the diff --git a/fluid_build/contract_tests.py b/fluid_build/contract_tests.py index 00295956..88f3cfcb 100644 --- a/fluid_build/contract_tests.py +++ b/fluid_build/contract_tests.py @@ -270,7 +270,13 @@ def _write_output(rel, outputs: Dict[str, Any], dry_run: bool) -> Tuple[int, Opt # Create a temp view and then COPY from it. tmp_view = "__fluid_tmp_out" - rel.create_view(tmp_view, replace=True) + try: + rel.create_view(tmp_view, replace=True) + except Exception as e: + # DuckDB 2.0 refuses a view over rows it already holds (a PIVOT, + # nextval(), RETURNING); the COPY below then fails too, and its + # pandas fallback writes the file. + LOGGER.debug(f"Failed to create temp view {tmp_view}: {e}") if mode == "append": # Append parquet: create if not exists then append is not trivial. # DuckDB supports COPY with APPEND (v0.10+). Try; else fallback. diff --git a/fluid_build/providers/_duckdb_compat.py b/fluid_build/providers/_duckdb_compat.py index 5cdf91c9..baadf647 100644 --- a/fluid_build/providers/_duckdb_compat.py +++ b/fluid_build/providers/_duckdb_compat.py @@ -19,7 +19,7 @@ needs Python 3.11, so a Python 3.10 install stays on 1.5.x while a newer one gets 2.x: the engine runs on both, and what differs lives here. -Three 2.0 changes reach the engine (duckdb-python v2.0.0 release notes, +These 2.0 changes reach the engine (duckdb-python v2.0.0 release notes, "Upgrading from 1.x"; https://duckdb.org/docs/2.0/clients/python/migration.html): * A ``con.execute()`` result streams, and owns its connection until it is read @@ -31,8 +31,15 @@ ... AS``, an ``INSERT`` without ``RETURNING``) returns a relation where 1.5 returned ``None``: over the statement's row count, or, after an earlier query in the same SQL, over that query's rows. Written out as a contract's - result, that is a one-row ``Count`` file or stale rows. :func:`query_relation` - answers ``None`` for it on both. + result, that is a one-row ``Count`` file or stale rows. It also runs the + statements ahead of one that does not parse, where 1.5 runs none, and a + relation of SQL that ends in a comment after its ``;`` fails to aggregate, + limit or write ("syntax error at or near ';'"). :func:`query_relation` + answers as 1.5 does on both. +* A relation whose rows 2.0 already holds (its SQL ran when the relation was + made: a ``PIVOT`` without an ``IN`` list, a ``SELECT`` calling + ``nextval()``, ``... RETURNING``) cannot back a view ("it reads rows held in + this process"). :func:`relation_view` puts those rows in a table instead. * A database file 2.0 creates uses storage version 2.0, which no 1.5 process can open ("can only read versions between 64 and 68"). :func:`storage_compat_config` has 2.x write the format 1.5 itself writes, so a file forge-cli creates is the @@ -44,13 +51,15 @@ from __future__ import annotations -from typing import Any, Dict, Optional, Tuple +from contextlib import contextmanager +from typing import Any, Dict, Iterator, Optional, Tuple -#: The storage format DuckDB 1.5 writes by default (storage version 64, which -#: DuckDB 1.0 and later open). Database files forge-cli creates on DuckDB 2.x -#: are written in it, so a 1.5 install (Python 3.10, or a tool pinned below 2) -#: opens them, and a 2.x connection accepts the same SQL on them as 1.5 does (a -#: VARIANT column, which needs storage v1.5.0, is refused on both). +#: Storage version 64, the one DuckDB 1.5 writes by default (its own setting +#: reads ``v0.10.2``, which writes the same version), and which DuckDB 1.0 and +#: later open. Database files forge-cli creates on DuckDB 2.x are written in +#: it, so a 1.5 install (Python 3.10, or a tool pinned below 2) opens them, and +#: a 2.x connection accepts the same SQL on them as 1.5 does (a VARIANT column, +#: which needs storage v1.5.0, is refused on both). STORAGE_COMPATIBILITY_VERSION = "v1.0.0" @@ -111,9 +120,10 @@ def query_relation(con: Any, sql: str) -> Any: statement is judged by the shape 2.0 gives one without a query result (a ``Count`` of changed rows, or a ``Success`` flag; every DDL, ``SET``, ``ATTACH``, ``LOAD`` and transaction statement returns one of the two). - Each statement runs once. One case stays apart: a ``RETURNING`` clause - whose only column is a ``BIGINT`` named ``Count``, or a ``BOOLEAN`` named - ``Success``, reads as no rows on 2.x. + Each statement runs once, and SQL that does not parse raises before any of + it runs, as on 1.5. One case stays apart: a ``RETURNING`` clause whose only + column is a ``BIGINT`` named ``Count``, or a ``BOOLEAN`` named ``Success``, + reads as no rows on 2.x. """ import duckdb @@ -121,22 +131,69 @@ def query_relation(con: Any, sql: str) -> Any: if duckdb_version(duckdb) < (2, 0, 0): return con.sql(sql) - try: - statements = con.extract_statements(sql) - except Exception: # noqa: BLE001 - DuckDB raises its own error from con.sql - statements = [] - if not statements: + # Parses the whole SQL first: a statement that does not parse raises here, + # before any runs (2.0's con.sql() would run the statements ahead of it). + statements = con.extract_statements(sql) + if not statements: # empty, or only comments return con.sql(sql) for statement in statements[:-1]: - con.execute(statement.query).fetchall() + con.execute(statement.query) + # To its end without reading its rows into Python (2.0's complete()): + # a SELECT ahead of the last statement is run for its effects only. + con.complete() last = statements[-1] - rel = con.sql(last.query) + rel = con.sql(_without_trailing_comments(last.query)) if last.type.name in _QUERY_STATEMENTS: return rel shape = (tuple(rel.columns), tuple(str(t) for t in rel.types)) return None if shape in _NO_RESULT_SHAPES else rel +def _without_trailing_comments(statement: str) -> str: + """``statement`` without the comments that end it. + + DuckDB 2.0 keeps a statement's text with the relation ``con.sql()`` makes + and reads it again inside the SQL of ``aggregate``, ``limit`` or + ``write_csv``, where a comment after the closing ``;`` no longer parses. + ``duckdb.tokenize`` tells a comment from a ``--`` inside a string, and + gives each token's offset in bytes. + """ + import duckdb + + cut = None + for offset, kind in reversed(duckdb.tokenize(statement)): + if kind != duckdb.token_type.comment: + break + cut = offset + if cut is None: + return statement + return statement.encode("utf-8")[:cut].decode("utf-8").rstrip() + + +@contextmanager +def relation_view(con: Any, rel: Any, name: str) -> Iterator[str]: + """``name`` reads ``rel`` while the block runs (a view), for SQL such as ``COPY``. + + DuckDB 2.0 refuses a view over a relation whose rows it already holds (see + the module docstring); those rows go to a table of that name instead. The + view or table is dropped when the block ends. ``name`` is an identifier the + caller owns. + """ + import duckdb + + try: + rel.create_view(name, replace=True) + kind = "VIEW" + except duckdb.InvalidInputException: + con.execute(f"DROP TABLE IF EXISTS {name}") + rel.to_table(name) + kind = "TABLE" + try: + yield name + finally: + con.execute(f"DROP {kind} IF EXISTS {name}") + + def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: """``duckdb.connect`` config that keeps files DuckDB ``version`` creates readable by 1.5. @@ -157,5 +214,6 @@ def storage_compat_config(version: Tuple[int, int, int]) -> Dict[str, str]: "first_row", "one_row", "query_relation", + "relation_view", "storage_compat_config", ] diff --git a/fluid_build/providers/local/local.py b/fluid_build/providers/local/local.py index 93d9157b..0bc0dc98 100644 --- a/fluid_build/providers/local/local.py +++ b/fluid_build/providers/local/local.py @@ -39,7 +39,12 @@ from typing import Any, Dict, List, Optional, Tuple, Union from fluid_build.observability.secret_redactor import redact_secret_text, redact_value -from fluid_build.providers._duckdb_compat import NO_ROWS_TO_WRITE, one_row, query_relation +from fluid_build.providers._duckdb_compat import ( + NO_ROWS_TO_WRITE, + one_row, + query_relation, + relation_view, +) from fluid_build.providers._duckdb_read import build_register_view_sql from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -1139,12 +1144,11 @@ def _copy_relation_to_uri(self, con: Any, rel: Any, uri: str, fmt: str) -> None: """ from fluid_build.build_runners.duckdb.runner import _build_copy_destination - view = "__fluid_embedded_result" - rel.create_view(view, replace=True) # ``replace`` on the first ``{select}``, not ``str.format``: the quoted # URI follows it in the template and may itself hold braces. template = _build_copy_destination(uri, fmt) - con.execute(template.replace("{select}", f"SELECT * FROM {view}", 1)) + with relation_view(con, rel, "__fluid_embedded_result") as view: + con.execute(template.replace("{select}", f"SELECT * FROM {view}", 1)) # ------------------ COPY / materialize (helper) --------------------- # diff --git a/scripts/verify_engines.sh b/scripts/verify_engines.sh index 0c0d9c65..f485293c 100755 --- a/scripts/verify_engines.sh +++ b/scripts/verify_engines.sh @@ -65,7 +65,7 @@ YAML ( cd "$WORK/duckdb" && python -m fluid_build.cli apply --build ingest contract.fluid.yaml >/repo/.verify-logs/duckdb.out 2>&1 ) state=$(run_state "$WORK/duckdb") [ -f "$WORK/duckdb/out/orders.parquet" ] \ - && rows=$(python -c "import duckdb;print(duckdb.sql(\"select count(*) from '$WORK/duckdb/out/orders.parquet'\").fetchone()[0])" 2>/dev/null) \ + && rows=$(python -c "import duckdb;print(duckdb.sql(\"select count(*) from '$WORK/duckdb/out/orders.parquet'\").fetchall()[0][0])" 2>/dev/null) \ && [ "$rows" = "5" ] \ && ok "5 rows landed in parquet (state=$state)" duckdb \ || miss "duckdb state=$state rows=${rows:-?} err=$(run_error "$WORK/duckdb")" duckdb diff --git a/tests/build_runners/test_duckdb_runner.py b/tests/build_runners/test_duckdb_runner.py index d96dc91a..17199b61 100644 --- a/tests/build_runners/test_duckdb_runner.py +++ b/tests/build_runners/test_duckdb_runner.py @@ -14,8 +14,9 @@ """DuckDB acquisition runner — unit + integration tests. -Filesystem-based tests run unconditionally; postgres tests require a live -Postgres (docker-compose / Testcontainers) and are gated by an env var. +Filesystem-based tests run unconditionally; the live postgres and mysql round +trips need a server and run only when their FLUID_TEST_* variables are set (no +CI job sets them). """ from __future__ import annotations @@ -212,6 +213,9 @@ def test_missing_uri_fails(self, tmp_path): # ── postgres, no server ─────────────────────────────────────────────────── +_UNSET_SECRET_REF = "env://FLUID_TEST_UNSET_SOURCE_SECRET" # pragma: allowlist secret +_REFUSED_SECRET_REF = "file://a/b" # pragma: allowlist secret + _CONNECT_FAILED = { "postgres": "Unable to connect to Postgres", "mysql": "Failed to connect to MySQL", @@ -303,10 +307,10 @@ def test_an_unreachable_database_source_fails_the_build_not_the_process( ({"port": "abc"}, "connection.port must be numeric"), # The run record's redactor reads "_SECRET: environment" as an # assignment and masks its value, so the record says "variable not set". - ({"secretRef": "env://FLUID_TEST_UNSET_SOURCE_SECRET"}, "variable not set"), + ({"secretRef": _UNSET_SECRET_REF}, "variable not set"), # The file backend refuses a name with a separator: a ConfigurationError, # as every secret backend but env:// raises, not a ValueError. - ({"secretRef": "file://a/b"}, "Invalid secret name"), + ({"secretRef": _REFUSED_SECRET_REF}, "Invalid secret name"), ], ids=["port", "unset-secret-ref", "secret-backend-error"], ) @@ -336,13 +340,116 @@ def test_a_source_the_runner_cannot_set_up_fails_the_build_with_a_run_record( assert stream["error"].startswith("source: ") and reason in stream["error"], stream +def test_a_postgres_enum_column_reads_as_text_as_postgres_scan_read_it(): + """``postgres_scan`` typed a Postgres enum ``VARCHAR``; the ``ATTACH``-ed + catalog types it ``ENUM(...)``, which the schema gate took for a type + change against a declared ``VARCHAR``. DuckDB's own enum stands in here + for the attached one: both describe as ``ENUM('a', 'b')``.""" + import types + + duckdb = pytest.importorskip("duckdb") + from fluid_build.build_runners.duckdb.runner import ( + _postgres_alias_for_build, + _postgres_enums_as_varchar, + _select_for_stream, + ) + + con = duckdb.connect() + # Under the alias the runner attaches the source as. + alias = _postgres_alias_for_build("b1") + con.execute(f"ATTACH ':memory:' AS {alias}") + con.execute(f"CREATE SCHEMA {alias}.app") + con.execute( + f"CREATE TABLE {alias}.app.src (id INTEGER, mood ENUM('sad', 'ok', 'happy'), " + "moods ENUM('sad', 'ok', 'happy')[], \"Odd \"\"Name\" ENUM('a)b', 'c'))" + ) + con.execute(f"INSERT INTO {alias}.app.src VALUES (1, 'ok', ['sad', 'happy'], 'a)b')") + ctx = types.SimpleNamespace( + build_id="b1", + source=types.SimpleNamespace(kind="postgres", connection=types.SimpleNamespace(raw={})), + ) + select_sql = _select_for_stream(con, ctx, "app.src") + assert [(row[0], row[1]) for row in con.execute(f"DESCRIBE {select_sql}").fetchall()] == [ + ("id", "INTEGER"), + ("mood", "VARCHAR"), + ("moods", "VARCHAR[]"), + ('Odd "Name', "VARCHAR"), + ] + assert con.execute(select_sql).fetchall() == [(1, "ok", ["sad", "happy"], "a)b")] + # Text order, not declaration order: 'ok' < 'sad' as text, not as this enum. + assert con.execute(f"SELECT mood < 'sad' FROM ({select_sql})").fetchall() == [(True,)] + no_enum = f"SELECT id FROM {alias}.app.src" + assert _postgres_enums_as_varchar(con, no_enum) == no_enum + + +def test_a_database_source_that_drifts_from_its_declared_schema_is_refused(tmp_path): + """The schema gate fingerprints the source on a connection of its own, + which must attach the source as the run's does: a database source exists + only through that ``ATTACH``. The gate swallows a fingerprint that fails, + so a fingerprint that cannot see the source would land drifted data.""" + import sqlite3 + + from fluid_build._errors import SchemaDriftError + + _require_extension("sqlite") + db = tmp_path / "src.sqlite" + source = sqlite3.connect(db) + source.execute("CREATE TABLE orders (id INTEGER, name TEXT)") + source.execute("INSERT INTO orders VALUES (1, 'a'), (2, 'b')") + source.commit() + source.close() + out = tmp_path / "orders.parquet" + contract = { + "fluidVersion": "0.7.3", + "id": "bronze.sqlite_orders", + "builds": [ + { + "id": "ingest_sqlite", + "pattern": "acquisition", + "engine": "duckdb", + "properties": { + "source": { + "kind": "sqlite", + "connection": {"path": str(db)}, + "mode": "full_refresh", + "streams": ["orders"], + }, + "sink": {"format": "parquet"}, + }, + "outputs": ["orders"], + } + ], + "exposes": [ + { + "exposeId": "orders", + "kind": "table", + "binding": { + "platform": "local", + "format": "parquet", + "location": {"path": str(out)}, + }, + "contract": { + "schemaPolicy": "strict", + "schema": [ + {"name": "id", "type": "integer"}, + {"name": "not_in_the_source", "type": "string"}, + ], + }, + } + ], + } + with pytest.raises(SchemaDriftError): + execute_duckdb_build(contract["builds"][0], contract, tmp_path, dry_run=False) + assert not out.exists() + + # ── postgres (docker-required) ──────────────────────────────────────────── @pytest.mark.integration @pytest.mark.skipif( os.environ.get("FLUID_TEST_POSTGRES_HOST") is None, - reason="Set FLUID_TEST_POSTGRES_HOST/PORT/USER/PASSWORD/DB to run; CI uses docker-compose.", + reason="Set FLUID_TEST_POSTGRES_HOST/PORT/USER/PASSWORD/DB to run (no CI job sets them).", ) class TestDuckdbPostgresLive: def test_postgres_round_trip(self, tmp_path): @@ -436,7 +543,7 @@ def test_postgres_round_trip(self, tmp_path): @pytest.mark.integration @pytest.mark.skipif( os.environ.get("FLUID_TEST_MYSQL_HOST") is None, - reason="Set FLUID_TEST_MYSQL_HOST/PORT/USER/PASSWORD/DB to run; CI uses docker-compose.", + reason="Set FLUID_TEST_MYSQL_HOST/PORT/USER/PASSWORD/DB to run (no CI job sets them).", ) class TestDuckdbMysqlLive: def test_mysql_scan_round_trip(self, tmp_path): diff --git a/tests/build_runners/test_duckdb_runner_mysql_attach.py b/tests/build_runners/test_duckdb_runner_mysql_attach.py index 564fbf9c..fe9b31c4 100644 --- a/tests/build_runners/test_duckdb_runner_mysql_attach.py +++ b/tests/build_runners/test_duckdb_runner_mysql_attach.py @@ -135,8 +135,8 @@ def test_an_empty_temporary_secret_is_named_in_the_attach(self): """Without it the extension looks for a default secret in the persistent storage the sandbox turned off: ``Unknown secret storage found: 'local_file'``, and no mysql source attached at all.""" - secret, sql = _executed(_BASE) - assert secret == "CREATE TEMPORARY SECRET mysql_b1 (TYPE mysql)" + store_sql, sql = _executed(_BASE) + assert store_sql == "CREATE TEMPORARY SECRET mysql_b1 (TYPE mysql)" assert sql.startswith("ATTACH '") @@ -222,10 +222,10 @@ class TestPostgresAttach: and sqlite sources, and attaches it read-only.""" def test_the_source_is_attached_read_only_under_a_per_build_alias(self): - secret, sql = _executed(dict(_BASE, port="5432"), kind="postgres") + store_sql, sql = _executed(dict(_BASE, port="5432"), kind="postgres") # An empty temporary secret: the sandbox's disabled persistent storage # otherwise fails the ATTACH's default-secret lookup. - assert secret == "CREATE TEMPORARY SECRET postgres_b1 (TYPE postgres)" + assert store_sql == "CREATE TEMPORARY SECRET postgres_b1 (TYPE postgres)" assert sql.startswith("ATTACH '") assert sql.endswith(" AS postgres_b1 (TYPE postgres, SECRET postgres_b1, READ_ONLY)") assert "dbname=app" in _dsn_of(sql) diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 81c10789..6ee529d6 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -36,6 +36,7 @@ first_row, one_row, query_relation, + relation_view, storage_compat_config, ) from fluid_build.providers._duckdb_sandbox import ( # noqa: E402 @@ -202,6 +203,72 @@ def test_query_relation_runs_each_statement_once(): assert first_row(con.execute("SELECT count(*) FROM t WHERE a = 8")) == (0,) +def test_query_relation_of_sql_that_does_not_parse_runs_none_of_it(): + """DuckDB 2.0's con.sql() runs the statements ahead of one that does not + parse before it raises; 1.5 parses the whole SQL first and runs none.""" + con = _table_t() + with pytest.raises(duckdb.ParserException): + query_relation(con, "INSERT INTO t VALUES (7); SELEC 1") + assert first_row(con.execute("SELECT count(*) FROM t WHERE a = 7")) == (0,) + + +@pytest.mark.parametrize("sql", ["", "-- nothing", "/* nothing */"]) +def test_query_relation_of_no_statement_is_none(sql): + assert query_relation(_table_t(), sql) is None + + +@pytest.mark.parametrize( + "sql, value", + [ + ("SELECT 'x' AS v FROM t; -- the result", "x"), + ("SELECT 'x' AS v FROM t; /* the result */", "x"), + ("SELECT 'x' AS v FROM t -- the result", "x"), + ("SELECT 'x' AS v FROM t;\n-- one\n-- two\n", "x"), + # A ';' or '--' inside a string is not the end, and the offsets + # DuckDB's tokenizer gives are bytes, which these characters are not. + ("SELECT '; --é€' AS v FROM t; -- ü", "; --é€"), + ], +) +def test_a_comment_after_the_last_statement_leaves_the_relation_usable(sql, value, tmp_path): + """DuckDB 2.0 reads the statement's text again inside the SQL of a + relation's aggregate, limit or write, where a comment after the closing + ';' fails to parse; 1.5 reads it fine.""" + rel = query_relation(_table_t(), sql) + assert one_row(rel.aggregate("count(*)")) == (3,) + assert rel.limit(1).fetchall() == [(value,)] + out = tmp_path / "out.csv" + rel.write_csv(str(out)) + assert out.read_text(encoding="utf-8").splitlines()[1] == value + + +@pytest.mark.parametrize( + "sql, rows", + [ + # DuckDB 2.0 runs these as the relation is made and holds their rows. + ("PIVOT t ON a USING count(*)", [(1, 1, 1)]), + ("SELECT nextval('s') AS n FROM t ORDER BY n", [(1,), (2,), (3,)]), + ("SELECT a FROM t ORDER BY a", [(0,), (1,), (2,)]), + ], + ids=["pivot", "nextval", "lazy"], +) +def test_relation_view_reads_rows_duckdb_already_holds(sql, rows, tmp_path): + """DuckDB 2.0 refuses a view over such a relation ("it reads rows held in + this process"); relation_view keeps the COPY the engine runs from it.""" + con = _table_t() + con.execute("CREATE SEQUENCE s") + rel = con.sql(sql) + out = tmp_path / "out.parquet" + with relation_view(con, rel, "__fluid_test_result") as name: + con.execute(f"COPY (SELECT * FROM {name}) TO '{out}' (FORMAT parquet)") + assert duckdb.connect().execute(f"SELECT * FROM '{out}'").fetchall() == rows + # The query ran once (the sequence moved by three, not six) and nothing is left. + if "nextval" in sql: + assert con.execute("SELECT currval('s')").fetchall() == [(3,)] + left = "SELECT count(*) FROM duckdb_{0}s() WHERE {0}_name = '__fluid_test_result'" + assert con.execute(left.format("view")).fetchall() == [(0,)] + assert con.execute(left.format("table")).fetchall() == [(0,)] + + # ── the engine, end to end ─────────────────────────────────────────────── @@ -272,6 +339,71 @@ def test_local_sql_that_returns_no_rows_fails_the_same_on_both_versions(product, assert not out.exists() +def test_local_sql_ending_in_a_comment_writes_its_result(product): + out = product / "out" / "orders.csv" + sql = "SELECT * FROM orders ORDER BY id; -- every order" + results = _local_apply(product, {"op": "sql", "sql": sql, "outputs": [str(out)]}) + assert results[1]["status"] == "ok", results + assert out.read_text(encoding="utf-8").splitlines() == ["id,amount", "1,10", "2,20"] + + +def test_local_sql_with_a_statement_that_does_not_parse_runs_none_of_it(product): + """On DuckDB 2.0 the statement ahead of the bad one ran, and the next + action in the same apply saw the table it made.""" + out = product / "out" + results = _local_apply( + product, + { + "op": "sql", + "sql": "CREATE TABLE made AS SELECT * FROM orders; SELEC 1", + "outputs": [str(out / "bad.csv")], + }, + { + "op": "sql", + "sql": "SELECT count(*) AS n FROM duckdb_tables() WHERE table_name = 'made'", + "outputs": [str(out / "made.csv")], + }, + ) + assert [r["status"] for r in results] == ["ok", "error", "ok"], results + assert (out / "made.csv").read_text(encoding="utf-8").splitlines() == ["n", "0"] + + +def _sales(con: Any) -> Any: + con.execute( + "CREATE TABLE sales AS SELECT * FROM (VALUES ('jan', 1), ('feb', 2), ('jan', 3)) " + "s(month, amount)" + ) + return con + + +def test_a_local_result_duckdb_already_holds_is_copied_to_a_remote_output(tmp_path): + """``_copy_relation_to_uri`` (an ``s3://`` output) made a view of the + result, which DuckDB 2.0 refuses for a PIVOT without an IN list. A local + path stands in for the bucket: the COPY is the same statement.""" + from fluid_build.providers.local.local import LocalProvider + + con = _sales(duckdb.connect()) + rel = con.sql("PIVOT sales ON month USING sum(amount)") + out = tmp_path / "pivot.parquet" + provider = LocalProvider(project="local", region="local", anchor_dir=tmp_path) + provider._copy_relation_to_uri(con, rel, str(out), "parquet") + assert duckdb.connect().execute(f"SELECT feb, jan FROM '{out}'").fetchall() == [(2, 4)] + + +def test_contract_tests_writes_a_result_duckdb_already_holds_to_parquet(tmp_path): + pytest.importorskip("pandas") + pytest.importorskip("pyarrow") + from fluid_build.contract_tests import _execute_sql, _write_output + + rel = _execute_sql(_sales(duckdb.connect()), "PIVOT sales ON month USING sum(amount)") + out = tmp_path / "pivot.parquet" + assert _write_output(rel, {"path": str(out), "format": "parquet"}, dry_run=False) == ( + 1, + str(out.resolve()), + ) + assert duckdb.connect().execute(f"SELECT feb, jan FROM '{out}'").fetchall() == [(2, 4)] + + def test_contract_tests_sql_that_returns_no_rows_fails_clearly(): from fluid_build.contract_tests import LocalProviderError, _execute_sql diff --git a/tests/test_ci_duckdb_floor.py b/tests/test_ci_duckdb_floor.py index 2cde2afc..aa049b72 100644 --- a/tests/test_ci_duckdb_floor.py +++ b/tests/test_ci_duckdb_floor.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + """The ``duckdb-floor`` CI job runs what differs between DuckDB versions. pyproject.toml takes ``duckdb>=1.5.0`` with no ceiling, and that job is the From bc3e2b414a0ac9b6dfa64c3283172f5f098d7d62 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 09:10:41 +0200 Subject: [PATCH 13/14] test(duckdb): the sandbox fence allows duckdb.tokenize; a fake connection parses The full suite on a2506ac4 found two test-side breaks of the third round: - test_every_duckdb_connection_goes_through_the_sandbox flags every call on the duckdb module, and _duckdb_compat now calls duckdb.tokenize to find a statement's trailing comments. tokenize takes no connection and runs no SQL (a nanobind function of _duckdb, unlike the default-connection wrappers such as duckdb.execute, which document a connection parameter). The fence exempts that one name; a test pins that tokenize documents no connection while execute does, and that a near name is still a bypass. - test_a_failed_sql_action_logs_its_error_redacted's fake connection had only sql(); on DuckDB 2.x query_relation parses with extract_statements first, so the fake raises its error there too. --- tests/providers/test_duckdb_sandbox.py | 27 +++++++++++++++++-- ...tials_stay_out_of_files_and_diagnostics.py | 4 +++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/tests/providers/test_duckdb_sandbox.py b/tests/providers/test_duckdb_sandbox.py index 11fc8121..6b75c904 100644 --- a/tests/providers/test_duckdb_sandbox.py +++ b/tests/providers/test_duckdb_sandbox.py @@ -1457,11 +1457,20 @@ def _duckdb_aliases(tree: ast.AST) -> tuple: return modules, members +#: Functions of the duckdb module that take no connection and read nothing but +#: their argument. ``tokenize`` splits a SQL string into token offsets and +#: types (``_duckdb_compat`` finds a statement's trailing comments with it); +#: unlike ``duckdb.sql`` and the other module-level functions, it has no +#: ``connection`` parameter and runs no SQL. +_CONNECTIONLESS = frozenset({"tokenize"}) + + def _bypasses(path: Path) -> List[str]: """Each place ``path`` opens or queries DuckDB without the helper. * any call on the duckdb module (``duckdb.connect``, ``duckdb.sql`` and the - other module-level functions run on an unsandboxed default connection); + other module-level functions run on an unsandboxed default connection), + except the :data:`_CONNECTIONLESS` ones; * a function imported from duckdb (``from duckdb import connect``); * ``.connect(...)``, which catches the module held in a variable (``self._duckdb.connect``, ``_Duck.get().connect``). @@ -1480,7 +1489,8 @@ def _bypasses(path: Path) -> List[str]: if isinstance(func, ast.Attribute): receiver = func.value if isinstance(receiver, ast.Name) and receiver.id in modules: - found.append(f"{where} duckdb.{func.attr}(...)") + if func.attr not in _CONNECTIONLESS: + found.append(f"{where} duckdb.{func.attr}(...)") elif func.attr == "connect" and "duck" in ast.unparse(receiver).lower(): found.append(f"{where} {ast.unparse(func)}(...)") elif isinstance(func, ast.Name) and func.id in members: @@ -1488,6 +1498,19 @@ def _bypasses(path: Path) -> List[str]: return found +def test_a_connectionless_duckdb_function_is_not_a_bypass(tmp_path): + """``duckdb.tokenize`` runs no SQL; the exemption is that name alone.""" + # A default-connection function documents its ``connection`` parameter; + # tokenize has none, on 1.5 and 2.x. + assert "connection" in (duckdb.execute.__doc__ or "") + assert "connection" not in (duckdb.tokenize.__doc__ or "") + sample = tmp_path / "probe.py" + sample.write_text("import duckdb\nduckdb.tokenize('SELECT 1 -- c')\n", encoding="utf-8") + assert _bypasses(sample) == [] + sample.write_text("import duckdb\nduckdb.tokenizer('SELECT 1')\n", encoding="utf-8") + assert _bypasses(sample) + + def test_the_bypass_detector_sees_the_helpers_own_connect(): """A guard that has never fired proves nothing: it must see the one real call.""" assert [b for b in _bypasses(_HELPER) if "duckdb.connect(" in b] diff --git a/tests/security/test_credentials_stay_out_of_files_and_diagnostics.py b/tests/security/test_credentials_stay_out_of_files_and_diagnostics.py index c1b4a913..4ab34afd 100644 --- a/tests/security/test_credentials_stay_out_of_files_and_diagnostics.py +++ b/tests/security/test_credentials_stay_out_of_files_and_diagnostics.py @@ -383,6 +383,10 @@ def __init__(self, message: str) -> None: def sql(self, _sql): raise RuntimeError(self.message) + def extract_statements(self, _sql): + # On DuckDB 2.x query_relation parses the SQL first (_duckdb_compat). + raise RuntimeError(self.message) + def test_no_local_provider_log_call_carries_a_raw_exception_text(): """Every ``"error": str(e)`` the local provider logs goes through the shared From b8701bb46e9ed960638e9da499e308cdc23740b0 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 22 Oct 2026 09:42:09 +0200 Subject: [PATCH 14/14] test(duckdb): two SQL cases concatenate their halves explicitly CodeQL's py/implicit-string-concatenation-in-list flagged two parametrize entries split over two lines (a MERGE and a CTAS-then-INSERT); a '+' makes each one string on purpose. --- tests/providers/test_duckdb_compat.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/providers/test_duckdb_compat.py b/tests/providers/test_duckdb_compat.py index 6ee529d6..a2cd6417 100644 --- a/tests/providers/test_duckdb_compat.py +++ b/tests/providers/test_duckdb_compat.py @@ -162,7 +162,7 @@ def test_query_relation_keeps_a_query(sql, columns): "UPDATE t SET a = a", "DELETE FROM t WHERE a = 100", "MERGE INTO t USING (SELECT 1 AS a) s ON t.a = s.a " - "WHEN NOT MATCHED THEN INSERT VALUES (s.a)", + + "WHEN NOT MATCHED THEN INSERT VALUES (s.a)", "DROP TABLE t", "SET threads = 2", "SELECT 1 AS x; CREATE TABLE made AS SELECT 1 AS q", @@ -323,7 +323,7 @@ def test_local_apply_counts_rows_and_materializes(product): "CREATE TABLE made AS SELECT * FROM orders", "CREATE TABLE made AS PIVOT orders ON id USING sum(amount)", "CREATE TABLE made AS SELECT * FROM orders; SELECT * FROM made; " - "INSERT INTO made VALUES (3, 30)", + + "INSERT INTO made VALUES (3, 30)", ], ids=["ctas", "pivot-ctas", "dml-after-a-query"], )