From 84fb4221ab9510f32f6f19c943d7e40c3b6154ec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 05:50:26 +0000 Subject: [PATCH 01/17] Open write task transactions explicitly with BEGIN IMMEDIATE Write tasks with transaction=True previously relied on the sqlite3 driver's implicit BEGIN, which only fires on the first raw data-modifying statement. sqlite-utils 4.0 write methods found no open transaction, committed their own work mid-task, and Datasette's commit/rollback at task end was a no-op - so a failing write function could leave partial writes permanently committed. The write thread (and the non-threaded write path) now executes BEGIN IMMEDIATE before invoking each transaction=True task, commits when it returns and rolls back if it raises. sqlite-utils methods nest inside that transaction as savepoints, restoring task-level atomicity for every write path. execute_write() now detects statements SQLite refuses to run inside a transaction (VACUUM, ATTACH, DETACH, PRAGMA) and runs those in autocommit mode, preserving previous behavior for e.g. trusted canned queries that run VACUUM. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/database.py | 47 ++++++++++++++-- docs/changelog.rst | 8 +++ docs/internals.rst | 8 ++- tests/test_internals_database.py | 94 ++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 7 deletions(-) diff --git a/datasette/database.py b/datasette/database.py index e7fe1ed9..e8271c63 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -5,6 +5,7 @@ import inspect import os from pathlib import Path import queue +import re import sqlite_utils import sys import tempfile @@ -42,6 +43,39 @@ class DatasetteClosedError(RuntimeError): _SHUTDOWN = object() +# Statements that SQLite refuses to execute inside a transaction. These run +# without the task transaction - a single statement needs no extra atomicity. +_STATEMENTS_DISALLOWED_IN_TRANSACTION = {"vacuum", "attach", "detach", "pragma"} + +_FIRST_KEYWORD_RE = re.compile( + r"^(?:\s+|--[^\n]*(?:\n|$)|/\*.*?\*/)*([a-zA-Z]+)", re.DOTALL +) + + +def _can_execute_in_transaction(sql): + match = _FIRST_KEYWORD_RE.match(sql) + if match is None: + return True + return match.group(1).lower() not in _STATEMENTS_DISALLOWED_IN_TRANSACTION + + +def _run_write_in_transaction(conn, fn): + # Open the transaction explicitly instead of relying on the sqlite3 + # driver's implicit BEGIN, which only fires on the first data-modifying + # statement. With a transaction genuinely open, sqlite-utils write + # methods nest inside it as savepoints instead of committing their own + # transactions mid-task: https://github.com/simonw/datasette/issues/2831 + conn.execute("BEGIN IMMEDIATE") + try: + result = fn(conn) + except Exception: + if conn.in_transaction: + conn.rollback() + raise + if conn.in_transaction: + conn.commit() + return result + class Database: # For table counts stop at this many rows: @@ -258,7 +292,12 @@ class Database: ) with trace("sql", database=self.name, sql=sql.strip(), params=params): - results = await self.execute_write_fn(_inner, block=block, request=request) + results = await self.execute_write_fn( + _inner, + block=block, + transaction=_can_execute_in_transaction(sql), + request=request, + ) return results async def execute_write_script(self, sql, block=True, request=None): @@ -347,8 +386,7 @@ class Database: self._write_connection = self.connect(write=True) self.ds._prepare_connection(self._write_connection, self.name) if transaction: - with self._write_connection: - result = fn(self._write_connection) + result = _run_write_in_transaction(self._write_connection, fn) else: result = fn(self._write_connection) else: @@ -476,8 +514,7 @@ class Database: else: try: if task.transaction: - with conn: - result = task.fn(conn) + result = _run_write_in_transaction(conn, task.fn) else: result = task.fn(conn) except Exception as e: diff --git a/docs/changelog.rst b/docs/changelog.rst index e3718543..f58b1c75 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,14 @@ Changelog ========= +.. _v_unreleased: + +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`) + .. _v1_0_a36: 1.0a36 (2026-07-07) diff --git a/docs/internals.rst b/docs/internals.rst index 2048c7e4..98fdcd82 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2059,7 +2059,7 @@ If you need to retrieve every row returned by a statement, pass ``return_all=Tru If you pass ``block=False`` this behavior changes to "fire and forget" - queries will be added to the write queue and executed in a separate thread while your code can continue to do other things. The method will return a UUID representing the queued task. -Each call to ``execute_write()`` will be executed inside a transaction. +Each call to ``execute_write()`` will be executed inside a transaction, with the exception of statements that SQLite does not allow to run inside a transaction: ``VACUUM``, ``ATTACH``, ``DETACH`` and ``PRAGMA``. Those statements are executed in autocommit mode instead. .. _database_execute_write_script: @@ -2151,7 +2151,11 @@ The value returned from ``await database.execute_write_fn(...)`` will be the ret If your function raises an exception that exception will be propagated up to the ``await`` line. -By default your function will be executed inside a transaction. You can pass ``transaction=False`` to disable this behavior, though if you do that you should be careful to manually apply transactions - ideally using the ``with conn:`` pattern, or you may see ``OperationalError: database table is locked`` errors. +By default your function will be executed inside a transaction. Datasette executes ``BEGIN IMMEDIATE`` on the write connection before calling your function, then commits the transaction when your function returns - or rolls it back if your function raises an exception. Nothing your function writes will be visible to other connections until that final commit. + +Because the transaction is already open when your function is called, write methods from libraries such as `sqlite-utils `__ will nest their work inside it (sqlite-utils uses savepoints) rather than committing independently, so an exception rolls back everything the function did. + +Your function should not manage transactions itself when ``transaction=True`` - do not execute ``BEGIN`` or call ``conn.commit()`` or ``conn.rollback()`` on the connection. If you need to manage transactions manually, pass ``transaction=False`` - ideally using the ``with conn:`` pattern, or you may see ``OperationalError: database table is locked`` errors. If you specify ``block=False`` the method becomes fire-and-forget, queueing your function to be executed and then allowing your code after the call to ``.execute_write_fn()`` to continue running while the underlying thread waits for an opportunity to run your function. A UUID representing the queued task will be returned. Any exceptions in your code will be silently swallowed. diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index bad4e8ca..1ca5ee77 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -11,6 +11,7 @@ from datasette.database import _deliver_write_result from datasette.utils.sqlite import sqlite3, supports_returning from datasette.utils import Column import pytest +import sqlite_utils import time import uuid @@ -111,6 +112,99 @@ async def test_execute_fn_transaction_false(): await db.execute_write_fn(run, transaction=False) +@pytest.mark.asyncio +async def test_execute_write_fn_wraps_sqlite_utils_writes_in_transaction(): + # https://github.com/simonw/datasette/issues/2831 + # sqlite-utils write methods commit their own transactions unless one is + # already open - the write thread must open one before running each task + # so that a failing task rolls back everything, including those writes. + datasette = Datasette(memory=True) + db = datasette.add_memory_database("test_txn_sqlite_utils") + await db.execute_write("create table t (a integer)") + + def failing_task(conn): + sqlite_utils.Database(conn)["t"].insert_all({"a": i} for i in range(5)) + assert conn.in_transaction + raise ValueError("boom") + + with pytest.raises(ValueError): + await db.execute_write_fn(failing_task) + count = (await db.execute("select count(*) from t")).single_value() + assert count == 0 + # The write connection should be back in autocommit mode + assert ( + await db.execute_write_fn(lambda conn: conn.in_transaction, transaction=False) + ) is False + # And a transaction=True task should see a transaction already open + assert (await db.execute_write_fn(lambda conn: conn.in_transaction)) is True + + +@pytest.mark.asyncio +async def test_execute_write_statements_disallowed_in_transaction(tmp_path): + # VACUUM (and ATTACH/DETACH/PRAGMA) cannot run inside a transaction, so + # execute_write() must run them outside one + # https://github.com/simonw/datasette/issues/2831 + path = str(tmp_path / "test.db") + setup_conn = sqlite3.connect(path) + setup_conn.execute("create table t (a integer)") + setup_conn.close() + datasette = Datasette([path]) + db = datasette.get_database("test") + await db.execute_write("vacuum") + await db.execute_write(" -- a comment\n VACUUM") + # But regular DML statements still run inside a transaction + from datasette.database import _can_execute_in_transaction + + assert _can_execute_in_transaction("insert into t values (1)") + assert _can_execute_in_transaction("with x as (select 1) insert into t select 1") + assert not _can_execute_in_transaction("vacuum") + assert not _can_execute_in_transaction("/* hi */ PRAGMA optimize") + + +@pytest.mark.asyncio +async def test_execute_write_fn_sqlite_utils_integrity_error_rolls_back_task(): + # https://github.com/simonw/datasette/issues/2831 + datasette = Datasette(memory=True) + db = datasette.add_memory_database("test_txn_integrity") + await db.execute_write("create table t (id integer primary key)") + + def two_inserts(conn): + table = sqlite_utils.Database(conn)["t"] + table.insert({"id": 1}) + table.insert({"id": 1}) # IntegrityError + + with pytest.raises(sqlite3.IntegrityError): + await db.execute_write_fn(two_inserts) + count = (await db.execute("select count(*) from t")).single_value() + assert count == 0 + + +@pytest.mark.asyncio +async def test_execute_write_fn_writes_invisible_to_readers_until_task_ends(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + path = str(tmp_path / "test.db") + setup_conn = sqlite3.connect(path) + setup_conn.execute("create table t (a integer)") + setup_conn.close() + datasette = Datasette([path]) + db = datasette.get_database("test") + + def insert_and_check(conn): + sqlite_utils.Database(conn)["t"].insert_all({"a": i} for i in range(5)) + # A separate read-only connection must not see the rows mid-task + reader = sqlite3.connect("file:{}?mode=ro".format(path), uri=True) + try: + return reader.execute("select count(*) from t").fetchone()[0] + finally: + reader.close() + + mid_task_count = await db.execute_write_fn(insert_and_check) + assert mid_task_count == 0 + # After the task commits the rows are visible + count = (await db.execute("select count(*) from t")).single_value() + assert count == 5 + + @pytest.mark.parametrize( "tables,exists", ( From 1ff4e67b790c3d194a65b8cad09fd1fce7967765 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 05:54:02 +0000 Subject: [PATCH 02/17] Make execute_write_script transactional, matching its documentation execute_write_script() was documented as running inside a transaction but actually passed transaction=False and used conn.executescript(), which commits each statement as it executes - a failing script could half-apply. Scripts are now split into complete statements (via sqlite3.complete_statement) and executed one at a time inside the task transaction, so a failing script applies nothing. Scripts containing statements that cannot run in a transaction (VACUUM, ATTACH, DETACH, PRAGMA) or that manage transactions themselves (BEGIN, COMMIT, SAVEPOINT etc) keep the previous executescript() autocommit behavior. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/database.py | 50 ++++++++++++++++++++++++++++++-- docs/changelog.rst | 1 + docs/internals.rst | 6 ++-- tests/test_internals_database.py | 30 +++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/datasette/database.py b/datasette/database.py index e8271c63..82522e6a 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -59,6 +59,44 @@ def _can_execute_in_transaction(sql): return match.group(1).lower() not in _STATEMENTS_DISALLOWED_IN_TRANSACTION +# Scripts that manage their own transactions also cannot run inside the +# task transaction +_SCRIPT_STATEMENTS_DISALLOWED_IN_TRANSACTION = _STATEMENTS_DISALLOWED_IN_TRANSACTION | { + "begin", + "commit", + "end", + "rollback", + "savepoint", + "release", +} + + +def _script_can_execute_in_transaction(statements): + for statement in statements: + match = _FIRST_KEYWORD_RE.match(statement) + if ( + match is not None + and match.group(1).lower() in _SCRIPT_STATEMENTS_DISALLOWED_IN_TRANSACTION + ): + return False + return True + + +def _iter_sql_statements(sql): + # Split a multi-statement SQL string into complete statements using + # sqlite3.complete_statement() + statement = [] + for char in sql: + statement.append(char) + statement_sql = "".join(statement).strip() + if statement_sql and sqlite3.complete_statement(statement_sql): + yield statement_sql + statement = [] + remainder = "".join(statement).strip() + if remainder: + yield remainder + + def _run_write_in_transaction(conn, fn): # Open the transaction explicitly instead of relying on the sqlite3 # driver's implicit BEGIN, which only fires on the first data-modifying @@ -302,13 +340,21 @@ class Database: async def execute_write_script(self, sql, block=True, request=None): self._check_not_closed() + statements = list(_iter_sql_statements(sql)) + transaction = _script_can_execute_in_transaction(statements) def _inner(conn): - return conn.executescript(sql) + if transaction: + # Execute statements one at a time so they run inside the + # task transaction - conn.executescript() would commit it + for statement in statements: + conn.execute(statement) + else: + return conn.executescript(sql) with trace("sql", database=self.name, sql=sql.strip(), executescript=True): results = await self.execute_write_fn( - _inner, block=block, transaction=False, request=request + _inner, block=block, transaction=transaction, request=request ) return results diff --git a/docs/changelog.rst b/docs/changelog.rst index f58b1c75..1f124fcc 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -11,6 +11,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`) .. _v1_0_a36: diff --git a/docs/internals.rst b/docs/internals.rst index 98fdcd82..9a5f3f50 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2066,9 +2066,11 @@ Each call to ``execute_write()`` will be executed inside a transaction, with the await db.execute_write_script(sql, block=True) ---------------------------------------------- -Like ``execute_write()`` but can be used to send multiple SQL statements in a single string separated by semicolons, using the ``sqlite3`` `conn.executescript() `__ method. +Like ``execute_write()`` but can be used to send multiple SQL statements in a single string separated by semicolons. -Each call to ``execute_write_script()`` will be executed inside a transaction. +Each call to ``execute_write_script()`` will be executed inside a transaction - if any statement fails, none of the statements will be applied. + +The exception is scripts that include statements which SQLite cannot execute inside a transaction - ``VACUUM``, ``ATTACH``, ``DETACH``, ``PRAGMA`` - or that manage transactions themselves using ``BEGIN``, ``COMMIT``, ``ROLLBACK``, ``SAVEPOINT`` or ``RELEASE``. Those scripts are executed using the ``sqlite3`` `conn.executescript() `__ method instead, where each statement is committed as it executes. .. _database_execute_write_many: diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 1ca5ee77..74818df9 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -179,6 +179,36 @@ async def test_execute_write_fn_sqlite_utils_integrity_error_rolls_back_task(): assert count == 0 +@pytest.mark.asyncio +async def test_execute_write_script_is_transactional(): + # https://github.com/simonw/datasette/issues/2831 + # A script that fails part way through should apply none of its statements + datasette = Datasette(memory=True) + db = datasette.add_memory_database("test_write_script_txn") + with pytest.raises(sqlite3.OperationalError): + await db.execute_write_script( + "create table one (id integer primary key);\n" + "insert into one (id) values (1);\n" + "insert into no_such_table (id) values (2);" + ) + assert "one" not in await db.table_names() + + +@pytest.mark.asyncio +async def test_execute_write_script_with_transaction_unsafe_statements(tmp_path): + # Scripts containing statements that cannot run inside a transaction + # (VACUUM, BEGIN etc) still execute, using the previous autocommit behavior + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path]) + db = datasette.get_database("test") + await db.execute_write_script("create table t (id integer primary key);\nvacuum;") + assert "t" in await db.table_names() + await db.execute_write_script("begin;\ninsert into t (id) values (1);\ncommit;") + count = (await db.execute("select count(*) from t")).single_value() + assert count == 1 + + @pytest.mark.asyncio async def test_execute_write_fn_writes_invisible_to_readers_until_task_ends(tmp_path): # https://github.com/simonw/datasette/issues/2831 From 26d326c709a9053af81d8cf214899cf7eebe8834 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 05:57:32 +0000 Subject: [PATCH 03/17] 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 From a3f8b440e6fc7333ce78993e94c4d1af264daa64 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 05:59:20 +0000 Subject: [PATCH 04/17] Rebuild internal catalog for a database in a single atomic write task populate_schema_tables() previously ran six separate transactions per database: one task deleting that database's catalog rows followed by five execute_write_many inserts. Readers of the internal database could observe the intermediate state where a database had no catalog rows. Schema details are still collected on a read connection of the target database first; the delete-and-reinsert now happens inside one execute_write_fn task so the rebuild is atomic. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/utils/internal_db.py | 124 +++++++++++++++++---------------- docs/changelog.rst | 1 + tests/test_internal_db.py | 48 +++++++++++++ 3 files changed, 112 insertions(+), 61 deletions(-) diff --git a/datasette/utils/internal_db.py b/datasette/utils/internal_db.py index 10ca32a5..f40c0f4a 100644 --- a/datasette/utils/internal_db.py +++ b/datasette/utils/internal_db.py @@ -183,26 +183,6 @@ async def init_internal_db(db): async def populate_schema_tables(internal_db, db): database_name = db.name - def delete_everything(conn): - conn.execute( - "DELETE FROM catalog_tables WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_views WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_columns WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_foreign_keys WHERE database_name = ?", - [database_name], - ) - conn.execute( - "DELETE FROM catalog_indexes WHERE database_name = ?", [database_name] - ) - - await internal_db.execute_write_fn(delete_everything) - tables = (await db.execute("select * from sqlite_master WHERE type = 'table'")).rows views = (await db.execute("select * from sqlite_master WHERE type = 'view'")).rows @@ -266,47 +246,69 @@ async def populate_schema_tables(internal_db, db): indexes_to_insert, ) = await db.execute_fn(collect_info) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_tables (database_name, table_name, rootpage, sql) - values (?, ?, ?, ?) - """, - tables_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_views (database_name, view_name, rootpage, sql) - values (?, ?, ?, ?) - """, - views_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_columns ( - database_name, table_name, cid, name, type, "notnull", default_value, is_pk, hidden - ) VALUES ( - :database_name, :table_name, :cid, :name, :type, :notnull, :default_value, :is_pk, :hidden + def update_catalog(conn): + # Delete and reinsert as one write task so internal database readers + # never observe a database with missing catalog rows + # https://github.com/simonw/datasette/issues/2831 + conn.execute( + "DELETE FROM catalog_tables WHERE database_name = ?", [database_name] ) - """, - columns_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_foreign_keys ( - database_name, table_name, "id", seq, "table", "from", "to", on_update, on_delete, match - ) VALUES ( - :database_name, :table_name, :id, :seq, :table, :from, :to, :on_update, :on_delete, :match + conn.execute( + "DELETE FROM catalog_views WHERE database_name = ?", [database_name] ) - """, - foreign_keys_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_indexes ( - database_name, table_name, seq, name, "unique", origin, partial - ) VALUES ( - :database_name, :table_name, :seq, :name, :unique, :origin, :partial + conn.execute( + "DELETE FROM catalog_columns WHERE database_name = ?", [database_name] ) - """, - indexes_to_insert, - ) + conn.execute( + "DELETE FROM catalog_foreign_keys WHERE database_name = ?", + [database_name], + ) + conn.execute( + "DELETE FROM catalog_indexes WHERE database_name = ?", [database_name] + ) + conn.executemany( + """ + INSERT INTO catalog_tables (database_name, table_name, rootpage, sql) + values (?, ?, ?, ?) + """, + tables_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_views (database_name, view_name, rootpage, sql) + values (?, ?, ?, ?) + """, + views_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_columns ( + database_name, table_name, cid, name, type, "notnull", default_value, is_pk, hidden + ) VALUES ( + :database_name, :table_name, :cid, :name, :type, :notnull, :default_value, :is_pk, :hidden + ) + """, + columns_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_foreign_keys ( + database_name, table_name, "id", seq, "table", "from", "to", on_update, on_delete, match + ) VALUES ( + :database_name, :table_name, :id, :seq, :table, :from, :to, :on_update, :on_delete, :match + ) + """, + foreign_keys_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_indexes ( + database_name, table_name, seq, name, "unique", origin, partial + ) VALUES ( + :database_name, :table_name, :seq, :name, :unique, :origin, :partial + ) + """, + indexes_to_insert, + ) + + await internal_db.execute_write_fn(update_catalog) diff --git a/docs/changelog.rst b/docs/changelog.rst index 52da8d3e..8cb72005 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -13,6 +13,7 @@ Unreleased - ``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`) +- Rebuilding the internal database catalog for a database is now a single atomic write. Previously the rebuild used six separate transactions, so queries against the internal database could observe a database with missing catalog rows while a rebuild was in progress. (:issue:`2831`) .. _v1_0_a36: diff --git a/tests/test_internal_db.py b/tests/test_internal_db.py index e1ab51bb..f2e953cc 100644 --- a/tests/test_internal_db.py +++ b/tests/test_internal_db.py @@ -334,3 +334,51 @@ async def test_orphan_stale_catalog_child_entries_removed(tmp_path): assert response.status_code == 200 ds2.close() + + +@pytest.mark.asyncio +async def test_populate_schema_tables_is_a_single_write_task(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + # The catalog rebuild should be one atomic write task, so readers of the + # internal database never observe a database with deleted-but-not-yet- + # reinserted catalog rows + from datasette.app import Datasette + from datasette.utils.internal_db import populate_schema_tables + + path = str(tmp_path / "data.db") + conn = sqlite3.connect(path) + conn.execute("create table t (id integer primary key, name text)") + conn.execute("create view v as select * from t") + conn.close() + ds = Datasette([path]) + await ds.invoke_startup() + await ds.refresh_schemas() + internal_db = ds.get_internal_database() + + write_fns = [] + original_execute_write_fn = internal_db.execute_write_fn + + async def counting_execute_write_fn(fn, *args, **kwargs): + write_fns.append(fn) + return await original_execute_write_fn(fn, *args, **kwargs) + + internal_db.execute_write_fn = counting_execute_write_fn + try: + await populate_schema_tables(internal_db, ds.get_database("data")) + finally: + internal_db.execute_write_fn = original_execute_write_fn + + assert len(write_fns) == 1 + # And the catalog should be correct + tables = await internal_db.execute( + "select table_name from catalog_tables where database_name = 'data'" + ) + assert [row["table_name"] for row in tables.rows] == ["t"] + views = await internal_db.execute( + "select view_name from catalog_views where database_name = 'data'" + ) + assert [row["view_name"] for row in views.rows] == ["v"] + columns = await internal_db.execute( + "select name from catalog_columns where database_name = 'data' and table_name = 't'" + ) + assert {row["name"] for row in columns.rows} == {"id", "name"} From 5cec9c9faa523b116d1bfdc7d58d4031da4cceec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 06:03:42 +0000 Subject: [PATCH 05/17] Stop running sqlite-utils plugins on Datasette connections Wrapping a connection in sqlite_utils.Database() runs sqlite-utils plugins' prepare_connection hooks against it by default. Datasette's write API views and introspection helpers now pass execute_plugins=False (matching what utils/internal_db.py already did), so third-party sqlite-utils plugins no longer touch Datasette's connections. Also apply PRAGMA recursive_triggers=on in Datasette._prepare_connection so every connection gets consistent trigger semantics - previously only the write connection got it, as a side effect of the first sqlite-utils based write. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/app.py | 4 +++ datasette/database.py | 2 +- datasette/views/row.py | 6 +++-- datasette/views/table.py | 4 +-- datasette/views/table_create_alter.py | 10 +++++--- docs/changelog.rst | 1 + tests/test_api_write.py | 35 +++++++++++++++++++++++++++ tests/test_internals_database.py | 21 ++++++++++++++++ 8 files changed, 75 insertions(+), 8 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 4ba5d20f..987c77df 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -1565,6 +1565,10 @@ class Datasette: conn.execute("SELECT load_extension(?)", [extension]) if self.setting("cache_size_kb"): conn.execute(f"PRAGMA cache_size=-{self.setting('cache_size_kb')}") + # Consistent trigger semantics on every connection - sqlite-utils + # would otherwise enable this on just the write connection, as a + # side effect of the first sqlite-utils based write + conn.execute("PRAGMA recursive_triggers=on") # pylint: disable=no-member if database != INTERNAL_DB_NAME: pm.hook.prepare_connection(conn=conn, database=database, datasette=self) diff --git a/datasette/database.py b/datasette/database.py index 82522e6a..6c1b3a9e 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -757,7 +757,7 @@ class Database: def column_details(conn): # Returns {column_name: (type, is_unique)} - db = sqlite_utils.Database(conn) + db = sqlite_utils.Database(conn, execute_plugins=False) columns = db[table].columns_dict indexes = db[table].indexes details = {} diff --git a/datasette/views/row.py b/datasette/views/row.py index c90a3bbe..feb67eb5 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -749,7 +749,9 @@ class RowDeleteView(BaseView): # Delete table def delete_row(conn): - sqlite_utils.Database(conn)[resolved.table].delete(resolved.pk_values) + sqlite_utils.Database(conn, execute_plugins=False)[resolved.table].delete( + resolved.pk_values + ) try: await resolved.db.execute_write_fn(delete_row, request=request) @@ -830,7 +832,7 @@ class RowUpdateView(BaseView): return Response.error(["Permission denied for alter-table"], 403) def update_row(conn): - sqlite_utils.Database(conn)[resolved.table].update( + sqlite_utils.Database(conn, execute_plugins=False)[resolved.table].update( resolved.pk_values, update, alter=alter ) diff --git a/datasette/views/table.py b/datasette/views/table.py index f7edd744..35afd223 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -1143,7 +1143,7 @@ class TableInsertView(BaseView): row_pk_values_for_later = [tuple(row[pk] for pk in pks) for row in rows] def insert_or_upsert_rows(conn): - table = sqlite_utils.Database(conn)[table_name] + table = sqlite_utils.Database(conn, execute_plugins=False)[table_name] kwargs = {} if upsert: kwargs = { @@ -1407,7 +1407,7 @@ class TableDropView(BaseView): # Drop table def drop_table(conn): - sqlite_utils.Database(conn)[table_name].drop() + sqlite_utils.Database(conn, execute_plugins=False)[table_name].drop() await db.execute_write_fn(drop_table, request=request) await self.ds.track_event( diff --git a/datasette/views/table_create_alter.py b/datasette/views/table_create_alter.py index f3e1779f..35f7ab86 100644 --- a/datasette/views/table_create_alter.py +++ b/datasette/views/table_create_alter.py @@ -889,11 +889,13 @@ class TableCreateView(BaseView): initial_schema = None if table_exists: initial_schema = await db.execute_fn( - lambda conn: sqlite_utils.Database(conn)[table_name].schema + lambda conn: sqlite_utils.Database(conn, execute_plugins=False)[ + table_name + ].schema ) def create_table(conn): - db_for_write = sqlite_utils.Database(conn) + db_for_write = sqlite_utils.Database(conn, execute_plugins=False) table = db_for_write[table_name] if rows: table.insert_all( @@ -1187,7 +1189,9 @@ class TableAlterView(BaseView): before_schema = _table_schema_from_conn(conn, table_name) def apply_operations(operation_conn): - db_for_write = sqlite_utils.Database(operation_conn) + db_for_write = sqlite_utils.Database( + operation_conn, execute_plugins=False + ) table = db_for_write[table_name] current_table_name = table_name diff --git a/docs/changelog.rst b/docs/changelog.rst index 8cb72005..caa1050f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -14,6 +14,7 @@ Unreleased - ``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`) - Rebuilding the internal database catalog for a database is now a single atomic write. Previously the rebuild used six separate transactions, so queries against the internal database could observe a database with missing catalog rows while a rebuild was in progress. (:issue:`2831`) +- sqlite-utils plugins no longer have their ``prepare_connection()`` hooks executed against Datasette's database connections - use Datasette's own :ref:`prepare_connection() ` plugin hook to customize connections. ``PRAGMA recursive_triggers=on`` is now applied consistently to every connection Datasette opens - previously it was enabled just on the write connection, as a side effect of the first sqlite-utils based write. (:issue:`2831`) .. _v1_0_a36: diff --git a/tests/test_api_write.py b/tests/test_api_write.py index 691f7ac9..4218712e 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -2782,3 +2782,38 @@ async def test_insert_with_return_failing_row_is_atomic(ds_write): await ds_write.get_database("data").execute("select count(*) from docs") ).single_value() assert count == 0 + + +@pytest.mark.asyncio +async def test_write_api_does_not_run_sqlite_utils_plugins(ds_write): + # https://github.com/simonw/datasette/issues/2831 + # sqlite-utils plugins should not have their prepare_connection hooks + # executed against Datasette's connections + import sqlite_utils.plugins + from sqlite_utils import hookimpl + + prepared = [] + + class TrackingPlugin: + @hookimpl + def prepare_connection(self, conn): + prepared.append(conn) + + sqlite_utils.plugins.pm.register(TrackingPlugin(), name="datasette-test-tracking") + try: + token = write_token(ds_write) + response = await ds_write.client.post( + "/data/docs/-/insert", + json={"rows": [{"id": 1, "title": "one"}]}, + headers=_headers(token), + ) + assert response.status_code == 201 + response = await ds_write.client.post( + "/data/docs/1/-/update", + json={"update": {"title": "two"}}, + headers=_headers(token), + ) + assert response.status_code == 200 + assert prepared == [] + finally: + sqlite_utils.plugins.pm.unregister(name="datasette-test-tracking") diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 74818df9..fb1adcba 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -1302,3 +1302,24 @@ async def test_database_close_is_idempotent(tmpdir): # Second call should be a no-op, not raise db.close() ds._internal_database.close() + + +@pytest.mark.asyncio +async def test_recursive_triggers_enabled_on_all_connections(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + # Previously recursive_triggers was only enabled on the write connection, + # and only as a side effect of the first sqlite-utils based write - so + # trigger semantics could differ between connections + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path]) + db = datasette.get_database("test") + write_value = await db.execute_write_fn( + lambda conn: conn.execute("PRAGMA recursive_triggers").fetchone()[0], + transaction=False, + ) + read_value = await db.execute_fn( + lambda conn: conn.execute("PRAGMA recursive_triggers").fetchone()[0] + ) + assert write_value == 1 + assert read_value == 1 From 14815cb092c96d30411a215cd5256518b7ddd1bb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 06:06:57 +0000 Subject: [PATCH 06/17] Add busy_timeout_ms setting The SQLite busy timeout was previously an implicit policy - every connection inherited the sqlite3 driver's silent 5 second default. It is now an explicit, documented setting passed as timeout= to every sqlite3.connect() call. The default remains 5000ms. This matters for deployments where external processes write to the same database files Datasette is serving. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/app.py | 5 +++++ datasette/database.py | 4 ++++ docs/changelog.rst | 1 + docs/cli-reference.rst | 2 ++ docs/settings.rst | 11 +++++++++++ tests/test_internals_database.py | 33 ++++++++++++++++++++++++++++++++ 6 files changed, 56 insertions(+) diff --git a/datasette/app.py b/datasette/app.py index 987c77df..53c41282 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -220,6 +220,11 @@ SETTINGS = ( "Number of threads in the thread pool for executing SQLite queries", ), Setting("sql_time_limit_ms", 1000, "Time limit for a SQL query in milliseconds"), + Setting( + "busy_timeout_ms", + 5000, + "How long SQLite waits for a locked database file before giving up", + ), Setting( "default_facet_size", 30, "Number of values to return for requested facets" ), diff --git a/datasette/database.py b/datasette/database.py index 6c1b3a9e..877c373b 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -210,6 +210,10 @@ class Database: extra_kwargs = {} if write: extra_kwargs["isolation_level"] = "IMMEDIATE" + # An explicit busy timeout policy rather than the driver's silent + # 5 second default - matters when external processes write to the + # same database files + extra_kwargs["timeout"] = self.ds.setting("busy_timeout_ms") / 1000 if self.memory_name: uri = "file:{}?mode=memory&cache=shared".format(self.memory_name) conn = sqlite3.connect( diff --git a/docs/changelog.rst b/docs/changelog.rst index caa1050f..0ab86365 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -15,6 +15,7 @@ Unreleased - 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`) - Rebuilding the internal database catalog for a database is now a single atomic write. Previously the rebuild used six separate transactions, so queries against the internal database could observe a database with missing catalog rows while a rebuild was in progress. (:issue:`2831`) - sqlite-utils plugins no longer have their ``prepare_connection()`` hooks executed against Datasette's database connections - use Datasette's own :ref:`prepare_connection() ` plugin hook to customize connections. ``PRAGMA recursive_triggers=on`` is now applied consistently to every connection Datasette opens - previously it was enabled just on the write connection, as a side effect of the first sqlite-utils based write. (:issue:`2831`) +- New :ref:`setting_busy_timeout_ms` setting controlling how long SQLite waits for a locked database file before giving up, previously hard-wired to the ``sqlite3`` driver's silent 5 second default. This matters when external processes write to the same database files Datasette is serving. (:issue:`2831`) .. _v1_0_a36: diff --git a/docs/cli-reference.rst b/docs/cli-reference.rst index 2302f742..c495a865 100644 --- a/docs/cli-reference.rst +++ b/docs/cli-reference.rst @@ -251,6 +251,8 @@ These can be passed to ``datasette serve`` using ``datasette serve --setting nam executing SQLite queries (default=3) sql_time_limit_ms Time limit for a SQL query in milliseconds (default=1000) + busy_timeout_ms How long SQLite waits for a locked database file + before giving up (default=5000) default_facet_size Number of values to return for requested facets (default=30) facet_time_limit_ms Time limit for calculating a requested facet diff --git a/docs/settings.rst b/docs/settings.rst index 9c114e4a..d7b78798 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -103,6 +103,17 @@ You can optionally set a lower time limit for an individual query using the ``?_ This would set the time limit to 100ms for that specific query. This feature is useful if you are working with databases of unknown size and complexity - a query that might make perfect sense for a smaller table could take too long to execute on a table with millions of rows. By setting custom time limits you can execute queries "optimistically" - e.g. give me an exact count of rows matching this query but only if it takes less than 100ms to calculate. +.. _setting_busy_timeout_ms: + +busy_timeout_ms +~~~~~~~~~~~~~~~ + +How long SQLite should wait when a database file is locked by another connection or process before giving up with a ``database is locked`` error, in milliseconds. The default is 5000 (five seconds), matching the default used by Python's ``sqlite3`` module. + +This mostly matters when other processes write to the same database files that Datasette is serving - a common pattern is a separate script (using `sqlite-utils `__ or similar) that periodically updates a database while Datasette serves it. A larger value makes Datasette more patient with long write transactions from those processes:: + + datasette mydatabase.db --setting busy_timeout_ms 10000 + .. _setting_max_returned_rows: max_returned_rows diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index fb1adcba..53f5e453 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -1323,3 +1323,36 @@ async def test_recursive_triggers_enabled_on_all_connections(tmp_path): ) assert write_value == 1 assert read_value == 1 + + +@pytest.mark.asyncio +async def test_busy_timeout_ms_setting(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + # The SQLite busy timeout should be an explicit, configurable policy + # instead of the sqlite3 driver's inherited 5 second default + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path], settings={"busy_timeout_ms": 250}) + db = datasette.get_database("test") + read_value = await db.execute_fn( + lambda conn: conn.execute("PRAGMA busy_timeout").fetchone()[0] + ) + write_value = await db.execute_write_fn( + lambda conn: conn.execute("PRAGMA busy_timeout").fetchone()[0], + transaction=False, + ) + assert read_value == 250 + assert write_value == 250 + + +@pytest.mark.asyncio +async def test_busy_timeout_ms_default(tmp_path): + # Default matches the sqlite3 driver's historical 5 second default + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path]) + db = datasette.get_database("test") + read_value = await db.execute_fn( + lambda conn: conn.execute("PRAGMA busy_timeout").fetchone()[0] + ) + assert read_value == 5000 From 3713d6c0d59dc03685a0af5318b9f826dbbeb7d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 06:10:15 +0000 Subject: [PATCH 07/17] Add opt-in journal_mode setting, enable WAL on persistent internal DB New journal_mode setting lets deployments opt mutable database files into WAL mode (or delete/truncate/persist), applied on the write connection. WAL is paired with PRAGMA synchronous=NORMAL. Datasette does not change the journal mode of database files by default. Also fixes an inconsistency: a persistent internal database passed via --internal now gets WAL enabled, matching the temporary internal database default (which was moved to a temp disk file specifically so it could use WAL). Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- datasette/app.py | 11 ++++++- datasette/database.py | 24 ++++++++++++--- docs/changelog.rst | 2 ++ docs/cli-reference.rst | 3 ++ docs/settings.rst | 22 ++++++++++++++ tests/test_internals_database.py | 50 ++++++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 5 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 53c41282..7ab1aa66 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -225,6 +225,11 @@ SETTINGS = ( 5000, "How long SQLite waits for a locked database file before giving up", ), + Setting( + "journal_mode", + "", + "Journal mode to set on mutable database files, e.g. wal - leave blank to use each file's existing mode", + ), Setting( "default_facet_size", 30, "Number of values to return for requested facets" ), @@ -483,7 +488,11 @@ class Datasette: if internal is None: self._internal_database = Database(self, is_temp_disk=True) else: - self._internal_database = Database(self, path=internal, mode="rwc") + # WAL for the same reason as the temporary internal database: + # the catalog can be read while it is being rewritten + self._internal_database = Database( + self, path=internal, mode="rwc", enable_wal=True + ) self._internal_database.name = INTERNAL_DB_NAME self.cache_headers = cache_headers diff --git a/datasette/database.py b/datasette/database.py index 877c373b..8963d210 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -36,6 +36,8 @@ EXECUTE_WRITE_RETURNING_LIMIT = 10 AttachedDatabase = namedtuple("AttachedDatabase", ("seq", "name", "file")) +ALLOWED_JOURNAL_MODES = {"delete", "truncate", "persist", "wal"} + class DatasetteClosedError(RuntimeError): """Raised when using a Datasette or Database instance after close().""" @@ -129,6 +131,7 @@ class Database: memory_name=None, mode=None, is_temp_disk=False, + enable_wal=False, ): self.name = None self._thread_local_id = f"x{self._thread_local_id_counter}" @@ -140,6 +143,8 @@ class Database: self.is_memory = is_memory self.memory_name = memory_name self.is_temp_disk = is_temp_disk + self.enable_wal = enable_wal or is_temp_disk + self._wal_enabled = False if memory_name is not None: self.is_memory = True if is_temp_disk: @@ -148,10 +153,7 @@ class Database: self.path = temp_path self.is_mutable = True self.mode = "rwc" - self._wal_enabled = False atexit.register(self._cleanup_temp_file) - else: - self._wal_enabled = False self.cached_hash = None self.cached_size = None self._cached_table_counts = None @@ -241,9 +243,23 @@ class Database: f"file:{self.path}{qs}", uri=True, check_same_thread=False, **extra_kwargs ) self._all_file_connections.append(conn) - if self.is_temp_disk and not self._wal_enabled: + if self.enable_wal and not self._wal_enabled: conn.execute("PRAGMA journal_mode=WAL") self._wal_enabled = True + if write and self.is_mutable and not self.enable_wal: + journal_mode = self.ds.setting("journal_mode") + if journal_mode: + if journal_mode not in ALLOWED_JOURNAL_MODES: + raise ValueError( + "journal_mode setting must be one of: {}".format( + ", ".join(sorted(ALLOWED_JOURNAL_MODES)) + ) + ) + conn.execute("PRAGMA journal_mode={}".format(journal_mode)) + if journal_mode == "wal": + # The standard WAL pairing - fewer fsyncs, application + # level consistency guarantees are unchanged + conn.execute("PRAGMA synchronous=NORMAL") return conn def close(self): diff --git a/docs/changelog.rst b/docs/changelog.rst index 0ab86365..515c4610 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,6 +16,8 @@ Unreleased - Rebuilding the internal database catalog for a database is now a single atomic write. Previously the rebuild used six separate transactions, so queries against the internal database could observe a database with missing catalog rows while a rebuild was in progress. (:issue:`2831`) - sqlite-utils plugins no longer have their ``prepare_connection()`` hooks executed against Datasette's database connections - use Datasette's own :ref:`prepare_connection() ` plugin hook to customize connections. ``PRAGMA recursive_triggers=on`` is now applied consistently to every connection Datasette opens - previously it was enabled just on the write connection, as a side effect of the first sqlite-utils based write. (:issue:`2831`) - New :ref:`setting_busy_timeout_ms` setting controlling how long SQLite waits for a locked database file before giving up, previously hard-wired to the ``sqlite3`` driver's silent 5 second default. This matters when external processes write to the same database files Datasette is serving. (:issue:`2831`) +- New :ref:`setting_journal_mode` setting for opting mutable database files into SQLite's `WAL mode `__ (or another journal mode), allowing reads and writes to proceed concurrently. Datasette does not change the journal mode of database files by default. (:issue:`2831`) +- A persistent internal database specified with ``--internal`` now has WAL mode enabled, matching the behavior of the default temporary internal database. (:issue:`2831`) .. _v1_0_a36: diff --git a/docs/cli-reference.rst b/docs/cli-reference.rst index c495a865..abf94f49 100644 --- a/docs/cli-reference.rst +++ b/docs/cli-reference.rst @@ -253,6 +253,9 @@ These can be passed to ``datasette serve`` using ``datasette serve --setting nam (default=1000) busy_timeout_ms How long SQLite waits for a locked database file before giving up (default=5000) + journal_mode Journal mode to set on mutable database files, + e.g. wal - leave blank to use each file's + existing mode (default=) default_facet_size Number of values to return for requested facets (default=30) facet_time_limit_ms Time limit for calculating a requested facet diff --git a/docs/settings.rst b/docs/settings.rst index d7b78798..bdbd1834 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -114,6 +114,28 @@ This mostly matters when other processes write to the same database files that D datasette mydatabase.db --setting busy_timeout_ms 10000 +.. _setting_journal_mode: + +journal_mode +~~~~~~~~~~~~ + +`Journal mode `__ to set on mutable database files. Leave blank (the default) to use whatever journal mode each database file already uses. + +Setting this to ``wal`` enables SQLite's `Write-Ahead Logging `__ mode, which allows reads and writes to proceed concurrently - in the default rollback journal mode each commit blocks readers, and long reads block the writer:: + + datasette mydatabase.db --setting journal_mode wal + +When ``wal`` is selected Datasette also sets ``PRAGMA synchronous=NORMAL`` on the write connection, the standard pairing for WAL which reduces the number of ``fsync`` operations without weakening application-level consistency guarantees. + +Other allowed values are ``delete``, ``truncate`` and ``persist``. + +Things to be aware of before enabling WAL: + +- WAL mode is persistent - it is recorded in the database file and stays in effect when other tools open the same file later. +- SQLite creates ``-wal`` and ``-shm`` files alongside the database file. +- WAL does not work reliably on network filesystems such as NFS. +- The directory containing the database must be writable by Datasette. + .. _setting_max_returned_rows: max_returned_rows diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 53f5e453..2e928e44 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -1356,3 +1356,53 @@ async def test_busy_timeout_ms_default(tmp_path): lambda conn: conn.execute("PRAGMA busy_timeout").fetchone()[0] ) assert read_value == 5000 + + +@pytest.mark.asyncio +async def test_journal_mode_setting_applies_wal(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + # Opt-in WAL support for mutable database files - not the default + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path], settings={"journal_mode": "wal"}) + db = datasette.get_database("test") + mode = await db.execute_write_fn( + lambda conn: conn.execute("PRAGMA journal_mode").fetchone()[0], + transaction=False, + ) + assert mode == "wal" + # WAL is paired with synchronous=NORMAL (1) on the write connection + synchronous = await db.execute_write_fn( + lambda conn: conn.execute("PRAGMA synchronous").fetchone()[0], + transaction=False, + ) + assert synchronous == 1 + + +@pytest.mark.asyncio +async def test_journal_mode_defaults_to_leaving_files_alone(tmp_path): + path = str(tmp_path / "test.db") + sqlite3.connect(path).close() + datasette = Datasette([path]) + db = datasette.get_database("test") + mode = await db.execute_write_fn( + lambda conn: conn.execute("PRAGMA journal_mode").fetchone()[0], + transaction=False, + ) + assert mode == "delete" + + +@pytest.mark.asyncio +async def test_persistent_internal_database_gets_wal(tmp_path): + # https://github.com/simonw/datasette/issues/2831 + # The temporary internal database enables WAL - a persistent one passed + # via --internal should get the same treatment + internal_path = str(tmp_path / "internal.db") + datasette = Datasette(memory=True, internal=internal_path) + await datasette.invoke_startup() + internal_db = datasette.get_internal_database() + mode = await internal_db.execute_write_fn( + lambda conn: conn.execute("PRAGMA journal_mode").fetchone()[0], + transaction=False, + ) + assert mode == "wal" From f6ded9af7527ce323d162121cc75682ca19781b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 06:11:39 +0000 Subject: [PATCH 08/17] Document how Datasette manages transactions Adds an internals documentation section describing the task-equals- transaction model for write connections, the autocommit read path, and the cross-project constraint that Datasette and sqlite-utils both rely on the legacy sqlite3 transaction handling - Python 3.12+ autocommit= style connections are not supported and any migration would need to be coordinated across both projects. Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- docs/internals.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/internals.rst b/docs/internals.rst index 9a5f3f50..1f1873e6 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2180,6 +2180,17 @@ Functions run using ``execute_isolated_fn()`` share the same queue as ``execute_ The return value of the function will be returned by this method. Any exceptions raised by the function will be raised out of the ``await`` line as well. +.. _internals_database_transactions: + +How Datasette manages transactions +---------------------------------- + +Each mutable database has a single write connection, owned by a dedicated write thread that executes queued write tasks one at a time. Each task submitted with ``transaction=True`` (the default) runs inside its own transaction: Datasette executes ``BEGIN IMMEDIATE`` before invoking the task, commits if it completes and rolls back if it raises. One write task equals one transaction, and writes made by a task only become visible to readers when the task completes. + +Read connections never open transactions - each read statement executes in autocommit mode with its own implicit SQLite read snapshot. + +Datasette's write connections rely on the legacy transaction handling of Python's ``sqlite3`` module (connections opened with ``isolation_level=`` rather than the ``autocommit=`` parameter introduced in Python 3.12). This is a deliberate constraint shared with `sqlite-utils `__, which refuses connections created with ``autocommit=True`` or ``autocommit=False`` - any future migration to the modern transaction API would need to be coordinated across both projects. Plugins should not change the transaction control mode of connections Datasette passes to them. + .. _database_close: db.close() From ca995df664e8ae5681f51f87c36e2adaf6e36b52 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 06:18:42 +0000 Subject: [PATCH 09/17] Add busy_timeout_ms and journal_mode to expected /-/settings.json Refs #2831 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01N76afGMhBRQk528VF1LTpR --- tests/test_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_api.py b/tests/test_api.py index 5ed14283..afc21341 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -694,6 +694,8 @@ async def test_settings_json(ds_client): "max_insert_rows": 100, "max_post_body_bytes": 2 * 1024 * 1024, "sql_time_limit_ms": 200, + "busy_timeout_ms": 5000, + "journal_mode": "", "allow_download": True, "allow_signed_tokens": True, "max_signed_tokens_ttl": 0, From ccace40e5a14ec51ae8ace8c65d27ac6bada4fac Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 13 Jul 2026 21:19:04 -0700 Subject: [PATCH 10/17] /-/plugins.json is now an array of objects again (#2843) Reverts the object envelope introduced in 1.0a36 for this endpoint - it once again returns a top-level JSON array of plugin objects. Closes #2842 Claude-Session: https://claude.ai/code/session_012TYc1NTBK4zEjabB3u2zqu Co-authored-by: Claude --- datasette/app.py | 2 +- docs/introspection.rst | 21 +++++++++------------ tests/test_api.py | 4 ++-- tests/test_config_dir.py | 2 +- tests/test_plugins.py | 2 +- tests/test_success_envelope.py | 15 +++++++-------- 6 files changed, 21 insertions(+), 25 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 4ba5d20f..c44f9095 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -2575,7 +2575,7 @@ class Datasette: JsonDataView.as_view( self, "plugins.json", - lambda request: {"plugins": self._plugins(request)}, + self._plugins, needs_request=True, ), r"/-/plugins(\.(?Pjson))?$", diff --git a/docs/introspection.rst b/docs/introspection.rst index b78e4860..14b6249f 100644 --- a/docs/introspection.rst +++ b/docs/introspection.rst @@ -80,18 +80,15 @@ Shows a list of currently installed plugins and their versions. `Plugins example .. code-block:: json - { - "ok": true, - "plugins": [ - { - "name": "datasette_cluster_map", - "static": true, - "templates": false, - "version": "0.10", - "hooks": ["extra_css_urls", "extra_js_urls", "extra_body_script"] - } - ] - } + [ + { + "name": "datasette_cluster_map", + "static": true, + "templates": false, + "version": "0.10", + "hooks": ["extra_css_urls", "extra_js_urls", "extra_body_script"] + } + ] Add ``?all=1`` to include details of the default plugins baked into Datasette. diff --git a/tests/test_api.py b/tests/test_api.py index 5ed14283..d5f519b9 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -614,13 +614,13 @@ async def test_plugins_json(ds_client): response = await ds_client.get("/-/plugins.json") # Filter out TrackEventPlugin actual_plugins = sorted( - [p for p in response.json()["plugins"] if p["name"] != "TrackEventPlugin"], + [p for p in response.json() if p["name"] != "TrackEventPlugin"], key=lambda p: p["name"], ) assert EXPECTED_PLUGINS == actual_plugins # Try with ?all=1 response = await ds_client.get("/-/plugins.json?all=1") - names = {p["name"] for p in response.json()["plugins"]} + names = {p["name"] for p in response.json()} assert names.issuperset(p["name"] for p in EXPECTED_PLUGINS) assert names.issuperset(DEFAULT_PLUGINS) diff --git a/tests/test_config_dir.py b/tests/test_config_dir.py index 42c6ae60..636b17eb 100644 --- a/tests/test_config_dir.py +++ b/tests/test_config_dir.py @@ -109,7 +109,7 @@ def test_settings(config_dir_client): def test_plugins(config_dir_client): response = config_dir_client.get("/-/plugins.json") assert 200 == response.status - plugins = response.json["plugins"] + plugins = response.json assert "hooray.py" in {p["name"] for p in plugins} assert "non_py_file.txt" not in {p["name"] for p in plugins} assert "mypy_cache" not in {p["name"] for p in plugins} diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 5c4034db..59b1c0bf 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -1482,7 +1482,7 @@ async def test_plugin_is_installed(): datasette.pm.register(DummyPlugin(), name="DummyPlugin") response = await datasette.client.get("/-/plugins.json") assert response.status_code == 200 - installed_plugins = {p["name"] for p in response.json()["plugins"]} + installed_plugins = {p["name"] for p in response.json()} assert "DummyPlugin" in installed_plugins finally: diff --git a/tests/test_success_envelope.py b/tests/test_success_envelope.py index 3c413d73..46a78ef0 100644 --- a/tests/test_success_envelope.py +++ b/tests/test_success_envelope.py @@ -2,8 +2,8 @@ Tests for the canonical JSON success envelope. Every JSON object returned by a Datasette endpoint on success should include -"ok": true. (Endpoints that return a top-level array are being converted to -objects separately - see /-/plugins, /-/databases, /-/actions.) +"ok": true. /-/plugins intentionally returns a top-level array instead, while +/-/databases and /-/actions use the object envelope. """ import pytest @@ -79,17 +79,16 @@ async def test_permissions_post_success_has_ok_true(ds_envelope): @pytest.mark.asyncio -async def test_plugins_json_is_object(ds_client): +async def test_plugins_json_is_array(ds_client): response = await ds_client.get("/-/plugins.json") assert response.status_code == 200 data = response.json() - assert set(data.keys()) == {"ok", "plugins"} - assert data["ok"] is True - assert isinstance(data["plugins"], list) + assert isinstance(data, list) + assert all(isinstance(plugin, dict) for plugin in data) # ?all=1 should include Datasette's default plugins in the same shape response_all = await ds_client.get("/-/plugins.json?all=1") - all_plugins = response_all.json()["plugins"] - assert len(all_plugins) > len(data["plugins"]) + all_plugins = response_all.json() + assert len(all_plugins) > len(data) @pytest.mark.asyncio From 10088dfa1dd7ab0075f97e380121dff4f6a5222c Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 13 Jul 2026 22:42:44 -0700 Subject: [PATCH 11/17] execute_write(transaction=False) parameter, plus fix for errors inside tasks Ensure a write inside a failing Datasette task never becomes visible. Refs #2831 --- datasette/database.py | 7 ++++++- datasette/views/database.py | 9 +++++++- docs/internals.rst | 8 ++++--- tests/test_internals_database.py | 36 ++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/datasette/database.py b/datasette/database.py index e7fe1ed9..eb402b0c 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -246,6 +246,7 @@ class Database: request=None, return_all=False, returning_limit=EXECUTE_WRITE_RETURNING_LIMIT, + transaction=True, ): self._check_not_closed() if returning_limit < 0: @@ -258,7 +259,9 @@ class Database: ) with trace("sql", database=self.name, sql=sql.strip(), params=params): - results = await self.execute_write_fn(_inner, block=block, request=request) + results = await self.execute_write_fn( + _inner, block=block, request=request, transaction=transaction + ) return results async def execute_write_script(self, sql, block=True, request=None): @@ -348,6 +351,7 @@ class Database: self.ds._prepare_connection(self._write_connection, self.name) if transaction: with self._write_connection: + self._write_connection.execute("BEGIN IMMEDIATE") result = fn(self._write_connection) else: result = fn(self._write_connection) @@ -477,6 +481,7 @@ class Database: try: if task.transaction: with conn: + conn.execute("BEGIN IMMEDIATE") result = task.fn(conn) else: result = task.fn(conn) diff --git a/datasette/views/database.py b/datasette/views/database.py index 10dc66ae..11646f45 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -643,8 +643,15 @@ class QueryView(View): ok = None redirect_url = None try: + execute_write_kwargs = {"request": request} + if stored_query.is_trusted: + analysis = await db.analyze_sql(stored_query.sql, params_for_query) + if any( + operation.operation == "vacuum" for operation in analysis.operations + ): + execute_write_kwargs["transaction"] = False cursor = await db.execute_write( - stored_query.sql, params_for_query, request=request + stored_query.sql, params_for_query, **execute_write_kwargs ) # success message can come from on_success_message or on_success_message_sql message = None diff --git a/docs/internals.rst b/docs/internals.rst index 2048c7e4..d2bd46ef 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2023,8 +2023,8 @@ Example usage: .. _database_execute_write: -await db.execute_write(sql, params=None, block=True, request=None, return_all=False, returning_limit=10) --------------------------------------------------------------------------------------------------------- +await db.execute_write(sql, params=None, block=True, request=None, return_all=False, returning_limit=10, transaction=True) +-------------------------------------------------------------------------------------------------------------------------- SQLite only allows one database connection to write at a time. Datasette handles this for you by maintaining a queue of writes to be executed against a given database. Plugins can submit write operations to this queue and they will be executed in the order in which they are received. @@ -2059,7 +2059,9 @@ If you need to retrieve every row returned by a statement, pass ``return_all=Tru If you pass ``block=False`` this behavior changes to "fire and forget" - queries will be added to the write queue and executed in a separate thread while your code can continue to do other things. The method will return a UUID representing the queued task. -Each call to ``execute_write()`` will be executed inside a transaction. +Each call to ``execute_write()`` will be executed inside a transaction. Pass +``transaction=False`` for statements such as ``VACUUM`` that cannot run inside +a transaction. .. _database_execute_write_script: diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index bad4e8ca..b1a212d9 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -11,6 +11,7 @@ from datasette.database import _deliver_write_result from datasette.utils.sqlite import sqlite3, supports_returning from datasette.utils import Column import pytest +import sqlite_utils import time import uuid @@ -718,6 +719,41 @@ async def test_execute_write_fn_exception(db): await db.execute_write_fn(write_fn) +@pytest.mark.asyncio +@pytest.mark.parametrize("num_sql_threads", (0, 1)) +async def test_execute_write_fn_sqlite_utils_transaction(tmp_path, num_sql_threads): + # A write inside a failing Datasette task must never become visible or + # survive the rollback. Exercise both the synchronous and writer-thread + # paths against a file-backed database so a second connection can observe + # committed state independently. + db_path = tmp_path / "test.db" + sqlite3.connect(db_path).close() + ds = Datasette([str(db_path)], settings={"num_sql_threads": num_sql_threads}) + db = ds.get_database("test") + await db.execute_write("create table items (id integer primary key)") + # This reader is used inside the write callback, which may run on another + # thread, but it is never accessed concurrently. + reader = sqlite3.connect(db_path, check_same_thread=False) + + def insert_then_fail(conn): + # Datasette must open the outer transaction before sqlite-utils writes. + assert conn.in_transaction + sqlite_utils.Database(conn)["items"].insert({"id": 1}) + # If sqlite-utils committed its own transaction, this would return 1. + assert reader.execute("select count(*) from items").fetchone()[0] == 0 + # Simulate a later step failing after the sqlite-utils write succeeded. + raise ValueError("deliberate") + + try: + with pytest.raises(ValueError, match="deliberate"): + await db.execute_write_fn(insert_then_fail) + # The outer transaction must roll back the sqlite-utils write as well. + assert reader.execute("select count(*) from items").fetchone()[0] == 0 + finally: + reader.close() + db.close() + + @pytest.mark.asyncio @pytest.mark.parametrize("param_name", ["conn", "connection", "db", "c"]) async def test_execute_write_fn_accepts_any_single_param_name(db, param_name): From 7f0a8b38aee771828fe7b3c7e54850249016d753 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 14 Jul 2026 08:40:07 -0700 Subject: [PATCH 12/17] Better permission debug tools and documentation Closes #2841 --- datasette/default_permissions/config.py | 4 + .../templates/_permission_ui_styles.html | 66 +++- .../templates/_permissions_debug_tabs.html | 8 +- datasette/templates/allow_debug.html | 56 ++- datasette/templates/debug_allowed.html | 2 +- datasette/templates/debug_check.html | 344 ++++++++++-------- .../debug_permissions_playground.html | 73 ++-- datasette/templates/debug_rules.html | 2 +- datasette/utils/actions_sql.py | 236 ++++++++++++ datasette/views/special.py | 42 ++- docs/authentication.rst | 89 ++++- tests/test_permissions.py | 228 +++++++++++- 12 files changed, 890 insertions(+), 260 deletions(-) diff --git a/datasette/default_permissions/config.py b/datasette/default_permissions/config.py index aab87c1c..8edc976e 100644 --- a/datasette/default_permissions/config.py +++ b/datasette/default_permissions/config.py @@ -96,6 +96,10 @@ class ConfigPermissionProcessor: """Evaluate an allow block against the current actor.""" if allow_block is None: return None + # Values passed using ``-s permissions.* 1`` or ``0`` are parsed as + # integers, but should retain the CLI's boolean 1/0 behavior. + if isinstance(allow_block, int) and allow_block in (0, 1): + return bool(allow_block) return actor_matches_allow(self.actor, allow_block) def is_in_restriction_allowlist( diff --git a/datasette/templates/_permission_ui_styles.html b/datasette/templates/_permission_ui_styles.html index 53a824f1..21a2ea8f 100644 --- a/datasette/templates/_permission_ui_styles.html +++ b/datasette/templates/_permission_ui_styles.html @@ -6,8 +6,20 @@ padding: 1.5em; margin-bottom: 2em; } +.permission-form form { + max-width: 60rem; +} +.permission-form-grid { + display: grid; + gap: 1.5rem; + grid-template-columns: repeat(2, minmax(0, 1fr)); +} +.permission-form-result { + margin-top: 1rem; + max-width: 60rem; +} .form-section { - margin-bottom: 1em; + margin-bottom: 1.25em; } .form-section label { display: block; @@ -15,22 +27,51 @@ font-weight: bold; } .form-section input[type="text"], -.form-section select { - width: 100%; - max-width: 500px; - padding: 0.5em; +.form-section input[type="number"], +.form-section select, +.permission-textarea { + background-color: #fff; + border: 1px solid #aaa; + border-radius: 4px; box-sizing: border-box; - border: 1px solid #ccc; - border-radius: 3px; + box-shadow: inset 0 1px 2px rgba(0, 0, 0, 0.08); + color: #222; + font-family: inherit; + font-size: 1rem; + line-height: 1.4; + max-width: none; + width: 100%; +} +.form-section input[type="text"] { + height: 3rem; + padding: 0.6rem 0.75rem; +} +.form-section input[type="number"] { + height: 3rem; + max-width: 7rem; + padding: 0.6rem 0.75rem; +} +.form-section select { + height: 3rem; + padding: 0.6rem 0.75rem; +} +.permission-textarea { + font-family: monospace; + min-height: 12rem; + padding: 0.75rem; + resize: vertical; } .form-section input[type="text"]:focus, -.form-section select:focus { - outline: 2px solid #0066cc; +.form-section input[type="number"]:focus, +.form-section select:focus, +.permission-textarea:focus { border-color: #0066cc; + box-shadow: 0 0 0 3px rgba(0, 102, 204, 0.18); + outline: none; } .form-section small { display: block; - margin-top: 0.3em; + margin-top: 0.45em; color: #666; } .form-actions { @@ -142,4 +183,9 @@ text-align: center; color: #666; } +@media only screen and (max-width: 576px) { + .permission-form-grid { + grid-template-columns: minmax(0, 1fr); + } +} diff --git a/datasette/templates/_permissions_debug_tabs.html b/datasette/templates/_permissions_debug_tabs.html index d7203c1e..8e0f486e 100644 --- a/datasette/templates/_permissions_debug_tabs.html +++ b/datasette/templates/_permissions_debug_tabs.html @@ -44,10 +44,10 @@ diff --git a/datasette/templates/allow_debug.html b/datasette/templates/allow_debug.html index 1ecc92df..fda4032c 100644 --- a/datasette/templates/allow_debug.html +++ b/datasette/templates/allow_debug.html @@ -3,29 +3,11 @@ {% block title %}Debug allow rules{% endblock %} {% block extra_head %} +{% include "_permission_ui_styles.html" %} {% endblock %} @@ -38,24 +20,28 @@ p.message-warning {

