From 26d326c709a9053af81d8cf214899cf7eebe8834 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 05:57:32 +0000 Subject: [PATCH] Write API atomicity regression tests, remove manual transaction in alter Adds regression tests confirming the JSON write API is atomic per request now that write tasks open an explicit transaction: /db/-/create with failing initial rows creates no table, a failing operation in /db/table/-/alter rolls back earlier operations, and insert with "return": true rolls back all rows if one fails. Also removes the "with operation_conn:" block from the alter endpoint - write functions run inside the task transaction and should not manage transactions themselves (that context manager would commit the task transaction early on success). Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/views/table_create_alter.py | 106 +++++++++++++------------- docs/changelog.rst | 1 + tests/test_api_write.py | 65 ++++++++++++++++ 3 files changed, 119 insertions(+), 53 deletions(-) diff --git a/datasette/views/table_create_alter.py b/datasette/views/table_create_alter.py index 4deeafcc..f3e1779f 100644 --- a/datasette/views/table_create_alter.py +++ b/datasette/views/table_create_alter.py @@ -1261,62 +1261,62 @@ class TableAlterView(BaseView): elif operation.op == "set_foreign_keys": foreign_keys = [fk.tuple for fk in args.foreign_keys] - with operation_conn: - for column in add_columns: - not_null_default = None - if column.not_null: - if "default_expr" in column.model_fields_set: - not_null_default = _default_expression_sql( - column.default_expr - ) - else: - not_null_default = _literal_default( - db_for_write, column.default - ) - table.add_column( - column.name, - column.type, - not_null_default=not_null_default, - ) + # The write task transaction makes these operations atomic + for column in add_columns: + not_null_default = None + if column.not_null: + if "default_expr" in column.model_fields_set: + not_null_default = _default_expression_sql( + column.default_expr + ) + else: + not_null_default = _literal_default( + db_for_write, column.default + ) + table.add_column( + column.name, + column.type, + not_null_default=not_null_default, + ) - should_transform = any( - ( - types, - rename, - drop, - not_null, - defaults, - column_order is not None, - pk is not SQLITE_UTILS_DEFAULT, - add_foreign_keys, - drop_foreign_keys, - foreign_keys is not None, + should_transform = any( + ( + types, + rename, + drop, + not_null, + defaults, + column_order is not None, + pk is not SQLITE_UTILS_DEFAULT, + add_foreign_keys, + drop_foreign_keys, + foreign_keys is not None, + ) + ) + if should_transform: + table.transform( + types=types or None, + rename=rename or None, + drop=drop or None, + pk=pk, + not_null=not_null or None, + defaults=defaults or None, + column_order=column_order, + add_foreign_keys=add_foreign_keys or None, + drop_foreign_keys=drop_foreign_keys or None, + foreign_keys=foreign_keys, + ) + if ( + rename_table_to is not None + and rename_table_to != current_table_name + ): + operation_conn.execute( + "alter table {} rename to {}".format( + escape_sqlite(current_table_name), + escape_sqlite(rename_table_to), ) ) - if should_transform: - table.transform( - types=types or None, - rename=rename or None, - drop=drop or None, - pk=pk, - not_null=not_null or None, - defaults=defaults or None, - column_order=column_order, - add_foreign_keys=add_foreign_keys or None, - drop_foreign_keys=drop_foreign_keys or None, - foreign_keys=foreign_keys, - ) - if ( - rename_table_to is not None - and rename_table_to != current_table_name - ): - operation_conn.execute( - "alter table {} rename to {}".format( - escape_sqlite(current_table_name), - escape_sqlite(rename_table_to), - ) - ) - current_table_name = rename_table_to + current_table_name = rename_table_to return current_table_name, _table_schema_from_conn( operation_conn, current_table_name diff --git a/docs/changelog.rst b/docs/changelog.rst index 1f124fcc..52da8d3e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -12,6 +12,7 @@ Unreleased - Write functions run via ``await db.execute_write_fn()`` now execute inside an explicitly opened ``BEGIN IMMEDIATE`` transaction, committed when the function returns or rolled back if it raises. Previously the transaction was only opened implicitly by the first raw data-modifying statement, which meant writes made through sqlite-utils committed independently mid-task - a function that used sqlite-utils and then failed could leave those writes permanently committed. sqlite-utils write methods now nest inside the task transaction as savepoints, so a failing write function rolls back everything it did. Functions run with ``transaction=True`` should no longer manage transactions themselves - use ``transaction=False`` for manual transaction control. (:issue:`2831`) - ``await db.execute_write()`` detects statements that SQLite cannot execute inside a transaction - ``VACUUM``, ``ATTACH``, ``DETACH`` and ``PRAGMA`` - and runs them in autocommit mode instead. (:issue:`2831`) - ``await db.execute_write_script()`` is now transactional, matching its documentation: if any statement in the script fails, none of its statements are applied. Scripts containing statements that cannot run inside a transaction, or that manage transactions themselves, fall back to the previous ``conn.executescript()`` autocommit behavior. (:issue:`2831`) +- The JSON write API is now atomic per request: ``/db/-/create`` with initial rows, multi-operation ``/db/table/-/alter`` calls and inserts using ``"return": true`` now either fully apply or roll back entirely if any part fails. Previously a failure part way through could leave earlier writes from the same request permanently committed. (:issue:`2831`) .. _v1_0_a36: diff --git a/tests/test_api_write.py b/tests/test_api_write.py index a803fbbc..691f7ac9 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -2717,3 +2717,68 @@ async def test_create_using_alter_against_existing_table( insert_rows_event = ds_write._tracked_events[1] assert insert_rows_event.name == "insert-rows" assert insert_rows_event.num_rows == 1 + + +@pytest.mark.asyncio +async def test_create_table_with_failing_rows_is_atomic(ds_write): + # https://github.com/simonw/datasette/issues/2831 + # If inserting the initial rows fails, the create table should + # be rolled back as well + token = write_token(ds_write) + response = await ds_write.client.post( + "/data/-/create", + json={ + "table": "atomic_create", + "rows": [{"id": 1, "name": "one"}, {"id": 1, "name": "dupe"}], + "pk": "id", + }, + headers=_headers(token), + ) + assert response.status_code == 400 + assert not await ds_write.get_database("data").table_exists("atomic_create") + + +@pytest.mark.asyncio +async def test_alter_table_with_failing_operation_is_atomic(ds_write): + # https://github.com/simonw/datasette/issues/2831 + # If a later operation fails, earlier operations should be rolled back + token = write_token(ds_write) + response = await ds_write.client.post( + "/data/docs/-/alter", + json={ + "operations": [ + {"op": "add_column", "args": {"name": "new_col", "type": "text"}}, + # Fails - column "title" already exists + {"op": "add_column", "args": {"name": "title", "type": "text"}}, + ] + }, + headers=_headers(token), + ) + assert response.status_code == 400 + columns = await ds_write.get_database("data").table_columns("docs") + assert "new_col" not in columns + + +@pytest.mark.asyncio +async def test_insert_with_return_failing_row_is_atomic(ds_write): + # https://github.com/simonw/datasette/issues/2831 + # Insert with "return": true runs one insert per row - a failure + # part way through should roll back the earlier rows + token = write_token(ds_write) + response = await ds_write.client.post( + "/data/docs/-/insert", + json={ + "rows": [ + {"id": 1, "title": "one"}, + {"id": 2, "title": "two"}, + {"id": 2, "title": "dupe"}, + ], + "return": True, + }, + headers=_headers(token), + ) + assert response.status_code == 400 + count = ( + await ds_write.get_database("data").execute("select count(*) from docs") + ).single_value() + assert count == 0