diff --git a/docs/TUI_FEATURES.md b/docs/TUI_FEATURES.md index 57b353f..aa83ef2 100644 --- a/docs/TUI_FEATURES.md +++ b/docs/TUI_FEATURES.md @@ -59,7 +59,9 @@ columns it is completing. Between them the completer now has both primitives the remaining entries need: a byte span to splice over, and a typed schema. **Recommended order: T3 β†’ T4 β†’ T5**, with **T7** droppable anywhere β€” it is -independent of the others and mostly deletion. T3 is mechanical but wants doing +independent of the others and mostly deletion. **T8** is done: it was T1's bug +in the other producer of column text, and it left behind +`src/sql/identifier.rs` as the one place the quoting rule lives. T3 is mechanical but wants doing *before* T4, not as a retrofit. T4 is the first entry that consumes what T2 captured (`ColumnInfo::cardinality`, `TableInfo::row_count`); those numbers are already flowing and pinned by tests, so the gate can be designed against real @@ -287,3 +289,53 @@ Older, non-living notes that still contain usable thinking: first two rows. - **Not to be confused with T2's leftovers:** the completer's *type* decisions are already schema-driven. This entry is about the surrounding TUI. + +### T8 β€” `SELECT *` expansion emits column names it cannot read back +- **Status:** 🟒 DONE 2026-09-05 +- **Where:** `src/sql/identifier.rs` (new), `src/buffer.rs:1552,1609`, + `src/data/csv_fixes.rs`, `src/sql/parser/formatter.rs` +- **Observed:** Ctrl+X (expand to all schema columns) and Alt+X (expand to + visible columns) both did `columns.join(", ")` on the raw names. On + `data/countries.csv` that produced + + ``` + SELECT name.common, name.official, tld, ..., idd.root, ... FROM countries + ``` + + which the parser reads as method calls on a `name` column. Every dotted name + β€” `name.*`, `idd.*` and 60-odd `translations.*.*`, i.e. most of the file β€” + came out unusable, and the user's next keystroke was to hand-quote 70 columns + or undo. +- **Why it is the same bug as T1 in a different hat:** completion had already + been taught to quote (it calls `quote_if_needed` at nine sites in + `cursor_aware_parser.rs`). Expansion is the *other* producer of column text + and never learned. Two producers, one of them right, is the drift T1's + "the parser owns semantics, the editor owns text" principle is meant to stop + β€” it just did not have anywhere to put the rule. +- **Fixed by:** `src/sql/identifier.rs`, the single home for *does this name + have to be quoted*. The rule mirrors `Lexer::read_identifier`, which is what + actually decides whether a bare word survives: Unicode alphanumerics plus + `_`, not starting with a digit. Keyword status comes from + `Token::from_keyword` rather than a second hand-kept list, so a column called + `row` or `end` is quoted for exactly as long as the lexer reserves those + words. + + Three call sites now share it: + + | Site | Was | Now | + |---|---|---| + | `csv_fixes::needs_quoting` (used by all of completion) | a 9-way `contains()` chain β€” missed leading digits and keywords | delegates | + | `formatter::needs_quotes` | its own 40-word reserved list, hand-kept | delegates (and stops re-quoting text the parser already handed back quoted) | + | `Buffer::expand_asterisk{,_visible}` | nothing at all | quotes | + +- **Tests:** `tests/asterisk_expansion.rs` (5) covers both expansion paths, + hidden columns, the rest of the query surviving intact, and names that + collide with keywords. `src/sql/identifier.rs` has 8 unit tests for the rule + itself. Verified end to end by running the full 76-column expansion of + `data/countries.csv`. +- **Left open:** `formatter::needs_quotes` is applied to + `SelectStatement::columns`, which is the deprecated legacy field and can hold + expression text, so the formatter still wraps `COUNT(*)` in quotes. That is a + pre-existing formatter bug about *what* it quotes, not *when* β€” unchanged + here, and it wants fixing where the field is retired rather than in the + quoting rule. diff --git a/src/buffer.rs b/src/buffer.rs index 95f84d9..7b1c010 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -5,6 +5,7 @@ use crate::data::data_view::DataView; use crate::data::datatable::DataTable; use crate::hybrid_parser::HybridParser; use crate::input_manager::{create_from_input, create_single_line, InputManager}; +use crate::sql::identifier::quote_if_needed; use anyhow::Result; use crossterm::event::KeyEvent; use fuzzy_matcher::skim::SkimMatcherV2; @@ -1573,8 +1574,16 @@ impl Buffer { self.status_message = format!("No columns found for table '{table_name}'"); } else { - // Build the replacement with all columns - let columns_str = columns.join(", "); + // Build the replacement with all columns. Every + // name goes through the shared quoting rule - + // countries.csv's name.official and idd.root are + // unparseable bare, and expansion used to emit + // them that way. + let columns_str = columns + .iter() + .map(|col| quote_if_needed(col)) + .collect::>() + .join(", "); // Replace * with the column list let before_star = &query[..star_abs_pos]; @@ -1614,8 +1623,13 @@ impl Buffer { let visible_columns = dataview.get_display_column_names(); if !visible_columns.is_empty() { - // Build the replacement with visible columns only - let columns_str = visible_columns.join(", "); + // Build the replacement with visible columns only, + // quoted by the same rule as the full expansion. + let columns_str = visible_columns + .iter() + .map(|col| quote_if_needed(col)) + .collect::>() + .join(", "); // Replace * with the column list let before_star = &query[..star_abs_pos]; diff --git a/src/data/csv_fixes.rs b/src/data/csv_fixes.rs index 1bd89ab..be288d4 100644 --- a/src/data/csv_fixes.rs +++ b/src/data/csv_fixes.rs @@ -3,27 +3,19 @@ use std::collections::HashMap; /// Check if a column name needs quoting (contains spaces or special characters) +/// +/// Thin re-export of [`crate::sql::identifier::needs_quoting`] β€” the rule lives +/// there so completion, `SELECT *` expansion and the formatter cannot drift +/// apart on it. #[must_use] pub fn needs_quoting(column_name: &str) -> bool { - column_name.contains(' ') - || column_name.contains('-') - || column_name.contains('.') - || column_name.contains('(') - || column_name.contains(')') - || column_name.contains('[') - || column_name.contains(']') - || column_name.contains('"') - || column_name.contains('\'') + crate::sql::identifier::needs_quoting(column_name) } /// Quote a column name if necessary #[must_use] pub fn quote_if_needed(column_name: &str) -> String { - if needs_quoting(column_name) { - format!("\"{}\"", column_name.replace('"', "\"\"")) - } else { - column_name.to_string() - } + crate::sql::identifier::quote_if_needed(column_name) } /// Build a case-insensitive lookup map for column names diff --git a/src/sql/identifier.rs b/src/sql/identifier.rs new file mode 100644 index 0000000..039bd8c --- /dev/null +++ b/src/sql/identifier.rs @@ -0,0 +1,127 @@ +//! The single source of truth for "does this column name have to be quoted?". +//! +//! Anything that splices a column name back into query text β€” tab completion, +//! `SELECT *` expansion, the pretty-printer β€” has to agree on this, or the +//! user ends up with a query the parser cannot read. `data/countries.csv` is +//! the standing example: `name.official`, `idd.root` and 60-odd +//! `translations.*.common` columns are all unparseable bare. +//! +//! The rules mirror [`Lexer::read_identifier`](crate::sql::parser::lexer), which +//! is what actually decides whether a bare word survives: alphanumeric (Unicode +//! included, so `paΓ­s` is fine) plus `_`, not starting with a digit. Keyword +//! status comes from [`Token::from_keyword`] rather than a second hand-kept +//! list, so a column called `row` or `end` is quoted for as long as the lexer +//! reserves those words and no longer. + +use crate::sql::parser::lexer::Token; + +/// Check if an identifier must be quoted to survive a round trip through the +/// parser. +#[must_use] +pub fn needs_quoting(name: &str) -> bool { + if name.is_empty() { + return true; + } + + // A leading digit makes the lexer read a number, not an identifier. + if name.starts_with(|c: char| c.is_ascii_digit()) { + return true; + } + + // Anything the lexer would stop reading at: '.', ' ', '-', '(', '"', ... + if !name.chars().all(is_identifier_char) { + return true; + } + + // A bare keyword would tokenize as that keyword rather than a column. + Token::from_keyword(name).is_some() +} + +/// Quote an identifier unconditionally, doubling any embedded `"` the way SQL +/// expects (`Has"Quote` -> `"Has""Quote"`). +#[must_use] +pub fn quote_identifier(name: &str) -> String { + format!("\"{}\"", name.replace('"', "\"\"")) +} + +/// Quote an identifier, but only if it would not parse bare. +#[must_use] +pub fn quote_if_needed(name: &str) -> String { + if needs_quoting(name) { + quote_identifier(name) + } else { + name.to_string() + } +} + +/// The characters the lexer will accept inside an unquoted identifier. +fn is_identifier_char(ch: char) -> bool { + ch.is_alphanumeric() || ch == '_' +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn plain_identifiers_are_left_alone() { + for name in ["City", "customer_id", "tld", "cca2", "unMember", "x1"] { + assert!(!needs_quoting(name), "{name} should not need quoting"); + assert_eq!(quote_if_needed(name), name); + } + } + + #[test] + fn dotted_names_need_quoting() { + // The countries.csv case that started this. + assert!(needs_quoting("name.official")); + assert_eq!(quote_if_needed("name.common"), "\"name.common\""); + assert_eq!( + quote_if_needed("translations.ara.official"), + "\"translations.ara.official\"" + ); + } + + #[test] + fn punctuation_and_spaces_need_quoting() { + assert!(needs_quoting("Phone 1")); + assert!(needs_quoting("Customer-ID")); + assert!(needs_quoting("Price ($)")); + assert!(needs_quoting("a/b")); + assert!(needs_quoting("col[0]")); + } + + #[test] + fn leading_digit_needs_quoting() { + assert!(needs_quoting("2024")); + assert!(needs_quoting("1st_place")); + assert!(!needs_quoting("q1")); + } + + #[test] + fn keywords_need_quoting_case_insensitively() { + assert!(needs_quoting("order")); + assert!(needs_quoting("ORDER")); + assert!(needs_quoting("Row")); + assert!(needs_quoting("end")); + // Not reserved by this lexer, so leave it bare. + assert!(!needs_quoting("status")); + assert!(!needs_quoting("independent")); + } + + #[test] + fn unicode_identifiers_do_not_need_quoting() { + assert!(!needs_quoting("paΓ­s")); + assert!(!needs_quoting("naΓ―ve_count")); + } + + #[test] + fn embedded_quotes_are_doubled() { + assert_eq!(quote_if_needed("Has\"Quote"), "\"Has\"\"Quote\""); + } + + #[test] + fn empty_name_is_quoted_rather_than_emitted_bare() { + assert_eq!(quote_if_needed(""), "\"\""); + } +} diff --git a/src/sql/mod.rs b/src/sql/mod.rs index ebbcada..d38d316 100644 --- a/src/sql/mod.rs +++ b/src/sql/mod.rs @@ -11,6 +11,7 @@ pub mod cursor_aware_parser; pub mod functions; pub mod generators; pub mod hybrid_parser; +pub mod identifier; pub mod parser; pub mod query_rewriter; pub mod recursive_parser; diff --git a/src/sql/parser/formatter.rs b/src/sql/parser/formatter.rs index 7a5db3c..462003d 100644 --- a/src/sql/parser/formatter.rs +++ b/src/sql/parser/formatter.rs @@ -896,34 +896,15 @@ fn format_token(token: &Token) -> String { } } -// Check if a column name needs quotes (contains special characters or is a reserved word) +// Check if a column name needs quotes. The rule itself lives in +// crate::sql::identifier so the formatter, tab completion and SELECT * +// expansion cannot drift apart on it; the only thing the formatter adds is +// leaving text the parser already handed back quoted alone. fn needs_quotes(name: &str) -> bool { - // Check for special characters that require quoting - if name.contains('-') || name.contains(' ') || name.contains('.') || name.contains('/') { - return true; + if name.starts_with('"') { + return false; } - - // Check if it starts with a number - if name.chars().next().map_or(false, |c| c.is_ascii_digit()) { - return true; - } - - // Check if it's a SQL reserved word (common ones) - let reserved_words = [ - "SELECT", "FROM", "WHERE", "ORDER", "GROUP", "BY", "HAVING", "INSERT", "UPDATE", "DELETE", - "CREATE", "DROP", "ALTER", "TABLE", "INDEX", "VIEW", "AND", "OR", "NOT", "IN", "EXISTS", - "BETWEEN", "LIKE", "CASE", "WHEN", "THEN", "ELSE", "END", "JOIN", "LEFT", "RIGHT", "INNER", - "OUTER", "ON", "AS", "DISTINCT", "ALL", "TOP", "LIMIT", "OFFSET", "ASC", "DESC", - ]; - - let upper_name = name.to_uppercase(); - if reserved_words.contains(&upper_name.as_str()) { - return true; - } - - // Check if all characters are valid for unquoted identifiers - // Valid: letters, numbers, underscore (but not starting with number) - !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') + crate::sql::identifier::needs_quoting(name) } // Format CASE expressions with proper indentation diff --git a/tests/asterisk_expansion.rs b/tests/asterisk_expansion.rs new file mode 100644 index 0000000..357f348 --- /dev/null +++ b/tests/asterisk_expansion.rs @@ -0,0 +1,122 @@ +//! `SELECT *` expansion (Ctrl+X / Alt+X) must emit column names the parser can +//! read back. +//! +//! `data/countries.csv` is the case that exposed this: `name.common`, +//! `idd.root` and 60-odd `translations.*.common` columns all have to be quoted. +//! Expansion used to join the raw names, so pressing Alt+X on that file +//! produced `SELECT name.common, name.official, ...` β€” a query the parser +//! reads as method calls on a `name` column. Tab completion already quoted +//! correctly (T1), which is exactly the drift this pins down: both go through +//! `sql::identifier::quote_if_needed` now. + +use sql_cli::buffer::{Buffer, BufferAPI}; +use sql_cli::data::data_view::DataView; +use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use sql_cli::hybrid_parser::HybridParser; +use std::sync::Arc; + +/// The awkward end of countries.csv, plus a couple of names that are fine bare. +const COLUMNS: &[&str] = &[ + "name.common", + "name.official", + "tld", + "cca2", + "idd.root", + "unMember", +]; + +fn table() -> DataTable { + let mut table = DataTable::new("countries"); + for name in COLUMNS { + table.add_column(DataColumn::new(*name)); + } + table + .add_row(DataRow::new( + COLUMNS + .iter() + .map(|c| DataValue::String((*c).to_string())) + .collect(), + )) + .expect("row matches column count"); + table +} + +fn parser() -> HybridParser { + let mut parser = HybridParser::new(); + parser.update_single_table( + "countries".to_string(), + COLUMNS.iter().map(|c| (*c).to_string()).collect(), + ); + parser +} + +fn buffer_with_query(query: &str) -> Buffer { + let mut buffer = Buffer::new(1); + buffer.set_input_text(query.to_string()); + buffer.set_dataview(Some(DataView::new(Arc::new(table())))); + buffer +} + +const EXPANDED: &str = "\"name.common\", \"name.official\", tld, cca2, \"idd.root\", unMember"; + +#[test] +fn schema_expansion_quotes_dotted_names() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + assert!(buffer.expand_asterisk(&parser())); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries") + ); +} + +#[test] +fn visible_expansion_quotes_dotted_names() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries") + ); +} + +#[test] +fn visible_expansion_follows_hidden_columns() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + let mut view = DataView::new(Arc::new(table())); + view.hide_column_by_name("tld"); + view.hide_column_by_name("cca2"); + buffer.set_dataview(Some(view)); + + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + "SELECT \"name.common\", \"name.official\", \"idd.root\", unMember FROM countries" + ); +} + +#[test] +fn expansion_leaves_the_rest_of_the_query_alone() { + let mut buffer = buffer_with_query("SELECT * FROM countries WHERE cca2 = 'GB' ORDER BY tld"); + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries WHERE cca2 = 'GB' ORDER BY tld") + ); +} + +#[test] +fn expansion_quotes_names_that_collide_with_keywords() { + let mut table = DataTable::new("t"); + for name in ["order", "Row", "Price ($)", "1st", "region"] { + table.add_column(DataColumn::new(name)); + } + let mut buffer = Buffer::new(1); + buffer.set_input_text("SELECT * FROM t".to_string()); + buffer.set_dataview(Some(DataView::new(Arc::new(table)))); + + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + "SELECT \"order\", \"Row\", \"Price ($)\", \"1st\", region FROM t" + ); +} diff --git a/tests/main.rs b/tests/main.rs index 954cfd2..0fc1ed6 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -225,3 +225,6 @@ mod projection_column_width_tests; #[path = "dotted_column_completion.rs"] mod dotted_column_completion; + +#[path = "asterisk_expansion.rs"] +mod asterisk_expansion;