From 28a0b0c4f6c46538920c29275b1e612bd3a6d40d Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Thu, 4 Jun 2026 14:18:29 +0200 Subject: [PATCH 1/4] feat(drivers): derive pagination LIMIT/OFFSET from a SQL parser The grid pagination rewriter read the user's LIMIT/OFFSET with a hand-rolled token scanner that only recognised the trailing `LIMIT OFFSET ` shape. It mis-handled MySQL's `LIMIT , `, `OFFSET` before `LIMIT`, and numeric expressions, producing wrong values or a malformed appended query. Parse the query with sqlparser using the driver's dialect and read LIMIT/OFFSET from the AST instead, normalising the MySQL comma form. The trailing clause is stripped at its keyword (consistent with what the parser saw) and the new pagination clause is rendered from a LimitClause AST node, then concatenated to the verbatim sliced base so leading comments, inline hints, and formatting are preserved. The token scanner is kept as a fallback for inputs the parser rejects, so behaviour never regresses. FETCH FIRST ... ROWS is out of scope and defers to the fallback. Builds on #275. --- src-tauri/Cargo.lock | 74 ++++++++++- src-tauri/Cargo.toml | 1 + src-tauri/src/drivers/common.rs | 4 +- src-tauri/src/drivers/common/query.rs | 182 +++++++++++++++++++++++++- src-tauri/src/drivers/common/tests.rs | 85 ++++++++++-- src-tauri/src/drivers/mysql/mod.rs | 7 +- src-tauri/src/drivers/postgres/mod.rs | 7 +- src-tauri/src/drivers/sqlite/mod.rs | 7 +- 8 files changed, 342 insertions(+), 25 deletions(-) diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index e8000030..651487de 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -166,6 +166,15 @@ version = "1.0.102" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +[[package]] +name = "ar_archive_writer" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7eb93bbb63b9c227414f6eb3a0adfddca591a8ce1e9b60661bb08969b87e340b" +dependencies = [ + "object", +] + [[package]] name = "arbitrary" version = "1.4.2" @@ -3846,6 +3855,15 @@ dependencies = [ "objc2-security", ] +[[package]] +name = "object" +version = "0.37.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ff76201f031d8863c38aa7f905eca4f53abbfa15f609db4277d44cd8938f33fe" +dependencies = [ + "memchr", +] + [[package]] name = "once_cell" version = "1.21.3" @@ -4575,6 +4593,16 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "psm" +version = "0.1.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "645dbe486e346d9b5de3ef16ede18c26e6c70ad97418f4874b8b1889d6e761ea" +dependencies = [ + "ar_archive_writer", + "cc", +] + [[package]] name = "ptr_meta" version = "0.1.4" @@ -4835,6 +4863,26 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "recursive" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0786a43debb760f491b1bc0269fe5e84155353c67482b9e60d0cfb596054b43e" +dependencies = [ + "recursive-proc-macro-impl", + "stacker", +] + +[[package]] +name = "recursive-proc-macro-impl" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76009fbe0614077fc1a2ce255e3a1881a2e3a3527097d5dc6d8212c585e7e38b" +dependencies = [ + "quote", + "syn 2.0.117", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -5825,6 +5873,16 @@ dependencies = [ "der", ] +[[package]] +name = "sqlparser" +version = "0.62.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13c6d1b651dc4edf07eead2a0c6c78016ce971bc2c10da5266861b13f25e7cec" +dependencies = [ + "log", + "recursive", +] + [[package]] name = "sqlx" version = "0.8.6" @@ -6031,6 +6089,19 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" +[[package]] +name = "stacker" +version = "0.1.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "640c8cdd92b6b12f5bcb1803ca3bbf5ab96e5e6b6b96b9ab77dabe9e880b3190" +dependencies = [ + "cc", + "cfg-if", + "libc", + "psm", + "windows-sys 0.61.2", +] + [[package]] name = "static_assertions" version = "1.1.0" @@ -6188,7 +6259,7 @@ dependencies = [ [[package]] name = "tabularis" -version = "0.12.0" +version = "0.13.0" dependencies = [ "async-trait", "base64 0.22.1", @@ -6218,6 +6289,7 @@ dependencies = [ "serde_json", "serde_yaml", "sha2", + "sqlparser", "sqlx", "sysinfo", "tauri", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 90524788..d8b42007 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -68,6 +68,7 @@ rustls-pemfile = "2" rustls-platform-verifier = "0.6" notify = "6" ulid = "1.2.1" +sqlparser = "0.62" # GTK dependencies for Wayland window title workaround (Linux only) [target.'cfg(target_os = "linux")'.dependencies] diff --git a/src-tauri/src/drivers/common.rs b/src-tauri/src/drivers/common.rs index 281659fb..de6540be 100644 --- a/src-tauri/src/drivers/common.rs +++ b/src-tauri/src/drivers/common.rs @@ -11,8 +11,8 @@ pub use blob::{ }; pub use query::{ build_paginated_query, calculate_offset, extract_user_limit, extract_user_offset, - is_explainable_query, - is_select_query, returns_result_set, strip_leading_sql_comments, strip_limit_offset, + is_explainable_query, is_select_query, returns_result_set, strip_leading_sql_comments, + strip_limit_offset, PaginationDialect, }; pub use safe_int::{ i64_to_json, parse_unsafe_bigint_string, u64_to_json, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, diff --git a/src-tauri/src/drivers/common/query.rs b/src-tauri/src/drivers/common/query.rs index 4d09e4ca..86df1ecd 100644 --- a/src-tauri/src/drivers/common/query.rs +++ b/src-tauri/src/drivers/common/query.rs @@ -1,3 +1,7 @@ +use sqlparser::ast::{Expr, LimitClause, Offset, OffsetRows, Statement, Value}; +use sqlparser::dialect::{Dialect, MySqlDialect, PostgreSqlDialect, SQLiteDialect}; +use sqlparser::parser::Parser; + /// Check if a query is a SELECT statement. /// /// Leading SQL comments are stripped before checking, matching @@ -281,22 +285,177 @@ pub fn extract_user_offset(query: &str) -> Option { None } +/// SQL dialect used to parse a query before rewriting its pagination. +/// +/// Driver-facing wrapper so the `sqlparser` dialect types stay internal to +/// this module — each driver passes the variant matching its engine. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PaginationDialect { + MySql, + Postgres, + Sqlite, +} + +impl PaginationDialect { + fn parser_dialect(self) -> Box { + match self { + PaginationDialect::MySql => Box::new(MySqlDialect {}), + PaginationDialect::Postgres => Box::new(PostgreSqlDialect {}), + PaginationDialect::Sqlite => Box::new(SQLiteDialect {}), + } + } +} + +/// LIMIT/OFFSET read from a query's parsed AST. +struct ParsedPagination { + /// User-supplied LIMIT, if it is a plain numeric literal. + user_limit: Option, + /// User-supplied OFFSET (0 when absent or non-numeric). + user_offset: u32, + /// Whether the query carries a top-level LIMIT/OFFSET clause to strip. + has_limit_clause: bool, +} + +/// Resolve a numeric-literal expression to a `u32`. +/// +/// Non-literal expressions (placeholders, arithmetic, bind parameters) yield +/// `None` so they are treated as "no value" rather than being mis-read. +fn eval_u32(expr: &Expr) -> Option { + match expr { + Expr::Value(value) => match &value.value { + Value::Number(n, _) => n.parse().ok(), + _ => None, + }, + _ => None, + } +} + +/// Parse `query` with `dialect` and read the top-level LIMIT/OFFSET from the +/// AST, normalising MySQL's `LIMIT , ` form to the same shape. +/// +/// Returns `None` — so the caller falls back to the token heuristics — when +/// the input is not a single query the parser understands, or when it relies +/// on a `FETCH FIRST … ROWS` clause this rewriter does not handle. +fn parse_pagination(query: &str, dialect: PaginationDialect) -> Option { + let statements = Parser::parse_sql(dialect.parser_dialect().as_ref(), query).ok()?; + if statements.len() != 1 { + return None; + } + let Statement::Query(q) = &statements[0] else { + return None; + }; + + // FETCH FIRST … ROWS ONLY is out of scope; defer to the fallback path so + // its behaviour is unchanged rather than producing a mixed clause. + if q.fetch.is_some() { + return None; + } + + match &q.limit_clause { + None => Some(ParsedPagination { + user_limit: None, + user_offset: 0, + has_limit_clause: false, + }), + Some(LimitClause::LimitOffset { limit, offset, .. }) => Some(ParsedPagination { + user_limit: limit.as_ref().and_then(eval_u32), + user_offset: offset + .as_ref() + .and_then(|o| eval_u32(&o.value)) + .unwrap_or(0), + has_limit_clause: true, + }), + Some(LimitClause::OffsetCommaLimit { offset, limit }) => Some(ParsedPagination { + user_limit: eval_u32(limit), + user_offset: eval_u32(offset).unwrap_or(0), + has_limit_clause: true, + }), + } +} + +/// True for the keyword/value tokens that make up a trailing LIMIT/OFFSET +/// clause (standard, MySQL comma, and FETCH spellings), so a LIMIT/OFFSET that +/// appears mid-query is never mistaken for the trailing pagination clause. +fn is_pagination_tail_token(tok: &str) -> bool { + let upper = tok.to_uppercase(); + matches!( + upper.as_str(), + "LIMIT" | "OFFSET" | "BY" | "ROW" | "ROWS" | "ONLY" | "NEXT" | "FIRST" | "," + ) || (!tok.is_empty() && tok.trim_end_matches(',').chars().all(|c| c.is_ascii_digit())) +} + +/// Cut the query immediately before its trailing top-level LIMIT/OFFSET clause. +/// +/// Reuses [`tokenize_sql_with_pos`], which collapses parenthesised groups into +/// a single token, so a LIMIT/OFFSET inside a subquery is never seen and only +/// the outer clause is removed. Unlike [`strip_limit_offset`], it cuts at the +/// keyword regardless of the value shape, so MySQL's `LIMIT , ` +/// is handled. Falls back to [`strip_limit_offset`] if no trailing clause is +/// found (kept total; the parser having reported a clause makes this rare). +fn strip_at_limit_keyword(query: &str) -> String { + let trimmed = query.trim_end(); + let tokens = tokenize_sql_with_pos(trimmed); + for (idx, (tok, pos)) in tokens.iter().enumerate() { + let upper = tok.to_uppercase(); + if (upper == "LIMIT" || upper == "OFFSET") + && tokens[idx + 1..] + .iter() + .all(|(t, _)| is_pagination_tail_token(t)) + { + return trimmed[..*pos].trim_end().to_string(); + } + } + strip_limit_offset(query) +} + +/// Build a numeric-literal expression for the rendered pagination clause. +fn number_expr(n: u32) -> Expr { + Expr::value(Value::Number(n.to_string(), false)) +} + /// Build a paginated query by stripping any user-supplied LIMIT/OFFSET and /// appending pagination clauses directly. ORDER BY is left in place so that /// table-qualified column references (e.g. `o.created_at`) remain valid — /// wrapping the original query in a subquery would move those references out /// of scope and cause "unknown column" errors. /// +/// The user's LIMIT/OFFSET are read from the parsed AST (using `dialect`), so +/// dialect-specific forms such as MySQL's `LIMIT , ` and +/// `OFFSET` before `LIMIT` are understood. When the parser cannot handle the +/// input, it falls back to a token-aware heuristic scan. The appended +/// pagination clause is rendered from a [`LimitClause`] AST node and +/// concatenated to the verbatim sliced base, so leading comments, inline +/// hints, and the body's formatting are preserved. +/// /// When the user wrote an explicit LIMIT, it is honoured as a cap on the total /// number of rows returned across all pages. A user-supplied OFFSET is honoured /// too: it is added to the per-page offset so that pagination walks the result /// set the user actually asked for (the rows after their OFFSET). Discarding it /// silently collapsed `LIMIT 1 OFFSET 1` to `LIMIT 1 OFFSET 0` on page 1. -pub fn build_paginated_query(query: &str, page_size: u32, page: u32) -> String { +pub fn build_paginated_query( + query: &str, + page_size: u32, + page: u32, + dialect: PaginationDialect, +) -> String { let page_offset = calculate_offset(page, page_size); - let user_limit = extract_user_limit(query); - let user_offset = extract_user_offset(query).unwrap_or(0); - let base = strip_limit_offset(query); + + let (user_limit, user_offset, base) = match parse_pagination(query, dialect) { + Some(parsed) => { + let base = if parsed.has_limit_clause { + strip_at_limit_keyword(query) + } else { + query.trim_end().to_string() + }; + (parsed.user_limit, parsed.user_offset, base) + } + // Parser could not handle the input — fall back to the token heuristics. + None => ( + extract_user_limit(query), + extract_user_offset(query).unwrap_or(0), + strip_limit_offset(query), + ), + }; let fetch_count = match user_limit { Some(ul) => { @@ -309,5 +468,18 @@ pub fn build_paginated_query(query: &str, page_size: u32, page: u32) -> String { let offset = user_offset.saturating_add(page_offset); - format!("{} LIMIT {} OFFSET {}", base, fetch_count, offset) + // Render the pagination clause from an AST node so the output is built by + // the parser rather than hand-formatted. + let clause = LimitClause::LimitOffset { + limit: Some(number_expr(fetch_count)), + offset: Some(Offset { + value: number_expr(offset), + rows: OffsetRows::None, + }), + limit_by: Vec::new(), + }; + + // `LimitClause`'s Display renders a leading space (it is meant to follow a + // preceding clause), so concatenate without inserting another one. + format!("{}{}", base, clause) } diff --git a/src-tauri/src/drivers/common/tests.rs b/src-tauri/src/drivers/common/tests.rs index 9fd46ec2..83b8c1c3 100644 --- a/src-tauri/src/drivers/common/tests.rs +++ b/src-tauri/src/drivers/common/tests.rs @@ -1,8 +1,8 @@ use super::{ build_paginated_query, decode_blob_wire_format, encode_blob, encode_blob_full, i64_to_json, is_explainable_query, is_select_query, parse_unsafe_bigint_string, strip_leading_sql_comments, - strip_limit_offset, u64_to_json, DEFAULT_MAX_BLOB_SIZE, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, - MAX_BLOB_PREVIEW_SIZE, + strip_limit_offset, u64_to_json, PaginationDialect, DEFAULT_MAX_BLOB_SIZE, JS_MAX_SAFE_INTEGER, + JS_MAX_SAFE_UINT, MAX_BLOB_PREVIEW_SIZE, }; #[test] @@ -277,7 +277,7 @@ fn test_extract_user_limit_table_name_contains_limit_with_real_limit() { #[test] fn test_build_paginated_query_no_user_limit() { let q = "SELECT o.id FROM orders o ORDER BY o.created_at DESC"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "SELECT o.id FROM orders o ORDER BY o.created_at DESC LIMIT 101 OFFSET 0" @@ -287,7 +287,7 @@ fn test_build_paginated_query_no_user_limit() { #[test] fn test_build_paginated_query_replaces_user_limit() { let q = "SELECT * FROM t ORDER BY id LIMIT 50"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); // User wanted 50 rows. page_size=100, so remaining=50, fetch = min(50, 101) = 50 assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 50 OFFSET 0"); } @@ -295,7 +295,7 @@ fn test_build_paginated_query_replaces_user_limit() { #[test] fn test_build_paginated_query_user_limit_second_page() { let q = "SELECT * FROM t ORDER BY id LIMIT 250"; - let result = build_paginated_query(q, 100, 2); + let result = build_paginated_query(q, 100, 2, PaginationDialect::Postgres); // offset=100, remaining=150, fetch = min(150, 101) = 101 assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 100"); } @@ -303,7 +303,7 @@ fn test_build_paginated_query_user_limit_second_page() { #[test] fn test_build_paginated_query_user_limit_exhausted() { let q = "SELECT * FROM t LIMIT 50"; - let result = build_paginated_query(q, 100, 2); + let result = build_paginated_query(q, 100, 2, PaginationDialect::Postgres); // offset=100, remaining=0 (50-100 saturates to 0), fetch = min(0, 101) = 0 assert_eq!(result, "SELECT * FROM t LIMIT 0 OFFSET 100"); } @@ -311,7 +311,7 @@ fn test_build_paginated_query_user_limit_exhausted() { #[test] fn test_build_paginated_query_table_name_contains_limit() { let q = "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id LIMIT 101 OFFSET 0" @@ -321,7 +321,7 @@ fn test_build_paginated_query_table_name_contains_limit() { #[test] fn test_build_paginated_query_table_name_contains_limit_with_user_limit() { let q = "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id LIMIT 10"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id LIMIT 10 OFFSET 0" @@ -357,7 +357,7 @@ fn test_build_paginated_query_preserves_user_offset() { // Regression for #273: `LIMIT 1 OFFSET 1` must keep OFFSET 1 on page 1, // not collapse to OFFSET 0 (which returned the 1st row instead of the 2nd). let q = "SELECT DISTINCT salary FROM employees ORDER BY salary DESC LIMIT 1 OFFSET 1"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "SELECT DISTINCT salary FROM employees ORDER BY salary DESC LIMIT 1 OFFSET 1" @@ -367,7 +367,7 @@ fn test_build_paginated_query_preserves_user_offset() { #[test] fn test_build_paginated_query_user_offset_no_limit() { let q = "SELECT * FROM t ORDER BY id OFFSET 5"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 5"); } @@ -375,14 +375,14 @@ fn test_build_paginated_query_user_offset_no_limit() { fn test_build_paginated_query_user_offset_second_page() { // page offset (100) is added on top of the user's OFFSET (5). let q = "SELECT * FROM t ORDER BY id OFFSET 5"; - let result = build_paginated_query(q, 100, 2); + let result = build_paginated_query(q, 100, 2, PaginationDialect::Postgres); assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 105"); } #[test] fn test_build_paginated_query_subquery_with_limit() { let q = "SELECT * FROM (SELECT id FROM t ORDER BY id LIMIT 100) sub ORDER BY id LIMIT 5"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "SELECT * FROM (SELECT id FROM t ORDER BY id LIMIT 100) sub ORDER BY id LIMIT 5 OFFSET 0" @@ -392,7 +392,7 @@ fn test_build_paginated_query_subquery_with_limit() { #[test] fn test_build_paginated_query_with_leading_comments() { let q = "-- header\nSELECT * FROM t ORDER BY id"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "-- header\nSELECT * FROM t ORDER BY id LIMIT 101 OFFSET 0" @@ -405,13 +405,70 @@ fn test_build_paginated_query_multiline_comments_with_user_limit() { // appended `LIMIT … OFFSET …` lands on its own line rather than // being swallowed into the `--` header as comment text. let q = "-- ============\n-- title\n-- ============\n\nSELECT * FROM t ORDER BY id LIMIT 50"; - let result = build_paginated_query(q, 100, 1); + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); assert_eq!( result, "-- ============\n-- title\n-- ============\n\nSELECT * FROM t ORDER BY id LIMIT 50 OFFSET 0" ); } +#[test] +fn test_build_paginated_query_mysql_comma_limit() { + // MySQL `LIMIT , ` — the parser normalises it so the offset + // (10) folds into the page offset and the count (5) caps the fetch. + let q = "SELECT * FROM t LIMIT 10, 5"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::MySql); + assert_eq!(result, "SELECT * FROM t LIMIT 5 OFFSET 10"); +} + +#[test] +fn test_build_paginated_query_mysql_comma_limit_second_page() { + let q = "SELECT * FROM t LIMIT 10, 250"; + let result = build_paginated_query(q, 100, 2, PaginationDialect::MySql); + // user_offset=10, user_limit=250, page_offset=100. + // fetch = min(250-100, 101) = 101, offset = 10 + 100 = 110 + assert_eq!(result, "SELECT * FROM t LIMIT 101 OFFSET 110"); +} + +#[test] +fn test_build_paginated_query_offset_before_limit() { + // Postgres allows OFFSET before LIMIT; both must be stripped and folded. + let q = "SELECT * FROM t ORDER BY id OFFSET 5 LIMIT 10"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 10 OFFSET 5"); +} + +#[test] +fn test_build_paginated_query_mysql_backtick_identifier() { + let q = "SELECT * FROM `orders` ORDER BY `id` LIMIT 10"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::MySql); + assert_eq!( + result, + "SELECT * FROM `orders` ORDER BY `id` LIMIT 10 OFFSET 0" + ); +} + +#[test] +fn test_build_paginated_query_preserves_inline_hint() { + // The body is sliced verbatim, so an inline comment/optimizer hint is kept + // even though the parser itself discards comments. + let q = "SELECT /*+ MAX_EXECUTION_TIME(1000) */ * FROM t LIMIT 5"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::MySql); + assert_eq!( + result, + "SELECT /*+ MAX_EXECUTION_TIME(1000) */ * FROM t LIMIT 5 OFFSET 0" + ); +} + +#[test] +fn test_build_paginated_query_falls_back_on_parse_error() { + // The parser rejects this (dangling LIMIT after WHERE), so the rewriter + // falls back to the token heuristics and still strips the trailing LIMIT. + let q = "SELECT * FROM t WHERE LIMIT 5"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t WHERE LIMIT 5 OFFSET 0"); +} + #[test] fn test_strip_limit_offset_preserves_leading_comments() { assert_eq!( diff --git a/src-tauri/src/drivers/mysql/mod.rs b/src-tauri/src/drivers/mysql/mod.rs index bd2ce962..53419c50 100644 --- a/src-tauri/src/drivers/mysql/mod.rs +++ b/src-tauri/src/drivers/mysql/mod.rs @@ -948,7 +948,12 @@ async fn exec_on_mysql_conn( if is_select && limit.is_some() { let l = limit.unwrap(); - final_query = crate::drivers::common::build_paginated_query(query, l, page); + final_query = crate::drivers::common::build_paginated_query( + query, + l, + page, + crate::drivers::common::PaginationDialect::MySql, + ); pagination = Some(Pagination { page, diff --git a/src-tauri/src/drivers/postgres/mod.rs b/src-tauri/src/drivers/postgres/mod.rs index 410432af..f6b1265a 100644 --- a/src-tauri/src/drivers/postgres/mod.rs +++ b/src-tauri/src/drivers/postgres/mod.rs @@ -810,7 +810,12 @@ async fn exec_on_pg_client( let (final_query, pagination_meta) = if is_select && limit.is_some() { let l = limit.unwrap(); - let data_query = crate::drivers::common::build_paginated_query(query, l, page); + let data_query = crate::drivers::common::build_paginated_query( + query, + l, + page, + crate::drivers::common::PaginationDialect::Postgres, + ); manual_limit = None; (data_query, Some((l, page))) } else { diff --git a/src-tauri/src/drivers/sqlite/mod.rs b/src-tauri/src/drivers/sqlite/mod.rs index 531ddedc..b040326f 100644 --- a/src-tauri/src/drivers/sqlite/mod.rs +++ b/src-tauri/src/drivers/sqlite/mod.rs @@ -540,7 +540,12 @@ async fn exec_on_sqlite_conn( if is_select && limit.is_some() { let l = limit.unwrap(); - final_query = crate::drivers::common::build_paginated_query(query, l, page); + final_query = crate::drivers::common::build_paginated_query( + query, + l, + page, + crate::drivers::common::PaginationDialect::Sqlite, + ); pagination = Some(Pagination { page, From 5bcdac209486c50583ef97c21ea78312b5b36974 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Thu, 4 Jun 2026 15:15:49 +0200 Subject: [PATCH 2/4] feat: attach executed SQL to DB errors and show in UI --- src-tauri/src/drivers/common.rs | 6 ++-- src-tauri/src/drivers/common/query.rs | 32 ++++++++++++++++++- src-tauri/src/drivers/common/tests.rs | 44 ++++++++++++++++++++++++--- src-tauri/src/drivers/mysql/mod.rs | 8 ++++- src-tauri/src/drivers/postgres/mod.rs | 14 +++++++-- src-tauri/src/drivers/sqlite/mod.rs | 8 ++++- src/components/ui/ErrorDisplay.tsx | 44 ++++++++++++++++++++++++--- src/i18n/locales/de.json | 2 ++ src/i18n/locales/en.json | 2 ++ src/i18n/locales/es.json | 2 ++ src/i18n/locales/fr.json | 2 ++ src/i18n/locales/it.json | 2 ++ src/i18n/locales/ja.json | 2 ++ src/i18n/locales/ru.json | 2 ++ src/i18n/locales/zh.json | 2 ++ 15 files changed, 156 insertions(+), 16 deletions(-) diff --git a/src-tauri/src/drivers/common.rs b/src-tauri/src/drivers/common.rs index de6540be..d238b965 100644 --- a/src-tauri/src/drivers/common.rs +++ b/src-tauri/src/drivers/common.rs @@ -10,9 +10,9 @@ pub use blob::{ DEFAULT_MAX_BLOB_SIZE, MAX_BLOB_PREVIEW_SIZE, }; pub use query::{ - build_paginated_query, calculate_offset, extract_user_limit, extract_user_offset, - is_explainable_query, is_select_query, returns_result_set, strip_leading_sql_comments, - strip_limit_offset, PaginationDialect, + annotate_error_with_query, build_paginated_query, calculate_offset, extract_user_limit, + extract_user_offset, is_explainable_query, is_select_query, returns_result_set, + strip_leading_sql_comments, strip_limit_offset, PaginationDialect, EXECUTED_QUERY_MARKER, }; pub use safe_int::{ i64_to_json, parse_unsafe_bigint_string, u64_to_json, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, diff --git a/src-tauri/src/drivers/common/query.rs b/src-tauri/src/drivers/common/query.rs index 86df1ecd..4590c97f 100644 --- a/src-tauri/src/drivers/common/query.rs +++ b/src-tauri/src/drivers/common/query.rs @@ -376,12 +376,17 @@ fn parse_pagination(query: &str, dialect: PaginationDialect) -> Option,` is recognised even when written without spaces +/// (`LIMIT 0,1`): the tokenizer splits only on whitespace, so the count and +/// offset arrive glued together as a single `0,1` token. fn is_pagination_tail_token(tok: &str) -> bool { let upper = tok.to_uppercase(); matches!( upper.as_str(), "LIMIT" | "OFFSET" | "BY" | "ROW" | "ROWS" | "ONLY" | "NEXT" | "FIRST" | "," - ) || (!tok.is_empty() && tok.trim_end_matches(',').chars().all(|c| c.is_ascii_digit())) + ) || (!tok.is_empty() && tok.chars().all(|c| c.is_ascii_digit() || c == ',')) } /// Cut the query immediately before its trailing top-level LIMIT/OFFSET clause. @@ -483,3 +488,28 @@ pub fn build_paginated_query( // preceding clause), so concatenate without inserting another one. format!("{}{}", base, clause) } + +/// Sentinel that separates a human error message from the actual SQL that +/// produced it inside a single error string. The leading code point is from +/// the Unicode private-use area, so it never appears in real SQL text or DB +/// driver error messages and survives JSON/IPC transport unescaped — letting +/// the frontend split on this marker without colliding with the `\n\n` +/// brief/detail convention. Must stay in sync with the parser in +/// `src/components/ui/ErrorDisplay.tsx`. +pub const EXECUTED_QUERY_MARKER: &str = "\u{E000}__TABULARIS_EXECUTED_QUERY__"; + +/// Appends the executed SQL to a DB error message so the UI can show the user +/// the exact statement that failed. This matters when pagination rewrote the +/// query (appending `LIMIT`/`OFFSET`): the database complains about clauses the +/// user never typed, and without the rewritten text the error is baffling. +/// +/// When `executed` matches `original` (ignoring surrounding whitespace) the +/// error is returned unchanged — echoing back the query the user can already +/// see would only add noise. +pub fn annotate_error_with_query(err: String, executed: &str, original: &str) -> String { + if executed.trim() == original.trim() { + err + } else { + format!("{err}{EXECUTED_QUERY_MARKER}{executed}") + } +} diff --git a/src-tauri/src/drivers/common/tests.rs b/src-tauri/src/drivers/common/tests.rs index 83b8c1c3..0c604548 100644 --- a/src-tauri/src/drivers/common/tests.rs +++ b/src-tauri/src/drivers/common/tests.rs @@ -1,10 +1,35 @@ use super::{ - build_paginated_query, decode_blob_wire_format, encode_blob, encode_blob_full, i64_to_json, - is_explainable_query, is_select_query, parse_unsafe_bigint_string, strip_leading_sql_comments, - strip_limit_offset, u64_to_json, PaginationDialect, DEFAULT_MAX_BLOB_SIZE, JS_MAX_SAFE_INTEGER, - JS_MAX_SAFE_UINT, MAX_BLOB_PREVIEW_SIZE, + annotate_error_with_query, build_paginated_query, decode_blob_wire_format, encode_blob, + encode_blob_full, i64_to_json, is_explainable_query, is_select_query, parse_unsafe_bigint_string, + strip_leading_sql_comments, strip_limit_offset, u64_to_json, PaginationDialect, + DEFAULT_MAX_BLOB_SIZE, EXECUTED_QUERY_MARKER, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, + MAX_BLOB_PREVIEW_SIZE, }; +#[test] +fn test_annotate_error_appends_rewritten_query() { + let err = "syntax error near 'LIMIT 1 OFFSET 0'".to_string(); + let executed = "SELECT * FROM t LIMIT 1 OFFSET 0"; + let original = "SELECT * FROM t"; + let out = annotate_error_with_query(err.clone(), executed, original); + + let (message, query) = out + .split_once(EXECUTED_QUERY_MARKER) + .expect("marker should be present when the query was rewritten"); + assert_eq!(message, err); + assert_eq!(query, executed); +} + +#[test] +fn test_annotate_error_noop_when_query_unchanged() { + let err = "permission denied".to_string(); + let q = "SELECT * FROM t"; + // Identical query (modulo surrounding whitespace) must not be appended. + let out = annotate_error_with_query(err.clone(), " SELECT * FROM t ", q); + assert_eq!(out, err); + assert!(!out.contains(EXECUTED_QUERY_MARKER)); +} + #[test] fn test_decode_blob_wire_format_valid() { // Encode some known bytes, then verify decode round-trips correctly @@ -421,6 +446,17 @@ fn test_build_paginated_query_mysql_comma_limit() { assert_eq!(result, "SELECT * FROM t LIMIT 5 OFFSET 10"); } +#[test] +fn test_build_paginated_query_mysql_comma_limit_no_space() { + // No space between offset and count (`LIMIT 0,1`). The whitespace tokenizer + // keeps `0,1` as a single token, so the trailing clause must still be + // recognised and stripped rather than producing the invalid + // `... LIMIT 0,1 LIMIT 1 OFFSET 0`. + let q = "select * from accounts LIMIT 0,1"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::MySql); + assert_eq!(result, "select * from accounts LIMIT 1 OFFSET 0"); +} + #[test] fn test_build_paginated_query_mysql_comma_limit_second_page() { let q = "SELECT * FROM t LIMIT 10, 250"; diff --git a/src-tauri/src/drivers/mysql/mod.rs b/src-tauri/src/drivers/mysql/mod.rs index 53419c50..bbe953b5 100644 --- a/src-tauri/src/drivers/mysql/mod.rs +++ b/src-tauri/src/drivers/mysql/mod.rs @@ -999,7 +999,13 @@ async fn exec_on_mysql_conn( } json_rows.push(json_row); } - Err(e) => return Err(e.to_string()), + Err(e) => { + return Err(crate::drivers::common::annotate_error_with_query( + e.to_string(), + &final_query, + query, + )) + } } } } // rows_stream dropped here — conn borrow released diff --git a/src-tauri/src/drivers/postgres/mod.rs b/src-tauri/src/drivers/postgres/mod.rs index f6b1265a..55ea9271 100644 --- a/src-tauri/src/drivers/postgres/mod.rs +++ b/src-tauri/src/drivers/postgres/mod.rs @@ -827,7 +827,11 @@ async fn exec_on_pg_client( client .query_raw(&final_query, &pg_params) .await - .map_err(|e| format_pg_error(&e))? + .map_err(|e| crate::drivers::common::annotate_error_with_query( + format_pg_error(&e), + &final_query, + query + ))? ); let mut columns: Vec = Vec::new(); @@ -856,7 +860,13 @@ async fn exec_on_pg_client( } json_rows.push(json_row); } - Err(e) => return Err(format_pg_error(&e)), + Err(e) => { + return Err(crate::drivers::common::annotate_error_with_query( + format_pg_error(&e), + &final_query, + query, + )) + } } } diff --git a/src-tauri/src/drivers/sqlite/mod.rs b/src-tauri/src/drivers/sqlite/mod.rs index b040326f..f747b896 100644 --- a/src-tauri/src/drivers/sqlite/mod.rs +++ b/src-tauri/src/drivers/sqlite/mod.rs @@ -589,7 +589,13 @@ async fn exec_on_sqlite_conn( } json_rows.push(json_row); } - Err(e) => return Err(e.to_string()), + Err(e) => { + return Err(crate::drivers::common::annotate_error_with_query( + e.to_string(), + &final_query, + query, + )) + } } } diff --git a/src/components/ui/ErrorDisplay.tsx b/src/components/ui/ErrorDisplay.tsx index a60160c1..abe120e7 100644 --- a/src/components/ui/ErrorDisplay.tsx +++ b/src/components/ui/ErrorDisplay.tsx @@ -7,13 +7,30 @@ interface ErrorDisplayProps { t: TFunction; } +// Sentinel used by the Rust drivers to attach the actually-executed SQL to an +// error string. Kept in sync with `EXECUTED_QUERY_MARKER` in +// `src-tauri/src/drivers/common/query.rs` (a Unicode private-use code point +// that never appears in real SQL or error text). +const EXECUTED_QUERY_MARKER = "\uE000__TABULARIS_EXECUTED_QUERY__"; + export function ErrorDisplay({ error, t }: ErrorDisplayProps) { const [showDetails, setShowDetails] = useState(false); + const [showQuery, setShowQuery] = useState(false); + + // Peel off the executed-query block (if the driver attached one) before + // applying the brief/detail split, so the SQL never leaks into the details. + const markerIndex = error.indexOf(EXECUTED_QUERY_MARKER); + const message = markerIndex === -1 ? error : error.slice(0, markerIndex); + const executedQuery = + markerIndex === -1 + ? "" + : error.slice(markerIndex + EXECUTED_QUERY_MARKER.length); - const separatorIndex = error.indexOf("\n\n"); - const hasDetails = separatorIndex !== -1 && separatorIndex < error.length - 2; - const brief = hasDetails ? error.slice(0, separatorIndex) : error; - const details = hasDetails ? error.slice(separatorIndex + 2) : ""; + const separatorIndex = message.indexOf("\n\n"); + const hasDetails = + separatorIndex !== -1 && separatorIndex < message.length - 2; + const brief = hasDetails ? message.slice(0, separatorIndex) : message; + const details = hasDetails ? message.slice(separatorIndex + 2) : ""; return (
@@ -37,6 +54,25 @@ export function ErrorDisplay({ error, t }: ErrorDisplayProps) { )} )} + {executedQuery && ( + <> + + {showQuery && ( + 
+              {executedQuery}
+
+ )} + + )}
); } diff --git a/src/i18n/locales/de.json b/src/i18n/locales/de.json index af303df5..21146d8f 100644 --- a/src/i18n/locales/de.json +++ b/src/i18n/locales/de.json @@ -815,6 +815,8 @@ "queryFailed": "Abfrage fehlgeschlagen.", "showErrorDetails": "Details anzeigen", "hideErrorDetails": "Details ausblenden", + "showExecutedQuery": "Ausgeführte Abfrage anzeigen", + "hideExecutedQuery": "Ausgeführte Abfrage ausblenden", "errorBoundary": { "title": "Der Editor ist unerwartet abgestürzt", "description": "Etwas im Editor konnte nicht gerendert werden. Der Fehler wird unten angezeigt — versuche es erneut oder kehre zu deinen Verbindungen zurück.", diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 0d9ba028..b4143d15 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -816,6 +816,8 @@ "queryFailed": "Query failed.", "showErrorDetails": "Show details", "hideErrorDetails": "Hide details", + "showExecutedQuery": "Show executed query", + "hideExecutedQuery": "Hide executed query", "errorBoundary": { "title": "The editor crashed unexpectedly", "description": "Something in the editor failed to render. The error is shown below — try again, or go back to your connections.", diff --git a/src/i18n/locales/es.json b/src/i18n/locales/es.json index 9eb9f739..cee4b500 100644 --- a/src/i18n/locales/es.json +++ b/src/i18n/locales/es.json @@ -813,6 +813,8 @@ "queryFailed": "La consulta falló.", "showErrorDetails": "Mostrar detalles", "hideErrorDetails": "Ocultar detalles", + "showExecutedQuery": "Mostrar consulta ejecutada", + "hideExecutedQuery": "Ocultar consulta ejecutada", "errorBoundary": { "title": "El editor falló inesperadamente", "description": "Algo en el editor no pudo renderizarse. El error se muestra abajo — vuelve a intentarlo o regresa a tus conexiones.", diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index b0461734..3346f523 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -815,6 +815,8 @@ "queryFailed": "Échec de la requête.", "showErrorDetails": "Afficher les détails", "hideErrorDetails": "Masquer les détails", + "showExecutedQuery": "Afficher la requête exécutée", + "hideExecutedQuery": "Masquer la requête exécutée", "errorBoundary": { "title": "L'éditeur a planté de manière inattendue", "description": "Un élément de l'éditeur n'a pas pu s'afficher. L'erreur est indiquée ci-dessous — réessayez ou revenez à vos connexions.", diff --git a/src/i18n/locales/it.json b/src/i18n/locales/it.json index 11927a3e..8eb4b3a4 100644 --- a/src/i18n/locales/it.json +++ b/src/i18n/locales/it.json @@ -798,6 +798,8 @@ "queryFailed": "Esecuzione query fallita.", "showErrorDetails": "Mostra dettagli", "hideErrorDetails": "Nascondi dettagli", + "showExecutedQuery": "Mostra query eseguita", + "hideExecutedQuery": "Nascondi query eseguita", "errorBoundary": { "title": "L'editor si è bloccato in modo imprevisto", "description": "Qualcosa nell'editor non è riuscito a renderizzarsi. L'errore è mostrato qui sotto — riprova oppure torna alle connessioni.", diff --git a/src/i18n/locales/ja.json b/src/i18n/locales/ja.json index 843edc96..dbdaba99 100644 --- a/src/i18n/locales/ja.json +++ b/src/i18n/locales/ja.json @@ -828,6 +828,8 @@ "queryFailed": "クエリが失敗しました。", "showErrorDetails": "詳細を表示", "hideErrorDetails": "詳細を非表示", + "showExecutedQuery": "実行されたクエリを表示", + "hideExecutedQuery": "実行されたクエリを非表示", "errorBoundary": { "title": "エディタが予期せずクラッシュしました", "description": "エディタの一部のレンダリングに失敗しました。エラーは下に表示されています。再試行するか、接続一覧に戻ってください。", diff --git a/src/i18n/locales/ru.json b/src/i18n/locales/ru.json index 7968c15e..02291f9d 100644 --- a/src/i18n/locales/ru.json +++ b/src/i18n/locales/ru.json @@ -810,6 +810,8 @@ "queryFailed": "Ошибка выполнения запроса.", "showErrorDetails": "Показать подробности", "hideErrorDetails": "Скрыть подробности", + "showExecutedQuery": "Показать выполненный запрос", + "hideExecutedQuery": "Скрыть выполненный запрос", "errorBoundary": { "title": "Редактор завершился с ошибкой", "description": "Ошибка при отображении редактора. Подробности ниже — попробуйте ещё раз или вернитесь к подключениям.", diff --git a/src/i18n/locales/zh.json b/src/i18n/locales/zh.json index 9aeba8ce..c6b73c7a 100644 --- a/src/i18n/locales/zh.json +++ b/src/i18n/locales/zh.json @@ -783,6 +783,8 @@ "queryFailed": "查询失败。", "showErrorDetails": "显示详情", "hideErrorDetails": "隐藏详情", + "showExecutedQuery": "显示已执行的查询", + "hideExecutedQuery": "隐藏已执行的查询", "errorBoundary": { "title": "编辑器意外崩溃", "description": "编辑器中的某些内容渲染失败。错误显示如下 — 请重试或返回到你的连接列表。", From 3ccec1101bd53c3a99285bed905ff60b1c4c4cc2 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Thu, 4 Jun 2026 15:27:35 +0200 Subject: [PATCH 3/4] feat(ui): show original query in error display --- src/components/notebook/SqlCell.tsx | 1 + src/components/notebook/SqlCellResult.tsx | 4 +- src/components/ui/ErrorDisplay.tsx | 107 ++++++++++++++-------- src/components/ui/ResultEntryContent.tsx | 2 +- src/i18n/locales/de.json | 2 + src/i18n/locales/en.json | 2 + src/i18n/locales/es.json | 2 + src/i18n/locales/fr.json | 2 + src/i18n/locales/it.json | 2 + src/i18n/locales/ja.json | 2 + src/i18n/locales/ru.json | 2 + src/i18n/locales/zh.json | 2 + src/pages/Editor.tsx | 6 +- 13 files changed, 95 insertions(+), 41 deletions(-) diff --git a/src/components/notebook/SqlCell.tsx b/src/components/notebook/SqlCell.tsx index 690fa5e1..e08bdd34 100644 --- a/src/components/notebook/SqlCell.tsx +++ b/src/components/notebook/SqlCell.tsx @@ -35,6 +35,7 @@ export function SqlCell({ - + ); } diff --git a/src/components/ui/ErrorDisplay.tsx b/src/components/ui/ErrorDisplay.tsx index abe120e7..5f350cab 100644 --- a/src/components/ui/ErrorDisplay.tsx +++ b/src/components/ui/ErrorDisplay.tsx @@ -5,6 +5,9 @@ import type { TFunction } from "i18next"; interface ErrorDisplayProps { error: string; t: TFunction; + /** The query the user submitted, shown in a collapsible block so they can + * see exactly what was run when an error occurs. */ + originalQuery?: string; } // Sentinel used by the Rust drivers to attach the actually-executed SQL to an @@ -13,10 +16,44 @@ interface ErrorDisplayProps { // that never appears in real SQL or error text). const EXECUTED_QUERY_MARKER = "\uE000__TABULARIS_EXECUTED_QUERY__"; -export function ErrorDisplay({ error, t }: ErrorDisplayProps) { - const [showDetails, setShowDetails] = useState(false); - const [showQuery, setShowQuery] = useState(false); +/** A chevron-toggled block: a small button that reveals a SQL/details panel. */ +function Collapsible({ + showLabel, + hideLabel, + children, + variant = "details", +}: { + showLabel: string; + hideLabel: string; + children: React.ReactNode; + variant?: "details" | "query"; +}) { + const [open, setOpen] = useState(false); + return ( + <> + + {open && + (variant === "query" ? ( + 
+            {children}
+
+ ) : ( +
+ {children} +
+ ))} + + ); +} +export function ErrorDisplay({ error, t, originalQuery }: ErrorDisplayProps) { // Peel off the executed-query block (if the driver attached one) before // applying the brief/detail split, so the SQL never leaks into the details. const markerIndex = error.indexOf(EXECUTED_QUERY_MARKER); @@ -32,46 +69,40 @@ export function ErrorDisplay({ error, t }: ErrorDisplayProps) { const brief = hasDetails ? message.slice(0, separatorIndex) : message; const details = hasDetails ? message.slice(separatorIndex + 2) : ""; + const original = originalQuery?.trim() ?? ""; + // Only surface the executed query when pagination actually rewrote the input + // — otherwise it just duplicates the original query the user already sees. + const showExecuted = + executedQuery.trim().length > 0 && executedQuery.trim() !== original; + return (
Error: {brief}
{hasDetails && ( - <> - - {showDetails && ( -
- {details} -
- )} - + + {details} + + )} + {original && ( + + {original} + )} - {executedQuery && ( - <> - - {showQuery && ( - 
-              {executedQuery}
-
- )} - + {showExecuted && ( + + {executedQuery} + )}
); diff --git a/src/components/ui/ResultEntryContent.tsx b/src/components/ui/ResultEntryContent.tsx index 7338fe5a..320e718b 100644 --- a/src/components/ui/ResultEntryContent.tsx +++ b/src/components/ui/ResultEntryContent.tsx @@ -45,7 +45,7 @@ export function ResultEntryContent({ if (entry.error) { return (
- +
); } diff --git a/src/i18n/locales/de.json b/src/i18n/locales/de.json index 21146d8f..7967837f 100644 --- a/src/i18n/locales/de.json +++ b/src/i18n/locales/de.json @@ -815,6 +815,8 @@ "queryFailed": "Abfrage fehlgeschlagen.", "showErrorDetails": "Details anzeigen", "hideErrorDetails": "Details ausblenden", + "showQuery": "Abfrage anzeigen", + "hideQuery": "Abfrage ausblenden", "showExecutedQuery": "Ausgeführte Abfrage anzeigen", "hideExecutedQuery": "Ausgeführte Abfrage ausblenden", "errorBoundary": { diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index b4143d15..3c21e313 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -816,6 +816,8 @@ "queryFailed": "Query failed.", "showErrorDetails": "Show details", "hideErrorDetails": "Hide details", + "showQuery": "Show query", + "hideQuery": "Hide query", "showExecutedQuery": "Show executed query", "hideExecutedQuery": "Hide executed query", "errorBoundary": { diff --git a/src/i18n/locales/es.json b/src/i18n/locales/es.json index cee4b500..1250c0f4 100644 --- a/src/i18n/locales/es.json +++ b/src/i18n/locales/es.json @@ -813,6 +813,8 @@ "queryFailed": "La consulta falló.", "showErrorDetails": "Mostrar detalles", "hideErrorDetails": "Ocultar detalles", + "showQuery": "Mostrar consulta", + "hideQuery": "Ocultar consulta", "showExecutedQuery": "Mostrar consulta ejecutada", "hideExecutedQuery": "Ocultar consulta ejecutada", "errorBoundary": { diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 3346f523..b3beaf98 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -815,6 +815,8 @@ "queryFailed": "Échec de la requête.", "showErrorDetails": "Afficher les détails", "hideErrorDetails": "Masquer les détails", + "showQuery": "Afficher la requête", + "hideQuery": "Masquer la requête", "showExecutedQuery": "Afficher la requête exécutée", "hideExecutedQuery": "Masquer la requête exécutée", "errorBoundary": { diff --git a/src/i18n/locales/it.json b/src/i18n/locales/it.json index 8eb4b3a4..32dade17 100644 --- a/src/i18n/locales/it.json +++ b/src/i18n/locales/it.json @@ -798,6 +798,8 @@ "queryFailed": "Esecuzione query fallita.", "showErrorDetails": "Mostra dettagli", "hideErrorDetails": "Nascondi dettagli", + "showQuery": "Mostra query", + "hideQuery": "Nascondi query", "showExecutedQuery": "Mostra query eseguita", "hideExecutedQuery": "Nascondi query eseguita", "errorBoundary": { diff --git a/src/i18n/locales/ja.json b/src/i18n/locales/ja.json index dbdaba99..6411e077 100644 --- a/src/i18n/locales/ja.json +++ b/src/i18n/locales/ja.json @@ -828,6 +828,8 @@ "queryFailed": "クエリが失敗しました。", "showErrorDetails": "詳細を表示", "hideErrorDetails": "詳細を非表示", + "showQuery": "クエリを表示", + "hideQuery": "クエリを非表示", "showExecutedQuery": "実行されたクエリを表示", "hideExecutedQuery": "実行されたクエリを非表示", "errorBoundary": { diff --git a/src/i18n/locales/ru.json b/src/i18n/locales/ru.json index 02291f9d..a8ef7621 100644 --- a/src/i18n/locales/ru.json +++ b/src/i18n/locales/ru.json @@ -810,6 +810,8 @@ "queryFailed": "Ошибка выполнения запроса.", "showErrorDetails": "Показать подробности", "hideErrorDetails": "Скрыть подробности", + "showQuery": "Показать запрос", + "hideQuery": "Скрыть запрос", "showExecutedQuery": "Показать выполненный запрос", "hideExecutedQuery": "Скрыть выполненный запрос", "errorBoundary": { diff --git a/src/i18n/locales/zh.json b/src/i18n/locales/zh.json index c6b73c7a..7380df8a 100644 --- a/src/i18n/locales/zh.json +++ b/src/i18n/locales/zh.json @@ -783,6 +783,8 @@ "queryFailed": "查询失败。", "showErrorDetails": "显示详情", "hideErrorDetails": "隐藏详情", + "showQuery": "显示查询", + "hideQuery": "隐藏查询", "showExecutedQuery": "显示已执行的查询", "hideExecutedQuery": "隐藏已执行的查询", "errorBoundary": { diff --git a/src/pages/Editor.tsx b/src/pages/Editor.tsx index d63ce974..1483690c 100644 --- a/src/pages/Editor.tsx +++ b/src/pages/Editor.tsx @@ -2964,7 +2964,11 @@ export const Editor = () => {

{t("editor.executingQuery")}

) : activeTab.error ? ( - + ) : activeTab.result || (activeTab.pendingInsertions && Object.keys(activeTab.pendingInsertions).length > 0) ? ( From d96ec81fe49e31fe86b8935e92c17437f85d30ff Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Wed, 10 Jun 2026 16:56:13 +0200 Subject: [PATCH 4/4] fix(drivers): gate pagination on the parse and strip LIMIT at its AST span Addresses review feedback on #286: - Paginate iff the query parses to a single Statement::Query instead of a starts_with("SELECT") check, so CTEs (WITH ... SELECT) and VALUES reach the parser-based rewriter while SHOW/EXPLAIN/DDL stay out. The prefix check remains as fallback when the parser rejects the input. - Strip a parsed LIMIT/OFFSET clause by cutting at its AST source span rather than re-scanning tokens, so clauses the heuristics don't recognise (LIMIT ALL, trailing comments, non-literal expressions) can no longer desync and produce mixed clauses or silently dropped pagination. A bare LIMIT ALL is folded into "no clause" by sqlparser and is located textually; queries with locking clauses defer to the fallback so FOR UPDATE is never silently dropped. - Make the heuristic tokenizer comment-aware so the fallback strip and extract scans cannot be shielded by trailing comments either. - Fix the misleading FETCH comment: the fallback does produce a mixed clause for FETCH FIRST queries. --- src-tauri/src/drivers/common.rs | 5 +- src-tauri/src/drivers/common/query.rs | 238 ++++++++++++++++++++++---- src-tauri/src/drivers/common/tests.rs | 177 +++++++++++++++++-- src-tauri/src/drivers/mysql/mod.rs | 25 ++- src-tauri/src/drivers/postgres/mod.rs | 47 +++-- src-tauri/src/drivers/sqlite/mod.rs | 33 ++-- 6 files changed, 438 insertions(+), 87 deletions(-) diff --git a/src-tauri/src/drivers/common.rs b/src-tauri/src/drivers/common.rs index d238b965..d0282730 100644 --- a/src-tauri/src/drivers/common.rs +++ b/src-tauri/src/drivers/common.rs @@ -11,8 +11,9 @@ pub use blob::{ }; pub use query::{ annotate_error_with_query, build_paginated_query, calculate_offset, extract_user_limit, - extract_user_offset, is_explainable_query, is_select_query, returns_result_set, - strip_leading_sql_comments, strip_limit_offset, PaginationDialect, EXECUTED_QUERY_MARKER, + extract_user_offset, is_explainable_query, is_paginatable_query, is_select_query, + returns_result_set, strip_leading_sql_comments, strip_limit_offset, PaginationDialect, + EXECUTED_QUERY_MARKER, }; pub use safe_int::{ i64_to_json, parse_unsafe_bigint_string, u64_to_json, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, diff --git a/src-tauri/src/drivers/common/query.rs b/src-tauri/src/drivers/common/query.rs index 4590c97f..c498a64e 100644 --- a/src-tauri/src/drivers/common/query.rs +++ b/src-tauri/src/drivers/common/query.rs @@ -1,13 +1,13 @@ -use sqlparser::ast::{Expr, LimitClause, Offset, OffsetRows, Statement, Value}; +use sqlparser::ast::{Expr, LimitClause, Offset, OffsetRows, Spanned, Statement, Value}; use sqlparser::dialect::{Dialect, MySqlDialect, PostgreSqlDialect, SQLiteDialect}; use sqlparser::parser::Parser; /// Check if a query is a SELECT statement. /// /// Leading SQL comments are stripped before checking, matching -/// [`returns_result_set`] and [`is_explainable_query`] — drivers rely -/// on this to route commented-header SELECTs through the pagination -/// path. +/// [`returns_result_set`] and [`is_explainable_query`]. Pagination routing +/// uses [`is_paginatable_query`], which derives the answer from the parse and +/// only falls back to this prefix check when the parser rejects the input. pub fn is_select_query(query: &str) -> bool { strip_leading_sql_comments(query) .trim_start() @@ -131,13 +131,16 @@ fn read_quoted(chars: &[(usize, char)], i: &mut usize, quote: char) -> String { /// - Double-quoted identifiers ("...") /// - Backtick-quoted identifiers (`...`) /// - Parenthesized groups (treated as single tokens) +/// - Comments (`-- …` and `/* … */`), skipped like the parser does /// - Whitespace as delimiter /// /// This prevents keywords like LIMIT or OFFSET from being matched /// inside string literals, quoted identifiers, or table names such as -/// `tapp_appointment_message_event_limit`. Each token is returned with -/// its starting byte offset so callers can slice the original input -/// instead of rebuilding it from tokens. +/// `tapp_appointment_message_event_limit` — and keeps a trailing +/// comment from shielding a LIMIT/OFFSET clause from the strip/extract +/// scans. Each token is returned with its starting byte offset so +/// callers can slice the original input instead of rebuilding it from +/// tokens. fn tokenize_sql_with_pos(sql: &str) -> Vec<(String, usize)> { let mut tokens: Vec<(String, usize)> = Vec::new(); let chars: Vec<(usize, char)> = sql.char_indices().collect(); @@ -158,6 +161,25 @@ fn tokenize_sql_with_pos(sql: &str) -> Vec<(String, usize)> { continue; } + if c == '-' && i + 1 < len && chars[i + 1].1 == '-' { + while i < len && chars[i].1 != '\n' { + i += 1; + } + continue; + } + + if c == '/' && i + 1 < len && chars[i + 1].1 == '*' { + i += 2; + while i < len { + if chars[i].1 == '*' && i + 1 < len && chars[i + 1].1 == '/' { + i += 2; + break; + } + i += 1; + } + continue; + } + if c == '(' { let mut token = String::new(); let mut depth = 0; @@ -200,6 +222,10 @@ fn tokenize_sql_with_pos(sql: &str) -> Vec<(String, usize)> { if ch.is_whitespace() || ch == '(' || ch == '\'' || ch == '"' || ch == '`' { break; } + let next = chars.get(i + 1).map(|&(_, n)| n); + if (ch == '-' && next == Some('-')) || (ch == '/' && next == Some('*')) { + break; + } token.push(ch); i += 1; } @@ -306,6 +332,21 @@ impl PaginationDialect { } } +/// Decide whether a query can be auto-paginated by appending LIMIT/OFFSET. +/// +/// Derived from the parse rather than a keyword prefix, so CTEs +/// (`WITH … SELECT`) and `VALUES` qualify while `SHOW`/`EXPLAIN`/DDL — which +/// parse to non-`Query` statements and reject a trailing `LIMIT` — do not. +/// When the parser cannot handle the input at all, falls back to +/// [`is_select_query`] so dialect quirks the parser misses keep the previous +/// prefix-based routing. +pub fn is_paginatable_query(query: &str, dialect: PaginationDialect) -> bool { + match Parser::parse_sql(dialect.parser_dialect().as_ref(), query) { + Ok(statements) => statements.len() == 1 && matches!(statements[0], Statement::Query(_)), + Err(_) => is_select_query(query), + } +} + /// LIMIT/OFFSET read from a query's parsed AST. struct ParsedPagination { /// User-supplied LIMIT, if it is a plain numeric literal. @@ -314,6 +355,11 @@ struct ParsedPagination { user_offset: u32, /// Whether the query carries a top-level LIMIT/OFFSET clause to strip. has_limit_clause: bool, + /// Byte offset in the original query where that clause's leading keyword + /// starts, derived from the AST's source span. `None` when the clause has + /// nothing to anchor a span to (`LIMIT ALL` parses to no expressions) — + /// the caller then strips via the token scan instead. + clause_start: Option, } /// Resolve a numeric-literal expression to a `u32`. @@ -330,6 +376,93 @@ fn eval_u32(expr: &Expr) -> Option { } } +/// Convert a 1-based line/column location (as reported by the parser's source +/// spans) into a byte offset in `sql`. Mirrors the parser's position tracking: +/// `\n` starts a new line, every other character (including `\r`) advances the +/// column by one. +fn location_to_byte(sql: &str, line: u64, column: u64) -> Option { + let mut cur_line = 1; + let mut cur_column = 1; + for (idx, ch) in sql.char_indices() { + if cur_line == line && cur_column == column { + return Some(idx); + } + if ch == '\n' { + cur_line += 1; + cur_column = 1; + } else { + cur_column += 1; + } + } + None +} + +/// Byte offset where the standalone keyword `kw` starts, given that +/// `query[..end]` should end with it (ignoring trailing whitespace). Returns +/// `None` when the keyword is absent or glued to an identifier or quote +/// (`mylimit`, `t.limit`), signalling the caller to fall back to the token +/// scan rather than cut mid-identifier. +fn trailing_keyword_start(query: &str, end: usize, kw: &str) -> Option { + let trimmed_len = query[..end].trim_end().len(); + let start = trimmed_len.checked_sub(kw.len())?; + if !query.is_char_boundary(start) || !query[start..trimmed_len].eq_ignore_ascii_case(kw) { + return None; + } + match query[..start].chars().next_back() { + Some(c) if c.is_ascii_alphanumeric() || matches!(c, '_' | '.' | '$' | '"' | '\'' | '`') => { + None + } + _ => Some(start), + } +} + +/// Locate where a parsed top-level LIMIT/OFFSET clause begins in the source +/// text, including its leading keyword. +/// +/// The AST span starts at the clause's first *value* (the keyword belongs to +/// no sub-expression), so this walks back over the keyword that must +/// immediately precede it. `LIMIT ALL` parses to no expression, so +/// `LIMIT ALL OFFSET n` anchors to the OFFSET value and the dangling +/// `LIMIT ALL` is peeled off as well (a bare `LIMIT ALL` never reaches the +/// AST at all — see [`trailing_limit_all_start`]). +fn limit_clause_start(query: &str, clause: &LimitClause) -> Option { + let span = clause.span(); + if span.start.line == 0 { + return None; + } + let value_start = location_to_byte(query, span.start.line, span.start.column)?; + if let Some(limit_kw) = trailing_keyword_start(query, value_start, "LIMIT") { + return Some(limit_kw); + } + let offset_kw = trailing_keyword_start(query, value_start, "OFFSET")?; + match trailing_keyword_start(query, offset_kw, "ALL") + .and_then(|all_kw| trailing_keyword_start(query, all_kw, "LIMIT")) + { + Some(limit_kw) => Some(limit_kw), + None => Some(offset_kw), + } +} + +/// Byte offset of a trailing top-level `LIMIT ALL`, if the query ends with one +/// (ignoring trailing comments). +/// +/// The parser swallows a bare `LIMIT ALL` — it means "no limit", so +/// `Query::limit_clause` comes back `None` and there is no AST node to anchor +/// a span to. Without this textual check the clause would survive the strip +/// and collide with the appended pagination clause. +fn trailing_limit_all_start(query: &str) -> Option { + let tokens = tokenize_sql_with_pos(query.trim_end()); + let n = tokens.len(); + if n >= 2 + && tokens[n - 2].0.eq_ignore_ascii_case("LIMIT") + && tokens[n - 1].0.eq_ignore_ascii_case("ALL") + { + Some(tokens[n - 2].1) + } else { + None + } +} + /// Parse `query` with `dialect` and read the top-level LIMIT/OFFSET from the /// AST, normalising MySQL's `LIMIT , ` form to the same shape. /// @@ -345,37 +478,59 @@ fn parse_pagination(query: &str, dialect: PaginationDialect) -> Option Some(ParsedPagination { - user_limit: None, - user_offset: 0, - has_limit_clause: false, - }), - Some(LimitClause::LimitOffset { limit, offset, .. }) => Some(ParsedPagination { - user_limit: limit.as_ref().and_then(eval_u32), - user_offset: offset - .as_ref() - .and_then(|o| eval_u32(&o.value)) - .unwrap_or(0), - has_limit_clause: true, - }), - Some(LimitClause::OffsetCommaLimit { offset, limit }) => Some(ParsedPagination { - user_limit: eval_u32(limit), - user_offset: eval_u32(offset).unwrap_or(0), - has_limit_clause: true, - }), + None => { + // A bare `LIMIT ALL` never reaches the AST (the parser folds it + // into "no limit clause"), so it must be located textually. + let clause_start = trailing_limit_all_start(query); + Some(ParsedPagination { + user_limit: None, + user_offset: 0, + has_limit_clause: clause_start.is_some(), + clause_start, + }) + } + Some(clause) => { + let (user_limit, user_offset) = match clause { + LimitClause::LimitOffset { limit, offset, .. } => ( + limit.as_ref().and_then(eval_u32), + offset + .as_ref() + .and_then(|o| eval_u32(&o.value)) + .unwrap_or(0), + ), + LimitClause::OffsetCommaLimit { offset, limit } => { + (eval_u32(limit), eval_u32(offset).unwrap_or(0)) + } + }; + Some(ParsedPagination { + user_limit, + user_offset, + has_limit_clause: true, + clause_start: limit_clause_start(query, clause), + }) + } } } /// True for the keyword/value tokens that make up a trailing LIMIT/OFFSET -/// clause (standard, MySQL comma, and FETCH spellings), so a LIMIT/OFFSET that -/// appears mid-query is never mistaken for the trailing pagination clause. +/// clause (standard, MySQL comma, `LIMIT ALL`, and FETCH spellings), so a +/// LIMIT/OFFSET that appears mid-query is never mistaken for the trailing +/// pagination clause. /// /// The numeric arm accepts digits interleaved with commas so MySQL's /// `LIMIT ,` is recognised even when written without spaces @@ -385,12 +540,16 @@ fn is_pagination_tail_token(tok: &str) -> bool { let upper = tok.to_uppercase(); matches!( upper.as_str(), - "LIMIT" | "OFFSET" | "BY" | "ROW" | "ROWS" | "ONLY" | "NEXT" | "FIRST" | "," + "LIMIT" | "OFFSET" | "ALL" | "BY" | "ROW" | "ROWS" | "ONLY" | "NEXT" | "FIRST" | "," ) || (!tok.is_empty() && tok.chars().all(|c| c.is_ascii_digit() || c == ',')) } /// Cut the query immediately before its trailing top-level LIMIT/OFFSET clause. /// +/// Safety net for the rare clause the parser reported but whose span could not +/// be anchored to the source ([`limit_clause_start`] returned `None`, e.g. +/// `LIMIT ALL` alone or a comment wedged between keyword and value). +/// /// Reuses [`tokenize_sql_with_pos`], which collapses parenthesised groups into /// a single token, so a LIMIT/OFFSET inside a subquery is never seen and only /// the outer clause is removed. Unlike [`strip_limit_offset`], it cuts at the @@ -426,11 +585,14 @@ fn number_expr(n: u32) -> Expr { /// /// The user's LIMIT/OFFSET are read from the parsed AST (using `dialect`), so /// dialect-specific forms such as MySQL's `LIMIT , ` and -/// `OFFSET` before `LIMIT` are understood. When the parser cannot handle the -/// input, it falls back to a token-aware heuristic scan. The appended -/// pagination clause is rendered from a [`LimitClause`] AST node and -/// concatenated to the verbatim sliced base, so leading comments, inline -/// hints, and the body's formatting are preserved. +/// `OFFSET` before `LIMIT` are understood. The clause is stripped by cutting +/// at its AST source span, so shapes the token heuristics cannot recognise +/// (`LIMIT ALL`, non-literal expressions, trailing comments) cannot desync +/// from what the parser saw. When the parser cannot handle the input, it +/// falls back to a token-aware heuristic scan. The appended pagination clause +/// is rendered from a [`LimitClause`] AST node and concatenated to the +/// verbatim sliced base, so leading comments, inline hints, and the body's +/// formatting are preserved. /// /// When the user wrote an explicit LIMIT, it is honoured as a cap on the total /// number of rows returned across all pages. A user-supplied OFFSET is honoured @@ -447,7 +609,11 @@ pub fn build_paginated_query( let (user_limit, user_offset, base) = match parse_pagination(query, dialect) { Some(parsed) => { - let base = if parsed.has_limit_clause { + let base = if let Some(cut) = parsed.clause_start { + // Cut at the clause's source span; everything from the clause + // onward (including any trailing comment) is dropped. + query[..cut].trim_end().to_string() + } else if parsed.has_limit_clause { strip_at_limit_keyword(query) } else { query.trim_end().to_string() diff --git a/src-tauri/src/drivers/common/tests.rs b/src-tauri/src/drivers/common/tests.rs index 0c604548..558f845e 100644 --- a/src-tauri/src/drivers/common/tests.rs +++ b/src-tauri/src/drivers/common/tests.rs @@ -1,9 +1,9 @@ use super::{ annotate_error_with_query, build_paginated_query, decode_blob_wire_format, encode_blob, - encode_blob_full, i64_to_json, is_explainable_query, is_select_query, parse_unsafe_bigint_string, - strip_leading_sql_comments, strip_limit_offset, u64_to_json, PaginationDialect, - DEFAULT_MAX_BLOB_SIZE, EXECUTED_QUERY_MARKER, JS_MAX_SAFE_INTEGER, JS_MAX_SAFE_UINT, - MAX_BLOB_PREVIEW_SIZE, + encode_blob_full, i64_to_json, is_explainable_query, is_paginatable_query, is_select_query, + parse_unsafe_bigint_string, strip_leading_sql_comments, strip_limit_offset, u64_to_json, + PaginationDialect, DEFAULT_MAX_BLOB_SIZE, EXECUTED_QUERY_MARKER, JS_MAX_SAFE_INTEGER, + JS_MAX_SAFE_UINT, MAX_BLOB_PREVIEW_SIZE, }; #[test] @@ -238,7 +238,9 @@ fn test_strip_limit_offset_table_name_contains_limit() { #[test] fn test_strip_limit_offset_table_name_contains_limit_with_real_limit() { assert_eq!( - strip_limit_offset("SELECT * FROM tapp_appointment_message_event_limit ORDER BY id LIMIT 10"), + strip_limit_offset( + "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id LIMIT 10" + ), "SELECT * FROM tapp_appointment_message_event_limit ORDER BY id" ); } @@ -371,10 +373,7 @@ fn test_extract_user_offset_only_offset() { #[test] fn test_extract_user_offset_absent() { - assert_eq!( - super::extract_user_offset("SELECT * FROM t LIMIT 50"), - None - ); + assert_eq!(super::extract_user_offset("SELECT * FROM t LIMIT 50"), None); } #[test] @@ -505,6 +504,161 @@ fn test_build_paginated_query_falls_back_on_parse_error() { assert_eq!(result, "SELECT * FROM t WHERE LIMIT 5 OFFSET 0"); } +#[test] +fn test_is_paginatable_query_select_cte_values() { + assert!(is_paginatable_query( + "SELECT * FROM users", + PaginationDialect::Postgres + )); + assert!(is_paginatable_query( + "-- header\nSELECT * FROM users", + PaginationDialect::Postgres + )); + // CTEs parse to `Statement::Query` and must be paginatable even though + // they do not start with SELECT. + assert!(is_paginatable_query( + "WITH recent AS (SELECT * FROM orders) SELECT * FROM recent", + PaginationDialect::Postgres + )); + assert!(is_paginatable_query( + "VALUES (1), (2)", + PaginationDialect::Postgres + )); +} + +#[test] +fn test_is_paginatable_query_excludes_non_queries() { + // These parse fine but reject a trailing LIMIT — they must not be widened + // into the pagination path. + assert!(!is_paginatable_query( + "SHOW TABLES", + PaginationDialect::MySql + )); + assert!(!is_paginatable_query( + "EXPLAIN SELECT 1", + PaginationDialect::Postgres + )); + assert!(!is_paginatable_query( + "INSERT INTO t VALUES (1)", + PaginationDialect::Postgres + )); + assert!(!is_paginatable_query( + "CREATE TABLE t (id INT)", + PaginationDialect::Sqlite + )); + assert!(!is_paginatable_query( + "SELECT 1; SELECT 2", + PaginationDialect::Postgres + )); +} + +#[test] +fn test_is_paginatable_query_parse_error_falls_back_to_prefix() { + // The parser rejects both; the SELECT prefix heuristic keeps the old + // routing so dialect quirks the parser misses still paginate. + assert!(is_paginatable_query( + "SELECT * FROM t WHERE LIMIT 5", + PaginationDialect::Postgres + )); + assert!(!is_paginatable_query( + "TOTALLY NOT SQL", + PaginationDialect::Postgres + )); +} + +#[test] +fn test_build_paginated_query_cte() { + let q = "WITH recent AS (SELECT * FROM orders ORDER BY id) SELECT * FROM recent"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!( + result, + "WITH recent AS (SELECT * FROM orders ORDER BY id) SELECT * FROM recent LIMIT 101 OFFSET 0" + ); +} + +#[test] +fn test_build_paginated_query_cte_with_outer_limit() { + // Only the outer LIMIT is stripped; the one inside the CTE body stays. + let q = "WITH recent AS (SELECT * FROM orders LIMIT 1000) SELECT * FROM recent LIMIT 50"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!( + result, + "WITH recent AS (SELECT * FROM orders LIMIT 1000) SELECT * FROM recent LIMIT 50 OFFSET 0" + ); +} + +#[test] +fn test_build_paginated_query_limit_all() { + // A bare `LIMIT ALL` is folded into "no limit clause" by the parser, so it + // never reaches the AST and must be located by the textual trailing check. + let q = "SELECT * FROM t ORDER BY id LIMIT ALL"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 0"); +} + +#[test] +fn test_build_paginated_query_limit_all_with_offset() { + // The span anchors to the OFFSET value; the dangling `LIMIT ALL` before it + // must be peeled off too, not left to collide with the appended clause. + let q = "SELECT * FROM t ORDER BY id LIMIT ALL OFFSET 5"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 5"); +} + +#[test] +fn test_build_paginated_query_limit_with_trailing_line_comment() { + // The `--` used to survive the strip and comment out the appended clause, + // silently disabling pagination. + let q = "SELECT * FROM t ORDER BY id LIMIT 2000 -- pagination note"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t ORDER BY id LIMIT 101 OFFSET 0"); +} + +#[test] +fn test_build_paginated_query_limit_with_trailing_block_comment() { + let q = "SELECT * FROM t LIMIT 50 /* cap */"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t LIMIT 50 OFFSET 0"); +} + +#[test] +fn test_build_paginated_query_expression_limit() { + // A non-literal LIMIT cannot be honoured as a cap, but the span cut must + // still strip it (the token scan does not recognise this shape). + let q = "SELECT * FROM t LIMIT 10 + 5"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT * FROM t LIMIT 101 OFFSET 0"); +} + +#[test] +fn test_build_paginated_query_multiline_with_trailing_comment() { + // Exercises the line/column → byte translation behind the span cut. + let q = "SELECT *\nFROM t\nORDER BY id\nLIMIT 25 -- note"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::Postgres); + assert_eq!(result, "SELECT *\nFROM t\nORDER BY id LIMIT 25 OFFSET 0"); +} + +#[test] +fn test_build_paginated_query_comment_between_limit_and_value() { + // The span anchor lands after the comment, so the keyword walk-back fails + // and the token-scan safety net strips the clause instead. + let q = "SELECT * FROM t LIMIT /* cap */ 10"; + let result = build_paginated_query(q, 100, 1, PaginationDialect::MySql); + assert_eq!(result, "SELECT * FROM t LIMIT 10 OFFSET 0"); +} + +#[test] +fn test_strip_limit_offset_trailing_comment() { + assert_eq!( + strip_limit_offset("SELECT * FROM t LIMIT 10 -- note"), + "SELECT * FROM t" + ); + assert_eq!( + strip_limit_offset("SELECT * FROM t LIMIT 10 /* note */"), + "SELECT * FROM t" + ); +} + #[test] fn test_strip_limit_offset_preserves_leading_comments() { assert_eq!( @@ -655,7 +809,10 @@ fn test_parse_unsafe_bigint_string_returns_outside_safe_range() { parse_unsafe_bigint_string("-9007199254740992"), Some(-JS_MAX_SAFE_INTEGER - 1) ); - assert_eq!(parse_unsafe_bigint_string(&i64::MAX.to_string()), Some(i64::MAX)); + assert_eq!( + parse_unsafe_bigint_string(&i64::MAX.to_string()), + Some(i64::MAX) + ); } #[test] diff --git a/src-tauri/src/drivers/mysql/mod.rs b/src-tauri/src/drivers/mysql/mod.rs index bbe953b5..05d5d926 100644 --- a/src-tauri/src/drivers/mysql/mod.rs +++ b/src-tauri/src/drivers/mysql/mod.rs @@ -939,13 +939,16 @@ async fn exec_on_mysql_conn( }); } - let is_select = crate::drivers::common::is_select_query(query); + let is_paginatable = crate::drivers::common::is_paginatable_query( + query, + crate::drivers::common::PaginationDialect::MySql, + ); let mut pagination: Option = None; let final_query: String; let mut manual_limit = limit; let mut truncated = false; - if is_select && limit.is_some() { + if is_paginatable && limit.is_some() { let l = limit.unwrap(); final_query = crate::drivers::common::build_paginated_query( @@ -1107,7 +1110,11 @@ pub async fn get_trigger_definition( ) -> Result { let pool = get_mysql_pool(params).await?; let qualified = match schema { - Some(s) => format!("`{}`.`{}`", escape_identifier(s), escape_identifier(trigger_name)), + Some(s) => format!( + "`{}`.`{}`", + escape_identifier(s), + escape_identifier(trigger_name) + ), None => format!("`{}`", escape_identifier(trigger_name)), }; let query = format!("SHOW CREATE TRIGGER {}", qualified); @@ -1149,7 +1156,11 @@ pub async fn drop_trigger( ) -> Result<(), String> { let pool = get_mysql_pool(params).await?; let qualified = match schema { - Some(s) => format!("`{}`.`{}`", escape_identifier(s), escape_identifier(trigger_name)), + Some(s) => format!( + "`{}`.`{}`", + escape_identifier(s), + escape_identifier(trigger_name) + ), None => format!("`{}`", escape_identifier(trigger_name)), }; let query = format!("DROP TRIGGER IF EXISTS {}", qualified); @@ -1315,10 +1326,8 @@ impl DatabaseDriver for MysqlDriver { } else { format!("{}:{}", user, encode(raw_pass)) }; - let max_allowed_packet = mysql_numeric_setting( - "maxAllowedPacket", - DEFAULT_MYSQL_MAX_ALLOWED_PACKET, - ); + let max_allowed_packet = + mysql_numeric_setting("maxAllowedPacket", DEFAULT_MYSQL_MAX_ALLOWED_PACKET); let socket_timeout = mysql_numeric_setting("socketTimeout", DEFAULT_MYSQL_SOCKET_TIMEOUT_MS); let connect_timeout = diff --git a/src-tauri/src/drivers/postgres/mod.rs b/src-tauri/src/drivers/postgres/mod.rs index 55ea9271..b931ce5a 100644 --- a/src-tauri/src/drivers/postgres/mod.rs +++ b/src-tauri/src/drivers/postgres/mod.rs @@ -16,7 +16,7 @@ use crate::models::{ TableColumn, TableInfo, TriggerInfo, ViewInfo, }; use crate::pool_manager::get_postgres_pool; -use binding::{PgValueOptions, bind_pg_value, build_pk_predicate}; +use binding::{bind_pg_value, build_pk_predicate, PgValueOptions}; use client::{execute, format_pg_error, get_client, query_all, query_one}; pub use explain::explain_query; use extract::extract_value; @@ -804,11 +804,14 @@ async fn exec_on_pg_client( }); } - let is_select = crate::drivers::common::is_select_query(query); + let is_paginatable = crate::drivers::common::is_paginatable_query( + query, + crate::drivers::common::PaginationDialect::Postgres, + ); let mut manual_limit = limit; let mut truncated = false; - let (final_query, pagination_meta) = if is_select && limit.is_some() { + let (final_query, pagination_meta) = if is_paginatable && limit.is_some() { let l = limit.unwrap(); let data_query = crate::drivers::common::build_paginated_query( query, @@ -823,16 +826,14 @@ async fn exec_on_pg_client( }; let pg_params: Vec = vec![]; - let mut rows_stream = std::pin::pin!( - client - .query_raw(&final_query, &pg_params) - .await - .map_err(|e| crate::drivers::common::annotate_error_with_query( - format_pg_error(&e), - &final_query, - query - ))? - ); + let mut rows_stream = std::pin::pin!(client + .query_raw(&final_query, &pg_params) + .await + .map_err(|e| crate::drivers::common::annotate_error_with_query( + format_pg_error(&e), + &final_query, + query + ))?); let mut columns: Vec = Vec::new(); let mut json_rows = Vec::new(); @@ -1345,7 +1346,9 @@ pub async fn drop_trigger( // Plugin wrapper // ============================================================ -use crate::drivers::driver_trait::{DatabaseDriver, DriverCapabilities, PluginManifest, SqlDialect}; +use crate::drivers::driver_trait::{ + DatabaseDriver, DriverCapabilities, PluginManifest, SqlDialect, +}; use async_trait::async_trait; use std::collections::HashMap; @@ -1604,7 +1607,13 @@ impl DatabaseDriver for PostgresDriver { table_name: &str, schema: Option<&str>, ) -> Result { - get_trigger_definition(params, trigger_name, table_name, self.resolve_schema(schema)).await + get_trigger_definition( + params, + trigger_name, + table_name, + self.resolve_schema(schema), + ) + .await } async fn create_trigger( @@ -1623,7 +1632,13 @@ impl DatabaseDriver for PostgresDriver { table_name: &str, schema: Option<&str>, ) -> Result<(), String> { - drop_trigger(params, trigger_name, table_name, self.resolve_schema(schema)).await + drop_trigger( + params, + trigger_name, + table_name, + self.resolve_schema(schema), + ) + .await } async fn execute_query( diff --git a/src-tauri/src/drivers/sqlite/mod.rs b/src-tauri/src/drivers/sqlite/mod.rs index f747b896..23760801 100644 --- a/src-tauri/src/drivers/sqlite/mod.rs +++ b/src-tauri/src/drivers/sqlite/mod.rs @@ -532,12 +532,15 @@ async fn exec_on_sqlite_conn( }); } - let is_select = crate::drivers::common::is_select_query(query); + let is_paginatable = crate::drivers::common::is_paginatable_query( + query, + crate::drivers::common::PaginationDialect::Sqlite, + ); let mut pagination: Option = None; let final_query: String; let mut manual_limit = limit; - if is_select && limit.is_some() { + if is_paginatable && limit.is_some() { let l = limit.unwrap(); final_query = crate::drivers::common::build_paginated_query( @@ -768,7 +771,10 @@ pub async fn get_view_columns( } pub async fn get_triggers(params: &ConnectionParams) -> Result, String> { - log::debug!("SQLite: Fetching triggers for database: {}", params.database); + log::debug!( + "SQLite: Fetching triggers for database: {}", + params.database + ); let pool = get_sqlite_pool(params).await?; let rows = sqlx::query( "SELECT name, tbl_name, sql FROM sqlite_master WHERE type='trigger' ORDER BY name ASC", @@ -826,13 +832,11 @@ pub async fn get_trigger_definition( trigger_name: &str, ) -> Result { let pool = get_sqlite_pool(params).await?; - let row = sqlx::query( - "SELECT sql FROM sqlite_master WHERE type='trigger' AND name = ?", - ) - .bind(trigger_name) - .fetch_one(&pool) - .await - .map_err(|e| format!("Failed to get trigger definition: {}", e))?; + let row = sqlx::query("SELECT sql FROM sqlite_master WHERE type='trigger' AND name = ?") + .bind(trigger_name) + .fetch_one(&pool) + .await + .map_err(|e| format!("Failed to get trigger definition: {}", e))?; let sql: String = row.try_get("sql").unwrap_or_default(); Ok(sql) } @@ -846,10 +850,7 @@ pub async fn create_trigger(params: &ConnectionParams, trigger_sql: &str) -> Res Ok(()) } -pub async fn drop_trigger( - params: &ConnectionParams, - trigger_name: &str, -) -> Result<(), String> { +pub async fn drop_trigger(params: &ConnectionParams, trigger_name: &str) -> Result<(), String> { let pool = get_sqlite_pool(params).await?; let sql = format!( "DROP TRIGGER IF EXISTS \"{}\"", @@ -866,7 +867,9 @@ pub async fn drop_trigger( // Plugin wrapper // ============================================================ -use crate::drivers::driver_trait::{DatabaseDriver, DriverCapabilities, PluginManifest, SqlDialect}; +use crate::drivers::driver_trait::{ + DatabaseDriver, DriverCapabilities, PluginManifest, SqlDialect, +}; use async_trait::async_trait; use std::collections::HashMap;