Use this tool to try out different actor and allow combinations. See Defining permissions with "allow" blocks for documentation.

-
-
-

- -
-
-

- -
-
- -
-
+
+
+
+
+ + +
+
+ + +
+
+
+ +
+
-{% if error %}

{{ error }}

{% endif %} + {% if error %}

{{ error }}

{% endif %} -{% if result == "True" %}

Result: allow

{% endif %} + {% if result == "True" %}

Result: allow

{% endif %} -{% if result == "False" %}

Result: deny

{% endif %} + {% if result == "False" %}

Result: deny

{% endif %} +
{% endblock %} diff --git a/datasette/templates/debug_allowed.html b/datasette/templates/debug_allowed.html index 83cc1ae6..80249d9c 100644 --- a/datasette/templates/debug_allowed.html +++ b/datasette/templates/debug_allowed.html @@ -49,7 +49,7 @@
- + Number of results per page (max 200)
diff --git a/datasette/templates/debug_check.html b/datasette/templates/debug_check.html index 3b229a25..b9fc636a 100644 --- a/datasette/templates/debug_check.html +++ b/datasette/templates/debug_check.html @@ -1,6 +1,6 @@ {% extends "base.html" %} -{% block title %}Permission Check{% endblock %} +{% block title %}Explain a permission decision{% endblock %} {% block extra_head %} @@ -13,29 +13,35 @@ border-radius: 5px; } #output.allowed { - background-color: #e8f5e9; + background-color: #f3fbf4; border: 2px solid #4caf50; } #output.denied { - background-color: #ffebee; + background-color: #fff7f7; border: 2px solid #f44336; } #output h2 { margin-top: 0; } -#output .result-badge { +#output h3 { + margin-bottom: 0.5em; +} +#output .result-badge, +.effect-badge, +.rule-status { display: inline-block; - padding: 0.3em 0.8em; + padding: 0.2em 0.5em; border-radius: 3px; font-weight: bold; - font-size: 1.1em; } -#output .allowed-badge { - background-color: #4caf50; +#output .allowed-badge, +.effect-allow { + background-color: #2e7d32; color: white; } -#output .denied-badge { - background-color: #f44336; +#output .denied-badge, +.effect-deny { + background-color: #c62828; color: white; } .details-section { @@ -48,70 +54,130 @@ .details-section dd { margin-left: 1em; } +.explanation-section { + background: rgba(255, 255, 255, 0.75); + border: 1px solid #ddd; + border-radius: 4px; + margin-top: 1em; + padding: 0 1em 1em; +} +.rules-table { + border-collapse: collapse; + width: 100%; +} +.rules-table th, +.rules-table td { + border-bottom: 1px solid #ddd; + padding: 0.5em; + text-align: left; + vertical-align: top; +} +.rule-status { + background: #e8f5e9; + color: #1b5e20; +} +.rule-ignored { + background: #eee; + color: #555; + font-weight: normal; +} +.requirement-allowed { + color: #1b5e20; +} +.requirement-denied { + color: #b71c1c; +} +@media only screen and (max-width: 576px) { + .rules-table, + .rules-table tbody, + .rules-table tr, + .rules-table td { + display: block; + } + .rules-table thead { + display: none; + } + .rules-table td::before { + content: attr(data-label) ": "; + font-weight: bold; + } +} {% endblock %} {% block content %} -

