diff --git a/crates/squawk_ide/src/code_actions/add_explicit_alias.rs b/crates/squawk_ide/src/code_actions/add_explicit_alias.rs index 6b29baac..9655e06f 100644 --- a/crates/squawk_ide/src/code_actions/add_explicit_alias.rs +++ b/crates/squawk_ide/src/code_actions/add_explicit_alias.rs @@ -1,10 +1,9 @@ use rowan::TextSize; use salsa::Database as Db; use squawk_syntax::ast::{self, AstNode}; +use squawk_syntax::quote::quote_column_alias; -use crate::{ - column_name::ColumnName, db::File, offsets::token_from_offset, quote::quote_column_alias, -}; +use crate::{column_name::ColumnName, db::File, offsets::token_from_offset}; use super::{ActionKind, CodeAction}; diff --git a/crates/squawk_ide/src/code_actions/unquote_identifier.rs b/crates/squawk_ide/src/code_actions/unquote_identifier.rs index 33f17239..c6acee65 100644 --- a/crates/squawk_ide/src/code_actions/unquote_identifier.rs +++ b/crates/squawk_ide/src/code_actions/unquote_identifier.rs @@ -2,7 +2,9 @@ use rowan::TextSize; use salsa::Database as Db; use squawk_syntax::ast::{self, AstNode}; -use crate::{db::File, offsets::token_from_offset, quote::unquote_ident}; +use squawk_syntax::quote::unquote_ident; + +use crate::{db::File, offsets::token_from_offset}; use super::{ActionKind, CodeAction}; diff --git a/crates/squawk_ide/src/column_name.rs b/crates/squawk_ide/src/column_name.rs index 47115bcf..c1781132 100644 --- a/crates/squawk_ide/src/column_name.rs +++ b/crates/squawk_ide/src/column_name.rs @@ -3,7 +3,7 @@ use squawk_syntax::{ ast::{self, AstNode}, }; -use crate::quote::normalize_identifier; +use squawk_syntax::quote::normalize_identifier; #[derive(Clone, Debug, PartialEq)] pub(crate) enum ColumnName { diff --git a/crates/squawk_ide/src/generated/mod.rs b/crates/squawk_ide/src/generated/mod.rs index c455eeca..e69de29b 100644 --- a/crates/squawk_ide/src/generated/mod.rs +++ b/crates/squawk_ide/src/generated/mod.rs @@ -1 +0,0 @@ -pub(crate) mod keywords; diff --git a/crates/squawk_ide/src/lib.rs b/crates/squawk_ide/src/lib.rs index 86a9680e..1e7f4598 100644 --- a/crates/squawk_ide/src/lib.rs +++ b/crates/squawk_ide/src/lib.rs @@ -13,7 +13,6 @@ pub mod expand_selection; mod file; pub mod find_references; pub mod folding_ranges; -mod generated; pub mod goto_definition; pub mod hover; mod infer; @@ -21,7 +20,6 @@ pub mod inlay_hints; pub mod location; mod name; mod offsets; -mod quote; mod resolve; mod scope; pub mod semantic_tokens; diff --git a/crates/squawk_ide/src/name.rs b/crates/squawk_ide/src/name.rs index 8f8b751f..348348c9 100644 --- a/crates/squawk_ide/src/name.rs +++ b/crates/squawk_ide/src/name.rs @@ -2,7 +2,7 @@ use smol_str::SmolStr; use squawk_syntax::ast::{self, AstNode}; use std::fmt; -use crate::quote::normalize_identifier; +use squawk_syntax::quote::normalize_identifier; #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub(crate) struct Name(pub(crate) SmolStr); diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index e47bb38f..9012f5fc 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -43,6 +43,7 @@ use rules::ban_uncommitted_transaction; use rules::changing_column_type; use rules::constraint_missing_not_valid; use rules::disallow_unique_constraint; +use rules::identifier_too_long; use rules::prefer_bigint_over_int; use rules::prefer_bigint_over_smallint; use rules::prefer_identity; @@ -94,6 +95,7 @@ pub enum Rule { BanUncommittedTransaction, RequireEnumValueOrdering, RequireTableSchema, + IdentifierTooLong, // xtask:new-rule:error-name } @@ -146,6 +148,7 @@ impl TryFrom<&str> for Rule { "ban-uncommitted-transaction" => Ok(Rule::BanUncommittedTransaction), "require-enum-value-ordering" => Ok(Rule::RequireEnumValueOrdering), "require-table-schema" => Ok(Rule::RequireTableSchema), + "identifier-too-long" => Ok(Rule::IdentifierTooLong), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -210,6 +213,7 @@ impl fmt::Display for Rule { Rule::BanUncommittedTransaction => "ban-uncommitted-transaction", Rule::RequireEnumValueOrdering => "require-enum-value-ordering", Rule::RequireTableSchema => "require-table-schema", + Rule::IdentifierTooLong => "identifier-too-long", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -443,6 +447,9 @@ impl Linter { if self.rules.contains(&Rule::RequireTableSchema) { require_table_schema(self, file); } + if self.rules.contains(&Rule::IdentifierTooLong) { + identifier_too_long(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/identifier_too_long.rs b/crates/squawk_linter/src/rules/identifier_too_long.rs new file mode 100644 index 00000000..7858aadf --- /dev/null +++ b/crates/squawk_linter/src/rules/identifier_too_long.rs @@ -0,0 +1,216 @@ +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use squawk_syntax::quote::normalize_identifier; + +use crate::{Edit, Fix, Linter, Rule, Violation}; + +// via: https://github.com/postgres/postgres/blob/228a1f9542792c6533ef74c2e7aefad0da1d9a7a/src/include/pg_config_manual.h#L39C6-L39C6 +const NAMEDATALEN: usize = 64; +const MAX_IDENT_BYTES: usize = NAMEDATALEN - 1; + +pub(crate) fn identifier_too_long(ctx: &mut Linter, parse: &Parse) { + for node in parse.tree().syntax().descendants() { + if let Some(name) = ast::Name::cast(node.clone()) { + check_name(ctx, &name); + } else if let Some(name_ref) = ast::NameRef::cast(node) { + check_name(ctx, &name_ref); + } + } +} + +fn check_name(ctx: &mut Linter, name_like: &impl ast::NameLike) { + let text = name_like.syntax().text().to_string(); + let ident = normalize_identifier(&text); + if ident.len() <= MAX_IDENT_BYTES { + return; + } + + let fix = truncate(&text).map(|truncated| { + Fix::new( + format!("Rename to `{truncated}`"), + vec![Edit::replace(name_like.syntax().text_range(), truncated)], + ) + }); + + ctx.report( + Violation::for_node( + Rule::IdentifierTooLong, + format!("`{ident}` is too long and will be truncated to {MAX_IDENT_BYTES} bytes."), + name_like.syntax(), + ) + .fix(fix), + ); +} + +fn truncate(text: &str) -> Option { + if has_escaped_quotes(text) { + return None; + } + + let unquoted = normalize_identifier(text); + let truncated = &unquoted[..unquoted.floor_char_boundary(MAX_IDENT_BYTES)]; + + Some(if text.starts_with('"') { + format!("\"{truncated}\"") + } else { + truncated.to_owned() + }) +} + +fn has_escaped_quotes(text: &str) -> bool { + text.strip_prefix('"') + .and_then(|t| t.strip_suffix('"')) + .is_some_and(|text| text.contains(r#""""#)) +} + +#[cfg(test)] +mod test { + use insta::assert_snapshot; + + use crate::Rule; + use crate::test_utils::{lint_errors, lint_ok}; + + #[test] + fn create_table_long_name_err() { + let sql = r#" +create table table_very_long_very_long_very_long_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_long_very_long bigint); + "#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @" + warning[identifier-too-long]: `table_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ create table table_very_long_very_long_very_long_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_lon… + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - create table table_very_long_very_long_very_long_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_long_very_long bigint); + 2 + create table table_very_long_very_long_very_long_very_long_very_long_very_lo (column_very_long_very_long_very_long_very_long_very_long_very_long bigint); + ╰╴ + warning[identifier-too-long]: `column_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ …ng_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_long_very_long bigint); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - create table table_very_long_very_long_very_long_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_long_very_long bigint); + 2 + create table table_very_long_very_long_very_long_very_long_very_long_very_long (column_very_long_very_long_very_long_very_long_very_long_very_l bigint); + ╰╴ + "); + } + + #[test] + fn create_table_ok() { + let sql = r#" +create table short_name (col bigint); + "#; + lint_ok(sql, Rule::IdentifierTooLong); + } + + #[test] + fn create_index_long_name_err() { + let sql = r#" +create index index_very_long_very_long_very_long_very_long_very_long_very_long on t (col); + "#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @" + warning[identifier-too-long]: `index_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ create index index_very_long_very_long_very_long_very_long_very_long_very_long on t (col); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - create index index_very_long_very_long_very_long_very_long_very_long_very_long on t (col); + 2 + create index index_very_long_very_long_very_long_very_long_very_long_very_lo on t (col); + ╰╴ + "); + } + + #[test] + fn drop_table_long_name_err() { + let sql = r#" +drop table table_very_long_very_long_very_long_very_long_very_long_very_long; + "#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @" + warning[identifier-too-long]: `table_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ drop table table_very_long_very_long_very_long_very_long_very_long_very_long; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - drop table table_very_long_very_long_very_long_very_long_very_long_very_long; + 2 + drop table table_very_long_very_long_very_long_very_long_very_long_very_lo; + ╰╴ + "); + } + + #[test] + fn alter_table_add_column_long_name_err() { + let sql = r#" +alter table t add column column_very_long_very_long_very_long_very_long_very_long_very_long bigint; + "#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @" + warning[identifier-too-long]: `column_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ alter table t add column column_very_long_very_long_very_long_very_long_very_long_very_long bigint; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - alter table t add column column_very_long_very_long_very_long_very_long_very_long_very_long bigint; + 2 + alter table t add column column_very_long_very_long_very_long_very_long_very_long_very_l bigint; + ╰╴ + "); + } + + #[test] + fn quoted_identifier_long_err() { + let sql = r#" +create table "table_very_long_very_long_very_long_very_long_very_long_very_long" (id bigint); + "#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @r#" + warning[identifier-too-long]: `table_very_long_very_long_very_long_very_long_very_long_very_long` is too long and will be truncated to 63 bytes. + ╭▸ + 2 │ create table "table_very_long_very_long_very_long_very_long_very_long_very_long" (id bigint); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 2 - create table "table_very_long_very_long_very_long_very_long_very_long_very_long" (id bigint); + 2 + create table "table_very_long_very_long_very_long_very_long_very_long_very_lo" (id bigint); + ╰╴ + "#); + } + + #[test] + fn multibyte_emoji_err() { + // postgres ends up slicing the emoji in two + let sql = r#"create table "👨‍👩‍👧‍👦👨‍👩‍👧‍👦👨‍👩‍👧‍👦" (id bigint);"#; + assert_snapshot!(lint_errors(sql, Rule::IdentifierTooLong), @r#" + warning[identifier-too-long]: `👨👩👧👦👨👩👧👦👨👩👧👦` is too long and will be truncated to 63 bytes. + ╭▸ + 1 │ create table "👨👩👧👦👨👩👧👦👨👩👧👦" (id bigint); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━ + ╭╴ + 1 - create table "👨👩👧👦👨👩👧👦👨👩👧👦" (id bigint); + 1 + create table "👨👩👧👦👨👩👧👦👨👩" (id bigint); + ╰╴ + "#); + } + + #[test] + fn quoted_identifier_with_escaped_quotes_message_unescapes_quotes() { + let identifier = format!(r#""{}""b""#, "a".repeat(63)); + // ^^ escaped quote + let sql = format!("create table {identifier} (id bigint);"); + // quoting is complicated so we don't add a fix + assert_snapshot!(lint_errors(&sql, Rule::IdentifierTooLong), @r#" + warning[identifier-too-long]: `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"b` is too long and will be truncated to 63 bytes. + ╭▸ + 1 │ create table "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa""b" (id bigint); + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + "#); + } + + #[test] + fn exactly_63_bytes_ok() { + // exactly 63 bytes -- should not trigger + let sql = r#" +create table table_very_long_very_long_very_long_very_long_very_long_very_lo (id bigint); +-- ^ exactly 63 bytes + "#; + lint_ok(sql, Rule::IdentifierTooLong); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index bd5069f2..b4550e50 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -16,6 +16,7 @@ pub(crate) mod ban_uncommitted_transaction; pub(crate) mod changing_column_type; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; +pub(crate) mod identifier_too_long; pub(crate) mod prefer_bigint_over_int; pub(crate) mod prefer_bigint_over_smallint; pub(crate) mod prefer_identity; @@ -50,6 +51,7 @@ pub(crate) use ban_uncommitted_transaction::ban_uncommitted_transaction; pub(crate) use changing_column_type::changing_column_type; pub(crate) use constraint_missing_not_valid::constraint_missing_not_valid; pub(crate) use disallow_unique_constraint::disallow_unique_constraint; +pub(crate) use identifier_too_long::identifier_too_long; pub(crate) use prefer_bigint_over_int::prefer_bigint_over_int; pub(crate) use prefer_bigint_over_smallint::prefer_bigint_over_smallint; pub(crate) use prefer_identity::prefer_identity; diff --git a/crates/squawk_ide/src/generated/keywords.rs b/crates/squawk_syntax/src/generated/keywords.rs similarity index 100% rename from crates/squawk_ide/src/generated/keywords.rs rename to crates/squawk_syntax/src/generated/keywords.rs diff --git a/crates/squawk_syntax/src/generated/mod.rs b/crates/squawk_syntax/src/generated/mod.rs new file mode 100644 index 00000000..c455eeca --- /dev/null +++ b/crates/squawk_syntax/src/generated/mod.rs @@ -0,0 +1 @@ +pub(crate) mod keywords; diff --git a/crates/squawk_syntax/src/lib.rs b/crates/squawk_syntax/src/lib.rs index e52bb0f4..4863d94f 100644 --- a/crates/squawk_syntax/src/lib.rs +++ b/crates/squawk_syntax/src/lib.rs @@ -25,9 +25,11 @@ // DEALINGS IN THE SOFTWARE. pub mod ast; +mod generated; pub mod identifier; mod parsing; mod ptr; +pub mod quote; pub mod syntax_error; mod syntax_node; mod token_text; diff --git a/crates/squawk_ide/src/quote.rs b/crates/squawk_syntax/src/quote.rs similarity index 89% rename from crates/squawk_ide/src/quote.rs rename to crates/squawk_syntax/src/quote.rs index e337d050..0b2944ab 100644 --- a/crates/squawk_ide/src/quote.rs +++ b/crates/squawk_syntax/src/quote.rs @@ -1,8 +1,7 @@ -use squawk_syntax::SyntaxNode; - +use crate::SyntaxNode; use crate::generated::keywords::RESERVED_KEYWORDS; -pub(crate) fn quote_column_alias(text: &str) -> String { +pub fn quote_column_alias(text: &str) -> String { if needs_quoting(text) { format!(r#""{}""#, text.replace('"', r#""""#)) } else { @@ -10,7 +9,7 @@ pub(crate) fn quote_column_alias(text: &str) -> String { } } -pub(crate) fn unquote_ident(node: &SyntaxNode) -> Option { +pub fn unquote_ident(node: &SyntaxNode) -> Option { let text = node.text().to_string(); if !text.starts_with('"') || !text.ends_with('"') { @@ -71,17 +70,17 @@ fn needs_quoting(text: &str) -> bool { false } -pub(crate) fn is_reserved_word(text: &str) -> bool { +pub fn is_reserved_word(text: &str) -> bool { RESERVED_KEYWORDS .binary_search(&text.to_lowercase().as_str()) .is_ok() } -pub(crate) fn normalize_identifier(text: &str) -> String { +pub fn normalize_identifier(text: &str) -> String { // TODO: Cow/SmolStr/Salsa Interned? text.strip_prefix('"') .and_then(|t| t.strip_suffix('"')) - .map(|x| x.to_string()) + .map(|x| x.replace(r#""""#, "\"")) .unwrap_or_else(|| text.to_ascii_lowercase()) } diff --git a/crates/xtask/src/codegen.rs b/crates/xtask/src/codegen.rs index dda9211a..6a3e3a97 100644 --- a/crates/xtask/src/codegen.rs +++ b/crates/xtask/src/codegen.rs @@ -95,7 +95,8 @@ pub(crate) fn codegen() -> Result<()> { project_root().join("crates/squawk_parser/src/generated/syntax_kind.rs"); std::fs::write(syntax_kinds_file, syntax_kinds).context("problem writing syntax kinds")?; - let ide_reserved_keywords = project_root().join("crates/squawk_ide/src/generated/keywords.rs"); + let ide_reserved_keywords = + project_root().join("crates/squawk_syntax/src/generated/keywords.rs"); let reserved_keywords = generate_reserved_keywords_array(&keyword_kinds.reserved_keywords)?; std::fs::write(ide_reserved_keywords, reserved_keywords) .context("problem writing reserved keywords")?; diff --git a/docs/docs/identifier-too-long.md b/docs/docs/identifier-too-long.md new file mode 100644 index 00000000..f89ec55e --- /dev/null +++ b/docs/docs/identifier-too-long.md @@ -0,0 +1,39 @@ +--- +id: identifier-too-long +title: identifier-too-long +--- + +## problem + +Postgres will truncate identifiers longer than 63 bytes and return a notice. + +This can result in suprising behavior as the names you provide aren't the names that Postgres uses. + +```sql +-- table name is 65 bytes and will be silently truncated to 63 +create table table_very_long_very_long_very_long_very_long_very_long_very_long ( + -- column name is 66 bytes and will also be silently truncated + column_very_long_very_long_very_long_very_long_very_long_very_long bigint +); +``` + +``` +NOTICE: identifier "table_very_long_very_long_very_long_very_long_very_long_very_long" will be truncated to "table_very_long_very_long_very_long_very_long_very_long_very_lo" +NOTICE: identifier "column_very_long_very_long_very_long_very_long_very_long_very_long" will be truncated to "column_very_long_very_long_very_long_very_long_very_long_very_l" + +Query 1 OK: CREATE TABLE +``` + +## solution + +Shorten the identifier! + +```sql +create table a_shorter_table_name ( + a_shorter_column_name bigint +); +``` + +## links + +- [Identifiers and Key Words](https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS) diff --git a/docs/sidebars.js b/docs/sidebars.js index 0ab3b93b..df11b267 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -36,6 +36,7 @@ module.exports = { "ban-uncommitted-transaction", "require-enum-value-ordering", "require-table-schema", + "identifier-too-long", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index bd7aa4ef..344bdbcd 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -212,6 +212,11 @@ const rules = [ tags: ["schema"], description: "Require explicit schema in table DDL to avoid ambiguity.", }, + { + name: "identifier-too-long", + tags: ["schema"], + description: "Warn about implicit truncation for identifiers that are too long.", + }, // xtask:new-rule:rule-doc-meta ]