From 1181baf9da8c89db7b73ed372a6054c2185dc7ee Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 26 Oct 2026 15:11:26 +0000 Subject: [PATCH] refactor(workspace): quote the expanded columns with the catalog's quote_identifier Expand star had its own quote_identifier, which quoted every keyword that isn't unreserved. It now uses pgls_catalog::quote_identifier: the first name of a column as a ColId, the names after a dot as labels. A column name keyword like `between` and any keyword after a dot, as in `t.select`, are no longer quoted, as Postgres reads them as the column. --- .../src/workspace/server.tests.rs | 6 +- .../src/workspace/server/expand_star.rs | 91 ++++++++----------- 2 files changed, 41 insertions(+), 56 deletions(-) diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index d635618a7..7ad10b24e 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -3348,7 +3348,7 @@ async fn test_code_actions_expand_star(test_db: PgPool) { }; // The columns of a table of the database and of one created in the file, qualified since - // there are two FROM items. + // there are two FROM items. After a dot, the keyword `select` needs no quotes. let actions = expand_actions("* from star_items"); assert_eq!(actions.len(), 1, "{actions:?}"); let (title, edit) = &actions[0]; @@ -3361,7 +3361,7 @@ async fn test_code_actions_expand_star(test_db: PgPool) { edit.edits, vec![TextEdit { range: range_of("*"), - new_text: r#"i.id, i."Label", t.id, t.item_id, t."select""#.into(), + new_text: r#"i.id, i."Label", t.id, t.item_id, t.select"#.into(), }] ); @@ -3373,7 +3373,7 @@ async fn test_code_actions_expand_star(test_db: PgPool) { actions[0].1.edits, vec![TextEdit { range: range_of("t.*"), - new_text: r#"t.id, t.item_id, t."select""#.into(), + new_text: r#"t.id, t.item_id, t.select"#.into(), }] ); diff --git a/crates/pgls_workspace/src/workspace/server/expand_star.rs b/crates/pgls_workspace/src/workspace/server/expand_star.rs index 2b7807087..6f07cb7b3 100644 --- a/crates/pgls_workspace/src/workspace/server/expand_star.rs +++ b/crates/pgls_workspace/src/workspace/server/expand_star.rs @@ -2,6 +2,7 @@ use pgls_analyse::{ActionCategory, Applicability, RefactorKind}; use pgls_catalog::resolve::{Reference, StarColumn}; +use pgls_catalog::{NamePosition, quote_identifier}; use pgls_query::protobuf::{KeywordKind, ScanToken, Token}; use pgls_text_size::TextSize; @@ -125,7 +126,8 @@ fn is_name(token: &ScanToken) -> bool { || token.keyword_kind() != KeywordKind::NoKeyword } -/// The columns as a select list, e.g. `u.id, u."Name"`. +/// The columns as a select list, e.g. `u.id, u."Name"`. The first name of each column is a +/// `ColId`, and the names after a dot are labels, which can be any keyword. fn column_list(columns: &[StarColumn]) -> String { columns .iter() @@ -134,7 +136,15 @@ fn column_list(columns: &[StarColumn]) -> String { .qualifier .iter() .chain(std::iter::once(&column.name)) - .map(|name| quote_identifier(name)) + .enumerate() + .map(|(i, name)| { + let position = if i == 0 { + NamePosition::ColId + } else { + NamePosition::ColLabel + }; + quote_identifier(name, position) + }) .collect::>() .join(".") }) @@ -142,60 +152,11 @@ fn column_list(columns: &[StarColumn]) -> String { .join(", ") } -/// `name`, quoted unless it reads as the same name without quotes: no upper case ASCII letters, -/// and not a keyword other than an unreserved one. Like Postgres' lexer, any character that isn't -/// ASCII can be part of an unquoted name, and isn't folded to lower case ([`scan.l`]). -/// -/// [`scan.l`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/scan.l#L332 -fn quote_identifier(name: &str) -> String { - let plain = name - .chars() - .next() - .is_some_and(|first| first.is_ascii_lowercase() || first == '_' || !first.is_ascii()) - && name.chars().all(|c| { - c.is_ascii_lowercase() || c.is_ascii_digit() || matches!(c, '_' | '$') || !c.is_ascii() - }); - let keyword = || { - pgls_query::scan(name).map_or(true, |scan| { - scan.tokens.first().is_none_or(|token| { - !matches!( - token.keyword_kind(), - KeywordKind::NoKeyword | KeywordKind::UnreservedKeyword - ) - }) - }) - }; - if plain && !keyword() { - name.to_owned() - } else { - format!("\"{}\"", name.replace('"', "\"\"")) - } -} - #[cfg(test)] mod tests { use pgls_text_size::TextSize; - use super::{StarColumn, column_list, quote_identifier}; - - #[test] - fn quotes_names_that_need_it() { - assert_eq!(quote_identifier("user_id"), "user_id"); - assert_eq!(quote_identifier("_x$1"), "_x$1"); - // Unreserved keywords can be column names. - assert_eq!(quote_identifier("name"), "name"); - assert_eq!(quote_identifier("Name"), "\"Name\""); - assert_eq!(quote_identifier("1st"), "\"1st\""); - assert_eq!(quote_identifier("my col"), "\"my col\""); - assert_eq!(quote_identifier("a\"b"), "\"a\"\"b\""); - assert_eq!(quote_identifier("select"), "\"select\""); - assert_eq!(quote_identifier("between"), "\"between\""); - assert_eq!(quote_identifier("left"), "\"left\""); - // Characters that aren't ASCII need no quotes, and aren't folded. - assert_eq!(quote_identifier("größe"), "größe"); - assert_eq!(quote_identifier("Ölstand"), "Ölstand"); - assert_eq!(quote_identifier("Café"), "\"Café\""); - } + use super::{StarColumn, column_list}; #[test] fn only_stars_of_select_lists_count() { @@ -245,9 +206,33 @@ mod tests { name: "title".into(), }, ]; + assert_eq!(column_list(&columns), r#"u.id, public."Posts".user, title"#); + } + + #[test] + fn quotes_names_where_postgres_needs_it() { + let column = |qualifier: &[&str], name: &str| StarColumn { + qualifier: qualifier.iter().map(|part| part.to_string()).collect(), + name: name.into(), + }; + // Postgres reads a column name keyword (`between`) as a column, but not a reserved + // keyword (`select`) or a type or function name keyword (`left`). After a dot, any + // keyword is a column. + let columns = [ + column(&[], "between"), + column(&[], "select"), + column(&[], "left"), + column(&["t"], "select"), + column(&["left"], "id"), + column(&[], "Name"), + column(&[], "my col"), + column(&[], "a\"b"), + column(&[], "größe"), + column(&[], "Café"), + ]; assert_eq!( column_list(&columns), - r#"u.id, public."Posts"."user", title"# + r#"between, "select", "left", t.select, "left".id, "Name", "my col", "a""b", größe, "Café""# ); } }