Permission check

+

Explain a permission decision

{% set current_tab = "check" %} {% include "_permissions_debug_tabs.html" %} -

Use this tool to test permission checks for the current actor. It queries the /-/check.json API endpoint.

- -{% if request.actor %} -

Current actor: {{ request.actor.get("id", "anonymous") }}

-{% else %} -

Current actor: anonymous (not logged in)

-{% endif %} +

Test an actor, action and resource. The result explains which rules matched, which specificity level won, and whether actor restrictions or required actions changed the verdict.

-
+
- + + + Use null for an anonymous actor. This actor is simulated; it does not change who you are signed in as. +
+ +
+ - The permission action to check + The operation to evaluate
-
- +
+ - For database-level permissions, specify the database name + The database or other parent resource
-
- - - For table-level permissions, specify the table name (requires parent) +
+ + + The table, query or other child resource
- +
+actionSelect.addEventListener('change', updateResourceFields); +(function initializeFromUrl() { + const params = populateFormFromURL(); + updateResourceFields(); + if (params.get('action')) { + performCheck(); + } +})(); + {% endblock %} diff --git a/datasette/templates/debug_permissions_playground.html b/datasette/templates/debug_permissions_playground.html index 4410a677..8b0cbbcf 100644 --- a/datasette/templates/debug_permissions_playground.html +++ b/datasette/templates/debug_permissions_playground.html @@ -1,6 +1,6 @@ {% extends "base.html" %} -{% block title %}Debug permissions{% endblock %} +{% block title %}Permission activity{% endblock %} {% block extra_head %} {% include "_permission_ui_styles.html" %} @@ -20,60 +20,45 @@ .check-action, .check-when, .check-result { font-size: 1.3em; } -textarea { - height: 10em; - width: 95%; - box-sizing: border-box; - padding: 0.5em; - border: 2px dotted black; -} -.two-col { - display: inline-block; - width: 48%; -} -.two-col label { - width: 48%; -} -@media only screen and (max-width: 576px) { - .two-col { - width: 100%; - } -} {% endblock %} {% block content %} -

