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/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(\.(?P<format>json))?$",

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)

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

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:
 

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.
 

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)
 

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}

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):

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:

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