From 63453fa5cdf65954fc849d17ce9282241dd0b8d0 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 14:01:46 +0200 Subject: [PATCH 1/7] OpenSpec change: a connection URL that names no driver uses SLayer's default driver --- .../.openspec.yaml | 2 + .../design.md | 64 ++++++++++ .../proposal.md | 44 +++++++ .../specs/sql/execution/spec.md | 116 ++++++++++++++++++ .../tasks.md | 21 ++++ 5 files changed, 247 insertions(+) create mode 100644 openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml create mode 100644 openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md create mode 100644 openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md create mode 100644 openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md create mode 100644 openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml new file mode 100644 index 00000000..9076a660 --- /dev/null +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-10-20 diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md new file mode 100644 index 00000000..3079e044 --- /dev/null +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md @@ -0,0 +1,64 @@ +## Context + +See proposal.md — Why. Today two authorities pick the driver: SLayer's `DriverFacts` (`url_scheme`, +`sync_driver`) for URLs it builds, SQLAlchemy's per-version default for any URL naming no driver. +Every consumer — sync engine (`engine_factory._build_engine`), async engine +(`client._get_async_engine`), engine-cache keys, `_runtime_fingerprint`, the `MissingDriverError` +hint — reads `DatasourceConfig.get_connection_string()`. + +Principles applied: sql §3.2 (dialect quirks only in `dialects/`) — the default-driver fact and the +pin rule live on `DriverFacts`; system §3.1/§3.2 — `core.models` reaches it only through the +existing `core.models -> sql.dialects` `#legacy` arrow (no new arrow, baseline unchanged); sql §3.9 +and §3.14 unchanged. No arc42 / `.c4` / `index.yaml` edit: the delta attaches to the existing +`sql/execution` spec of node `sql`. + +## Goals / Non-Goals + +**Goals:** `DriverFacts` is the only authority on a driverless URL's driver; one pin step at the one +URL source; the bug class (SQLAlchemy changing a default) cannot recur for any dialect whose scheme +names a driver. + +**Non-Goals:** SQLAlchemy 2.1 compatibility (snowflake-sqlalchemy import, duckdb-engine foreign-key +reflection) and the latest-SQLAlchemy CI leg — DEV-2118, which also lifts the `<2.1` cap; `postgres://` +(an unregistered SQLAlchemy backend, not a default-driver question). + +## Decisions + +1. **One source for the default driver.** `sync_driver` is a read-only property — the `+driver` part + of `url_scheme`, `None` when the scheme names none — on both `SqlDialect` and `DriverFacts`. + `PostgresDialect.url_scheme = "postgresql+psycopg2"`; the `sync_driver` declarations on Postgres + and MySQL are deleted. ClickHouse (`clickhouse+http`) and SQL Server (`mssql+pyodbc`) gain a + default driver without any new declaration. +2. **Backends a type serves.** `SqlDialect.url_backend_aliases: frozenset[str] = frozenset()`, + carried on `DriverFacts`; `MysqlDialect` (inherited by `MariadbDialect`) declares `{"mariadb"}`. + `DriverFacts.url_backends` is a property: `{url_backend} | url_backend_aliases`, empty when + `url_scheme` is `None`. +3. **One pin step: `DriverFacts.pin_default_driver(url: str) -> str`.** Pins iff the URL parses + (`make_url`; `ArgumentError` → pass through), its backend is in `url_backends`, its drivername + has no `+`, and `sync_driver` is set: `url.set(drivername=f"{backend}+{sync_driver}")` with the + URL's own backend, rendered `hide_password=False`. Every other input is returned byte-identical + (no re-render), so explicit drivers, foreign backends, `:memory:` and unparseable strings are + untouched. +4. **Applied once.** `get_connection_string()` returns + `dialect.driver_facts(self.type).pin_default_driver()`; the current branching + body moves to a private helper. No consumer changes; stored `connection_string` is never rewritten. +5. **Async derivation.** `_async_connection_string` matches the backend against `url_backends` and + rebuilds with the URL's own backend (`mariadb+pymysql` → `mariadb+aiomysql`). Its "plain URL" leg + is removed — the pin means no driverless URL reaches it: driver == `sync_driver` → `async_driver`; + another sync driver → sync; async-capable driver → kept. Rewriting an explicitly named default + sync driver (`+psycopg2`, `+pymysql`) to its async counterpart is kept on purpose: after the pin a + bare URL and an explicit default-sync URL are the same URL, and the brief's "named drivers are + respected" rule governs which sync driver is chosen, not the sync→async pairing (Codex review, + rejected finding). +6. **`_is_default_driver`** matches the backend against `url_backends`; its `scheme_driver` branch + folds into `sync_driver`. +7. **Version cap** `sqlalchemy = ">=2.0,<2.1"`; the lock stays on 2.0.49. + +## Risks / Trade-offs + +- [The CI env cannot reproduce the 2.1 Postgres failure: pytest-postgresql pulls in psycopg 3, and + the cap keeps SQLAlchemy at 2.0] → the version-independent resolver test asserts that every pinned + URL names `sync_driver` explicitly and that `get_dialect().driver == sync_driver`; its `mysql://` / + `mariadb://` cases fail without the fix on 2.0.49. +- [Structured Postgres URLs change from `postgresql://` to `postgresql+psycopg2://`] → engine-cache + keys are in-process only; nothing persists the built URL. diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md new file mode 100644 index 00000000..c57ca74a --- /dev/null +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md @@ -0,0 +1,44 @@ +## Why + +A connection URL that names no driver (`postgresql://`, `mysql://`, `mariadb://`) is resolved by +SQLAlchemy's own per-version default driver, not the one SLayer's extra installs. SQLAlchemy 2.1 +switched bare `postgresql://` to psycopg 3, so every Postgres query on a fresh +`pip install 'motley-slayer[postgres]'` fails with a `MissingDriverError` whose hint points back at +the installed extra (GitHub #500); bare `mysql://` / `mariadb://` already resolve to `mysqldb`, +which `[mysql]` does not install, on 2.0. + +## What Changes + +- A URL whose drivername names no driver, on a backend the datasource type serves, is pinned to that + type's declared default driver — the `+driver` part of its URL scheme — before any engine, cache + key or error hint sees it. Explicitly named drivers and every other URL pass through unchanged. +- Each dialect declares its default driver once, in its URL scheme; Postgres's scheme becomes + `postgresql+psycopg2`, so structured Postgres datasources also name their driver. +- MySQL-family types serve both the `mysql` and `mariadb` backends: `mariadb://` pins to + `mariadb+pymysql` and runs asynchronously on `mariadb+aiomysql`. +- `sqlalchemy` is capped at `<2.1`: on 2.1, snowflake-sqlalchemy fails to import and duckdb-engine's + foreign-key reflection breaks. A follow-up lifts the cap and adds a latest-SQLAlchemy CI leg. +- Docs: a `connection_string` naming no driver uses the driver from SLayer's extra. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `sql/execution`: new requirement pinning driverless URLs to SLayer's default driver; the + structured-config URL requirement lists Postgres's new scheme; the asynchronous-path requirement + covers the `mariadb` backend and drops the now-unreachable no-driver case. + +## Impact + +- Code: `slayer/sql/dialects/base.py` (`DriverFacts`, `SqlDialect` driver facts), + `slayer/sql/dialects/{postgres,mysql}.py`, `slayer/core/models.py` + (`DatasourceConfig.get_connection_string`), `slayer/sql/client.py` (`_async_connection_string`), + `slayer/sql/dialects/drivers.py` (`_is_default_driver`). +- Dependencies: `pyproject.toml` / `poetry.lock` — `sqlalchemy >=2.0,<2.1`. +- Docs: `docs/configuration/datasources.md`. +- Behaviour: structured Postgres URLs and engine-cache keys now carry `+psycopg2`; stored + `connection_string` values are never rewritten. diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md new file mode 100644 index 00000000..85eeaeba --- /dev/null +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md @@ -0,0 +1,116 @@ +## ADDED Requirements + +### Requirement: A connection URL that names no driver uses SLayer's default driver +For a datasource type that declares a default driver (postgres/postgresql: psycopg2; mysql/mariadb: +pymysql; clickhouse: http; sqlserver/mssql/tsql: pyodbc), a connection URL — the datasource's +`connection_string`, or the URL built from its structured fields — whose backend is one the type +serves (postgres/postgresql: `postgresql`; mysql/mariadb: `mysql` and `mariadb`; clickhouse: +`clickhouse`; sqlserver/mssql/tsql: `mssql`) and which names no driver SHALL connect with the type's +default driver on the URL's own backend, whatever the installed SQLAlchemy's own default driver for +that backend is. Credentials, host, port, database and query parameters SHALL be preserved. + +A URL that names a driver, whose backend the type does not serve, or that cannot be parsed as a +URL, and every URL of a type without a declared default driver, SHALL be used exactly as given. A +stored `connection_string` SHALL NOT be rewritten. + +#### Scenario: Bare Postgres URL uses psycopg2 +- **WHEN** a postgres datasource's `connection_string` is + `postgresql://u:p%40ss@h:5432/d?sslmode=require` +- **THEN** it connects as `postgresql+psycopg2://u:p%40ss@h:5432/d?sslmode=require` + +#### Scenario: Bare MySQL and MariaDB URLs use pymysql +- **WHEN** a mysql datasource's `connection_string` is `mysql://u:p%40ss@h/d?charset=utf8mb4`, or a + mariadb datasource's is `mariadb://u@h/d` +- **THEN** they connect as `mysql+pymysql://u:p%40ss@h/d?charset=utf8mb4` and + `mariadb+pymysql://u@h/d` respectively + +#### Scenario: ClickHouse and SQL Server URLs use their default driver +- **WHEN** a clickhouse datasource's `connection_string` is `clickhouse://u@h/d`, or a sqlserver + datasource's is `mssql://u@h/d` +- **THEN** they connect as `clickhouse+http://u@h/d` and `mssql+pyodbc://u@h/d` respectively + +#### Scenario: A named driver is kept +- **WHEN** a postgres datasource's `connection_string` is `postgresql+psycopg://u@h/d` or + `postgresql+asyncpg://u@h/d`, or a mysql datasource's is `mysql+mysqldb://u@h/d` +- **THEN** the connection URL is exactly the `connection_string` + +#### Scenario: Types without a default driver are untouched +- **WHEN** a sqlite, duckdb, snowflake, trino, redshift or unregistered-type datasource connects, + including a sqlite `:memory:` datasource +- **THEN** its connection URL is exactly as given or as built today + +#### Scenario: A foreign backend is untouched +- **WHEN** a postgres datasource's `connection_string` is `mysql://u@h/d` +- **THEN** the connection URL is exactly the `connection_string` + +#### Scenario: Resolution does not depend on SQLAlchemy's defaults +- **WHEN** a datasource of any type that declares a default driver connects with a URL naming no + driver, or with structured fields +- **THEN** SQLAlchemy resolves that URL to the type's declared default driver + +#### Scenario: Missing default driver names the extra +- **WHEN** a postgres datasource's `connection_string` is `postgresql://u@h/d` and `psycopg2` is not + importable +- **THEN** a missing-driver error naming `psycopg2` and `pip install 'motley-slayer[postgres]'` is + raised + +#### Scenario: Stored configuration is not rewritten +- **WHEN** a datasource with `connection_string` `postgresql://u@h/d` is used and then read back +- **THEN** its `connection_string` is still `postgresql://u@h/d` + +## MODIFIED Requirements + +### Requirement: The asynchronous path honours the user's driver choice +For a datasource type with a native asynchronous driver (postgres/postgresql: asyncpg; +mysql/mariadb: aiomysql), the asynchronous connection SHALL be derived from the datasource's +connection URL — after a URL naming no driver has been given the type's default driver — as follows, +preserving the backend and every other URL part (credentials, host, port, database, query): +- a URL that names an asynchronous-capable driver SHALL be used unchanged; +- a URL that names the type's default synchronous driver (postgres: psycopg2; mysql/mariadb: + pymysql) SHALL use the type's native asynchronous driver; +- a URL that names any other synchronous-only driver, or whose backend is not one the type serves, + SHALL run on the synchronous path in a worker thread. + +A datasource type without a native asynchronous driver SHALL run on the synchronous path in a +worker thread. + +#### Scenario: psycopg v3 is kept +- **WHEN** a postgres datasource's `connection_string` is `postgresql+psycopg://u:p@h:5432/d` +- **THEN** the asynchronous connection uses psycopg with the same URL, and asyncpg is not required + +#### Scenario: Plain and default-sync URLs move to asyncpg +- **WHEN** a postgres datasource's URL is `postgresql://u:p@h:5432/d?sslmode=require` or + `postgresql+psycopg2://u:p@h:5432/d?sslmode=require` +- **THEN** the asynchronous connection URL is `postgresql+asyncpg://u:p@h:5432/d?sslmode=require` + +#### Scenario: MySQL follows the same rule +- **WHEN** a mysql datasource's URL is `mysql://…` or `mysql+pymysql://…`, or is + `mysql+asyncmy://…` +- **THEN** the first two use aiomysql and the third is used unchanged + +#### Scenario: MariaDB keeps its backend +- **WHEN** a mariadb datasource's URL is `mariadb://u@h/d` or `mariadb+pymysql://u@h/d` +- **THEN** the asynchronous connection URL is `mariadb+aiomysql://u@h/d` + +#### Scenario: Another named sync driver stays synchronous +- **WHEN** a postgres datasource's URL is `postgresql+pg8000://…`, or a mysql datasource's URL is + `mysql+mysqldb://…` +- **THEN** statements run on the synchronous path in a worker thread with the named driver + +#### Scenario: Type without an async driver +- **WHEN** a Snowflake datasource executes asynchronously +- **THEN** statements run on the synchronous path in a worker thread + +### Requirement: Structured-config connection URLs are stable +A datasource configured by fields rather than a `connection_string` SHALL produce a connection URL +whose scheme names the type's default driver where the type declares one; a datasource type that no +registered dialect declares SHALL keep `://` as its URL scheme. + +#### Scenario: Known types keep their scheme +- **WHEN** structured postgres, mysql, mariadb, clickhouse and sqlserver datasources build their URLs +- **THEN** the schemes are `postgresql+psycopg2`, `mysql+pymysql`, `mysql+pymysql`, + `clickhouse+http` and `mssql+pyodbc` + +#### Scenario: Unknown type keeps its own scheme +- **WHEN** a structured datasource of an unregistered type `foo` builds its URL +- **THEN** the URL scheme is `foo`, not the Postgres fallback dialect's scheme diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md new file mode 100644 index 00000000..af762378 --- /dev/null +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md @@ -0,0 +1,21 @@ +## 1. Tests (pr-tests stage; all fail before implementation) + +- [ ] 1.1 Pin-step tests for every scenario of "A connection URL that names no driver uses SLayer's default driver": bare postgres / mysql / mariadb / clickhouse / mssql URLs pinned (reserved-char credentials + query params preserved); named drivers (`postgresql+psycopg`, `postgresql+asyncpg`, `mysql+mysqldb`), foreign backend, sqlite / duckdb / `:memory:` / snowflake / trino / redshift / unregistered `foo` returned byte-identical; stored `connection_string` unchanged after use; verify they fail on the current code +- [ ] 1.2 Version-independent resolver test: for every registered dialect whose scheme names a driver, `make_url(get_connection_string()).get_dialect().driver == sync_driver` for structured fields AND a bare user URL. Gotcha: CI cannot reproduce the 2.1 Postgres failure (pytest-postgresql installs psycopg 3; SQLAlchemy is capped at 2.0), so this test plus 1.1's explicit `+driver` assertions are the guard; verify the `mysql://` / `mariadb://` cases fail now +- [ ] 1.3 Driver-facts tests: `sync_driver` derived from `url_scheme` (None for driverless schemes); `url_backends` = `{url_backend} | aliases`, `{"mysql","mariadb"}` for mysql/mariadb, empty for an unregistered type's facts with no scheme; Postgres scheme is `postgresql+psycopg2` +- [ ] 1.4 Async tests: bare postgres / mysql URLs → `+asyncpg` / `+aiomysql`; `mariadb://` and `mariadb+pymysql://` → `mariadb+aiomysql://`; missing `psycopg2` on a bare `postgresql://` raises a missing-driver error naming the `[postgres]` extra +- [ ] 1.5 Update existing tests that assert bare/old URLs to the pinned form (behaviour change per DEV-2116): `tests/test_models.py:1068,1075`, `tests/test_dialect_driver_facts.py:86,99-100` (`test_registry_is_the_one_driver_table` stops setting `sync_driver` in `model_copy`), `tests/test_engine_factory.py:90`, `tests/test_async_driver_selection.py:33`; structured sqlserver scheme `mssql+pyodbc` in the "Known types keep their scheme" test + +## 2. Implementation + +- [ ] 2.1 `slayer/sql/dialects/base.py`: `sync_driver` becomes a property on `SqlDialect` and `DriverFacts`; add `url_backend_aliases` field + `DriverFacts.url_backends` property; add `DriverFacts.pin_default_driver`; verify 1.3 passes +- [ ] 2.2 `postgres.py`: `url_scheme = "postgresql+psycopg2"`, drop `sync_driver`; `mysql.py`: drop `sync_driver`, declare `url_backend_aliases = {"mariadb"}`; verify 1.3 passes +- [ ] 2.3 `slayer/core/models.py`: `get_connection_string()` pins its result via the private-helper split; verify 1.1, 1.2 pass +- [ ] 2.4 `slayer/sql/client.py` `_async_connection_string`: `url_backends` match, URL's own backend, drop the plain-URL leg; verify 1.4 passes +- [ ] 2.5 `slayer/sql/dialects/drivers.py` `_is_default_driver`: `url_backends` match, fold `scheme_driver` into `sync_driver`; verify the missing-driver suite passes +- [ ] 2.6 `pyproject.toml`: `sqlalchemy = ">=2.0,<2.1"`; `poetry lock` (no upgrades — lock stays on 2.0.49); verify `poetry check --lock` + +## 3. Docs and gates + +- [ ] 3.1 One sentence in `docs/configuration/datasources.md`: a `connection_string` naming no driver (`postgresql://`, `mysql://`) uses the driver from SLayer's extra; name one explicitly to use another +- [ ] 3.2 Full unit suite (`poetry run pytest -m "not integration" -n logical`), `ruff check slayer/ tests/`, `basedpyright` (no new errors vs baseline), `la-arch-check` all green From 364e436aa0855a33e1cf7ec8a35fff7aa981ad36 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 16:11:06 +0200 Subject: [PATCH 2/7] Failing tests: a connection URL that names no driver uses SLayer's default driver --- .../tasks.md | 8 +- tests/test_async_driver_selection.py | 17 +- tests/test_datasource_snowflake.py | 2 +- tests/test_default_driver_pin.py | 234 ++++++++++++++++++ tests/test_dialect_driver_facts.py | 47 +++- tests/test_engine_factory.py | 2 +- tests/test_models.py | 2 +- 7 files changed, 292 insertions(+), 20 deletions(-) create mode 100644 tests/test_default_driver_pin.py diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md index af762378..b669f4e3 100644 --- a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md @@ -1,9 +1,9 @@ ## 1. Tests (pr-tests stage; all fail before implementation) -- [ ] 1.1 Pin-step tests for every scenario of "A connection URL that names no driver uses SLayer's default driver": bare postgres / mysql / mariadb / clickhouse / mssql URLs pinned (reserved-char credentials + query params preserved); named drivers (`postgresql+psycopg`, `postgresql+asyncpg`, `mysql+mysqldb`), foreign backend, sqlite / duckdb / `:memory:` / snowflake / trino / redshift / unregistered `foo` returned byte-identical; stored `connection_string` unchanged after use; verify they fail on the current code -- [ ] 1.2 Version-independent resolver test: for every registered dialect whose scheme names a driver, `make_url(get_connection_string()).get_dialect().driver == sync_driver` for structured fields AND a bare user URL. Gotcha: CI cannot reproduce the 2.1 Postgres failure (pytest-postgresql installs psycopg 3; SQLAlchemy is capped at 2.0), so this test plus 1.1's explicit `+driver` assertions are the guard; verify the `mysql://` / `mariadb://` cases fail now -- [ ] 1.3 Driver-facts tests: `sync_driver` derived from `url_scheme` (None for driverless schemes); `url_backends` = `{url_backend} | aliases`, `{"mysql","mariadb"}` for mysql/mariadb, empty for an unregistered type's facts with no scheme; Postgres scheme is `postgresql+psycopg2` -- [ ] 1.4 Async tests: bare postgres / mysql URLs → `+asyncpg` / `+aiomysql`; `mariadb://` and `mariadb+pymysql://` → `mariadb+aiomysql://`; missing `psycopg2` on a bare `postgresql://` raises a missing-driver error naming the `[postgres]` extra +- [x] 1.1 Pin-step tests for every scenario of "A connection URL that names no driver uses SLayer's default driver": bare postgres / mysql / mariadb / clickhouse / mssql URLs pinned (reserved-char credentials + query params preserved); named drivers (`postgresql+psycopg`, `postgresql+asyncpg`, `mysql+mysqldb`), foreign backend, sqlite / duckdb / `:memory:` / snowflake / trino / redshift / unregistered `foo` returned byte-identical; stored `connection_string` unchanged after use; verify they fail on the current code +- [x] 1.2 Version-independent resolver test: for every registered dialect whose scheme names a driver, `make_url(get_connection_string()).get_dialect().driver == sync_driver` for structured fields AND a bare user URL. Gotcha: CI cannot reproduce the 2.1 Postgres failure (pytest-postgresql installs psycopg 3; SQLAlchemy is capped at 2.0), so this test plus 1.1's explicit `+driver` assertions are the guard; verify the `mysql://` / `mariadb://` cases fail now +- [x] 1.3 Driver-facts tests: `sync_driver` derived from `url_scheme` (None for driverless schemes); `url_backends` = `{url_backend} | aliases`, `{"mysql","mariadb"}` for mysql/mariadb, empty for an unregistered type's facts with no scheme; Postgres scheme is `postgresql+psycopg2` +- [x] 1.4 Async tests: bare postgres / mysql URLs → `+asyncpg` / `+aiomysql`; `mariadb://` and `mariadb+pymysql://` → `mariadb+aiomysql://`; missing `psycopg2` on a bare `postgresql://` raises a missing-driver error naming the `[postgres]` extra - [ ] 1.5 Update existing tests that assert bare/old URLs to the pinned form (behaviour change per DEV-2116): `tests/test_models.py:1068,1075`, `tests/test_dialect_driver_facts.py:86,99-100` (`test_registry_is_the_one_driver_table` stops setting `sync_driver` in `model_copy`), `tests/test_engine_factory.py:90`, `tests/test_async_driver_selection.py:33`; structured sqlserver scheme `mssql+pyodbc` in the "Known types keep their scheme" test ## 2. Implementation diff --git a/tests/test_async_driver_selection.py b/tests/test_async_driver_selection.py index b0d8ad17..591c73cd 100644 --- a/tests/test_async_driver_selection.py +++ b/tests/test_async_driver_selection.py @@ -5,11 +5,18 @@ import pytest from sqlalchemy.engine.url import make_url +from slayer.core.models import DatasourceConfig from slayer.sql.client import _async_connection_string _URL_TAIL = "u:p%40ss@h:5432/d?sslmode=require" +def _async_url(db_type: str, connection_string: str) -> str | None: + """The async URL a datasource with ``connection_string`` connects with.""" + ds = DatasourceConfig(name="d", type=db_type, connection_string=connection_string) + return _async_connection_string(connection_string=ds.get_connection_string(), db_type=db_type) + + @pytest.mark.parametrize( "db_type,connection_string", [ @@ -35,12 +42,13 @@ def test_async_capable_driver_kept_unchanged(db_type, connection_string) -> None ("mysql", f"mysql://{_URL_TAIL}", f"mysql+aiomysql://{_URL_TAIL}"), ("mysql", f"mysql+pymysql://{_URL_TAIL}", f"mysql+aiomysql://{_URL_TAIL}"), ("mariadb", f"mysql+pymysql://{_URL_TAIL}", f"mysql+aiomysql://{_URL_TAIL}"), + ("mariadb", "mariadb://u@h/d", "mariadb+aiomysql://u@h/d"), + ("mariadb", "mariadb+pymysql://u@h/d", "mariadb+aiomysql://u@h/d"), + ("mysql", "mariadb://u@h/d", "mariadb+aiomysql://u@h/d"), ], ) def test_plain_or_default_sync_url_moves_to_async_driver(db_type, connection_string, expected) -> None: - assert _async_connection_string( - connection_string=connection_string, db_type=db_type, - ) == expected + assert _async_url(db_type, connection_string) == expected def test_swap_preserves_every_other_url_part() -> None: @@ -58,10 +66,11 @@ def test_swap_preserves_every_other_url_part() -> None: ("postgres", f"redshift+redshift_connector://{_URL_TAIL}"), ("snowflake", "snowflake://u:p@acct/db"), ("foo", f"postgresql://{_URL_TAIL}"), + ("postgres", f"mysql://{_URL_TAIL}"), ], ) def test_no_async_url(db_type, connection_string) -> None: - assert _async_connection_string(connection_string=connection_string, db_type=db_type) is None + assert _async_url(db_type, connection_string) is None @pytest.mark.parametrize( diff --git a/tests/test_datasource_snowflake.py b/tests/test_datasource_snowflake.py index fe178de0..431e6419 100644 --- a/tests/test_datasource_snowflake.py +++ b/tests/test_datasource_snowflake.py @@ -76,7 +76,7 @@ def test_non_snowflake_type_ignores_new_fields(self) -> None: role="should_be_ignored", ) cs = ds.get_connection_string() - assert cs.startswith("postgresql://") + assert cs.startswith("postgresql+psycopg2://") assert "should_be_ignored" not in cs diff --git a/tests/test_default_driver_pin.py b/tests/test_default_driver_pin.py new file mode 100644 index 00000000..689e34d8 --- /dev/null +++ b/tests/test_default_driver_pin.py @@ -0,0 +1,234 @@ +"""A connection URL that names no driver connects with SLayer's default driver, not SQLAlchemy's.""" + +from __future__ import annotations + +import sys +from collections import OrderedDict + +import pytest +import sqlalchemy.dialects.postgresql.base as pg_base +from sqlalchemy.dialects.postgresql.psycopg import PGDialect_psycopg +from sqlalchemy.engine.url import make_url + +from slayer.core.errors import MissingDriverError +from slayer.core.models import DatasourceConfig +from slayer.sql import dialects, engine_factory +from slayer.sql.client import SlayerSQLClient +from slayer.sql.dialects import dialect_for_ds_type +from slayer.sql.dialects.base import DriverFacts + +_HOST = "default-driver.invalid" + +# ds type → (default driver, a backend it serves) +_DEFAULT_DRIVER = { + "postgres": ("psycopg2", "postgresql"), + "postgresql": ("psycopg2", "postgresql"), + "mysql": ("pymysql", "mysql"), + "mariadb": ("pymysql", "mariadb"), + "clickhouse": ("http", "clickhouse"), + "sqlserver": ("pyodbc", "mssql"), + "mssql": ("pyodbc", "mssql"), + "tsql": ("pyodbc", "mssql"), +} + + +@pytest.fixture(autouse=True) +def _isolated_engine_cache(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(engine_factory, "_engine_cache", OrderedDict()) + + +@pytest.fixture +def sa21_postgres_default(monkeypatch: pytest.MonkeyPatch) -> None: + """SQLAlchemy 2.1's default: bare ``postgresql://`` resolves to psycopg 3.""" + monkeypatch.setattr(pg_base, "dialect", PGDialect_psycopg) + assert make_url("postgresql://").get_dialect().driver == "psycopg" + + +def _url(ds_type: str | None, connection_string: str) -> str: + return DatasourceConfig(name="d", type=ds_type, connection_string=connection_string).get_connection_string() + + +# --------------------------------------------------------------------------- +# Pinned +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "ds_type,connection_string,expected", + [ + ("postgres", "postgresql://u:p%40ss@h:5432/d?sslmode=require", + "postgresql+psycopg2://u:p%40ss@h:5432/d?sslmode=require"), + ("postgresql", "postgresql://u:p%40ss@h:5432/d?sslmode=require", + "postgresql+psycopg2://u:p%40ss@h:5432/d?sslmode=require"), + ("mysql", "mysql://u:p%40ss@h/d?charset=utf8mb4", "mysql+pymysql://u:p%40ss@h/d?charset=utf8mb4"), + ("mariadb", "mariadb://u@h/d", "mariadb+pymysql://u@h/d"), + ("mysql", "mariadb://u@h/d", "mariadb+pymysql://u@h/d"), + ("mariadb", "mysql://u@h/d", "mysql+pymysql://u@h/d"), + ("clickhouse", "clickhouse://u@h/d", "clickhouse+http://u@h/d"), + ("sqlserver", "mssql://u@h/d", "mssql+pyodbc://u@h/d"), + ("mssql", "mssql://u@h/d", "mssql+pyodbc://u@h/d"), + ("tsql", "mssql://u@h/d", "mssql+pyodbc://u@h/d"), + ], +) +def test_bare_url_pinned_to_default_driver(ds_type, connection_string, expected) -> None: + assert _url(ds_type, connection_string) == expected + + +def test_pin_preserves_every_url_part() -> None: + cs = "postgresql://us%3Aer:p%40ss%2Fw@db.example:6543/warehouse?sslmode=require&application_name=x" + out = make_url(_url("postgres", cs)) + assert out == make_url(cs).set(drivername="postgresql+psycopg2") + assert out.password == "p@ss/w" + + +# --------------------------------------------------------------------------- +# Used exactly as given +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "ds_type,connection_string", + [ + ("postgres", "postgresql+psycopg://u@h/d?b=2&a=1"), + ("postgres", "postgresql+asyncpg://u@h/d?b=2&a=1"), + ("postgres", "postgresql+psycopg2://u@h/d?b=2&a=1"), + ("mysql", "mysql+mysqldb://u@h/d?b=2&a=1"), + ("mariadb", "mariadb+mysqldb://u@h/d?b=2&a=1"), + ], +) +def test_named_driver_kept(ds_type, connection_string) -> None: + assert _url(ds_type, connection_string) == connection_string + + +@pytest.mark.parametrize( + "ds_type,connection_string", + [ + ("sqlite", "sqlite:///tmp/x.db"), + ("sqlite", "sqlite:///:memory:"), + ("sqlite", ":memory:"), + ("duckdb", "duckdb:///tmp/x.duckdb"), + ("snowflake", "snowflake://u:p@acct/db?warehouse=w"), + ("trino", "trino://u@h:8080/catalog"), + ("redshift", "postgresql://u@h:5439/d?b=2&a=1"), + ("foo", "postgresql://u@h/d?b=2&a=1"), + (None, "postgresql://u@h/d?b=2&a=1"), + ], +) +def test_type_without_default_driver_untouched(ds_type, connection_string) -> None: + assert _url(ds_type, connection_string) == connection_string + + +def test_in_memory_sqlite_structured_untouched() -> None: + ds = DatasourceConfig(name="d", type="sqlite", database=":memory:") + assert ds.get_connection_string() == "sqlite:///:memory:" + + +@pytest.mark.parametrize( + "ds_type,connection_string", + [ + ("postgres", "mysql://u@h/d?b=2&a=1"), + ("mysql", "postgresql://u@h/d?b=2&a=1"), + ("clickhouse", "mysql://u@h/d"), + ("postgres", "not a url"), + ("mysql", "garbage::/"), + ], +) +def test_foreign_backend_or_unparseable_untouched(ds_type, connection_string) -> None: + assert _url(ds_type, connection_string) == connection_string + + +def test_stored_connection_string_not_rewritten() -> None: + ds = DatasourceConfig(name="d", type="postgres", connection_string=f"postgresql://u@{_HOST}/d") + engine = engine_factory.get_engine(ds) + try: + assert engine.url.drivername == "postgresql+psycopg2" + finally: + engine.dispose() + assert ds.get_connection_string() == f"postgresql+psycopg2://u@{_HOST}/d" + assert ds.connection_string == f"postgresql://u@{_HOST}/d" + assert ds.model_dump()["connection_string"] == f"postgresql://u@{_HOST}/d" + + +# --------------------------------------------------------------------------- +# Resolution does not depend on SQLAlchemy's defaults +# --------------------------------------------------------------------------- + + +def test_default_driver_types_match_registry() -> None: + declared = {t for t in dialects._BY_DS_TYPE if dialect_for_ds_type(t).driver_facts(t).sync_driver} + assert declared == set(_DEFAULT_DRIVER) + + +@pytest.mark.parametrize("simulate_sa21", [False, True], ids=["installed", "sa21-postgres-default"]) +@pytest.mark.parametrize("ds_type", sorted(_DEFAULT_DRIVER)) +def test_sqlalchemy_resolves_declared_default_driver(request, ds_type, simulate_sa21) -> None: + if simulate_sa21: + request.getfixturevalue("sa21_postgres_default") + driver, backend = _DEFAULT_DRIVER[ds_type] + structured = DatasourceConfig(name="d", type=ds_type, host="h", database="db", username="u", password="p") + for url in (structured.get_connection_string(), _url(ds_type, f"{backend}://u@h/d")): + parsed = make_url(url) + assert parsed.get_dialect().driver == driver + assert parsed.drivername.partition("+")[2] == driver + + +# --------------------------------------------------------------------------- +# Engines see the pinned URL +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("sa21_postgres_default") +def test_sync_engine_uses_psycopg2_whatever_sqlalchemys_default() -> None: + ds = DatasourceConfig(name="d", type="postgres", connection_string=f"postgresql://u@{_HOST}/d") + engine = engine_factory.get_engine(ds) + try: + assert engine.url.drivername == "postgresql+psycopg2" + assert engine.dialect.driver == "psycopg2" + finally: + engine.dispose() + + +@pytest.mark.usefixtures("sa21_postgres_default") +async def test_async_engine_uses_asyncpg_whatever_sqlalchemys_default() -> None: + ds = DatasourceConfig(name="d", type="postgres", connection_string=f"postgresql://u@{_HOST}/d") + client = SlayerSQLClient(ds) + try: + engine = client._get_async_engine() + assert engine is not None + assert engine.url.drivername == "postgresql+asyncpg" + finally: + await client.aclose() + + +@pytest.mark.usefixtures("sa21_postgres_default") +def test_missing_default_driver_names_extra(monkeypatch) -> None: + ds = DatasourceConfig(name="pg_ds", type="postgres", connection_string=f"postgresql://u@{_HOST}/d") + monkeypatch.setitem(sys.modules, "psycopg2", None) + with pytest.raises(MissingDriverError) as info: + engine_factory.get_engine(ds) + assert info.value.missing == "psycopg2" + assert "pip install 'motley-slayer[postgres]'" in str(info.value) + + +# --------------------------------------------------------------------------- +# The pin step itself +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "facts,url,expected", + [ + (DriverFacts(url_scheme="postgresql+psycopg2"), "postgresql://u@h/d", "postgresql+psycopg2://u@h/d"), + (DriverFacts(url_scheme="postgresql+psycopg2"), "postgresql+pg8000://u@h/d", "postgresql+pg8000://u@h/d"), + (DriverFacts(url_scheme="postgresql"), "postgresql://u@h/d", "postgresql://u@h/d"), + (DriverFacts(url_scheme="foo"), "foo://u@h/d", "foo://u@h/d"), + (DriverFacts(), "postgresql://u@h/d", "postgresql://u@h/d"), + ( + DriverFacts(url_scheme="mysql+pymysql", url_backend_aliases=frozenset({"mariadb"})), + "mariadb://u@h/d", "mariadb+pymysql://u@h/d", + ), + (DriverFacts(url_scheme="mysql+pymysql"), "mariadb://u@h/d", "mariadb://u@h/d"), + ], +) +def test_pin_default_driver(facts, url, expected) -> None: + assert facts.pin_default_driver(url) == expected diff --git a/tests/test_dialect_driver_facts.py b/tests/test_dialect_driver_facts.py index d656eabf..40974b04 100644 --- a/tests/test_dialect_driver_facts.py +++ b/tests/test_dialect_driver_facts.py @@ -18,14 +18,14 @@ # ds type → (url_scheme, sync_driver, async_driver, install_extra) _EXPECTED = { - "postgres": ("postgresql", "psycopg2", "asyncpg", "postgres"), - "postgresql": ("postgresql", "psycopg2", "asyncpg", "postgres"), + "postgres": ("postgresql+psycopg2", "psycopg2", "asyncpg", "postgres"), + "postgresql": ("postgresql+psycopg2", "psycopg2", "asyncpg", "postgres"), "mysql": ("mysql+pymysql", "pymysql", "aiomysql", "mysql"), "mariadb": ("mysql+pymysql", "pymysql", "aiomysql", "mysql"), - "clickhouse": ("clickhouse+http", None, None, "clickhouse"), - "mssql": ("mssql+pyodbc", None, None, "sqlserver"), - "sqlserver": ("mssql+pyodbc", None, None, "sqlserver"), - "tsql": ("mssql+pyodbc", None, None, "sqlserver"), + "clickhouse": ("clickhouse+http", "http", None, "clickhouse"), + "mssql": ("mssql+pyodbc", "pyodbc", None, "sqlserver"), + "sqlserver": ("mssql+pyodbc", "pyodbc", None, "sqlserver"), + "tsql": ("mssql+pyodbc", "pyodbc", None, "sqlserver"), "snowflake": ("snowflake", None, None, "snowflake"), "bigquery": ("bigquery", None, None, "bigquery"), "sqlite": ("sqlite", None, None, None), @@ -56,6 +56,33 @@ def test_dialect_declares_driver_facts(ds_type) -> None: def test_unregistered_type_gets_no_fallback_driver_facts() -> None: facts = dialect_for_ds_type("foo").driver_facts("foo") assert facts == DriverFacts(url_scheme="foo") + assert facts.sync_driver is None + + +@pytest.mark.parametrize( + "url_scheme,sync_driver", + [("postgresql+psycopg2", "psycopg2"), ("mssql+pyodbc", "pyodbc"), ("sqlite", None), (None, None)], +) +def test_sync_driver_is_the_scheme_driver(url_scheme, sync_driver) -> None: + assert DriverFacts(url_scheme=url_scheme).sync_driver == sync_driver + + +@pytest.mark.parametrize( + "ds_type,backends", + [ + ("postgres", {"postgresql"}), + ("mysql", {"mysql", "mariadb"}), + ("mariadb", {"mysql", "mariadb"}), + ("sqlserver", {"mssql"}), + ("foo", {"foo"}), + ], +) +def test_url_backends(ds_type, backends) -> None: + assert dialect_for_ds_type(ds_type).driver_facts(ds_type).url_backends == backends + + +def test_no_scheme_serves_no_backend() -> None: + assert DriverFacts(url_backend_aliases=frozenset({"mariadb"})).url_backends == frozenset() def test_install_extras_exist_in_pyproject() -> None: @@ -83,8 +110,10 @@ def test_registry_is_the_one_driver_table(monkeypatch) -> None: monkeypatch.setitem(dialects._BY_DS_TYPE, "postgres", swapped) ds = DatasourceConfig(name="d", type="postgres", **_STRUCTURED) assert ds.get_connection_string().startswith("postgresql+psycopg://") + bare = DatasourceConfig(name="d", type="postgres", connection_string="postgresql://h/d") + assert bare.get_connection_string() == "postgresql+psycopg://h/d" assert client._async_connection_string( - connection_string="postgresql://h/d", db_type="postgres", + connection_string=bare.get_connection_string(), db_type="postgres", ) == "postgresql+psycopg://h/d" @@ -96,8 +125,8 @@ def test_registry_is_the_one_driver_table(monkeypatch) -> None: @pytest.mark.parametrize( "ds_type,expected", [ - ("postgres", "postgresql://u:p%40ss@h.example:1234/db"), - ("postgresql", "postgresql://u:p%40ss@h.example:1234/db"), + ("postgres", "postgresql+psycopg2://u:p%40ss@h.example:1234/db"), + ("postgresql", "postgresql+psycopg2://u:p%40ss@h.example:1234/db"), ("mysql", "mysql+pymysql://u:p%40ss@h.example:1234/db"), ("mariadb", "mysql+pymysql://u:p%40ss@h.example:1234/db"), ("clickhouse", "clickhouse+http://u:p%40ss@h.example:1234/db"), diff --git a/tests/test_engine_factory.py b/tests/test_engine_factory.py index dde3441a..24c12da9 100644 --- a/tests/test_engine_factory.py +++ b/tests/test_engine_factory.py @@ -87,7 +87,7 @@ def test_dialect_build_engine_none_falls_through_to_create_engine(self) -> None: result = engine_factory.get_engine(ds) assert result is fake args, kwargs = create_engine_mock.call_args - assert args[0].startswith("postgresql://") + assert args[0].startswith("postgresql+psycopg2://") assert kwargs.get("pool_pre_ping") is True diff --git a/tests/test_models.py b/tests/test_models.py index d186c837..c7de59bd 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1065,7 +1065,7 @@ def test_postgres_connection_string(self) -> None: password="pass", ) cs = ds.get_connection_string() - assert cs == "postgresql://user:pass@localhost:5432/mydb" + assert cs == "postgresql+psycopg2://user:pass@localhost:5432/mydb" def test_explicit_connection_string(self) -> None: ds = DatasourceConfig( From 9a486d745a98bf06eadd2794661156df589f6d65 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 16:12:18 +0200 Subject: [PATCH 3/7] tasks.md: task 1.5 file list matches the spec --- .../tasks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md index b669f4e3..a06683da 100644 --- a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md @@ -4,7 +4,7 @@ - [x] 1.2 Version-independent resolver test: for every registered dialect whose scheme names a driver, `make_url(get_connection_string()).get_dialect().driver == sync_driver` for structured fields AND a bare user URL. Gotcha: CI cannot reproduce the 2.1 Postgres failure (pytest-postgresql installs psycopg 3; SQLAlchemy is capped at 2.0), so this test plus 1.1's explicit `+driver` assertions are the guard; verify the `mysql://` / `mariadb://` cases fail now - [x] 1.3 Driver-facts tests: `sync_driver` derived from `url_scheme` (None for driverless schemes); `url_backends` = `{url_backend} | aliases`, `{"mysql","mariadb"}` for mysql/mariadb, empty for an unregistered type's facts with no scheme; Postgres scheme is `postgresql+psycopg2` - [x] 1.4 Async tests: bare postgres / mysql URLs → `+asyncpg` / `+aiomysql`; `mariadb://` and `mariadb+pymysql://` → `mariadb+aiomysql://`; missing `psycopg2` on a bare `postgresql://` raises a missing-driver error naming the `[postgres]` extra -- [ ] 1.5 Update existing tests that assert bare/old URLs to the pinned form (behaviour change per DEV-2116): `tests/test_models.py:1068,1075`, `tests/test_dialect_driver_facts.py:86,99-100` (`test_registry_is_the_one_driver_table` stops setting `sync_driver` in `model_copy`), `tests/test_engine_factory.py:90`, `tests/test_async_driver_selection.py:33`; structured sqlserver scheme `mssql+pyodbc` in the "Known types keep their scheme" test +- [x] 1.5 Update existing tests that assert bare/old URLs to the pinned form (behaviour change per DEV-2116): `tests/test_models.py:1068`, `tests/test_datasource_snowflake.py:79`, `tests/test_dialect_driver_facts.py:86,99-100` (`test_registry_is_the_one_driver_table` stops setting `sync_driver` in `model_copy`), `tests/test_engine_factory.py:90`, `tests/test_async_driver_selection.py:33`; structured sqlserver scheme `mssql+pyodbc` in the "Known types keep their scheme" test ## 2. Implementation From 7a11dacad6228f073c447c0ac7f9838e7b2e642e Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 16:29:18 +0200 Subject: [PATCH 4/7] A connection URL that names no driver uses SLayer's default driver --- docs/configuration/datasources.md | 1 + .../tasks.md | 16 +++---- poetry.lock | 2 +- pyproject.toml | 2 +- slayer/core/models.py | 6 +++ slayer/sql/client.py | 11 ++--- slayer/sql/dialects/base.py | 46 +++++++++++++++++-- slayer/sql/dialects/drivers.py | 7 ++- slayer/sql/dialects/mysql.py | 2 +- slayer/sql/dialects/postgres.py | 3 +- tests/test_datasource_snowflake.py | 6 +-- tests/test_engine_factory.py | 8 ++-- tests/test_models.py | 45 +++++++++--------- 13 files changed, 97 insertions(+), 58 deletions(-) diff --git a/docs/configuration/datasources.md b/docs/configuration/datasources.md index 1d8af3a0..f0e7c2b3 100644 --- a/docs/configuration/datasources.md +++ b/docs/configuration/datasources.md @@ -65,6 +65,7 @@ These databases are verified by integration tests and runnable Docker examples. | `bigquery` | `motley-slayer[bigquery]` | `bigquery://project/dataset` | | `trino` | `motley-slayer[trino]` | `trino://user@host:8080/catalog/schema` | +A `connection_string` naming no driver (`postgresql://`, `mysql://`) uses the driver from SLayer's extra; name one explicitly to use another. A driver you name yourself in `connection_string` (e.g. `postgresql+psycopg`) is not in any extra, so install it separately; SLayer keeps it on the async path when it can run async. `duckdb` has no install extra because `duckdb` and `duckdb-engine` are core diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md index a06683da..a3db9bfb 100644 --- a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md +++ b/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md @@ -8,14 +8,14 @@ ## 2. Implementation -- [ ] 2.1 `slayer/sql/dialects/base.py`: `sync_driver` becomes a property on `SqlDialect` and `DriverFacts`; add `url_backend_aliases` field + `DriverFacts.url_backends` property; add `DriverFacts.pin_default_driver`; verify 1.3 passes -- [ ] 2.2 `postgres.py`: `url_scheme = "postgresql+psycopg2"`, drop `sync_driver`; `mysql.py`: drop `sync_driver`, declare `url_backend_aliases = {"mariadb"}`; verify 1.3 passes -- [ ] 2.3 `slayer/core/models.py`: `get_connection_string()` pins its result via the private-helper split; verify 1.1, 1.2 pass -- [ ] 2.4 `slayer/sql/client.py` `_async_connection_string`: `url_backends` match, URL's own backend, drop the plain-URL leg; verify 1.4 passes -- [ ] 2.5 `slayer/sql/dialects/drivers.py` `_is_default_driver`: `url_backends` match, fold `scheme_driver` into `sync_driver`; verify the missing-driver suite passes -- [ ] 2.6 `pyproject.toml`: `sqlalchemy = ">=2.0,<2.1"`; `poetry lock` (no upgrades — lock stays on 2.0.49); verify `poetry check --lock` +- [x] 2.1 `slayer/sql/dialects/base.py`: `sync_driver` becomes a property on `SqlDialect` and `DriverFacts`; add `url_backend_aliases` field + `DriverFacts.url_backends` property; add `DriverFacts.pin_default_driver`; verify 1.3 passes +- [x] 2.2 `postgres.py`: `url_scheme = "postgresql+psycopg2"`, drop `sync_driver`; `mysql.py`: drop `sync_driver`, declare `url_backend_aliases = {"mariadb"}`; verify 1.3 passes +- [x] 2.3 `slayer/core/models.py`: `get_connection_string()` pins its result via the private-helper split; verify 1.1, 1.2 pass +- [x] 2.4 `slayer/sql/client.py` `_async_connection_string`: `url_backends` match, URL's own backend, drop the plain-URL leg; verify 1.4 passes +- [x] 2.5 `slayer/sql/dialects/drivers.py` `_is_default_driver`: `url_backends` match, fold `scheme_driver` into `sync_driver`; verify the missing-driver suite passes +- [x] 2.6 `pyproject.toml`: `sqlalchemy = ">=2.0,<2.1"`; `poetry lock` (no upgrades — lock stays on 2.0.49); verify `poetry check --lock` ## 3. Docs and gates -- [ ] 3.1 One sentence in `docs/configuration/datasources.md`: a `connection_string` naming no driver (`postgresql://`, `mysql://`) uses the driver from SLayer's extra; name one explicitly to use another -- [ ] 3.2 Full unit suite (`poetry run pytest -m "not integration" -n logical`), `ruff check slayer/ tests/`, `basedpyright` (no new errors vs baseline), `la-arch-check` all green +- [x] 3.1 One sentence in `docs/configuration/datasources.md`: a `connection_string` naming no driver (`postgresql://`, `mysql://`) uses the driver from SLayer's extra; name one explicitly to use another +- [x] 3.2 Full unit suite (`poetry run pytest -m "not integration" -n logical`), `ruff check slayer/ tests/`, `basedpyright` (no new errors vs baseline), `la-arch-check` all green diff --git a/poetry.lock b/poetry.lock index 6bdbe9cf..df678aac 100644 --- a/poetry.lock +++ b/poetry.lock @@ -8282,4 +8282,4 @@ trino = ["trino"] [metadata] lock-version = "2.1" python-versions = "^3.11" -content-hash = "b6e6f775928254caf6293c7ef5cb8474686fa374dddb4c3602066fad57ddec79" +content-hash = "e318b76bd724a2ca9aa8658ba05c53396639fff8ceb2dc4523df6097f292af38" diff --git a/pyproject.toml b/pyproject.toml index d8db0422..7d95d364 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,7 +30,7 @@ Discord = "https://discord.gg/egWxMctHCA" [tool.poetry.dependencies] python = "^3.11" sqlglot = {version = ">=30.17", extras = ["c"]} -sqlalchemy = ">=2.0" +sqlalchemy = ">=2.0,<2.1" greenlet = ">=1" pydantic = ">=2.0" python-dateutil = ">=2.8" diff --git a/slayer/core/models.py b/slayer/core/models.py index 8ea58352..a0befba7 100644 --- a/slayer/core/models.py +++ b/slayer/core/models.py @@ -1030,6 +1030,12 @@ def _get_tsql_connection_string(self) -> str: ).render_as_string(hide_password=False) def get_connection_string(self) -> str: + """The connection URL; one naming no driver gets the type's default driver.""" + return dialect_for_ds_type(self.type).driver_facts(self.type).pin_default_driver( + self._unpinned_connection_string() + ) + + def _unpinned_connection_string(self) -> str: if self.connection_string: return self.connection_string # Dialect-specific hook (e.g. Snowflake); others return None and fall through. diff --git a/slayer/sql/client.py b/slayer/sql/client.py index 64ffc33f..a9d73c8c 100644 --- a/slayer/sql/client.py +++ b/slayer/sql/client.py @@ -48,20 +48,19 @@ def _get_async_engine(connection_string: str): def _async_connection_string(connection_string: str, db_type: str | None) -> str | None: """The async URL for ``connection_string``, or ``None`` to run sync-in-thread. - An async-capable driver is kept; a plain URL or the default sync driver moves to the - dialect's async driver; any other driver, or a foreign backend, stays sync. + An async-capable driver is kept; the default sync driver moves to the dialect's async + driver; any other driver, or a foreign backend, stays sync. """ facts = dialect_for_ds_type(db_type).driver_facts(db_type) if facts.async_driver is None: return None url = make_url(connection_string) - backend = facts.url_backend - if url.get_backend_name() != backend: + backend = url.get_backend_name() + if backend not in facts.url_backends: return None if url.get_dialect(_is_async=True).is_async: return connection_string - driver = url.drivername.partition("+")[2] - if driver and driver != facts.sync_driver: + if url.drivername.partition("+")[2] != facts.sync_driver: return None return url.set(drivername=f"{backend}+{facts.async_driver}").render_as_string(hide_password=False) diff --git a/slayer/sql/dialects/base.py b/slayer/sql/dialects/base.py index f097826b..bdb0837d 100644 --- a/slayer/sql/dialects/base.py +++ b/slayer/sql/dialects/base.py @@ -10,6 +10,8 @@ from collections.abc import Callable, Sequence from pydantic import BaseModel, ConfigDict +from sqlalchemy.engine.url import make_url +from sqlalchemy.exc import ArgumentError from sqlglot import exp from sqlglot.expressions.core import Expression from sqlglot.dialects.dialect import Dialect as _SqlglotDialect @@ -192,6 +194,10 @@ def _sqlglot_masking_lexis(sqlglot_name: str) -> SqlLexis: ) +def _scheme_driver(url_scheme: str | None) -> str | None: + return (url_scheme or "").partition("+")[2] or None + + def _digest(secret: str | None) -> str: """Non-reversible 16-hex id for secret material in cache keys.""" if not secret: @@ -215,7 +221,7 @@ class DriverFacts(BaseModel): model_config = ConfigDict(frozen=True) url_scheme: str | None = None - sync_driver: str | None = None + url_backend_aliases: frozenset[str] = frozenset() async_driver: str | None = None install_extra: str | None = None @@ -224,6 +230,31 @@ def url_backend(self) -> str: """The SQLAlchemy backend name of ``url_scheme``.""" return (self.url_scheme or "").partition("+")[0] + @property + def url_backends(self) -> frozenset[str]: + """Every SQLAlchemy backend this type serves; none without a scheme.""" + if self.url_scheme is None: + return frozenset() + return frozenset({self.url_backend}) | self.url_backend_aliases + + @property + def sync_driver(self) -> str | None: + """The default driver: ``url_scheme``'s ``+driver`` part.""" + return _scheme_driver(self.url_scheme) + + def pin_default_driver(self, url: str) -> str: + """``url`` with the default driver named when it names none on a served backend; else unchanged.""" + if self.sync_driver is None: + return url + try: + parsed = make_url(url) + except (ArgumentError, ValueError): + return url + backend = parsed.get_backend_name() + if "+" in parsed.drivername or backend not in self.url_backends: + return url + return parsed.set(drivername=f"{backend}+{self.sync_driver}").render_as_string(hide_password=False) + class SqlDialect(BaseModel): """One database's quirks; the base IS the Postgres-shaped default, subclasses override what differs.""" @@ -259,20 +290,25 @@ class SqlDialect(BaseModel): # An ``IN`` subquery and its joins run once on the initiator (``GLOBAL``): exact over shards. global_in_subqueries: bool = False - # URL scheme (``None`` = datasource type), sync/async DBAPI drivers, and their pip extra; - # read through ``driver_facts``. + # URL scheme (``None`` = datasource type; its ``+driver`` is the default driver), extra URL + # backends served, async DBAPI driver, and their pip extra; read through ``driver_facts``. url_scheme: str | None = None - sync_driver: str | None = None + url_backend_aliases: frozenset[str] = frozenset() async_driver: str | None = None install_extra: str | None = None + @property + def sync_driver(self) -> str | None: + """The default driver: ``url_scheme``'s ``+driver`` part.""" + return _scheme_driver(self.url_scheme) + def driver_facts(self, ds_type: str | None) -> DriverFacts: """``ds_type``'s driver facts: this dialect's when it serves ``ds_type``, else only ``ds_type`` as scheme.""" if ds_type is None or ds_type not in self.ds_type_aliases: return DriverFacts(url_scheme=ds_type) return DriverFacts( url_scheme=self.url_scheme or ds_type, - sync_driver=self.sync_driver, + url_backend_aliases=self.url_backend_aliases, async_driver=self.async_driver, install_extra=self.install_extra, ) diff --git a/slayer/sql/dialects/drivers.py b/slayer/sql/dialects/drivers.py index dac88576..d80a80a3 100644 --- a/slayer/sql/dialects/drivers.py +++ b/slayer/sql/dialects/drivers.py @@ -44,12 +44,11 @@ def _missing_driver( def _is_default_driver(facts: DriverFacts, *, url: URL, driver: str | None) -> bool: """Whether ``url`` selects SLayer's default driver; ``driver=None`` (none named) judges the backend alone.""" - if url.get_backend_name() != facts.url_backend: + if url.get_backend_name() not in facts.url_backends: return False - scheme_driver = (facts.url_scheme or "").partition("+")[2] - if driver is None or ("+" not in url.drivername and not scheme_driver): + if driver is None or ("+" not in url.drivername and not facts.sync_driver): return True - return driver in {facts.sync_driver, facts.async_driver, scheme_driver} + return driver in {facts.sync_driver, facts.async_driver} @contextlib.contextmanager diff --git a/slayer/sql/dialects/mysql.py b/slayer/sql/dialects/mysql.py index b258173b..8fd89cf8 100644 --- a/slayer/sql/dialects/mysql.py +++ b/slayer/sql/dialects/mysql.py @@ -45,7 +45,7 @@ class MysqlDialect(SqlDialect): # Conservative: MySQL allows 256 for column aliases but errors (not truncates). max_identifier_bytes: int | None = 64 url_scheme: str | None = "mysql+pymysql" - sync_driver: str | None = "pymysql" + url_backend_aliases: frozenset[str] = frozenset({"mariadb"}) async_driver: str | None = "aiomysql" install_extra: str | None = "mysql" diff --git a/slayer/sql/dialects/postgres.py b/slayer/sql/dialects/postgres.py index 6d25ffc2..6844a98d 100644 --- a/slayer/sql/dialects/postgres.py +++ b/slayer/sql/dialects/postgres.py @@ -44,8 +44,7 @@ class PostgresDialect(SqlDialect): # NAMEDATALEN: 63 usable bytes; over-length names are SILENTLY truncated. max_identifier_bytes: int | None = 63 statement_timeout_best_effort: bool = True - url_scheme: str | None = "postgresql" - sync_driver: str | None = "psycopg2" + url_scheme: str | None = "postgresql+psycopg2" async_driver: str | None = "asyncpg" install_extra: str | None = "postgres" diff --git a/tests/test_datasource_snowflake.py b/tests/test_datasource_snowflake.py index 431e6419..c401d387 100644 --- a/tests/test_datasource_snowflake.py +++ b/tests/test_datasource_snowflake.py @@ -1,4 +1,4 @@ -"""Tests for Snowflake-related fields on DatasourceConfig (DEV-1551). +"""Tests for Snowflake-related fields on DatasourceConfig. The URL-building tests live in ``tests/dialects/test_snowflake.py`` — that's the strategy class. Here we cover: @@ -11,6 +11,8 @@ ``snowflake-connector-python`` and ``snowflake-sqlalchemy``. """ +import pathlib +import tomllib from urllib.parse import parse_qs, urlparse import pytest @@ -120,8 +122,6 @@ class TestSnowflakePackagingExtras: """Pin the new pip extra at the packaging-metadata level.""" def _load_pyproject(self) -> dict: - import pathlib - import tomllib pyproject = pathlib.Path(__file__).parent.parent / "pyproject.toml" with open(pyproject, "rb") as f: return tomllib.load(f) diff --git a/tests/test_engine_factory.py b/tests/test_engine_factory.py index 24c12da9..21efc9ae 100644 --- a/tests/test_engine_factory.py +++ b/tests/test_engine_factory.py @@ -1,4 +1,4 @@ -"""Tests for the shared SQLAlchemy engine factory (DEV-1551). +"""Tests for the shared SQLAlchemy engine factory. The factory itself is dialect-agnostic. Each dialect's ``SqlDialect`` strategy class carries the runtime hooks; this module covers: @@ -112,7 +112,7 @@ def test_snowflake_engine_attaches_session_overrides_listener(self) -> None: engine_factory.get_engine(ds) listens_for_mock.assert_called_once() _engine_arg, event_name = listens_for_mock.call_args.args - # DEV-1551: use ``checkout`` (not ``connect``) so session state + # Use ``checkout`` (not ``connect``) so session state # is re-applied on every pool checkout, not just on the first # physical connection. assert event_name == "checkout" @@ -822,7 +822,7 @@ def run() -> None: class TestInMemorySqliteBuilder: - """DEV-1943 D2 — the factory's single StaticPool builder for in-memory SQLite.""" + """The factory's single StaticPool builder for in-memory SQLite.""" def test_builder_uses_static_pool(self) -> None: engine = engine_factory.build_in_memory_sqlite_engine("sqlite:///:memory:") @@ -866,7 +866,7 @@ def read_v() -> object: class TestInMemoryFactoryCoherence: - """DEV-1943 — in-memory SQLite through the shared factory: one db per + """In-memory SQLite through the shared factory: one db per datasource name, coherent across threads, disposed cleanly on reset.""" @staticmethod diff --git a/tests/test_models.py b/tests/test_models.py index c7de59bd..72573506 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -88,7 +88,7 @@ def test_colon_syntax_normalized(self) -> None: assert item.raw_formula == "revenue:sum" def test_funcstyle_builtin_agg_preserved(self) -> None: - """DEV-1826: the functional spelling is preserved verbatim — a + """The functional spelling is preserved verbatim — a placeholder defers resolution to binding (like custom aggs).""" item = OrderItem(column="sum(revenue)", direction="desc") assert item.column.name == _FUNCSTYLE_PENDING @@ -118,7 +118,7 @@ def test_plain_column_no_raw_formula(self) -> None: class TestOrderShorthandHealing: - """DEV-1575: heal LLM-shorthand ``order`` items and harden direction validation. + """Heal LLM-shorthand ``order`` items and harden direction validation. Shorthand ``{col: "desc"}`` (single- or multi-key) heals to canonical ``{"column": col, "direction": "desc"}``; direction is normalized @@ -382,7 +382,7 @@ def test_model_measure_may_be_named_after_transform(self) -> None: assert ModelMeasure(name="cumsum", formula="revenue:sum").name == "cumsum" def test_model_measure_formula_with_raw_over_raises(self) -> None: - """DEV-1336: a ``ModelMeasure`` formula containing raw `OVER (...)` SQL + """A ``ModelMeasure`` formula containing raw `OVER (...)` SQL cannot be parsed by SLayer's formula grammar (Python AST) and would produce invalid SQL on every dialect if used as a filter. Reject at construction time with an actionable error pointing at SLayer's @@ -444,7 +444,7 @@ def test_measure_sql_single_dot_unchanged(self) -> None: assert meas.sql == "orders.amount" def test_model_name_allows_double_underscore(self) -> None: - # DEV-1743: `__` is legal in model names (the dotted-canonical flip + # `__` is legal in model names (the dotted-canonical flip # made join-path ambiguity structurally impossible). model = SlayerModel(name="my__model", sql_table="t", data_source="test") assert model.name == "my__model" @@ -464,7 +464,7 @@ def test_measure_name_allows_double_underscore(self) -> None: assert meas.name == "stores__tax_rate_sum" def test_query_name_allows_double_underscore(self) -> None: - # DEV-1743: `__` is legal in query names. + # `__` is legal in query names. q = SlayerQuery(name="my__query", source_model="orders") assert q.name == "my__query" @@ -499,7 +499,7 @@ def test_measure_name_without_dot_allowed(self) -> None: assert meas.name == "order_total_sum" def test_model_data_source_rejects_dot(self) -> None: - """DEV-1405: dots in data_source would let a sibling datasource + """Dots in data_source would let a sibling datasource ``prod.legacy`` collide with cascade-delete of ``prod``.""" with pytest.raises(ValueError, match=r"must not contain '\.'"): SlayerModel( @@ -507,7 +507,7 @@ def test_model_data_source_rejects_dot(self) -> None: ) def test_datasource_name_rejects_dot(self) -> None: - """DEV-1405: dots in DatasourceConfig.name break canonical-id + """Dots in DatasourceConfig.name break canonical-id namespace boundaries.""" with pytest.raises(ValueError, match=r"must not contain '\.'"): DatasourceConfig(name="prod.legacy", type="postgres") @@ -1153,7 +1153,7 @@ def test_sqlserver_special_chars_in_password_are_url_encoded(self) -> None: assert "sqlserver" in cs assert "slayer_demo" in cs - # Issue #240: the generic (non-tsql) connection-string branch must not + # The generic (non-tsql) connection-string branch must not # corrupt credentials whose password contains reserved URL characters. # The password below exercises every generic-branch delimiter: '@' # (userinfo/host), ':' (userinfo/port), '/' (path), '#' (fragment), @@ -1163,13 +1163,12 @@ def test_sqlserver_special_chars_in_password_are_url_encoded(self) -> None: @pytest.mark.parametrize( ("ds_type", "expected_driver"), [ - ("postgres", "postgresql"), - ("postgresql", "postgresql"), + ("postgres", "postgresql+psycopg2"), + ("postgresql", "postgresql+psycopg2"), ("mysql", "mysql+pymysql"), ("mariadb", "mysql+pymysql"), ("clickhouse", "clickhouse+http"), - # Fallback dialect (issue #240 names "fallback dialects"): an - # unmapped type keeps its own name as the driver and must still + # An unmapped type keeps its own name as the driver and must still # encode reserved-char credentials safely. ("customdb", "customdb"), ], @@ -1204,8 +1203,8 @@ def test_reserved_char_password_round_trips( assert url.database == "analytics" def test_reserved_char_username_round_trips(self) -> None: - # Issue #240 names manually interpolated usernames as affected too, - # not just passwords. URL.create must percent-encode reserved + # Manually interpolated usernames are affected too, not just + # passwords. URL.create must percent-encode reserved # characters in the username so it round-trips intact. username = "user@tenant/name:role" ds = DatasourceConfig( @@ -1362,7 +1361,7 @@ def test_period_end_month(self) -> None: end = TimeGranularity.MONTH.period_end(datetime.date(2024, 3, 15)) assert end == datetime.date(2024, 3, 31) - # DEV-1572: WEEK_SUNDAY — Sunday-anchored week. period_start rounds back to + # WEEK_SUNDAY — Sunday-anchored week. period_start rounds back to # the Sunday at or before the date; period_end is the following Saturday. # 2024-01-07 is a Sunday; 2024-01-08..13 are Mon..Sat of that week, all of # which belong to the Sunday-week [2024-01-07 .. 2024-01-13]. @@ -1571,7 +1570,7 @@ def test_custom_without_formula_raises(self) -> None: Aggregation(name="my_agg") def test_name_with_dot_rejected(self) -> None: - """DEV-1567: ``Aggregation.name`` must enforce the same identifier + """``Aggregation.name`` must enforce the same identifier rules as ``Column.name`` and ``ModelMeasure.name``. Otherwise the ``column_x_custom_aggs`` catalog expansion produces a same-model metric like ``amount_my.agg`` whose dotted name the cross-model @@ -1640,7 +1639,7 @@ def test_filter_excluded_when_none(self) -> None: class TestSubstituteVariables: - # DEV-1625 made ``escape`` a required keyword-only argument. These legacy + # ``escape`` is a required keyword-only argument. These legacy # generic tests use ``escape="python"`` (the Mode-B query-filter lineage); # none of the values here contain quotes/backslashes so both modes agree. def test_string_variable(self) -> None: @@ -1709,7 +1708,7 @@ def test_invalid_variable_name_raises(self) -> None: def test_invalid_type_raises(self) -> None: # A dict is neither scalar nor list/tuple → rejected. (Lists ARE now a - # valid IN-list body — DEV-1730 — covered in + # valid IN-list body, covered in # tests/test_mode_a_variable_substitution.py.) with pytest.raises(ValueError, match="must be a string, number, or list/tuple"): substitute_variables( @@ -2083,7 +2082,7 @@ def test_similar_model_name_not_stripped_in_formula(self) -> None: class TestColumnTypeLenientValidator: - """DEV-1361: a Pydantic ``before``-validator absorbs legacy lowercase type + """A Pydantic ``before``-validator absorbs legacy lowercase type spellings on ``Column.type`` so old MCP/REST input keeps working after the sqlglot-aligned enum rename. No deprecation warning.""" @@ -2181,7 +2180,7 @@ def test_opaque_column_without_allowed_aggregations_is_valid(self) -> None: class TestModelMeasureType: - """DEV-1361: ModelMeasure gains an optional ``type`` declaring the formula's + """ModelMeasure has an optional ``type`` declaring the formula's result type. None (default) → no cast. Set value → outer CAST on the aggregation expression. Surfaces transparently on ``SlayerQuery.measures[i]`` since query measures use the same shape.""" @@ -2217,7 +2216,7 @@ def test_query_inline_measure_accepts_type(self) -> None: class TestColumnSampledValuesAndDistinctCount: - """DEV-1480: ``Column`` gains a structured sibling ``sampled_values`` for + """``Column`` has a structured sibling ``sampled_values`` for the text ``sampled`` string, and a ``distinct_count`` integer for the column's total cardinality at profile time. Both default to ``None``.""" @@ -2281,8 +2280,8 @@ def test_sampled_text_string_preserved_alongside_structured_fields(self) -> None class TestSlayerModelVersionBump: """SlayerModel.version tracks ``CURRENT_VERSIONS['SlayerModel']`` — v8 added - the optional ``source_kind`` field; DEV-1743 bumped to v9 (the ``__`` ban - lift + legacy-alias load-path rewrite). Both forward steps are no-op + the optional ``source_kind`` field; v9 lifted the ``__`` ban and added the + legacy-alias load-path rewrite. Both forward steps are no-op converters.""" def test_slayer_model_version_matches_current(self) -> None: From 0a2e86de87a25a910d0a1d0bda841f18ad9ca558 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 18:56:53 +0200 Subject: [PATCH 5/7] Review fix: SQLAlchemy-default parametrization is a params fixture, not request.getfixturevalue --- tests/test_default_driver_pin.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tests/test_default_driver_pin.py b/tests/test_default_driver_pin.py index 689e34d8..fedcd52a 100644 --- a/tests/test_default_driver_pin.py +++ b/tests/test_default_driver_pin.py @@ -37,13 +37,23 @@ def _isolated_engine_cache(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(engine_factory, "_engine_cache", OrderedDict()) -@pytest.fixture -def sa21_postgres_default(monkeypatch: pytest.MonkeyPatch) -> None: +def _simulate_sa21_postgres_default(monkeypatch: pytest.MonkeyPatch) -> None: """SQLAlchemy 2.1's default: bare ``postgresql://`` resolves to psycopg 3.""" monkeypatch.setattr(pg_base, "dialect", PGDialect_psycopg) assert make_url("postgresql://").get_dialect().driver == "psycopg" +@pytest.fixture +def sa21_postgres_default(monkeypatch: pytest.MonkeyPatch) -> None: + _simulate_sa21_postgres_default(monkeypatch) + + +@pytest.fixture(params=[False, True], ids=["installed", "sa21-postgres-default"]) +def sqlalchemy_postgres_default(request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch) -> None: + if request.param: + _simulate_sa21_postgres_default(monkeypatch) + + def _url(ds_type: str | None, connection_string: str) -> str: return DatasourceConfig(name="d", type=ds_type, connection_string=connection_string).get_connection_string() @@ -159,11 +169,9 @@ def test_default_driver_types_match_registry() -> None: assert declared == set(_DEFAULT_DRIVER) -@pytest.mark.parametrize("simulate_sa21", [False, True], ids=["installed", "sa21-postgres-default"]) +@pytest.mark.usefixtures("sqlalchemy_postgres_default") @pytest.mark.parametrize("ds_type", sorted(_DEFAULT_DRIVER)) -def test_sqlalchemy_resolves_declared_default_driver(request, ds_type, simulate_sa21) -> None: - if simulate_sa21: - request.getfixturevalue("sa21_postgres_default") +def test_sqlalchemy_resolves_declared_default_driver(ds_type) -> None: driver, backend = _DEFAULT_DRIVER[ds_type] structured = DatasourceConfig(name="d", type=ds_type, host="h", database="db", username="u", password="p") for url in (structured.get_connection_string(), _url(ds_type, f"{backend}://u@h/d")): From 9431178d402c240321c40b1cd1ebc3e4de02385f Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 20 Oct 2026 21:40:41 +0200 Subject: [PATCH 6/7] Archive the dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers OpenSpec change --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/sql/execution/spec.md | 0 .../tasks.md | 0 openspec/specs/sql/execution/spec.md | 85 ++++++++++++++++--- 6 files changed, 74 insertions(+), 11 deletions(-) rename openspec/changes/{dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers => archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers}/.openspec.yaml (100%) rename openspec/changes/{dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers => archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers}/design.md (100%) rename openspec/changes/{dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers => archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers}/proposal.md (100%) rename openspec/changes/{dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers => archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers}/specs/sql/execution/spec.md (100%) rename openspec/changes/{dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers => archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers}/tasks.md (100%) diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml b/openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml similarity index 100% rename from openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml rename to openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/.openspec.yaml diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md b/openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md similarity index 100% rename from openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md rename to openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/design.md diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md b/openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md similarity index 100% rename from openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md rename to openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/proposal.md diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md b/openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md similarity index 100% rename from openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md rename to openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/specs/sql/execution/spec.md diff --git a/openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md b/openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md similarity index 100% rename from openspec/changes/dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md rename to openspec/changes/archive/2026-10-20-dev-2116-a-connection-url-that-names-no-driver-always-uses-slayers/tasks.md diff --git a/openspec/specs/sql/execution/spec.md b/openspec/specs/sql/execution/spec.md index 221500b7..0150b81b 100644 --- a/openspec/specs/sql/execution/spec.md +++ b/openspec/specs/sql/execution/spec.md @@ -283,13 +283,13 @@ propagate unchanged. ### Requirement: The asynchronous path honours the user's driver choice For a datasource type with a native asynchronous driver (postgres/postgresql: asyncpg; mysql/mariadb: aiomysql), the asynchronous connection SHALL be derived from the datasource's -connection URL as follows, preserving every other URL part (credentials, host, port, database, -query): +connection URL — after a URL naming no driver has been given the type's default driver — as follows, +preserving the backend and every other URL part (credentials, host, port, database, query): - a URL that names an asynchronous-capable driver SHALL be used unchanged; -- a URL that names no driver, or names the type's default synchronous driver (postgres: psycopg2; - mysql/mariadb: pymysql), SHALL use the type's native asynchronous driver; -- a URL that names any other synchronous-only driver, or whose backend is not the type's own, SHALL - run on the synchronous path in a worker thread. +- a URL that names the type's default synchronous driver (postgres: psycopg2; mysql/mariadb: + pymysql) SHALL use the type's native asynchronous driver; +- a URL that names any other synchronous-only driver, or whose backend is not one the type serves, + SHALL run on the synchronous path in a worker thread. A datasource type without a native asynchronous driver SHALL run on the synchronous path in a worker thread. @@ -308,6 +308,10 @@ worker thread. `mysql+asyncmy://…` - **THEN** the first two use aiomysql and the third is used unchanged +#### Scenario: MariaDB keeps its backend +- **WHEN** a mariadb datasource's URL is `mariadb://u@h/d` or `mariadb+pymysql://u@h/d` +- **THEN** the asynchronous connection URL is `mariadb+aiomysql://u@h/d` + #### Scenario: Another named sync driver stays synchronous - **WHEN** a postgres datasource's URL is `postgresql+pg8000://…`, or a mysql datasource's URL is `mysql+mysqldb://…` @@ -318,13 +322,14 @@ worker thread. - **THEN** statements run on the synchronous path in a worker thread ### Requirement: Structured-config connection URLs are stable -A datasource configured by fields rather than a `connection_string` SHALL produce the same -connection URL as before this change for every type; a datasource type that no registered dialect -declares SHALL keep `://` as its URL scheme. +A datasource configured by fields rather than a `connection_string` SHALL produce a connection URL +whose scheme names the type's default driver where the type declares one; a datasource type that no +registered dialect declares SHALL keep `://` as its URL scheme. #### Scenario: Known types keep their scheme -- **WHEN** structured postgres, mysql, mariadb and clickhouse datasources build their URLs -- **THEN** the schemes are `postgresql`, `mysql+pymysql`, `mysql+pymysql` and `clickhouse+http` +- **WHEN** structured postgres, mysql, mariadb, clickhouse and sqlserver datasources build their URLs +- **THEN** the schemes are `postgresql+psycopg2`, `mysql+pymysql`, `mysql+pymysql`, + `clickhouse+http` and `mssql+pyodbc` #### Scenario: Unknown type keeps its own scheme - **WHEN** a structured datasource of an unregistered type `foo` builds its URL @@ -373,3 +378,61 @@ parameter SHALL reach the driver unchanged. #### Scenario: Reserved characters survive - **WHEN** a `trino` datasource's password contains `@`, `/` and `+` - **THEN** the driver receives the password unchanged + +### Requirement: A connection URL that names no driver uses SLayer's default driver +For a datasource type that declares a default driver (postgres/postgresql: psycopg2; mysql/mariadb: +pymysql; clickhouse: http; sqlserver/mssql/tsql: pyodbc), a connection URL — the datasource's +`connection_string`, or the URL built from its structured fields — whose backend is one the type +serves (postgres/postgresql: `postgresql`; mysql/mariadb: `mysql` and `mariadb`; clickhouse: +`clickhouse`; sqlserver/mssql/tsql: `mssql`) and which names no driver SHALL connect with the type's +default driver on the URL's own backend, whatever the installed SQLAlchemy's own default driver for +that backend is. Credentials, host, port, database and query parameters SHALL be preserved. + +A URL that names a driver, whose backend the type does not serve, or that cannot be parsed as a +URL, and every URL of a type without a declared default driver, SHALL be used exactly as given. A +stored `connection_string` SHALL NOT be rewritten. + +#### Scenario: Bare Postgres URL uses psycopg2 +- **WHEN** a postgres datasource's `connection_string` is + `postgresql://u:p%40ss@h:5432/d?sslmode=require` +- **THEN** it connects as `postgresql+psycopg2://u:p%40ss@h:5432/d?sslmode=require` + +#### Scenario: Bare MySQL and MariaDB URLs use pymysql +- **WHEN** a mysql datasource's `connection_string` is `mysql://u:p%40ss@h/d?charset=utf8mb4`, or a + mariadb datasource's is `mariadb://u@h/d` +- **THEN** they connect as `mysql+pymysql://u:p%40ss@h/d?charset=utf8mb4` and + `mariadb+pymysql://u@h/d` respectively + +#### Scenario: ClickHouse and SQL Server URLs use their default driver +- **WHEN** a clickhouse datasource's `connection_string` is `clickhouse://u@h/d`, or a sqlserver + datasource's is `mssql://u@h/d` +- **THEN** they connect as `clickhouse+http://u@h/d` and `mssql+pyodbc://u@h/d` respectively + +#### Scenario: A named driver is kept +- **WHEN** a postgres datasource's `connection_string` is `postgresql+psycopg://u@h/d` or + `postgresql+asyncpg://u@h/d`, or a mysql datasource's is `mysql+mysqldb://u@h/d` +- **THEN** the connection URL is exactly the `connection_string` + +#### Scenario: Types without a default driver are untouched +- **WHEN** a sqlite, duckdb, snowflake, trino, redshift or unregistered-type datasource connects, + including a sqlite `:memory:` datasource +- **THEN** its connection URL is exactly as given or as built today + +#### Scenario: A foreign backend is untouched +- **WHEN** a postgres datasource's `connection_string` is `mysql://u@h/d` +- **THEN** the connection URL is exactly the `connection_string` + +#### Scenario: Resolution does not depend on SQLAlchemy's defaults +- **WHEN** a datasource of any type that declares a default driver connects with a URL naming no + driver, or with structured fields +- **THEN** SQLAlchemy resolves that URL to the type's declared default driver + +#### Scenario: Missing default driver names the extra +- **WHEN** a postgres datasource's `connection_string` is `postgresql://u@h/d` and `psycopg2` is not + importable +- **THEN** a missing-driver error naming `psycopg2` and `pip install 'motley-slayer[postgres]'` is + raised + +#### Scenario: Stored configuration is not rewritten +- **WHEN** a datasource with `connection_string` `postgresql://u@h/d` is used and then read back +- **THEN** its `connection_string` is still `postgresql://u@h/d` From 9799b3feea84f8969432ba5d399945f50f315284 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 21 Oct 2026 10:56:57 +0200 Subject: [PATCH 7/7] sql/execution spec: the driver-named-URL rule defers to the async requirement for the async connection --- openspec/specs/sql/execution/spec.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openspec/specs/sql/execution/spec.md b/openspec/specs/sql/execution/spec.md index 0150b81b..5448b511 100644 --- a/openspec/specs/sql/execution/spec.md +++ b/openspec/specs/sql/execution/spec.md @@ -389,8 +389,9 @@ default driver on the URL's own backend, whatever the installed SQLAlchemy's own that backend is. Credentials, host, port, database and query parameters SHALL be preserved. A URL that names a driver, whose backend the type does not serve, or that cannot be parsed as a -URL, and every URL of a type without a declared default driver, SHALL be used exactly as given. A -stored `connection_string` SHALL NOT be rewritten. +URL, and every URL of a type without a declared default driver, SHALL be used exactly as given; the +asynchronous path then derives its URL from this one as "The asynchronous path honours the user's +driver choice" requires. A stored `connection_string` SHALL NOT be rewritten. #### Scenario: Bare Postgres URL uses psycopg2 - **WHEN** a postgres datasource's `connection_string` is