Permission playground

+

Permission activity

{% set current_tab = "permissions" %} {% include "_permissions_debug_tabs.html" %} -

This tool lets you simulate an actor and a permission check for that actor.

+

Raw simulator

+ +

This form runs a hypothetical permission check and returns its raw explanation JSON. Use the Explain tool for a visual explanation of the same decision.

-
-
- - +
+
+
+ + +
-
-
-
- - -
-
- - -
-
- - +
+
+ + +
+
+ + +
+
+ + +
@@ -125,7 +110,7 @@ debugPost.addEventListener('submit', function(ev) { }); -

Recent permissions checks

+

Recent permission checks

{% if filter != "all" %}All{% else %}All{% endif %}, diff --git a/datasette/templates/debug_rules.html b/datasette/templates/debug_rules.html index d00ba9cc..233c0e94 100644 --- a/datasette/templates/debug_rules.html +++ b/datasette/templates/debug_rules.html @@ -37,7 +37,7 @@

- + Number of results per page (max 200)
diff --git a/datasette/utils/actions_sql.py b/datasette/utils/actions_sql.py index c7137e6b..67d3ce73 100644 --- a/datasette/utils/actions_sql.py +++ b/datasette/utils/actions_sql.py @@ -673,3 +673,239 @@ async def check_permission_for_resource( child=child, ) return results[action] + + +async def explain_permission_for_resource( + *, + datasette: "Datasette", + actor: dict | None, + action: str, + parent: str | None, + child: str | None, +) -> dict: + """Explain a permission decision for one action and resource. + + This is intended for Datasette's permission debugging tools. It uses the + same ``permission_resources_sql`` hook results and the same resolution + rules as :func:`check_permissions_for_actions`, but also returns the + matching rules, actor restriction results and ``also_requires`` chain. + + The returned dictionary is part of Datasette's unstable debugging API. + """ + + action_obj = datasette.actions.get(action) + if action_obj is None: + raise ValueError(f"Unknown action: {action}") + + explanation = await _explain_single_action( + datasette=datasette, + actor=actor, + action=action, + parent=parent, + child=child, + ) + + required_actions = [] + if action_obj.also_requires: + required = await explain_permission_for_resource( + datasette=datasette, + actor=actor, + action=action_obj.also_requires, + parent=parent, + child=child, + ) + required_actions.append(required) + + explanation["required_actions"] = required_actions + explanation["allowed"] = bool( + explanation["rule_allowed"] + and explanation["restriction_allowed"] + and all(required["allowed"] for required in required_actions) + ) + explanation["summary"] = _permission_explanation_summary(explanation) + return explanation + + +async def _explain_single_action( + *, + datasette: "Datasette", + actor: dict | None, + action: str, + parent: str | None, + child: str | None, +) -> dict: + """Return matching rules and restrictions for a single action.""" + from datasette.utils.permissions import SKIP_PERMISSION_CHECKS + + permission_sqls = await gather_permission_sql_from_hooks( + datasette=datasette, + actor=actor, + action=action, + ) + + if permission_sqls is SKIP_PERMISSION_CHECKS: + return { + "action": action, + "rule_allowed": True, + "restriction_allowed": True, + "winning_scope": "global", + "matched_rules": [ + { + "scope": "global", + "effect": "allow", + "source": "skip_permission_checks", + "reason": "Permission checks were explicitly skipped", + "decisive": True, + "ignored_because": None, + } + ], + "restrictions": [], + } + + db = datasette.get_internal_database() + matched_rules = [] + restrictions = [] + + for permission_sql in permission_sqls: + params = dict(permission_sql.params or {}) + parent_param = _unused_parameter_name(params, "_explain_parent") + params[parent_param] = parent + child_param = _unused_parameter_name(params, "_explain_child") + params[child_param] = child + + if permission_sql.sql: + rows = await db.execute( + f""" + SELECT parent, child, allow, reason + FROM ({permission_sql.sql}) AS permission_rules + WHERE (parent IS NULL OR parent = :{parent_param}) + AND (child IS NULL OR child = :{child_param}) + """, + params, + ) + for row in rows: + specificity = ( + 2 + if row["child"] is not None + else 1 if row["parent"] is not None else 0 + ) + matched_rules.append( + { + "scope": ("resource", "parent", "global")[2 - specificity], + "effect": "allow" if row["allow"] else "deny", + "source": permission_sql.source, + "reason": row["reason"], + "_specificity": specificity, + } + ) + + if permission_sql.restriction_sql: + restriction_row = ( + await db.execute( + f""" + SELECT EXISTS( + SELECT 1 FROM ({permission_sql.restriction_sql}) AS restriction_rules + WHERE (parent IS NULL OR parent = :{parent_param}) + AND (child IS NULL OR child = :{child_param}) + ) AS resource_is_in_allowlist + """, + params, + ) + ).first() + restriction_allowed = bool(restriction_row[0]) + restrictions.append( + { + "source": permission_sql.source, + "allowed": restriction_allowed, + "reason": params.get("deny") + or ( + "Resource is included in this restriction allowlist" + if restriction_allowed + else "Resource is not included in this restriction allowlist" + ), + } + ) + + matched_rules.sort( + key=lambda rule: ( + -rule["_specificity"], + 0 if rule["effect"] == "deny" else 1, + rule["source"] or "", + rule["reason"] or "", + ) + ) + + if matched_rules: + winning_specificity = matched_rules[0]["_specificity"] + winning_rules = [ + rule + for rule in matched_rules + if rule["_specificity"] == winning_specificity + ] + rule_allowed = not any(rule["effect"] == "deny" for rule in winning_rules) + winning_scope = winning_rules[0]["scope"] + else: + winning_specificity = None + rule_allowed = False + winning_scope = None + + for rule in matched_rules: + specificity = rule.pop("_specificity") + if specificity != winning_specificity: + rule["decisive"] = False + rule["ignored_because"] = "A more specific rule matched" + elif not rule_allowed and rule["effect"] == "allow": + rule["decisive"] = False + rule["ignored_because"] = "A deny rule matched at the same scope" + else: + rule["decisive"] = True + rule["ignored_because"] = None + + return { + "action": action, + "rule_allowed": rule_allowed, + "restriction_allowed": all( + restriction["allowed"] for restriction in restrictions + ), + "winning_scope": winning_scope, + "matched_rules": matched_rules, + "restrictions": restrictions, + } + + +def _unused_parameter_name(params: dict, preferred: str) -> str: + """Return a SQL parameter name that is not already in ``params``.""" + candidate = preferred + suffix = 2 + while candidate in params: + candidate = f"{preferred}_{suffix}" + suffix += 1 + return candidate + + +def _permission_explanation_summary(explanation: dict) -> str: + denied_requirement = next( + ( + required + for required in explanation["required_actions"] + if not required["allowed"] + ), + None, + ) + if denied_requirement: + return ( + f"Denied because {explanation['action']} also requires " + f"{denied_requirement['action']}, which was denied." + ) + if not explanation["matched_rules"]: + return "Denied because no permission rule matched this actor and resource." + if not explanation["rule_allowed"]: + return ( + f"Denied by a {explanation['winning_scope']}-level rule. " + "Deny rules take precedence over allow rules at the same scope." + ) + if not explanation["restriction_allowed"]: + return ( + "Denied because the resource is not included in the actor's restrictions." + ) + return f"Allowed by the matching {explanation['winning_scope']}-level rule." diff --git a/datasette/views/special.py b/datasette/views/special.py index c13191a1..28d34208 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -600,7 +600,7 @@ class PermissionRulesView(BaseView): async def _check_permission_for_actor(ds, action, parent, child, actor): - """Shared logic for checking permissions. Returns a dict with check results.""" + """Shared logic for checking and explaining a permission decision.""" if action not in ds.actions: return error_body(f"Unknown action: {action}", 404), 404 @@ -629,15 +629,28 @@ async def _check_permission_for_actor(ds, action, parent, child, actor): allowed = await ds.allowed(action=action, resource=resource_obj, actor=actor) + from datasette.utils.actions_sql import explain_permission_for_resource + + explanation = await explain_permission_for_resource( + datasette=ds, + actor=actor, + action=action, + parent=parent, + child=child, + ) + response = { "ok": True, + "unstable": UNSTABLE_API_MESSAGE, "action": action, "allowed": bool(allowed), + "actor": actor, "resource": { "parent": parent, "child": child, "path": _resource_path(parent, child), }, + "explanation": explanation, } if actor and "id" in actor: @@ -655,11 +668,25 @@ class PermissionCheckView(BaseView): as_format = request.url_vars.get("format") if not as_format: + actions = [ + { + "name": action.name, + "description": action.description, + "takes_parent": action.takes_parent, + "takes_child": action.takes_child, + "also_requires": action.also_requires, + } + for action in sorted( + self.ds.actions.values(), key=lambda action: action.name + ) + ] return await self.render( ["debug_check.html"], request, { - "sorted_actions": sorted(self.ds.actions.keys()), + "actions": actions, + "actor_json": request.args.get("actor") + or json.dumps(request.actor, indent=2), "has_debug_permission": True, }, ) @@ -671,9 +698,18 @@ class PermissionCheckView(BaseView): parent = request.args.get("parent") child = request.args.get("child") + actor = request.actor + actor_json = request.args.get("actor") + if actor_json is not None: + try: + actor = json.loads(actor_json) + except json.JSONDecodeError as ex: + return Response.error(f"Invalid actor JSON: {ex}", 400) + if actor is not None and not isinstance(actor, dict): + return Response.error("actor must be a JSON object or null", 400) response, status = await _check_permission_for_actor( - self.ds, action, parent, child, request.actor + self.ds, action, parent, child, actor ) return Response.json(response, status=status) diff --git a/docs/authentication.rst b/docs/authentication.rst index 51fa07d5..d720c4db 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -45,7 +45,7 @@ Using the "root" actor Datasette currently leaves almost all forms of authentication to plugins - `datasette-auth-github `__ for example. -The one exception is the "root" account, which you can sign into while using Datasette on your local machine. The root user has **all permissions** - they can perform any action regardless of other permission rules. +The one exception is the "root" account, which you can sign into while using Datasette on your local machine. The root user starts with **all permissions**: Datasette contributes a global allow rule for every action. More specific deny rules can still override that global rule. The ``--root`` flag is designed for local development and testing. When you start Datasette with ``--root``, the root user automatically receives every permission, including: @@ -84,12 +84,12 @@ Click on that link and then visit ``http://127.0.0.1:8001/-/actor`` to confirm t Permissions =========== -Datasette's permissions system is built around SQL queries. Datasette and its plugins construct SQL queries to resolve the list of resources that an actor cas access. - The key question the permissions system answers is this: Is this **actor** allowed to perform this **action**, optionally against this particular **resource**? +Every permission decision can be understood in terms of those three values. Datasette implements the decisions using SQL, but you do not need to understand the generated SQL to configure or debug permissions. + **Actors** are :ref:`described above `. An **action** is a string describing the action the actor would like to perform. A full list is :ref:`provided below ` - examples include ``view-table`` and ``execute-sql``. @@ -138,7 +138,51 @@ This configuration will deny access to everyone except the user with ``id`` of ` How permissions are resolved ---------------------------- -Datasette performs permission checks using the internal :ref:`datasette_allowed`, method which accepts keyword arguments for ``action``, ``resource`` and an optional ``actor``. +Permission rules describe an effect (``allow`` or ``deny``) at one of three levels: + +``resource`` + A specific child resource, such as the ``analytics/sales`` table. + +``parent`` + A parent resource, such as the ``analytics`` database. A parent rule also applies to its child resources. + +``global`` + Every resource for that action. + +Datasette resolves matching rules from most specific to least specific: + +#. Resource rules take precedence over parent and global rules. +#. Parent rules take precedence over global rules. +#. If both allow and deny rules match at the same level, deny takes precedence. +#. If no rule matches, access is denied. + +This means a resource-level allow can provide an exception to a parent-level deny. It also means that two plugins which disagree at the same level resolve to deny. + +.. list-table:: Permission rule examples + :header-rows: 1 + + * - Matching rules + - Result + - Explanation + * - Global allow + - Allow + - The global rule is the most specific matching rule. + * - Global allow, parent deny + - Deny + - The parent rule is more specific. + * - Parent deny, resource allow + - Allow + - The resource rule is more specific. + * - Resource allow and resource deny + - Deny + - Deny takes precedence at the same level. + * - No matching rules + - Deny + - Permissions default to deny when no rule applies. + +The built-in public defaults are global allow rules for actions such as ``view-instance``, ``view-database`` and ``view-table``. They follow the same precedence rules as configuration and plugin rules. The ``--default-deny`` option prevents Datasette from contributing those default allow rules. + +Datasette performs checks using :ref:`datasette_allowed`, which accepts keyword arguments for ``action``, ``resource`` and an optional ``actor``. ``resource`` should be an instance of the appropriate ``Resource`` subclass from :mod:`datasette.resources`—for example ``InstanceResource()``, ``DatabaseResource(database="...``)`` or ``TableResource(database="...", table="...")``. This defaults to ``InstanceResource()`` if not specified. @@ -149,12 +193,12 @@ resources were allowed or denied. The combined sources are: * ``allow`` blocks configured in :ref:`datasette.yaml `. * :ref:`Actor restrictions ` encoded into the actor dictionary or API token. -* The "root" user shortcut when ``--root`` (or :attr:`Datasette.root_enabled `) is active, replying ``True`` to all permission chucks unless configuration rules deny them at a more specific level. +* The "root" user rule when ``--root`` (or :attr:`Datasette.root_enabled `) is active. This is a global allow rule, so a more specific configuration deny can override it. * Any additional SQL provided by plugins implementing :ref:`plugin_hook_permission_resources_sql`. -Datasette evaluates the SQL to determine if the requested ``resource`` is -included. Explicit deny rules returned by configuration or plugins will block -access even if other rules allowed it. +Actor restrictions are applied after the allow/deny rules. They act as an additional allowlist: a restriction can remove access but cannot grant access that the actor did not already have. See :ref:`authentication_cli_create_token_restrict`. + +Some actions have dependencies on other actions. These are evaluated as an ``AND`` condition. For example, ``execute-sql`` also requires ``view-database``: both decisions must be allowed for the final result to be allowed. .. _authentication_permissions_allow: @@ -1145,11 +1189,21 @@ The debug tool at ``/-/permissions`` is available to any actor with the ``permis datasette -s permissions.permissions-debug true data.db -The page shows the permission checks that have been carried out by the Datasette instance. +The permission debug tools answer four different questions: -It also provides an interface for running hypothetical permission checks against a hypothetical actor. This is a useful way of confirming that your configured permissions work in the way you expect. +Why was this decision allowed or denied? + Use :ref:`PermissionCheckView`. It shows every matching rule, identifies the winning specificity level, applies actor restrictions and evaluates any required actions. -This is designed to help administrators and plugin authors understand exactly how permission checks are being carried out, in order to effectively configure Datasette's permission system. +Which resources can the current actor access? + Use :ref:`AllowedResourcesView` to view an access map for a selected action. + +Which raw rules did Datasette and its plugins contribute? + Use :ref:`PermissionRulesView` to inspect the rules before they are resolved into decisions. + +Which checks has this Datasette instance performed recently? + Use ``/-/permissions`` to view recent permission activity. + +These tools are designed to help administrators and plugin authors understand and confirm the effective permissions configuration. These debug endpoints are exempt from the :ref:`JSON API stability promise ` - their JSON shapes may change in future releases. @@ -1184,11 +1238,20 @@ This endpoint requires the ``permissions-debug`` permission. Permission check view --------------------- -The ``/-/check`` endpoint evaluates a single action/resource pair and returns information indicating whether the access was allowed along with diagnostic information. +The ``/-/check`` endpoint evaluates and explains a single actor, action and resource decision. The explanation includes: + +* Every matching allow and deny rule, with its source and reason. +* The winning resource, parent or global scope. +* Rules ignored because a more specific rule matched, or because a deny won at the same scope. +* Actor restriction allowlists that included or excluded the resource. +* Additional actions required by the requested action. +* An explicit default-deny explanation when no rule matched. This endpoint provides an interactive HTML form interface. Add ``.json`` to the URL path (e.g. ``/-/check.json?action=view-instance``) to get the raw JSON response instead. -Pass ``?action=`` to specify the action to check, and optional ``?parent=`` and ``?child=`` parameters to specify the resource. +Pass ``?action=`` to specify the action to check, and optional ``?parent=`` and ``?child=`` parameters to specify the resource. The interactive form also accepts actor JSON, allowing a hypothetical actor to be tested without signing in as that actor. The JSON endpoint accepts the same value using the ``actor`` query string parameter. Use ``actor=null`` to represent an anonymous actor. + +This endpoint requires the ``permissions-debug`` permission. The hypothetical actor is used only for the decision being explained; access to the debug tool is checked against the actor who is actually signed in. .. _authentication_ds_actor: diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 88fe577f..cd1050d0 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -457,6 +457,20 @@ async def test_permissions_debug(ds_client, filter_): assert checks == expected_checks +@pytest.mark.asyncio +@pytest.mark.parametrize( + "permissions_debug,expected_status", + ( + (1, 200), + (0, 403), + ), +) +async def test_permissions_debug_numeric_boolean(permissions_debug, expected_status): + ds = Datasette(config={"permissions": {"permissions-debug": permissions_debug}}) + response = await ds.client.get("/-/permissions") + assert response.status_code == expected_status + + @pytest.mark.asyncio @pytest.mark.parametrize( "actor,allow,expected_fragment", @@ -748,7 +762,12 @@ async def test_actor_restricted_permissions( } if actor.get("id"): expected["actor_id"] = actor["id"] - assert response.json() == expected + data = response.json() + for key, value in expected.items(): + assert data[key] == value + assert data["actor"] == actor + assert data["explanation"]["allowed"] is expected_result + assert data["explanation"]["summary"] PermConfigTestCase = collections.namedtuple( @@ -1734,6 +1753,8 @@ async def test_permission_check_view_requires_debug_permission(): data = response.json() assert data["action"] == "view-instance" assert data["allowed"] is True + assert data["explanation"]["allowed"] is True + assert data["explanation"]["summary"] @pytest.mark.asyncio @@ -1759,6 +1780,211 @@ async def test_permission_check_view_query_actions(action): } +@pytest.mark.asyncio +async def test_permission_check_explains_specificity_for_hypothetical_actor(): + ds = Datasette( + config={ + "permissions": {"view-table": {"id": "alice"}}, + "databases": { + "analytics": { + "permissions": {"view-table": False}, + "tables": { + "public": {"permissions": {"view-table": {"id": "alice"}}} + }, + } + }, + } + ) + ds.root_enabled = True + await ds.invoke_startup() + + def path_for(child): + return "/-/check.json?" + urllib.parse.urlencode( + { + "action": "view-table", + "parent": "analytics", + "child": child, + "actor": json.dumps({"id": "alice"}), + } + ) + + public_response = await ds.client.get(path_for("public"), actor={"id": "root"}) + assert public_response.status_code == 200 + public = public_response.json() + assert public["actor"] == {"id": "alice"} + assert public["allowed"] is True + assert public["explanation"]["allowed"] is True + assert public["explanation"]["winning_scope"] == "resource" + public_rules = public["explanation"]["matched_rules"] + assert any( + rule["scope"] == "resource" and rule["effect"] == "allow" and rule["decisive"] + for rule in public_rules + ) + assert any( + rule["scope"] == "parent" + and rule["effect"] == "deny" + and rule["ignored_because"] == "A more specific rule matched" + for rule in public_rules + ) + + private_response = await ds.client.get(path_for("private"), actor={"id": "root"}) + assert private_response.status_code == 200 + private = private_response.json() + assert private["allowed"] is False + assert private["explanation"]["allowed"] is False + assert private["explanation"]["winning_scope"] == "parent" + assert private["explanation"]["summary"].startswith("Denied by a parent-level rule") + + +@pytest.mark.asyncio +async def test_permission_check_explains_deny_wins_at_same_scope(): + ds = Datasette(config={"permissions": {"view-table": {"id": "someone-else"}}}) + ds.root_enabled = True + await ds.invoke_startup() + path = "/-/check.json?" + urllib.parse.urlencode( + { + "action": "view-table", + "parent": "analytics", + "child": "users", + "actor": json.dumps({"id": "alice"}), + } + ) + response = await ds.client.get(path, actor={"id": "root"}) + assert response.status_code == 200 + data = response.json() + assert data["allowed"] is False + assert data["explanation"]["winning_scope"] == "global" + rules = data["explanation"]["matched_rules"] + assert any(rule["effect"] == "deny" and rule["decisive"] for rule in rules) + assert any( + rule["effect"] == "allow" + and rule["ignored_because"] == "A deny rule matched at the same scope" + for rule in rules + ) + + +@pytest.mark.asyncio +async def test_permission_check_explains_default_deny(): + ds = Datasette() + ds.root_enabled = True + await ds.invoke_startup() + path = "/-/check.json?" + urllib.parse.urlencode( + { + "action": "insert-row", + "parent": "analytics", + "child": "users", + "actor": json.dumps({"id": "alice"}), + } + ) + response = await ds.client.get(path, actor={"id": "root"}) + assert response.status_code == 200 + data = response.json() + assert data["allowed"] is False + explanation = data["explanation"] + assert explanation["allowed"] is False + assert explanation["matched_rules"] == [] + assert explanation["winning_scope"] is None + assert explanation["summary"] == ( + "Denied because no permission rule matched this actor and resource." + ) + + +@pytest.mark.asyncio +async def test_permission_check_explains_actor_restrictions(): + ds = Datasette() + ds.root_enabled = True + await ds.invoke_startup() + restricted_actor = { + "id": "alice", + "_r": {"r": {"analytics": {"public": ["vt"]}}}, + } + path = "/-/check.json?" + urllib.parse.urlencode( + { + "action": "view-table", + "parent": "analytics", + "child": "private", + "actor": json.dumps(restricted_actor), + } + ) + response = await ds.client.get(path, actor={"id": "root"}) + assert response.status_code == 200 + data = response.json() + assert data["allowed"] is False + explanation = data["explanation"] + assert explanation["rule_allowed"] is True + assert explanation["restriction_allowed"] is False + assert explanation["allowed"] is False + assert explanation["restrictions"] + assert any( + restriction["allowed"] is False for restriction in explanation["restrictions"] + ) + assert "actor's restrictions" in explanation["summary"] + + +@pytest.mark.asyncio +async def test_permission_check_explains_required_actions(): + from datasette import hookimpl + from datasette.permissions import PermissionSQL + + class StoreQueryPermissions: + @hookimpl + def permission_resources_sql(self, actor, action): + if not actor or actor.get("id") != "alice": + return None + if action == "store-query": + return PermissionSQL( + sql="SELECT 'analytics' AS parent, NULL AS child, 1 AS allow, 'alice can store queries' AS reason" + ) + if action == "execute-sql": + return PermissionSQL( + sql="SELECT 'analytics' AS parent, NULL AS child, 0 AS allow, 'alice cannot execute SQL' AS reason" + ) + + ds = Datasette() + ds.root_enabled = True + await ds.invoke_startup() + ds.pm.register(StoreQueryPermissions(), name="store-query-test") + path = "/-/check.json?" + urllib.parse.urlencode( + { + "action": "store-query", + "parent": "analytics", + "actor": json.dumps({"id": "alice"}), + } + ) + response = await ds.client.get(path, actor={"id": "root"}) + assert response.status_code == 200 + data = response.json() + assert data["allowed"] is False + explanation = data["explanation"] + assert explanation["rule_allowed"] is True + assert explanation["required_actions"][0]["action"] == "execute-sql" + assert explanation["required_actions"][0]["allowed"] is False + assert explanation["summary"] == ( + "Denied because store-query also requires execute-sql, which was denied." + ) + + +@pytest.mark.asyncio +async def test_permission_check_hypothetical_actor_validation(): + ds = Datasette() + ds.root_enabled = True + await ds.invoke_startup() + + response = await ds.client.get( + "/-/check.json?action=view-instance&actor=not-json", + actor={"id": "root"}, + ) + assert response.status_code == 400 + assert response.json()["error"].startswith("Invalid actor JSON:") + + response = await ds.client.get( + "/-/check.json?action=view-instance&actor=%5B%5D", + actor={"id": "root"}, + ) + assert response.status_code == 400 + assert response.json()["error"] == "actor must be a JSON object or null" + + @pytest.mark.asyncio async def test_root_allow_block_with_table_restricted_actor(): """ From 9cfc252394eb21d05f45818dac9c374e2d141a35 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 14 Jul 2026 08:41:27 -0700 Subject: [PATCH 13/17] Make internal catalog refresh atomic Refs #2831 --- datasette/app.py | 14 +--- datasette/utils/internal_db.py | 133 ++++++++++++++++++--------------- 2 files changed, 72 insertions(+), 75 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index c44f9095..0e31273d 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -753,19 +753,7 @@ class Datasette: # Compare schema versions to see if we should skip it if schema_version == current_schema_versions.get(database_name): continue - placeholders = "(?, ?, ?, ?)" - values = [database_name, str(db.path), db.is_memory, schema_version] - if db.path is None: - placeholders = "(?, null, ?, ?)" - values = [database_name, db.is_memory, schema_version] - await internal_db.execute_write( - """ - INSERT OR REPLACE INTO catalog_databases (database_name, path, is_memory, schema_version) - VALUES {} - """.format(placeholders), - values, - ) - await populate_schema_tables(internal_db, db) + await populate_schema_tables(internal_db, db, schema_version) @property def urls(self): diff --git a/datasette/utils/internal_db.py b/datasette/utils/internal_db.py index 10ca32a5..e061d882 100644 --- a/datasette/utils/internal_db.py +++ b/datasette/utils/internal_db.py @@ -180,29 +180,9 @@ async def init_internal_db(db): await db.execute_write_fn(apply_migrations, transaction=False) -async def populate_schema_tables(internal_db, db): +async def populate_schema_tables(internal_db, db, schema_version): database_name = db.name - def delete_everything(conn): - conn.execute( - "DELETE FROM catalog_tables WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_views WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_columns WHERE database_name = ?", [database_name] - ) - conn.execute( - "DELETE FROM catalog_foreign_keys WHERE database_name = ?", - [database_name], - ) - conn.execute( - "DELETE FROM catalog_indexes WHERE database_name = ?", [database_name] - ) - - await internal_db.execute_write_fn(delete_everything) - tables = (await db.execute("select * from sqlite_master WHERE type = 'table'")).rows views = (await db.execute("select * from sqlite_master WHERE type = 'view'")).rows @@ -266,47 +246,76 @@ async def populate_schema_tables(internal_db, db): indexes_to_insert, ) = await db.execute_fn(collect_info) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_tables (database_name, table_name, rootpage, sql) - values (?, ?, ?, ?) - """, - tables_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_views (database_name, view_name, rootpage, sql) - values (?, ?, ?, ?) - """, - views_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_columns ( - database_name, table_name, cid, name, type, "notnull", default_value, is_pk, hidden - ) VALUES ( - :database_name, :table_name, :cid, :name, :type, :notnull, :default_value, :is_pk, :hidden + def replace_catalog(conn): + # Delete child rows before their catalog_tables parents so this also + # works if a prepare_connection plugin enables foreign key enforcement. + for table in ( + "catalog_columns", + "catalog_foreign_keys", + "catalog_indexes", + "catalog_views", + "catalog_tables", + ): + conn.execute( + "DELETE FROM {} WHERE database_name = ?".format(table), + [database_name], + ) + conn.execute( + """ + INSERT OR REPLACE INTO catalog_databases ( + database_name, path, is_memory, schema_version + ) VALUES (?, ?, ?, ?) + """, + [ + database_name, + str(db.path) if db.path is not None else None, + db.is_memory, + schema_version, + ], ) - """, - columns_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_foreign_keys ( - database_name, table_name, "id", seq, "table", "from", "to", on_update, on_delete, match - ) VALUES ( - :database_name, :table_name, :id, :seq, :table, :from, :to, :on_update, :on_delete, :match + conn.executemany( + """ + INSERT INTO catalog_tables (database_name, table_name, rootpage, sql) + values (?, ?, ?, ?) + """, + tables_to_insert, ) - """, - foreign_keys_to_insert, - ) - await internal_db.execute_write_many( - """ - INSERT INTO catalog_indexes ( - database_name, table_name, seq, name, "unique", origin, partial - ) VALUES ( - :database_name, :table_name, :seq, :name, :unique, :origin, :partial + conn.executemany( + """ + INSERT INTO catalog_views (database_name, view_name, rootpage, sql) + values (?, ?, ?, ?) + """, + views_to_insert, ) - """, - indexes_to_insert, - ) + conn.executemany( + """ + INSERT INTO catalog_columns ( + database_name, table_name, cid, name, type, "notnull", default_value, is_pk, hidden + ) VALUES ( + :database_name, :table_name, :cid, :name, :type, :notnull, :default_value, :is_pk, :hidden + ) + """, + columns_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_foreign_keys ( + database_name, table_name, "id", seq, "table", "from", "to", on_update, on_delete, match + ) VALUES ( + :database_name, :table_name, :id, :seq, :table, :from, :to, :on_update, :on_delete, :match + ) + """, + foreign_keys_to_insert, + ) + conn.executemany( + """ + INSERT INTO catalog_indexes ( + database_name, table_name, seq, name, "unique", origin, partial + ) VALUES ( + :database_name, :table_name, :seq, :name, :unique, :origin, :partial + ) + """, + indexes_to_insert, + ) + + await internal_db.execute_write_fn(replace_catalog) From 591b909a4d216ed76d3c775484df52b54e89dc74 Mon Sep 17 00:00:00 2001 From: TowyTowy <85077986+TowyTowy@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:53:45 +0200 Subject: [PATCH 14/17] Escape table names with [square] brackets, refs #2431 (#2846) Several internal helpers quoted table names using SQLite [bracket] identifiers built with an f-string, e.g. PRAGMA foreign_key_list([{table}]). Bracket quoting cannot escape a "]" character, so any table whose name contains "]" (for example "[foo]" or "foo]") produced "sqlite3.OperationalError: unrecognized token" - crashing schema introspection at startup and 500-ing the table page. Switch these call sites to the existing escape_sqlite() helper, which uses "double quote" quoting with correct "" escaping (the same approach already used elsewhere in the codebase and in the test suite): - utils/internal_db.py: PRAGMA foreign_key_list / index_list - utils/__init__.py: get_outbound_foreign_keys - database.py: table_counts count query - facets.py: default "select * from" SQL Added a regression test covering table names with "]" characters. Co-authored-by: Claude --- datasette/database.py | 3 ++- datasette/facets.py | 2 +- datasette/utils/__init__.py | 2 +- datasette/utils/internal_db.py | 8 +++++--- tests/test_api.py | 33 ++++++++++++++++++++++++++++++++- 5 files changed, 41 insertions(+), 7 deletions(-) diff --git a/datasette/database.py b/datasette/database.py index eb402b0c..bab3a378 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -17,6 +17,7 @@ from .utils import ( detect_fts, detect_primary_keys, detect_spatialite, + escape_sqlite, get_all_foreign_keys, get_outbound_foreign_keys, md5_not_usedforsecurity, @@ -608,7 +609,7 @@ class Database: try: table_count = ( await self.execute( - f"select count(*) from (select * from [{table}] limit {self.count_limit + 1})", + f"select count(*) from (select * from {escape_sqlite(table)} limit {self.count_limit + 1})", custom_time_limit=limit, ) ).rows[0][0] diff --git a/datasette/facets.py b/datasette/facets.py index abe0605e..5f757df3 100644 --- a/datasette/facets.py +++ b/datasette/facets.py @@ -85,7 +85,7 @@ class Facet: self.database = database # For foreign key expansion. Can be None for e.g. stored SQL queries: self.table = table - self.sql = sql or f"select * from [{table}]" + self.sql = sql or f"select * from {escape_sqlite(table)}" self.params = params or [] self.table_config = table_config # row_count can be None, in which case we calculate it ourselves: diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index 42574d3b..18d3ba52 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -636,7 +636,7 @@ def detect_primary_keys(conn, table): def get_outbound_foreign_keys(conn, table): - infos = conn.execute(f"PRAGMA foreign_key_list([{table}])").fetchall() + infos = conn.execute(f"PRAGMA foreign_key_list({escape_sqlite(table)})").fetchall() fks = [] for info in infos: if info is not None: diff --git a/datasette/utils/internal_db.py b/datasette/utils/internal_db.py index e061d882..702b53d8 100644 --- a/datasette/utils/internal_db.py +++ b/datasette/utils/internal_db.py @@ -3,7 +3,7 @@ import textwrap from sqlite_utils import Database as SQLiteUtilsDatabase from sqlite_utils import Migrations -from datasette.utils import table_column_details +from datasette.utils import escape_sqlite, table_column_details INTERNAL_DB_SCHEMA_TABLES = { "catalog_databases", @@ -213,7 +213,7 @@ async def populate_schema_tables(internal_db, db, schema_version): for column in columns ) foreign_keys = conn.execute( - f"PRAGMA foreign_key_list([{table_name}])" + f"PRAGMA foreign_key_list({escape_sqlite(table_name)})" ).fetchall() foreign_keys_to_insert.extend( { @@ -222,7 +222,9 @@ async def populate_schema_tables(internal_db, db, schema_version): } for foreign_key in foreign_keys ) - indexes = conn.execute(f"PRAGMA index_list([{table_name}])").fetchall() + indexes = conn.execute( + f"PRAGMA index_list({escape_sqlite(table_name)})" + ).fetchall() indexes_to_insert.extend( { **{"database_name": database_name, "table_name": table_name}, diff --git a/tests/test_api.py b/tests/test_api.py index d5f519b9..191d064a 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,6 +1,6 @@ from datasette.app import Datasette from datasette.plugins import DEFAULT_PLUGINS -from datasette.utils import UNSTABLE_API_MESSAGE +from datasette.utils import UNSTABLE_API_MESSAGE, escape_sqlite, tilde_encode from datasette.utils.sqlite import sqlite_version from datasette.version import __version__ from .fixtures import make_app_client, EXPECTED_PLUGINS @@ -930,6 +930,37 @@ async def test_tilde_encoded_database_names(db_name): assert response2.status_code == 200 +@pytest.mark.asyncio +@pytest.mark.parametrize("table_name", ("[foo]", "foo]", "[foo]/bar")) +async def test_table_with_reserved_characters_in_name(table_name): + # Table names containing characters such as "]" that cannot be escaped + # using SQLite [bracket] quoting used to break schema introspection and + # the table page - https://github.com/simonw/datasette/issues/2431 + ds = Datasette() + db = ds.add_memory_database("test_reserved_table_names") + await db.execute_write( + "create table {} (id integer primary key, name text)".format( + escape_sqlite(table_name) + ) + ) + await db.execute_write( + "insert into {} (id, name) values (1, 'one')".format(escape_sqlite(table_name)) + ) + # Schema introspection (populate_schema_tables) must not crash: + db_response = await ds.client.get("/test_reserved_table_names.json") + assert db_response.status_code == 200 + tables = {t["name"]: t for t in db_response.json()["tables"]} + assert tables[table_name]["count"] == 1 + # And the table page itself must load and return the row: + table_response = await ds.client.get( + "/test_reserved_table_names/{}.json?_shape=array".format( + tilde_encode(table_name) + ) + ) + assert table_response.status_code == 200 + assert table_response.json() == [{"id": 1, "name": "one"}] + + @pytest.mark.asyncio @pytest.mark.parametrize( "config,expected", From 8b7c942d5e5ada887aa89c4d58567af39d5a3e07 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 14 Jul 2026 09:18:51 -0700 Subject: [PATCH 15/17] Major performance boost for SQL permissions, closes #2832 --- datasette/utils/actions_sql.py | 217 +++++++++++++++++---------------- 1 file changed, 111 insertions(+), 106 deletions(-) diff --git a/datasette/utils/actions_sql.py b/datasette/utils/actions_sql.py index 67d3ce73..d767e391 100644 --- a/datasette/utils/actions_sql.py +++ b/datasette/utils/actions_sql.py @@ -252,88 +252,62 @@ async def _build_single_action_sql( ] ) - # Continue with the cascading logic - query_parts.extend( - [ - "child_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow,", - " json_group_array(CASE WHEN ar.allow = 0 THEN ar.source_plugin || ': ' || ar.reason END) AS deny_reasons,", - " json_group_array(CASE WHEN ar.allow = 1 THEN ar.source_plugin || ': ' || ar.reason END) AS allow_reasons", - " FROM base b", - " LEFT JOIN all_rules ar ON ar.parent = b.parent AND ar.child = b.child", - " GROUP BY b.parent, b.child", - "),", - "parent_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow,", - " json_group_array(CASE WHEN ar.allow = 0 THEN ar.source_plugin || ': ' || ar.reason END) AS deny_reasons,", - " json_group_array(CASE WHEN ar.allow = 1 THEN ar.source_plugin || ': ' || ar.reason END) AS allow_reasons", - " FROM base b", - " LEFT JOIN all_rules ar ON ar.parent = b.parent AND ar.child IS NULL", - " GROUP BY b.parent, b.child", - "),", - "global_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow,", - " json_group_array(CASE WHEN ar.allow = 0 THEN ar.source_plugin || ': ' || ar.reason END) AS deny_reasons,", - " json_group_array(CASE WHEN ar.allow = 1 THEN ar.source_plugin || ': ' || ar.reason END) AS allow_reasons", - " FROM base b", - " LEFT JOIN all_rules ar ON ar.parent IS NULL AND ar.child IS NULL", - " GROUP BY b.parent, b.child", - "),", + # Continue with the cascading logic. + # Aggregate the RULES by cascade level (small), rather than grouping + # base x rules (which scales with the number of resources). + def _agg(select_key, where, group_by): + parts = [ + f" SELECT {select_key}", + " MAX(CASE WHEN allow = 0 THEN 1 ELSE 0 END) AS any_deny,", + " MAX(CASE WHEN allow = 1 THEN 1 ELSE 0 END) AS any_allow,", + " json_group_array(CASE WHEN allow = 0 THEN source_plugin || ': ' || reason END) AS deny_reasons,", + " json_group_array(CASE WHEN allow = 1 THEN source_plugin || ': ' || reason END) AS allow_reasons", + f" FROM all_rules WHERE {where}", ] + if group_by: + parts.append(f" GROUP BY {group_by}") + return parts + + query_parts.extend( + ["child_agg AS ("] + + _agg( + "parent, child,", + "parent IS NOT NULL AND child IS NOT NULL", + "parent, child", + ) + + ["),", "parent_agg AS ("] + + _agg("parent,", "parent IS NOT NULL AND child IS NULL", "parent") + + ["),", "global_agg AS ("] + + _agg("", "parent IS NULL AND child IS NULL", None) + + ["),"] ) # Add anonymous decision logic if needed if include_is_private: - query_parts.extend( - [ - "anon_child_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow", - " FROM base b", - " LEFT JOIN anon_rules ar ON ar.parent = b.parent AND ar.child = b.child", - " GROUP BY b.parent, b.child", - "),", - "anon_parent_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow", - " FROM base b", - " LEFT JOIN anon_rules ar ON ar.parent = b.parent AND ar.child IS NULL", - " GROUP BY b.parent, b.child", - "),", - "anon_global_lvl AS (", - " SELECT b.parent, b.child,", - " MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny,", - " MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow", - " FROM base b", - " LEFT JOIN anon_rules ar ON ar.parent IS NULL AND ar.child IS NULL", - " GROUP BY b.parent, b.child", - "),", - "anon_decisions AS (", - " SELECT", - " b.parent, b.child,", - " CASE", - " WHEN acl.any_deny = 1 THEN 0", - " WHEN acl.any_allow = 1 THEN 1", - " WHEN apl.any_deny = 1 THEN 0", - " WHEN apl.any_allow = 1 THEN 1", - " WHEN agl.any_deny = 1 THEN 0", - " WHEN agl.any_allow = 1 THEN 1", - " ELSE 0", - " END AS anon_is_allowed", - " FROM base b", - " JOIN anon_child_lvl acl ON b.parent = acl.parent AND (b.child = acl.child OR (b.child IS NULL AND acl.child IS NULL))", - " JOIN anon_parent_lvl apl ON b.parent = apl.parent AND (b.child = apl.child OR (b.child IS NULL AND apl.child IS NULL))", - " JOIN anon_global_lvl agl ON b.parent = agl.parent AND (b.child = agl.child OR (b.child IS NULL AND agl.child IS NULL))", - "),", + + def _anon_agg(select_key, where, group_by): + parts = [ + f" SELECT {select_key}", + " MAX(CASE WHEN allow = 0 THEN 1 ELSE 0 END) AS any_deny,", + " MAX(CASE WHEN allow = 1 THEN 1 ELSE 0 END) AS any_allow", + f" FROM anon_rules WHERE {where}", ] + if group_by: + parts.append(f" GROUP BY {group_by}") + return parts + + query_parts.extend( + ["anon_child_agg AS ("] + + _anon_agg( + "parent, child,", + "parent IS NOT NULL AND child IS NOT NULL", + "parent, child", + ) + + ["),", "anon_parent_agg AS ("] + + _anon_agg("parent,", "parent IS NOT NULL AND child IS NULL", "parent") + + ["),", "anon_global_agg AS ("] + + _anon_agg("", "parent IS NULL AND child IS NULL", None) + + ["),"] ) # Final decisions @@ -342,31 +316,28 @@ async def _build_single_action_sql( "decisions AS (", " SELECT", " b.parent, b.child,", - " -- Cascading permission logic: child → parent → global, DENY beats ALLOW at each level", + " -- Cascading permission logic: child -> parent -> global, DENY beats ALLOW at each level", " -- Priority order:", - " -- 1. Child-level deny (most specific, blocks access)", - " -- 2. Child-level allow (most specific, grants access)", - " -- 3. Parent-level deny (intermediate, blocks access)", - " -- 4. Parent-level allow (intermediate, grants access)", - " -- 5. Global-level deny (least specific, blocks access)", - " -- 6. Global-level allow (least specific, grants access)", + " -- 1. Child-level deny 2. Child-level allow", + " -- 3. Parent-level deny 4. Parent-level allow", + " -- 5. Global-level deny 6. Global-level allow", " -- 7. Default deny (no rules match)", " CASE", - " WHEN cl.any_deny = 1 THEN 0", - " WHEN cl.any_allow = 1 THEN 1", - " WHEN pl.any_deny = 1 THEN 0", - " WHEN pl.any_allow = 1 THEN 1", - " WHEN gl.any_deny = 1 THEN 0", - " WHEN gl.any_allow = 1 THEN 1", + " WHEN ca.any_deny = 1 THEN 0", + " WHEN ca.any_allow = 1 THEN 1", + " WHEN pa.any_deny = 1 THEN 0", + " WHEN pa.any_allow = 1 THEN 1", + " WHEN ga.any_deny = 1 THEN 0", + " WHEN ga.any_allow = 1 THEN 1", " ELSE 0", " END AS is_allowed,", " CASE", - " WHEN cl.any_deny = 1 THEN cl.deny_reasons", - " WHEN cl.any_allow = 1 THEN cl.allow_reasons", - " WHEN pl.any_deny = 1 THEN pl.deny_reasons", - " WHEN pl.any_allow = 1 THEN pl.allow_reasons", - " WHEN gl.any_deny = 1 THEN gl.deny_reasons", - " WHEN gl.any_allow = 1 THEN gl.allow_reasons", + " WHEN ca.any_deny = 1 THEN ca.deny_reasons", + " WHEN ca.any_allow = 1 THEN ca.allow_reasons", + " WHEN pa.any_deny = 1 THEN pa.deny_reasons", + " WHEN pa.any_allow = 1 THEN pa.allow_reasons", + " WHEN ga.any_deny = 1 THEN ga.deny_reasons", + " WHEN ga.any_allow = 1 THEN ga.allow_reasons", " ELSE '[]'", " END AS reason", ] @@ -374,21 +345,34 @@ async def _build_single_action_sql( if include_is_private: query_parts.append( - " , CASE WHEN ad.anon_is_allowed = 0 THEN 1 ELSE 0 END AS is_private" + " , CASE WHEN (" + "CASE" + " WHEN aca.any_deny = 1 THEN 0" + " WHEN aca.any_allow = 1 THEN 1" + " WHEN apa.any_deny = 1 THEN 0" + " WHEN apa.any_allow = 1 THEN 1" + " WHEN aga.any_deny = 1 THEN 0" + " WHEN aga.any_allow = 1 THEN 1" + " ELSE 0 END" + ") = 0 THEN 1 ELSE 0 END AS is_private" ) query_parts.extend( [ " FROM base b", - " JOIN child_lvl cl ON b.parent = cl.parent AND (b.child = cl.child OR (b.child IS NULL AND cl.child IS NULL))", - " JOIN parent_lvl pl ON b.parent = pl.parent AND (b.child = pl.child OR (b.child IS NULL AND pl.child IS NULL))", - " JOIN global_lvl gl ON b.parent = gl.parent AND (b.child = gl.child OR (b.child IS NULL AND gl.child IS NULL))", + " LEFT JOIN child_agg ca ON ca.parent = b.parent AND ca.child = b.child", + " LEFT JOIN parent_agg pa ON pa.parent = b.parent", + " CROSS JOIN global_agg ga", ] ) if include_is_private: - query_parts.append( - " JOIN anon_decisions ad ON b.parent = ad.parent AND (b.child = ad.child OR (b.child IS NULL AND ad.child IS NULL))" + query_parts.extend( + [ + " LEFT JOIN anon_child_agg aca ON aca.parent = b.parent AND aca.child = b.child", + " LEFT JOIN anon_parent_agg apa ON apa.parent = b.parent", + " CROSS JOIN anon_global_agg aga", + ] ) query_parts.append(")") @@ -400,8 +384,28 @@ async def _build_single_action_sql( restriction_intersect = "\nINTERSECT\n".join( f"SELECT * FROM ({sql})" for sql in restriction_sqls ) + # Decompose by NULL-pattern so the final filter can use pure-equality + # EXISTS lookups (satisfiable via automatic indexes) instead of a + # correlated OR-scan over the whole list. query_parts.extend( - [",", "restriction_list AS (", f" {restriction_intersect}", ")"] + [ + ",", + "restriction_list AS (", + f" {restriction_intersect}", + "),", + "restriction_exact AS (", + " SELECT parent, child FROM restriction_list WHERE parent IS NOT NULL AND child IS NOT NULL", + "),", + "restriction_parent_any AS (", + " SELECT DISTINCT parent FROM restriction_list WHERE parent IS NOT NULL AND child IS NULL", + "),", + "restriction_child_any AS (", + " SELECT DISTINCT child FROM restriction_list WHERE parent IS NULL AND child IS NOT NULL", + "),", + "restriction_all AS (", + " SELECT 1 AS matched FROM restriction_list WHERE parent IS NULL AND child IS NULL LIMIT 1", + ")", + ] ) # Final SELECT @@ -416,10 +420,11 @@ async def _build_single_action_sql( # Add restriction filter if there are restrictions if restriction_sqls: query_parts.append(""" - AND EXISTS ( - SELECT 1 FROM restriction_list r - WHERE (r.parent = decisions.parent OR r.parent IS NULL) - AND (r.child = decisions.child OR r.child IS NULL) + AND ( + EXISTS (SELECT 1 FROM restriction_all) + OR EXISTS (SELECT 1 FROM restriction_parent_any r WHERE r.parent = decisions.parent) + OR EXISTS (SELECT 1 FROM restriction_child_any r WHERE r.child = decisions.child) + OR EXISTS (SELECT 1 FROM restriction_exact r WHERE r.parent = decisions.parent AND r.child = decisions.child) )""") # Add parent filter if specified From 2ffd8a860e84ff58922e633c8e85e9a8e088ca93 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 14 Jul 2026 09:28:29 -0700 Subject: [PATCH 16/17] Release 1.0a37 Refs #2831, #2832, #2841, #2842, #2843, #2846 --- datasette/version.py | 2 +- docs/changelog.rst | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/datasette/version.py b/datasette/version.py index 387144e9..8e238ab5 100644 --- a/datasette/version.py +++ b/datasette/version.py @@ -1,2 +1,2 @@ -__version__ = "1.0a36" +__version__ = "1.0a37" __version_info__ = tuple(__version__.split(".")) diff --git a/docs/changelog.rst b/docs/changelog.rst index e3718543..1327baa6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,20 @@ Changelog ========= +.. _v1_0_a37: + +1.0a37 (2026-07-14) +------------------- + +Performance improvement for SQL-backed permission checks, plus an improved permission debugging interface. + +- SQL used to resolve permission checks now aggregates permission rules before joining them to resources, improving performance on instances with large schemas. (:issue:`2832`) +- The :ref:`PermissionCheckView` permission debugger now explains why a decision was allowed or denied, including the matching rules. The interactive form can also test a hypothetical actor supplied as JSON, and the :ref:`permissions documentation ` now describes resolution rules in more detail. (:issue:`2841`) +- :ref:`database_execute_write` has a new ``transaction=`` parameter, which can be set to ``False`` for statements such as ``VACUUM`` that cannot run inside a transaction. Write tasks now start their transactions using ``BEGIN IMMEDIATE``, which also ensures that writes are rolled back if the task fails. (:issue:`2831`) +- Refreshing a database's schema in Datasette's internal catalog is now performed as a single atomic operation. (:issue:`2831`) +- Fixed schema introspection, table pages, facets and table counts for tables with names containing a ``]`` character. Thanks, `TowyTowy `__. (:issue:`2431`, :pr:`2846`) +- ``/-/plugins.json`` once again returns a top-level JSON array of plugin objects, reverting the object envelope introduced in 1.0a36. This should fix a large number of trivial test failures in existing plugins. (:issue:`2842`, :pr:`2843`) + .. _v1_0_a36: 1.0a36 (2026-07-07) From 481df7ff6d78a8ccf919984d27f27201b081bd53 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 14 Jul 2026 09:31:28 -0700 Subject: [PATCH 17/17] Shorten link text in changelog --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1327baa6..670166bb 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -13,7 +13,7 @@ Performance improvement for SQL-backed permission checks, plus an improved permi - SQL used to resolve permission checks now aggregates permission rules before joining them to resources, improving performance on instances with large schemas. (:issue:`2832`) - The :ref:`PermissionCheckView` permission debugger now explains why a decision was allowed or denied, including the matching rules. The interactive form can also test a hypothetical actor supplied as JSON, and the :ref:`permissions documentation ` now describes resolution rules in more detail. (:issue:`2841`) -- :ref:`database_execute_write` has a new ``transaction=`` parameter, which can be set to ``False`` for statements such as ``VACUUM`` that cannot run inside a transaction. Write tasks now start their transactions using ``BEGIN IMMEDIATE``, which also ensures that writes are rolled back if the task fails. (:issue:`2831`) +- :ref:`db.execute_write(sql, ..., transaction=True) ` has a new ``transaction=`` parameter, which can be set to ``False`` for statements such as ``VACUUM`` that cannot run inside a transaction. Write tasks now start their transactions using ``BEGIN IMMEDIATE``, which also ensures that writes are rolled back if the task fails. (:issue:`2831`) - Refreshing a database's schema in Datasette's internal catalog is now performed as a single atomic operation. (:issue:`2831`) - Fixed schema introspection, table pages, facets and table counts for tables with names containing a ``]`` character. Thanks, `TowyTowy `__. (:issue:`2431`, :pr:`2846`) - ``/-/plugins.json`` once again returns a top-level JSON array of plugin objects, reverting the object envelope introduced in 1.0a36. This should fix a large number of trivial test failures in existing plugins. (:issue:`2842`, :pr:`2843`)