From 0c369a447eeaf39084f0d14a45b3eeb7eacb631b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 17:33:23 +0000 Subject: [PATCH 01/34] Add 4.0rc1 pre-release review notes Consolidated findings from a final review before the stable 4.0 release: five release blockers, semantic decisions to lock in now, polish items, and verified-sound areas. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- fable-review-4.0rc1.md | 375 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 375 insertions(+) create mode 100644 fable-review-4.0rc1.md diff --git a/fable-review-4.0rc1.md b/fable-review-4.0rc1.md new file mode 100644 index 0000000..c590b10 --- /dev/null +++ b/fable-review-4.0rc1.md @@ -0,0 +1,375 @@ +# sqlite-utils 4.0rc1 pre-release review + +Final review before the stable 4.0 release, focused on issues that would be +**breaking changes if fixed after 4.0 ships**. Reviewed at commit `79117b9` +(4.0rc1 plus four commits). Every finding below was verified against the +actual code; the two most serious were reproduced end-to-end. + +Test suite at review time: **1080 passed, 16 skipped**. + +**Verdict: do not tag stable yet.** There are five release blockers, all of +which are small fixes, plus a handful of semantic decisions that are cheap to +make now and breaking to change later. Recommended path: fix the blockers, +make the documented decisions, cut an **rc2**. + +--- + +## Release blockers + +Data loss or wrong-by-default behavior that 4.0 would lock in. + +### 1. `delete_where()` never commits and poisons the connection (data loss) + +`Table.delete_where()` (`sqlite_utils/db.py:2948`) runs its DELETE via a bare +`self.db.execute()` with no `atomic()` wrapper — compare `Table.delete()` at +`db.py:2944`, which wraps correctly. The connection is left +`in_transaction=True`, so every *subsequent* `atomic()` call takes the +savepoint branch (`db.py:430-440`) and never commits either. + +Reproduced end-to-end: + +```python +db = sqlite_utils.Database("dw.db") +db["t"].insert_all([{"id": i} for i in range(3)], pk="id") +db["t"].delete_where("id = ?", [0]) # conn.in_transaction is now True +db["t"].insert({"id": 50}) +db["u"].insert({"a": 1}) +db.close() +# Reopen: rows are [0, 1, 2] — the delete, row 50, AND table u are all gone. +``` + +In 3.x this leak was latent because later operations used `with db.conn:` +which committed the pending work (verified against 3.38, where the same +sequence persists everything). The 4.0 `atomic()` design — "don't commit an +existing transaction" — converts the old latent leak into permanent silent +data loss. + +**`optimize()` (`db.py:2790`) and `rebuild_fts()` (`db.py:2752`) have the +identical leak** — both run `INSERT INTO fts(fts) VALUES(...)` via bare +`self.db.execute()`. The CLI escapes only because `cli.py:341` and +`cli.py:369` wrap the calls in `with db.conn:`. + +Tellingly, the only `delete_where` example in the docs +(`docs/python-api.rst:983-990`) already wraps it in `with db.atomic():`, +papering over the bug, while every other single-op example needs no wrapper. + +**Fix:** wrap all three in `with self.db.atomic():`; update the docs example. +These were the only leaky public write ops found — every other op +(insert/upsert/update/delete/lookup/transform/extract/create/add_column/ +create_index/enable_fts/enable_counts/m2m/convert/duplicate) was verified to +leave `in_transaction=False`. + +### 2. `drop-view` drops tables and `drop-table` drops views, silently + +`cli.py:1728` (`drop-table`) and `cli.py:1800` (`drop-view`) both use +`db[name].drop()`. `Database.__getitem__` dispatches on the actual object +type, and `Table.drop()`/`View.drop()` each issue their matching `DROP` +statement. + +Reproduced: `sqlite-utils drop-view t.db t` deleted **table `t`** with exit +code 0. The mirror case (`drop-table` on a view) also succeeds silently. + +This is a data-loss footgun that contradicts the issue #657 table/view split +that is a headline 4.0 breaking change. Fixing it later converts a silent +"success" into an error — a semver problem, so it must land in 4.0. + +**Fix:** one line each — use `db.table(name)` / `db.view(name)` and catch +`NoTable` / `NoView` for a clean `ClickException`. + +### 3. Post-rc1 `insert({})` change lets `upsert` silently insert rows + +The new `DEFAULT VALUES` branch (commit `b5d0080`, +`db.py:3184-3194`) checks `not list_mode and not all_columns` but never +checks the `upsert` flag: + +- `db["t"].upsert({}, pk="id")` executes `INSERT INTO t DEFAULT VALUES` — a + brand-new row is written — and *then* raises an unrelated `KeyError: 'id'` + from the `last_pk` computation (`db.py:3742-3746`). +- `upsert_all([{}, {}], pk="id")` (2+ records skips the `last_pk` path) + **silently inserts two new rows on every call, no error at all**. + +At rc1 both failed fast with `ZeroDivisionError` — ugly, but zero rows +written. Going from crash-without-mutation to silent insertion under +"upsert" semantics is a regression 4.0 would lock in. + +Relatedly, the compound-pk auto-detection (commit `bfd74a3`, +`db.py:3557-3560`) makes records that *omit* the pk value reachable where +they previously raised `PrimaryKeyRequired` before touching the database: +`upsert_all([{"v": "a"}, {"v": "b"}])` on an existing pk table now inserts +with `id=NULL` (never conflicts) and appends new rows on every call +(verified: two calls → 4 rows). Same for compound pks with a missing +component. Not covered by any test. + +**Fix:** raise a clean error in the empty-record and missing-pk-value paths +when `upsert=True` (an empty record has no pk value, so it can never be an +upsert). + +### 4. `Migrations.apply()` transaction semantics are accidental — decide now + +`sqlite_utils/migrations.py:84-95` runs each migration function and its +tracking-row insert with no transaction wrapper. Verified: a migration that +fails halfway leaves its partial side effects committed, records nothing, +and re-running re-executes the *entire* function including already-applied +statements — the classic double-apply hazard. + +This matches sqlite-migrate, so either behavior is defensible — but 4.0 +freezes the contract: adding per-migration transactions in 4.1 would break +migrations that manage their own transactions or run statements illegal +inside one (`VACUUM`, some `PRAGMA`s). + +**Decide now:** wrap each migration in `db.atomic()`, or explicitly document +"migrations are not transactional; write idempotent steps" so the current +behavior is the contract rather than an accident. + +Also decide now: **`_AppliedMigration.applied_at` is annotated +`datetime.datetime` (`migrations.py:21`) but is a `str` at runtime** +(`migrations.py:67` passes the TEXT column value straight through), and the +type is published via autodoc (`docs/migrations.rst:168-171`). Changing the +annotation to `str` now is a free one-liner (and the sqlite-migrate- +compatible choice); "fixing" it to a real datetime later breaks every +consumer doing string operations. + +### 5. `enable_wal()` / `disable_wal()` commit open transactions + +`ensure_autocommit_off()` (`db.py:456-472`) assigns `conn.isolation_level`, +and CPython's setter **commits any pending transaction** as a side effect. +Verified consequences: + +- `with db.atomic(): insert(2); db.enable_wal(); insert(3); raise` → **all + rows persist despite the exception**, directly contradicting the + documented rollback guarantee (`docs/python-api.rst:255`), which is *the* + headline 4.0 semantic. +- A user's own `BEGIN` + insert + `enable_wal()` → their subsequent + `rollback()` is a no-op; the insert persists. This is exactly the + "unexpectedly committing an existing transaction" bug class `atomic()` was + introduced to eliminate (changelog, issue #755). + +Pre-existing in 3.x, but 4.0 is the moment to fix. + +**Fix:** make `enable_wal`/`disable_wal` raise (or skip the isolation dance) +when `conn.in_transaction`. + +--- + +## Decisions to make now — cheap today, breaking after 4.0 + +### `Database.__enter__` / `__exit__` contract is unpinned + +`__exit__` only calls `self.close()` (`db.py:408-417`), which silently rolls +back uncommitted changes. Verified: a raw `db.execute("insert ...")` inside +`with Database(p) as db:` is discarded on exit. This diverges from +`sqlite3.Connection`'s own context manager (commits on success, does *not* +close). The docs (`docs/python-api.rst:139-145`) say only "automatically +close the connection" — no mention of rollback — and **no test anywhere uses +`with Database(...)`**, so the contract is completely unpinned. Whichever way +this might later be "improved" is a breaking behavior change. Decide now, +add one docs sentence ("any uncommitted changes are rolled back") and a +pinning test. Note blockers 1–2 make this worse: after a `delete_where`, +exiting the `with` block discards everything. + +### `atomic()` is broken on Python 3.12+ `autocommit` connections + +`db.py:441-453` uses `conn.commit()`/`conn.rollback()`, documented no-ops +when `Connection.autocommit=True`. Verified on 3.13: + +- `autocommit=True` connection → a successful `atomic()` block leaves the + transaction permanently open; rollback on exception is also a no-op. +- `autocommit=False` connection → the connection is always in a transaction, + so `atomic()` always takes the savepoint branch and never commits + anything. + +`Database(filename_or_conn)` accepts arbitrary connections (`db.py:397`). +Either handle these modes (issue literal `COMMIT`/`ROLLBACK` statements, or +reject such connections) or document supported connection modes in +`python_api_atomic` now — changing commit timing later is breaking. +(Legacy `isolation_level=None` connections were verified to work correctly.) + +### `atomic()` savepoint-branch semantics are intended but undocumented + +If the connection is already in a transaction (user ran `BEGIN` or raw DML), +`atomic()` becomes a savepoint and its successful exit does **not** persist +anything until the user commits (verified: a user `rollback()` discards the +atomic block's writes). Reasonable semantics, but the `conn.in_transaction` +sniffing must be documented before it is locked — the docs currently +describe nesting only in terms of `atomic()`-inside-`atomic()`. + +### `atomic()` issues plain deferred `BEGIN` + +`db.py:442` — no way to request `BEGIN IMMEDIATE`. A future switch of the +default would change locking/`SQLITE_BUSY` behavior (breaking); adding an +`immediate=` parameter later is fine. Documenting "deferred" now costs one +sentence. + +### No-op `-d/--detect-types` flag: keep or kill + +`cli.py:942-947` still defines `-d/--detect-types` on `insert`/`upsert` +(help says "(default)"), deliberately tested as a no-op +(`tests/test_cli.py:2299,2322`). The `detect_types` parameter of +`insert_upsert_implementation` (`cli.py:1002`) is dead code — the body only +reads `no_detect_types`. If the flag is ever to be removed, 4.0 is the only +chance; keeping it as back-compat is defensible, but note +`--detect-types --no-detect-types` together silently favors the latter with +no conflict error. + +### `--stop-before` is silently ignored for old `sqlite_migrate` objects + +The duck-typing in `_compatible_migration_set` (`cli.py:3286-3289`) exists +precisely so migration files still doing `from sqlite_migrate import +Migrations` keep working — but `cli.py:3393` always passes `stop_before=` as +a **list**, and the old plugin's `apply()` does `if name == stop_before:` +against a string. Verified with the released 0.1b0: `--stop-before step2` +applied `step2` anyway. The CLI applies the exact migration the user asked +it not to, on the one upgrade path the duck typing exists to support. + +Related: a **typo'd `--stop-before` name silently applies everything**, +including the migration you meant to stop before (unknown names simply never +match; no error from the CLI). Validation has to live in the CLI ("each +`--stop-before` value must match at least one set") because unqualified +names legitimately fan out across sets. Adding that error post-4.0 turns +currently-succeeding invocations into failures — decide now. + +### Public-API validation via bare `assert` + +User-facing errors raised via `assert` throughout `db.py` (370, 396, 1016, +1950, 2829, 3028, 3565, 3571, 3967, …). They vanish under `python -O`, and +`db.py:3028` carries `# TODO: Test this works (rolls back) - use better +exception:`. Converting `AssertionError` to `ValueError` later changes +exception types callers may catch — best done in a major, if ever. + +--- + +## Should-fix polish (non-breaking later, but ugly for a stable release) + +- **`insert`/`upsert` into a view name prints a raw traceback** — + `NoTable("Table v is actually a view")` from `cli.py:1154` is not + converted to `ClickException` (contrast `duplicate` at `cli.py:1676`). +- **Misleading `NoView` message** — `db.view("t")` where `t` is a table says + "View t does not exist" (`db.py:661`); the mirror case helpfully says + "Table v is actually a view" (`db.py:650`). +- **Stale `__getitem__` docstring** (`db.py:502`) says it "returns a Table + object"; it returns a `View` for views. Feeds autodoc. +- **Wrong `hash_id` doc in the `Table` docstring** (`db.py:1588`) — says + bool; it's a column-name string (`Optional[str]`). +- **`sqlite-utils migrate --list` writes to the database** — + `pending()`/`applied()` call `ensure_migrations_table` + (`migrations.py:48,65`), so `--list` creates the tracking table, performs + the one-way legacy sqlite-migrate schema upgrade, and — since `db_path` + has no `exists=True` (`cli.py:3338-3340`) — creates the database file + itself. A read-looking operation should not do any of that. +- **`applied()` has no `ORDER BY`** (`migrations.py:66-71`) — relies on + rowid order; `tests/test_cli_migrate.py:104-107` asserts that order. + Add `order_by="id"` to make `--list` deterministic by contract. +- **Duplicate migration names within a set**: both functions execute (side + effects committed) before an opaque `IntegrityError`; only the first is + recorded (`migrations.py:36-42`). A `ValueError` at registration would be + cheap and is effectively non-breaking to add now. +- **`pending()`/`applied()` return underscore-private dataclasses** + (`_Migration`/`_AppliedMigration`) that are excluded from autodoc + (`docs/migrations.rst:171`), so the `.name`/`.applied_at`/`.fn` fields + users must access are undocumented API. +- **`set:name` colon syntax is unvalidated** (`cli.py:3328` uses + `partition(":")`) — a set or migration name containing `:` can never be + targeted. Document/validate "no colons in names" now. +- **Bare `@migrations` decorator** gives an opaque `TypeError` + (`migrations.py:30`); a helpful message would be purely additive. +- **Tracer never sees transaction statements** — `atomic()` uses + `self.conn.execute()`/`conn.commit()` directly (`db.py:432-450`), + bypassing the tracer. Routing through `self.execute()` later would change + tracer output that downstream tests may assert on — cheap to decide now. +- **Order-dependent empty-dict semantics** (from `b5d0080`): + `insert_all([{}, {"v": "hi"}])` gives the empty record its column DEFAULTs, + but `insert_all([{"v": "hi"}, {}])` inserts explicit `NULL` for it — a + `NOT NULL ... DEFAULT` column succeeds in one ordering and raises + `IntegrityError` in the other. +- **Batch-size cliff when the first record is `{}`** (`db.py:3625-3629`) — + `num_columns` comes from the first record only, forcing `batch_size=1` + for the entire stream. Performance-only, degenerate input. +- **`pyproject.toml` build-system underpinned** — `requires = ["setuptools"]` + while the PEP 639 `license = "Apache-2.0"` string needs setuptools ≥ + 77.0.3; non-isolated sdist builds on older setuptools will fail. Pin + `setuptools>=77`. +- **Undocumented post-rc1 behavior changes** — neither upsert pk + auto-detection (`bfd74a3`) nor `insert({})`-uses-DEFAULT-VALUES + (`b5d0080`) is mentioned in `docs/` or the changelog. Note also that + `db.table(name).insert({})` only works for *existing* tables — on a + missing table it still raises `AssertionError: Tables must have at least + one column`, so the issue #759 title scenario is only partially covered. + +--- + +## Verified sound — no action needed + +- **sqlite-migrate on-disk compatibility** (the scariest area): tested + against the actual 0.1b0 sdist from PyPI. Same `_sqlite_migrations` table; + the legacy compound-pk `(migration_set, name)` schema is detected via + `table.pks != ["id"]` and upgraded in place (`migrations.py:97-117`); rows + are preserved and previously-applied migrations are **not** re-applied. + Both timestamp formats are identical. Tests cover both legacy layouts + (`tests/test_migrations.py:83-110`). No data-corruption risk for upgrading + users. The built-in `migrate` command is registered *after* plugin hooks + (`cli.py:3415-3417`), so a still-installed old plugin's command is + correctly overridden. +- Migration ordering is definition order (documented and tested); identity + is `(set name, migration name)` with a unique index; same name across sets + is fine. The new CLI is a strict superset of the old plugin's; file + discovery is now `sorted()` where the plugin iterated an unordered set. +- Nested savepoint rollback/release logic including exception paths + (`tests/test_atomic.py:44-120`); commit-time deferred-FK failure rolls + back cleanly; `transform()` inside an open transaction correctly uses + `PRAGMA defer_foreign_keys`; `_executescript` statement-splitting avoids + sqlite3's implicit commit. Savepoint naming via `secrets.token_hex(16)` is + collision-free. +- Compound-pk upsert detection happy path works for single and compound pks + on both the `ON CONFLICT` and `use_old_upsert` implementations; explicit + `pk=` still wins over detection; rowid/missing/hash_id tables still raise + `PrimaryKeyRequired` (`tests/test_upsert.py:52-97`). +- The table/view split (#657) is otherwise complete: `db.table()` on a view + raises `NoTable`, never returns a `View`; `db.view()` accepts no + table-only kwargs. The tracer-test fix (`401fb69`) and click pin + (`79117b9`, dev-group only) are correct. +- `__all__` exports are coherent (`Database`, `Migrations`, + `suggest_column_types`, `hookimpl`, `hookspec`); every `sqlite_utils.X` + docs reference resolves. The `pip` runtime dependency is genuinely + required (`cli.py:2967-2983` uses `run_module("pip")` for + `install`/`uninstall`). Classifiers match `requires-python`. No + deprecation debt: zero `DeprecationWarning` markers; clean under + `-W error::DeprecationWarning`. + +--- + +## The transform() incoming-FK branch: hold for 4.1 + +The `update_incoming_fks` work on `claude/investigate-transform-fk-KMTZ7` is +right to hold: + +- It is purely additive (`update_incoming_fks: bool = False` keyword + + `--update-incoming-fks` flag; default behavior unchanged), so 4.1 is safe + semver-wise. +- The branch predates the `atomic()` refactor — it still uses + `with self.db.conn:` directly in `transform()`, the exact pattern 4.0 + eliminated (#755) — and has real issues: `_skip_fk_validation` is mutable + state on the whole `Database` object, and the incoming-FK rebuilds execute + `transform_sql()` output raw for referencing tables, silently dropping + their indexes (the 3.38 index-recreation logic only runs for the primary + table). +- One decision belongs to 4.0: whether `transform()` should eventually + refuse/warn *by default* when a rename would leave dangling incoming FKs. + Adding a warning later is fine; making it an error by default later is + breaking. If strict-by-default is the desired end state, add just the + guard in 4.0 and ship the auto-update machinery in 4.1. + +--- + +## Suggested path to stable + +1. Fix the five blockers — three `atomic()` wrappers (`delete_where`, + `optimize`, `rebuild_fts`), two CLI lookups (`drop-table`/`drop-view`), + the `upsert` guard in the DEFAULT VALUES / missing-pk paths, the + `applied_at: str` annotation, and the `in_transaction` guard in + `enable_wal`/`disable_wal`. +2. Make and document the decisions: `apply()` transactionality, + `Database.__exit__` semantics (+ pinning test), `atomic()` supported + connection modes and deferred-`BEGIN` note, `--detect-types` keep/kill, + `--stop-before` validation and old-plugin compat. +3. Cut **rc2** — the transaction-semantics fixes deserve one more candidate + round before the semantics freeze. From f3c012774a609999e65d49f032f354315dda6cb9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 17:39:12 +0000 Subject: [PATCH 02/34] Add review feedback for the create-table-parser branch Assessment of the CREATE TABLE parser as the foundation for 4.1 transform() improvements: fit for 4.1, two tokenizer bugs to fix first (comments, numeric literal defaults), test wiring, and design decisions around round-trip fidelity and API visibility. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- create-table-parser-feedback.md | 167 ++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 create-table-parser-feedback.md diff --git a/create-table-parser-feedback.md b/create-table-parser-feedback.md new file mode 100644 index 0000000..2b6f727 --- /dev/null +++ b/create-table-parser-feedback.md @@ -0,0 +1,167 @@ +# Review: `create-table-parser` branch + +Feedback on the `create-table-parser` branch (commits `ed1de15`, `95d6ff4`, +`407db59` — 819-line `create_table_parser.py` plus JSON test corpora), +reviewed as the proposed foundation for 4.1 `transform()` improvements that +make UNIQUE and CHECK constraints supported instead of silently dropped. + +**Verdict: good fit for 4.1 — right design, right release — but it has two +silent-corruption bugs that must be fixed before `transform()` can trust it, +and a few design decisions to settle.** + +## Why 4.1 is the right home + +- The parser is purely additive: a new module and a new capability. Nothing + about it needs to land in 4.0, so it does not constrain or delay the + stable release. +- The `transform()` improvement it enables — preserving CHECK and UNIQUE + constraints instead of silently dropping them — is a behavior change in + the bug-fix direction, appropriate for a minor release with a prominent + changelog note. +- SQLite exposes no introspection for CHECK constraints (and only partial + detail elsewhere), so parsing the `sqlite_master` DDL is the correct — and + only — approach. The module docstring says exactly this. + +## What holds up well (verified empirically) + +- **Corpus results:** all 272 statements in `tests/valid_create_table.json` + parse without crashing; the 230 that are independently executable were + cross-checked against SQLite's own `PRAGMA table_xinfo` — **0 column-name + or primary-key mismatches**. The 29 invalid statements also do not crash + the parser. +- Survived adversarial probing: identifier quoting in all four styles + including embedded escaped quotes (`"col ""x"""`, `[square]`, backticks), + strings containing `,)` and keywords inside CHECK expressions, + `col IN (...)` option extraction with escaped quotes, generated columns + (`GENERATED ALWAYS AS ... STORED` and bare `AS`), FK action clauses + (`ON DELETE SET NULL ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED`), + `WITHOUT ROWID, STRICT` trailers, keyword-named columns (`"unique"`), + multi-word types with parenthesized arguments, and `CREATE TABLE AS + SELECT`. +- The design is genuinely good: recursive-descent functions mirroring the + SQLite grammar are readable; the constraint list as source of truth with + derived accessor properties (`table.checks`, `col.not_null`, + `table.primary_key`) is the right model — adding a constraint kind means + adding a dataclass and a property. The group-capturing tokenizer + (parenthesized groups taken whole so nested commas never reach the parser) + is a clean way to sidestep expression parsing. + +## Must-fix before `transform()` builds on it + +### 1. Comments produce phantom columns (silent corruption) + +The tokenizer does not handle `--` line comments or `/* */` block comments, +and `sqlite_master` stores DDL verbatim. Reproduced: + +``` +CREATE TABLE t ( + a TEXT, -- user's name, (important) + b INTEGER +) +→ columns parsed as ['a', '-'] (real: ['a', 'b']) + +CREATE TABLE t (a TEXT /* legacy, do not use */, b INTEGER) +→ columns parsed as ['a', 'do', 'b'] (real: ['a', 'b']) +``` + +Hand-written schemas — exactly the ones that have CHECK constraints — are +exactly the ones with comments. If `transform()` rebuilds a table from a +model with wrong columns, that is data loss. Comment handling belongs in +`_tokenize` / `_locate_body` / `_split_top_level` (all three scan raw text). + +### 2. Numeric literal defaults are truncated (silent corruption) + +`_tokenize` splits `1.5` into `1` / `.` / `5` and `_default_value` consumes +one token. Reproduced: + +| DDL | parsed default | correct | +|----------------------|----------------|---------| +| `DEFAULT 1.5` | `'1'` | `1.5` | +| `DEFAULT -1.5` | `'-1'` | `-1.5` | +| `DEFAULT 1e-3` | `'1e'` | `1e-3` | +| `DEFAULT x'0102'` | `'x'` | `x'0102'` | + +Re-emitting these in a transform rewrites `DEFAULT 1.5` as `DEFAULT 1` — +silent schema corruption. Notably the corpus already contains +`DEFAULT -45.8e22`, but nothing catches this because the fixtures are +input-only (see next point). Fix by lexing numeric literals (including +sign, decimal point, exponent) and blob literals (`x'...'`) as single +tokens, or by making `_default_value` consume the full literal. + +### 3. No tests are actually executed + +The JSON corpora are not wired into pytest — there is no `test_*.py` on the +branch, so the parser currently has zero executed tests. Beyond wiring the +corpus in, add **expected-output snapshots** (parse each valid statement, +assert the full structured result), not just "doesn't crash": the +`DEFAULT -45.8e22` bug sat undetected in the corpus precisely because only +inputs are recorded. A cheap high-value addition: property test that for +every executable statement, parsed column names / pk / not-null match +`PRAGMA table_xinfo`. + +## Design decisions to settle + +### Discard-vs-error policy for grammar the model does not capture + +The plan is presumably parse → modify model → serialize, which matches how +`transform()` already rebuilds tables. That makes "what the model discards" +the critical list, because anything discarded is silently stripped from the +user's schema on transform: + +- `ON CONFLICT` clauses on PRIMARY KEY / UNIQUE / NOT NULL (parsed by + `_conflict_clause`, thrown away) +- FK `MATCH` and `DEFERRABLE INITIALLY DEFERRED` (parsed, thrown away — + deferred FKs are real in the wild) +- `ASC` / `DESC` in table-level `PRIMARY KEY (a DESC)` / `UNIQUE (...)` + column lists (`_column_list_group` keeps only the leading identifier; + inline single-column PK order *is* captured) +- Unknown column constraints (`_column_constraint` consumes one token and + returns `None`) + +Either capture these in the model, or have `transform()` raise +`TransformError` when the source DDL contains grammar it cannot round-trip — +the honest-failure pattern 3.38 established for un-recreatable indexes. +Silence is the one wrong answer. + +Same question for column renames: a renamed column referenced inside a CHECK +expression (stored as a raw string) cannot be mechanically rewritten, so +`transform(rename=...)` intersecting a check should raise `TransformError` +rather than emit a stale constraint. Likewise dropping a column referenced +by a table-level CHECK or multi-column UNIQUE. + +### Ship it private in 4.1 + +The module defines `Table`, `Column`, and `ForeignKey` — the package already +has `db.Table` and a `db.ForeignKey` namedtuple with different, incompatible +fields (`(table, column, other_table, other_column)` vs +`(columns, table, references, ...)`). Two public `ForeignKey` types in one +package is a confusion trap, and 4.0 is a fresh reminder that public surface +is forever. Recommendation: land it as a private module +(`sqlite_utils._parser` or similar) powering `transform()` in 4.1, promote +to documented public API in a later minor once battle-tested. Public-later +is additive; public-now-retract-later is breaking. + +Related naming nit: the parser's `Table.schema` is the attached-database +name (`main`/`temp`), while `db.Table.schema` is the DDL string. Rename one +(e.g. `database` or `schema_name`) before anything goes public. + +### Housekeeping + +- `create_table_parser.py` lives at the repo root; it needs to move inside + the `sqlite_utils/` package. +- `parse_checks` is documented as a "backwards-compatible helper" but + nothing in sqlite-utils exists for it to be compatible with — stale + comment from an earlier draft. +- The valid corpus is strong on grammar coverage but light on real-world + mess: add fixtures with comments, tabs/newlines in odd places, numeric + and blob defaults, and a couple of schemas generated by sqlite-utils + itself (double-quoted everything) and by common tools. + +## Bottom line + +Hold it for 4.1 exactly as planned — it has no 4.0 dependency and should not +delay the stable release. Before `transform()` builds on it: fix comment +handling and numeric/blob literal lexing, wire the fixtures into pytest with +expected-output snapshots, decide the discard-vs-error policy, and keep the +module private for one release. The bones are solid; both bugs are +tokenizer-level and small. From b17c37144eded6af1a326699e8fa732b039ff4be Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 17:57:35 +0000 Subject: [PATCH 03/34] Fix delete_where() leaving the connection in an open transaction Table.delete_where() ran its DELETE via a bare execute() with no commit, unlike Table.delete() which wraps in db.atomic(). The connection was left with an open implicit transaction, so the deletion (and all subsequent writes, including later atomic() blocks which switched to savepoint mode) was silently rolled back when the connection closed. Wrap the DELETE in db.atomic() and remove the now-unnecessary atomic() wrapper from the delete_where documentation example. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 3 +-- sqlite_utils/db.py | 3 ++- tests/test_delete.py | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/python-api.rst b/docs/python-api.rst index eab858e..b756605 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -984,8 +984,7 @@ You can delete all records in a table that match a specific WHERE statement usin >>> db = sqlite_utils.Database("dogs.db") >>> # Delete every dog with age less than 3 - >>> with db.atomic(): - >>> db.table("dogs").delete_where("age < ?", [3]) + >>> db.table("dogs").delete_where("age < ?", [3]) Calling ``table.delete_where()`` with no other arguments will delete every row in the table. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index ae99322..5acab69 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -2966,7 +2966,8 @@ class Table(Queryable): sql = "delete from {}".format(quote_identifier(self.name)) if where is not None: sql += " where " + where - self.db.execute(sql, where_args or []) + with self.db.atomic(): + self.db.execute(sql, where_args or []) if analyze: self.analyze() return self diff --git a/tests/test_delete.py b/tests/test_delete.py index 652096c..a2d93aa 100644 --- a/tests/test_delete.py +++ b/tests/test_delete.py @@ -1,3 +1,6 @@ +import sqlite_utils + + def test_delete_rowid_table(fresh_db): table = fresh_db["table"] table.insert({"foo": 1}).last_pk @@ -32,6 +35,21 @@ def test_delete_where_all(fresh_db): assert table.count == 0 +def test_delete_where_commits(tmpdir): + path = str(tmpdir / "test.db") + db = sqlite_utils.Database(path) + db["table"].insert_all([{"id": i} for i in range(5)], pk="id") + db["table"].delete_where("id > ?", [2]) + # The connection must not be left inside an open transaction, + # otherwise subsequent atomic() blocks never commit either + assert not db.conn.in_transaction + db["table"].insert({"id": 100}) + db.close() + db2 = sqlite_utils.Database(path) + assert [r["id"] for r in db2["table"].rows] == [0, 1, 2, 100] + db2.close() + + def test_delete_where_analyze(fresh_db): table = fresh_db["table"] table.insert_all(({"id": i, "i": i} for i in range(10)), pk="id") From d36639ed73a1e05e425dd3cfa666a389c9fa26d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:06:06 +0000 Subject: [PATCH 04/34] Run each migration inside a transaction Migrations.apply() now wraps each migration function and its _sqlite_migrations tracking row in db.atomic(), so they commit together: a failing migration rolls back cleanly, is not recorded, and stays pending, eliminating the double-apply hazard where a partially-failed migration left committed side effects behind. Migrations that cannot run inside a transaction (VACUUM, journal mode changes, manual transaction management) can opt out by registering with @migrations(transactional=False). Also fixed the test fixtures which used db.query() for INSERT statements - query() is a lazy generator, so those inserts never actually executed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/migrations.rst | 19 ++++++++++ sqlite_utils/migrations.py | 42 ++++++++++++++++------ tests/test_migrations.py | 74 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 12 deletions(-) diff --git a/docs/migrations.rst b/docs/migrations.rst index 948b468..5628ded 100644 --- a/docs/migrations.rst +++ b/docs/migrations.rst @@ -70,6 +70,25 @@ When you apply a set of migrations you can stop part way through by specifying a migrations.apply(db, stop_before="add_weight") +.. _migrations_transactions: + +Migrations and transactions +=========================== + +Each migration runs inside a transaction, together with the ``_sqlite_migrations`` record of it having been applied. If a migration function raises an exception, everything it did is rolled back, no record is written and the migration stays pending - so fixing the error and re-applying will run that migration again from a clean state. Migrations that completed earlier in the same ``apply()`` run stay applied. + +Some operations cannot run inside a transaction, for example ``VACUUM`` or changing the journal mode with ``db.enable_wal()``. Register migrations like these with ``transactional=False``: + +.. code-block:: python + + @migrations(transactional=False) + def compact(db): + db.execute("VACUUM") + +A migration registered with ``transactional=False`` runs without a wrapping transaction, so if it fails part way through any changes it already made will not be rolled back, and re-applying will run the whole function again. + +Avoid calling ``db.conn.commit()`` or otherwise managing transactions manually inside a transactional migration - register the migration with ``transactional=False`` if it needs to control its own transactions. + Applying migrations using the CLI ================================= diff --git a/sqlite_utils/migrations.py b/sqlite_utils/migrations.py index f349b17..8af0784 100644 --- a/sqlite_utils/migrations.py +++ b/sqlite_utils/migrations.py @@ -14,6 +14,7 @@ class Migrations: class _Migration: name: str fn: Callable + transactional: bool = True @dataclass class _AppliedMigration: @@ -27,15 +28,22 @@ class Migrations: self.name = name self._migrations: list[Migrations._Migration] = [] - def __call__(self, *, name: str | None = None) -> Callable: + def __call__( + self, *, name: str | None = None, transactional: bool = True + ) -> Callable: """ :param name: The name to use for this migration - if not provided, the name of the function will be used. + :param transactional: If ``True`` (the default) the migration and the + record of it having been applied are wrapped in a transaction, which + will be rolled back if the migration raises an exception. Pass + ``False`` for migrations that cannot run inside a transaction, for + example those that execute ``VACUUM``. """ def inner(func: Callable) -> Callable: self._migrations.append( - self._Migration(name or getattr(func, "__name__"), func) + self._Migration(name or getattr(func, "__name__"), func, transactional) ) return func @@ -73,6 +81,12 @@ class Migrations: def apply(self, db: "Database", *, stop_before: str | Iterable[str] | None = None): """ Apply any pending migrations to the database. + + Each migration runs inside a transaction, together with the record of + it having been applied - if the migration raises an exception its + changes are rolled back, no record is written and the migration stays + pending. Migrations registered with ``transactional=False`` run + outside of a transaction. """ self.ensure_migrations_table(db) if stop_before is None: @@ -85,14 +99,22 @@ class Migrations: name = migration.name if name in stop_before_names: return - migration.fn(db) - _table(db, self.migrations_table).insert( - { - "migration_set": self.name, - "name": name, - "applied_at": str(datetime.datetime.now(datetime.timezone.utc)), - } - ) + if migration.transactional: + with db.atomic(): + migration.fn(db) + self._record_applied(db, name) + else: + migration.fn(db) + self._record_applied(db, name) + + def _record_applied(self, db: "Database", name: str): + _table(db, self.migrations_table).insert( + { + "migration_set": self.name, + "name": name, + "applied_at": str(datetime.datetime.now(datetime.timezone.utc)), + } + ) def ensure_migrations_table(self, db: "Database"): """ diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 6a0dd33..34741a5 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -14,7 +14,7 @@ def migrations(): @migrations() def m002(db): db["cats"].create({"name": str}) - db.query("insert into dogs (name) values ('Pancakes')") + db.execute("insert into dogs (name) values ('Pancakes')") return migrations @@ -32,7 +32,7 @@ def migrations_not_ordered_alphabetically(): @migrations() def m001(db): db["cats"].create({"name": str}) - db.query("insert into dogs (name) values ('Pancakes')") + db.execute("insert into dogs (name) values ('Pancakes')") return migrations @@ -80,6 +80,76 @@ def test_order_does_not_matter(migrations, migrations_not_ordered_alphabetically assert db1.schema == db2.schema +def test_failing_migration_rolls_back(migrations): + @migrations() + def m003(db): + db["birds"].create({"name": str}) + db.execute("insert into dogs (name) values ('Dozer')") + raise ValueError("boom") + + db = sqlite_utils.Database(memory=True) + with pytest.raises(ValueError): + migrations.apply(db) + # m001 and m002 committed before the failure and stay applied + assert set(db.table_names()) == {"_sqlite_migrations", "dogs", "cats"} + assert [r["name"] for r in db["dogs"].rows] == ["Cleo", "Pancakes"] + assert [m.name for m in migrations.applied(db)] == ["m001", "m002"] + # Everything m003 did was rolled back and it is still pending + assert [m.name for m in migrations.pending(db)] == ["m003"] + + +def test_rerun_after_failure_applies_each_migration_once(): + state = {"fail": True} + migrations = Migrations("test") + + @migrations() + def m001(db): + db["dogs"].insert({"name": "Cleo"}) + + @migrations() + def m002(db): + db["dogs"].insert({"name": "Pancakes"}) + if state["fail"]: + raise ValueError("boom") + + db = sqlite_utils.Database(memory=True) + with pytest.raises(ValueError): + migrations.apply(db) + state["fail"] = False + migrations.apply(db) + # m001 must not have been re-applied, m002 applied exactly once + assert [r["name"] for r in db["dogs"].rows] == ["Cleo", "Pancakes"] + assert [m.name for m in migrations.applied(db)] == ["m001", "m002"] + + +def test_non_transactional_migration_allows_vacuum(tmpdir): + path = str(tmpdir / "test.db") + db = sqlite_utils.Database(path) + migrations = Migrations("test") + + @migrations() + def m001(db): + db["dogs"].insert({"name": "Cleo"}) + + @migrations(transactional=False) + def m002(db): + db.execute("VACUUM") + + migrations.apply(db) + assert [m.name for m in migrations.applied(db)] == ["m001", "m002"] + db.close() + + +def test_apply_composes_inside_outer_transaction(migrations): + db = sqlite_utils.Database(memory=True) + with pytest.raises(ZeroDivisionError): + with db.atomic(): + migrations.apply(db) + raise ZeroDivisionError + # The outer transaction rolled back, taking the migrations with it + assert db.table_names() == [] + + @pytest.mark.parametrize( "create_table,pk", ( From 862944bc38381ea4cb23fe050e0481c4a7495447 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:13:50 +0000 Subject: [PATCH 05/34] drop-table and drop-view now refuse to drop the wrong object type drop-table on a view name silently dropped the view, and drop-view on a table name silently dropped the table, because both used db[name].drop() which dispatches on the actual object type. They now use db.table() / db.view() and exit with an explanatory error pointing at the correct command. --ignore still exits cleanly but no longer drops anything. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- sqlite_utils/cli.py | 19 ++++++++++++++++--- tests/test_cli.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index f15850d..d51ba7b 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -14,6 +14,7 @@ from sqlite_utils.db import ( DEFAULT, DescIndex, NoTable, + NoView, quote_identifier, ) from sqlite_utils.plugins import ensure_plugins_loaded, pm, get_plugins @@ -1725,7 +1726,13 @@ def drop_table(path, table, ignore, load_extension): _register_db_for_cleanup(db) _load_extensions(db, load_extension) try: - db[table].drop(ignore=ignore) + db.table(table).drop(ignore=ignore) + except NoTable: + # A view exists with this name + if not ignore: + raise click.ClickException( + '"{}" is a view, not a table - use drop-view to drop it'.format(table) + ) except OperationalError: raise click.ClickException('Table "{}" does not exist'.format(table)) @@ -1797,8 +1804,14 @@ def drop_view(path, view, ignore, load_extension): _register_db_for_cleanup(db) _load_extensions(db, load_extension) try: - db[view].drop(ignore=ignore) - except OperationalError: + db.view(view).drop(ignore=ignore) + except NoView: + if ignore: + return + if view in db.table_names(): + raise click.ClickException( + '"{}" is a table, not a view - use drop-table to drop it'.format(view) + ) raise click.ClickException('View "{}" does not exist'.format(view)) diff --git a/tests/test_cli.py b/tests/test_cli.py index 565fbc2..02c00ab 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1467,6 +1467,24 @@ def test_drop_table_error(): assert result.exit_code == 0 +def test_drop_table_on_view_errors(): + runner = CliRunner() + with runner.isolated_filesystem(): + db = Database("test.db") + db["t"].insert({"id": 1}) + db.create_view("v", "select * from t") + result = runner.invoke(cli.cli, ["drop-table", "test.db", "v"]) + assert result.exit_code == 1 + assert 'Error: "v" is a view, not a table - use drop-view to drop it' == ( + result.output.strip() + ) + assert "v" in db.view_names() + # --ignore exits cleanly but must still not drop the view + result = runner.invoke(cli.cli, ["drop-table", "test.db", "v", "--ignore"]) + assert result.exit_code == 0 + assert "v" in db.view_names() + + def test_drop_view(): runner = CliRunner() with runner.isolated_filesystem(): @@ -1485,6 +1503,23 @@ def test_drop_view(): assert "hello" not in db.view_names() +def test_drop_view_on_table_errors(): + runner = CliRunner() + with runner.isolated_filesystem(): + db = Database("test.db") + db["t"].insert({"id": 1}) + result = runner.invoke(cli.cli, ["drop-view", "test.db", "t"]) + assert result.exit_code == 1 + assert 'Error: "t" is a table, not a view - use drop-table to drop it' == ( + result.output.strip() + ) + assert "t" in db.table_names() + # --ignore exits cleanly but must still not drop the table + result = runner.invoke(cli.cli, ["drop-view", "test.db", "t", "--ignore"]) + assert result.exit_code == 0 + assert "t" in db.table_names() + + def test_drop_view_error(): runner = CliRunner() with runner.isolated_filesystem(): From 45af93d46314db8af0b9621462d20f0840227b42 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:24:52 +0000 Subject: [PATCH 06/34] db.query() now executes its SQL immediately Previously query() was a generator function, so nothing - including the SQL itself - ran until the result was first iterated. A write statement passed to query() silently did nothing, and SQL errors surfaced far from the call site. The SQL now executes as soon as query() is called, while rows are still fetched lazily during iteration. Statements that return no rows now raise a ValueError directing callers to execute() instead. As a side effect, statements like INSERT ... RETURNING now work naturally with query(). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 4 ++++ sqlite_utils/db.py | 18 ++++++++++++++++-- tests/test_query.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/docs/python-api.rst b/docs/python-api.rst index b756605..39361c9 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -219,6 +219,10 @@ The ``db.query(sql)`` function executes a SQL query and returns an iterator over # {'name': 'Cleo'} # {'name': 'Pancakes'} +The SQL query is executed as soon as ``db.query()`` is called. The resulting rows are fetched lazily as you iterate, so large result sets are not loaded into memory all at once. Because execution is immediate, an error in your SQL will raise an exception straight away, and a statement such as ``INSERT ... RETURNING`` will take effect even if you do not iterate over its results. + +``db.query()`` can only be used with SQL that returns rows. Passing a statement that returns no rows - an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause, for example - will raise a ``ValueError``. Use :ref:`db.execute() ` for those statements instead. + .. _python_api_execute: db.execute(sql, params) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 5acab69..63c798f 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -593,14 +593,28 @@ class Database: """ Execute ``sql`` and return an iterable of dictionaries representing each row. + The SQL is executed as soon as this method is called - the resulting rows + are then fetched lazily as the returned iterable is iterated over. + :param sql: SQL query to execute :param params: Parameters to use in that query - an iterable for ``where id = ?`` parameters, or a dictionary for ``where id = :id`` + :raises ValueError: if the SQL statement does not return rows - use + :meth:`execute` for those statements instead """ cursor = self.execute(sql, params or tuple()) + if cursor.description is None: + raise ValueError( + "query() can only be used with SQL that returns rows - " + "use execute() for other statements" + ) keys = [d[0] for d in cursor.description] - for row in cursor: - yield dict(zip(keys, row)) + + def rows() -> Generator[dict, None, None]: + for row in cursor: + yield dict(zip(keys, row)) + + return rows() def execute( self, sql: str, parameters: Optional[Union[Sequence, Dict[str, Any]]] = None diff --git a/tests/test_query.py b/tests/test_query.py index fe79cc0..0b0b9c5 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -1,3 +1,5 @@ +import pytest +import sqlite3 import types @@ -8,6 +10,33 @@ def test_query(fresh_db): assert list(results) == [{"name": "Pancakes"}, {"name": "Cleo"}] +def test_query_executes_eagerly(fresh_db): + # The SQL runs when query() is called, not when the result is iterated, + # so errors are raised at the call site + with pytest.raises(sqlite3.OperationalError): + fresh_db.query("select * from missing_table") + + +def test_query_rejects_statements_that_return_no_rows(fresh_db): + fresh_db["dogs"].insert({"name": "Cleo"}) + with pytest.raises(ValueError) as ex: + fresh_db.query("update dogs set name = 'Cleopaws'") + assert "execute()" in str(ex.value) + + +@pytest.mark.skipif( + sqlite3.sqlite_version_info < (3, 35, 0), + reason="RETURNING requires SQLite 3.35.0 or higher", +) +def test_query_insert_returning(fresh_db): + fresh_db["dogs"].insert({"name": "Cleo"}) + rows = list( + fresh_db.query("insert into dogs (name) values ('Pancakes') returning name") + ) + assert rows == [{"name": "Pancakes"}] + assert fresh_db["dogs"].count == 2 + + def test_execute_returning_dicts(fresh_db): # Like db.query() but returns a list, included for backwards compatibility # see https://github.com/simonw/sqlite-utils/issues/290 From e762656d0687d86607c6c2acdefc086584f31d70 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:35:45 +0000 Subject: [PATCH 07/34] optimize() and rebuild_fts() now commit their changes Both ran their INSERT INTO fts(fts) statements via a bare execute() with no commit, leaving the connection inside an open implicit transaction - the FTS operation and all subsequent writes were then silently rolled back when the connection closed. Both are now wrapped in db.atomic(), matching delete_where() and the other write operations. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- sqlite_utils/db.py | 16 +++++++++------- tests/test_fts.py | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 63c798f..6f947e1 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -2769,11 +2769,12 @@ class Table(Queryable): if fts_table is None: # Assume this is itself an FTS table fts_table = self.name - self.db.execute( - "INSERT INTO {table}({table}) VALUES('rebuild');".format( - table=quote_identifier(fts_table) + with self.db.atomic(): + self.db.execute( + "INSERT INTO {table}({table}) VALUES('rebuild');".format( + table=quote_identifier(fts_table) + ) ) - ) return self def detect_fts(self) -> Optional[str]: @@ -2805,9 +2806,10 @@ class Table(Queryable): "Run the ``optimize`` operation against the associated full-text search index table." fts_table = self.detect_fts() if fts_table is not None: - self.db.execute(""" - INSERT INTO {table} ({table}) VALUES ("optimize"); - """.strip().format(table=quote_identifier(fts_table))) + with self.db.atomic(): + self.db.execute(""" + INSERT INTO {table} ({table}) VALUES ("optimize"); + """.strip().format(table=quote_identifier(fts_table))) return self def search_sql( diff --git a/tests/test_fts.py b/tests/test_fts.py index ba4b32c..11df898 100644 --- a/tests/test_fts.py +++ b/tests/test_fts.py @@ -336,6 +336,24 @@ def test_rebuild_fts(fresh_db): assert len(rows2) == 2 +@pytest.mark.parametrize("method", ["optimize", "rebuild_fts"]) +def test_optimize_and_rebuild_fts_commit(tmpdir, method): + path = str(tmpdir / "test.db") + db = Database(path) + table = db["searchable"] + table.insert(search_records[0]) + table.enable_fts(["text", "country"]) + getattr(table, method)() + # The connection must not be left inside an open transaction, + # otherwise this and all subsequent writes are lost on close + assert not db.conn.in_transaction + table.insert(search_records[1]) + db.close() + db2 = Database(path) + assert db2["searchable"].count == 2 + db2.close() + + @pytest.mark.parametrize("invalid_table", ["does_not_exist", "not_searchable"]) def test_rebuild_fts_invalid(fresh_db, invalid_table): fresh_db["not_searchable"].insert({"foo": "bar"}) From 286a87dba4b93c034614a1fe931fcf503b5e643d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:37:20 +0000 Subject: [PATCH 08/34] Add Unreleased changelog section covering changes since 4.0rc1 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 765b0e0..2c7e2f4 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,18 @@ Changelog =========== +.. _unreleased: + +Unreleased +---------- + +- **Breaking change**: ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. +- Fixed a bug where ``table.delete_where()``, ``table.optimize()`` and ``table.rebuild_fts()`` did not commit their changes, leaving the connection inside an open transaction. Their work - and any subsequent writes - could then be silently rolled back when the connection was closed. All three now use ``db.atomic()``, consistent with the other write methods. +- The ``sqlite-utils drop-table`` command now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. Both now exit with an error suggesting the correct command to use. +- Migrations applied by the new :ref:`migrations system ` now run inside a transaction, together with the record of the migration having been applied. If a migration raises an exception its changes are rolled back and it stays pending, so it can be safely re-applied after the error is fixed. Migrations that cannot run inside a transaction, such as those executing ``VACUUM``, can opt out using ``@migrations(transactional=False)`` - see :ref:`migrations_transactions`. +- ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. +- ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) + .. _v4_0rc1: 4.0rc1 (2026-06-21) From d6753a9f57f3d58b89c40bbbebbbbea465a33327 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:43:37 +0000 Subject: [PATCH 09/34] upsert() now requires a value for every primary key column A record missing a primary key value - or with None for one - can never match an existing row, because a NULL primary key never satisfies ON CONFLICT. Such records were previously inserted as brand new rows (silently, for multi-record upsert_all calls) or triggered a KeyError after the insert had already happened. Both cases, including upserting an empty record, now raise PrimaryKeyRequired before any SQL is executed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/python-api.rst | 2 ++ sqlite_utils/db.py | 25 +++++++++++++++++++++++++ tests/test_upsert.py | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2c7e2f4..3a36722 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -14,6 +14,7 @@ Unreleased - The ``sqlite-utils drop-table`` command now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. Both now exit with an error suggesting the correct command to use. - Migrations applied by the new :ref:`migrations system ` now run inside a transaction, together with the record of the migration having been applied. If a migration raises an exception its changes are rolled back and it stays pending, so it can be safely re-applied after the error is fixed. Migrations that cannot run inside a transaction, such as those executing ``VACUUM``, can opt out using ``@migrations(transactional=False)`` - see :ref:`migrations_transactions`. - ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. +- ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) .. _v4_0rc1: diff --git a/docs/python-api.rst b/docs/python-api.rst index 39361c9..ae2c0cb 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -1021,6 +1021,8 @@ Note that the ``pk`` and ``column_order`` parameters here are optional if you ar An ``upsert_all()`` method is also available, which behaves like ``insert_all()`` but performs upserts instead. +Every record passed to ``upsert()`` or ``upsert_all()`` must include a value for each primary key column - a record without one could never match an existing row, so a ``sqlite_utils.db.PrimaryKeyRequired`` exception is raised instead of quietly inserting a new row. + .. note:: ``.upsert()`` and ``.upsert_all()`` in sqlite-utils 1.x worked like ``.insert(..., replace=True)`` and ``.insert_all(..., replace=True)`` do in 2.x. See `issue #66 `__ for details of this change. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 6f947e1..b4f5dcc 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -3199,6 +3199,11 @@ class Table(Queryable): # Dict-mode insert({}) has no explicit columns; SQLite spells that as # DEFAULT VALUES. List mode with no columns is a different input shape. if not list_mode and not all_columns: + if upsert: + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key - " + "an empty record cannot be upserted" + ) or_clause = "" if replace: or_clause = " OR REPLACE" @@ -3286,6 +3291,26 @@ class Table(Queryable): # Everything from here on is for upsert=True pk_cols = [pk] if isinstance(pk, str) else list(pk) + # Every record must provide a value for every primary key column - a + # NULL primary key never matches ON CONFLICT, so the record would be + # inserted as a brand new row instead of upserted + missing_pk_cols = [c for c in pk_cols if c not in all_columns] + if missing_pk_cols: + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key column{}: {}".format( + "s" if len(missing_pk_cols) > 1 else "", + ", ".join(missing_pk_cols), + ) + ) + pk_indexes = [all_columns.index(c) for c in pk_cols] + for record_values in values: + if any(record_values[i] is None for i in pk_indexes): + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key column{}: {}".format( + "s" if len(pk_cols) > 1 else "", + ", ".join(pk_cols), + ) + ) non_pk_cols = [c for c in all_columns if c not in pk_cols] conflict_sql = ", ".join(quote_identifier(c) for c in pk_cols) diff --git a/tests/test_upsert.py b/tests/test_upsert.py index 09783bb..a782b26 100644 --- a/tests/test_upsert.py +++ b/tests/test_upsert.py @@ -49,6 +49,42 @@ def test_upsert_error_if_no_pk(fresh_db): table.upsert({"id": 1, "name": "Cleo"}) +@pytest.mark.parametrize("use_old_upsert", (False, True)) +def test_upsert_empty_record_errors(use_old_upsert): + db = Database(memory=True, use_old_upsert=use_old_upsert) + table = db["table"] + table.insert({"id": 1, "name": "Cleo"}, pk="id") + with pytest.raises(PrimaryKeyRequired): + table.upsert({}, pk="id") + with pytest.raises(PrimaryKeyRequired): + table.upsert_all([{}, {}], pk="id") + # No rows can have been inserted + assert table.count == 1 + + +@pytest.mark.parametrize("use_old_upsert", (False, True)) +def test_upsert_missing_pk_value_errors(use_old_upsert): + db = Database(memory=True, use_old_upsert=use_old_upsert) + table = db["table"] + table.insert({"id": 1, "name": "Cleo"}, pk="id") + # Records that omit the pk column entirely + with pytest.raises(PrimaryKeyRequired): + table.upsert_all([{"name": "Pancakes"}, {"name": "Marnie"}], pk="id") + # A record with an explicit None pk value can never conflict + with pytest.raises(PrimaryKeyRequired): + table.upsert({"id": None, "name": "Pancakes"}, pk="id") + assert list(table.rows) == [{"id": 1, "name": "Cleo"}] + + +def test_upsert_missing_compound_pk_value_errors(fresh_db): + table = fresh_db["table"] + table.insert({"a": "x", "b": "y", "v": 1}, pk=("a", "b")) + # Missing one component of the detected compound primary key + with pytest.raises(PrimaryKeyRequired): + table.upsert({"a": "x", "v": 2}) + assert list(table.rows) == [{"a": "x", "b": "y", "v": 1}] + + def test_upsert_error_if_existing_table_has_no_pk(fresh_db): table = fresh_db.create_table("table", {"id": int, "name": str}) with pytest.raises(PrimaryKeyRequired): From ffec11cfb7875120e34d52d89dc5a06e79de0f9d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:46:09 +0000 Subject: [PATCH 10/34] enable_wal() and disable_wal() refuse to run inside a transaction Changing the journal mode assigns conn.isolation_level, which commits any open transaction as a side effect - silently breaking the rollback guarantee of atomic() blocks and of user-managed transactions. Both methods now raise RuntimeError if a transaction is open. Calling them when the database is already in the requested mode remains a no-op. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/python-api.rst | 2 ++ sqlite_utils/db.py | 21 ++++++++++++++++++++- tests/test_wal.py | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3a36722..0c409a1 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,6 +16,7 @@ Unreleased - ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. - ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) +- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``RuntimeError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. .. _v4_0rc1: diff --git a/docs/python-api.rst b/docs/python-api.rst index ae2c0cb..f03f963 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -2702,6 +2702,8 @@ You can disable WAL mode using ``.disable_wal()``: Database("my_database.db").disable_wal() +The journal mode can only be changed outside of a transaction. Calling either method while a transaction is open - inside a ``db.atomic()`` block, for example - raises a ``RuntimeError``, unless the database is already in the requested mode in which case the call is a no-op. + You can check the current journal mode for a database using the ``journal_mode`` property: .. code-block:: python diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index b4f5dcc..97e8920 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -854,17 +854,36 @@ class Database: def enable_wal(self) -> None: """ Sets ``journal_mode`` to ``'wal'`` to enable Write-Ahead Log mode. + + :raises RuntimeError: if called while a transaction is open - the + journal mode can only be changed outside of a transaction """ if self.journal_mode != "wal": + self._ensure_no_open_transaction("enable_wal()") with self.ensure_autocommit_off(): self.execute("PRAGMA journal_mode=wal;") def disable_wal(self) -> None: - "Sets ``journal_mode`` back to ``'delete'`` to disable Write-Ahead Log mode." + """ + Sets ``journal_mode`` back to ``'delete'`` to disable Write-Ahead Log mode. + + :raises RuntimeError: if called while a transaction is open - the + journal mode can only be changed outside of a transaction + """ if self.journal_mode != "delete": + self._ensure_no_open_transaction("disable_wal()") with self.ensure_autocommit_off(): self.execute("PRAGMA journal_mode=delete;") + def _ensure_no_open_transaction(self, operation: str) -> None: + # Changing journal mode assigns conn.isolation_level, which commits + # any open transaction as a side effect - breaking the rollback + # guarantee of atomic() and of user-managed transactions + if self.conn.in_transaction: + raise RuntimeError( + "{} cannot be used while a transaction is open".format(operation) + ) + def _ensure_counts_table(self) -> None: with self.atomic(): self.execute(_COUNTS_TABLE_CREATE_SQL.format(self._counts_table_name)) diff --git a/tests/test_wal.py b/tests/test_wal.py index 23ca144..fa3c855 100644 --- a/tests/test_wal.py +++ b/tests/test_wal.py @@ -21,3 +21,39 @@ def test_enable_disable_wal(db_path_tmpdir): db.disable_wal() assert "delete" == db.journal_mode assert "test.db-wal" not in [f.basename for f in tmpdir.listdir()] + + +def test_enable_wal_inside_transaction_raises(db_path_tmpdir): + db, path, tmpdir = db_path_tmpdir + db["test"].insert({"id": 1}, pk="id") + with pytest.raises(RuntimeError): + with db.atomic(): + db["test"].insert({"id": 2}, pk="id") + db.enable_wal() + # The atomic() block must have rolled back cleanly and the + # journal mode must be unchanged + assert db.journal_mode == "delete" + assert [r["id"] for r in db["test"].rows] == [1] + + +def test_disable_wal_inside_transaction_raises(db_path_tmpdir): + db, path, tmpdir = db_path_tmpdir + db.enable_wal() + db["test"].insert({"id": 1}, pk="id") + with pytest.raises(RuntimeError): + with db.atomic(): + db["test"].insert({"id": 2}, pk="id") + db.disable_wal() + assert db.journal_mode == "wal" + assert [r["id"] for r in db["test"].rows] == [1] + + +def test_enable_wal_noop_inside_transaction_is_allowed(db_path_tmpdir): + # Calling enable_wal() when WAL is already enabled is a no-op, + # so it is fine inside a transaction + db, path, tmpdir = db_path_tmpdir + db.enable_wal() + with db.atomic(): + db["test"].insert({"id": 1}, pk="id") + db.enable_wal() + assert [r["id"] for r in db["test"].rows] == [1] From c76aad50aeec0bc78a7847bdcdb87f3afa616c5b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:47:08 +0000 Subject: [PATCH 11/34] Correct applied_at type annotation to str _AppliedMigration.applied_at was annotated datetime.datetime but the value is the TEXT timestamp read straight from the _sqlite_migrations table - always a string, matching the format written by both this module and the older sqlite-migrate package. Added a test pinning the runtime type. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- sqlite_utils/migrations.py | 4 +++- tests/test_migrations.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/sqlite_utils/migrations.py b/sqlite_utils/migrations.py index 8af0784..20572af 100644 --- a/sqlite_utils/migrations.py +++ b/sqlite_utils/migrations.py @@ -19,7 +19,9 @@ class Migrations: @dataclass class _AppliedMigration: name: str - applied_at: datetime.datetime + # A string timestamp such as "2026-07-04 12:00:00.000000+00:00" - + # stored as TEXT in the _sqlite_migrations table + applied_at: str def __init__(self, name: str): """ diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 34741a5..c190fd5 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -80,6 +80,18 @@ def test_order_does_not_matter(migrations, migrations_not_ordered_alphabetically assert db1.schema == db2.schema +def test_applied_at_is_a_string(migrations): + db = sqlite_utils.Database(memory=True) + migrations.apply(db) + applied = migrations.applied(db) + assert len(applied) == 2 + for migration in applied: + # applied_at is the TEXT timestamp straight from the + # _sqlite_migrations table, e.g. "2026-07-04 12:00:00.000000+00:00" + assert isinstance(migration.applied_at, str) + assert migration.applied_at.endswith("+00:00") + + def test_failing_migration_rolls_back(migrations): @migrations() def m003(db): From 70b48a3b16669d2b02711f2c95714665902c3450 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 18:51:55 +0000 Subject: [PATCH 12/34] Use the sqlite3/sqlean shim in test_query.py test_query_executes_eagerly failed on the sqlean CI matrix jobs because sqlean.dbapi2.OperationalError is not a subclass of the stdlib sqlite3.OperationalError. Import sqlite3 via sqlite_utils.utils like the other test modules do. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- tests/test_query.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_query.py b/tests/test_query.py index 0b0b9c5..91bf1ac 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -1,7 +1,8 @@ import pytest -import sqlite3 import types +from sqlite_utils.utils import sqlite3 + def test_query(fresh_db): fresh_db["dogs"].insert_all([{"name": "Cleo"}, {"name": "Pancakes"}]) From a6fcb5af624ed667d7c2b3a6ede3ad41ae0f067b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:05:59 +0000 Subject: [PATCH 13/34] Pin the Database context manager contract with Database(...) closes the connection on exit without committing, so uncommitted changes are rolled back. This was the existing behavior but was undocumented and untested - it is now stated in the docs and pinned by a test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 2 ++ tests/test_constructor.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/docs/python-api.rst b/docs/python-api.rst index f03f963..d575ba6 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -144,6 +144,8 @@ The ``Database`` object also works as a context manager, which will automaticall db["my_table"].insert({"name": "Example"}) # Connection is automatically closed here +Exiting the block closes the connection without committing, so any changes that are still inside an open transaction at that point - for example writes made with raw ``db.execute()`` calls - will be rolled back. Commit explicitly, or use methods such as ``.insert()`` and the ``db.atomic()`` context manager which commit their own transactions. + .. _python_api_attach: Attaching additional databases diff --git a/tests/test_constructor.py b/tests/test_constructor.py index 8743911..0798996 100644 --- a/tests/test_constructor.py +++ b/tests/test_constructor.py @@ -29,6 +29,21 @@ def test_sqlite_version(): assert actual == as_string +def test_database_context_manager(tmpdir): + path = str(tmpdir / "test.db") + with Database(path) as db: + db["t"].insert({"id": 1}) + # A raw write left uncommitted on purpose: + db.execute("insert into t (id) values (2)") + # The connection is closed... + with pytest.raises(sqlite3.ProgrammingError): + db.execute("select 1") + # ... and the uncommitted change was rolled back, not committed + db2 = Database(path) + assert [r["id"] for r in db2["t"].rows] == [1] + db2.close() + + @pytest.mark.parametrize("memory", [True, False]) def test_database_close(tmpdir, memory): if memory: From fd867282b3ae8ba05e60eee39471181008102c47 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:06:44 +0000 Subject: [PATCH 14/34] Document atomic() deferred BEGIN, savepoint behavior and connection modes Documents that atomic() opens a deferred BEGIN, that calling it while the connection is already inside a transaction produces a savepoint whose changes only persist when the outer transaction commits, and that Python 3.12+ autocommit=True/False connections are not supported. Adds a test pinning the savepoint-inside-manual- BEGIN behavior. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 6 ++++++ tests/test_atomic.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/docs/python-api.rst b/docs/python-api.rst index d575ba6..e888b7d 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -274,6 +274,12 @@ If an exception is raised, changes made inside the block will be rolled back. pass db.table("dogs").insert({"id": 3, "name": "Marnie"}) +The transaction is opened with a deferred ``BEGIN`` - SQLite takes the necessary locks when the first statement inside the block runs. + +The same savepoint mechanism used for nesting applies if the connection is already inside a transaction when ``db.atomic()`` is called - for example if you executed ``BEGIN`` yourself, or made writes using raw ``db.execute()`` calls. In that case the block becomes a savepoint within your transaction: exiting the block releases the savepoint, but nothing is committed to disk until your outer transaction commits. + +``db.atomic()`` is designed for connections in Python's default transaction handling mode. Connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options are not supported, because ``commit()`` and ``rollback()`` behave differently on those connections. + .. _python_api_parameters: Passing parameters diff --git a/tests/test_atomic.py b/tests/test_atomic.py index acd5474..8c4d18d 100644 --- a/tests/test_atomic.py +++ b/tests/test_atomic.py @@ -172,3 +172,20 @@ def test_transform_detects_foreign_key_check_violations(fresh_db): assert fresh_db["books"].foreign_keys == [] assert fresh_db.conn.execute("PRAGMA foreign_keys").fetchone()[0] + + +def test_atomic_inside_manual_transaction_uses_savepoint(fresh_db): + fresh_db["t"].insert({"id": 1}, pk="id") + fresh_db.execute("begin") + with fresh_db.atomic(): + fresh_db["t"].insert({"id": 2}, pk="id") + # Nothing is committed until the user's own transaction commits + assert fresh_db.conn.in_transaction + fresh_db.conn.rollback() + assert [r["id"] for r in fresh_db["t"].rows] == [1] + # And with a commit instead, the atomic block's writes persist + fresh_db.execute("begin") + with fresh_db.atomic(): + fresh_db["t"].insert({"id": 3}, pk="id") + fresh_db.conn.commit() + assert [r["id"] for r in fresh_db["t"].rows] == [1, 3] From 0bad21280fc29e42a58358a68f702cd270bb9094 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:08:53 +0000 Subject: [PATCH 15/34] migrate --stop-before: validate names and fix legacy compatibility An unknown --stop-before value previously matched nothing and every migration was silently applied, including the one the user meant to stop before. The CLI now errors unless each value matches a known migration. The CLI also always passed stop_before as a list, but older duck-typed sqlite-migrate Migrations objects compare it against a single string name - so the flag was silently ignored for exactly the migration files the compatibility shim exists to support. Legacy sets now receive a single string, with an error if multiple values target one legacy set. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/migrations.rst | 2 + sqlite_utils/cli.py | 35 +++++++++-- tests/test_cli_migrate.py | 125 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 4 deletions(-) diff --git a/docs/migrations.rst b/docs/migrations.rst index 5628ded..eddac5f 100644 --- a/docs/migrations.rst +++ b/docs/migrations.rst @@ -157,6 +157,8 @@ You can also target a specific migration set using ``migration_set:migration_nam The ``--stop-before`` option can be passed more than once. +If a ``--stop-before`` value does not match any known migration the command exits with an error, rather than silently applying everything. + Verbose output ============== diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index d51ba7b..23f04a7 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -3400,11 +3400,38 @@ def migrate(db_path, migrations, stop_before, list_, verbose): click.echo("\nSchema before:\n") click.echo(textwrap.indent(prev_schema, " ") or " (empty)") click.echo() + if stop_before: + # Every --stop-before value must match at least one known migration + known_names = set() + for migration_set in migration_sets: + names = {m.name for m in migration_set.pending(db)} + names.update(m.name for m in migration_set.applied(db)) + known_names.update(names) + known_names.update( + "{}:{}".format(migration_set.name, name) for name in names + ) + unknown = [value for value in stop_before if value not in known_names] + if unknown: + raise click.ClickException( + "--stop-before did not match any migrations: {}".format( + ", ".join(unknown) + ) + ) for migration_set in migration_sets: - migration_set.apply( - db, - stop_before=_stop_before_for_migration_set(stop_before, migration_set.name), - ) + matches = _stop_before_for_migration_set(stop_before, migration_set.name) + if isinstance(migration_set, sqlite_utils.Migrations): + migration_set.apply(db, stop_before=matches) + else: + # Legacy sqlite-migrate Migrations objects take a single string + # for stop_before, not a list + distinct = list(dict.fromkeys(matches)) + if len(distinct) > 1: + raise click.ClickException( + "Migration set '{}' uses the older sqlite-migrate class, " + "which only supports a single --stop-before value - " + "got: {}".format(migration_set.name, ", ".join(distinct)) + ) + migration_set.apply(db, stop_before=distinct[0] if distinct else None) if verbose: click.echo("Schema after:\n") post_schema = db.schema diff --git a/tests/test_cli_migrate.py b/tests/test_cli_migrate.py index 229bc9c..4a2ca9c 100644 --- a/tests/test_cli_migrate.py +++ b/tests/test_cli_migrate.py @@ -301,3 +301,128 @@ def test_stop_before_multiple_qualified(two_sets_same_migration_name): assert not db["creature_weights"].exists() assert db["sales"].exists() assert not db["sales_weights"].exists() + + +LEGACY_MIGRATIONS = """ +import datetime + +class _Migration: + def __init__(self, name, fn): + self.name = name + self.fn = fn + +class _Applied: + def __init__(self, name, applied_at): + self.name = name + self.applied_at = applied_at + +class LegacyMigrations: + # Mimics the sqlite-migrate 0.x Migrations class, in particular + # apply(db, stop_before=None) taking a single string + migrations_table = "_sqlite_migrations" + + def __init__(self, name): + self.name = name + self._migrations = [] + + def __call__(self, fn): + self._migrations.append(_Migration(fn.__name__, fn)) + return fn + + def ensure_migrations_table(self, db): + db[self.migrations_table].create( + {"migration_set": str, "name": str, "applied_at": str}, + pk=("migration_set", "name"), + if_not_exists=True, + ) + + def applied(self, db): + self.ensure_migrations_table(db) + return [ + _Applied(row["name"], row["applied_at"]) + for row in db[self.migrations_table].rows_where( + "migration_set = ?", [self.name] + ) + ] + + def pending(self, db): + applied = {m.name for m in self.applied(db)} + return [m for m in self._migrations if m.name not in applied] + + def apply(self, db, stop_before=None): + for migration in self.pending(db): + if migration.name == stop_before: + return + migration.fn(db) + db[self.migrations_table].insert( + { + "migration_set": self.name, + "name": migration.name, + "applied_at": str( + datetime.datetime.now(datetime.timezone.utc) + ), + } + ) + +legacy = LegacyMigrations("legacy_set") + +@legacy +def first(db): + db["first"].insert({"hello": "world"}) + +@legacy +def second(db): + db["second"].insert({"hello": "world"}) +""" + + +def test_stop_before_unknown_name_errors(two_migrations): + path, _ = two_migrations + db_path = str(path / "test.db") + runner = CliRunner() + result = runner.invoke( + sqlite_utils.cli.cli, + ["migrate", db_path, str(path), "--stop-before", "fooo"], + ) + assert result.exit_code == 1 + assert "--stop-before did not match any migrations: fooo" in result.output + # Nothing should have been applied + db = sqlite_utils.Database(db_path) + assert "foo" not in db.table_names() + assert "bar" not in db.table_names() + + +def test_stop_before_with_legacy_migrations_class(tmpdir): + path = pathlib.Path(tmpdir) + (path / "migrations.py").write_text(LEGACY_MIGRATIONS, "utf-8") + db_path = str(path / "test.db") + runner = CliRunner() + result = runner.invoke( + sqlite_utils.cli.cli, + ["migrate", db_path, str(path), "--stop-before", "second"], + ) + assert result.exit_code == 0, result.output + db = sqlite_utils.Database(db_path) + assert "first" in db.table_names() + assert "second" not in db.table_names() + + +def test_stop_before_multiple_values_for_legacy_set_errors(tmpdir): + path = pathlib.Path(tmpdir) + (path / "migrations.py").write_text(LEGACY_MIGRATIONS, "utf-8") + db_path = str(path / "test.db") + runner = CliRunner() + result = runner.invoke( + sqlite_utils.cli.cli, + [ + "migrate", + db_path, + str(path), + "--stop-before", + "legacy_set:first", + "--stop-before", + "legacy_set:second", + ], + ) + assert result.exit_code == 1 + assert "single --stop-before" in result.output From 30cc95c0a6c4fc1947f43feb1dc9798305a1b9e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:10:56 +0000 Subject: [PATCH 16/34] migrate --list is now read-only pending() and applied() no longer create the _sqlite_migrations table or perform the one-way legacy schema upgrade - that now only happens in apply(). The CLI --list path no longer creates the database file when it does not exist. applied() results are now explicitly ordered by insertion. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/migrations.rst | 2 +- sqlite_utils/cli.py | 12 +++++++++--- sqlite_utils/migrations.py | 22 +++++++++++----------- tests/test_cli_migrate.py | 37 +++++++++++++++++++++++++++++++++++++ tests/test_migrations.py | 8 ++++++++ 6 files changed, 67 insertions(+), 15 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 0c409a1..1e268ea 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -17,6 +17,7 @@ Unreleased - ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) - ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``RuntimeError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. +- Improvements to the ``sqlite-utils migrate`` command: ``--stop-before`` values that do not match any known migration are now an error instead of being silently ignored, ``--stop-before`` now works correctly with migration files that still use the older ``sqlite_migrate.Migrations`` class, and ``--list`` is now a read-only operation that no longer creates the database file or the migrations tracking table. ``migrations.applied()`` now returns migrations in the order they were applied. .. _v4_0rc1: diff --git a/docs/migrations.rst b/docs/migrations.rst index eddac5f..61709c4 100644 --- a/docs/migrations.rst +++ b/docs/migrations.rst @@ -117,7 +117,7 @@ Running the command repeatedly is safe. Migrations that already have a matching Listing migrations ================== -Use ``--list`` to show applied and pending migrations without running them: +Use ``--list`` to show applied and pending migrations without running them. This is a read-only operation - it will not create the database file or the ``_sqlite_migrations`` table: .. code-block:: bash diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index 23f04a7..1c94806 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -3387,13 +3387,19 @@ def migrate(db_path, migrations, stop_before, list_, verbose): if not migration_sets: raise click.ClickException("No migrations.py files found") - db = sqlite_utils.Database(db_path) - _register_db_for_cleanup(db) - if list_: + if pathlib.Path(db_path).exists(): + db = sqlite_utils.Database(db_path) + else: + # Listing is read-only - don't create the database file + db = sqlite_utils.Database(memory=True) + _register_db_for_cleanup(db) _display_migration_list(db, migration_sets) return + db = sqlite_utils.Database(db_path) + _register_db_for_cleanup(db) + prev_schema = db.schema if verbose: click.echo("Migrating {}".format(db_path)) diff --git a/sqlite_utils/migrations.py b/sqlite_utils/migrations.py index 20572af..7ae72e6 100644 --- a/sqlite_utils/migrations.py +++ b/sqlite_utils/migrations.py @@ -54,14 +54,10 @@ class Migrations: def pending(self, db: "Database") -> list["Migrations._Migration"]: """ Return a list of pending migrations. + + This is a read-only operation - it does not write to the database. """ - self.ensure_migrations_table(db) - already_applied = { - r["name"] - for r in db[self.migrations_table].rows_where( - "migration_set = ?", [self.name] - ) - } + already_applied = {migration.name for migration in self.applied(db)} return [ migration for migration in self._migrations @@ -70,13 +66,17 @@ class Migrations: def applied(self, db: "Database") -> list["Migrations._AppliedMigration"]: """ - Return a list of applied migrations. + Return a list of applied migrations, in the order they were applied. + + This is a read-only operation - it does not write to the database. """ - self.ensure_migrations_table(db) + table = _table(db, self.migrations_table) + if not table.exists(): + return [] return [ self._AppliedMigration(name=row["name"], applied_at=row["applied_at"]) - for row in db[self.migrations_table].rows_where( - "migration_set = ?", [self.name] + for row in table.rows_where( + "migration_set = ?", [self.name], order_by="rowid" ) ] diff --git a/tests/test_cli_migrate.py b/tests/test_cli_migrate.py index 4a2ca9c..1cdf8e7 100644 --- a/tests/test_cli_migrate.py +++ b/tests/test_cli_migrate.py @@ -426,3 +426,40 @@ def test_stop_before_multiple_values_for_legacy_set_errors(tmpdir): ) assert result.exit_code == 1 assert "single --stop-before" in result.output + + +def test_list_does_not_create_database_file(two_migrations): + path, _ = two_migrations + db_path = path / "test.db" + runner = CliRunner() + result = runner.invoke( + sqlite_utils.cli.cli, ["migrate", str(db_path), str(path), "--list"] + ) + assert result.exit_code == 0, result.output + assert "Pending:\n foo\n bar" in result.output + # Listing migrations must not create the database file + assert not db_path.exists() + + +def test_list_does_not_upgrade_legacy_migrations_table(two_migrations): + path, _ = two_migrations + db_path = str(path / "test.db") + db = sqlite_utils.Database(db_path) + db["_sqlite_migrations"].create( + {"migration_set": str, "name": str, "applied_at": str}, + pk=("migration_set", "name"), + ) + db["_sqlite_migrations"].insert( + {"migration_set": "hello", "name": "foo", "applied_at": "x"} + ) + db.close() + runner = CliRunner() + result = runner.invoke( + sqlite_utils.cli.cli, ["migrate", db_path, str(path), "--list"] + ) + assert result.exit_code == 0, result.output + assert "foo - x" in result.output + # --list must not perform the one-way legacy schema upgrade + db2 = sqlite_utils.Database(db_path) + assert db2["_sqlite_migrations"].pks == ["migration_set", "name"] + db2.close() diff --git a/tests/test_migrations.py b/tests/test_migrations.py index c190fd5..1733aa4 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -190,3 +190,11 @@ def test_upgrades_sqlite_migrations(migrations, create_table, pk): assert db["_sqlite_migrations"].pks == ([pk] if isinstance(pk, str) else list(pk)) migrations.apply(db) assert db["_sqlite_migrations"].pks == ["id"] + + +def test_pending_and_applied_are_read_only(migrations): + db = sqlite_utils.Database(memory=True) + assert [m.name for m in migrations.pending(db)] == ["m001", "m002"] + assert migrations.applied(db) == [] + # Neither call should have created the tracking table + assert db.table_names() == [] From 397cdcc491342825e460cb9a08ef06da7552a409 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:13:15 +0000 Subject: [PATCH 17/34] Polish: clearer errors, corrected docstrings, setuptools pin - insert/upsert into a name that is actually a view now shows a clean error instead of a traceback - db.view() on a name that is a table now says so in its error - Database.__getitem__ docstring documents that views are returned for view names; hash_id docstring corrected (it is a column name, not a boolean) - Registering two migrations with the same name in one set now raises ValueError at registration time instead of executing both and failing with IntegrityError at apply time - build-system requires setuptools>=77, needed for the PEP 639 license expression Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- pyproject.toml | 3 ++- sqlite_utils/cli.py | 2 ++ sqlite_utils/db.py | 17 +++++++++++++---- sqlite_utils/migrations.py | 9 ++++++++- tests/test_cli_insert.py | 13 +++++++++++++ tests/test_create.py | 5 ++++- tests/test_migrations.py | 16 ++++++++++++++++ 7 files changed, 58 insertions(+), 7 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 2c86f85..4ad1bd9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,7 +72,8 @@ CI = "https://github.com/simonw/sqlite-utils/actions" sqlite-utils = "sqlite_utils.cli:cli" [build-system] -requires = ["setuptools"] +# setuptools 77+ is needed for the PEP 639 license = "Apache-2.0" expression +requires = ["setuptools>=77"] build-backend = "setuptools.build_meta" [tool.flake8] diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index 1c94806..f2e1ba6 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -1155,6 +1155,8 @@ def insert_upsert_implementation( db.table(table).insert_all( docs, pk=pk, batch_size=batch_size, alter=alter, **extra_kwargs ) + except NoTable as e: + raise click.ClickException(str(e)) except Exception as e: if ( isinstance(e, OperationalError) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 97e8920..b53df51 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -499,10 +499,12 @@ class Database: def __getitem__(self, table_name: str) -> Union["Table", "View"]: """ - ``db[table_name]`` returns a :class:`.Table` object for the table with the specified name. - If the table does not exist yet it will be created the first time data is inserted into it. + ``db[name]`` returns a :class:`.Table` object for the table with the specified name, + or a :class:`.View` object if the name matches an existing SQL view. + If neither exists yet, a table is assumed - it will be created the first + time data is inserted into it. - :param table_name: The name of the table + :param table_name: The name of the table or view """ if table_name in self.view_names(): return self.view(table_name) @@ -672,6 +674,12 @@ class Database: :param view_name: Name of the view """ if view_name not in self.view_names(): + if view_name in self.table_names(): + raise NoView( + "View {name} does not exist - {name} is a table".format( + name=view_name + ) + ) raise NoView("View {} does not exist".format(view_name)) return View(self, view_name) @@ -1618,7 +1626,8 @@ class Table(Queryable): :param not_null: List of columns that cannot be null :param defaults: Dictionary of column names and default values :param batch_size: Integer number of rows to insert at a time - :param hash_id: If True, use a hash of the row values as the primary key + :param hash_id: Name of a column to create and use as a primary key, where the + value of that primary key is derived from a hash of the row values :param hash_id_columns: List of columns to use for the hash_id :param alter: If True, automatically alter the table if it doesn't match the schema :param ignore: If True, ignore rows that already exist when inserting diff --git a/sqlite_utils/migrations.py b/sqlite_utils/migrations.py index 7ae72e6..36e053d 100644 --- a/sqlite_utils/migrations.py +++ b/sqlite_utils/migrations.py @@ -44,8 +44,15 @@ class Migrations: """ def inner(func: Callable) -> Callable: + migration_name = name or getattr(func, "__name__") + if any(m.name == migration_name for m in self._migrations): + raise ValueError( + "Migration '{}' is already registered in set '{}'".format( + migration_name, self.name + ) + ) self._migrations.append( - self._Migration(name or getattr(func, "__name__"), func, transactional) + self._Migration(migration_name, func, transactional) ) return func diff --git a/tests/test_cli_insert.py b/tests/test_cli_insert.py index 5812851..2df1e0c 100644 --- a/tests/test_cli_insert.py +++ b/tests/test_cli_insert.py @@ -615,3 +615,16 @@ def test_insert_csv_headers_only(tmpdir): # Table should not exist since there were no data rows db = Database(db_path) assert not db["data"].exists() + + +def test_insert_into_view_errors(tmpdir): + db_path = str(tmpdir / "test.db") + db = Database(db_path) + db["t"].insert({"id": 1}) + db.create_view("v", "select * from t") + db.close() + result = CliRunner().invoke( + cli.cli, ["insert", db_path, "v", "-"], input='{"id": 2}' + ) + assert result.exit_code == 1 + assert result.output.strip() == "Error: Table v is actually a view" diff --git a/tests/test_create.py b/tests/test_create.py index b1a6ad1..afa7a65 100644 --- a/tests/test_create.py +++ b/tests/test_create.py @@ -1382,7 +1382,10 @@ def test_bad_table_and_view_exceptions(fresh_db): assert ex.value.args[0] == "Table v is actually a view" with pytest.raises(NoView) as ex2: fresh_db.view("t") - assert ex2.value.args[0] == "View t does not exist" + assert ex2.value.args[0] == "View t does not exist - t is a table" + with pytest.raises(NoView) as ex3: + fresh_db.view("missing") + assert ex3.value.args[0] == "View missing does not exist" # Tests for issue #655: Table configuration should be stored in _defaults diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 1733aa4..5634d79 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -198,3 +198,19 @@ def test_pending_and_applied_are_read_only(migrations): assert migrations.applied(db) == [] # Neither call should have created the tracking table assert db.table_names() == [] + + +def test_duplicate_migration_name_errors(): + migrations = Migrations("test") + + @migrations() + def m001(db): + pass + + with pytest.raises(ValueError) as ex: + + @migrations(name="m001") + def m001_again(db): + pass + + assert "m001" in str(ex.value) From f1cdceaca9fb6e46eef12e16e06a6aad959160dc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:15:31 +0000 Subject: [PATCH 18/34] Remove the no-op -d/--detect-types flag from insert and upsert Type detection has been the default for CSV/TSV data since 4.0a1, making this flag a no-op kept only for backwards compatibility. 4.0 is the release where it can be removed - the flag now errors, prompting scripts to drop it. --no-detect-types is unchanged. The dead detect_types parameter has been removed from insert_upsert_implementation. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/cli-reference.rst | 2 -- sqlite_utils/cli.py | 14 +------------- tests/test_cli.py | 36 +++++++++++++++++++++--------------- 4 files changed, 23 insertions(+), 30 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1e268ea..ce1397f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,6 +9,7 @@ Unreleased ---------- +- **Breaking change**: The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. - **Breaking change**: ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. - Fixed a bug where ``table.delete_where()``, ``table.optimize()`` and ``table.rebuild_fts()`` did not commit their changes, leaving the connection inside an open transaction. Their work - and any subsequent writes - could then be silently rolled back when the connection was closed. All three now use ``db.atomic()``, consistent with the other write methods. - The ``sqlite-utils drop-table`` command now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. Both now exit with an error suggesting the correct command to use. diff --git a/docs/cli-reference.rst b/docs/cli-reference.rst index 48d0145..cbac48b 100644 --- a/docs/cli-reference.rst +++ b/docs/cli-reference.rst @@ -286,7 +286,6 @@ See :ref:`cli_inserting_data`, :ref:`cli_insert_csv_tsv`, :ref:`cli_insert_unstr --alter Alter existing table to add any missing columns --not-null TEXT Columns that should be created as NOT NULL --default ... Default value that should be set for a column - -d, --detect-types Detect types for columns in CSV/TSV data (default) --no-detect-types Treat all CSV/TSV columns as TEXT --analyze Run ANALYZE at the end of this operation --load-extension TEXT Path to SQLite extension, with optional :entrypoint @@ -344,7 +343,6 @@ See :ref:`cli_upsert`. --alter Alter existing table to add any missing columns --not-null TEXT Columns that should be created as NOT NULL --default ... Default value that should be set for a column - -d, --detect-types Detect types for columns in CSV/TSV data (default) --no-detect-types Treat all CSV/TSV columns as TEXT --analyze Run ANALYZE at the end of this operation --load-extension TEXT Path to SQLite extension, with optional :entrypoint diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index f2e1ba6..a2f881a 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -940,12 +940,6 @@ def insert_upsert_options(*, require_pk=False): type=(str, str), help="Default value that should be set for a column", ), - click.option( - "-d", - "--detect-types", - is_flag=True, - help="Detect types for columns in CSV/TSV data (default)", - ), click.option( "--no-detect-types", is_flag=True, @@ -1000,7 +994,6 @@ def insert_upsert_implementation( truncate=False, not_null=None, default=None, - detect_types=None, no_detect_types=False, analyze=False, load_extension=None, @@ -1067,7 +1060,7 @@ def insert_upsert_implementation( ) else: docs = (dict(zip(headers, row)) for row in reader) - # detect_types is now the default, unless --no-detect-types is passed + # Type detection is the default, unless --no-detect-types is passed if not no_detect_types: tracker = TypeTracker() docs = tracker.wrap(docs) @@ -1241,7 +1234,6 @@ def insert( batch_size, stop_after, alter, - detect_types, no_detect_types, analyze, load_extension, @@ -1324,7 +1316,6 @@ def insert( ignore=ignore, replace=replace, truncate=truncate, - detect_types=detect_types, no_detect_types=no_detect_types, analyze=analyze, load_extension=load_extension, @@ -1363,7 +1354,6 @@ def upsert( alter, not_null, default, - detect_types, no_detect_types, analyze, load_extension, @@ -1409,7 +1399,6 @@ def upsert( upsert=True, not_null=not_null, default=default, - detect_types=detect_types, no_detect_types=no_detect_types, analyze=analyze, load_extension=load_extension, @@ -1497,7 +1486,6 @@ def bulk( upsert=False, not_null=set(), default={}, - detect_types=False, no_detect_types=True, load_extension=load_extension, silent=False, diff --git a/tests/test_cli.py b/tests/test_cli.py index 02c00ab..d26e4dd 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2331,18 +2331,14 @@ def test_csv_insert_bom(tmpdir): ] -@pytest.mark.parametrize("option", (None, "-d", "--detect-types")) -def test_insert_detect_types(tmpdir, option): - """Test that type detection is now the default behavior""" +def test_insert_detect_types(tmpdir): + """Test that type detection is the default behavior""" db_path = str(tmpdir / "test.db") data = "name,age,weight\nCleo,6,45.5\nDori,1,3.5" - extra = [] - if option: - extra = [option] result = CliRunner().invoke( cli.cli, - ["insert", db_path, "creatures", "-", "--csv"] + extra, + ["insert", db_path, "creatures", "-", "--csv"], catch_exceptions=False, input=data, ) @@ -2354,17 +2350,27 @@ def test_insert_detect_types(tmpdir, option): ] -@pytest.mark.parametrize("option", (None, "-d", "--detect-types")) -def test_upsert_detect_types(tmpdir, option): - """Test that type detection is now the default behavior for upsert""" +@pytest.mark.parametrize("command", ("insert", "upsert")) +@pytest.mark.parametrize("option", ("-d", "--detect-types")) +def test_detect_types_flag_removed(tmpdir, command, option): + # The old no-op flag was removed in 4.0 - it should now error db_path = str(tmpdir / "test.db") - data = "id,name,age,weight\n1,Cleo,6,45.5\n2,Dori,1,3.5" - extra = [] - if option: - extra = [option] result = CliRunner().invoke( cli.cli, - ["upsert", db_path, "creatures", "-", "--csv", "--pk", "id"] + extra, + [command, db_path, "creatures", "-", "--csv", "--pk", "id", option], + input="id,name\n1,Cleo", + ) + assert result.exit_code == 2 + assert "No such option" in result.output + + +def test_upsert_detect_types(tmpdir): + """Test that type detection is the default behavior for upsert""" + db_path = str(tmpdir / "test.db") + data = "id,name,age,weight\n1,Cleo,6,45.5\n2,Dori,1,3.5" + result = CliRunner().invoke( + cli.cli, + ["upsert", db_path, "creatures", "-", "--csv", "--pk", "id"], catch_exceptions=False, input=data, ) From a2e6ea2eca5f2a49c116c9af1a37aba4b1a7eaac Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:19:24 +0000 Subject: [PATCH 19/34] Raise ValueError instead of AssertionError for invalid arguments User-facing argument validation in db.py previously used bare assert statements, which vanish entirely under python -O and raise AssertionError - an exception type callers should not have to catch for input validation. Fifteen validation sites now raise ValueError with the same messages. Internal invariants (an unreachable branch and a post-update rowcount check) remain asserts. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + sqlite_utils/db.py | 123 ++++++++++++++++++++++---------------- tests/test_convert.py | 2 +- tests/test_create.py | 12 ++-- tests/test_create_view.py | 2 +- tests/test_m2m.py | 4 +- tests/test_recreate.py | 2 +- 7 files changed, 83 insertions(+), 63 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ce1397f..0b41875 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,6 +9,7 @@ Unreleased ---------- +- **Breaking change**: Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. - **Breaking change**: The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. - **Breaking change**: ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. - Fixed a bug where ``table.delete_where()``, ``table.optimize()`` and ``table.rebuild_fts()`` did not commit their changes, leaving the connection inside an open transaction. Their work - and any subsequent writes - could then be silently rolled back when the connection was closed. All three now use ``db.atomic()``, consistent with the other write methods. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index b53df51..a0ca6ed 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -367,9 +367,11 @@ class Database: self.memory_name = None self.memory = False self.use_old_upsert = use_old_upsert - assert (filename_or_conn is not None and (not memory and not memory_name)) or ( - filename_or_conn is None and (memory or memory_name) - ), "Either specify a filename_or_conn or pass memory=True" + if not ( + (filename_or_conn is not None and (not memory and not memory_name)) + or (filename_or_conn is None and (memory or memory_name)) + ): + raise ValueError("Either specify a filename_or_conn or pass memory=True") if memory_name: uri = "file:{}?mode=memory&cache=shared".format(memory_name) self.conn = sqlite3.connect( @@ -393,7 +395,8 @@ class Database: raise self.conn = sqlite3.connect(str(filename_or_conn)) else: - assert not recreate, "recreate cannot be used with connections, only paths" + if recreate: + raise ValueError("recreate cannot be used with connections, only paths") self.conn = cast(sqlite3.Connection, filename_or_conn) self._tracer: Optional[Tracer] = tracer if recursive_triggers: @@ -968,20 +971,21 @@ class Database: other_column = table.guess_foreign_column(other_table) fks.append(ForeignKey(name, column, other_table, other_column)) return fks - assert all( - isinstance(fk, (tuple, list)) for fk in foreign_keys - ), "foreign_keys= should be a list of tuples" + if not all(isinstance(fk, (tuple, list)) for fk in foreign_keys): + raise ValueError("foreign_keys= should be a list of tuples") fks = [] for tuple_or_list in foreign_keys: if len(tuple_or_list) == 4: - assert ( - tuple_or_list[0] == name - ), "First item in {} should have been {}".format(tuple_or_list, name) - assert len(tuple_or_list) in ( - 2, - 3, - 4, - ), "foreign_keys= should be a list of tuple pairs or triples" + if tuple_or_list[0] != name: + raise ValueError( + "First item in {} should have been {}".format( + tuple_or_list, name + ) + ) + if len(tuple_or_list) not in (2, 3, 4): + raise ValueError( + "foreign_keys= should be a list of tuple pairs or triples" + ) if len(tuple_or_list) in (3, 4): if len(tuple_or_list) == 4: tuple_or_list = cast(Tuple[str, str, str], tuple_or_list[1:]) @@ -1054,17 +1058,20 @@ class Database: # Soundness check not_null, and defaults if provided not_null = not_null or set() defaults = defaults or {} - assert columns, "Tables must have at least one column" - assert all( - n in columns for n in not_null - ), "not_null set {} includes items not in columns {}".format( - repr(not_null), repr(set(columns.keys())) - ) - assert all( - n in columns for n in defaults - ), "defaults set {} includes items not in columns {}".format( - repr(set(defaults)), repr(set(columns.keys())) - ) + if not columns: + raise ValueError("Tables must have at least one column") + if not all(n in columns for n in not_null): + raise ValueError( + "not_null set {} includes items not in columns {}".format( + repr(not_null), repr(set(columns.keys())) + ) + ) + if not all(n in columns for n in defaults): + raise ValueError( + "defaults set {} includes items not in columns {}".format( + repr(set(defaults)), repr(set(columns.keys())) + ) + ) column_items = list(columns.items()) if column_order is not None: @@ -1295,9 +1302,8 @@ class Database: :param ignore: Set to ``True`` to do nothing if a view with this name already exists :param replace: Set to ``True`` to replace the view if one with this name already exists """ - assert not ( - ignore and replace - ), "Use one or the other of ignore/replace, not both" + if ignore and replace: + raise ValueError("Use one or the other of ignore/replace, not both") create_sql = "CREATE VIEW {name} AS {sql}".format( name=quote_identifier(name), sql=sql ) @@ -1342,9 +1348,13 @@ class Database: tuples """ # foreign_keys is a list of explicit 4-tuples - assert all( + if not all( len(fk) == 4 and isinstance(fk, (list, tuple)) for fk in foreign_keys - ), "foreign_keys must be a list of 4-tuples, (table, column, other_table, other_column)" + ): + raise ValueError( + "foreign_keys must be a list of 4-tuples, " + "(table, column, other_table, other_column)" + ) foreign_keys_to_create = [] @@ -1989,7 +1999,8 @@ class Table(Queryable): :param keep_table: If specified, the existing table will be renamed to this and will not be dropped """ - assert self.exists(), "Cannot transform a table that doesn't exist yet" + if not self.exists(): + raise ValueError("Cannot transform a table that doesn't exist yet") sqls = self.transform_sql( types=types, rename=rename, @@ -2161,8 +2172,10 @@ class Table(Queryable): elif not not_null: pass else: - assert False, "not_null must be a dict or a set or None, it was {}".format( - repr(not_null) + raise ValueError( + "not_null must be a dict or a set or None, it was {}".format( + repr(not_null) + ) ) # defaults= create_table_defaults = { @@ -2870,9 +2883,10 @@ class Table(Queryable): "{}.{}".format(original_quoted, quote_identifier(c)) for c in columns ) fts_table = self.detect_fts() - assert fts_table, "Full-text search is not configured for table '{}'".format( - self.name - ) + if not fts_table: + raise ValueError( + "Full-text search is not configured for table '{}'".format(self.name) + ) fts_table_quoted = quote_identifier(fts_table) virtual_table_using = self.db.table(fts_table).virtual_table_using sql = textwrap.dedent(""" @@ -3119,7 +3133,8 @@ class Table(Queryable): ) if output is not None: - assert len(columns) == 1, "output= can only be used with a single column" + if len(columns) != 1: + raise ValueError("output= can only be used with a single column") if output not in self.columns_dict: self.add_column(output, output_type or "text") @@ -3632,15 +3647,15 @@ class Table(Queryable): if upsert and (not pk and not hash_id): raise PrimaryKeyRequired("upsert() requires a pk") - assert not (hash_id and pk), "Use either pk= or hash_id=" + if hash_id and pk: + raise ValueError("Use either pk= or hash_id=") if hash_id_columns and (hash_id is None): hash_id = "id" if hash_id: pk = hash_id - assert not ( - ignore and replace - ), "Use either ignore=True or replace=True, not both" + if ignore and replace: + raise ValueError("Use either ignore=True or replace=True, not both") all_columns = [] first = True num_records_processed = 0 @@ -3689,9 +3704,10 @@ class Table(Queryable): first_record = cast(Dict[str, Any], first_record) num_columns = len(first_record.keys()) - assert ( - num_columns <= SQLITE_MAX_VARS - ), "Rows can have a maximum of {} columns".format(SQLITE_MAX_VARS) + if num_columns > SQLITE_MAX_VARS: + raise ValueError( + "Rows can have a maximum of {} columns".format(SQLITE_MAX_VARS) + ) batch_size = ( 1 if num_columns == 0 @@ -3946,10 +3962,12 @@ class Table(Queryable): :param extra_values: Additional column values to be used only if creating a new record :param strict: Boolean, apply STRICT mode if creating the table. """ - assert isinstance(lookup_values, dict) - assert pk is not None - if extra_values is not None: - assert isinstance(extra_values, dict) + if not isinstance(lookup_values, dict): + raise ValueError("lookup_values must be a dictionary") + if pk is None: + raise ValueError("pk cannot be None") + if extra_values is not None and not isinstance(extra_values, dict): + raise ValueError("extra_values must be a dictionary") combined_values = dict(lookup_values) if extra_values is not None: combined_values.update(extra_values) @@ -4034,9 +4052,10 @@ class Table(Queryable): other_table = self.db.table(other_table, pk=pk) our_id = self.last_pk if lookup is not None: - assert record_or_iterable is None, "Provide lookup= or record, not both" - else: - assert record_or_iterable is not None, "Provide lookup= or record, not both" + if record_or_iterable is not None: + raise ValueError("Provide lookup= or record, not both") + elif record_or_iterable is None: + raise ValueError("Provide lookup= or record, not both") tables = list(sorted([self.name, other_table.name])) columns = ["{}_id".format(t) for t in tables] if m2m_table is not None: diff --git a/tests/test_convert.py b/tests/test_convert.py index 848b39a..ea3fd96 100644 --- a/tests/test_convert.py +++ b/tests/test_convert.py @@ -77,7 +77,7 @@ def test_convert_output(fresh_db, drop, expected): def test_convert_output_multiple_column_error(fresh_db): table = fresh_db["table"] - with pytest.raises(AssertionError) as excinfo: + with pytest.raises(ValueError) as excinfo: table.convert(["title", "other"], lambda v: v, output="out") assert "output= can only be used with a single column" in str(excinfo.value) diff --git a/tests/test_create.py b/tests/test_create.py index afa7a65..decefcf 100644 --- a/tests/test_create.py +++ b/tests/test_create.py @@ -108,7 +108,7 @@ def test_create_table_with_defaults(fresh_db): def test_create_table_with_bad_not_null(fresh_db): - with pytest.raises(AssertionError): + with pytest.raises(ValueError): fresh_db.create_table( "players", {"name": str, "score": int}, not_null={"mouse"} ) @@ -243,11 +243,11 @@ def test_create_table_column_order(fresh_db, use_table_factory): # If you specify a column that doesn't point to a table, you get an error: (("one_id", "two_id", "three_id"), NoObviousTable), # Tuples of the wrong length get an error: - ((("one_id", "one", "id", "five"), ("two_id", "two", "id")), AssertionError), + ((("one_id", "one", "id", "five"), ("two_id", "two", "id")), ValueError), # Likewise a bad column: ((("one_id", "one", "id2"),), AlterError), # Or a list of dicts - (({"one_id": "one"},), AssertionError), + (({"one_id": "one"},), ValueError), ), ) @pytest.mark.parametrize("use_table_factory", [True, False]) @@ -700,7 +700,7 @@ def test_bulk_insert_more_than_999_values(fresh_db): def test_error_if_more_than_999_columns(fresh_db, num_columns, should_error): record = dict([("c{}".format(i), i) for i in range(num_columns)]) if should_error: - with pytest.raises(AssertionError): + with pytest.raises(ValueError): fresh_db["big"].insert(record) else: fresh_db["big"].insert(record) @@ -1061,7 +1061,7 @@ def test_create_table_numpy(fresh_db): def test_cannot_provide_both_filename_and_memory(): with pytest.raises( - AssertionError, match="Either specify a filename_or_conn or pass memory=True" + ValueError, match="Either specify a filename_or_conn or pass memory=True" ): Database("/tmp/foo.db", memory=True) @@ -1214,7 +1214,7 @@ def test_create_if_not_exists(fresh_db): def test_create_if_no_columns(fresh_db): - with pytest.raises(AssertionError) as error: + with pytest.raises(ValueError) as error: fresh_db["t"].create({}) assert error.value.args[0] == "Tables must have at least one column" diff --git a/tests/test_create_view.py b/tests/test_create_view.py index f0e2855..056e246 100644 --- a/tests/test_create_view.py +++ b/tests/test_create_view.py @@ -15,7 +15,7 @@ def test_create_view_error(fresh_db): def test_create_view_only_arrow_one_param(fresh_db): - with pytest.raises(AssertionError): + with pytest.raises(ValueError): fresh_db.create_view("bar", "select 1 + 2", ignore=True, replace=True) diff --git a/tests/test_m2m.py b/tests/test_m2m.py index 2cb2ca3..d613bb9 100644 --- a/tests/test_m2m.py +++ b/tests/test_m2m.py @@ -139,9 +139,9 @@ def test_m2m_lookup(fresh_db): def test_m2m_requires_either_records_or_lookup(fresh_db): people = fresh_db.table("people", pk="id").insert({"name": "Wahyu"}) - with pytest.raises(AssertionError): + with pytest.raises(ValueError): people.m2m("tags") - with pytest.raises(AssertionError): + with pytest.raises(ValueError): people.m2m("tags", {"tag": "hello"}, lookup={"foo": "bar"}) diff --git a/tests/test_recreate.py b/tests/test_recreate.py index 224d182..bce53d5 100644 --- a/tests/test_recreate.py +++ b/tests/test_recreate.py @@ -15,7 +15,7 @@ def test_recreate_ignored_for_in_memory(): def test_recreate_not_allowed_for_connection(): conn = sqlite3.connect(":memory:") try: - with pytest.raises(AssertionError): + with pytest.raises(ValueError): Database(conn, recreate=True) finally: conn.close() From 910a90970dc41748fd92ae5026baa0ac0315c824 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:21:34 +0000 Subject: [PATCH 20/34] Add upgrading guide to the documentation New docs page describing the changes needed to upgrade between major versions, with a detailed 3.x to 4.0 section covering CLI, Python API and packaging changes, plus brief sections for the 2.x to 3.0 and 1.x to 2.0 upgrades. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/index.rst | 1 + docs/upgrading.rst | 113 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 docs/upgrading.rst diff --git a/docs/index.rst b/docs/index.rst index 052b790..f190d47 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -40,5 +40,6 @@ Contents plugins reference cli-reference + upgrading contributing changelog diff --git a/docs/upgrading.rst b/docs/upgrading.rst new file mode 100644 index 0000000..36d720b --- /dev/null +++ b/docs/upgrading.rst @@ -0,0 +1,113 @@ +.. _upgrading: + +=========== + Upgrading +=========== + +This page describes the changes you may need to make to your own code or scripts when upgrading between major versions of ``sqlite-utils``. + +For the full list of changes in every release see the :ref:`changelog`. + +.. _upgrading_3_to_4: + +Upgrading from 3.x to 4.0 +========================= + +Requirements +------------ + +- Python 3.10 or higher is required. +- The ``click`` dependency must be version 8.3.1 or later. + +Command-line changes +-------------------- + +**Type detection is now the default for CSV and TSV imports.** ``sqlite-utils insert`` and ``sqlite-utils upsert`` now detect column types when importing CSV or TSV data - previously every column was created as ``TEXT`` unless you passed ``--detect-types``. To restore the old behavior pass the new ``--no-detect-types`` flag: + +.. code-block:: bash + + sqlite-utils insert data.db rows data.csv --csv --no-detect-types + +Two related things have been removed: + +- The ``SQLITE_UTILS_DETECT_TYPES`` environment variable. +- The old ``-d/--detect-types`` flag itself. Since detection is now the default the flag did nothing - remove it from any scripts that used it. + +**The convert command no longer skips falsey values.** ``sqlite-utils convert`` previously skipped values that evaluated to ``False`` (empty strings, ``0``) unless you passed ``--no-skip-false``. All values are now converted and the ``--no-skip-false`` flag has been removed. + +**drop-table and drop-view check the object type.** ``sqlite-utils drop-table`` now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. If you relied on that (unlikely), use the matching command instead. + +**sqlite-utils tui has moved to a plugin.** The optional terminal interface is now provided by the `sqlite-utils-tui `__ plugin: + +.. code-block:: bash + + sqlite-utils install sqlite-utils-tui + +Python API changes +------------------ + +**db.table() no longer returns views.** ``db.table(name)`` now raises a ``sqlite_utils.db.NoTable`` exception if ``name`` is a SQL view. Use the new ``db.view(name)`` method for views: + +.. code-block:: python + + table = db.table("my_table") + view = db.view("my_view") + +``db["name"]`` still returns either a ``Table`` or a ``View`` depending on what exists in the database. + +**db.query() executes immediately.** ``db.query(sql)`` previously returned a generator that did not execute the SQL until you started iterating over it. The SQL now runs as soon as the method is called - rows are still fetched lazily. Two consequences: + +- Errors in your SQL now raise at the ``db.query()`` call site rather than on first iteration. +- Passing a statement that returns no rows - such as an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause - previously did nothing at all, silently. It now raises a ``ValueError``. Use ``db.execute()`` for statements that do not return rows. + +**Upserts use INSERT ... ON CONFLICT.** Upsert operations now use SQLite's ``INSERT ... ON CONFLICT SET`` syntax rather than the previous ``INSERT OR IGNORE`` followed by ``UPDATE``. If your code depends on the old behavior, pass ``use_old_upsert=True`` to the ``Database()`` constructor - see :ref:`python_api_old_upsert`. + +**Upsert records must include their primary keys.** ``table.upsert()`` and ``table.upsert_all()`` now raise ``sqlite_utils.db.PrimaryKeyRequired`` if a record is missing a value for any primary key column (or has ``None`` for one). Previously such records were quietly inserted as new rows. Relatedly, ``pk=`` is now optional when the table already exists with a primary key - it is detected automatically. + +**Floating point columns are now REAL.** Auto-detected floating point columns are created with the correct SQLite type ``REAL`` instead of ``FLOAT``. Code that inspects column types should expect ``REAL``. + +**Generated schemas use double quotes.** Tables created by this library now wrap table and column names in ``"double-quotes"`` where they previously used ``[square-braces]``. If you compare ``table.schema`` strings against expected values you will need to update them. + +**table.convert() no longer skips falsey values.** Matching the CLI change above, ``table.convert()`` now converts every value. The ``skip_false`` parameter has been removed - previously it defaulted to ``True``, skipping empty strings and other falsey values. + +**Validation errors raise ValueError.** Invalid arguments to Python API methods - for example ``create_table()`` with no columns, or ``ignore=True`` together with ``replace=True`` - now raise ``ValueError``. They previously raised ``AssertionError`` from bare ``assert`` statements, which were silently skipped under ``python -O``. + +**Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation. Changes you may notice: + +- Multi-step operations such as ``table.transform()`` no longer commit an existing transaction you have open - they use savepoints inside it instead. +- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``RuntimeError`` if called while a transaction is open, instead of silently committing it. +- Using ``Database`` as a context manager (``with Database(path) as db:``) closes the connection on exit *without* committing - uncommitted changes from raw ``db.execute()`` calls are rolled back. + +Packaging changes +----------------- + +- ``sqlite-utils`` now uses ``pyproject.toml`` in place of ``setup.py``. +- ``pip`` is now a runtime dependency, used by the ``sqlite-utils install`` and ``uninstall`` commands. + +New features to be aware of +--------------------------- + +Not breaking changes, but new in 4.0 and worth knowing about when you upgrade: + +- A :ref:`database migrations system `, incorporating the functionality of the ``sqlite-migrate`` plugin. If you used that plugin, the built-in system reads the same ``_sqlite_migrations`` table - your applied migrations will not run again. Update your migration files to use ``from sqlite_utils import Migrations``. +- :ref:`db.atomic() ` for nested transaction support. +- ``table.insert_all()`` and ``table.upsert_all()`` accept an iterator of lists or tuples as an alternative to dictionaries - see :ref:`python_api_insert_lists`. + +.. _upgrading_2_to_3: + +Upgrading from 2.x to 3.0 +========================= + +The 3.0 release redesigned search. The breaking changes were minor: + +- ``table.search()`` returns a generator of dictionaries, sorted by relevance. It previously returned a list of tuples sorted by ``rowid``. +- The ``-c`` shortcut for ``--csv`` and the ``-f`` shortcut for ``--fmt`` were removed from the CLI - use the full option names. + +.. _upgrading_1_to_2: + +Upgrading from 1.x to 2.0 +========================= + +The 2.0 release changed the meaning of *upsert*. In 1.x, ``table.upsert()`` and ``table.upsert_all()`` actually performed ``INSERT OR REPLACE`` operations - entirely replacing the existing row. Since 2.0 an upsert updates only the columns you provide, leaving other columns untouched. + +If you want the 1.x behavior, use ``table.insert(..., replace=True)`` or ``table.insert_all(..., replace=True)`` instead. From 509d87b292407ad7350b8fd0e1c57c27b39b7ca8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:56:42 +0000 Subject: [PATCH 21/34] Remove review notes files These were working documents for the 4.0 pre-release review - the actionable findings have all been fixed on this branch and the changelog and upgrading guide carry the user-facing record. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- create-table-parser-feedback.md | 167 -------------- fable-review-4.0rc1.md | 375 -------------------------------- 2 files changed, 542 deletions(-) delete mode 100644 create-table-parser-feedback.md delete mode 100644 fable-review-4.0rc1.md diff --git a/create-table-parser-feedback.md b/create-table-parser-feedback.md deleted file mode 100644 index 2b6f727..0000000 --- a/create-table-parser-feedback.md +++ /dev/null @@ -1,167 +0,0 @@ -# Review: `create-table-parser` branch - -Feedback on the `create-table-parser` branch (commits `ed1de15`, `95d6ff4`, -`407db59` — 819-line `create_table_parser.py` plus JSON test corpora), -reviewed as the proposed foundation for 4.1 `transform()` improvements that -make UNIQUE and CHECK constraints supported instead of silently dropped. - -**Verdict: good fit for 4.1 — right design, right release — but it has two -silent-corruption bugs that must be fixed before `transform()` can trust it, -and a few design decisions to settle.** - -## Why 4.1 is the right home - -- The parser is purely additive: a new module and a new capability. Nothing - about it needs to land in 4.0, so it does not constrain or delay the - stable release. -- The `transform()` improvement it enables — preserving CHECK and UNIQUE - constraints instead of silently dropping them — is a behavior change in - the bug-fix direction, appropriate for a minor release with a prominent - changelog note. -- SQLite exposes no introspection for CHECK constraints (and only partial - detail elsewhere), so parsing the `sqlite_master` DDL is the correct — and - only — approach. The module docstring says exactly this. - -## What holds up well (verified empirically) - -- **Corpus results:** all 272 statements in `tests/valid_create_table.json` - parse without crashing; the 230 that are independently executable were - cross-checked against SQLite's own `PRAGMA table_xinfo` — **0 column-name - or primary-key mismatches**. The 29 invalid statements also do not crash - the parser. -- Survived adversarial probing: identifier quoting in all four styles - including embedded escaped quotes (`"col ""x"""`, `[square]`, backticks), - strings containing `,)` and keywords inside CHECK expressions, - `col IN (...)` option extraction with escaped quotes, generated columns - (`GENERATED ALWAYS AS ... STORED` and bare `AS`), FK action clauses - (`ON DELETE SET NULL ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED`), - `WITHOUT ROWID, STRICT` trailers, keyword-named columns (`"unique"`), - multi-word types with parenthesized arguments, and `CREATE TABLE AS - SELECT`. -- The design is genuinely good: recursive-descent functions mirroring the - SQLite grammar are readable; the constraint list as source of truth with - derived accessor properties (`table.checks`, `col.not_null`, - `table.primary_key`) is the right model — adding a constraint kind means - adding a dataclass and a property. The group-capturing tokenizer - (parenthesized groups taken whole so nested commas never reach the parser) - is a clean way to sidestep expression parsing. - -## Must-fix before `transform()` builds on it - -### 1. Comments produce phantom columns (silent corruption) - -The tokenizer does not handle `--` line comments or `/* */` block comments, -and `sqlite_master` stores DDL verbatim. Reproduced: - -``` -CREATE TABLE t ( - a TEXT, -- user's name, (important) - b INTEGER -) -→ columns parsed as ['a', '-'] (real: ['a', 'b']) - -CREATE TABLE t (a TEXT /* legacy, do not use */, b INTEGER) -→ columns parsed as ['a', 'do', 'b'] (real: ['a', 'b']) -``` - -Hand-written schemas — exactly the ones that have CHECK constraints — are -exactly the ones with comments. If `transform()` rebuilds a table from a -model with wrong columns, that is data loss. Comment handling belongs in -`_tokenize` / `_locate_body` / `_split_top_level` (all three scan raw text). - -### 2. Numeric literal defaults are truncated (silent corruption) - -`_tokenize` splits `1.5` into `1` / `.` / `5` and `_default_value` consumes -one token. Reproduced: - -| DDL | parsed default | correct | -|----------------------|----------------|---------| -| `DEFAULT 1.5` | `'1'` | `1.5` | -| `DEFAULT -1.5` | `'-1'` | `-1.5` | -| `DEFAULT 1e-3` | `'1e'` | `1e-3` | -| `DEFAULT x'0102'` | `'x'` | `x'0102'` | - -Re-emitting these in a transform rewrites `DEFAULT 1.5` as `DEFAULT 1` — -silent schema corruption. Notably the corpus already contains -`DEFAULT -45.8e22`, but nothing catches this because the fixtures are -input-only (see next point). Fix by lexing numeric literals (including -sign, decimal point, exponent) and blob literals (`x'...'`) as single -tokens, or by making `_default_value` consume the full literal. - -### 3. No tests are actually executed - -The JSON corpora are not wired into pytest — there is no `test_*.py` on the -branch, so the parser currently has zero executed tests. Beyond wiring the -corpus in, add **expected-output snapshots** (parse each valid statement, -assert the full structured result), not just "doesn't crash": the -`DEFAULT -45.8e22` bug sat undetected in the corpus precisely because only -inputs are recorded. A cheap high-value addition: property test that for -every executable statement, parsed column names / pk / not-null match -`PRAGMA table_xinfo`. - -## Design decisions to settle - -### Discard-vs-error policy for grammar the model does not capture - -The plan is presumably parse → modify model → serialize, which matches how -`transform()` already rebuilds tables. That makes "what the model discards" -the critical list, because anything discarded is silently stripped from the -user's schema on transform: - -- `ON CONFLICT` clauses on PRIMARY KEY / UNIQUE / NOT NULL (parsed by - `_conflict_clause`, thrown away) -- FK `MATCH` and `DEFERRABLE INITIALLY DEFERRED` (parsed, thrown away — - deferred FKs are real in the wild) -- `ASC` / `DESC` in table-level `PRIMARY KEY (a DESC)` / `UNIQUE (...)` - column lists (`_column_list_group` keeps only the leading identifier; - inline single-column PK order *is* captured) -- Unknown column constraints (`_column_constraint` consumes one token and - returns `None`) - -Either capture these in the model, or have `transform()` raise -`TransformError` when the source DDL contains grammar it cannot round-trip — -the honest-failure pattern 3.38 established for un-recreatable indexes. -Silence is the one wrong answer. - -Same question for column renames: a renamed column referenced inside a CHECK -expression (stored as a raw string) cannot be mechanically rewritten, so -`transform(rename=...)` intersecting a check should raise `TransformError` -rather than emit a stale constraint. Likewise dropping a column referenced -by a table-level CHECK or multi-column UNIQUE. - -### Ship it private in 4.1 - -The module defines `Table`, `Column`, and `ForeignKey` — the package already -has `db.Table` and a `db.ForeignKey` namedtuple with different, incompatible -fields (`(table, column, other_table, other_column)` vs -`(columns, table, references, ...)`). Two public `ForeignKey` types in one -package is a confusion trap, and 4.0 is a fresh reminder that public surface -is forever. Recommendation: land it as a private module -(`sqlite_utils._parser` or similar) powering `transform()` in 4.1, promote -to documented public API in a later minor once battle-tested. Public-later -is additive; public-now-retract-later is breaking. - -Related naming nit: the parser's `Table.schema` is the attached-database -name (`main`/`temp`), while `db.Table.schema` is the DDL string. Rename one -(e.g. `database` or `schema_name`) before anything goes public. - -### Housekeeping - -- `create_table_parser.py` lives at the repo root; it needs to move inside - the `sqlite_utils/` package. -- `parse_checks` is documented as a "backwards-compatible helper" but - nothing in sqlite-utils exists for it to be compatible with — stale - comment from an earlier draft. -- The valid corpus is strong on grammar coverage but light on real-world - mess: add fixtures with comments, tabs/newlines in odd places, numeric - and blob defaults, and a couple of schemas generated by sqlite-utils - itself (double-quoted everything) and by common tools. - -## Bottom line - -Hold it for 4.1 exactly as planned — it has no 4.0 dependency and should not -delay the stable release. Before `transform()` builds on it: fix comment -handling and numeric/blob literal lexing, wire the fixtures into pytest with -expected-output snapshots, decide the discard-vs-error policy, and keep the -module private for one release. The bones are solid; both bugs are -tokenizer-level and small. diff --git a/fable-review-4.0rc1.md b/fable-review-4.0rc1.md deleted file mode 100644 index c590b10..0000000 --- a/fable-review-4.0rc1.md +++ /dev/null @@ -1,375 +0,0 @@ -# sqlite-utils 4.0rc1 pre-release review - -Final review before the stable 4.0 release, focused on issues that would be -**breaking changes if fixed after 4.0 ships**. Reviewed at commit `79117b9` -(4.0rc1 plus four commits). Every finding below was verified against the -actual code; the two most serious were reproduced end-to-end. - -Test suite at review time: **1080 passed, 16 skipped**. - -**Verdict: do not tag stable yet.** There are five release blockers, all of -which are small fixes, plus a handful of semantic decisions that are cheap to -make now and breaking to change later. Recommended path: fix the blockers, -make the documented decisions, cut an **rc2**. - ---- - -## Release blockers - -Data loss or wrong-by-default behavior that 4.0 would lock in. - -### 1. `delete_where()` never commits and poisons the connection (data loss) - -`Table.delete_where()` (`sqlite_utils/db.py:2948`) runs its DELETE via a bare -`self.db.execute()` with no `atomic()` wrapper — compare `Table.delete()` at -`db.py:2944`, which wraps correctly. The connection is left -`in_transaction=True`, so every *subsequent* `atomic()` call takes the -savepoint branch (`db.py:430-440`) and never commits either. - -Reproduced end-to-end: - -```python -db = sqlite_utils.Database("dw.db") -db["t"].insert_all([{"id": i} for i in range(3)], pk="id") -db["t"].delete_where("id = ?", [0]) # conn.in_transaction is now True -db["t"].insert({"id": 50}) -db["u"].insert({"a": 1}) -db.close() -# Reopen: rows are [0, 1, 2] — the delete, row 50, AND table u are all gone. -``` - -In 3.x this leak was latent because later operations used `with db.conn:` -which committed the pending work (verified against 3.38, where the same -sequence persists everything). The 4.0 `atomic()` design — "don't commit an -existing transaction" — converts the old latent leak into permanent silent -data loss. - -**`optimize()` (`db.py:2790`) and `rebuild_fts()` (`db.py:2752`) have the -identical leak** — both run `INSERT INTO fts(fts) VALUES(...)` via bare -`self.db.execute()`. The CLI escapes only because `cli.py:341` and -`cli.py:369` wrap the calls in `with db.conn:`. - -Tellingly, the only `delete_where` example in the docs -(`docs/python-api.rst:983-990`) already wraps it in `with db.atomic():`, -papering over the bug, while every other single-op example needs no wrapper. - -**Fix:** wrap all three in `with self.db.atomic():`; update the docs example. -These were the only leaky public write ops found — every other op -(insert/upsert/update/delete/lookup/transform/extract/create/add_column/ -create_index/enable_fts/enable_counts/m2m/convert/duplicate) was verified to -leave `in_transaction=False`. - -### 2. `drop-view` drops tables and `drop-table` drops views, silently - -`cli.py:1728` (`drop-table`) and `cli.py:1800` (`drop-view`) both use -`db[name].drop()`. `Database.__getitem__` dispatches on the actual object -type, and `Table.drop()`/`View.drop()` each issue their matching `DROP` -statement. - -Reproduced: `sqlite-utils drop-view t.db t` deleted **table `t`** with exit -code 0. The mirror case (`drop-table` on a view) also succeeds silently. - -This is a data-loss footgun that contradicts the issue #657 table/view split -that is a headline 4.0 breaking change. Fixing it later converts a silent -"success" into an error — a semver problem, so it must land in 4.0. - -**Fix:** one line each — use `db.table(name)` / `db.view(name)` and catch -`NoTable` / `NoView` for a clean `ClickException`. - -### 3. Post-rc1 `insert({})` change lets `upsert` silently insert rows - -The new `DEFAULT VALUES` branch (commit `b5d0080`, -`db.py:3184-3194`) checks `not list_mode and not all_columns` but never -checks the `upsert` flag: - -- `db["t"].upsert({}, pk="id")` executes `INSERT INTO t DEFAULT VALUES` — a - brand-new row is written — and *then* raises an unrelated `KeyError: 'id'` - from the `last_pk` computation (`db.py:3742-3746`). -- `upsert_all([{}, {}], pk="id")` (2+ records skips the `last_pk` path) - **silently inserts two new rows on every call, no error at all**. - -At rc1 both failed fast with `ZeroDivisionError` — ugly, but zero rows -written. Going from crash-without-mutation to silent insertion under -"upsert" semantics is a regression 4.0 would lock in. - -Relatedly, the compound-pk auto-detection (commit `bfd74a3`, -`db.py:3557-3560`) makes records that *omit* the pk value reachable where -they previously raised `PrimaryKeyRequired` before touching the database: -`upsert_all([{"v": "a"}, {"v": "b"}])` on an existing pk table now inserts -with `id=NULL` (never conflicts) and appends new rows on every call -(verified: two calls → 4 rows). Same for compound pks with a missing -component. Not covered by any test. - -**Fix:** raise a clean error in the empty-record and missing-pk-value paths -when `upsert=True` (an empty record has no pk value, so it can never be an -upsert). - -### 4. `Migrations.apply()` transaction semantics are accidental — decide now - -`sqlite_utils/migrations.py:84-95` runs each migration function and its -tracking-row insert with no transaction wrapper. Verified: a migration that -fails halfway leaves its partial side effects committed, records nothing, -and re-running re-executes the *entire* function including already-applied -statements — the classic double-apply hazard. - -This matches sqlite-migrate, so either behavior is defensible — but 4.0 -freezes the contract: adding per-migration transactions in 4.1 would break -migrations that manage their own transactions or run statements illegal -inside one (`VACUUM`, some `PRAGMA`s). - -**Decide now:** wrap each migration in `db.atomic()`, or explicitly document -"migrations are not transactional; write idempotent steps" so the current -behavior is the contract rather than an accident. - -Also decide now: **`_AppliedMigration.applied_at` is annotated -`datetime.datetime` (`migrations.py:21`) but is a `str` at runtime** -(`migrations.py:67` passes the TEXT column value straight through), and the -type is published via autodoc (`docs/migrations.rst:168-171`). Changing the -annotation to `str` now is a free one-liner (and the sqlite-migrate- -compatible choice); "fixing" it to a real datetime later breaks every -consumer doing string operations. - -### 5. `enable_wal()` / `disable_wal()` commit open transactions - -`ensure_autocommit_off()` (`db.py:456-472`) assigns `conn.isolation_level`, -and CPython's setter **commits any pending transaction** as a side effect. -Verified consequences: - -- `with db.atomic(): insert(2); db.enable_wal(); insert(3); raise` → **all - rows persist despite the exception**, directly contradicting the - documented rollback guarantee (`docs/python-api.rst:255`), which is *the* - headline 4.0 semantic. -- A user's own `BEGIN` + insert + `enable_wal()` → their subsequent - `rollback()` is a no-op; the insert persists. This is exactly the - "unexpectedly committing an existing transaction" bug class `atomic()` was - introduced to eliminate (changelog, issue #755). - -Pre-existing in 3.x, but 4.0 is the moment to fix. - -**Fix:** make `enable_wal`/`disable_wal` raise (or skip the isolation dance) -when `conn.in_transaction`. - ---- - -## Decisions to make now — cheap today, breaking after 4.0 - -### `Database.__enter__` / `__exit__` contract is unpinned - -`__exit__` only calls `self.close()` (`db.py:408-417`), which silently rolls -back uncommitted changes. Verified: a raw `db.execute("insert ...")` inside -`with Database(p) as db:` is discarded on exit. This diverges from -`sqlite3.Connection`'s own context manager (commits on success, does *not* -close). The docs (`docs/python-api.rst:139-145`) say only "automatically -close the connection" — no mention of rollback — and **no test anywhere uses -`with Database(...)`**, so the contract is completely unpinned. Whichever way -this might later be "improved" is a breaking behavior change. Decide now, -add one docs sentence ("any uncommitted changes are rolled back") and a -pinning test. Note blockers 1–2 make this worse: after a `delete_where`, -exiting the `with` block discards everything. - -### `atomic()` is broken on Python 3.12+ `autocommit` connections - -`db.py:441-453` uses `conn.commit()`/`conn.rollback()`, documented no-ops -when `Connection.autocommit=True`. Verified on 3.13: - -- `autocommit=True` connection → a successful `atomic()` block leaves the - transaction permanently open; rollback on exception is also a no-op. -- `autocommit=False` connection → the connection is always in a transaction, - so `atomic()` always takes the savepoint branch and never commits - anything. - -`Database(filename_or_conn)` accepts arbitrary connections (`db.py:397`). -Either handle these modes (issue literal `COMMIT`/`ROLLBACK` statements, or -reject such connections) or document supported connection modes in -`python_api_atomic` now — changing commit timing later is breaking. -(Legacy `isolation_level=None` connections were verified to work correctly.) - -### `atomic()` savepoint-branch semantics are intended but undocumented - -If the connection is already in a transaction (user ran `BEGIN` or raw DML), -`atomic()` becomes a savepoint and its successful exit does **not** persist -anything until the user commits (verified: a user `rollback()` discards the -atomic block's writes). Reasonable semantics, but the `conn.in_transaction` -sniffing must be documented before it is locked — the docs currently -describe nesting only in terms of `atomic()`-inside-`atomic()`. - -### `atomic()` issues plain deferred `BEGIN` - -`db.py:442` — no way to request `BEGIN IMMEDIATE`. A future switch of the -default would change locking/`SQLITE_BUSY` behavior (breaking); adding an -`immediate=` parameter later is fine. Documenting "deferred" now costs one -sentence. - -### No-op `-d/--detect-types` flag: keep or kill - -`cli.py:942-947` still defines `-d/--detect-types` on `insert`/`upsert` -(help says "(default)"), deliberately tested as a no-op -(`tests/test_cli.py:2299,2322`). The `detect_types` parameter of -`insert_upsert_implementation` (`cli.py:1002`) is dead code — the body only -reads `no_detect_types`. If the flag is ever to be removed, 4.0 is the only -chance; keeping it as back-compat is defensible, but note -`--detect-types --no-detect-types` together silently favors the latter with -no conflict error. - -### `--stop-before` is silently ignored for old `sqlite_migrate` objects - -The duck-typing in `_compatible_migration_set` (`cli.py:3286-3289`) exists -precisely so migration files still doing `from sqlite_migrate import -Migrations` keep working — but `cli.py:3393` always passes `stop_before=` as -a **list**, and the old plugin's `apply()` does `if name == stop_before:` -against a string. Verified with the released 0.1b0: `--stop-before step2` -applied `step2` anyway. The CLI applies the exact migration the user asked -it not to, on the one upgrade path the duck typing exists to support. - -Related: a **typo'd `--stop-before` name silently applies everything**, -including the migration you meant to stop before (unknown names simply never -match; no error from the CLI). Validation has to live in the CLI ("each -`--stop-before` value must match at least one set") because unqualified -names legitimately fan out across sets. Adding that error post-4.0 turns -currently-succeeding invocations into failures — decide now. - -### Public-API validation via bare `assert` - -User-facing errors raised via `assert` throughout `db.py` (370, 396, 1016, -1950, 2829, 3028, 3565, 3571, 3967, …). They vanish under `python -O`, and -`db.py:3028` carries `# TODO: Test this works (rolls back) - use better -exception:`. Converting `AssertionError` to `ValueError` later changes -exception types callers may catch — best done in a major, if ever. - ---- - -## Should-fix polish (non-breaking later, but ugly for a stable release) - -- **`insert`/`upsert` into a view name prints a raw traceback** — - `NoTable("Table v is actually a view")` from `cli.py:1154` is not - converted to `ClickException` (contrast `duplicate` at `cli.py:1676`). -- **Misleading `NoView` message** — `db.view("t")` where `t` is a table says - "View t does not exist" (`db.py:661`); the mirror case helpfully says - "Table v is actually a view" (`db.py:650`). -- **Stale `__getitem__` docstring** (`db.py:502`) says it "returns a Table - object"; it returns a `View` for views. Feeds autodoc. -- **Wrong `hash_id` doc in the `Table` docstring** (`db.py:1588`) — says - bool; it's a column-name string (`Optional[str]`). -- **`sqlite-utils migrate --list` writes to the database** — - `pending()`/`applied()` call `ensure_migrations_table` - (`migrations.py:48,65`), so `--list` creates the tracking table, performs - the one-way legacy sqlite-migrate schema upgrade, and — since `db_path` - has no `exists=True` (`cli.py:3338-3340`) — creates the database file - itself. A read-looking operation should not do any of that. -- **`applied()` has no `ORDER BY`** (`migrations.py:66-71`) — relies on - rowid order; `tests/test_cli_migrate.py:104-107` asserts that order. - Add `order_by="id"` to make `--list` deterministic by contract. -- **Duplicate migration names within a set**: both functions execute (side - effects committed) before an opaque `IntegrityError`; only the first is - recorded (`migrations.py:36-42`). A `ValueError` at registration would be - cheap and is effectively non-breaking to add now. -- **`pending()`/`applied()` return underscore-private dataclasses** - (`_Migration`/`_AppliedMigration`) that are excluded from autodoc - (`docs/migrations.rst:171`), so the `.name`/`.applied_at`/`.fn` fields - users must access are undocumented API. -- **`set:name` colon syntax is unvalidated** (`cli.py:3328` uses - `partition(":")`) — a set or migration name containing `:` can never be - targeted. Document/validate "no colons in names" now. -- **Bare `@migrations` decorator** gives an opaque `TypeError` - (`migrations.py:30`); a helpful message would be purely additive. -- **Tracer never sees transaction statements** — `atomic()` uses - `self.conn.execute()`/`conn.commit()` directly (`db.py:432-450`), - bypassing the tracer. Routing through `self.execute()` later would change - tracer output that downstream tests may assert on — cheap to decide now. -- **Order-dependent empty-dict semantics** (from `b5d0080`): - `insert_all([{}, {"v": "hi"}])` gives the empty record its column DEFAULTs, - but `insert_all([{"v": "hi"}, {}])` inserts explicit `NULL` for it — a - `NOT NULL ... DEFAULT` column succeeds in one ordering and raises - `IntegrityError` in the other. -- **Batch-size cliff when the first record is `{}`** (`db.py:3625-3629`) — - `num_columns` comes from the first record only, forcing `batch_size=1` - for the entire stream. Performance-only, degenerate input. -- **`pyproject.toml` build-system underpinned** — `requires = ["setuptools"]` - while the PEP 639 `license = "Apache-2.0"` string needs setuptools ≥ - 77.0.3; non-isolated sdist builds on older setuptools will fail. Pin - `setuptools>=77`. -- **Undocumented post-rc1 behavior changes** — neither upsert pk - auto-detection (`bfd74a3`) nor `insert({})`-uses-DEFAULT-VALUES - (`b5d0080`) is mentioned in `docs/` or the changelog. Note also that - `db.table(name).insert({})` only works for *existing* tables — on a - missing table it still raises `AssertionError: Tables must have at least - one column`, so the issue #759 title scenario is only partially covered. - ---- - -## Verified sound — no action needed - -- **sqlite-migrate on-disk compatibility** (the scariest area): tested - against the actual 0.1b0 sdist from PyPI. Same `_sqlite_migrations` table; - the legacy compound-pk `(migration_set, name)` schema is detected via - `table.pks != ["id"]` and upgraded in place (`migrations.py:97-117`); rows - are preserved and previously-applied migrations are **not** re-applied. - Both timestamp formats are identical. Tests cover both legacy layouts - (`tests/test_migrations.py:83-110`). No data-corruption risk for upgrading - users. The built-in `migrate` command is registered *after* plugin hooks - (`cli.py:3415-3417`), so a still-installed old plugin's command is - correctly overridden. -- Migration ordering is definition order (documented and tested); identity - is `(set name, migration name)` with a unique index; same name across sets - is fine. The new CLI is a strict superset of the old plugin's; file - discovery is now `sorted()` where the plugin iterated an unordered set. -- Nested savepoint rollback/release logic including exception paths - (`tests/test_atomic.py:44-120`); commit-time deferred-FK failure rolls - back cleanly; `transform()` inside an open transaction correctly uses - `PRAGMA defer_foreign_keys`; `_executescript` statement-splitting avoids - sqlite3's implicit commit. Savepoint naming via `secrets.token_hex(16)` is - collision-free. -- Compound-pk upsert detection happy path works for single and compound pks - on both the `ON CONFLICT` and `use_old_upsert` implementations; explicit - `pk=` still wins over detection; rowid/missing/hash_id tables still raise - `PrimaryKeyRequired` (`tests/test_upsert.py:52-97`). -- The table/view split (#657) is otherwise complete: `db.table()` on a view - raises `NoTable`, never returns a `View`; `db.view()` accepts no - table-only kwargs. The tracer-test fix (`401fb69`) and click pin - (`79117b9`, dev-group only) are correct. -- `__all__` exports are coherent (`Database`, `Migrations`, - `suggest_column_types`, `hookimpl`, `hookspec`); every `sqlite_utils.X` - docs reference resolves. The `pip` runtime dependency is genuinely - required (`cli.py:2967-2983` uses `run_module("pip")` for - `install`/`uninstall`). Classifiers match `requires-python`. No - deprecation debt: zero `DeprecationWarning` markers; clean under - `-W error::DeprecationWarning`. - ---- - -## The transform() incoming-FK branch: hold for 4.1 - -The `update_incoming_fks` work on `claude/investigate-transform-fk-KMTZ7` is -right to hold: - -- It is purely additive (`update_incoming_fks: bool = False` keyword + - `--update-incoming-fks` flag; default behavior unchanged), so 4.1 is safe - semver-wise. -- The branch predates the `atomic()` refactor — it still uses - `with self.db.conn:` directly in `transform()`, the exact pattern 4.0 - eliminated (#755) — and has real issues: `_skip_fk_validation` is mutable - state on the whole `Database` object, and the incoming-FK rebuilds execute - `transform_sql()` output raw for referencing tables, silently dropping - their indexes (the 3.38 index-recreation logic only runs for the primary - table). -- One decision belongs to 4.0: whether `transform()` should eventually - refuse/warn *by default* when a rename would leave dangling incoming FKs. - Adding a warning later is fine; making it an error by default later is - breaking. If strict-by-default is the desired end state, add just the - guard in 4.0 and ship the auto-update machinery in 4.1. - ---- - -## Suggested path to stable - -1. Fix the five blockers — three `atomic()` wrappers (`delete_where`, - `optimize`, `rebuild_fts`), two CLI lookups (`drop-table`/`drop-view`), - the `upsert` guard in the DEFAULT VALUES / missing-pk paths, the - `applied_at: str` annotation, and the `in_transaction` guard in - `enable_wal`/`disable_wal`. -2. Make and document the decisions: `apply()` transactionality, - `Database.__exit__` semantics (+ pinning test), `atomic()` supported - connection modes and deferred-`BEGIN` note, `--detect-types` keep/kill, - `--stop-before` validation and old-plugin compat. -3. Cut **rc2** — the transaction-semantics fixes deserve one more candidate - round before the semantics freeze. From 70055d74323f5a4d15d27f1d13b83590ade68258 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 20:16:15 +0000 Subject: [PATCH 22/34] Document the transaction model as a first-class concept New top-level 'Transactions and saving your changes' section in the Python API documentation, stating the fundamental contract up front: every write method commits its own changes before returning, no commit() call is ever needed. The db.atomic() documentation moves under it, alongside new subsections covering the two cases where the user owns the commit - raw db.execute() writes and manually managed transactions - and the supported connection modes. Cross-referenced from Getting started (the first insert example now says the rows are saved immediately), a warning on db.execute(), the closing-a-database section and the upgrading guide. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 132 ++++++++++++++++++++++++++++++++------------ docs/upgrading.rst | 2 +- 2 files changed, 98 insertions(+), 36 deletions(-) diff --git a/docs/python-api.rst b/docs/python-api.rst index e888b7d..2c43070 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -33,6 +33,8 @@ Here's how to create a new SQLite database file containing a new ``chickens`` ta "color": "black", }]) +The inserted rows are saved to the database file straight away - methods like ``insert_all()`` commit their own changes, so no ``commit()`` call is needed. See :ref:`python_api_transactions` for how this works. + You can loop through those rows like this: .. code-block:: python @@ -144,7 +146,7 @@ The ``Database`` object also works as a context manager, which will automaticall db["my_table"].insert({"name": "Example"}) # Connection is automatically closed here -Exiting the block closes the connection without committing, so any changes that are still inside an open transaction at that point - for example writes made with raw ``db.execute()`` calls - will be rolled back. Commit explicitly, or use methods such as ``.insert()`` and the ``db.atomic()`` context manager which commit their own transactions. +Exiting the block closes the connection without committing, so any changes that are still inside an open transaction at that point - for example writes made with raw ``db.execute()`` calls - will be rolled back. Commit explicitly, or use methods such as ``.insert()`` and the ``db.atomic()`` context manager which commit their own transactions - see :ref:`python_api_transactions`. .. _python_api_attach: @@ -245,40 +247,8 @@ The ``db.execute()`` and ``db.executescript()`` methods provide wrappers around Other cursor methods such as ``.fetchone()`` and ``.fetchall()`` are also available, see the `standard library documentation `__. -.. _python_api_atomic: - -Transactions with db.atomic() ------------------------------ - -Use ``db.atomic()`` to group multiple operations in a transaction: - -.. code-block:: python - - with db.atomic(): - db.table("dogs").insert({"id": 1, "name": "Cleo"}, pk="id") - db.table("dogs").insert({"id": 2, "name": "Pancakes"}) - -If an exception is raised, changes made inside the block will be rolled back. - -``db.atomic()`` can be nested. Nested blocks use SQLite savepoints, so an exception in an inner block can roll back to that savepoint without rolling back the entire outer transaction: - -.. code-block:: python - - with db.atomic(): - db.table("dogs").insert({"id": 1, "name": "Cleo"}, pk="id") - try: - with db.atomic(): - db.table("dogs").insert({"id": 2, "name": "Pancakes"}) - raise ValueError("skip this one") - except ValueError: - pass - db.table("dogs").insert({"id": 3, "name": "Marnie"}) - -The transaction is opened with a deferred ``BEGIN`` - SQLite takes the necessary locks when the first statement inside the block runs. - -The same savepoint mechanism used for nesting applies if the connection is already inside a transaction when ``db.atomic()`` is called - for example if you executed ``BEGIN`` yourself, or made writes using raw ``db.execute()`` calls. In that case the block becomes a savepoint within your transaction: exiting the block releases the savepoint, but nothing is committed to disk until your outer transaction commits. - -``db.atomic()`` is designed for connections in Python's default transaction handling mode. Connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options are not supported, because ``commit()`` and ``rollback()`` behave differently on those connections. +.. warning:: + Unlike the table methods described elsewhere in this documentation, ``db.execute()`` does **not** commit. If you use it to modify the database you are responsible for committing the change yourself - see :ref:`python_api_transactions_execute`. .. _python_api_parameters: @@ -308,6 +278,98 @@ Named parameters using ``:name`` can be filled using a dictionary: In this example ``next()`` is used to retrieve the first result in the iterator returned by the ``db.query()`` method. +.. _python_api_transactions: + +Transactions and saving your changes +==================================== + +Every method in this library that writes to the database - ``insert()``, ``upsert()``, ``update()``, ``delete()``, ``delete_where()``, ``transform()``, ``create_table()``, ``create_index()``, ``enable_fts()`` and the rest - runs inside its own transaction and commits it before returning. Your changes are saved to disk as soon as the method call finishes: + +.. code-block:: python + + db = Database("data.db") + db.table("news").insert({"headline": "Dog wins award"}) + # The new row is already saved - no commit() required + +You never need to call ``commit()`` after using these methods, and you do not need to close the database to persist your changes. + +There are exactly three situations where you need to think about transactions: + +1. You want to group several write operations together, so they either all succeed or all fail - use :ref:`db.atomic() `. +2. You are executing raw SQL writes with ``db.execute()``, which does *not* commit automatically - see :ref:`python_api_transactions_execute`. +3. You are :ref:`managing a transaction yourself `, in which case the library will never commit it for you. + +.. _python_api_atomic: + +Grouping changes with db.atomic() +--------------------------------- + +Use ``db.atomic()`` to group multiple operations in a single transaction: + +.. code-block:: python + + with db.atomic(): + db.table("dogs").insert({"id": 1, "name": "Cleo"}, pk="id") + db.table("dogs").insert({"id": 2, "name": "Pancakes"}) + +The transaction commits when the block exits. If an exception is raised, changes made inside the block will be rolled back. + +``db.atomic()`` can be nested. Nested blocks use SQLite savepoints, so an exception in an inner block can roll back to that savepoint without rolling back the entire outer transaction: + +.. code-block:: python + + with db.atomic(): + db.table("dogs").insert({"id": 1, "name": "Cleo"}, pk="id") + try: + with db.atomic(): + db.table("dogs").insert({"id": 2, "name": "Pancakes"}) + raise ValueError("skip this one") + except ValueError: + pass + db.table("dogs").insert({"id": 3, "name": "Marnie"}) + +The transaction is opened with a deferred ``BEGIN`` - SQLite takes the necessary locks when the first statement inside the block runs. + +.. _python_api_transactions_execute: + +Raw SQL writes with db.execute() +-------------------------------- + +:ref:`db.execute() ` is a thin wrapper around the underlying ``sqlite3`` connection, and it follows that connection's rules rather than this library's: a write statement opens a transaction that stays open until something commits it. Commit explicitly: + +.. code-block:: python + + db.execute("insert into news (headline) values (?)", ["Dog wins award"]) + db.conn.commit() + +Or wrap the calls in ``db.atomic()``, which commits for you: + +.. code-block:: python + + with db.atomic(): + db.execute("insert into news (headline) values (?)", ["Dog wins award"]) + +If you do neither, the write can be deceptive: reads on the same connection will see the new row, making it look saved, but the open transaction will be rolled back when the connection closes and the row will be gone. + +.. _python_api_transactions_manual: + +Managing transactions yourself +------------------------------ + +You can take full manual control using ``db.execute("begin")`` (or any raw write, as above) followed by ``db.conn.commit()`` or ``db.conn.rollback()``. + +The library will never commit a transaction you opened. If you call write methods such as ``insert()`` - or use ``db.atomic()`` - while your transaction is open, they participate in it using SQLite savepoints instead of committing: exiting an ``atomic()`` block releases its savepoint, but nothing is saved to disk until you commit the outer transaction yourself. If you roll back, their changes are rolled back too. + +Two related safeguards to be aware of: + +- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``RuntimeError`` if called while a transaction is open, because changing the journal mode would commit it as a side effect. +- Closing the database - explicitly with ``db.close()``, or by exiting a ``with Database(...) as db:`` block - rolls back any transaction that is still open, see :ref:`python_api_close`. + +Supported connection modes +-------------------------- + +``db.atomic()`` and the automatic per-method transactions are designed for connections in Python's default transaction handling mode. Connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options are not supported, because ``commit()`` and ``rollback()`` behave differently on those connections. + .. _python_api_table: Accessing tables diff --git a/docs/upgrading.rst b/docs/upgrading.rst index 36d720b..553afd4 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -72,7 +72,7 @@ Python API changes **Validation errors raise ValueError.** Invalid arguments to Python API methods - for example ``create_table()`` with no columns, or ``ignore=True`` together with ``replace=True`` - now raise ``ValueError``. They previously raised ``AssertionError`` from bare ``assert`` statements, which were silently skipped under ``python -O``. -**Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation. Changes you may notice: +**Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation - the full model is described in :ref:`python_api_transactions`. Changes you may notice: - Multi-step operations such as ``table.transform()`` no longer commit an existing transaction you have open - they use savepoints inside it instead. - ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``RuntimeError`` if called while a transaction is open, instead of silently committing it. From adea475a618e94d037caf8cc17fec3a3537a58b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 20:44:04 +0000 Subject: [PATCH 23/34] Remove View.enable_fts() The method existed only to raise NotImplementedError, since full-text search is not supported for views, and it showed up in the generated API reference as a documented View method. Calling enable_fts() on a View now raises AttributeError like any other missing method. The sqlite-utils enable-fts command uses db.table() and shows a clean error when pointed at a view instead of a traceback. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/upgrading.rst | 2 ++ sqlite_utils/cli.py | 4 ++-- sqlite_utils/db.py | 6 ------ tests/test_fts.py | 22 ++++++++++++++++++---- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 0b41875..532b23b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,6 +9,7 @@ Unreleased ---------- +- **Breaking change**: The ``View`` class no longer has an ``enable_fts()`` method. It existed only to raise ``NotImplementedError``, since full-text search is not supported for views - calling it now raises ``AttributeError`` instead, and the method no longer appears in the API reference. The ``sqlite-utils enable-fts`` command shows a clean error when pointed at a view. - **Breaking change**: Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. - **Breaking change**: The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. - **Breaking change**: ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. diff --git a/docs/upgrading.rst b/docs/upgrading.rst index 553afd4..e6f4d4d 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -70,6 +70,8 @@ Python API changes **table.convert() no longer skips falsey values.** Matching the CLI change above, ``table.convert()`` now converts every value. The ``skip_false`` parameter has been removed - previously it defaulted to ``True``, skipping empty strings and other falsey values. +**View.enable_fts() has been removed.** The ``View`` class previously had an ``enable_fts()`` method that existed only to raise ``NotImplementedError`` - full-text search is not supported for views. Calling it now raises ``AttributeError`` like any other missing method. + **Validation errors raise ValueError.** Invalid arguments to Python API methods - for example ``create_table()`` with no columns, or ``ignore=True`` together with ``replace=True`` - now raise ``ValueError``. They previously raised ``AssertionError`` from bare ``assert`` statements, which were silently skipped under ``python -O``. **Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation - the full model is described in :ref:`python_api_transactions`. Changes you may notice: diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index a2f881a..d609ad5 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -702,14 +702,14 @@ def enable_fts( _register_db_for_cleanup(db) _load_extensions(db, load_extension) try: - db[table].enable_fts( + db.table(table).enable_fts( column, fts_version=fts_version, tokenize=tokenize, create_triggers=create_triggers, replace=replace, ) - except OperationalError as ex: + except (NoTable, OperationalError) as ex: raise click.ClickException(str(ex)) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index a0ca6ed..49d9f63 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -4317,12 +4317,6 @@ class View(Queryable): if not ignore: raise - def enable_fts(self, *args: object, **kwargs: object) -> None: - "``enable_fts()`` is supported on tables but not on views." - raise NotImplementedError( - "enable_fts() is supported on tables but not on views" - ) - def jsonify_if_needed(value: object) -> object: if isinstance(value, decimal.Decimal): diff --git a/tests/test_fts.py b/tests/test_fts.py index 11df898..3f7c5a9 100644 --- a/tests/test_fts.py +++ b/tests/test_fts.py @@ -461,12 +461,12 @@ def test_enable_fts_replace_handles_legacy_bracket_quoted_content_table(): assert 'content="books"' in db["books_fts"].schema -def test_enable_fts_error_message_on_views(): +def test_view_has_no_enable_fts(): db = Database(memory=True) db.create_view("hello", "select 1 + 1") - with pytest.raises(NotImplementedError) as e: - db["hello"].enable_fts() # type: ignore[call-arg] - assert e.value.args[0] == "enable_fts() is supported on tables but not on views" + # Views deliberately do not have an enable_fts() method + with pytest.raises(AttributeError): + db["hello"].enable_fts() # type: ignore[union-attr] @pytest.mark.parametrize( @@ -718,3 +718,17 @@ def test_search_quote(fresh_db): list(table.search(query)) # No exception with quote=True list(table.search(query, quote=True)) + + +def test_enable_fts_cli_on_view_errors(tmpdir): + db_path = str(tmpdir / "test.db") + db = Database(db_path) + db["t"].insert({"text": "hello"}) + db.create_view("v", "select * from t") + db.close() + from click.testing import CliRunner + from sqlite_utils import cli as cli_module + + result = CliRunner().invoke(cli_module.cli, ["enable-fts", db_path, "v", "text"]) + assert result.exit_code == 1 + assert result.output.strip() == "Error: Table v is actually a view" From 0fca5908c14f7e14651def56abb4acff239ea89f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:19:47 +0000 Subject: [PATCH 24/34] Group the Unreleased changelog into breaking changes and everything else Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 532b23b..04d5405 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,18 +9,24 @@ Unreleased ---------- -- **Breaking change**: The ``View`` class no longer has an ``enable_fts()`` method. It existed only to raise ``NotImplementedError``, since full-text search is not supported for views - calling it now raises ``AttributeError`` instead, and the method no longer appears in the API reference. The ``sqlite-utils enable-fts`` command shows a clean error when pointed at a view. -- **Breaking change**: Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. -- **Breaking change**: The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. -- **Breaking change**: ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. +Breaking changes: + +- ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. +- Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. +- ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. +- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``RuntimeError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. +- The ``View`` class no longer has an ``enable_fts()`` method. It existed only to raise ``NotImplementedError``, since full-text search is not supported for views - calling it now raises ``AttributeError`` instead, and the method no longer appears in the API reference. The ``sqlite-utils enable-fts`` command shows a clean error when pointed at a view. +- The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. + +Everything else: + - Fixed a bug where ``table.delete_where()``, ``table.optimize()`` and ``table.rebuild_fts()`` did not commit their changes, leaving the connection inside an open transaction. Their work - and any subsequent writes - could then be silently rolled back when the connection was closed. All three now use ``db.atomic()``, consistent with the other write methods. - The ``sqlite-utils drop-table`` command now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. Both now exit with an error suggesting the correct command to use. - Migrations applied by the new :ref:`migrations system ` now run inside a transaction, together with the record of the migration having been applied. If a migration raises an exception its changes are rolled back and it stays pending, so it can be safely re-applied after the error is fixed. Migrations that cannot run inside a transaction, such as those executing ``VACUUM``, can opt out using ``@migrations(transactional=False)`` - see :ref:`migrations_transactions`. - ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. -- ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) -- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``RuntimeError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. - Improvements to the ``sqlite-utils migrate`` command: ``--stop-before`` values that do not match any known migration are now an error instead of being silently ignored, ``--stop-before`` now works correctly with migration files that still use the older ``sqlite_migrate.Migrations`` class, and ``--list`` is now a read-only operation that no longer creates the database file or the migrations tracking table. ``migrations.applied()`` now returns migrations in the order they were applied. +- New documentation: :ref:`python_api_transactions` describes how transactions work and when changes are committed, and a new :ref:`upgrading` page details the changes needed to move between major versions. .. _v4_0rc1: From ff76fb7740a1f280831713274b90c07a70f8dc86 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:20:37 +0000 Subject: [PATCH 25/34] Clarify that db.atomic() is fine inside transactional migrations The manual-transaction warning could be read as covering db.atomic() too. Nested atomic() blocks are safe - they become savepoints inside the migration's transaction, preserving the all-or-nothing rollback guarantee. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/migrations.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migrations.rst b/docs/migrations.rst index 61709c4..1bd1457 100644 --- a/docs/migrations.rst +++ b/docs/migrations.rst @@ -87,7 +87,7 @@ Some operations cannot run inside a transaction, for example ``VACUUM`` or chang A migration registered with ``transactional=False`` runs without a wrapping transaction, so if it fails part way through any changes it already made will not be rolled back, and re-applying will run the whole function again. -Avoid calling ``db.conn.commit()`` or otherwise managing transactions manually inside a transactional migration - register the migration with ``transactional=False`` if it needs to control its own transactions. +Avoid calling ``db.conn.commit()`` or otherwise managing transactions manually inside a transactional migration - register the migration with ``transactional=False`` if it needs to control its own transactions. Using ``with db.atomic():`` blocks inside a migration is fine: they nest as savepoints within the migration's transaction, so the migration as a whole still commits or rolls back as a single unit. See :ref:`python_api_transactions`. Applying migrations using the CLI ================================= From 6c88067ab76b9597fb1c538c53164632526a2891 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:25:28 +0000 Subject: [PATCH 26/34] Explain why the Database context manager rolls back instead of committing Expands the closing-a-database docs: exit is equivalent to close(), open transactions can only come from manual transaction control, auto-committing could silently persist half-finished work, and the behavior deliberately differs from sqlite3.Connection's own context manager. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/python-api.rst | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/python-api.rst b/docs/python-api.rst index 2c43070..163dad5 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -146,7 +146,22 @@ The ``Database`` object also works as a context manager, which will automaticall db["my_table"].insert({"name": "Example"}) # Connection is automatically closed here -Exiting the block closes the connection without committing, so any changes that are still inside an open transaction at that point - for example writes made with raw ``db.execute()`` calls - will be rolled back. Commit explicitly, or use methods such as ``.insert()`` and the ``db.atomic()`` context manager which commit their own transactions - see :ref:`python_api_transactions`. +Exiting the block is equivalent to calling ``db.close()``: the connection is +closed and any transaction still open at that point is rolled back. This +matches SQLite's own behavior when a connection closes. + +This rarely matters in practice. Every method in this library that writes to +the database commits its own changes before returning, so a transaction can +only be left open here if you opened it yourself - through raw ``db.execute()`` +writes or an explicit ``BEGIN``. In that case the decision to commit stays +with you: committing automatically on exit could silently persist +half-finished work, for example if your code returned early from the block. +Commit explicitly with ``db.conn.commit()``, or wrap your writes in +``db.atomic()`` which commits for you. + +Note this differs from the ``sqlite3.Connection`` context manager in the +standard library, which commits on success but does not close the connection. +See :ref:`python_api_transactions` for the full transaction model. .. _python_api_attach: From 788c393a30dde58257ca76fa249ee0842c17b2e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:31:22 +0000 Subject: [PATCH 27/34] Add db.begin(), db.commit() and db.rollback() methods Manual transaction control previously required mixing raw db.execute("begin") strings with methods on db.conn. These thin wrappers give the documentation and callers a single consistent idiom. Docs updated to use them throughout. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/migrations.rst | 2 +- docs/python-api.rst | 19 +++++++++++++++---- sqlite_utils/db.py | 25 +++++++++++++++++++++++++ tests/test_atomic.py | 34 +++++++++++++++++++++++++++++++++- 5 files changed, 75 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 04d5405..baff4af 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -26,6 +26,7 @@ Everything else: - ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) - Improvements to the ``sqlite-utils migrate`` command: ``--stop-before`` values that do not match any known migration are now an error instead of being silently ignored, ``--stop-before`` now works correctly with migration files that still use the older ``sqlite_migrate.Migrations`` class, and ``--list`` is now a read-only operation that no longer creates the database file or the migrations tracking table. ``migrations.applied()`` now returns migrations in the order they were applied. +- New ``db.begin()``, ``db.commit()`` and ``db.rollback()`` methods for taking manual control of transactions, as an alternative to the ``db.atomic()`` context manager. - New documentation: :ref:`python_api_transactions` describes how transactions work and when changes are committed, and a new :ref:`upgrading` page details the changes needed to move between major versions. .. _v4_0rc1: diff --git a/docs/migrations.rst b/docs/migrations.rst index 1bd1457..e685936 100644 --- a/docs/migrations.rst +++ b/docs/migrations.rst @@ -87,7 +87,7 @@ Some operations cannot run inside a transaction, for example ``VACUUM`` or chang A migration registered with ``transactional=False`` runs without a wrapping transaction, so if it fails part way through any changes it already made will not be rolled back, and re-applying will run the whole function again. -Avoid calling ``db.conn.commit()`` or otherwise managing transactions manually inside a transactional migration - register the migration with ``transactional=False`` if it needs to control its own transactions. Using ``with db.atomic():`` blocks inside a migration is fine: they nest as savepoints within the migration's transaction, so the migration as a whole still commits or rolls back as a single unit. See :ref:`python_api_transactions`. +Avoid calling ``db.commit()`` or otherwise managing transactions manually inside a transactional migration - register the migration with ``transactional=False`` if it needs to control its own transactions. Using ``with db.atomic():`` blocks inside a migration is fine: they nest as savepoints within the migration's transaction, so the migration as a whole still commits or rolls back as a single unit. See :ref:`python_api_transactions`. Applying migrations using the CLI ================================= diff --git a/docs/python-api.rst b/docs/python-api.rst index 163dad5..400c0fa 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -156,7 +156,7 @@ only be left open here if you opened it yourself - through raw ``db.execute()`` writes or an explicit ``BEGIN``. In that case the decision to commit stays with you: committing automatically on exit could silently persist half-finished work, for example if your code returned early from the block. -Commit explicitly with ``db.conn.commit()``, or wrap your writes in +Commit explicitly with ``db.commit()``, or wrap your writes in ``db.atomic()`` which commits for you. Note this differs from the ``sqlite3.Connection`` context manager in the @@ -350,12 +350,12 @@ The transaction is opened with a deferred ``BEGIN`` - SQLite takes the necessary Raw SQL writes with db.execute() -------------------------------- -:ref:`db.execute() ` is a thin wrapper around the underlying ``sqlite3`` connection, and it follows that connection's rules rather than this library's: a write statement opens a transaction that stays open until something commits it. Commit explicitly: +:ref:`db.execute() ` is a thin wrapper around the underlying ``sqlite3`` connection, and it follows that connection's rules rather than this library's: a write statement opens a transaction that stays open until something commits it. Commit explicitly with ``db.commit()``: .. code-block:: python db.execute("insert into news (headline) values (?)", ["Dog wins award"]) - db.conn.commit() + db.commit() Or wrap the calls in ``db.atomic()``, which commits for you: @@ -371,7 +371,18 @@ If you do neither, the write can be deceptive: reads on the same connection will Managing transactions yourself ------------------------------ -You can take full manual control using ``db.execute("begin")`` (or any raw write, as above) followed by ``db.conn.commit()`` or ``db.conn.rollback()``. +You can take full manual control using the ``db.begin()``, ``db.commit()`` and ``db.rollback()`` methods: + +.. code-block:: python + + db.begin() + db.table("news").insert({"headline": "Dog wins award"}) + if all_looks_good: + db.commit() + else: + db.rollback() + +``db.begin()`` raises ``sqlite3.OperationalError`` if a transaction is already open. ``db.commit()`` and ``db.rollback()`` do nothing if there is no open transaction. A raw write executed with ``db.execute()`` (as above) opens a transaction implicitly, without needing ``db.begin()``. The library will never commit a transaction you opened. If you call write methods such as ``insert()`` - or use ``db.atomic()`` - while your transaction is open, they participate in it using SQLite savepoints instead of committing: exiting an ``atomic()`` block releases its savepoint, but nothing is saved to disk until you commit the outer transaction yourself. If you roll back, their changes are rolled back too. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 49d9f63..aeafe56 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -455,6 +455,31 @@ class Database: self.conn.rollback() raise + def begin(self) -> None: + """ + Start a transaction with ``BEGIN``, taking manual control of transaction + handling. End it by calling :meth:`commit` or :meth:`rollback`. + + Raises ``sqlite3.OperationalError`` if a transaction is already open. + + Most code should use the :meth:`atomic` context manager instead, which + commits and rolls back automatically. See :ref:`python_api_transactions`. + """ + self.execute("BEGIN") + + def commit(self) -> None: + """ + Commit the current transaction. Does nothing if no transaction is open. + """ + self.conn.commit() + + def rollback(self) -> None: + """ + Roll back the current transaction, discarding its changes. Does nothing + if no transaction is open. + """ + self.conn.rollback() + @contextlib.contextmanager def ensure_autocommit_off(self) -> Generator[None, None, None]: """ diff --git a/tests/test_atomic.py b/tests/test_atomic.py index 8c4d18d..b75bc6c 100644 --- a/tests/test_atomic.py +++ b/tests/test_atomic.py @@ -1,6 +1,6 @@ import pytest -from sqlite_utils.db import _iter_complete_sql_statements +from sqlite_utils.db import Database, _iter_complete_sql_statements from sqlite_utils.utils import sqlite3 @@ -189,3 +189,35 @@ def test_atomic_inside_manual_transaction_uses_savepoint(fresh_db): fresh_db["t"].insert({"id": 3}, pk="id") fresh_db.conn.commit() assert [r["id"] for r in fresh_db["t"].rows] == [1, 3] + + +def test_begin_commit_rollback(tmpdir): + path = str(tmpdir / "test.db") + db = Database(path) + db["t"].insert({"id": 1}, pk="id") + db.begin() + db["t"].insert({"id": 2}, pk="id") + assert db.conn.in_transaction + db.rollback() + assert not db.conn.in_transaction + assert [r["id"] for r in db["t"].rows] == [1] + db.begin() + db["t"].insert({"id": 3}, pk="id") + db.commit() + db.close() + db2 = Database(path) + assert [r["id"] for r in db2["t"].rows] == [1, 3] + db2.close() + + +def test_begin_inside_transaction_errors(fresh_db): + fresh_db.begin() + with pytest.raises(sqlite3.OperationalError): + fresh_db.begin() + fresh_db.rollback() + + +def test_commit_and_rollback_without_transaction_are_noops(fresh_db): + fresh_db.commit() + fresh_db.rollback() + assert not fresh_db.conn.in_transaction From 96793668ed89b47f5a299105d37ab55cebeb70f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:32:36 +0000 Subject: [PATCH 28/34] Raise TransactionError instead of RuntimeError from enable_wal/disable_wal A dedicated sqlite_utils.db.TransactionError exception is clearer to catch than a generic RuntimeError, and gives future transaction-state guards a home. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 2 +- docs/python-api.rst | 4 ++-- docs/upgrading.rst | 2 +- sqlite_utils/db.py | 10 +++++++--- tests/test_wal.py | 5 +++-- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index baff4af..f6f97b6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -14,7 +14,7 @@ Breaking changes: - ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. - Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. - ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. -- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``RuntimeError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. +- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. - The ``View`` class no longer has an ``enable_fts()`` method. It existed only to raise ``NotImplementedError``, since full-text search is not supported for views - calling it now raises ``AttributeError`` instead, and the method no longer appears in the API reference. The ``sqlite-utils enable-fts`` command shows a clean error when pointed at a view. - The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. diff --git a/docs/python-api.rst b/docs/python-api.rst index 400c0fa..810f846 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -388,7 +388,7 @@ The library will never commit a transaction you opened. If you call write method Two related safeguards to be aware of: -- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``RuntimeError`` if called while a transaction is open, because changing the journal mode would commit it as a side effect. +- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, because changing the journal mode would commit it as a side effect. - Closing the database - explicitly with ``db.close()``, or by exiting a ``with Database(...) as db:`` block - rolls back any transaction that is still open, see :ref:`python_api_close`. Supported connection modes @@ -2798,7 +2798,7 @@ You can disable WAL mode using ``.disable_wal()``: Database("my_database.db").disable_wal() -The journal mode can only be changed outside of a transaction. Calling either method while a transaction is open - inside a ``db.atomic()`` block, for example - raises a ``RuntimeError``, unless the database is already in the requested mode in which case the call is a no-op. +The journal mode can only be changed outside of a transaction. Calling either method while a transaction is open - inside a ``db.atomic()`` block, for example - raises a ``sqlite_utils.db.TransactionError``, unless the database is already in the requested mode in which case the call is a no-op. You can check the current journal mode for a database using the ``journal_mode`` property: diff --git a/docs/upgrading.rst b/docs/upgrading.rst index e6f4d4d..a18a88e 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -77,7 +77,7 @@ Python API changes **Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation - the full model is described in :ref:`python_api_transactions`. Changes you may notice: - Multi-step operations such as ``table.transform()`` no longer commit an existing transaction you have open - they use savepoints inside it instead. -- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``RuntimeError`` if called while a transaction is open, instead of silently committing it. +- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, instead of silently committing it. - Using ``Database`` as a context manager (``with Database(path) as db:``) closes the connection on exit *without* committing - uncommitted changes from raw ``db.execute()`` calls are rolled back. Packaging changes diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index aeafe56..d53e758 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -301,6 +301,10 @@ class InvalidColumns(Exception): "Specified columns do not exist" +class TransactionError(Exception): + "Operation cannot be performed while a transaction is open" + + class DescIndex(str): pass @@ -891,7 +895,7 @@ class Database: """ Sets ``journal_mode`` to ``'wal'`` to enable Write-Ahead Log mode. - :raises RuntimeError: if called while a transaction is open - the + :raises TransactionError: if called while a transaction is open - the journal mode can only be changed outside of a transaction """ if self.journal_mode != "wal": @@ -903,7 +907,7 @@ class Database: """ Sets ``journal_mode`` back to ``'delete'`` to disable Write-Ahead Log mode. - :raises RuntimeError: if called while a transaction is open - the + :raises TransactionError: if called while a transaction is open - the journal mode can only be changed outside of a transaction """ if self.journal_mode != "delete": @@ -916,7 +920,7 @@ class Database: # any open transaction as a side effect - breaking the rollback # guarantee of atomic() and of user-managed transactions if self.conn.in_transaction: - raise RuntimeError( + raise TransactionError( "{} cannot be used while a transaction is open".format(operation) ) diff --git a/tests/test_wal.py b/tests/test_wal.py index fa3c855..ee7ecf0 100644 --- a/tests/test_wal.py +++ b/tests/test_wal.py @@ -1,5 +1,6 @@ import pytest from sqlite_utils import Database +from sqlite_utils.db import TransactionError @pytest.fixture @@ -26,7 +27,7 @@ def test_enable_disable_wal(db_path_tmpdir): def test_enable_wal_inside_transaction_raises(db_path_tmpdir): db, path, tmpdir = db_path_tmpdir db["test"].insert({"id": 1}, pk="id") - with pytest.raises(RuntimeError): + with pytest.raises(TransactionError): with db.atomic(): db["test"].insert({"id": 2}, pk="id") db.enable_wal() @@ -40,7 +41,7 @@ def test_disable_wal_inside_transaction_raises(db_path_tmpdir): db, path, tmpdir = db_path_tmpdir db.enable_wal() db["test"].insert({"id": 1}, pk="id") - with pytest.raises(RuntimeError): + with pytest.raises(TransactionError): with db.atomic(): db["test"].insert({"id": 2}, pk="id") db.disable_wal() From bcd9a26560ff9fad9993a3b5bf90c5f238982903 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:38:31 +0000 Subject: [PATCH 29/34] Reject Python 3.12+ autocommit connections in the Database constructor Connections created with sqlite3.connect(autocommit=True) make commit() a documented no-op, and autocommit=False connections are permanently inside a transaction - in both modes every write made by this library appeared to work in-process but was silently discarded when the connection closed. Database() now raises TransactionError for these connections instead of losing data. Tests are skipped on Python versions before 3.12, where the autocommit parameter does not exist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/python-api.rst | 8 +++++++- docs/upgrading.rst | 1 + sqlite_utils/db.py | 12 ++++++++++++ tests/test_constructor.py | 30 ++++++++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index f6f97b6..13bcc62 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -17,6 +17,7 @@ Breaking changes: - ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions. - The ``View`` class no longer has an ``enable_fts()`` method. It existed only to raise ``NotImplementedError``, since full-text search is not supported for views - calling it now raises ``AttributeError`` instead, and the method no longer appears in the API reference. The ``sqlite-utils enable-fts`` command shows a clean error when pointed at a view. - The no-op ``-d/--detect-types`` flag has been removed from the ``insert`` and ``upsert`` commands. Type detection has been the default for CSV/TSV data since 4.0a1, so the flag did nothing - invocations using it should simply drop it. ``--no-detect-types`` remains available to disable detection. +- ``Database()`` now raises a ``sqlite_utils.db.TransactionError`` if passed a connection created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options. ``commit()`` and ``rollback()`` behave differently on those connections, which previously caused every write made by the library to be silently discarded when the connection closed. Everything else: diff --git a/docs/python-api.rst b/docs/python-api.rst index 810f846..6041460 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -95,6 +95,8 @@ Instead of a file path you can pass in an existing SQLite connection: db = Database(sqlite3.connect("my_database.db")) +The connection must use Python's default transaction handling. Connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options are rejected with a ``sqlite_utils.db.TransactionError`` - see :ref:`python_api_transactions_modes`. + If you want to create an in-memory database, you can do so like this: .. code-block:: python @@ -391,10 +393,14 @@ Two related safeguards to be aware of: - ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, because changing the journal mode would commit it as a side effect. - Closing the database - explicitly with ``db.close()``, or by exiting a ``with Database(...) as db:`` block - rolls back any transaction that is still open, see :ref:`python_api_close`. +.. _python_api_transactions_modes: + Supported connection modes -------------------------- -``db.atomic()`` and the automatic per-method transactions are designed for connections in Python's default transaction handling mode. Connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options are not supported, because ``commit()`` and ``rollback()`` behave differently on those connections. +``db.atomic()`` and the automatic per-method transactions require a connection in Python's default transaction handling mode. Passing a connection created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options to ``Database()`` raises a ``sqlite_utils.db.TransactionError``. + +This is because ``commit()`` and ``rollback()`` behave differently on those connections - under ``autocommit=True`` they are documented no-ops - which would cause every write made by this library to be silently discarded when the connection closed, rather than failing loudly. .. _python_api_table: diff --git a/docs/upgrading.rst b/docs/upgrading.rst index a18a88e..c9380f0 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -79,6 +79,7 @@ Python API changes - Multi-step operations such as ``table.transform()`` no longer commit an existing transaction you have open - they use savepoints inside it instead. - ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, instead of silently committing it. - Using ``Database`` as a context manager (``with Database(path) as db:``) closes the connection on exit *without* committing - uncommitted changes from raw ``db.execute()`` calls are rolled back. +- ``Database()`` rejects connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options, raising ``sqlite_utils.db.TransactionError``. On those connections every write the library made was silently discarded when the connection closed. Packaging changes ----------------- diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index d53e758..1395433 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -402,6 +402,18 @@ class Database: if recreate: raise ValueError("recreate cannot be used with connections, only paths") self.conn = cast(sqlite3.Connection, filename_or_conn) + # Python 3.12+ autocommit=True/False connections make commit() + # and rollback() behave differently, silently breaking the + # transaction handling used by every write method + autocommit = getattr(self.conn, "autocommit", None) + if autocommit is not None and autocommit != getattr( + sqlite3, "LEGACY_TRANSACTION_CONTROL", -1 + ): + raise TransactionError( + "sqlite-utils requires a connection that uses the default " + "transaction handling - connections created with " + "autocommit=True or autocommit=False are not supported" + ) self._tracer: Optional[Tracer] = tracer if recursive_triggers: self.execute("PRAGMA recursive_triggers=on;") diff --git a/tests/test_constructor.py b/tests/test_constructor.py index 0798996..c7d0fc0 100644 --- a/tests/test_constructor.py +++ b/tests/test_constructor.py @@ -1,6 +1,8 @@ from sqlite_utils import Database +from sqlite_utils.db import TransactionError from sqlite_utils.utils import sqlite3 import pytest +import sys def test_recursive_triggers(): @@ -54,3 +56,31 @@ def test_database_close(tmpdir, memory): db.close() with pytest.raises(sqlite3.ProgrammingError): db.execute("select 1 + 1") + + +@pytest.mark.skipif( + sys.version_info < (3, 12), + reason="sqlite3.connect(autocommit=) requires Python 3.12", +) +@pytest.mark.parametrize("autocommit", [True, False]) +def test_autocommit_connections_are_rejected(tmpdir, autocommit): + # These connection modes break commit()/rollback() in ways that + # silently lose data, so the constructor refuses them + conn = sqlite3.connect(str(tmpdir / "test.db"), autocommit=autocommit) + with pytest.raises(TransactionError): + Database(conn) + conn.close() + + +@pytest.mark.skipif( + sys.version_info < (3, 12), + reason="sqlite3.LEGACY_TRANSACTION_CONTROL requires Python 3.12", +) +def test_legacy_transaction_control_connection_is_accepted(tmpdir): + conn = sqlite3.connect( + str(tmpdir / "test.db"), autocommit=sqlite3.LEGACY_TRANSACTION_CONTROL + ) + db = Database(conn) + db["t"].insert({"id": 1}, pk="id") + assert [r["id"] for r in db["t"].rows] == [1] + db.close() From f7ff3e2027aefb9905ebb2e611e5bbb0a62382c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 23:15:01 +0000 Subject: [PATCH 30/34] db.execute() write statements now commit automatically Raw write statements previously opened an implicit transaction that stayed open until something committed it - the write was visible on the same connection, making it look saved, but was silently rolled back when the connection closed. execute() now commits any implicit transaction it opens, so raw writes behave like every other write in the library: committed as soon as they run, unless an explicit transaction (db.begin() or db.atomic()) is open, in which case they join it. Row-returning writes such as INSERT ... RETURNING via db.query() commit once their rows have been iterated. atomic(), commit() and rollback() now issue literal COMMIT/ROLLBACK statements, which have identical semantics in every sqlite3 connection mode. The CLI bulk command uses db.atomic() instead of 'with db.conn:'. This simplifies the transaction contract to: everything commits immediately unless you explicitly opened a transaction. Docs updated throughout; changelog and upgrading guide cover the breaking change for code that relied on rolling back uncommitted execute() writes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/changelog.rst | 1 + docs/python-api.rst | 38 +++++++++++++++--------------- docs/upgrading.rst | 3 ++- sqlite_utils/cli.py | 2 +- sqlite_utils/db.py | 49 +++++++++++++++++++++++++++++++++------ tests/test_atomic.py | 45 +++++++++++++++++++++++++++++++++-- tests/test_constructor.py | 9 ++++--- 7 files changed, 114 insertions(+), 33 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 13bcc62..fbc58d8 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -11,6 +11,7 @@ Unreleased Breaking changes: +- Write statements executed with ``db.execute()`` are now committed automatically, unless a transaction is already open in which case they join it. Previously they opened an implicit transaction that stayed open until something committed it - writes appeared to work when read on the same connection but were silently rolled back when the connection closed. Code that relied on rolling back uncommitted ``db.execute()`` writes should use the new ``db.begin()`` method to open an explicit transaction first. The transaction model is documented in full at :ref:`python_api_transactions`. - ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. - Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead. - ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. diff --git a/docs/python-api.rst b/docs/python-api.rst index 6041460..4b6b43e 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -152,14 +152,13 @@ Exiting the block is equivalent to calling ``db.close()``: the connection is closed and any transaction still open at that point is rolled back. This matches SQLite's own behavior when a connection closes. -This rarely matters in practice. Every method in this library that writes to -the database commits its own changes before returning, so a transaction can -only be left open here if you opened it yourself - through raw ``db.execute()`` -writes or an explicit ``BEGIN``. In that case the decision to commit stays -with you: committing automatically on exit could silently persist -half-finished work, for example if your code returned early from the block. -Commit explicitly with ``db.commit()``, or wrap your writes in -``db.atomic()`` which commits for you. +This rarely matters in practice. Everything that writes to the database - +including raw ``db.execute()`` statements - commits automatically, so a +transaction can only be open here if you explicitly started one with +``db.begin()`` and have not yet committed it. In that case the decision to +commit stays with you: committing automatically on exit could silently +persist half-finished work, for example if your code returned early from the +block. Call ``db.commit()`` when the work is complete. Note this differs from the ``sqlite3.Connection`` context manager in the standard library, which commits on success but does not close the connection. @@ -264,8 +263,8 @@ The ``db.execute()`` and ``db.executescript()`` methods provide wrappers around Other cursor methods such as ``.fetchone()`` and ``.fetchall()`` are also available, see the `standard library documentation `__. -.. warning:: - Unlike the table methods described elsewhere in this documentation, ``db.execute()`` does **not** commit. If you use it to modify the database you are responsible for committing the change yourself - see :ref:`python_api_transactions_execute`. +.. note:: + Write statements executed this way are committed automatically, unless a transaction is already open in which case they become part of it - see :ref:`python_api_transactions_execute`. .. _python_api_parameters: @@ -308,13 +307,12 @@ Every method in this library that writes to the database - ``insert()``, ``upser db.table("news").insert({"headline": "Dog wins award"}) # The new row is already saved - no commit() required -You never need to call ``commit()`` after using these methods, and you do not need to close the database to persist your changes. +The same applies to raw SQL executed with :ref:`db.execute() ` - a write statement is committed as soon as it has run. -There are exactly three situations where you need to think about transactions: +You never need to call ``commit()``, and you do not need to close the database to persist your changes. There are exactly two situations where you need to think about transactions: 1. You want to group several write operations together, so they either all succeed or all fail - use :ref:`db.atomic() `. -2. You are executing raw SQL writes with ``db.execute()``, which does *not* commit automatically - see :ref:`python_api_transactions_execute`. -3. You are :ref:`managing a transaction yourself `, in which case the library will never commit it for you. +2. You are :ref:`managing a transaction yourself ` with ``db.begin()``, in which case nothing is committed until you commit - the library will never commit a transaction you opened. .. _python_api_atomic: @@ -352,21 +350,23 @@ The transaction is opened with a deferred ``BEGIN`` - SQLite takes the necessary Raw SQL writes with db.execute() -------------------------------- -:ref:`db.execute() ` is a thin wrapper around the underlying ``sqlite3`` connection, and it follows that connection's rules rather than this library's: a write statement opens a transaction that stays open until something commits it. Commit explicitly with ``db.commit()``: +Write statements executed with :ref:`db.execute() ` follow the same rule as everything else: they are committed automatically as soon as they have run. .. code-block:: python db.execute("insert into news (headline) values (?)", ["Dog wins award"]) - db.commit() + # Already committed -Or wrap the calls in ``db.atomic()``, which commits for you: +If a transaction is open - because the call happens inside a ``db.atomic()`` block, or after ``db.begin()`` - the statement becomes part of that transaction instead, and commits when the transaction commits: .. code-block:: python with db.atomic(): db.execute("insert into news (headline) values (?)", ["Dog wins award"]) + db.execute("insert into news (headline) values (?)", ["Cat unimpressed"]) + # Both rows committed together -If you do neither, the write can be deceptive: reads on the same connection will see the new row, making it look saved, but the open transaction will be rolled back when the connection closes and the row will be gone. +One corner case: a row-returning write such as ``INSERT ... RETURNING`` executed through ``db.execute()`` cannot be auto-committed, because its rows have not been read yet - call ``db.commit()`` after fetching them, or use :ref:`db.query() ` for those statements, which commits automatically once you have iterated over the results. .. _python_api_transactions_manual: @@ -384,7 +384,7 @@ You can take full manual control using the ``db.begin()``, ``db.commit()`` and ` else: db.rollback() -``db.begin()`` raises ``sqlite3.OperationalError`` if a transaction is already open. ``db.commit()`` and ``db.rollback()`` do nothing if there is no open transaction. A raw write executed with ``db.execute()`` (as above) opens a transaction implicitly, without needing ``db.begin()``. +``db.begin()`` raises ``sqlite3.OperationalError`` if a transaction is already open. ``db.commit()`` and ``db.rollback()`` do nothing if there is no open transaction. The library will never commit a transaction you opened. If you call write methods such as ``insert()`` - or use ``db.atomic()`` - while your transaction is open, they participate in it using SQLite savepoints instead of committing: exiting an ``atomic()`` block releases its savepoint, but nothing is saved to disk until you commit the outer transaction yourself. If you roll back, their changes are rolled back too. diff --git a/docs/upgrading.rst b/docs/upgrading.rst index c9380f0..3109a7c 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -76,9 +76,10 @@ Python API changes **Transaction behavior is now well-defined.** 4.0 introduces the :ref:`db.atomic() ` context manager and uses it consistently for every write operation - the full model is described in :ref:`python_api_transactions`. Changes you may notice: +- Write statements executed with raw ``db.execute()`` calls now commit automatically, unless a transaction is already open in which case they join it. Previously they opened an implicit transaction that nothing committed - if your code used ``db.execute()`` for writes and relied on ``db.conn.rollback()`` to undo them, open an explicit transaction with the new ``db.begin()`` method first. - Multi-step operations such as ``table.transform()`` no longer commit an existing transaction you have open - they use savepoints inside it instead. - ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, instead of silently committing it. -- Using ``Database`` as a context manager (``with Database(path) as db:``) closes the connection on exit *without* committing - uncommitted changes from raw ``db.execute()`` calls are rolled back. +- Using ``Database`` as a context manager (``with Database(path) as db:``) closes the connection on exit *without* committing - a transaction you explicitly opened with ``db.begin()`` and did not commit is rolled back. - ``Database()`` rejects connections created with the Python 3.12+ ``sqlite3.connect(..., autocommit=True)`` or ``autocommit=False`` options, raising ``sqlite_utils.db.TransactionError``. On those connections every write the library made was silently discarded when the connection closed. Packaging changes diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index d609ad5..a30f79a 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -1140,7 +1140,7 @@ def insert_upsert_implementation( else: doc_chunks = [docs] for doc_chunk in doc_chunks: - with db.conn: + with db.atomic(): db.conn.cursor().executemany(bulk_sql, doc_chunk) return diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 1395433..ef367e1 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -324,6 +324,16 @@ CREATE TABLE IF NOT EXISTS "{}"( """.strip() +_TRANSACTION_CONTROL_PREFIXES = ( + "BEGIN", + "COMMIT", + "END", + "ROLLBACK", + "SAVEPOINT", + "RELEASE", +) + + class Database: """ Wrapper for a SQLite database connection that adds a variety of useful utility methods. @@ -462,13 +472,13 @@ class Database: try: yield self except BaseException: - self.conn.rollback() + self.conn.execute("ROLLBACK") raise else: try: - self.conn.commit() + self.conn.execute("COMMIT") except BaseException: - self.conn.rollback() + self.conn.execute("ROLLBACK") raise def begin(self) -> None: @@ -487,14 +497,16 @@ class Database: """ Commit the current transaction. Does nothing if no transaction is open. """ - self.conn.commit() + if self.conn.in_transaction: + self.conn.execute("COMMIT") def rollback(self) -> None: """ Roll back the current transaction, discarding its changes. Does nothing if no transaction is open. """ - self.conn.rollback() + if self.conn.in_transaction: + self.conn.execute("ROLLBACK") @contextlib.contextmanager def ensure_autocommit_off(self) -> Generator[None, None, None]: @@ -648,6 +660,7 @@ class Database: :raises ValueError: if the SQL statement does not return rows - use :meth:`execute` for those statements instead """ + was_in_transaction = self.conn.in_transaction cursor = self.execute(sql, params or tuple()) if cursor.description is None: raise ValueError( @@ -659,6 +672,11 @@ class Database: def rows() -> Generator[dict, None, None]: for row in cursor: yield dict(zip(keys, row)) + if not was_in_transaction and self.conn.in_transaction: + # A row-returning write such as INSERT ... RETURNING opened + # an implicit transaction - commit it now that its rows have + # been consumed + self.conn.execute("COMMIT") return rows() @@ -668,16 +686,33 @@ class Database: """ Execute SQL query and return a ``sqlite3.Cursor``. + A write statement - ``INSERT``, ``UPDATE``, ``CREATE TABLE`` and so on - + is committed automatically, unless a transaction is already open, in + which case it becomes part of that transaction. See + :ref:`python_api_transactions`. + :param sql: SQL query to execute :param parameters: Parameters to use in that query - an iterable for ``where id = ?`` parameters, or a dictionary for ``where id = :id`` """ if self._tracer: self._tracer(sql, parameters) + was_in_transaction = self.conn.in_transaction if parameters is not None: - return self.conn.execute(sql, parameters) + cursor = self.conn.execute(sql, parameters) else: - return self.conn.execute(sql) + cursor = self.conn.execute(sql) + if ( + not was_in_transaction + and self.conn.in_transaction + and cursor.description is None + and not sql.lstrip().upper().startswith(_TRANSACTION_CONTROL_PREFIXES) + ): + # The statement opened an implicit transaction - commit it, so + # that execute() behaves consistently with the rest of the + # library and identically across connection modes + self.conn.execute("COMMIT") + return cursor def executescript(self, sql: str) -> sqlite3.Cursor: """ diff --git a/tests/test_atomic.py b/tests/test_atomic.py index b75bc6c..a8e3563 100644 --- a/tests/test_atomic.py +++ b/tests/test_atomic.py @@ -181,13 +181,13 @@ def test_atomic_inside_manual_transaction_uses_savepoint(fresh_db): fresh_db["t"].insert({"id": 2}, pk="id") # Nothing is committed until the user's own transaction commits assert fresh_db.conn.in_transaction - fresh_db.conn.rollback() + fresh_db.rollback() assert [r["id"] for r in fresh_db["t"].rows] == [1] # And with a commit instead, the atomic block's writes persist fresh_db.execute("begin") with fresh_db.atomic(): fresh_db["t"].insert({"id": 3}, pk="id") - fresh_db.conn.commit() + fresh_db.commit() assert [r["id"] for r in fresh_db["t"].rows] == [1, 3] @@ -221,3 +221,44 @@ def test_commit_and_rollback_without_transaction_are_noops(fresh_db): fresh_db.commit() fresh_db.rollback() assert not fresh_db.conn.in_transaction + + +def test_execute_write_commits_immediately(tmpdir): + path = str(tmpdir / "test.db") + db = Database(path) + db["t"].insert({"id": 1}, pk="id") + db.execute("insert into t (id) values (2)") + # No implicit transaction is left open + assert not db.conn.in_transaction + # A completely separate connection sees the row straight away + other = sqlite3.connect(path) + assert other.execute("select count(*) from t").fetchone()[0] == 2 + other.close() + db.close() + + +def test_execute_write_respects_explicit_transaction(fresh_db): + fresh_db["t"].insert({"id": 1}, pk="id") + fresh_db.begin() + fresh_db.execute("insert into t (id) values (2)") + # Still inside the explicit transaction - not committed + assert fresh_db.conn.in_transaction + fresh_db.rollback() + assert [r["id"] for r in fresh_db["t"].rows] == [1] + + +def test_query_returning_commits_after_iteration(tmpdir): + if sqlite3.sqlite_version_info < (3, 35, 0): + import pytest as _pytest + + _pytest.skip("RETURNING requires SQLite 3.35.0 or higher") + path = str(tmpdir / "test.db") + db = Database(path) + db["t"].insert({"id": 1}, pk="id") + rows = list(db.query("insert into t (id) values (2) returning id")) + assert rows == [{"id": 2}] + assert not db.conn.in_transaction + other = sqlite3.connect(path) + assert other.execute("select count(*) from t").fetchone()[0] == 2 + other.close() + db.close() diff --git a/tests/test_constructor.py b/tests/test_constructor.py index c7d0fc0..7714f26 100644 --- a/tests/test_constructor.py +++ b/tests/test_constructor.py @@ -35,14 +35,17 @@ def test_database_context_manager(tmpdir): path = str(tmpdir / "test.db") with Database(path) as db: db["t"].insert({"id": 1}) - # A raw write left uncommitted on purpose: + # Raw writes commit automatically too db.execute("insert into t (id) values (2)") + # An explicitly opened transaction left uncommitted on purpose: + db.begin() + db.execute("insert into t (id) values (3)") # The connection is closed... with pytest.raises(sqlite3.ProgrammingError): db.execute("select 1") - # ... and the uncommitted change was rolled back, not committed + # ... and the open explicit transaction was rolled back, not committed db2 = Database(path) - assert [r["id"] for r in db2["t"].rows] == [1] + assert [r["id"] for r in db2["t"].rows] == [1, 2] db2.close() From 2510745de42d67636bd9e9e54d3c1b513087a6f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 23:19:51 +0000 Subject: [PATCH 31/34] Add --sqlite-autocommit pytest option Runs the entire test suite against connections created with the Python 3.12+ sqlite3.connect(autocommit=True) mode, by patching connect() to default autocommit=True for internally-created connections. Errors on older Python versions. The full suite passes in both modes with no test changes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- tests/conftest.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 6fd35b8..728db7b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,11 +8,36 @@ create table Gosh2 (c1 text, c2 text, c3 text); """ +def pytest_addoption(parser): + parser.addoption( + "--sqlite-autocommit", + action="store_true", + default=False, + help=( + "Run every test against connections created with the Python 3.12+ " + "sqlite3.connect(autocommit=True) mode" + ), + ) + + def pytest_configure(config): import sys sys._called_from_test = True # type: ignore[attr-defined] + if config.getoption("--sqlite-autocommit"): + if sys.version_info < (3, 12): + raise pytest.UsageError( + "--sqlite-autocommit requires Python 3.12 or higher" + ) + real_connect = sqlite3.connect + + def autocommit_connect(*args, **kwargs): + kwargs.setdefault("autocommit", True) + return real_connect(*args, **kwargs) + + sqlite3.connect = autocommit_connect + @pytest.fixture(autouse=True) def close_all_databases(): From b49af65a6d2100fa484dec71ff35495aa820e967 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 23:20:31 +0000 Subject: [PATCH 32/34] Fix PytestRemovedIn10Warning in test_sniff parametrize received a generator from Path.glob(), which pytest 10 will reject. sorted() materializes the list and makes the parameter order deterministic as a bonus. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- tests/test_sniff.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_sniff.py b/tests/test_sniff.py index 62fac86..4bbdb66 100644 --- a/tests/test_sniff.py +++ b/tests/test_sniff.py @@ -6,7 +6,7 @@ import pytest sniff_dir = pathlib.Path(__file__).parent / "sniff" -@pytest.mark.parametrize("filepath", sniff_dir.glob("example*")) +@pytest.mark.parametrize("filepath", sorted(sniff_dir.glob("example*"))) def test_sniff(tmpdir, filepath): db_path = str(tmpdir / "test.db") runner = CliRunner() From 89af511f799ee235d1d35e00fc0557ef98794792 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 4 Jul 2026 16:24:07 -0700 Subject: [PATCH 33/34] Also run "pytest --sqlite-autocommit" for one matrix item in CI To test that our code works correctly with the new autocommit option introduced in Python 3.13+ https://github.com/simonw/sqlite-utils/pull/767#discussion_r3523983636 --- .github/workflows/test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0f65d6a..7e1e953 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -41,6 +41,9 @@ jobs: - name: Run tests run: | pytest -v + - name: Run autocommit tests just on 3.14/Ubuntu + if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.14' + run: pytest --sqlite-autocommit - name: run mypy run: mypy sqlite_utils tests - name: run flake8 From 7a015cff52f0f740bdf1bb8a83ad5e8183db3e20 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 23:33:38 +0000 Subject: [PATCH 34/34] Unwrap hard-wrapped prose in the documentation The documentation style for this project is one line per paragraph, not text-wrapped RST. Unwraps three recently added paragraphs in python-api.rst and two older ones in plugins.rst. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd --- docs/plugins.rst | 7 ++----- docs/python-api.rst | 16 +++------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/docs/plugins.rst b/docs/plugins.rst index 6357ad9..99588ee 100644 --- a/docs/plugins.rst +++ b/docs/plugins.rst @@ -140,9 +140,7 @@ As a special niche feature, if your plugin needs to import some files and then a prepare_connection(conn) ~~~~~~~~~~~~~~~~~~~~~~~~ -This hook is called when a new SQLite database connection is created. You can -use it to `register custom SQL functions `_, -aggregates and collations. For example: +This hook is called when a new SQLite database connection is created. You can use it to `register custom SQL functions `_, aggregates and collations. For example: .. code-block:: python @@ -154,8 +152,7 @@ aggregates and collations. For example: "hello", 1, lambda name: f"Hello, {name}!" ) -This registers a SQL function called ``hello`` which takes a single -argument and can be called like this: +This registers a SQL function called ``hello`` which takes a single argument and can be called like this: .. code-block:: sql diff --git a/docs/python-api.rst b/docs/python-api.rst index 4b6b43e..e9eb371 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -148,21 +148,11 @@ The ``Database`` object also works as a context manager, which will automaticall db["my_table"].insert({"name": "Example"}) # Connection is automatically closed here -Exiting the block is equivalent to calling ``db.close()``: the connection is -closed and any transaction still open at that point is rolled back. This -matches SQLite's own behavior when a connection closes. +Exiting the block is equivalent to calling ``db.close()``: the connection is closed and any transaction still open at that point is rolled back. This matches SQLite's own behavior when a connection closes. -This rarely matters in practice. Everything that writes to the database - -including raw ``db.execute()`` statements - commits automatically, so a -transaction can only be open here if you explicitly started one with -``db.begin()`` and have not yet committed it. In that case the decision to -commit stays with you: committing automatically on exit could silently -persist half-finished work, for example if your code returned early from the -block. Call ``db.commit()`` when the work is complete. +This rarely matters in practice. Everything that writes to the database - including raw ``db.execute()`` statements - commits automatically, so a transaction can only be open here if you explicitly started one with ``db.begin()`` and have not yet committed it. In that case the decision to commit stays with you: committing automatically on exit could silently persist half-finished work, for example if your code returned early from the block. Call ``db.commit()`` when the work is complete. -Note this differs from the ``sqlite3.Connection`` context manager in the -standard library, which commits on success but does not close the connection. -See :ref:`python_api_transactions` for the full transaction model. +Note this differs from the ``sqlite3.Connection`` context manager in the standard library, which commits on success but does not close the connection. See :ref:`python_api_transactions` for the full transaction model. .. _python_api_attach: