From a4bb8b8a26651818883fa3be7055e8f2caa1ec92 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:20 -0800 Subject: [PATCH 01/19] feat(migrations): add saved queries and sql_snapshots --- migrations/postgres/v6.sql | 67 ++++++++++++++++++++++++++++++ migrations/sqlite/v6.sql | 83 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 migrations/postgres/v6.sql create mode 100644 migrations/sqlite/v6.sql diff --git a/migrations/postgres/v6.sql b/migrations/postgres/v6.sql new file mode 100644 index 0000000..18adc27 --- /dev/null +++ b/migrations/postgres/v6.sql @@ -0,0 +1,67 @@ +-- Saved queries with versioned SQL snapshots. +-- SQL text is stored in a content-addressable sql_snapshots table, +-- referenced by ID from saved_query_versions and query_runs. + +-- 1) Create sql_snapshots table (content-addressable SQL store) +CREATE TABLE sql_snapshots ( + id TEXT PRIMARY KEY, + sql_hash TEXT NOT NULL, + sql_text TEXT NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); +CREATE UNIQUE INDEX ux_sql_snapshots_hash_text ON sql_snapshots (sql_hash, sql_text); +CREATE INDEX idx_sql_snapshots_hash ON sql_snapshots (sql_hash); + +-- 2) Create saved_queries table +-- Note: name is intentionally NOT UNIQUE — duplicate names are allowed. +CREATE TABLE saved_queries ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL, + latest_version INTEGER NOT NULL DEFAULT 1, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +-- 3) Create saved_query_versions table +CREATE TABLE saved_query_versions ( + saved_query_id TEXT NOT NULL REFERENCES saved_queries(id) ON DELETE CASCADE, + version INTEGER NOT NULL, + snapshot_id TEXT NOT NULL REFERENCES sql_snapshots(id), + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + PRIMARY KEY (saved_query_id, version) +); + +-- 4) Backfill sql_snapshots from existing query_runs +INSERT INTO sql_snapshots (id, sql_hash, sql_text, created_at) +SELECT + 'snap' || substr(md5(sql_hash || sql_text || gen_random_uuid()::text), 1, 26), + sql_hash, + sql_text, + MIN(created_at) +FROM query_runs +WHERE sql_text IS NOT NULL +GROUP BY sql_hash, sql_text; + +-- 5) Add snapshot_id to query_runs and populate +ALTER TABLE query_runs ADD COLUMN snapshot_id TEXT; + +UPDATE query_runs qr +SET snapshot_id = s.id +FROM sql_snapshots s +WHERE s.sql_hash = qr.sql_hash AND s.sql_text = qr.sql_text; + +-- Make snapshot_id NOT NULL and add FK +ALTER TABLE query_runs ALTER COLUMN snapshot_id SET NOT NULL; +ALTER TABLE query_runs + ADD CONSTRAINT fk_qr_snapshot FOREIGN KEY (snapshot_id) REFERENCES sql_snapshots(id); + +-- 6) Drop old columns from query_runs +ALTER TABLE query_runs DROP COLUMN sql_text; +ALTER TABLE query_runs DROP COLUMN sql_hash; +DROP INDEX IF EXISTS idx_query_runs_sql_hash_created; + +-- 7) Add new indexes +ALTER TABLE query_runs ADD COLUMN saved_query_id TEXT; +ALTER TABLE query_runs ADD COLUMN saved_query_version INTEGER; +CREATE INDEX idx_query_runs_saved_query_id ON query_runs(saved_query_id); +CREATE INDEX idx_query_runs_snapshot_id ON query_runs(snapshot_id); diff --git a/migrations/sqlite/v6.sql b/migrations/sqlite/v6.sql new file mode 100644 index 0000000..60d4493 --- /dev/null +++ b/migrations/sqlite/v6.sql @@ -0,0 +1,83 @@ +-- Saved queries with versioned SQL snapshots. +-- SQL text is stored in a content-addressable sql_snapshots table, +-- referenced by ID from saved_query_versions and query_runs. + +-- 1) Create sql_snapshots table (content-addressable SQL store) +CREATE TABLE sql_snapshots ( + id TEXT PRIMARY KEY, + sql_hash TEXT NOT NULL, + sql_text TEXT NOT NULL, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP +); +CREATE UNIQUE INDEX ux_sql_snapshots_hash_text ON sql_snapshots (sql_hash, sql_text); +CREATE INDEX idx_sql_snapshots_hash ON sql_snapshots (sql_hash); + +-- 2) Create saved_queries table +-- Note: name is intentionally NOT UNIQUE — duplicate names are allowed. +CREATE TABLE saved_queries ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL, + latest_version INTEGER NOT NULL DEFAULT 1, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +-- 3) Create saved_query_versions table +CREATE TABLE saved_query_versions ( + saved_query_id TEXT NOT NULL REFERENCES saved_queries(id), + version INTEGER NOT NULL, + snapshot_id TEXT NOT NULL REFERENCES sql_snapshots(id), + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (saved_query_id, version) +); + +-- 4) Backfill sql_snapshots from existing query_runs +INSERT INTO sql_snapshots (id, sql_hash, sql_text, created_at) +SELECT + 'snap' || lower(hex(randomblob(13))), + sql_hash, + sql_text, + MIN(created_at) +FROM query_runs +WHERE sql_text IS NOT NULL +GROUP BY sql_hash, sql_text; + +-- 5) Recreate query_runs with snapshot_id, without sql_text/sql_hash +CREATE TABLE query_runs_new ( + id TEXT PRIMARY KEY, + snapshot_id TEXT NOT NULL REFERENCES sql_snapshots(id), + trace_id TEXT, + status TEXT NOT NULL, + result_id TEXT, + error_message TEXT, + warning_message TEXT, + row_count BIGINT, + execution_time_ms BIGINT, + saved_query_id TEXT, + saved_query_version INTEGER, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + completed_at TIMESTAMP +); + +INSERT INTO query_runs_new ( + id, snapshot_id, trace_id, status, result_id, + error_message, warning_message, row_count, execution_time_ms, + saved_query_id, saved_query_version, created_at, completed_at +) +SELECT + qr.id, + s.id, + qr.trace_id, qr.status, qr.result_id, + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, + NULL, NULL, qr.created_at, qr.completed_at +FROM query_runs qr +JOIN sql_snapshots s ON s.sql_hash = qr.sql_hash AND s.sql_text = qr.sql_text; + +DROP TABLE query_runs; +ALTER TABLE query_runs_new RENAME TO query_runs; + +-- Recreate indexes on query_runs +CREATE INDEX idx_query_runs_created_id ON query_runs(created_at DESC, id DESC); +CREATE INDEX idx_query_runs_trace_id ON query_runs(trace_id); +CREATE INDEX idx_query_runs_saved_query_id ON query_runs(saved_query_id); +CREATE INDEX idx_query_runs_snapshot_id ON query_runs(snapshot_id); From 058d684812eb08607fe7058bff7d72daacbe1b8e Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:23 -0800 Subject: [PATCH 02/19] feat(catalog): add SqlSnapshot entity and saved query types --- src/catalog/manager.rs | 288 ++++++++++++++++++++++++++++++++++++++++- src/catalog/mod.rs | 7 +- src/id.rs | 34 +++++ 3 files changed, 324 insertions(+), 5 deletions(-) diff --git a/src/catalog/manager.rs b/src/catalog/manager.rs index 01c1631..02a43c1 100644 --- a/src/catalog/manager.rs +++ b/src/catalog/manager.rs @@ -3,9 +3,17 @@ use anyhow::Result; use async_trait::async_trait; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; use sqlx::FromRow; use std::fmt::Debug; +/// Compute SHA-256 hex hash of SQL text. +pub fn sql_hash(sql: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(sql.as_bytes()); + format!("{:x}", hasher.finalize()) +} + /// Status of a persisted query result. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] @@ -188,8 +196,12 @@ impl std::fmt::Display for QueryRunStatus { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct QueryRun { pub id: String, + /// SQL text, resolved from sql_snapshots via JOIN. pub sql_text: String, + /// SHA-256 hash of the SQL text, resolved from sql_snapshots via JOIN. pub sql_hash: String, + /// The sql_snapshot that contains the SQL text for this run. + pub snapshot_id: String, pub trace_id: Option, pub status: QueryRunStatus, pub result_id: Option, @@ -197,16 +209,20 @@ pub struct QueryRun { pub warning_message: Option, pub row_count: Option, pub execution_time_ms: Option, + pub saved_query_id: Option, + pub saved_query_version: Option, pub created_at: DateTime, pub completed_at: Option>, } /// Raw database row for query runs (used by sqlx in SQLite where timestamps are strings). +/// sql_text is resolved from sql_snapshots via JOIN. #[derive(Debug, Clone, FromRow)] pub struct QueryRunRow { pub id: String, pub sql_text: String, pub sql_hash: String, + pub snapshot_id: String, pub trace_id: Option, pub status: String, pub result_id: Option, @@ -214,6 +230,8 @@ pub struct QueryRunRow { pub warning_message: Option, pub row_count: Option, pub execution_time_ms: Option, + pub saved_query_id: Option, + pub saved_query_version: Option, pub created_at: String, pub completed_at: Option, } @@ -245,6 +263,7 @@ impl QueryRunRow { id: self.id, sql_text: self.sql_text, sql_hash: self.sql_hash, + snapshot_id: self.snapshot_id, trace_id: self.trace_id, status: QueryRunStatus::parse(&self.status), result_id: self.result_id, @@ -252,6 +271,8 @@ impl QueryRunRow { warning_message: self.warning_message, row_count: self.row_count, execution_time_ms: self.execution_time_ms, + saved_query_id: self.saved_query_id, + saved_query_version: self.saved_query_version, created_at, completed_at, } @@ -259,11 +280,13 @@ impl QueryRunRow { } /// Raw database row for query runs in Postgres (native timestamps). +/// sql_text is resolved from sql_snapshots via JOIN. #[derive(Debug, Clone, FromRow)] pub struct QueryRunRowPg { pub id: String, pub sql_text: String, pub sql_hash: String, + pub snapshot_id: String, pub trace_id: Option, pub status: String, pub result_id: Option, @@ -271,6 +294,8 @@ pub struct QueryRunRowPg { pub warning_message: Option, pub row_count: Option, pub execution_time_ms: Option, + pub saved_query_id: Option, + pub saved_query_version: Option, pub created_at: DateTime, pub completed_at: Option>, } @@ -281,6 +306,7 @@ impl From for QueryRun { id: row.id, sql_text: row.sql_text, sql_hash: row.sql_hash, + snapshot_id: row.snapshot_id, trace_id: row.trace_id, status: QueryRunStatus::parse(&row.status), result_id: row.result_id, @@ -288,6 +314,8 @@ impl From for QueryRun { warning_message: row.warning_message, row_count: row.row_count, execution_time_ms: row.execution_time_ms, + saved_query_id: row.saved_query_id, + saved_query_version: row.saved_query_version, created_at: row.created_at, completed_at: row.completed_at, } @@ -297,9 +325,10 @@ impl From for QueryRun { /// Parameters for creating a new query run. pub struct CreateQueryRun<'a> { pub id: &'a str, - pub sql_text: &'a str, - pub sql_hash: &'a str, + pub snapshot_id: &'a str, pub trace_id: Option<&'a str>, + pub saved_query_id: Option<&'a str>, + pub saved_query_version: Option, } /// Parameters for completing a query run. @@ -336,6 +365,208 @@ pub struct UploadInfo { pub consumed_at: Option>, } +/// A saved query with versioned SQL snapshots. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SavedQuery { + pub id: String, + pub name: String, + pub latest_version: i32, + pub created_at: DateTime, + pub updated_at: DateTime, +} + +/// Raw database row for saved queries in SQLite (timestamps as strings). +#[derive(Debug, Clone, FromRow)] +pub struct SavedQueryRow { + pub id: String, + pub name: String, + pub latest_version: i32, + pub created_at: String, + pub updated_at: String, +} + +impl SavedQueryRow { + pub fn into_saved_query(self) -> SavedQuery { + let created_at = self.created_at.parse().unwrap_or_else(|e| { + tracing::warn!( + id = %self.id, + raw_timestamp = %self.created_at, + error = %e, + "Failed to parse saved query created_at, falling back to now" + ); + Utc::now() + }); + let updated_at = self.updated_at.parse().unwrap_or_else(|e| { + tracing::warn!( + id = %self.id, + raw_timestamp = %self.updated_at, + error = %e, + "Failed to parse saved query updated_at, falling back to now" + ); + Utc::now() + }); + SavedQuery { + id: self.id, + name: self.name, + latest_version: self.latest_version, + created_at, + updated_at, + } + } +} + +/// Raw database row for saved queries in Postgres (native timestamps). +#[derive(Debug, Clone, FromRow)] +pub struct SavedQueryRowPg { + pub id: String, + pub name: String, + pub latest_version: i32, + pub created_at: DateTime, + pub updated_at: DateTime, +} + +impl From for SavedQuery { + fn from(row: SavedQueryRowPg) -> Self { + Self { + id: row.id, + name: row.name, + latest_version: row.latest_version, + created_at: row.created_at, + updated_at: row.updated_at, + } + } +} + +/// A content-addressable SQL snapshot. Deduplicated by (sql_hash, sql_text). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SqlSnapshot { + pub id: String, + pub sql_hash: String, + pub sql_text: String, + pub created_at: DateTime, +} + +/// Raw database row for SQL snapshots in SQLite (timestamps as strings). +#[derive(Debug, Clone, FromRow)] +pub struct SqlSnapshotRow { + pub id: String, + pub sql_hash: String, + pub sql_text: String, + pub created_at: String, +} + +impl SqlSnapshotRow { + pub fn into_sql_snapshot(self) -> SqlSnapshot { + let created_at = self.created_at.parse().unwrap_or_else(|e| { + tracing::warn!( + id = %self.id, + raw_timestamp = %self.created_at, + error = %e, + "Failed to parse sql snapshot created_at, falling back to now" + ); + Utc::now() + }); + SqlSnapshot { + id: self.id, + sql_hash: self.sql_hash, + sql_text: self.sql_text, + created_at, + } + } +} + +/// Raw database row for SQL snapshots in Postgres (native timestamps). +#[derive(Debug, Clone, FromRow)] +pub struct SqlSnapshotRowPg { + pub id: String, + pub sql_hash: String, + pub sql_text: String, + pub created_at: DateTime, +} + +impl From for SqlSnapshot { + fn from(row: SqlSnapshotRowPg) -> Self { + Self { + id: row.id, + sql_hash: row.sql_hash, + sql_text: row.sql_text, + created_at: row.created_at, + } + } +} + +/// An immutable SQL version under a saved query. +/// The sql_text and sql_hash fields are resolved via JOIN to sql_snapshots at query time. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SavedQueryVersion { + pub saved_query_id: String, + pub version: i32, + pub snapshot_id: String, + pub sql_text: String, + pub sql_hash: String, + pub created_at: DateTime, +} + +/// Raw database row for saved query versions in SQLite (timestamps as strings). +/// Fields sql_text and sql_hash come from JOIN to sql_snapshots. +#[derive(Debug, Clone, FromRow)] +pub struct SavedQueryVersionRow { + pub saved_query_id: String, + pub version: i32, + pub snapshot_id: String, + pub sql_text: String, + pub sql_hash: String, + pub created_at: String, +} + +impl SavedQueryVersionRow { + pub fn into_saved_query_version(self) -> SavedQueryVersion { + let created_at = self.created_at.parse().unwrap_or_else(|e| { + tracing::warn!( + saved_query_id = %self.saved_query_id, + version = %self.version, + raw_timestamp = %self.created_at, + error = %e, + "Failed to parse saved query version created_at, falling back to now" + ); + Utc::now() + }); + SavedQueryVersion { + saved_query_id: self.saved_query_id, + version: self.version, + snapshot_id: self.snapshot_id, + sql_text: self.sql_text, + sql_hash: self.sql_hash, + created_at, + } + } +} + +/// Raw database row for saved query versions in Postgres (native timestamps). +/// Fields sql_text and sql_hash come from JOIN to sql_snapshots. +#[derive(Debug, Clone, FromRow)] +pub struct SavedQueryVersionRowPg { + pub saved_query_id: String, + pub version: i32, + pub snapshot_id: String, + pub sql_text: String, + pub sql_hash: String, + pub created_at: DateTime, +} + +impl From for SavedQueryVersion { + fn from(row: SavedQueryVersionRowPg) -> Self { + Self { + saved_query_id: row.saved_query_id, + version: row.version, + snapshot_id: row.snapshot_id, + sql_text: row.sql_text, + sql_hash: row.sql_hash, + created_at: row.created_at, + } + } +} + /// A user-curated dataset. #[derive(Debug, Clone, Serialize, Deserialize, FromRow)] pub struct DatasetInfo { @@ -550,6 +781,59 @@ pub trait CatalogManager: Debug + Send + Sync { /// after claiming but before consuming. Returns true if the upload was processing. async fn release_upload(&self, id: &str) -> Result; + // SQL snapshot methods + + /// Get or create a content-addressable SQL snapshot. + /// Deduplicates by (sql_hash, sql_text). Returns the existing or newly created snapshot. + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result; + + // Saved query methods + + /// Create a new saved query with its first version. + /// The snapshot_id should come from get_or_create_snapshot. + async fn create_saved_query(&self, name: &str, snapshot_id: &str) -> Result; + + /// Get a saved query by ID. + async fn get_saved_query(&self, id: &str) -> Result>; + + /// List saved queries with offset pagination. + /// Returns (queries, has_more). + async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)>; + + /// Create a new version for a saved query, optionally updating its name. + /// The snapshot_id should come from get_or_create_snapshot. + /// Returns the updated saved query, or None if not found. + async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + snapshot_id: &str, + ) -> Result>; + + /// Delete a saved query by ID. Returns true if it existed. + /// Versions are hard-deleted (explicitly for SQLite, via CASCADE for Postgres). + /// Query runs retain history via their snapshot_id FK to sql_snapshots. + async fn delete_saved_query(&self, id: &str) -> Result; + + /// Get a specific version of a saved query. + /// sql_text and sql_hash are resolved via JOIN to sql_snapshots. + async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result>; + + /// List all versions of a saved query, ordered by version ascending. + /// sql_text and sql_hash are resolved via JOIN to sql_snapshots. + async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result>; + // Dataset management methods /// Create a new dataset record. diff --git a/src/catalog/mod.rs b/src/catalog/mod.rs index e0b041b..3599f4b 100644 --- a/src/catalog/mod.rs +++ b/src/catalog/mod.rs @@ -14,9 +14,10 @@ mod mock_catalog; pub use mock_catalog::MockCatalog; pub use manager::{ - CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, - QueryResult, QueryResultRow, QueryRun, QueryRunCursor, QueryRunRow, QueryRunRowPg, - QueryRunStatus, QueryRunUpdate, ResultStatus, ResultUpdate, TableInfo, UploadInfo, + sql_hash, CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, + PendingDeletion, QueryResult, QueryResultRow, QueryRun, QueryRunCursor, QueryRunRow, + QueryRunRowPg, QueryRunStatus, QueryRunUpdate, ResultStatus, ResultUpdate, SavedQuery, + SavedQueryVersion, SqlSnapshot, TableInfo, UploadInfo, }; pub use postgres_manager::PostgresCatalogManager; pub use sqlite_manager::SqliteCatalogManager; diff --git a/src/id.rs b/src/id.rs index 7458abc..e5ffc88 100644 --- a/src/id.rs +++ b/src/id.rs @@ -121,6 +121,8 @@ define_resource_ids! { Upload => "upld", Dataset => "data", QueryRun => "qrun", + SavedQuery => "svqr", + SqlSnapshot => "snap", } /// Generate a 30-char ID: 4-char prefix + 26-char nanoid (lowercase alphanumeric). @@ -159,6 +161,16 @@ pub fn generate_query_run_id() -> String { generate_id(ResourceId::QueryRun) } +/// Generate a saved query ID (prefix: "svqr"). +pub fn generate_saved_query_id() -> String { + generate_id(ResourceId::SavedQuery) +} + +/// Generate a SQL snapshot ID (prefix: "snap"). +pub fn generate_snapshot_id() -> String { + generate_id(ResourceId::SqlSnapshot) +} + #[cfg(test)] mod tests { use super::*; @@ -232,6 +244,8 @@ mod tests { assert_eq!(ResourceId::Upload.prefix(), "upld"); assert_eq!(ResourceId::Dataset.prefix(), "data"); assert_eq!(ResourceId::QueryRun.prefix(), "qrun"); + assert_eq!(ResourceId::SavedQuery.prefix(), "svqr"); + assert_eq!(ResourceId::SqlSnapshot.prefix(), "snap"); } #[test] @@ -243,4 +257,24 @@ mod tests { .chars() .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit())); } + + #[test] + fn test_saved_query_id_format() { + let id = generate_saved_query_id(); + assert_eq!(id.len(), 30); + assert!(id.starts_with("svqr")); + assert!(id + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit())); + } + + #[test] + fn test_snapshot_id_format() { + let id = generate_snapshot_id(); + assert_eq!(id.len(), 30); + assert!(id.starts_with("snap")); + assert!(id + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit())); + } } From c85413ef4caffa6cbd26307c606e2b561beca187 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:27 -0800 Subject: [PATCH 03/19] feat(catalog): implement sql_snapshots and saved queries --- src/catalog/caching_manager.rs | 58 +++++- src/catalog/mock_catalog.rs | 58 +++++- src/catalog/postgres_manager.rs | 298 ++++++++++++++++++++++++++++-- src/catalog/sqlite_manager.rs | 315 ++++++++++++++++++++++++++++++-- 4 files changed, 691 insertions(+), 38 deletions(-) diff --git a/src/catalog/caching_manager.rs b/src/catalog/caching_manager.rs index 3506308..4c9503a 100644 --- a/src/catalog/caching_manager.rs +++ b/src/catalog/caching_manager.rs @@ -6,8 +6,8 @@ use super::{ CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, - QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, TableInfo, - UploadInfo, + QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, SavedQuery, + SavedQueryVersion, SqlSnapshot, TableInfo, UploadInfo, }; use crate::config::CacheConfig; use crate::secrets::{SecretMetadata, SecretStatus}; @@ -1124,6 +1124,60 @@ impl CatalogManager for CachingCatalogManager { self.inner().release_upload(id).await } + // SQL snapshot methods (pass-through, no caching) + + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { + self.inner().get_or_create_snapshot(sql_text).await + } + + // Saved query methods (pass-through, no caching) + + async fn create_saved_query(&self, name: &str, snapshot_id: &str) -> Result { + self.inner().create_saved_query(name, snapshot_id).await + } + + async fn get_saved_query(&self, id: &str) -> Result> { + self.inner().get_saved_query(id).await + } + + async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.inner().list_saved_queries(limit, offset).await + } + + async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + snapshot_id: &str, + ) -> Result> { + self.inner().update_saved_query(id, name, snapshot_id).await + } + + async fn delete_saved_query(&self, id: &str) -> Result { + self.inner().delete_saved_query(id).await + } + + async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result> { + self.inner() + .get_saved_query_version(saved_query_id, version) + .await + } + + async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result> { + self.inner().list_saved_query_versions(saved_query_id).await + } + // Dataset management methods (with cache invalidation for list_dataset_table_names) #[tracing::instrument(name = "catalog.create_dataset", skip(self, dataset), fields(dataset_id = %dataset.id))] diff --git a/src/catalog/mock_catalog.rs b/src/catalog/mock_catalog.rs index 6bc8d56..b07c4af 100644 --- a/src/catalog/mock_catalog.rs +++ b/src/catalog/mock_catalog.rs @@ -5,8 +5,8 @@ use super::{ CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, - QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, TableInfo, - UploadInfo, + QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, SavedQuery, + SavedQueryVersion, SqlSnapshot, TableInfo, UploadInfo, }; use crate::secrets::{SecretMetadata, SecretStatus}; use anyhow::Result; @@ -339,6 +339,60 @@ impl CatalogManager for MockCatalog { Ok(false) } + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { + let hash = crate::catalog::manager::sql_hash(sql_text); + Ok(SqlSnapshot { + id: crate::id::generate_snapshot_id(), + sql_hash: hash, + sql_text: sql_text.to_string(), + created_at: chrono::Utc::now(), + }) + } + + async fn create_saved_query(&self, _name: &str, _snapshot_id: &str) -> Result { + Err(anyhow::anyhow!("Not implemented in mock")) + } + + async fn get_saved_query(&self, _id: &str) -> Result> { + Ok(None) + } + + async fn list_saved_queries( + &self, + _limit: usize, + _offset: usize, + ) -> Result<(Vec, bool)> { + Ok((vec![], false)) + } + + async fn update_saved_query( + &self, + _id: &str, + _name: Option<&str>, + _snapshot_id: &str, + ) -> Result> { + Ok(None) + } + + async fn delete_saved_query(&self, _id: &str) -> Result { + Ok(false) + } + + async fn get_saved_query_version( + &self, + _saved_query_id: &str, + _version: i32, + ) -> Result> { + Ok(None) + } + + async fn list_saved_query_versions( + &self, + _saved_query_id: &str, + ) -> Result> { + Ok(vec![]) + } + async fn create_dataset(&self, _dataset: &DatasetInfo) -> Result<()> { Ok(()) } diff --git a/src/catalog/postgres_manager.rs b/src/catalog/postgres_manager.rs index aa03398..1603749 100644 --- a/src/catalog/postgres_manager.rs +++ b/src/catalog/postgres_manager.rs @@ -2,7 +2,8 @@ use crate::catalog::backend::CatalogBackend; use crate::catalog::manager::{ CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, QueryResult, QueryResultRow, QueryRun, QueryRunCursor, QueryRunRowPg, QueryRunUpdate, - ResultStatus, ResultUpdate, TableInfo, UploadInfo, + ResultStatus, ResultUpdate, SavedQuery, SavedQueryRowPg, SavedQueryVersion, + SavedQueryVersionRowPg, SqlSnapshot, SqlSnapshotRowPg, TableInfo, UploadInfo, }; use crate::catalog::migrations::{ run_migrations, wrap_migration_sql, CatalogMigrations, Migration, POSTGRES_MIGRATIONS, @@ -637,13 +638,14 @@ impl CatalogManager for PostgresCatalogManager { async fn create_query_run(&self, params: CreateQueryRun<'_>) -> Result { let now = Utc::now(); sqlx::query( - "INSERT INTO query_runs (id, sql_text, sql_hash, trace_id, status, created_at) - VALUES ($1, $2, $3, $4, 'running', $5)", + "INSERT INTO query_runs (id, snapshot_id, trace_id, status, saved_query_id, saved_query_version, created_at) + VALUES ($1, $2, $3, 'running', $4, $5, $6)", ) .bind(params.id) - .bind(params.sql_text) - .bind(params.sql_hash) + .bind(params.snapshot_id) .bind(params.trace_id) + .bind(params.saved_query_id) + .bind(params.saved_query_version) .bind(now) .execute(self.backend.pool()) .await?; @@ -704,11 +706,14 @@ impl CatalogManager for PostgresCatalogManager { let fetch_limit = (limit + 1) as i64; let rows: Vec = if let Some(cursor) = cursor { sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs \ - WHERE (created_at < $1 OR (created_at = $1 AND id < $2)) \ - ORDER BY created_at DESC, id DESC \ + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + WHERE (qr.created_at < $1 OR (qr.created_at = $1 AND qr.id < $2)) \ + ORDER BY qr.created_at DESC, qr.id DESC \ LIMIT $3", ) .bind(cursor.created_at) @@ -718,10 +723,13 @@ impl CatalogManager for PostgresCatalogManager { .await? } else { sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs \ - ORDER BY created_at DESC, id DESC \ + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + ORDER BY qr.created_at DESC, qr.id DESC \ LIMIT $1", ) .bind(fetch_limit) @@ -741,9 +749,13 @@ impl CatalogManager for PostgresCatalogManager { )] async fn get_query_run(&self, id: &str) -> Result> { let row: Option = sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs WHERE id = $1", + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + WHERE qr.id = $1", ) .bind(id) .fetch_optional(self.backend.pool()) @@ -861,6 +873,258 @@ impl CatalogManager for PostgresCatalogManager { Ok(result.rows_affected() > 0) } + // SQL snapshot methods + + #[tracing::instrument( + name = "catalog_get_or_create_snapshot", + skip(self, sql_text), + fields(db = "postgres") + )] + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { + let hash = crate::catalog::manager::sql_hash(sql_text); + let id = crate::id::generate_snapshot_id(); + let now = Utc::now(); + + // Single-statement upsert: ON CONFLICT performs a no-op update so RETURNING + // works on both insert and conflict paths. Race-safe via UNIQUE constraint. + let row: SqlSnapshotRowPg = sqlx::query_as( + "INSERT INTO sql_snapshots (id, sql_hash, sql_text, created_at) \ + VALUES ($1, $2, $3, $4) \ + ON CONFLICT (sql_hash, sql_text) DO UPDATE SET sql_hash = EXCLUDED.sql_hash \ + RETURNING id, sql_hash, sql_text, created_at", + ) + .bind(&id) + .bind(&hash) + .bind(sql_text) + .bind(now) + .fetch_one(self.backend.pool()) + .await?; + + Ok(SqlSnapshot::from(row)) + } + + // Saved query methods + + #[tracing::instrument( + name = "catalog_create_saved_query", + skip(self), + fields(db = "postgres") + )] + async fn create_saved_query(&self, name: &str, snapshot_id: &str) -> Result { + let id = crate::id::generate_saved_query_id(); + let now = Utc::now(); + + let mut tx = self.backend.pool().begin().await?; + + sqlx::query( + "INSERT INTO saved_queries (id, name, latest_version, created_at, updated_at) \ + VALUES ($1, $2, 1, $3, $4)", + ) + .bind(&id) + .bind(name) + .bind(now) + .bind(now) + .execute(&mut *tx) + .await?; + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES ($1, 1, $2, $3)", + ) + .bind(&id) + .bind(snapshot_id) + .bind(now) + .execute(&mut *tx) + .await?; + + let row: SavedQueryRowPg = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = $1", + ) + .bind(&id) + .fetch_one(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(SavedQuery::from(row)) + } + + #[tracing::instrument( + name = "catalog_get_saved_query", + skip(self), + fields(db = "postgres", runtimedb.saved_query_id = %id) + )] + async fn get_saved_query(&self, id: &str) -> Result> { + let row: Option = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = $1", + ) + .bind(id) + .fetch_optional(self.backend.pool()) + .await?; + + Ok(row.map(SavedQuery::from)) + } + + #[tracing::instrument( + name = "catalog_list_saved_queries", + skip(self), + fields(db = "postgres") + )] + async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + let fetch_limit = limit.saturating_add(1) as i64; + let rows: Vec = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries \ + ORDER BY name ASC \ + LIMIT $1 OFFSET $2", + ) + .bind(fetch_limit) + .bind(offset as i64) + .fetch_all(self.backend.pool()) + .await?; + + let has_more = rows.len() > limit; + let queries = rows.into_iter().take(limit).map(SavedQuery::from).collect(); + Ok((queries, has_more)) + } + + #[tracing::instrument( + name = "catalog_update_saved_query", + skip(self), + fields(db = "postgres", runtimedb.saved_query_id = %id) + )] + async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + snapshot_id: &str, + ) -> Result> { + let mut tx = self.backend.pool().begin().await?; + + // FOR UPDATE acquires a row-level lock to prevent concurrent updates + // from reading the same latest_version and producing duplicate versions. + let existing: Option = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = $1 FOR UPDATE", + ) + .bind(id) + .fetch_optional(&mut *tx) + .await?; + + let existing = match existing { + Some(row) => row, + None => return Ok(None), + }; + + let new_version = existing.latest_version + 1; + let now = Utc::now(); + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES ($1, $2, $3, $4)", + ) + .bind(id) + .bind(new_version) + .bind(snapshot_id) + .bind(now) + .execute(&mut *tx) + .await?; + + let effective_name = name.unwrap_or(&existing.name); + sqlx::query( + "UPDATE saved_queries SET name = $1, latest_version = $2, updated_at = $3 WHERE id = $4", + ) + .bind(effective_name) + .bind(new_version) + .bind(now) + .bind(id) + .execute(&mut *tx) + .await?; + + let row: SavedQueryRowPg = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = $1", + ) + .bind(id) + .fetch_one(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(Some(SavedQuery::from(row))) + } + + #[tracing::instrument( + name = "catalog_delete_saved_query", + skip(self), + fields(db = "postgres", runtimedb.saved_query_id = %id) + )] + async fn delete_saved_query(&self, id: &str) -> Result { + // ON DELETE CASCADE on the saved_query_versions FK handles child deletion. + // Query runs retain history via their snapshot_id FK to sql_snapshots. + let result = sqlx::query("DELETE FROM saved_queries WHERE id = $1") + .bind(id) + .execute(self.backend.pool()) + .await?; + + Ok(result.rows_affected() > 0) + } + + #[tracing::instrument( + name = "catalog_get_saved_query_version", + skip(self), + fields(db = "postgres", runtimedb.saved_query_id = %saved_query_id) + )] + async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result> { + let row: Option = sqlx::query_as( + "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ + snap.sql_text, snap.sql_hash, sqv.created_at \ + FROM saved_query_versions sqv \ + JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ + WHERE sqv.saved_query_id = $1 AND sqv.version = $2", + ) + .bind(saved_query_id) + .bind(version) + .fetch_optional(self.backend.pool()) + .await?; + + Ok(row.map(SavedQueryVersion::from)) + } + + #[tracing::instrument( + name = "catalog_list_saved_query_versions", + skip(self), + fields(db = "postgres", runtimedb.saved_query_id = %saved_query_id) + )] + async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result> { + let rows: Vec = sqlx::query_as( + "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ + snap.sql_text, snap.sql_hash, sqv.created_at \ + FROM saved_query_versions sqv \ + JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ + WHERE sqv.saved_query_id = $1 \ + ORDER BY sqv.version ASC", + ) + .bind(saved_query_id) + .fetch_all(self.backend.pool()) + .await?; + + Ok(rows.into_iter().map(SavedQueryVersion::from).collect()) + } + // Dataset management methods #[tracing::instrument( diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index 100acbe..f81cc9f 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -2,7 +2,8 @@ use crate::catalog::backend::CatalogBackend; use crate::catalog::manager::{ CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, QueryResult, QueryResultRow, QueryRun, QueryRunCursor, QueryRunRow, QueryRunUpdate, - ResultStatus, ResultUpdate, TableInfo, UploadInfo, + ResultStatus, ResultUpdate, SavedQuery, SavedQueryRow, SavedQueryVersion, SavedQueryVersionRow, + SqlSnapshot, SqlSnapshotRow, TableInfo, UploadInfo, }; use crate::catalog::migrations::{ run_migrations, wrap_migration_sql, CatalogMigrations, Migration, SQLITE_MIGRATIONS, @@ -673,13 +674,14 @@ impl CatalogManager for SqliteCatalogManager { async fn create_query_run(&self, params: CreateQueryRun<'_>) -> Result { let now = Utc::now().to_rfc3339(); sqlx::query( - "INSERT INTO query_runs (id, sql_text, sql_hash, trace_id, status, created_at) - VALUES (?, ?, ?, ?, 'running', ?)", + "INSERT INTO query_runs (id, snapshot_id, trace_id, status, saved_query_id, saved_query_version, created_at) + VALUES (?, ?, ?, 'running', ?, ?, ?)", ) .bind(params.id) - .bind(params.sql_text) - .bind(params.sql_hash) + .bind(params.snapshot_id) .bind(params.trace_id) + .bind(params.saved_query_id) + .bind(params.saved_query_version) .bind(&now) .execute(self.backend.pool()) .await?; @@ -741,11 +743,14 @@ impl CatalogManager for SqliteCatalogManager { let rows: Vec = if let Some(cursor) = cursor { let cursor_ts = cursor.created_at.to_rfc3339(); sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs \ - WHERE (created_at < ? OR (created_at = ? AND id < ?)) \ - ORDER BY created_at DESC, id DESC \ + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + WHERE (qr.created_at < ? OR (qr.created_at = ? AND qr.id < ?)) \ + ORDER BY qr.created_at DESC, qr.id DESC \ LIMIT ?", ) .bind(&cursor_ts) @@ -756,10 +761,13 @@ impl CatalogManager for SqliteCatalogManager { .await? } else { sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs \ - ORDER BY created_at DESC, id DESC \ + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + ORDER BY qr.created_at DESC, qr.id DESC \ LIMIT ?", ) .bind(fetch_limit) @@ -783,9 +791,13 @@ impl CatalogManager for SqliteCatalogManager { )] async fn get_query_run(&self, id: &str) -> Result> { let row: Option = sqlx::query_as( - "SELECT id, sql_text, sql_hash, trace_id, status, result_id, \ - error_message, warning_message, row_count, execution_time_ms, created_at, completed_at \ - FROM query_runs WHERE id = ?", + "SELECT qr.id, snap.sql_text, snap.sql_hash, qr.snapshot_id, \ + qr.trace_id, qr.status, qr.result_id, \ + qr.error_message, qr.warning_message, qr.row_count, qr.execution_time_ms, \ + qr.saved_query_id, qr.saved_query_version, qr.created_at, qr.completed_at \ + FROM query_runs qr \ + JOIN sql_snapshots snap ON snap.id = qr.snapshot_id \ + WHERE qr.id = ?", ) .bind(id) .fetch_optional(self.backend.pool()) @@ -908,6 +920,275 @@ impl CatalogManager for SqliteCatalogManager { Ok(result.rows_affected() > 0) } + // SQL snapshot methods + + #[tracing::instrument( + name = "catalog_get_or_create_snapshot", + skip(self, sql_text), + fields(db = "sqlite") + )] + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { + let hash = crate::catalog::manager::sql_hash(sql_text); + let id = crate::id::generate_snapshot_id(); + let now = Utc::now().to_rfc3339(); + + // INSERT OR IGNORE for idempotent upsert (race-safe via UNIQUE constraint) + sqlx::query( + "INSERT OR IGNORE INTO sql_snapshots (id, sql_hash, sql_text, created_at) \ + VALUES (?, ?, ?, ?)", + ) + .bind(&id) + .bind(&hash) + .bind(sql_text) + .bind(&now) + .execute(self.backend.pool()) + .await?; + + // Fetch the existing or newly inserted row + let row: SqlSnapshotRow = sqlx::query_as( + "SELECT id, sql_hash, sql_text, created_at FROM sql_snapshots \ + WHERE sql_hash = ? AND sql_text = ?", + ) + .bind(&hash) + .bind(sql_text) + .fetch_one(self.backend.pool()) + .await?; + + Ok(row.into_sql_snapshot()) + } + + // Saved query methods + + #[tracing::instrument(name = "catalog_create_saved_query", skip(self), fields(db = "sqlite"))] + async fn create_saved_query(&self, name: &str, snapshot_id: &str) -> Result { + let id = crate::id::generate_saved_query_id(); + let now = Utc::now().to_rfc3339(); + + let mut tx = self.backend.pool().begin().await?; + + sqlx::query( + "INSERT INTO saved_queries (id, name, latest_version, created_at, updated_at) \ + VALUES (?, ?, 1, ?, ?)", + ) + .bind(&id) + .bind(name) + .bind(&now) + .bind(&now) + .execute(&mut *tx) + .await?; + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES (?, 1, ?, ?)", + ) + .bind(&id) + .bind(snapshot_id) + .bind(&now) + .execute(&mut *tx) + .await?; + + let row: SavedQueryRow = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(&id) + .fetch_one(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(row.into_saved_query()) + } + + #[tracing::instrument( + name = "catalog_get_saved_query", + skip(self), + fields(db = "sqlite", runtimedb.saved_query_id = %id) + )] + async fn get_saved_query(&self, id: &str) -> Result> { + let row: Option = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(id) + .fetch_optional(self.backend.pool()) + .await?; + + Ok(row.map(SavedQueryRow::into_saved_query)) + } + + #[tracing::instrument(name = "catalog_list_saved_queries", skip(self), fields(db = "sqlite"))] + async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + let fetch_limit = limit.saturating_add(1); + let rows: Vec = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries \ + ORDER BY name ASC \ + LIMIT ? OFFSET ?", + ) + .bind(fetch_limit as i64) + .bind(offset as i64) + .fetch_all(self.backend.pool()) + .await?; + + let has_more = rows.len() > limit; + let queries = rows + .into_iter() + .take(limit) + .map(SavedQueryRow::into_saved_query) + .collect(); + Ok((queries, has_more)) + } + + #[tracing::instrument( + name = "catalog_update_saved_query", + skip(self), + fields(db = "sqlite", runtimedb.saved_query_id = %id) + )] + async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + snapshot_id: &str, + ) -> Result> { + // SQLite acquires an exclusive database-level lock on the first write + // within a transaction, so explicit row-level locking (FOR UPDATE) is + // unnecessary — concurrent writers are serialized at the engine level. + let mut tx = self.backend.pool().begin().await?; + + let existing: Option = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(id) + .fetch_optional(&mut *tx) + .await?; + + let existing = match existing { + Some(row) => row, + None => return Ok(None), + }; + + let new_version = existing.latest_version + 1; + let now = Utc::now().to_rfc3339(); + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES (?, ?, ?, ?)", + ) + .bind(id) + .bind(new_version) + .bind(snapshot_id) + .bind(&now) + .execute(&mut *tx) + .await?; + + let effective_name = name.unwrap_or(&existing.name); + sqlx::query( + "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", + ) + .bind(effective_name) + .bind(new_version) + .bind(&now) + .bind(id) + .execute(&mut *tx) + .await?; + + let row: SavedQueryRow = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(id) + .fetch_one(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(Some(row.into_saved_query())) + } + + #[tracing::instrument( + name = "catalog_delete_saved_query", + skip(self), + fields(db = "sqlite", runtimedb.saved_query_id = %id) + )] + async fn delete_saved_query(&self, id: &str) -> Result { + let mut tx = self.backend.pool().begin().await?; + + // Explicitly delete child versions first (SQLite PRAGMA foreign_keys is + // not guaranteed to be ON, so we cannot rely on ON DELETE CASCADE). + // Query runs retain history via their snapshot_id FK to sql_snapshots. + sqlx::query("DELETE FROM saved_query_versions WHERE saved_query_id = ?") + .bind(id) + .execute(&mut *tx) + .await?; + + let result = sqlx::query("DELETE FROM saved_queries WHERE id = ?") + .bind(id) + .execute(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(result.rows_affected() > 0) + } + + #[tracing::instrument( + name = "catalog_get_saved_query_version", + skip(self), + fields(db = "sqlite", runtimedb.saved_query_id = %saved_query_id) + )] + async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result> { + let row: Option = sqlx::query_as( + "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ + snap.sql_text, snap.sql_hash, sqv.created_at \ + FROM saved_query_versions sqv \ + JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ + WHERE sqv.saved_query_id = ? AND sqv.version = ?", + ) + .bind(saved_query_id) + .bind(version) + .fetch_optional(self.backend.pool()) + .await?; + + Ok(row.map(SavedQueryVersionRow::into_saved_query_version)) + } + + #[tracing::instrument( + name = "catalog_list_saved_query_versions", + skip(self), + fields(db = "sqlite", runtimedb.saved_query_id = %saved_query_id) + )] + async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result> { + let rows: Vec = sqlx::query_as( + "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ + snap.sql_text, snap.sql_hash, sqv.created_at \ + FROM saved_query_versions sqv \ + JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ + WHERE sqv.saved_query_id = ? \ + ORDER BY sqv.version ASC", + ) + .bind(saved_query_id) + .fetch_all(self.backend.pool()) + .await?; + + Ok(rows + .into_iter() + .map(SavedQueryVersionRow::into_saved_query_version) + .collect()) + } + // Dataset management methods #[tracing::instrument( From 7aab321a12a368ef1e1fb32ad0d0468f4dfcad3a Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:31 -0800 Subject: [PATCH 04/19] feat(engine): wire snapshot creation into query execution --- src/engine.rs | 127 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 14 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 24bcfc8..319d819 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,7 +1,7 @@ use crate::catalog::{ is_dataset_table_name_conflict, CachingCatalogManager, CatalogManager, ConnectionInfo, CreateQueryRun, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, - SqliteCatalogManager, TableInfo, + SavedQuery, SavedQueryVersion, SqliteCatalogManager, TableInfo, }; use crate::datafetch::native::StreamingParquetWriter; use crate::datafetch::{BatchWriter, FetchOrchestrator, NativeFetcher}; @@ -28,7 +28,6 @@ use instrumented_object_store::instrument_object_store; use liquid_cache_client::LiquidCacheClientBuilder; use object_store::ObjectStore; use opentelemetry::trace::TraceContextExt; -use sha2::{Digest, Sha256}; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -88,6 +87,12 @@ pub struct QueryResponse { #[error("{0}")] pub struct QueryInputError(pub String); +/// Error for a resource that was not found. +/// Maps to HTTP 404 via the `From for ApiError` conversion. +#[derive(Debug, thiserror::Error)] +#[error("{0}")] +pub struct NotFoundError(pub String); + /// Result of a query execution with async persistence and run tracking. /// /// Returned by [`RuntimeEngine::execute_query_with_persistence`]. Contains all @@ -126,13 +131,6 @@ pub struct QueryRunPage { pub next_cursor: Option, } -/// Compute SHA-256 hex hash of SQL text. -pub fn sql_hash(sql: &str) -> String { - let mut hasher = Sha256::new(); - hasher.update(sql.as_bytes()); - format!("{:x}", hasher.finalize()) -} - /// Extract the current span's trace ID if valid, else None. fn current_trace_id() -> Option { let ctx = tracing::Span::current().context(); @@ -613,8 +611,14 @@ impl RuntimeEngine { /// **Note:** If the server crashes between step 1 and step 4, the query run will /// remain in `running` status indefinitely. A periodic cleanup job to mark stale /// `running` records as `failed` should be added in a future iteration. + pub async fn execute_query_with_persistence(&self, sql: &str) -> Result { + self.execute_query_with_persistence_inner(sql, None, None) + .await + } + + /// Inner method that supports optional saved query linkage. #[tracing::instrument( - name = "execute_query_with_persistence", + name = "execute_query_with_persistence_inner", skip(self, sql), fields( runtimedb.query_run_id = tracing::field::Empty, @@ -622,10 +626,15 @@ impl RuntimeEngine { runtimedb.row_count = tracing::field::Empty, ) )] - pub async fn execute_query_with_persistence(&self, sql: &str) -> Result { + async fn execute_query_with_persistence_inner( + &self, + sql: &str, + saved_query_id: Option<&str>, + saved_query_version: Option, + ) -> Result { let start = Instant::now(); let query_run_id = crate::id::generate_query_run_id(); - let hash = sql_hash(sql); + let snapshot = self.catalog.get_or_create_snapshot(sql).await?; let trace_id = current_trace_id(); tracing::Span::current().record("runtimedb.query_run_id", &query_run_id); @@ -634,9 +643,10 @@ impl RuntimeEngine { self.catalog .create_query_run(CreateQueryRun { id: &query_run_id, - sql_text: sql, - sql_hash: &hash, + snapshot_id: &snapshot.id, trace_id: trace_id.as_deref(), + saved_query_id, + saved_query_version, }) .await?; @@ -1001,6 +1011,95 @@ impl RuntimeEngine { }) } + // Saved query methods + + pub async fn create_saved_query(&self, name: &str, sql: &str) -> Result { + let snapshot = self.catalog.get_or_create_snapshot(sql).await?; + self.catalog.create_saved_query(name, &snapshot.id).await + } + + pub async fn get_saved_query(&self, id: &str) -> Result> { + self.catalog.get_saved_query(id).await + } + + pub async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.catalog.list_saved_queries(limit, offset).await + } + + pub async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + sql: &str, + ) -> Result> { + let snapshot = self.catalog.get_or_create_snapshot(sql).await?; + self.catalog + .update_saved_query(id, name, &snapshot.id) + .await + } + + pub async fn delete_saved_query(&self, id: &str) -> Result { + self.catalog.delete_saved_query(id).await + } + + pub async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result> { + self.catalog + .get_saved_query_version(saved_query_id, version) + .await + } + + pub async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result> { + self.catalog.list_saved_query_versions(saved_query_id).await + } + + /// Execute a saved query by ID, optionally pinned to a specific version. + /// Resolves the SQL from the saved query version and creates a query run + /// with saved query linkage. + pub async fn execute_saved_query( + &self, + saved_query_id: &str, + version: Option, + ) -> Result { + // Look up the saved query + let saved_query = self + .catalog + .get_saved_query(saved_query_id) + .await? + .ok_or_else(|| NotFoundError(format!("Saved query '{}' not found", saved_query_id)))?; + + // Resolve the version to execute + let target_version = version.unwrap_or(saved_query.latest_version); + let version_record = self + .catalog + .get_saved_query_version(saved_query_id, target_version) + .await? + .ok_or_else(|| { + NotFoundError(format!( + "Version {} not found for saved query '{}'", + target_version, saved_query_id + )) + })?; + + // Execute with saved query linkage + self.execute_query_with_persistence_inner( + &version_record.sql_text, + Some(saved_query_id), + Some(target_version), + ) + .await + } + /// Purge all cached data for a connection (clears parquet files and resets sync state). #[tracing::instrument( name = "purge_connection", From 2797f961bbb76a7883845e81309421dde95b166b Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:35 -0800 Subject: [PATCH 05/19] feat(http): add saved queries CRUD and execute endpoints --- src/http/app_server.rs | 30 +- src/http/controllers/mod.rs | 5 + src/http/controllers/query_runs_controller.rs | 3 + .../controllers/saved_queries_controller.rs | 301 ++++++++++++++++++ src/http/error.rs | 3 + src/http/models.rs | 96 ++++++ 6 files changed, 432 insertions(+), 6 deletions(-) create mode 100644 src/http/controllers/saved_queries_controller.rs diff --git a/src/http/app_server.rs b/src/http/app_server.rs index a78b49b..7b0813a 100644 --- a/src/http/app_server.rs +++ b/src/http/app_server.rs @@ -1,11 +1,13 @@ use crate::http::controllers::{ - check_connection_health_handler, create_connection_handler, create_dataset, - create_secret_handler, delete_connection_handler, delete_dataset, delete_secret_handler, - get_connection_handler, get_dataset, get_result_handler, get_secret_handler, health_handler, + check_connection_health_handler, create_connection_handler, create_dataset, create_saved_query, + create_secret_handler, delete_connection_handler, delete_dataset, delete_saved_query, + delete_secret_handler, execute_saved_query, get_connection_handler, get_dataset, + get_result_handler, get_saved_query, get_secret_handler, health_handler, information_schema_handler, list_connections_handler, list_datasets, list_query_runs_handler, - list_results_handler, list_secrets_handler, list_uploads, purge_connection_cache_handler, - purge_table_cache_handler, query_handler, refresh_handler, update_dataset, - update_secret_handler, upload_file, MAX_UPLOAD_SIZE, + list_results_handler, list_saved_queries, list_saved_query_versions, list_secrets_handler, + list_uploads, purge_connection_cache_handler, purge_table_cache_handler, query_handler, + refresh_handler, update_dataset, update_saved_query, update_secret_handler, upload_file, + MAX_UPLOAD_SIZE, }; use crate::RuntimeEngine; use axum::extract::DefaultBodyLimit; @@ -92,6 +94,10 @@ pub const PATH_RESULT: &str = "/results/{id}"; pub const PATH_FILES: &str = "/v1/files"; pub const PATH_DATASETS: &str = "/v1/datasets"; pub const PATH_DATASET: &str = "/v1/datasets/{id}"; +pub const PATH_SAVED_QUERIES: &str = "/v1/queries"; +pub const PATH_SAVED_QUERY: &str = "/v1/queries/{id}"; +pub const PATH_SAVED_QUERY_VERSIONS: &str = "/v1/queries/{id}/versions"; +pub const PATH_SAVED_QUERY_EXECUTE: &str = "/v1/queries/{id}/execute"; impl AppServer { pub fn new(engine: RuntimeEngine) -> Self { @@ -141,6 +147,18 @@ impl AppServer { PATH_DATASET, get(get_dataset).put(update_dataset).delete(delete_dataset), ) + .route( + PATH_SAVED_QUERIES, + post(create_saved_query).get(list_saved_queries), + ) + .route( + PATH_SAVED_QUERY, + get(get_saved_query) + .put(update_saved_query) + .delete(delete_saved_query), + ) + .route(PATH_SAVED_QUERY_VERSIONS, get(list_saved_query_versions)) + .route(PATH_SAVED_QUERY_EXECUTE, post(execute_saved_query)) .with_state(engine.clone()) .layer(middleware::from_fn(trace_id_response_header)) .layer( diff --git a/src/http/controllers/mod.rs b/src/http/controllers/mod.rs index a6b58a9..c7d3f16 100644 --- a/src/http/controllers/mod.rs +++ b/src/http/controllers/mod.rs @@ -6,6 +6,7 @@ pub mod query_controller; pub mod query_runs_controller; pub mod refresh_controller; pub mod results_controller; +pub mod saved_queries_controller; pub mod secrets_controller; pub mod uploads_controller; @@ -23,6 +24,10 @@ pub use query_controller::query_handler; pub use query_runs_controller::list_query_runs_handler; pub use refresh_controller::refresh_handler; pub use results_controller::{get_result_handler, list_results_handler}; +pub use saved_queries_controller::{ + create_saved_query, delete_saved_query, execute_saved_query, get_saved_query, + list_saved_queries, list_saved_query_versions, update_saved_query, +}; pub use secrets_controller::{ create_secret_handler, delete_secret_handler, get_secret_handler, list_secrets_handler, update_secret_handler, diff --git a/src/http/controllers/query_runs_controller.rs b/src/http/controllers/query_runs_controller.rs index 76fd93f..c6585e8 100644 --- a/src/http/controllers/query_runs_controller.rs +++ b/src/http/controllers/query_runs_controller.rs @@ -35,12 +35,15 @@ pub async fn list_query_runs_handler( status: r.status.to_string(), sql_text: r.sql_text, sql_hash: r.sql_hash, + snapshot_id: r.snapshot_id, trace_id: r.trace_id, result_id: r.result_id, error_message: r.error_message, warning_message: r.warning_message, row_count: r.row_count, execution_time_ms: r.execution_time_ms, + saved_query_id: r.saved_query_id, + saved_query_version: r.saved_query_version, created_at: r.created_at, completed_at: r.completed_at, }) diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs new file mode 100644 index 0000000..670ced2 --- /dev/null +++ b/src/http/controllers/saved_queries_controller.rs @@ -0,0 +1,301 @@ +use crate::http::controllers::query_controller::serialize_batches; +use crate::http::error::ApiError; +use crate::http::models::{ + CreateSavedQueryRequest, CreateSavedQueryResponse, ExecuteSavedQueryRequest, + ListSavedQueriesParams, ListSavedQueriesResponse, ListSavedQueryVersionsResponse, + QueryResponse, SavedQueryDetail, SavedQuerySummary, SavedQueryVersionInfo, + UpdateSavedQueryRequest, +}; +use crate::RuntimeEngine; +use axum::{ + extract::{Path, Query as QueryParams, State}, + http::StatusCode, + Json, +}; +use std::sync::Arc; + +const DEFAULT_LIMIT: usize = 100; +const MAX_LIMIT: usize = 1000; +const MAX_NAME_LENGTH: usize = 255; +const MAX_SQL_LENGTH: usize = 1_000_000; + +/// Handler for POST /v1/queries +pub async fn create_saved_query( + State(engine): State>, + Json(request): Json, +) -> Result<(StatusCode, Json), ApiError> { + if request.name.len() > MAX_NAME_LENGTH || request.sql.len() > MAX_SQL_LENGTH { + return Err(ApiError::bad_request( + "Request field exceeds maximum length", + )); + } + + let name = request.name.trim().to_string(); + let sql = request.sql.trim().to_string(); + + if name.is_empty() { + return Err(ApiError::bad_request("Saved query name cannot be empty")); + } + if name.len() > MAX_NAME_LENGTH { + return Err(ApiError::bad_request(format!( + "Saved query name exceeds maximum length of {} characters", + MAX_NAME_LENGTH + ))); + } + if sql.is_empty() { + return Err(ApiError::bad_request("SQL cannot be empty")); + } + if sql.len() > MAX_SQL_LENGTH { + return Err(ApiError::bad_request(format!( + "SQL exceeds maximum length of {} characters", + MAX_SQL_LENGTH + ))); + } + + let saved_query = engine + .create_saved_query(&name, &sql) + .await + .map_err(|e| -> ApiError { e.into() })?; + + let version = engine + .get_saved_query_version(&saved_query.id, saved_query.latest_version) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| { + ApiError::internal_error(format!( + "Version {} missing for saved query '{}'", + saved_query.latest_version, saved_query.id + )) + })?; + + Ok(( + StatusCode::CREATED, + Json(CreateSavedQueryResponse { + id: saved_query.id, + name: saved_query.name, + latest_version: saved_query.latest_version, + sql: version.sql_text, + sql_hash: version.sql_hash, + created_at: saved_query.created_at, + updated_at: saved_query.updated_at, + }), + )) +} + +/// Handler for GET /v1/queries +pub async fn list_saved_queries( + State(engine): State>, + QueryParams(params): QueryParams, +) -> Result, ApiError> { + let limit = params.limit.unwrap_or(DEFAULT_LIMIT).clamp(1, MAX_LIMIT); + let offset = params.offset.unwrap_or(0); + + let (queries, has_more) = engine + .list_saved_queries(limit, offset) + .await + .map_err(|e| -> ApiError { e.into() })?; + + let count = queries.len(); + let summaries: Vec = queries + .into_iter() + .map(|q| SavedQuerySummary { + id: q.id, + name: q.name, + latest_version: q.latest_version, + created_at: q.created_at, + updated_at: q.updated_at, + }) + .collect(); + + Ok(Json(ListSavedQueriesResponse { + queries: summaries, + count, + offset, + limit, + has_more, + })) +} + +/// Handler for GET /v1/queries/{id} +pub async fn get_saved_query( + State(engine): State>, + Path(id): Path, +) -> Result, ApiError> { + let saved_query = engine + .get_saved_query(&id) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| ApiError::not_found(format!("Saved query '{}' not found", id)))?; + + let version = engine + .get_saved_query_version(&saved_query.id, saved_query.latest_version) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| { + ApiError::internal_error(format!( + "Latest version {} missing for saved query '{}'", + saved_query.latest_version, id + )) + })?; + + Ok(Json(SavedQueryDetail { + id: saved_query.id, + name: saved_query.name, + latest_version: saved_query.latest_version, + sql: version.sql_text, + sql_hash: version.sql_hash, + created_at: saved_query.created_at, + updated_at: saved_query.updated_at, + })) +} + +/// Handler for PUT /v1/queries/{id} — creates a new version, optionally renames +pub async fn update_saved_query( + State(engine): State>, + Path(id): Path, + Json(request): Json, +) -> Result, ApiError> { + if request.sql.len() > MAX_SQL_LENGTH { + return Err(ApiError::bad_request( + "Request field exceeds maximum length", + )); + } + if let Some(ref name) = request.name { + if name.len() > MAX_NAME_LENGTH { + return Err(ApiError::bad_request( + "Request field exceeds maximum length", + )); + } + } + + let sql = request.sql.trim().to_string(); + if sql.is_empty() { + return Err(ApiError::bad_request("SQL cannot be empty")); + } + if sql.len() > MAX_SQL_LENGTH { + return Err(ApiError::bad_request(format!( + "SQL exceeds maximum length of {} characters", + MAX_SQL_LENGTH + ))); + } + + let trimmed_name = request.name.as_deref().map(str::trim); + if let Some(name) = trimmed_name { + if name.is_empty() { + return Err(ApiError::bad_request("Saved query name cannot be empty")); + } + if name.len() > MAX_NAME_LENGTH { + return Err(ApiError::bad_request(format!( + "Saved query name exceeds maximum length of {} characters", + MAX_NAME_LENGTH + ))); + } + } + + let saved_query = engine + .update_saved_query(&id, trimmed_name, &sql) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| ApiError::not_found(format!("Saved query '{}' not found", id)))?; + + let version = engine + .get_saved_query_version(&saved_query.id, saved_query.latest_version) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| { + ApiError::internal_error(format!( + "Version {} missing for saved query '{}'", + saved_query.latest_version, saved_query.id + )) + })?; + + Ok(Json(SavedQueryDetail { + id: saved_query.id, + name: saved_query.name, + latest_version: saved_query.latest_version, + sql: version.sql_text, + sql_hash: version.sql_hash, + created_at: saved_query.created_at, + updated_at: saved_query.updated_at, + })) +} + +/// Handler for DELETE /v1/queries/{id} +pub async fn delete_saved_query( + State(engine): State>, + Path(id): Path, +) -> Result { + let deleted = engine + .delete_saved_query(&id) + .await + .map_err(|e| -> ApiError { e.into() })?; + + if deleted { + Ok(StatusCode::NO_CONTENT) + } else { + Err(ApiError::not_found(format!( + "Saved query '{}' not found", + id + ))) + } +} + +/// Handler for GET /v1/queries/{id}/versions +pub async fn list_saved_query_versions( + State(engine): State>, + Path(id): Path, +) -> Result, ApiError> { + // Verify saved query exists + engine + .get_saved_query(&id) + .await + .map_err(|e| -> ApiError { e.into() })? + .ok_or_else(|| ApiError::not_found(format!("Saved query '{}' not found", id)))?; + + let versions = engine + .list_saved_query_versions(&id) + .await + .map_err(|e| -> ApiError { e.into() })?; + + let version_infos: Vec = versions + .into_iter() + .map(|v| SavedQueryVersionInfo { + version: v.version, + sql: v.sql_text, + sql_hash: v.sql_hash, + created_at: v.created_at, + }) + .collect(); + + Ok(Json(ListSavedQueryVersionsResponse { + saved_query_id: id, + versions: version_infos, + })) +} + +/// Handler for POST /v1/queries/{id}/execute +pub async fn execute_saved_query( + State(engine): State>, + Path(id): Path, + Json(request): Json, +) -> Result, ApiError> { + // engine.execute_saved_query returns NotFoundError (→ 404) if the saved + // query or requested version does not exist, so no pre-check is needed. + let result = engine + .execute_saved_query(&id, request.version) + .await + .map_err(|e| -> ApiError { e.into() })?; + + let (columns, nullable, rows) = serialize_batches(&result.schema, &result.results)?; + + Ok(Json(QueryResponse { + query_run_id: result.query_run_id, + result_id: result.result_id, + columns, + nullable, + rows, + row_count: result.row_count, + execution_time_ms: result.execution_time_ms, + warning: result.warning, + })) +} diff --git a/src/http/error.rs b/src/http/error.rs index 24276c8..ee7f269 100644 --- a/src/http/error.rs +++ b/src/http/error.rs @@ -103,6 +103,9 @@ impl From for ApiError { { return ApiError::bad_request(err.to_string()); } + if err.downcast_ref::().is_some() { + return ApiError::not_found(err.to_string()); + } // Default to internal server error for unknown errors ApiError::internal_error(err.to_string()) } diff --git a/src/http/models.rs b/src/http/models.rs index 562bbed..f01920e 100644 --- a/src/http/models.rs +++ b/src/http/models.rs @@ -95,6 +95,7 @@ pub struct QueryRunInfo { pub status: String, pub sql_text: String, pub sql_hash: String, + pub snapshot_id: String, #[serde(skip_serializing_if = "Option::is_none")] pub trace_id: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -107,6 +108,10 @@ pub struct QueryRunInfo { pub row_count: Option, #[serde(skip_serializing_if = "Option::is_none")] pub execution_time_ms: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub saved_query_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub saved_query_version: Option, pub created_at: DateTime, #[serde(skip_serializing_if = "Option::is_none")] pub completed_at: Option>, @@ -545,6 +550,97 @@ pub struct UpdateDatasetResponse { pub updated_at: DateTime, } +// Saved query models + +/// Request body for POST /v1/queries +#[derive(Debug, Deserialize)] +pub struct CreateSavedQueryRequest { + pub name: String, + pub sql: String, +} + +/// Request body for PUT /v1/queries/{id} +#[derive(Debug, Deserialize)] +pub struct UpdateSavedQueryRequest { + /// Optional new name. When omitted the existing name is preserved. + #[serde(default)] + pub name: Option, + pub sql: String, +} + +/// Request body for POST /v1/queries/{id}/execute +#[derive(Debug, Deserialize)] +pub struct ExecuteSavedQueryRequest { + pub version: Option, +} + +/// Query params for GET /v1/queries +#[derive(Debug, Deserialize)] +pub struct ListSavedQueriesParams { + pub limit: Option, + pub offset: Option, +} + +/// Saved query summary for listing +#[derive(Debug, Serialize)] +pub struct SavedQuerySummary { + pub id: String, + pub name: String, + pub latest_version: i32, + pub created_at: DateTime, + pub updated_at: DateTime, +} + +/// Saved query detail (includes latest version's SQL) +#[derive(Debug, Serialize)] +pub struct SavedQueryDetail { + pub id: String, + pub name: String, + pub latest_version: i32, + pub sql: String, + pub sql_hash: String, + pub created_at: DateTime, + pub updated_at: DateTime, +} + +/// Response body for POST /v1/queries +#[derive(Debug, Serialize)] +pub struct CreateSavedQueryResponse { + pub id: String, + pub name: String, + pub latest_version: i32, + pub sql: String, + pub sql_hash: String, + pub created_at: DateTime, + pub updated_at: DateTime, +} + +/// Response body for GET /v1/queries +#[derive(Debug, Serialize)] +pub struct ListSavedQueriesResponse { + pub queries: Vec, + pub count: usize, + pub offset: usize, + pub limit: usize, + pub has_more: bool, +} + +/// Single saved query version +#[derive(Debug, Serialize)] +pub struct SavedQueryVersionInfo { + pub version: i32, + pub sql: String, + pub sql_hash: String, + pub created_at: DateTime, +} + +/// Response body for GET /v1/queries/{id}/versions +#[derive(Debug, Serialize)] +pub struct ListSavedQueryVersionsResponse { + pub saved_query_id: String, + pub versions: Vec, +} + #[cfg(test)] mod tests { use super::*; From 744f73796b3a2d8547b182226aacfc85d5916aff Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:20:39 -0800 Subject: [PATCH 06/19] test: add saved queries and sql_snapshots test coverage --- tests/caching_catalog_tests.rs | 56 +-- tests/catalog_manager_suite.rs | 358 +++++++++++++++++- tests/result_persistence_tests.rs | 58 ++- tests/saved_query_tests.rs | 577 ++++++++++++++++++++++++++++++ 4 files changed, 1009 insertions(+), 40 deletions(-) create mode 100644 tests/saved_query_tests.rs diff --git a/tests/caching_catalog_tests.rs b/tests/caching_catalog_tests.rs index 6d7b082..466474f 100644 --- a/tests/caching_catalog_tests.rs +++ b/tests/caching_catalog_tests.rs @@ -665,12 +665,13 @@ async fn test_query_list_first_page_cache_miss_then_hit() { // Create a query run so the list is non-empty let id = runtimedb::id::generate_query_run_id(); + let snapshot = caching.get_or_create_snapshot("SELECT 1").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 1", - sql_hash: "abc123", - + snapshot_id: &snapshot.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -736,12 +737,13 @@ async fn test_query_list_dirty_marker_on_create() { // Create a query run — should set dirty marker let id = runtimedb::id::generate_query_run_id(); + let snapshot = caching.get_or_create_snapshot("SELECT 42").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 42", - sql_hash: "def456", - + snapshot_id: &snapshot.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -800,12 +802,13 @@ async fn test_query_list_dirty_marker_on_update() { // Create a query run let id = runtimedb::id::generate_query_run_id(); + let snapshot = caching.get_or_create_snapshot("SELECT 1").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 1", - sql_hash: "abc", - + snapshot_id: &snapshot.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -864,12 +867,13 @@ async fn test_query_list_cache_bypass_for_cursors_and_odd_limits() { // Create a query run let id = runtimedb::id::generate_query_run_id(); + let snapshot = caching.get_or_create_snapshot("SELECT 1").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 1", - sql_hash: "aaa", - + snapshot_id: &snapshot.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -930,12 +934,13 @@ async fn test_query_list_stale_while_revalidate() { // Create first query run and populate cache let id1 = runtimedb::id::generate_query_run_id(); + let snapshot1 = caching.get_or_create_snapshot("SELECT 1").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id1, - sql_text: "SELECT 1", - sql_hash: "hash1", - + snapshot_id: &snapshot1.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -955,12 +960,13 @@ async fn test_query_list_stale_while_revalidate() { // Now create a second query run — this sets the dirty marker let id2 = runtimedb::id::generate_query_run_id(); + let snapshot2 = caching.get_or_create_snapshot("SELECT 2").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id2, - sql_text: "SELECT 2", - sql_hash: "hash2", - + snapshot_id: &snapshot2.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -1015,12 +1021,13 @@ async fn test_revalidation_lock_ownership_safety() { // Seed a query run so the list is non-empty for cache population let id1 = runtimedb::id::generate_query_run_id(); + let snapshot1 = caching.get_or_create_snapshot("SELECT 1").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id1, - sql_text: "SELECT 1", - sql_hash: "own_test", - + snapshot_id: &snapshot1.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -1038,12 +1045,13 @@ async fn test_revalidation_lock_ownership_safety() { // Create a second query run (sets dirty marker) let id2 = runtimedb::id::generate_query_run_id(); + let snapshot2 = caching.get_or_create_snapshot("SELECT 2").await.unwrap(); caching .create_query_run(CreateQueryRun { id: &id2, - sql_text: "SELECT 2", - sql_hash: "own_test2", - + snapshot_id: &snapshot2.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await diff --git a/tests/catalog_manager_suite.rs b/tests/catalog_manager_suite.rs index 4fcfa69..ef1f7c8 100644 --- a/tests/catalog_manager_suite.rs +++ b/tests/catalog_manager_suite.rs @@ -1,6 +1,7 @@ +#[allow(unused_imports)] use runtimedb::catalog::{ CatalogManager, CreateQueryRun, PostgresCatalogManager, QueryRunCursor, QueryRunStatus, - QueryRunUpdate, SqliteCatalogManager, + QueryRunUpdate, SavedQuery, SavedQueryVersion, SqlSnapshot, SqliteCatalogManager, }; use runtimedb::datasets::DEFAULT_SCHEMA; use sqlx::{PgPool, SqlitePool}; @@ -688,13 +689,14 @@ macro_rules! catalog_manager_tests { let catalog = ctx.manager(); let id = runtimedb::id::generate_query_run_id(); + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); catalog .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 1", - sql_hash: "abc123", - + snapshot_id: &snap.id, + saved_query_id: None, + saved_query_version: None, trace_id: Some("trace-000"), }) .await @@ -703,7 +705,7 @@ macro_rules! catalog_manager_tests { let run = catalog.get_query_run(&id).await.unwrap().unwrap(); assert_eq!(run.id, id); assert_eq!(run.sql_text, "SELECT 1"); - assert_eq!(run.sql_hash, "abc123"); + assert_eq!(run.snapshot_id, snap.id); assert_eq!(run.trace_id.as_deref(), Some("trace-000")); assert_eq!(run.status, QueryRunStatus::Running); assert!(run.result_id.is_none()); @@ -716,13 +718,14 @@ macro_rules! catalog_manager_tests { let catalog = ctx.manager(); let id = runtimedb::id::generate_query_run_id(); + let snap = catalog.get_or_create_snapshot("SELECT 42").await.unwrap(); catalog .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT 42", - sql_hash: "def456", - + snapshot_id: &snap.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -756,13 +759,14 @@ macro_rules! catalog_manager_tests { let catalog = ctx.manager(); let id = runtimedb::id::generate_query_run_id(); + let snap = catalog.get_or_create_snapshot("SELECT bad").await.unwrap(); catalog .create_query_run(CreateQueryRun { id: &id, - sql_text: "SELECT bad", - sql_hash: "ghi789", - + snapshot_id: &snap.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -797,12 +801,14 @@ macro_rules! catalog_manager_tests { let mut ids = Vec::new(); for i in 0..5 { let id = runtimedb::id::generate_query_run_id(); + let sql = format!("SELECT {}", i); + let snap = catalog.get_or_create_snapshot(&sql).await.unwrap(); catalog .create_query_run(CreateQueryRun { id: &id, - sql_text: &format!("SELECT {}", i), - sql_hash: &format!("hash{}", i), - + snapshot_id: &snap.id, + saved_query_id: None, + saved_query_version: None, trace_id: None, }) .await @@ -869,6 +875,330 @@ macro_rules! catalog_manager_tests { assert!(!updated); } + // ─────────────────────────────────────────────────────────── + // Saved query tests + // ─────────────────────────────────────────────────────────── + + #[tokio::test] + async fn saved_query_create_and_get() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("my query", &snap.id) + .await + .unwrap(); + + assert!(!sq.id.is_empty()); + assert_eq!(sq.name, "my query"); + assert_eq!(sq.latest_version, 1); + + let fetched = catalog.get_saved_query(&sq.id).await.unwrap().unwrap(); + assert_eq!(fetched.id, sq.id); + assert_eq!(fetched.name, "my query"); + assert_eq!(fetched.latest_version, 1); + } + + #[tokio::test] + async fn saved_query_version_created_on_create() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 42").await.unwrap(); + let sq = catalog + .create_saved_query("q1", &snap.id) + .await + .unwrap(); + + let version = catalog + .get_saved_query_version(&sq.id, 1) + .await + .unwrap() + .expect("version 1 should exist"); + + assert_eq!(version.saved_query_id, sq.id); + assert_eq!(version.version, 1); + assert_eq!(version.snapshot_id, snap.id); + assert_eq!(version.sql_text, "SELECT 42"); + assert_eq!(version.sql_hash, snap.sql_hash); + } + + #[tokio::test] + async fn saved_query_update_creates_new_version() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap1 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("q1", &snap1.id) + .await + .unwrap(); + + let snap2 = catalog.get_or_create_snapshot("SELECT 2").await.unwrap(); + let updated = catalog + .update_saved_query(&sq.id, None, &snap2.id) + .await + .unwrap() + .expect("update should return saved query"); + + assert_eq!(updated.latest_version, 2); + + // Both versions should exist + let v1 = catalog + .get_saved_query_version(&sq.id, 1) + .await + .unwrap() + .unwrap(); + assert_eq!(v1.sql_text, "SELECT 1"); + + let v2 = catalog + .get_saved_query_version(&sq.id, 2) + .await + .unwrap() + .unwrap(); + assert_eq!(v2.sql_text, "SELECT 2"); + } + + #[tokio::test] + async fn saved_query_list_with_pagination() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + for i in 0..5 { + let snap = catalog + .get_or_create_snapshot(&format!("SELECT {}", i)) + .await + .unwrap(); + catalog + .create_saved_query( + &format!("query_{}", i), + &snap.id, + ) + .await + .unwrap(); + } + + let (page1, has_more) = catalog.list_saved_queries(3, 0).await.unwrap(); + assert_eq!(page1.len(), 3); + assert!(has_more); + + let (page2, has_more) = catalog.list_saved_queries(3, 3).await.unwrap(); + assert_eq!(page2.len(), 2); + assert!(!has_more); + } + + #[tokio::test] + async fn saved_query_delete() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("to_delete", &snap.id) + .await + .unwrap(); + + let deleted = catalog.delete_saved_query(&sq.id).await.unwrap(); + assert!(deleted); + + let fetched = catalog.get_saved_query(&sq.id).await.unwrap(); + assert!(fetched.is_none()); + + // Versions should be cascade-deleted + let versions = catalog.list_saved_query_versions(&sq.id).await.unwrap(); + assert!(versions.is_empty()); + } + + #[tokio::test] + async fn saved_query_delete_nonexistent() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let deleted = catalog.delete_saved_query("svqr_nope").await.unwrap(); + assert!(!deleted); + } + + #[tokio::test] + async fn saved_query_get_nonexistent() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let result = catalog.get_saved_query("svqr_nope").await.unwrap(); + assert!(result.is_none()); + } + + #[tokio::test] + async fn saved_query_update_nonexistent() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let result = catalog + .update_saved_query("svqr_nope", None, &snap.id) + .await + .unwrap(); + assert!(result.is_none()); + } + + #[tokio::test] + async fn saved_query_rename() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap1 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("original", &snap1.id) + .await + .unwrap(); + + let snap2 = catalog.get_or_create_snapshot("SELECT 2").await.unwrap(); + let updated = catalog + .update_saved_query(&sq.id, Some("renamed"), &snap2.id) + .await + .unwrap() + .unwrap(); + + assert_eq!(updated.name, "renamed"); + assert_eq!(updated.latest_version, 2); + + let fetched = catalog.get_saved_query(&sq.id).await.unwrap().unwrap(); + assert_eq!(fetched.name, "renamed"); + } + + #[tokio::test] + async fn saved_query_list_versions() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap1 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("q1", &snap1.id) + .await + .unwrap(); + let snap2 = catalog.get_or_create_snapshot("SELECT 2").await.unwrap(); + catalog + .update_saved_query(&sq.id, None, &snap2.id) + .await + .unwrap(); + let snap3 = catalog.get_or_create_snapshot("SELECT 3").await.unwrap(); + catalog + .update_saved_query(&sq.id, None, &snap3.id) + .await + .unwrap(); + + let versions = catalog.list_saved_query_versions(&sq.id).await.unwrap(); + assert_eq!(versions.len(), 3); + assert_eq!(versions[0].version, 1); + assert_eq!(versions[1].version, 2); + assert_eq!(versions[2].version, 3); + assert_eq!(versions[0].sql_text, "SELECT 1"); + assert_eq!(versions[2].sql_text, "SELECT 3"); + } + + #[tokio::test] + async fn saved_query_version_nonexistent() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("q1", &snap.id) + .await + .unwrap(); + + let v = catalog + .get_saved_query_version(&sq.id, 99) + .await + .unwrap(); + assert!(v.is_none()); + } + + #[tokio::test] + async fn query_run_with_saved_query_linkage() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("linked", &snap.id) + .await + .unwrap(); + + let id = runtimedb::id::generate_query_run_id(); + catalog + .create_query_run(CreateQueryRun { + id: &id, + snapshot_id: &snap.id, + saved_query_id: Some(&sq.id), + saved_query_version: Some(1), + trace_id: None, + }) + .await + .unwrap(); + + let run = catalog.get_query_run(&id).await.unwrap().unwrap(); + assert_eq!(run.saved_query_id.as_deref(), Some(sq.id.as_str())); + assert_eq!(run.saved_query_version, Some(1)); + assert_eq!(run.sql_text, "SELECT 1"); + assert_eq!(run.snapshot_id, snap.id); + } + + #[tokio::test] + async fn get_or_create_snapshot_dedup() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap1 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let snap2 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + + // Same SQL should return the same snapshot id + assert_eq!(snap1.id, snap2.id); + assert_eq!(snap1.sql_hash, snap2.sql_hash); + assert_eq!(snap1.sql_text, "SELECT 1"); + + // Different SQL should return a different snapshot id + let snap3 = catalog.get_or_create_snapshot("SELECT 2").await.unwrap(); + assert_ne!(snap1.id, snap3.id); + assert_ne!(snap1.sql_hash, snap3.sql_hash); + assert_eq!(snap3.sql_text, "SELECT 2"); + } + + #[tokio::test] + async fn delete_saved_query_preserves_query_run_history() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + // Create a saved query + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("ephemeral", &snap.id) + .await + .unwrap(); + + // Create a query run linked to that saved query + let run_id = runtimedb::id::generate_query_run_id(); + catalog + .create_query_run(CreateQueryRun { + id: &run_id, + snapshot_id: &snap.id, + saved_query_id: Some(&sq.id), + saved_query_version: Some(1), + trace_id: None, + }) + .await + .unwrap(); + + // Delete the saved query + let deleted = catalog.delete_saved_query(&sq.id).await.unwrap(); + assert!(deleted); + + // The query run should still exist with resolvable sql_text + let run = catalog.get_query_run(&run_id).await.unwrap().unwrap(); + assert_eq!(run.sql_text, "SELECT 1"); + assert_eq!(run.snapshot_id, snap.id); + } + } }; } diff --git a/tests/result_persistence_tests.rs b/tests/result_persistence_tests.rs index b935b18..070906f 100644 --- a/tests/result_persistence_tests.rs +++ b/tests/result_persistence_tests.rs @@ -10,8 +10,8 @@ use datafusion::prelude::SessionContext; use rand::RngCore; use runtimedb::catalog::{ CatalogManager, ConnectionInfo, CreateQueryRun, DatasetInfo, OptimisticLock, PendingDeletion, - QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, - SqliteCatalogManager, TableInfo, UploadInfo, + QueryResult, QueryRun, QueryRunCursor, QueryRunUpdate, ResultStatus, ResultUpdate, SavedQuery, + SavedQueryVersion, SqlSnapshot, SqliteCatalogManager, TableInfo, UploadInfo, }; use runtimedb::http::app_server::{AppServer, PATH_QUERY, PATH_RESULT, PATH_RESULTS}; use runtimedb::secrets::{SecretMetadata, SecretStatus}; @@ -346,6 +346,60 @@ impl CatalogManager for FailingCatalog { async fn get_query_run(&self, id: &str) -> Result> { self.inner.get_query_run(id).await } + + // SQL snapshot methods + + async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { + self.inner.get_or_create_snapshot(sql_text).await + } + + // Saved query methods + + async fn create_saved_query(&self, name: &str, snapshot_id: &str) -> Result { + self.inner.create_saved_query(name, snapshot_id).await + } + + async fn get_saved_query(&self, id: &str) -> Result> { + self.inner.get_saved_query(id).await + } + + async fn list_saved_queries( + &self, + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.inner.list_saved_queries(limit, offset).await + } + + async fn update_saved_query( + &self, + id: &str, + name: Option<&str>, + snapshot_id: &str, + ) -> Result> { + self.inner.update_saved_query(id, name, snapshot_id).await + } + + async fn delete_saved_query(&self, id: &str) -> Result { + self.inner.delete_saved_query(id).await + } + + async fn get_saved_query_version( + &self, + saved_query_id: &str, + version: i32, + ) -> Result> { + self.inner + .get_saved_query_version(saved_query_id, version) + .await + } + + async fn list_saved_query_versions( + &self, + saved_query_id: &str, + ) -> Result> { + self.inner.list_saved_query_versions(saved_query_id).await + } } async fn setup_test() -> Result<(AppServer, TempDir)> { diff --git a/tests/saved_query_tests.rs b/tests/saved_query_tests.rs new file mode 100644 index 0000000..74e8f8f --- /dev/null +++ b/tests/saved_query_tests.rs @@ -0,0 +1,577 @@ +use anyhow::Result; +use axum::{ + body::Body, + http::{Request, StatusCode}, + Router, +}; +use base64::{engine::general_purpose::STANDARD, Engine}; +use rand::RngCore; +use runtimedb::http::app_server::{ + AppServer, PATH_QUERY, PATH_SAVED_QUERIES, PATH_SAVED_QUERY, PATH_SAVED_QUERY_EXECUTE, + PATH_SAVED_QUERY_VERSIONS, +}; +use runtimedb::RuntimeEngine; +use serde_json::json; +use tempfile::TempDir; +use tower::util::ServiceExt; + +fn generate_test_secret_key() -> String { + let mut key = [0u8; 32]; + rand::thread_rng().fill_bytes(&mut key); + STANDARD.encode(key) +} + +async fn setup_test() -> Result<(Router, TempDir)> { + let temp_dir = tempfile::tempdir()?; + let engine = RuntimeEngine::builder() + .base_dir(temp_dir.path()) + .secret_key(generate_test_secret_key()) + .build() + .await?; + let app = AppServer::new(engine); + Ok((app.router, temp_dir)) +} + +async fn send_request(router: &Router, request: Request) -> Result { + Ok(router.clone().oneshot(request).await?) +} + +async fn response_json(response: axum::response::Response) -> Result { + let body = axum::body::to_bytes(response.into_body(), usize::MAX).await?; + Ok(serde_json::from_slice(&body)?) +} + +async fn create_saved_query(router: &Router, name: &str, sql: &str) -> Result { + let response = send_request( + router, + Request::builder() + .method("POST") + .uri(PATH_SAVED_QUERIES) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "name": name, "sql": sql }), + )?))?, + ) + .await?; + assert_eq!(response.status(), StatusCode::CREATED); + response_json(response).await +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_create_saved_query() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let json = create_saved_query(&router, "my query", "SELECT 1").await?; + + assert!(json["id"].as_str().unwrap().starts_with("svqr")); + assert_eq!(json["name"], "my query"); + assert_eq!(json["latest_version"], 1); + assert_eq!(json["sql"], "SELECT 1"); + assert!(json["sql_hash"].is_string()); + assert!(json["created_at"].is_string()); + assert!(json["updated_at"].is_string()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_create_saved_query_empty_name_rejected() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(PATH_SAVED_QUERIES) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "name": " ", "sql": "SELECT 1" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_create_saved_query_empty_sql_rejected() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(PATH_SAVED_QUERIES) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "name": "q", "sql": "" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_get_saved_query() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "get test", "SELECT 42").await?; + let id = created["id"].as_str().unwrap(); + + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(&uri) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["id"], id); + assert_eq!(json["name"], "get test"); + assert_eq!(json["sql"], "SELECT 42"); + assert_eq!(json["latest_version"], 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_get_saved_query_not_found() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri("/v1/queries/svqr_nonexistent") + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_list_saved_queries() -> Result<()> { + let (router, _dir) = setup_test().await?; + + create_saved_query(&router, "q1", "SELECT 1").await?; + create_saved_query(&router, "q2", "SELECT 2").await?; + create_saved_query(&router, "q3", "SELECT 3").await?; + + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(format!("{}?limit=2&offset=0", PATH_SAVED_QUERIES)) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["count"], 2); + assert_eq!(json["limit"], 2); + assert!(json["has_more"].as_bool().unwrap()); + let queries = json["queries"].as_array().unwrap(); + assert_eq!(queries.len(), 2); + // Results are ordered by name ASC + assert_eq!(queries[0]["name"], "q1"); + assert_eq!(queries[1]["name"], "q2"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_update_saved_query_creates_new_version() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "versioned", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 2" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["latest_version"], 2); + assert_eq!(json["sql"], "SELECT 2"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_update_saved_query_with_rename() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "old name", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "name": "new name", "sql": "SELECT 2" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["name"], "new name"); + assert_eq!(json["sql"], "SELECT 2"); + assert_eq!(json["latest_version"], 2); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_update_saved_query_not_found() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri("/v1/queries/svqr_nonexistent") + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 1" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_delete_saved_query() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "delete me", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("DELETE") + .uri(&uri) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NO_CONTENT); + + // Verify it's gone + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(&uri) + .body(Body::empty())?, + ) + .await?; + assert_eq!(response.status(), StatusCode::NOT_FOUND); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_delete_saved_query_not_found() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("DELETE") + .uri("/v1/queries/svqr_nonexistent") + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_list_saved_query_versions() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "versioned", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + // Create version 2 + let uri = PATH_SAVED_QUERY.replace("{id}", id); + send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 2" }), + )?))?, + ) + .await?; + + // Create version 3 + send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 3" }), + )?))?, + ) + .await?; + + // List versions + let versions_uri = PATH_SAVED_QUERY_VERSIONS.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(&versions_uri) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["saved_query_id"], id); + let versions = json["versions"].as_array().unwrap(); + assert_eq!(versions.len(), 3); + assert_eq!(versions[0]["version"], 1); + assert_eq!(versions[0]["sql"], "SELECT 1"); + assert_eq!(versions[2]["version"], 3); + assert_eq!(versions[2]["sql"], "SELECT 3"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_list_versions_not_found() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri("/v1/queries/svqr_nonexistent/versions") + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_latest() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "exec test", "SELECT 1 as num").await?; + let id = created["id"].as_str().unwrap(); + + let uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&json!({}))?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + assert_eq!(json["columns"], json!(["num"])); + assert_eq!(json["row_count"], 1); + assert_eq!(json["rows"][0][0], 1); + assert!(json["query_run_id"].is_string()); + assert!(json["execution_time_ms"].is_number()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_pinned_version() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "pin test", "SELECT 1 as v").await?; + let id = created["id"].as_str().unwrap(); + + // Update to version 2 + let update_uri = PATH_SAVED_QUERY.replace("{id}", id); + send_request( + &router, + Request::builder() + .method("PUT") + .uri(&update_uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 2 as v" }), + )?))?, + ) + .await?; + + // Execute pinned to version 1 + let exec_uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&exec_uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&json!({ "version": 1 }))?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + // Should execute version 1's SQL (SELECT 1) + assert_eq!(json["rows"][0][0], 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_not_found() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri("/v1/queries/svqr_nonexistent/execute") + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&json!({}))?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_query_run_linkage_on_saved_query_execution() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "linkage test", "SELECT 1 as x").await?; + let id = created["id"].as_str().unwrap(); + + // Execute the saved query + let exec_uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&exec_uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&json!({}))?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let exec_json = response_json(response).await?; + let query_run_id = exec_json["query_run_id"].as_str().unwrap(); + + // Check the query run via the query-runs list endpoint + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri("/query-runs?limit=10") + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let list_json = response_json(response).await?; + + let runs = list_json["query_runs"].as_array().unwrap(); + let run = runs.iter().find(|r| r["id"] == query_run_id).unwrap(); + + assert_eq!(run["saved_query_id"], id); + assert_eq!(run["saved_query_version"], 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_ad_hoc_query_run_has_null_linkage() -> Result<()> { + let (router, _dir) = setup_test().await?; + + // Execute an ad-hoc query + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(PATH_QUERY) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 1" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let exec_json = response_json(response).await?; + let query_run_id = exec_json["query_run_id"].as_str().unwrap(); + + // Check the query run + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri("/query-runs?limit=10") + .body(Body::empty())?, + ) + .await?; + + let list_json = response_json(response).await?; + + let runs = list_json["query_runs"].as_array().unwrap(); + let run = runs.iter().find(|r| r["id"] == query_run_id).unwrap(); + + assert!(run["saved_query_id"].is_null()); + assert!(run["saved_query_version"].is_null()); + // Ad-hoc queries should still have sql_text and a snapshot + assert!(run["sql_text"].is_string()); + assert!(run["snapshot_id"].is_string()); + + Ok(()) +} From 3bda42eee2f31083b8240061972862c5f91f051f Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 09:36:19 -0800 Subject: [PATCH 07/19] fix(catalog): use BEGIN IMMEDIATE for SQLite write transactions --- src/catalog/sqlite_manager.rs | 222 ++++++++++++++++++++------------- tests/catalog_manager_suite.rs | 38 ++++++ 2 files changed, 171 insertions(+), 89 deletions(-) diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index f81cc9f..47ccad9 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -964,40 +964,55 @@ impl CatalogManager for SqliteCatalogManager { let id = crate::id::generate_saved_query_id(); let now = Utc::now().to_rfc3339(); - let mut tx = self.backend.pool().begin().await?; + // Use BEGIN IMMEDIATE to acquire a RESERVED lock up front, preventing + // concurrent writers from interleaving reads before locks are held. + let mut conn = self.backend.pool().acquire().await?; + sqlx::query("BEGIN IMMEDIATE").execute(&mut *conn).await?; + + let result: Result = async { + sqlx::query( + "INSERT INTO saved_queries (id, name, latest_version, created_at, updated_at) \ + VALUES (?, ?, 1, ?, ?)", + ) + .bind(&id) + .bind(name) + .bind(&now) + .bind(&now) + .execute(&mut *conn) + .await?; - sqlx::query( - "INSERT INTO saved_queries (id, name, latest_version, created_at, updated_at) \ - VALUES (?, ?, 1, ?, ?)", - ) - .bind(&id) - .bind(name) - .bind(&now) - .bind(&now) - .execute(&mut *tx) - .await?; + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES (?, 1, ?, ?)", + ) + .bind(&id) + .bind(snapshot_id) + .bind(&now) + .execute(&mut *conn) + .await?; - sqlx::query( - "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ - VALUES (?, 1, ?, ?)", - ) - .bind(&id) - .bind(snapshot_id) - .bind(&now) - .execute(&mut *tx) - .await?; + let row: SavedQueryRow = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(&id) + .fetch_one(&mut *conn) + .await?; - let row: SavedQueryRow = sqlx::query_as( - "SELECT id, name, latest_version, created_at, updated_at \ - FROM saved_queries WHERE id = ?", - ) - .bind(&id) - .fetch_one(&mut *tx) - .await?; + Ok(row.into_saved_query()) + } + .await; - tx.commit().await?; + match &result { + Ok(_) => { + sqlx::query("COMMIT").execute(&mut *conn).await?; + } + Err(_) => { + let _ = sqlx::query("ROLLBACK").execute(&mut *conn).await; + } + } - Ok(row.into_saved_query()) + result } #[tracing::instrument( @@ -1055,60 +1070,75 @@ impl CatalogManager for SqliteCatalogManager { name: Option<&str>, snapshot_id: &str, ) -> Result> { - // SQLite acquires an exclusive database-level lock on the first write - // within a transaction, so explicit row-level locking (FOR UPDATE) is - // unnecessary — concurrent writers are serialized at the engine level. - let mut tx = self.backend.pool().begin().await?; + // Use BEGIN IMMEDIATE to acquire a RESERVED lock up front, so the + // read of latest_version and subsequent insert of the next version are + // atomic. A deferred BEGIN would only take a SHARED lock on the read, + // allowing two concurrent updates to read the same latest_version and + // race on the (saved_query_id, version) primary key. + let mut conn = self.backend.pool().acquire().await?; + sqlx::query("BEGIN IMMEDIATE").execute(&mut *conn).await?; + + let result: Result> = async { + let existing: Option = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(id) + .fetch_optional(&mut *conn) + .await?; - let existing: Option = sqlx::query_as( - "SELECT id, name, latest_version, created_at, updated_at \ - FROM saved_queries WHERE id = ?", - ) - .bind(id) - .fetch_optional(&mut *tx) - .await?; + let existing = match existing { + Some(row) => row, + None => return Ok(None), + }; - let existing = match existing { - Some(row) => row, - None => return Ok(None), - }; + let new_version = existing.latest_version + 1; + let now = Utc::now().to_rfc3339(); - let new_version = existing.latest_version + 1; - let now = Utc::now().to_rfc3339(); + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES (?, ?, ?, ?)", + ) + .bind(id) + .bind(new_version) + .bind(snapshot_id) + .bind(&now) + .execute(&mut *conn) + .await?; - sqlx::query( - "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ - VALUES (?, ?, ?, ?)", - ) - .bind(id) - .bind(new_version) - .bind(snapshot_id) - .bind(&now) - .execute(&mut *tx) - .await?; + let effective_name = name.unwrap_or(&existing.name); + sqlx::query( + "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", + ) + .bind(effective_name) + .bind(new_version) + .bind(&now) + .bind(id) + .execute(&mut *conn) + .await?; - let effective_name = name.unwrap_or(&existing.name); - sqlx::query( - "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", - ) - .bind(effective_name) - .bind(new_version) - .bind(&now) - .bind(id) - .execute(&mut *tx) - .await?; + let row: SavedQueryRow = sqlx::query_as( + "SELECT id, name, latest_version, created_at, updated_at \ + FROM saved_queries WHERE id = ?", + ) + .bind(id) + .fetch_one(&mut *conn) + .await?; - let row: SavedQueryRow = sqlx::query_as( - "SELECT id, name, latest_version, created_at, updated_at \ - FROM saved_queries WHERE id = ?", - ) - .bind(id) - .fetch_one(&mut *tx) - .await?; + Ok(Some(row.into_saved_query())) + } + .await; - tx.commit().await?; + match &result { + Ok(_) => { + sqlx::query("COMMIT").execute(&mut *conn).await?; + } + Err(_) => { + let _ = sqlx::query("ROLLBACK").execute(&mut *conn).await; + } + } - Ok(Some(row.into_saved_query())) + result } #[tracing::instrument( @@ -1117,24 +1147,38 @@ impl CatalogManager for SqliteCatalogManager { fields(db = "sqlite", runtimedb.saved_query_id = %id) )] async fn delete_saved_query(&self, id: &str) -> Result { - let mut tx = self.backend.pool().begin().await?; + // Use BEGIN IMMEDIATE for consistency with other write transactions. + let mut conn = self.backend.pool().acquire().await?; + sqlx::query("BEGIN IMMEDIATE").execute(&mut *conn).await?; + + let result: Result = async { + // Explicitly delete child versions first (SQLite PRAGMA foreign_keys is + // not guaranteed to be ON, so we cannot rely on ON DELETE CASCADE). + // Query runs retain history via their snapshot_id FK to sql_snapshots. + sqlx::query("DELETE FROM saved_query_versions WHERE saved_query_id = ?") + .bind(id) + .execute(&mut *conn) + .await?; - // Explicitly delete child versions first (SQLite PRAGMA foreign_keys is - // not guaranteed to be ON, so we cannot rely on ON DELETE CASCADE). - // Query runs retain history via their snapshot_id FK to sql_snapshots. - sqlx::query("DELETE FROM saved_query_versions WHERE saved_query_id = ?") - .bind(id) - .execute(&mut *tx) - .await?; + let del = sqlx::query("DELETE FROM saved_queries WHERE id = ?") + .bind(id) + .execute(&mut *conn) + .await?; - let result = sqlx::query("DELETE FROM saved_queries WHERE id = ?") - .bind(id) - .execute(&mut *tx) - .await?; + Ok(del.rows_affected() > 0) + } + .await; - tx.commit().await?; + match &result { + Ok(_) => { + sqlx::query("COMMIT").execute(&mut *conn).await?; + } + Err(_) => { + let _ = sqlx::query("ROLLBACK").execute(&mut *conn).await; + } + } - Ok(result.rows_affected() > 0) + result } #[tracing::instrument( diff --git a/tests/catalog_manager_suite.rs b/tests/catalog_manager_suite.rs index ef1f7c8..112f40e 100644 --- a/tests/catalog_manager_suite.rs +++ b/tests/catalog_manager_suite.rs @@ -1206,6 +1206,44 @@ macro_rules! catalog_manager_tests { catalog_manager_tests!(sqlite, create_sqlite_catalog); catalog_manager_tests!(postgres, create_postgres_catalog); +/// Test that concurrent SQLite updates to the same saved query don't race on +/// the (saved_query_id, version) primary key. BEGIN IMMEDIATE serializes the +/// read-then-increment pattern so both updates succeed with distinct versions. +#[tokio::test] +async fn sqlite_concurrent_saved_query_updates() { + let ctx = create_sqlite_catalog().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("concurrent-test", &snap.id) + .await + .unwrap(); + assert_eq!(sq.latest_version, 1); + + let snap2 = catalog.get_or_create_snapshot("SELECT 2").await.unwrap(); + let snap3 = catalog.get_or_create_snapshot("SELECT 3").await.unwrap(); + + // Launch two updates concurrently against the same saved query. + let (r1, r2) = tokio::join!( + catalog.update_saved_query(&sq.id, None, &snap2.id), + catalog.update_saved_query(&sq.id, None, &snap3.id), + ); + + // Both must succeed — no PK violation. + let u1 = r1.unwrap().expect("update 1 should find the query"); + let u2 = r2.unwrap().expect("update 2 should find the query"); + + // One should be version 2, the other version 3, in either order. + let mut versions = vec![u1.latest_version, u2.latest_version]; + versions.sort(); + assert_eq!(versions, vec![2, 3]); + + // Final state should be version 3. + let final_sq = catalog.get_saved_query(&sq.id).await.unwrap().unwrap(); + assert_eq!(final_sq.latest_version, 3); +} + /// Test that migration hash mismatch is detected on startup. /// This simulates the scenario where a migration SQL file was modified after /// being applied to a database. From b5981162db7623dd728445dc5cecec57b1e1c69a Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 12:07:19 -0800 Subject: [PATCH 08/19] fix(migrations): drop pgcrypto dep, document backfill ID format --- migrations/postgres/v6.sql | 8 ++++++-- migrations/sqlite/v6.sql | 6 +++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/migrations/postgres/v6.sql b/migrations/postgres/v6.sql index 18adc27..184e380 100644 --- a/migrations/postgres/v6.sql +++ b/migrations/postgres/v6.sql @@ -31,10 +31,14 @@ CREATE TABLE saved_query_versions ( PRIMARY KEY (saved_query_id, version) ); --- 4) Backfill sql_snapshots from existing query_runs +-- 4) Backfill sql_snapshots from existing query_runs. +-- IDs use the 'snap' prefix + 26 hex chars from md5, matching the 30-char +-- length of runtime nanoid-based IDs. The hex charset (0-9a-f) is narrower +-- than the full nanoid alphabet but functionally equivalent since IDs are +-- opaque. random() provides uniqueness without requiring pgcrypto. INSERT INTO sql_snapshots (id, sql_hash, sql_text, created_at) SELECT - 'snap' || substr(md5(sql_hash || sql_text || gen_random_uuid()::text), 1, 26), + 'snap' || substr(md5(sql_hash || sql_text || random()::text), 1, 26), sql_hash, sql_text, MIN(created_at) diff --git a/migrations/sqlite/v6.sql b/migrations/sqlite/v6.sql index 60d4493..781f81d 100644 --- a/migrations/sqlite/v6.sql +++ b/migrations/sqlite/v6.sql @@ -31,7 +31,11 @@ CREATE TABLE saved_query_versions ( PRIMARY KEY (saved_query_id, version) ); --- 4) Backfill sql_snapshots from existing query_runs +-- 4) Backfill sql_snapshots from existing query_runs. +-- IDs use the 'snap' prefix + 26 hex chars from randomblob, matching the +-- 30-char length of runtime nanoid-based IDs. The hex charset (0-9a-f) is +-- narrower than the full nanoid alphabet but functionally equivalent since +-- IDs are opaque. INSERT INTO sql_snapshots (id, sql_hash, sql_text, created_at) SELECT 'snap' || lower(hex(randomblob(13))), From 601895bad107795f52d230d6b0a3c89830d7c7d6 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 12:07:22 -0800 Subject: [PATCH 09/19] perf(engine): skip version creation on no-op updates --- src/engine.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 319d819..8c6fe1a 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -618,7 +618,7 @@ impl RuntimeEngine { /// Inner method that supports optional saved query linkage. #[tracing::instrument( - name = "execute_query_with_persistence_inner", + name = "execute_query_with_persistence", skip(self, sql), fields( runtimedb.query_run_id = tracing::field::Empty, @@ -1037,6 +1037,27 @@ impl RuntimeEngine { sql: &str, ) -> Result> { let snapshot = self.catalog.get_or_create_snapshot(sql).await?; + + // Skip version creation when neither SQL nor name actually changed. + if let Some(ref sq) = self.catalog.get_saved_query(id).await? { + let effective_name = name.unwrap_or(&sq.name); + if let Some(current) = self + .catalog + .get_saved_query_version(id, sq.latest_version) + .await? + { + if current.snapshot_id == snapshot.id && effective_name == sq.name { + return Ok(Some(SavedQuery { + id: sq.id.clone(), + name: sq.name.clone(), + latest_version: sq.latest_version, + created_at: sq.created_at, + updated_at: sq.updated_at, + })); + } + } + } + self.catalog .update_saved_query(id, name, &snapshot.id) .await @@ -1059,8 +1080,12 @@ impl RuntimeEngine { pub async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result> { - self.catalog.list_saved_query_versions(saved_query_id).await + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.catalog + .list_saved_query_versions(saved_query_id, limit, offset) + .await } /// Execute a saved query by ID, optionally pinned to a specific version. From d413f4561aaf85381f305f2d6596581e22495ec0 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 12:07:27 -0800 Subject: [PATCH 10/19] refactor(http): clean up validation and accept empty execute body --- .../controllers/saved_queries_controller.rs | 44 ++++++++----------- src/http/models.rs | 11 +++++ 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs index 670ced2..98d0fc7 100644 --- a/src/http/controllers/saved_queries_controller.rs +++ b/src/http/controllers/saved_queries_controller.rs @@ -2,9 +2,9 @@ use crate::http::controllers::query_controller::serialize_batches; use crate::http::error::ApiError; use crate::http::models::{ CreateSavedQueryRequest, CreateSavedQueryResponse, ExecuteSavedQueryRequest, - ListSavedQueriesParams, ListSavedQueriesResponse, ListSavedQueryVersionsResponse, - QueryResponse, SavedQueryDetail, SavedQuerySummary, SavedQueryVersionInfo, - UpdateSavedQueryRequest, + ListSavedQueriesParams, ListSavedQueriesResponse, ListSavedQueryVersionsParams, + ListSavedQueryVersionsResponse, QueryResponse, SavedQueryDetail, SavedQuerySummary, + SavedQueryVersionInfo, UpdateSavedQueryRequest, }; use crate::RuntimeEngine; use axum::{ @@ -24,12 +24,6 @@ pub async fn create_saved_query( State(engine): State>, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - if request.name.len() > MAX_NAME_LENGTH || request.sql.len() > MAX_SQL_LENGTH { - return Err(ApiError::bad_request( - "Request field exceeds maximum length", - )); - } - let name = request.name.trim().to_string(); let sql = request.sql.trim().to_string(); @@ -155,19 +149,6 @@ pub async fn update_saved_query( Path(id): Path, Json(request): Json, ) -> Result, ApiError> { - if request.sql.len() > MAX_SQL_LENGTH { - return Err(ApiError::bad_request( - "Request field exceeds maximum length", - )); - } - if let Some(ref name) = request.name { - if name.len() > MAX_NAME_LENGTH { - return Err(ApiError::bad_request( - "Request field exceeds maximum length", - )); - } - } - let sql = request.sql.trim().to_string(); if sql.is_empty() { return Err(ApiError::bad_request("SQL cannot be empty")); @@ -244,7 +225,11 @@ pub async fn delete_saved_query( pub async fn list_saved_query_versions( State(engine): State>, Path(id): Path, + QueryParams(params): QueryParams, ) -> Result, ApiError> { + let limit = params.limit.unwrap_or(DEFAULT_LIMIT).clamp(1, MAX_LIMIT); + let offset = params.offset.unwrap_or(0); + // Verify saved query exists engine .get_saved_query(&id) @@ -252,11 +237,12 @@ pub async fn list_saved_query_versions( .map_err(|e| -> ApiError { e.into() })? .ok_or_else(|| ApiError::not_found(format!("Saved query '{}' not found", id)))?; - let versions = engine - .list_saved_query_versions(&id) + let (versions, has_more) = engine + .list_saved_query_versions(&id, limit, offset) .await .map_err(|e| -> ApiError { e.into() })?; + let count = versions.len(); let version_infos: Vec = versions .into_iter() .map(|v| SavedQueryVersionInfo { @@ -270,6 +256,10 @@ pub async fn list_saved_query_versions( Ok(Json(ListSavedQueryVersionsResponse { saved_query_id: id, versions: version_infos, + count, + offset, + limit, + has_more, })) } @@ -277,12 +267,14 @@ pub async fn list_saved_query_versions( pub async fn execute_saved_query( State(engine): State>, Path(id): Path, - Json(request): Json, + body: Option>, ) -> Result, ApiError> { + let version = body.and_then(|Json(r)| r.version); + // engine.execute_saved_query returns NotFoundError (→ 404) if the saved // query or requested version does not exist, so no pre-check is needed. let result = engine - .execute_saved_query(&id, request.version) + .execute_saved_query(&id, version) .await .map_err(|e| -> ApiError { e.into() })?; diff --git a/src/http/models.rs b/src/http/models.rs index f01920e..86e4e24 100644 --- a/src/http/models.rs +++ b/src/http/models.rs @@ -634,11 +634,22 @@ pub struct SavedQueryVersionInfo { pub created_at: DateTime, } +/// Query params for GET /v1/queries/{id}/versions +#[derive(Debug, Deserialize)] +pub struct ListSavedQueryVersionsParams { + pub limit: Option, + pub offset: Option, +} + /// Response body for GET /v1/queries/{id}/versions #[derive(Debug, Serialize)] pub struct ListSavedQueryVersionsResponse { pub saved_query_id: String, pub versions: Vec, + pub count: usize, + pub offset: usize, + pub limit: usize, + pub has_more: bool, } #[cfg(test)] From f87076a81d7025af83736c42216618463857f9f4 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Sun, 8 Feb 2026 12:07:32 -0800 Subject: [PATCH 11/19] feat(catalog): paginate versions, dedup mock snapshots --- src/catalog/caching_manager.rs | 8 ++++++-- src/catalog/manager.rs | 7 +++++-- src/catalog/mock_catalog.rs | 21 ++++++++++++++++----- src/catalog/postgres_manager.rs | 19 ++++++++++++++++--- src/catalog/sqlite_manager.rs | 18 ++++++++++++++---- tests/catalog_manager_suite.rs | 17 +++++++++-------- tests/result_persistence_tests.rs | 8 ++++++-- tests/saved_query_tests.rs | 10 ++++++---- 8 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/catalog/caching_manager.rs b/src/catalog/caching_manager.rs index 4c9503a..d832994 100644 --- a/src/catalog/caching_manager.rs +++ b/src/catalog/caching_manager.rs @@ -1174,8 +1174,12 @@ impl CatalogManager for CachingCatalogManager { async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result> { - self.inner().list_saved_query_versions(saved_query_id).await + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.inner() + .list_saved_query_versions(saved_query_id, limit, offset) + .await } // Dataset management methods (with cache invalidation for list_dataset_table_names) diff --git a/src/catalog/manager.rs b/src/catalog/manager.rs index 02a43c1..3ef88ed 100644 --- a/src/catalog/manager.rs +++ b/src/catalog/manager.rs @@ -827,12 +827,15 @@ pub trait CatalogManager: Debug + Send + Sync { version: i32, ) -> Result>; - /// List all versions of a saved query, ordered by version ascending. + /// List versions of a saved query, ordered by version descending. /// sql_text and sql_hash are resolved via JOIN to sql_snapshots. + /// Returns (versions, has_more) for pagination. async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result>; + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)>; // Dataset management methods diff --git a/src/catalog/mock_catalog.rs b/src/catalog/mock_catalog.rs index b07c4af..0d4f501 100644 --- a/src/catalog/mock_catalog.rs +++ b/src/catalog/mock_catalog.rs @@ -21,6 +21,7 @@ use std::sync::{Mutex, RwLock}; pub struct MockCatalog { tables: Mutex>, results: RwLock>, + snapshots: Mutex>, fail_update: AtomicBool, next_id: AtomicUsize, } @@ -30,6 +31,7 @@ impl MockCatalog { Self { tables: Mutex::new(HashMap::new()), results: RwLock::new(HashMap::new()), + snapshots: Mutex::new(HashMap::new()), fail_update: AtomicBool::new(false), next_id: AtomicUsize::new(1), } @@ -341,12 +343,19 @@ impl CatalogManager for MockCatalog { async fn get_or_create_snapshot(&self, sql_text: &str) -> Result { let hash = crate::catalog::manager::sql_hash(sql_text); - Ok(SqlSnapshot { + let mut snapshots = self.snapshots.lock().unwrap(); + // Deduplicate by sql_hash — return existing snapshot if one exists + if let Some(existing) = snapshots.get(&hash) { + return Ok(existing.clone()); + } + let snapshot = SqlSnapshot { id: crate::id::generate_snapshot_id(), - sql_hash: hash, + sql_hash: hash.clone(), sql_text: sql_text.to_string(), created_at: chrono::Utc::now(), - }) + }; + snapshots.insert(hash, snapshot.clone()); + Ok(snapshot) } async fn create_saved_query(&self, _name: &str, _snapshot_id: &str) -> Result { @@ -389,8 +398,10 @@ impl CatalogManager for MockCatalog { async fn list_saved_query_versions( &self, _saved_query_id: &str, - ) -> Result> { - Ok(vec![]) + _limit: usize, + _offset: usize, + ) -> Result<(Vec, bool)> { + Ok((vec![], false)) } async fn create_dataset(&self, _dataset: &DatasetInfo) -> Result<()> { diff --git a/src/catalog/postgres_manager.rs b/src/catalog/postgres_manager.rs index 1603749..e37191d 100644 --- a/src/catalog/postgres_manager.rs +++ b/src/catalog/postgres_manager.rs @@ -1109,20 +1109,33 @@ impl CatalogManager for PostgresCatalogManager { async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result> { + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + let fetch_limit = limit.saturating_add(1); let rows: Vec = sqlx::query_as( "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ snap.sql_text, snap.sql_hash, sqv.created_at \ FROM saved_query_versions sqv \ JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ WHERE sqv.saved_query_id = $1 \ - ORDER BY sqv.version ASC", + ORDER BY sqv.version DESC \ + LIMIT $2 OFFSET $3", ) .bind(saved_query_id) + .bind(fetch_limit as i64) + .bind(offset as i64) .fetch_all(self.backend.pool()) .await?; - Ok(rows.into_iter().map(SavedQueryVersion::from).collect()) + let has_more = rows.len() > limit; + let versions = rows + .into_iter() + .take(limit) + .map(SavedQueryVersion::from) + .collect(); + + Ok((versions, has_more)) } // Dataset management methods diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index 47ccad9..2f380db 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -1214,23 +1214,33 @@ impl CatalogManager for SqliteCatalogManager { async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result> { + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + let fetch_limit = limit.saturating_add(1); let rows: Vec = sqlx::query_as( "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ snap.sql_text, snap.sql_hash, sqv.created_at \ FROM saved_query_versions sqv \ JOIN sql_snapshots snap ON snap.id = sqv.snapshot_id \ WHERE sqv.saved_query_id = ? \ - ORDER BY sqv.version ASC", + ORDER BY sqv.version DESC \ + LIMIT ? OFFSET ?", ) .bind(saved_query_id) + .bind(fetch_limit as i64) + .bind(offset as i64) .fetch_all(self.backend.pool()) .await?; - Ok(rows + let has_more = rows.len() > limit; + let versions = rows .into_iter() + .take(limit) .map(SavedQueryVersionRow::into_saved_query_version) - .collect()) + .collect(); + + Ok((versions, has_more)) } // Dataset management methods diff --git a/tests/catalog_manager_suite.rs b/tests/catalog_manager_suite.rs index 112f40e..ef7c520 100644 --- a/tests/catalog_manager_suite.rs +++ b/tests/catalog_manager_suite.rs @@ -1,7 +1,6 @@ -#[allow(unused_imports)] use runtimedb::catalog::{ CatalogManager, CreateQueryRun, PostgresCatalogManager, QueryRunCursor, QueryRunStatus, - QueryRunUpdate, SavedQuery, SavedQueryVersion, SqlSnapshot, SqliteCatalogManager, + QueryRunUpdate, SqliteCatalogManager, }; use runtimedb::datasets::DEFAULT_SCHEMA; use sqlx::{PgPool, SqlitePool}; @@ -1006,7 +1005,7 @@ macro_rules! catalog_manager_tests { assert!(fetched.is_none()); // Versions should be cascade-deleted - let versions = catalog.list_saved_query_versions(&sq.id).await.unwrap(); + let (versions, _) = catalog.list_saved_query_versions(&sq.id, 100, 0).await.unwrap(); assert!(versions.is_empty()); } @@ -1087,13 +1086,15 @@ macro_rules! catalog_manager_tests { .await .unwrap(); - let versions = catalog.list_saved_query_versions(&sq.id).await.unwrap(); + let (versions, has_more) = catalog.list_saved_query_versions(&sq.id, 100, 0).await.unwrap(); + assert!(!has_more); assert_eq!(versions.len(), 3); - assert_eq!(versions[0].version, 1); + // Ordered by version DESC (newest first) + assert_eq!(versions[0].version, 3); assert_eq!(versions[1].version, 2); - assert_eq!(versions[2].version, 3); - assert_eq!(versions[0].sql_text, "SELECT 1"); - assert_eq!(versions[2].sql_text, "SELECT 3"); + assert_eq!(versions[2].version, 1); + assert_eq!(versions[0].sql_text, "SELECT 3"); + assert_eq!(versions[2].sql_text, "SELECT 1"); } #[tokio::test] diff --git a/tests/result_persistence_tests.rs b/tests/result_persistence_tests.rs index 070906f..154d14e 100644 --- a/tests/result_persistence_tests.rs +++ b/tests/result_persistence_tests.rs @@ -397,8 +397,12 @@ impl CatalogManager for FailingCatalog { async fn list_saved_query_versions( &self, saved_query_id: &str, - ) -> Result> { - self.inner.list_saved_query_versions(saved_query_id).await + limit: usize, + offset: usize, + ) -> Result<(Vec, bool)> { + self.inner + .list_saved_query_versions(saved_query_id, limit, offset) + .await } } diff --git a/tests/saved_query_tests.rs b/tests/saved_query_tests.rs index 74e8f8f..47390ee 100644 --- a/tests/saved_query_tests.rs +++ b/tests/saved_query_tests.rs @@ -369,12 +369,14 @@ async fn test_list_saved_query_versions() -> Result<()> { let json = response_json(response).await?; assert_eq!(json["saved_query_id"], id); + assert!(json["has_more"].is_boolean()); let versions = json["versions"].as_array().unwrap(); assert_eq!(versions.len(), 3); - assert_eq!(versions[0]["version"], 1); - assert_eq!(versions[0]["sql"], "SELECT 1"); - assert_eq!(versions[2]["version"], 3); - assert_eq!(versions[2]["sql"], "SELECT 3"); + // Ordered by version DESC (newest first) + assert_eq!(versions[0]["version"], 3); + assert_eq!(versions[0]["sql"], "SELECT 3"); + assert_eq!(versions[2]["version"], 1); + assert_eq!(versions[2]["sql"], "SELECT 1"); Ok(()) } From fc3cc09ddb409dc85fa6b6845edf8186e40dc6d4 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Tue, 10 Feb 2026 09:00:41 -0800 Subject: [PATCH 12/19] fix(catalog): move no-op version check inside transaction --- src/catalog/postgres_manager.rs | 30 +++++++++++++++++++++++++----- src/catalog/sqlite_manager.rs | 28 +++++++++++++++++++++++----- src/engine.rs | 21 --------------------- 3 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/catalog/postgres_manager.rs b/src/catalog/postgres_manager.rs index e37191d..16b1af7 100644 --- a/src/catalog/postgres_manager.rs +++ b/src/catalog/postgres_manager.rs @@ -977,7 +977,7 @@ impl CatalogManager for PostgresCatalogManager { limit: usize, offset: usize, ) -> Result<(Vec, bool)> { - let fetch_limit = limit.saturating_add(1) as i64; + let fetch_limit = i64::try_from(limit.saturating_add(1)).unwrap_or(i64::MAX); let rows: Vec = sqlx::query_as( "SELECT id, name, latest_version, created_at, updated_at \ FROM saved_queries \ @@ -985,7 +985,7 @@ impl CatalogManager for PostgresCatalogManager { LIMIT $1 OFFSET $2", ) .bind(fetch_limit) - .bind(offset as i64) + .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; @@ -1022,6 +1022,27 @@ impl CatalogManager for PostgresCatalogManager { None => return Ok(None), }; + let effective_name = name.unwrap_or(&existing.name); + + // Check if the latest version already points to the same snapshot + // and the name is unchanged — skip creating a redundant version. + // This check runs inside the FOR UPDATE lock, so no TOCTOU race. + let current_snapshot: Option<(String,)> = sqlx::query_as( + "SELECT snapshot_id FROM saved_query_versions \ + WHERE saved_query_id = $1 AND version = $2", + ) + .bind(id) + .bind(existing.latest_version) + .fetch_optional(&mut *tx) + .await?; + + if let Some((current_sid,)) = current_snapshot { + if current_sid == snapshot_id && effective_name == existing.name { + tx.commit().await?; + return Ok(Some(SavedQuery::from(existing))); + } + } + let new_version = existing.latest_version + 1; let now = Utc::now(); @@ -1036,7 +1057,6 @@ impl CatalogManager for PostgresCatalogManager { .execute(&mut *tx) .await?; - let effective_name = name.unwrap_or(&existing.name); sqlx::query( "UPDATE saved_queries SET name = $1, latest_version = $2, updated_at = $3 WHERE id = $4", ) @@ -1123,8 +1143,8 @@ impl CatalogManager for PostgresCatalogManager { LIMIT $2 OFFSET $3", ) .bind(saved_query_id) - .bind(fetch_limit as i64) - .bind(offset as i64) + .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index 2f380db..c2a5879 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -1045,8 +1045,8 @@ impl CatalogManager for SqliteCatalogManager { ORDER BY name ASC \ LIMIT ? OFFSET ?", ) - .bind(fetch_limit as i64) - .bind(offset as i64) + .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; @@ -1092,6 +1092,25 @@ impl CatalogManager for SqliteCatalogManager { None => return Ok(None), }; + let effective_name = name.unwrap_or(&existing.name); + + // Check if the latest version already points to the same snapshot + // and the name is unchanged — skip creating a redundant version. + let current_snapshot: Option<(String,)> = sqlx::query_as( + "SELECT snapshot_id FROM saved_query_versions \ + WHERE saved_query_id = ? AND version = ?", + ) + .bind(id) + .bind(existing.latest_version) + .fetch_optional(&mut *conn) + .await?; + + if let Some((current_sid,)) = current_snapshot { + if current_sid == snapshot_id && effective_name == existing.name { + return Ok(Some(existing.into_saved_query())); + } + } + let new_version = existing.latest_version + 1; let now = Utc::now().to_rfc3339(); @@ -1106,7 +1125,6 @@ impl CatalogManager for SqliteCatalogManager { .execute(&mut *conn) .await?; - let effective_name = name.unwrap_or(&existing.name); sqlx::query( "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", ) @@ -1228,8 +1246,8 @@ impl CatalogManager for SqliteCatalogManager { LIMIT ? OFFSET ?", ) .bind(saved_query_id) - .bind(fetch_limit as i64) - .bind(offset as i64) + .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; diff --git a/src/engine.rs b/src/engine.rs index 8c6fe1a..e7bbcaf 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1037,27 +1037,6 @@ impl RuntimeEngine { sql: &str, ) -> Result> { let snapshot = self.catalog.get_or_create_snapshot(sql).await?; - - // Skip version creation when neither SQL nor name actually changed. - if let Some(ref sq) = self.catalog.get_saved_query(id).await? { - let effective_name = name.unwrap_or(&sq.name); - if let Some(current) = self - .catalog - .get_saved_query_version(id, sq.latest_version) - .await? - { - if current.snapshot_id == snapshot.id && effective_name == sq.name { - return Ok(Some(SavedQuery { - id: sq.id.clone(), - name: sq.name.clone(), - latest_version: sq.latest_version, - created_at: sq.created_at, - updated_at: sq.updated_at, - })); - } - } - } - self.catalog .update_saved_query(id, name, &snapshot.id) .await From 539680c0ceedf510a980a44b15a6bfba97c31946 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Tue, 10 Feb 2026 09:00:45 -0800 Subject: [PATCH 13/19] refactor(catalog): add raw-length guard, safe i64 casts --- .../controllers/saved_queries_controller.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs index 98d0fc7..0114ccb 100644 --- a/src/http/controllers/saved_queries_controller.rs +++ b/src/http/controllers/saved_queries_controller.rs @@ -18,12 +18,22 @@ const DEFAULT_LIMIT: usize = 100; const MAX_LIMIT: usize = 1000; const MAX_NAME_LENGTH: usize = 255; const MAX_SQL_LENGTH: usize = 1_000_000; +/// Upper bound on raw (pre-trim) input length to avoid spending CPU trimming +/// extremely large whitespace payloads. Any input exceeding this cannot be +/// valid after trimming since the post-trim limits are well below this. +const MAX_RAW_INPUT_LENGTH: usize = 2_000_000; /// Handler for POST /v1/queries pub async fn create_saved_query( State(engine): State>, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { + if request.name.len() > MAX_RAW_INPUT_LENGTH || request.sql.len() > MAX_RAW_INPUT_LENGTH { + return Err(ApiError::bad_request( + "Request field exceeds maximum length", + )); + } + let name = request.name.trim().to_string(); let sql = request.sql.trim().to_string(); @@ -149,6 +159,17 @@ pub async fn update_saved_query( Path(id): Path, Json(request): Json, ) -> Result, ApiError> { + let raw_too_large = request.sql.len() > MAX_RAW_INPUT_LENGTH + || request + .name + .as_ref() + .is_some_and(|n| n.len() > MAX_RAW_INPUT_LENGTH); + if raw_too_large { + return Err(ApiError::bad_request( + "Request field exceeds maximum length", + )); + } + let sql = request.sql.trim().to_string(); if sql.is_empty() { return Err(ApiError::bad_request("SQL cannot be empty")); From 130856a15c4134284d8447ca2877881dd52a68ee Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 11 Feb 2026 20:50:13 -0800 Subject: [PATCH 14/19] fix(catalog): checked version increment, stable pagination, name-only rename --- src/catalog/postgres_manager.rs | 75 +++++++++++------- src/catalog/sqlite_manager.rs | 78 ++++++++++++------- .../controllers/saved_queries_controller.rs | 22 ++++-- src/http/models.rs | 12 --- 4 files changed, 110 insertions(+), 77 deletions(-) diff --git a/src/catalog/postgres_manager.rs b/src/catalog/postgres_manager.rs index 16b1af7..ca2e65d 100644 --- a/src/catalog/postgres_manager.rs +++ b/src/catalog/postgres_manager.rs @@ -981,7 +981,7 @@ impl CatalogManager for PostgresCatalogManager { let rows: Vec = sqlx::query_as( "SELECT id, name, latest_version, created_at, updated_at \ FROM saved_queries \ - ORDER BY name ASC \ + ORDER BY name ASC, id ASC \ LIMIT $1 OFFSET $2", ) .bind(fetch_limit) @@ -1036,36 +1036,55 @@ impl CatalogManager for PostgresCatalogManager { .fetch_optional(&mut *tx) .await?; - if let Some((current_sid,)) = current_snapshot { - if current_sid == snapshot_id && effective_name == existing.name { - tx.commit().await?; - return Ok(Some(SavedQuery::from(existing))); - } + let sql_unchanged = current_snapshot + .as_ref() + .is_some_and(|(sid,)| sid == snapshot_id); + let name_unchanged = effective_name == existing.name; + + if sql_unchanged && name_unchanged { + // Complete no-op — nothing to change + tx.commit().await?; + return Ok(Some(SavedQuery::from(existing))); } - let new_version = existing.latest_version + 1; let now = Utc::now(); - sqlx::query( - "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ - VALUES ($1, $2, $3, $4)", - ) - .bind(id) - .bind(new_version) - .bind(snapshot_id) - .bind(now) - .execute(&mut *tx) - .await?; + if sql_unchanged { + // Name-only rename — no new version needed + sqlx::query("UPDATE saved_queries SET name = $1, updated_at = $2 WHERE id = $3") + .bind(effective_name) + .bind(now) + .bind(id) + .execute(&mut *tx) + .await?; + } else { + // SQL changed — create a new version + let new_version = existing + .latest_version + .checked_add(1) + .ok_or_else(|| anyhow::anyhow!("Version limit reached for saved query '{}'", id))?; + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES ($1, $2, $3, $4)", + ) + .bind(id) + .bind(new_version) + .bind(snapshot_id) + .bind(now) + .execute(&mut *tx) + .await?; - sqlx::query( - "UPDATE saved_queries SET name = $1, latest_version = $2, updated_at = $3 WHERE id = $4", - ) - .bind(effective_name) - .bind(new_version) - .bind(now) - .bind(id) - .execute(&mut *tx) - .await?; + sqlx::query( + "UPDATE saved_queries SET name = $1, latest_version = $2, updated_at = $3 WHERE id = $4", + ) + .bind(effective_name) + .bind(new_version) + .bind(now) + .bind(id) + .execute(&mut *tx) + .await?; + } let row: SavedQueryRowPg = sqlx::query_as( "SELECT id, name, latest_version, created_at, updated_at \ @@ -1132,7 +1151,7 @@ impl CatalogManager for PostgresCatalogManager { limit: usize, offset: usize, ) -> Result<(Vec, bool)> { - let fetch_limit = limit.saturating_add(1); + let fetch_limit = i64::try_from(limit.saturating_add(1)).unwrap_or(i64::MAX); let rows: Vec = sqlx::query_as( "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ snap.sql_text, snap.sql_hash, sqv.created_at \ @@ -1143,7 +1162,7 @@ impl CatalogManager for PostgresCatalogManager { LIMIT $2 OFFSET $3", ) .bind(saved_query_id) - .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(fetch_limit) .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index c2a5879..8ac773e 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -1038,14 +1038,14 @@ impl CatalogManager for SqliteCatalogManager { limit: usize, offset: usize, ) -> Result<(Vec, bool)> { - let fetch_limit = limit.saturating_add(1); + let fetch_limit = i64::try_from(limit.saturating_add(1)).unwrap_or(i64::MAX); let rows: Vec = sqlx::query_as( "SELECT id, name, latest_version, created_at, updated_at \ FROM saved_queries \ - ORDER BY name ASC \ + ORDER BY name ASC, id ASC \ LIMIT ? OFFSET ?", ) - .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(fetch_limit) .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; @@ -1105,35 +1105,55 @@ impl CatalogManager for SqliteCatalogManager { .fetch_optional(&mut *conn) .await?; - if let Some((current_sid,)) = current_snapshot { - if current_sid == snapshot_id && effective_name == existing.name { - return Ok(Some(existing.into_saved_query())); - } + let sql_unchanged = current_snapshot + .as_ref() + .is_some_and(|(sid,)| sid == snapshot_id); + let name_unchanged = effective_name == existing.name; + + if sql_unchanged && name_unchanged { + // Complete no-op — nothing to change + return Ok(Some(existing.into_saved_query())); } - let new_version = existing.latest_version + 1; let now = Utc::now().to_rfc3339(); - sqlx::query( - "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ - VALUES (?, ?, ?, ?)", - ) - .bind(id) - .bind(new_version) - .bind(snapshot_id) - .bind(&now) - .execute(&mut *conn) - .await?; + if sql_unchanged { + // Name-only rename — no new version needed + sqlx::query( + "UPDATE saved_queries SET name = ?, updated_at = ? WHERE id = ?", + ) + .bind(effective_name) + .bind(&now) + .bind(id) + .execute(&mut *conn) + .await?; + } else { + // SQL changed — create a new version + let new_version = existing.latest_version.checked_add(1).ok_or_else(|| { + anyhow::anyhow!("Version limit reached for saved query '{}'", id) + })?; + + sqlx::query( + "INSERT INTO saved_query_versions (saved_query_id, version, snapshot_id, created_at) \ + VALUES (?, ?, ?, ?)", + ) + .bind(id) + .bind(new_version) + .bind(snapshot_id) + .bind(&now) + .execute(&mut *conn) + .await?; - sqlx::query( - "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", - ) - .bind(effective_name) - .bind(new_version) - .bind(&now) - .bind(id) - .execute(&mut *conn) - .await?; + sqlx::query( + "UPDATE saved_queries SET name = ?, latest_version = ?, updated_at = ? WHERE id = ?", + ) + .bind(effective_name) + .bind(new_version) + .bind(&now) + .bind(id) + .execute(&mut *conn) + .await?; + } let row: SavedQueryRow = sqlx::query_as( "SELECT id, name, latest_version, created_at, updated_at \ @@ -1235,7 +1255,7 @@ impl CatalogManager for SqliteCatalogManager { limit: usize, offset: usize, ) -> Result<(Vec, bool)> { - let fetch_limit = limit.saturating_add(1); + let fetch_limit = i64::try_from(limit.saturating_add(1)).unwrap_or(i64::MAX); let rows: Vec = sqlx::query_as( "SELECT sqv.saved_query_id, sqv.version, sqv.snapshot_id, \ snap.sql_text, snap.sql_hash, sqv.created_at \ @@ -1246,7 +1266,7 @@ impl CatalogManager for SqliteCatalogManager { LIMIT ? OFFSET ?", ) .bind(saved_query_id) - .bind(i64::try_from(fetch_limit).unwrap_or(i64::MAX)) + .bind(fetch_limit) .bind(i64::try_from(offset).unwrap_or(i64::MAX)) .fetch_all(self.backend.pool()) .await?; diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs index 0114ccb..da20cee 100644 --- a/src/http/controllers/saved_queries_controller.rs +++ b/src/http/controllers/saved_queries_controller.rs @@ -1,10 +1,10 @@ use crate::http::controllers::query_controller::serialize_batches; use crate::http::error::ApiError; use crate::http::models::{ - CreateSavedQueryRequest, CreateSavedQueryResponse, ExecuteSavedQueryRequest, - ListSavedQueriesParams, ListSavedQueriesResponse, ListSavedQueryVersionsParams, - ListSavedQueryVersionsResponse, QueryResponse, SavedQueryDetail, SavedQuerySummary, - SavedQueryVersionInfo, UpdateSavedQueryRequest, + CreateSavedQueryRequest, ExecuteSavedQueryRequest, ListSavedQueriesParams, + ListSavedQueriesResponse, ListSavedQueryVersionsParams, ListSavedQueryVersionsResponse, + QueryResponse, SavedQueryDetail, SavedQuerySummary, SavedQueryVersionInfo, + UpdateSavedQueryRequest, }; use crate::RuntimeEngine; use axum::{ @@ -27,7 +27,7 @@ const MAX_RAW_INPUT_LENGTH: usize = 2_000_000; pub async fn create_saved_query( State(engine): State>, Json(request): Json, -) -> Result<(StatusCode, Json), ApiError> { +) -> Result<(StatusCode, Json), ApiError> { if request.name.len() > MAX_RAW_INPUT_LENGTH || request.sql.len() > MAX_RAW_INPUT_LENGTH { return Err(ApiError::bad_request( "Request field exceeds maximum length", @@ -74,7 +74,7 @@ pub async fn create_saved_query( Ok(( StatusCode::CREATED, - Json(CreateSavedQueryResponse { + Json(SavedQueryDetail { id: saved_query.id, name: saved_query.name, latest_version: saved_query.latest_version, @@ -288,9 +288,15 @@ pub async fn list_saved_query_versions( pub async fn execute_saved_query( State(engine): State>, Path(id): Path, - body: Option>, + body: axum::body::Bytes, ) -> Result, ApiError> { - let version = body.and_then(|Json(r)| r.version); + let version = if body.is_empty() { + None + } else { + let req: ExecuteSavedQueryRequest = serde_json::from_slice(&body) + .map_err(|e| ApiError::bad_request(format!("Invalid JSON: {}", e)))?; + req.version + }; // engine.execute_saved_query returns NotFoundError (→ 404) if the saved // query or requested version does not exist, so no pre-check is needed. diff --git a/src/http/models.rs b/src/http/models.rs index 86e4e24..59fbf04 100644 --- a/src/http/models.rs +++ b/src/http/models.rs @@ -603,18 +603,6 @@ pub struct SavedQueryDetail { pub updated_at: DateTime, } -/// Response body for POST /v1/queries -#[derive(Debug, Serialize)] -pub struct CreateSavedQueryResponse { - pub id: String, - pub name: String, - pub latest_version: i32, - pub sql: String, - pub sql_hash: String, - pub created_at: DateTime, - pub updated_at: DateTime, -} - /// Response body for GET /v1/queries #[derive(Debug, Serialize)] pub struct ListSavedQueriesResponse { From 04e34501e8f7064212bb877e5a1000adef3311e7 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 11 Feb 2026 20:50:17 -0800 Subject: [PATCH 15/19] test(catalog): add no-op, name-only, dup-name, empty-body tests --- tests/catalog_manager_suite.rs | 99 ++++++++++++++++++ tests/saved_query_tests.rs | 186 +++++++++++++++++++++++++++++++++ 2 files changed, 285 insertions(+) diff --git a/tests/catalog_manager_suite.rs b/tests/catalog_manager_suite.rs index ef7c520..48a928e 100644 --- a/tests/catalog_manager_suite.rs +++ b/tests/catalog_manager_suite.rs @@ -1165,6 +1165,105 @@ macro_rules! catalog_manager_tests { assert_eq!(snap3.sql_text, "SELECT 2"); } + #[tokio::test] + async fn saved_query_noop_update_skips_version() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("noop_test", &snap.id) + .await + .unwrap(); + assert_eq!(sq.latest_version, 1); + + // Update with the exact same SQL and no name change + let updated = catalog + .update_saved_query(&sq.id, None, &snap.id) + .await + .unwrap() + .unwrap(); + + // Should still be version 1 — no new version created + assert_eq!(updated.latest_version, 1); + assert_eq!(updated.name, "noop_test"); + + let (versions, _) = catalog + .list_saved_query_versions(&sq.id, 100, 0) + .await + .unwrap(); + assert_eq!(versions.len(), 1); + } + + #[tokio::test] + async fn saved_query_name_only_rename_skips_version() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("original_name", &snap.id) + .await + .unwrap(); + assert_eq!(sq.latest_version, 1); + + // Rename without changing SQL + let updated = catalog + .update_saved_query(&sq.id, Some("new_name"), &snap.id) + .await + .unwrap() + .unwrap(); + + // Name should be updated but version should NOT increment + assert_eq!(updated.name, "new_name"); + assert_eq!(updated.latest_version, 1); + + // Only one version should exist + let (versions, _) = catalog + .list_saved_query_versions(&sq.id, 100, 0) + .await + .unwrap(); + assert_eq!(versions.len(), 1); + assert_eq!(versions[0].sql_text, "SELECT 1"); + } + + #[tokio::test] + async fn saved_query_list_stable_with_duplicate_names() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + // Create 5 saved queries all named "dup" + let mut ids = Vec::new(); + for i in 0..5 { + let snap = catalog + .get_or_create_snapshot(&format!("SELECT {}", i)) + .await + .unwrap(); + let sq = catalog + .create_saved_query("dup", &snap.id) + .await + .unwrap(); + ids.push(sq.id); + } + + // Fetch page 1 (3 items) and page 2 (2 items) + let (page1, has_more) = catalog.list_saved_queries(3, 0).await.unwrap(); + assert_eq!(page1.len(), 3); + assert!(has_more); + + let (page2, has_more2) = catalog.list_saved_queries(3, 3).await.unwrap(); + assert_eq!(page2.len(), 2); + assert!(!has_more2); + + // All pages combined should have exactly the 5 distinct IDs, no duplicates + let mut all_ids: Vec = page1.iter().map(|q| q.id.clone()) + .chain(page2.iter().map(|q| q.id.clone())) + .collect(); + all_ids.sort(); + all_ids.dedup(); + assert_eq!(all_ids.len(), 5); + } + #[tokio::test] async fn delete_saved_query_preserves_query_run_history() { let ctx = super::$setup_fn().await; diff --git a/tests/saved_query_tests.rs b/tests/saved_query_tests.rs index 47390ee..4b817a1 100644 --- a/tests/saved_query_tests.rs +++ b/tests/saved_query_tests.rs @@ -577,3 +577,189 @@ async fn test_ad_hoc_query_run_has_null_linkage() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_noop_update_preserves_version() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "noop test", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + // Update with the exact same SQL and no name change + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": "SELECT 1" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + // Version should not increment on no-op + assert_eq!(json["latest_version"], 1); + assert_eq!(json["name"], "noop test"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_name_only_rename_preserves_version() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "old name", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + // Rename without changing SQL + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "name": "new name", "sql": "SELECT 1" }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + + // Name changed but version should NOT increment + assert_eq!(json["name"], "new name"); + assert_eq!(json["latest_version"], 1); + + // Verify only 1 version exists + let versions_uri = PATH_SAVED_QUERY_VERSIONS.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(&versions_uri) + .body(Body::empty())?, + ) + .await?; + + let json = response_json(response).await?; + let versions = json["versions"].as_array().unwrap(); + assert_eq!(versions.len(), 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_truly_empty_body() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "empty body test", "SELECT 1 as num").await?; + let id = created["id"].as_str().unwrap(); + + // Send POST with completely empty body (no Content-Type header either) + let uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&uri) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + assert_eq!(json["rows"][0][0], 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_empty_body_with_content_type() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "ct empty test", "SELECT 1 as num").await?; + let id = created["id"].as_str().unwrap(); + + // Send POST with Content-Type: application/json but empty body + let uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + assert_eq!(json["rows"][0][0], 1); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_list_saved_queries_stable_with_duplicate_names() -> Result<()> { + let (router, _dir) = setup_test().await?; + + // Create 5 queries all named "dup" + let mut ids = Vec::new(); + for i in 0..5 { + let json = create_saved_query(&router, "dup", &format!("SELECT {}", i)).await?; + ids.push(json["id"].as_str().unwrap().to_string()); + } + + // Fetch page 1 (3 items) + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(format!("{}?limit=3&offset=0", PATH_SAVED_QUERIES)) + .body(Body::empty())?, + ) + .await?; + let json = response_json(response).await?; + assert_eq!(json["count"], 3); + assert!(json["has_more"].as_bool().unwrap()); + let page1: Vec = json["queries"] + .as_array() + .unwrap() + .iter() + .map(|q| q["id"].as_str().unwrap().to_string()) + .collect(); + + // Fetch page 2 (2 items) + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(format!("{}?limit=3&offset=3", PATH_SAVED_QUERIES)) + .body(Body::empty())?, + ) + .await?; + let json = response_json(response).await?; + assert_eq!(json["count"], 2); + assert!(!json["has_more"].as_bool().unwrap()); + let page2: Vec = json["queries"] + .as_array() + .unwrap() + .iter() + .map(|q| q["id"].as_str().unwrap().to_string()) + .collect(); + + // All pages should have distinct IDs (no duplicates, no skips) + let mut all_ids: Vec = page1.into_iter().chain(page2).collect(); + all_ids.sort(); + all_ids.dedup(); + assert_eq!(all_ids.len(), 5); + + Ok(()) +} From f93b1827e0bad64ae876b25e5fcfbce4f9ba96f3 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Thu, 12 Feb 2026 09:08:20 -0800 Subject: [PATCH 16/19] refactor(catalog): optional sql on update, skip redundant snapshot lookup --- src/catalog/sqlite_manager.rs | 10 +++- src/engine.rs | 47 +++++++++++++++---- .../controllers/saved_queries_controller.rs | 34 +++++++++----- src/http/models.rs | 4 +- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/src/catalog/sqlite_manager.rs b/src/catalog/sqlite_manager.rs index 8ac773e..8d20685 100644 --- a/src/catalog/sqlite_manager.rs +++ b/src/catalog/sqlite_manager.rs @@ -932,6 +932,12 @@ impl CatalogManager for SqliteCatalogManager { let id = crate::id::generate_snapshot_id(); let now = Utc::now().to_rfc3339(); + // Use a single connection so the INSERT OR IGNORE and SELECT are + // guaranteed to see the same row, even if a concurrent DELETE were + // to run between them (snapshots are never deleted, but this is + // defensive). + let mut conn = self.backend.pool().acquire().await?; + // INSERT OR IGNORE for idempotent upsert (race-safe via UNIQUE constraint) sqlx::query( "INSERT OR IGNORE INTO sql_snapshots (id, sql_hash, sql_text, created_at) \ @@ -941,7 +947,7 @@ impl CatalogManager for SqliteCatalogManager { .bind(&hash) .bind(sql_text) .bind(&now) - .execute(self.backend.pool()) + .execute(&mut *conn) .await?; // Fetch the existing or newly inserted row @@ -951,7 +957,7 @@ impl CatalogManager for SqliteCatalogManager { ) .bind(&hash) .bind(sql_text) - .fetch_one(self.backend.pool()) + .fetch_one(&mut *conn) .await?; Ok(row.into_sql_snapshot()) diff --git a/src/engine.rs b/src/engine.rs index e7bbcaf..b296a0a 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -612,14 +612,17 @@ impl RuntimeEngine { /// remain in `running` status indefinitely. A periodic cleanup job to mark stale /// `running` records as `failed` should be added in a future iteration. pub async fn execute_query_with_persistence(&self, sql: &str) -> Result { - self.execute_query_with_persistence_inner(sql, None, None) + self.execute_query_with_persistence_inner(sql, None, None, None) .await } /// Inner method that supports optional saved query linkage. + /// + /// When `known_snapshot_id` is provided, skips the `get_or_create_snapshot` + /// call (useful when the caller has already resolved the snapshot). #[tracing::instrument( name = "execute_query_with_persistence", - skip(self, sql), + skip(self, sql, known_snapshot_id), fields( runtimedb.query_run_id = tracing::field::Empty, runtimedb.result_id = tracing::field::Empty, @@ -631,10 +634,14 @@ impl RuntimeEngine { sql: &str, saved_query_id: Option<&str>, saved_query_version: Option, + known_snapshot_id: Option<&str>, ) -> Result { let start = Instant::now(); let query_run_id = crate::id::generate_query_run_id(); - let snapshot = self.catalog.get_or_create_snapshot(sql).await?; + let snapshot_id = match known_snapshot_id { + Some(id) => id.to_string(), + None => self.catalog.get_or_create_snapshot(sql).await?.id, + }; let trace_id = current_trace_id(); tracing::Span::current().record("runtimedb.query_run_id", &query_run_id); @@ -643,7 +650,7 @@ impl RuntimeEngine { self.catalog .create_query_run(CreateQueryRun { id: &query_run_id, - snapshot_id: &snapshot.id, + snapshot_id: &snapshot_id, trace_id: trace_id.as_deref(), saved_query_id, saved_query_version, @@ -1034,11 +1041,33 @@ impl RuntimeEngine { &self, id: &str, name: Option<&str>, - sql: &str, + sql: Option<&str>, ) -> Result> { - let snapshot = self.catalog.get_or_create_snapshot(sql).await?; + let snapshot_id = match sql { + Some(sql) => self.catalog.get_or_create_snapshot(sql).await?.id, + None => { + // No SQL provided — resolve the current version's snapshot_id + let sq = self.catalog.get_saved_query(id).await?; + let sq = match sq { + Some(sq) => sq, + None => return Ok(None), + }; + let version = self + .catalog + .get_saved_query_version(id, sq.latest_version) + .await? + .ok_or_else(|| { + anyhow::anyhow!( + "Latest version {} missing for saved query '{}'", + sq.latest_version, + id + ) + })?; + version.snapshot_id + } + }; self.catalog - .update_saved_query(id, name, &snapshot.id) + .update_saved_query(id, name, &snapshot_id) .await } @@ -1095,11 +1124,13 @@ impl RuntimeEngine { )) })?; - // Execute with saved query linkage + // Execute with saved query linkage, passing the already-resolved snapshot_id + // to avoid a redundant get_or_create_snapshot round-trip. self.execute_query_with_persistence_inner( &version_record.sql_text, Some(saved_query_id), Some(target_version), + Some(&version_record.snapshot_id), ) .await } diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs index da20cee..c09f74e 100644 --- a/src/http/controllers/saved_queries_controller.rs +++ b/src/http/controllers/saved_queries_controller.rs @@ -159,7 +159,10 @@ pub async fn update_saved_query( Path(id): Path, Json(request): Json, ) -> Result, ApiError> { - let raw_too_large = request.sql.len() > MAX_RAW_INPUT_LENGTH + let raw_too_large = request + .sql + .as_ref() + .is_some_and(|s| s.len() > MAX_RAW_INPUT_LENGTH) || request .name .as_ref() @@ -170,15 +173,17 @@ pub async fn update_saved_query( )); } - let sql = request.sql.trim().to_string(); - if sql.is_empty() { - return Err(ApiError::bad_request("SQL cannot be empty")); - } - if sql.len() > MAX_SQL_LENGTH { - return Err(ApiError::bad_request(format!( - "SQL exceeds maximum length of {} characters", - MAX_SQL_LENGTH - ))); + let trimmed_sql = request.sql.as_deref().map(str::trim); + if let Some(sql) = trimmed_sql { + if sql.is_empty() { + return Err(ApiError::bad_request("SQL cannot be empty")); + } + if sql.len() > MAX_SQL_LENGTH { + return Err(ApiError::bad_request(format!( + "SQL exceeds maximum length of {} characters", + MAX_SQL_LENGTH + ))); + } } let trimmed_name = request.name.as_deref().map(str::trim); @@ -194,8 +199,14 @@ pub async fn update_saved_query( } } + if trimmed_name.is_none() && trimmed_sql.is_none() { + return Err(ApiError::bad_request( + "At least one of 'name' or 'sql' must be provided", + )); + } + let saved_query = engine - .update_saved_query(&id, trimmed_name, &sql) + .update_saved_query(&id, trimmed_name, trimmed_sql) .await .map_err(|e| -> ApiError { e.into() })? .ok_or_else(|| ApiError::not_found(format!("Saved query '{}' not found", id)))?; @@ -285,6 +296,7 @@ pub async fn list_saved_query_versions( } /// Handler for POST /v1/queries/{id}/execute +#[tracing::instrument(name = "http_execute_saved_query", skip(engine, body))] pub async fn execute_saved_query( State(engine): State>, Path(id): Path, diff --git a/src/http/models.rs b/src/http/models.rs index 59fbf04..2d20c43 100644 --- a/src/http/models.rs +++ b/src/http/models.rs @@ -565,7 +565,9 @@ pub struct UpdateSavedQueryRequest { /// Optional new name. When omitted the existing name is preserved. #[serde(default)] pub name: Option, - pub sql: String, + /// Optional new SQL. When omitted the existing SQL is preserved. + #[serde(default)] + pub sql: Option, } /// Request body for POST /v1/queries/{id}/execute From 685319600d8f6b698b012644b7400d122084106d Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Thu, 12 Feb 2026 09:08:25 -0800 Subject: [PATCH 17/19] test(catalog): add sql-less rename and empty-body update tests --- tests/saved_query_tests.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/tests/saved_query_tests.rs b/tests/saved_query_tests.rs index 4b817a1..1669704 100644 --- a/tests/saved_query_tests.rs +++ b/tests/saved_query_tests.rs @@ -616,7 +616,7 @@ async fn test_name_only_rename_preserves_version() -> Result<()> { let created = create_saved_query(&router, "old name", "SELECT 1").await?; let id = created["id"].as_str().unwrap(); - // Rename without changing SQL + // Rename without providing SQL at all let uri = PATH_SAVED_QUERY.replace("{id}", id); let response = send_request( &router, @@ -625,7 +625,7 @@ async fn test_name_only_rename_preserves_version() -> Result<()> { .uri(&uri) .header("content-type", "application/json") .body(Body::from(serde_json::to_string( - &json!({ "name": "new name", "sql": "SELECT 1" }), + &json!({ "name": "new name" }), )?))?, ) .await?; @@ -763,3 +763,26 @@ async fn test_list_saved_queries_stable_with_duplicate_names() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_update_empty_body_rejected() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "empty update", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + // Send update with neither name nor sql + let uri = PATH_SAVED_QUERY.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("PUT") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&json!({}))?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} From 402fb3184a1d1be04b66c3ba1276cbe87e8bf1de Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 18 Feb 2026 11:56:48 -0800 Subject: [PATCH 18/19] refactor(http): extract detail helper, add tracing to all handlers --- .../controllers/saved_queries_controller.rs | 49 +++++++++---------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/http/controllers/saved_queries_controller.rs b/src/http/controllers/saved_queries_controller.rs index c09f74e..1fd7cc2 100644 --- a/src/http/controllers/saved_queries_controller.rs +++ b/src/http/controllers/saved_queries_controller.rs @@ -1,3 +1,4 @@ +use crate::catalog::{SavedQuery, SavedQueryVersion}; use crate::http::controllers::query_controller::serialize_batches; use crate::http::error::ApiError; use crate::http::models::{ @@ -23,7 +24,20 @@ const MAX_SQL_LENGTH: usize = 1_000_000; /// valid after trimming since the post-trim limits are well below this. const MAX_RAW_INPUT_LENGTH: usize = 2_000_000; +fn build_saved_query_detail(sq: SavedQuery, version: SavedQueryVersion) -> SavedQueryDetail { + SavedQueryDetail { + id: sq.id, + name: sq.name, + latest_version: sq.latest_version, + sql: version.sql_text, + sql_hash: version.sql_hash, + created_at: sq.created_at, + updated_at: sq.updated_at, + } +} + /// Handler for POST /v1/queries +#[tracing::instrument(name = "http_create_saved_query", skip(engine, request))] pub async fn create_saved_query( State(engine): State>, Json(request): Json, @@ -74,19 +88,12 @@ pub async fn create_saved_query( Ok(( StatusCode::CREATED, - Json(SavedQueryDetail { - id: saved_query.id, - name: saved_query.name, - latest_version: saved_query.latest_version, - sql: version.sql_text, - sql_hash: version.sql_hash, - created_at: saved_query.created_at, - updated_at: saved_query.updated_at, - }), + Json(build_saved_query_detail(saved_query, version)), )) } /// Handler for GET /v1/queries +#[tracing::instrument(name = "http_list_saved_queries", skip(engine))] pub async fn list_saved_queries( State(engine): State>, QueryParams(params): QueryParams, @@ -121,6 +128,7 @@ pub async fn list_saved_queries( } /// Handler for GET /v1/queries/{id} +#[tracing::instrument(name = "http_get_saved_query", skip(engine))] pub async fn get_saved_query( State(engine): State>, Path(id): Path, @@ -142,18 +150,11 @@ pub async fn get_saved_query( )) })?; - Ok(Json(SavedQueryDetail { - id: saved_query.id, - name: saved_query.name, - latest_version: saved_query.latest_version, - sql: version.sql_text, - sql_hash: version.sql_hash, - created_at: saved_query.created_at, - updated_at: saved_query.updated_at, - })) + Ok(Json(build_saved_query_detail(saved_query, version))) } /// Handler for PUT /v1/queries/{id} — creates a new version, optionally renames +#[tracing::instrument(name = "http_update_saved_query", skip(engine, request))] pub async fn update_saved_query( State(engine): State>, Path(id): Path, @@ -222,18 +223,11 @@ pub async fn update_saved_query( )) })?; - Ok(Json(SavedQueryDetail { - id: saved_query.id, - name: saved_query.name, - latest_version: saved_query.latest_version, - sql: version.sql_text, - sql_hash: version.sql_hash, - created_at: saved_query.created_at, - updated_at: saved_query.updated_at, - })) + Ok(Json(build_saved_query_detail(saved_query, version))) } /// Handler for DELETE /v1/queries/{id} +#[tracing::instrument(name = "http_delete_saved_query", skip(engine))] pub async fn delete_saved_query( State(engine): State>, Path(id): Path, @@ -254,6 +248,7 @@ pub async fn delete_saved_query( } /// Handler for GET /v1/queries/{id}/versions +#[tracing::instrument(name = "http_list_saved_query_versions", skip(engine))] pub async fn list_saved_query_versions( State(engine): State>, Path(id): Path, From e271ceaba1ec5006ee165f0b2c256cee01b83d6c Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 18 Feb 2026 11:56:54 -0800 Subject: [PATCH 19/19] test(catalog): add overflow, pagination, nonexistent version tests --- tests/catalog_manager_suite.rs | 87 +++++++++++++++++++++++++++++++++ tests/saved_query_tests.rs | 88 ++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) diff --git a/tests/catalog_manager_suite.rs b/tests/catalog_manager_suite.rs index 48a928e..9b8df93 100644 --- a/tests/catalog_manager_suite.rs +++ b/tests/catalog_manager_suite.rs @@ -1299,6 +1299,51 @@ macro_rules! catalog_manager_tests { assert_eq!(run.snapshot_id, snap.id); } + #[tokio::test] + async fn saved_query_version_list_pagination() { + let ctx = super::$setup_fn().await; + let catalog = ctx.manager(); + + let snap1 = catalog.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = catalog + .create_saved_query("paginated", &snap1.id) + .await + .unwrap(); + + // Create versions 2..5 + for i in 2..=5 { + let snap = catalog + .get_or_create_snapshot(&format!("SELECT {}", i)) + .await + .unwrap(); + catalog + .update_saved_query(&sq.id, None, &snap.id) + .await + .unwrap(); + } + + // Page 1: newest 3 versions (5, 4, 3) + let (page1, has_more) = catalog + .list_saved_query_versions(&sq.id, 3, 0) + .await + .unwrap(); + assert_eq!(page1.len(), 3); + assert!(has_more); + assert_eq!(page1[0].version, 5); + assert_eq!(page1[1].version, 4); + assert_eq!(page1[2].version, 3); + + // Page 2: remaining 2 versions (2, 1) + let (page2, has_more2) = catalog + .list_saved_query_versions(&sq.id, 3, 3) + .await + .unwrap(); + assert_eq!(page2.len(), 2); + assert!(!has_more2); + assert_eq!(page2[0].version, 2); + assert_eq!(page2[1].version, 1); + } + } }; } @@ -1344,6 +1389,48 @@ async fn sqlite_concurrent_saved_query_updates() { assert_eq!(final_sq.latest_version, 3); } +/// Test that updating a saved query at i32::MAX version returns an error +/// instead of overflowing. +#[tokio::test] +async fn sqlite_version_overflow_returns_error() { + let dir = TempDir::new().expect("failed to create temp dir"); + let db_path = dir.path().join("catalog.sqlite"); + let db_uri = format!("sqlite:{}?mode=rwc", db_path.display()); + + let manager = SqliteCatalogManager::new(db_path.to_str().unwrap()) + .await + .unwrap(); + manager.run_migrations().await.unwrap(); + + let snap1 = manager.get_or_create_snapshot("SELECT 1").await.unwrap(); + let sq = manager + .create_saved_query("overflow-test", &snap1.id) + .await + .unwrap(); + assert_eq!(sq.latest_version, 1); + + // Directly set latest_version to i32::MAX via raw SQL + let pool = SqlitePool::connect(&db_uri).await.unwrap(); + sqlx::query("UPDATE saved_queries SET latest_version = ? WHERE id = ?") + .bind(i32::MAX) + .bind(&sq.id) + .execute(&pool) + .await + .unwrap(); + pool.close().await; + + // Attempt to create a new version — should fail, not overflow + let snap2 = manager.get_or_create_snapshot("SELECT 2").await.unwrap(); + let result = manager.update_saved_query(&sq.id, None, &snap2.id).await; + assert!(result.is_err(), "Should fail on version overflow"); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("Version limit reached"), + "Expected 'Version limit reached' error, got: {}", + err + ); +} + /// Test that migration hash mismatch is detected on startup. /// This simulates the scenario where a migration SQL file was modified after /// being applied to a database. diff --git a/tests/saved_query_tests.rs b/tests/saved_query_tests.rs index 1669704..89c5bef 100644 --- a/tests/saved_query_tests.rs +++ b/tests/saved_query_tests.rs @@ -786,3 +786,91 @@ async fn test_update_empty_body_rejected() -> Result<()> { assert_eq!(response.status(), StatusCode::BAD_REQUEST); Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_saved_query_nonexistent_version() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "version test", "SELECT 1 as num").await?; + let id = created["id"].as_str().unwrap(); + + // Try to execute version 99 which doesn't exist + let uri = PATH_SAVED_QUERY_EXECUTE.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("POST") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "version": 99 }), + )?))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_list_saved_query_versions_pagination() -> Result<()> { + let (router, _dir) = setup_test().await?; + + let created = create_saved_query(&router, "paginated", "SELECT 1").await?; + let id = created["id"].as_str().unwrap(); + + // Create versions 2..5 + let update_uri = PATH_SAVED_QUERY.replace("{id}", id); + for i in 2..=5 { + send_request( + &router, + Request::builder() + .method("PUT") + .uri(&update_uri) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string( + &json!({ "sql": format!("SELECT {}", i) }), + )?))?, + ) + .await?; + } + + // Page 1: limit=3, offset=0 → versions 5, 4, 3 + let versions_uri = PATH_SAVED_QUERY_VERSIONS.replace("{id}", id); + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(format!("{}?limit=3&offset=0", versions_uri)) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + assert_eq!(json["count"], 3); + assert!(json["has_more"].as_bool().unwrap()); + let versions = json["versions"].as_array().unwrap(); + assert_eq!(versions[0]["version"], 5); + assert_eq!(versions[2]["version"], 3); + + // Page 2: limit=3, offset=3 → versions 2, 1 + let response = send_request( + &router, + Request::builder() + .method("GET") + .uri(format!("{}?limit=3&offset=3", versions_uri)) + .body(Body::empty())?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + let json = response_json(response).await?; + assert_eq!(json["count"], 2); + assert!(!json["has_more"].as_bool().unwrap()); + let versions = json["versions"].as_array().unwrap(); + assert_eq!(versions[0]["version"], 2); + assert_eq!(versions[1]["version"], 1); + + Ok(()) +}