diff --git a/datasette/app.py b/datasette/app.py index 409aed23..023568dd 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -283,6 +283,16 @@ FAVICON_PATH = app_root / "datasette" / "static" / "favicon.png" DEFAULT_NOT_SET = object() UNCHANGED = object() +QUERY_OPTION_FIELDS = ( + "hide_sql", + "fragment", + "on_success_message", + "on_success_message_sql", + "on_success_redirect", + "on_error_message", + "on_error_redirect", +) + ResourcesSQL = collections.namedtuple("ResourcesSQL", ("sql", "params")) @@ -1056,6 +1066,7 @@ class Datasette: if row is None: return None parameters = json.loads(row["parameters"] or "[]") + options = json.loads(row["options"] or "{}") is_write = bool(row["is_write"]) return { "database": row["database_name"], @@ -1064,8 +1075,8 @@ class Datasette: "title": row["title"], "description": row["description"], "description_html": row["description_html"], - "hide_sql": bool(row["hide_sql"]), - "fragment": row["fragment"], + "hide_sql": bool(options.get("hide_sql")), + "fragment": options.get("fragment"), "params": parameters, "parameters": parameters, "is_write": is_write, @@ -1073,13 +1084,25 @@ class Datasette: "published": bool(row["published"]), "source": row["source"], "owner_id": row["owner_id"], - "on_success_message": row["on_success_message"], - "on_success_message_sql": row["on_success_message_sql"], - "on_success_redirect": row["on_success_redirect"], - "on_error_message": row["on_error_message"], - "on_error_redirect": row["on_error_redirect"], + "on_success_message": options.get("on_success_message"), + "on_success_message_sql": options.get("on_success_message_sql"), + "on_success_redirect": options.get("on_success_redirect"), + "on_error_message": options.get("on_error_message"), + "on_error_redirect": options.get("on_error_redirect"), } + @staticmethod + def _query_options_json(options): + options_dict = {} + for field in QUERY_OPTION_FIELDS: + value = options.get(field) + if field == "hide_sql": + if value: + options_dict[field] = True + elif value is not None: + options_dict[field] = value + return json.dumps(options_dict, sort_keys=True) + async def add_query( self, database, @@ -1104,13 +1127,22 @@ class Datasette: replace=True, ): parameters_json = json.dumps(list(parameters or [])) + options_json = self._query_options_json( + { + "hide_sql": hide_sql, + "fragment": fragment, + "on_success_message": on_success_message, + "on_success_message_sql": on_success_message_sql, + "on_success_redirect": on_success_redirect, + "on_error_message": on_error_message, + "on_error_redirect": on_error_redirect, + } + ) sql_statement = """ INSERT INTO queries ( database_name, name, sql, title, description, description_html, - hide_sql, fragment, parameters, is_write, published, source, - owner_id, on_success_message, on_success_message_sql, - on_success_redirect, on_error_message, on_error_redirect - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + options, parameters, is_write, published, source, owner_id + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """ if replace: sql_statement += """ @@ -1119,18 +1151,12 @@ class Datasette: title = excluded.title, description = excluded.description, description_html = excluded.description_html, - hide_sql = excluded.hide_sql, - fragment = excluded.fragment, + options = excluded.options, parameters = excluded.parameters, is_write = excluded.is_write, published = excluded.published, source = excluded.source, owner_id = excluded.owner_id, - on_success_message = excluded.on_success_message, - on_success_message_sql = excluded.on_success_message_sql, - on_success_redirect = excluded.on_success_redirect, - on_error_message = excluded.on_error_message, - on_error_redirect = excluded.on_error_redirect, updated_at = CURRENT_TIMESTAMP """ await self.get_internal_database().execute_write( @@ -1142,18 +1168,12 @@ class Datasette: title, description, description_html, - int(bool(hide_sql)), - fragment, + options_json, parameters_json, int(bool(is_write)), int(bool(published)), source, owner_id, - on_success_message, - on_success_message_sql, - on_success_redirect, - on_error_message, - on_error_redirect, ], ) @@ -1184,13 +1204,15 @@ class Datasette: "title": title, "description": description, "description_html": description_html, - "hide_sql": hide_sql, - "fragment": fragment, "parameters": parameters, "is_write": is_write, "published": published, "source": source, "owner_id": owner_id, + } + option_fields = { + "hide_sql": hide_sql, + "fragment": fragment, "on_success_message": on_success_message, "on_success_message_sql": on_success_message_sql, "on_success_redirect": on_success_redirect, @@ -1202,12 +1224,39 @@ class Datasette: for field, value in fields.items(): if value is UNCHANGED: continue - if field in {"hide_sql", "is_write", "published"}: + if field in {"is_write", "published"}: value = int(bool(value)) elif field == "parameters": value = json.dumps(list(value or [])) updates.append(f"{field} = ?") params.append(value) + changed_options = { + field: value + for field, value in option_fields.items() + if value is not UNCHANGED + } + if changed_options: + rows = await self.get_internal_database().execute( + """ + SELECT options FROM queries + WHERE database_name = ? AND name = ? + """, + [database, name], + ) + row = rows.first() + options = json.loads(row["options"] or "{}") if row is not None else {} + for field, value in changed_options.items(): + if field == "hide_sql": + if value: + options[field] = True + else: + options.pop(field, None) + elif value is None: + options.pop(field, None) + else: + options[field] = value + updates.append("options = ?") + params.append(json.dumps(options, sort_keys=True)) if not updates: return updates.append("updated_at = CURRENT_TIMESTAMP") diff --git a/datasette/utils/internal_db.py b/datasette/utils/internal_db.py index 9008c083..854e8784 100644 --- a/datasette/utils/internal_db.py +++ b/datasette/utils/internal_db.py @@ -120,18 +120,12 @@ async def initialize_metadata_tables(db): title TEXT, description TEXT, description_html TEXT, - hide_sql INTEGER NOT NULL DEFAULT 0 CHECK (hide_sql IN (0, 1)), - fragment TEXT, + options TEXT NOT NULL DEFAULT '{}', parameters TEXT NOT NULL DEFAULT '[]', is_write INTEGER NOT NULL DEFAULT 0 CHECK (is_write IN (0, 1)), published INTEGER NOT NULL DEFAULT 0 CHECK (published IN (0, 1)), source TEXT NOT NULL DEFAULT 'user', owner_id TEXT, - on_success_message TEXT, - on_success_message_sql TEXT, - on_success_redirect TEXT, - on_error_message TEXT, - on_error_redirect TEXT, created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (database_name, name), diff --git a/docs/internals.rst b/docs/internals.rst index e0123a7b..a0845ade 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -2148,6 +2148,26 @@ The internal database schema is as follows: config TEXT, PRIMARY KEY (database_name, resource_name, column_name) ); + CREATE TABLE queries ( + database_name TEXT NOT NULL, + name TEXT NOT NULL, + sql TEXT NOT NULL, + title TEXT, + description TEXT, + description_html TEXT, + options TEXT NOT NULL DEFAULT '{}', + parameters TEXT NOT NULL DEFAULT '[]', + is_write INTEGER NOT NULL DEFAULT 0 CHECK (is_write IN (0, 1)), + published INTEGER NOT NULL DEFAULT 0 CHECK (published IN (0, 1)), + source TEXT NOT NULL DEFAULT 'user', + owner_id TEXT, + created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (database_name, name), + CHECK (is_write = 0 OR published = 0) + ); + CREATE INDEX queries_owner_idx + ON queries(owner_id); .. [[[end]]] diff --git a/queries-plan.md b/queries-plan.md index 283ca866..dbc46101 100644 --- a/queries-plan.md +++ b/queries-plan.md @@ -42,18 +42,12 @@ CREATE TABLE IF NOT EXISTS queries ( title TEXT, description TEXT, description_html TEXT, - hide_sql INTEGER NOT NULL DEFAULT 0 CHECK (hide_sql IN (0, 1)), - fragment TEXT, + options TEXT NOT NULL DEFAULT '{}', parameters TEXT NOT NULL DEFAULT '[]', is_write INTEGER NOT NULL DEFAULT 0 CHECK (is_write IN (0, 1)), published INTEGER NOT NULL DEFAULT 0 CHECK (published IN (0, 1)), source TEXT NOT NULL DEFAULT 'user', owner_id TEXT, - on_success_message TEXT, - on_success_message_sql TEXT, - on_success_redirect TEXT, - on_error_message TEXT, - on_error_redirect TEXT, created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (database_name, name), @@ -67,9 +61,10 @@ CREATE INDEX IF NOT EXISTS queries_owner_idx Column notes: - `database_name`, `name`, and `sql` are the routing and execution core. -- Display fields become columns: `title`, `description`, `description_html`, `hide_sql`, and `fragment`. +- Display fields become columns: `title`, `description`, and `description_html`. +- Less common presentation and writable-query behavior lives in `options`, stored as a JSON object. That covers `hide_sql`, `fragment`, `on_success_message`, `on_success_message_sql`, `on_success_redirect`, `on_error_message`, and `on_error_redirect`. - `parameters` is a JSON array of parameter names, stored as text. This preserves explicit parameter order, but does not support labels or default values. -- Existing writable query behavior gets columns too: `is_write`, success/error messages, success/error redirects, and `on_success_message_sql`. +- Existing writable query behavior gets `is_write` as a column. Success/error messages, success/error redirects, and `on_success_message_sql` are stored in `options`. - `published` only applies to read-only queries. A writable query can still be public through explicit `view-query` permissions, but the "publish for users without execute-sql" shortcut should be read-only. - `source` distinguishes `user`, `config`, and `plugin` rows. - `owner_id` is the actor id for user-created rows. It is `NULL` for config/plugin rows. @@ -372,11 +367,11 @@ await datasette.update_query( ) ``` -That call should set `on_success_redirect` to SQL `NULL`; omitting `on_success_redirect` should leave the existing value unchanged. +For column-backed fields, `None` should write SQL `NULL`. For option fields, `None` should remove that key from the JSON object so `get_query()` returns `None`; omitting the field should leave the existing option unchanged. Implementation detail: build the `UPDATE` statement dynamically from fields whose value is not `UNCHANGED`, validate non-nullable fields before writing, and update `updated_at` whenever at least one field changes. -The read methods should reconstruct the existing dictionary shape used by query execution and templates, with `name`, `sql`, display fields, write fields, `params`, `published`, `owner_id`, and `source`. `parameters` should be returned as the decoded JSON array and exposed as `params` where existing query execution code expects that key. +The read methods should reconstruct the existing dictionary shape used by query execution and templates, with `name`, `sql`, display fields, write fields, `params`, `published`, `owner_id`, and `source`. `parameters` should be returned as the decoded JSON array and exposed as `params` where existing query execution code expects that key. Option values should be unpacked from the `options` JSON object and returned as the same top-level keys accepted by `add_query()` and `update_query()`. ## Query page save UI @@ -430,7 +425,7 @@ The existing edit-SQL flow from query pages can continue to point back to arbitr - Query update uses `POST /{database}/{query}/-/update` with an `{"update": {...}}` body. - Query delete uses `POST /{database}/{query}/-/delete`. - There are no `PATCH` or HTTP `DELETE` routes for query management. -- `datasette.update_query(..., field=None)` writes `NULL`, while omitted fields are left unchanged. +- `datasette.update_query(..., field=None)` writes `NULL` for column-backed fields and removes JSON keys for option fields, while omitted fields are left unchanged. - Owner gets default `update-query` and `delete-query` for their own user-created rows. - Admin can manage other users' queries with `update-query` and `delete-query`. - User API rejects magic parameters. diff --git a/tests/test_queries.py b/tests/test_queries.py index 1c9175cc..edb9484a 100644 --- a/tests/test_queries.py +++ b/tests/test_queries.py @@ -1,3 +1,5 @@ +import json + import pytest from datasette.app import Datasette @@ -25,18 +27,12 @@ async def test_queries_internal_table_schema(): "title", "description", "description_html", - "hide_sql", - "fragment", + "options", "parameters", "is_write", "published", "source", "owner_id", - "on_success_message", - "on_success_message_sql", - "on_success_redirect", - "on_error_message", - "on_error_redirect", "created_at", "updated_at", ] @@ -62,6 +58,20 @@ async def test_add_get_and_remove_query(): owner_id="alice", ) + options_row = ( + await ds.get_internal_database().execute( + """ + SELECT options FROM queries + WHERE database_name = ? AND name = ? + """, + ["data", "top_customers"], + ) + ).first() + assert json.loads(options_row["options"]) == { + "fragment": "chart", + "hide_sql": True, + } + query = await ds.get_query("data", "top_customers") assert query == { "database": "data", @@ -108,6 +118,17 @@ async def test_update_query_only_updates_provided_fields(): parameters=["one"], ) + options_row = ( + await ds.get_internal_database().execute( + """ + SELECT options FROM queries + WHERE database_name = ? AND name = ? + """, + ["data", "redirect"], + ) + ).first() + assert json.loads(options_row["options"]) == {"on_success_redirect": "/original"} + await ds.update_query( "data", "redirect", @@ -123,6 +144,16 @@ async def test_update_query_only_updates_provided_fields(): assert query["on_success_redirect"] is None assert query["sql"] == "select 1" assert query["published"] is False + options_row = ( + await ds.get_internal_database().execute( + """ + SELECT options FROM queries + WHERE database_name = ? AND name = ? + """, + ["data", "redirect"], + ) + ).first() + assert json.loads(options_row["options"]) == {} @pytest.mark.asyncio