mirror of
https://github.com/simonw/datasette.git
synced 2026-07-13 11:04:37 +02:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR
This commit is contained in:
Claude
parent
1ff4e67b79
commit
26d326c709
3 changed files with 119 additions and 53 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue