From 509d87b292407ad7350b8fd0e1c57c27b39b7ca8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 19:56:42 +0000 Subject: [PATCH] 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.