From 15b6f7d34b3e5c1c66d128dc246f2a8430bca05a Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 19 Oct 2026 06:10:50 +0000 Subject: [PATCH 1/3] feat(workspace): return rows from the execute statement command and explain why it is disabled --- crates/pgls_lsp/src/handlers/code_actions.rs | 5 +- crates/pgls_lsp/src/server.rs | 3 +- crates/pgls_lsp/tests/server.rs | 14 +- .../src/features/code_actions.rs | 23 ++ crates/pgls_workspace/src/settings.rs | 116 ++++++++- crates/pgls_workspace/src/workspace/server.rs | 45 ++-- .../src/workspace/server.tests.rs | 227 +++++++++++++++++- .../workspace/server/statement_execution.rs | 89 +++++++ docs/guides/configure_database.md | 24 ++ 9 files changed, 507 insertions(+), 39 deletions(-) create mode 100644 crates/pgls_workspace/src/workspace/server/statement_execution.rs diff --git a/crates/pgls_lsp/src/handlers/code_actions.rs b/crates/pgls_lsp/src/handlers/code_actions.rs index 726e9dd61..9e4e72a23 100644 --- a/crates/pgls_lsp/src/handlers/code_actions.rs +++ b/crates/pgls_lsp/src/handlers/code_actions.rs @@ -116,10 +116,11 @@ pub async fn execute_command( session .client - .show_message(MessageType::INFO, result.message) + .show_message(MessageType::INFO, &result.message) .await; - Ok(None) + // The rows go back to the client, so editor extensions can render them as a table. + Ok(Some(serde_json::to_value(result)?)) } "pgls.invalidateSchemaCache" => { session.workspace.invalidate_schema_cache(true)?; diff --git a/crates/pgls_lsp/src/server.rs b/crates/pgls_lsp/src/server.rs index 5fb902ea2..967412e1e 100644 --- a/crates/pgls_lsp/src/server.rs +++ b/crates/pgls_lsp/src/server.rs @@ -300,8 +300,7 @@ impl LanguageServer for LSPServer { params: ExecuteCommandParams, ) -> LspResult> { match handlers::code_actions::execute_command(&self.session, params).await { - // we'll inform the client within `code_actions::execute_command` - Ok(_) => LspResult::Ok(None), + Ok(result) => LspResult::Ok(result), Err(err) => LspResult::Err(into_lsp_error(err)), } } diff --git a/crates/pgls_lsp/tests/server.rs b/crates/pgls_lsp/tests/server.rs index c47ce78d6..ec2258ad7 100644 --- a/crates/pgls_lsp/tests/server.rs +++ b/crates/pgls_lsp/tests/server.rs @@ -1239,6 +1239,7 @@ async fn test_execute_statement(test_db: PgPool) -> Result<()> { conf.merge_with(PartialConfiguration { db: Some(PartialDatabaseConfiguration { database: Some(database), + allow_statement_executions_against: Some(StringSet::from_iter([format!("{host}/*")])), host: Some(host), ..Default::default() }), @@ -1340,7 +1341,7 @@ async fn test_execute_statement(test_db: PgPool) -> Result<()> { }) .expect("Did not find executeStatement command!"); - server + let result = server .request::>( "workspace/executeCommand", "_execStmt", @@ -1352,6 +1353,17 @@ async fn test_execute_statement(test_db: PgPool) -> Result<()> { ) .await?; + assert_eq!( + result, + Some(Some(json!({ + "message": "Successfully executed statement. Rows affected: 0", + "columns": [], + "rows": [], + "truncated": false, + "rowsAffected": 0, + }))) + ); + assert!( users_tbl_exists().await, "Users table did not exists even though it should've been created by the workspace/executeStatement command." diff --git a/crates/pgls_workspace/src/features/code_actions.rs b/crates/pgls_workspace/src/features/code_actions.rs index fb16e49d2..7592b02d6 100644 --- a/crates/pgls_workspace/src/features/code_actions.rs +++ b/crates/pgls_workspace/src/features/code_actions.rs @@ -58,8 +58,31 @@ pub struct ExecuteStatementParams { pub path: PgLSPath, } +/// The most rows an executed statement returns; the rest are dropped and the result is marked +/// as truncated. +pub const EXECUTE_STATEMENT_MAX_ROWS: usize = 100; + +/// The outcome of executing a statement. +/// +/// A statement that returns rows (`select`, `values`, `returning`, `show`, `explain`, ...) fills +/// `columns` and `rows`. Any other statement sets `rows_affected` instead. #[derive(Debug, serde::Serialize, serde::Deserialize, Default, PartialEq, Eq)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +#[serde(rename_all = "camelCase")] pub struct ExecuteStatementResult { + /// A short summary for the user. pub message: String, + /// The names of the returned columns. + #[serde(default)] + pub columns: Vec, + /// The returned rows, at most [`EXECUTE_STATEMENT_MAX_ROWS`]. Each value is in its Postgres + /// text representation, as `psql` shows it, and `null` for SQL `NULL`. + #[serde(default)] + pub rows: Vec>>, + /// Whether the statement returned more rows than `rows` holds. + #[serde(default)] + pub truncated: bool, + /// The number of rows affected, for a statement that returns no rows. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rows_affected: Option, } diff --git a/crates/pgls_workspace/src/settings.rs b/crates/pgls_workspace/src/settings.rs index e289a03b4..5068ebabc 100644 --- a/crates/pgls_workspace/src/settings.rs +++ b/crates/pgls_workspace/src/settings.rs @@ -10,7 +10,7 @@ use std::{ sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}, time::Duration, }; -use tracing::trace; +use tracing::{trace, warn}; use ignore::gitignore::{Gitignore, GitignoreBuilder}; use pgls_configuration::{ @@ -744,6 +744,33 @@ pub struct DatabaseSettings { pub database: String, pub conn_timeout_secs: Duration, pub allow_statement_executions: bool, + /// The patterns of `db.allowStatementExecutionsAgainst` that are not valid globs. + pub invalid_statement_execution_patterns: Vec, +} + +impl DatabaseSettings { + /// Why statements cannot be executed against this database, or `None` if they can. + pub fn statement_execution_disabled_reason(&self) -> Option { + if self.allow_statement_executions { + return None; + } + + let host = &self.host; + let database = &self.database; + let mut reason = format!( + "Statement execution is disabled for `{host}/{database}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{host}/*\"]." + ); + if !self.invalid_statement_execution_patterns.is_empty() { + let patterns = self + .invalid_statement_execution_patterns + .iter() + .map(|pattern| format!("`{pattern}`")) + .collect::>() + .join(", "); + reason.push_str(&format!(" Ignored invalid patterns: {patterns}.")); + } + Some(reason) + } } impl std::fmt::Debug for DatabaseSettings { @@ -764,6 +791,10 @@ impl std::fmt::Debug for DatabaseSettings { "allow_statement_executions", &self.allow_statement_executions, ) + .field( + "invalid_statement_execution_patterns", + &self.invalid_statement_execution_patterns, + ) .finish() } } @@ -780,6 +811,7 @@ impl Default for DatabaseSettings { database: "postgres".to_string(), conn_timeout_secs: Duration::from_secs(10), allow_statement_executions: true, + invalid_statement_execution_patterns: Vec::new(), } } } @@ -827,18 +859,26 @@ impl From for DatabaseSettings { } } - let allow_statement_executions = value + // An invalid pattern is ignored rather than failing the whole configuration; the reason a + // statement cannot be executed names it. + let target = format!("{host}/{database}"); + let mut allow_statement_executions = false; + let mut invalid_statement_execution_patterns = Vec::new(); + for pattern in value .allow_statement_executions_against - .map(|stringset| { - stringset.iter().any(|pattern| { - let glob = Glob::new(pattern) - .unwrap_or_else(|_| panic!("Invalid pattern: {pattern}")) - .compile_matcher(); - - glob.is_match(format!("{host}/{database}")) - }) - }) - .unwrap_or(false); + .iter() + .flat_map(|set| set.iter()) + { + match Glob::new(pattern) { + Ok(glob) => allow_statement_executions |= glob.compile_matcher().is_match(&target), + Err(err) => { + warn!( + "Ignoring invalid pattern `{pattern}` in `db.allowStatementExecutionsAgainst`: {err}" + ); + invalid_statement_execution_patterns.push(pattern.clone()); + } + } + } Self { enable_connection, @@ -857,6 +897,7 @@ impl From for DatabaseSettings { .unwrap_or(d.conn_timeout_secs), allow_statement_executions, + invalid_statement_execution_patterns, } } } @@ -974,7 +1015,56 @@ mod tests { let config = DatabaseSettings::from(partial_config); - assert!(!config.allow_statement_executions) + assert!(!config.allow_statement_executions); + assert_eq!( + config.statement_execution_disabled_reason().as_deref(), + Some( + "Statement execution is disabled for `production/test-db`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"production/*\"]." + ) + ); + } + + #[test] + fn ignores_invalid_statement_execution_patterns() { + let partial_config = PartialDatabaseConfiguration { + allow_statement_executions_against: Some(StringSet::from_iter(vec![ + String::from("local[host/*"), + String::from("localhost/*"), + ])), + host: Some("localhost".into()), + database: Some("test-db".into()), + ..Default::default() + }; + + let config = DatabaseSettings::from(partial_config); + + assert!(config.allow_statement_executions); + assert_eq!( + config.invalid_statement_execution_patterns, + ["local[host/*"] + ); + } + + #[test] + fn names_invalid_statement_execution_patterns_in_disabled_reason() { + let partial_config = PartialDatabaseConfiguration { + allow_statement_executions_against: Some(StringSet::from_iter(vec![String::from( + "local[host/*", + )])), + host: Some("localhost".into()), + database: Some("test-db".into()), + ..Default::default() + }; + + let config = DatabaseSettings::from(partial_config); + + assert!(!config.allow_statement_executions); + assert!( + config + .statement_execution_disabled_reason() + .unwrap() + .ends_with("Ignored invalid patterns: `local[host/*`.") + ); } #[cfg(feature = "db")] diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index 33946c974..cef3436ac 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -37,8 +37,6 @@ use pgls_text_size::{TextRange, TextSize}; use pgls_typecheck::TypecheckParams; use pgls_workspace_macros::ignored_path; use snapshot_manager::SnapshotManager; -#[cfg(feature = "db")] -use sqlx::Executor; use tracing::{debug, info}; use crate::{ @@ -84,6 +82,8 @@ mod pg_query; mod pgpass; mod snapshot_manager; mod sql_function; +#[cfg(feature = "db")] +mod statement_execution; mod statement_identifier; mod tree_sitter; @@ -654,8 +654,7 @@ impl Workspace for WorkspaceServer { let settings = settings.settings(); let disabled_reason = match settings { - Some(settings) if settings.db.allow_statement_executions => None, - Some(_) => Some("Statement execution is disabled in the settings.".into()), + Some(settings) => settings.db.statement_execution_disabled_reason(), None => Some("Statement execution not allowed against database.".into()), }; @@ -724,6 +723,7 @@ impl Workspace for WorkspaceServer { if stmt.is_none() { return Ok(ExecuteStatementResult { message: "Statement was not found in document.".into(), + ..Default::default() }); }; @@ -732,29 +732,35 @@ impl Workspace for WorkspaceServer { if ast.is_none() { return Ok(ExecuteStatementResult { message: "Statement is invalid.".into(), + ..Default::default() }); }; let settings = self.workspaces(); - let Some(result) = settings.settings().and_then(|settings| { - self.connection.with_pool(&settings.db, |pool| { - let pool = pool.clone(); - run_async(async move { pool.execute(sqlx::query(&content)).await })? - .map_err(WorkspaceError::from) - }) - }) else { + let no_connection = ExecuteStatementResult { + message: "No database connection available.".into(), + ..Default::default() + }; + let Some(settings) = settings.settings() else { + return Ok(no_connection); + }; + + if let Some(reason) = settings.db.statement_execution_disabled_reason() { return Ok(ExecuteStatementResult { - message: "No database connection available.".into(), + message: reason, + ..Default::default() }); + } + + let Some(result) = self.connection.with_pool(&settings.db, |pool| { + let pool = pool.clone(); + run_async(async move { statement_execution::execute_statement(&pool, &content).await })? + .map_err(WorkspaceError::from) + }) else { + return Ok(no_connection); }; - let result = result?; - Ok(ExecuteStatementResult { - message: format!( - "Successfully executed statement. Rows affected: {}", - result.rows_affected() - ), - }) + result } #[cfg(not(feature = "db"))] @@ -766,6 +772,7 @@ impl Workspace for WorkspaceServer { ) -> Result { Ok(ExecuteStatementResult { message: "Statement execution requires database support.".into(), + ..Default::default() }) } diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index 3cdcf78c7..cadb826cb 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -16,12 +16,15 @@ use pgls_configuration::{ use pgls_configuration::plpgsql_check::PartialPlPgSqlCheckConfiguration; use pgls_diagnostics::Diagnostic; use pgls_fs::PgLSPath; -use pgls_text_size::TextRange; +use pgls_text_size::{TextRange, TextSize}; use sqlx::{Executor, PgPool}; use crate::{ Workspace, WorkspaceError, - features::code_actions::ExecuteStatementResult, + features::code_actions::{ + CodeActionKind, CodeActionsParams, CommandAction, CommandActionCategory, + EXECUTE_STATEMENT_MAX_ROWS, ExecuteStatementParams, ExecuteStatementResult, + }, features::format::PullFileFormattingParams, workspace::{ OpenFileParams, RegisterProjectFolderParams, StatementId, UpdateSettingsParams, @@ -1814,3 +1817,223 @@ fn is_typecheck(diagnostic: &pgls_diagnostics::serde::Diagnostic) -> bool { }) }) } + +fn get_statement_execution_workspace( + test_db: &PgPool, + allow_statement_executions_against: &[String], +) -> WorkspaceServer { + let mut conf = PartialConfiguration::init(); + conf.merge_with(PartialConfiguration { + db: Some(PartialDatabaseConfiguration { + host: Some(test_db.connect_options().get_host().to_string()), + database: Some( + test_db + .connect_options() + .get_database() + .unwrap() + .to_string(), + ), + allow_statement_executions_against: Some(StringSet::from_iter( + allow_statement_executions_against.iter().cloned(), + )), + ..Default::default() + }), + ..Default::default() + }); + + get_test_workspace(Some(conf)).expect("Unable to create test workspace") +} + +/// Executes the single statement in `sql` through its code action, returning the action's +/// disabled reason and the result. +fn execute_sql(workspace: &WorkspaceServer, sql: &str) -> (Option, ExecuteStatementResult) { + let path = PgLSPath::new("execute.sql"); + workspace + .open_file(OpenFileParams { + path: path.clone(), + content: sql.into(), + version: 1, + }) + .expect("Unable to open test file"); + + let action = workspace + .pull_code_actions(CodeActionsParams { + path: path.clone(), + cursor_position: TextSize::new(0), + only: vec![], + skip: vec![], + }) + .expect("Unable to pull code actions") + .actions + .into_iter() + .find_map(|action| match action.kind { + CodeActionKind::Command(CommandAction { + category: CommandActionCategory::ExecuteStatement(statement_id), + }) => Some((action.disabled_reason, statement_id)), + _ => None, + }); + let (disabled_reason, statement_id) = + action.expect("Did not find the execute statement action"); + + let result = workspace + .execute_statement(ExecuteStatementParams { + path: path.clone(), + statement_id, + }) + .expect("Unable to execute statement"); + + workspace + .close_file(crate::workspace::CloseFileParams { path }) + .expect("Unable to close test file"); + + (disabled_reason, result) +} + +fn allow_test_host(test_db: &PgPool) -> Vec { + vec![format!("{}/*", test_db.connect_options().get_host())] +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_is_disabled_without_matching_pattern(test_db: PgPool) { + let workspace = get_statement_execution_workspace(&test_db, &[]); + + let (disabled_reason, result) = execute_sql(&workspace, "create table not_created (id int);"); + + let host = test_db.connect_options().get_host().to_string(); + let database = test_db + .connect_options() + .get_database() + .unwrap() + .to_string(); + let reason = format!( + "Statement execution is disabled for `{host}/{database}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{host}/*\"]." + ); + assert_eq!(disabled_reason.as_deref(), Some(reason.as_str())); + assert_eq!( + result, + ExecuteStatementResult { + message: reason, + ..Default::default() + } + ); + + let table_exists: bool = sqlx::query_scalar( + "select exists (select 1 from pg_catalog.pg_tables where tablename = 'not_created')", + ) + .fetch_one(&test_db) + .await + .unwrap(); + assert!(!table_exists, "A disabled statement must not be executed"); +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_ignores_invalid_patterns(test_db: PgPool) { + let mut patterns = vec!["local[host/*".to_string()]; + patterns.extend(allow_test_host(&test_db)); + let workspace = get_statement_execution_workspace(&test_db, &patterns); + + let (disabled_reason, result) = execute_sql(&workspace, "select 1 as one;"); + + assert_eq!(disabled_reason, None); + assert_eq!(result.rows, [[Some("1".to_string())]]); +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_returns_rows(test_db: PgPool) { + let workspace = get_statement_execution_workspace(&test_db, &allow_test_host(&test_db)); + + let (_, result) = execute_sql( + &workspace, + "select 1 as id, null::text as name, array[1, 2] as numbers, interval '1 day' as span, '{\"a\": 1}'::jsonb as data + union all + select 2, 'two', null, null, null;", + ); + + assert_eq!( + result, + ExecuteStatementResult { + message: "2 rows".into(), + columns: ["id", "name", "numbers", "span", "data"] + .map(String::from) + .into(), + rows: vec![ + vec![ + Some("1".into()), + None, + Some("{1,2}".into()), + Some("1 day".into()), + Some("{\"a\": 1}".into()), + ], + vec![Some("2".into()), Some("two".into()), None, None, None], + ], + truncated: false, + rows_affected: None, + } + ); +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_returns_columns_without_rows(test_db: PgPool) { + let workspace = get_statement_execution_workspace(&test_db, &allow_test_host(&test_db)); + + let (_, result) = execute_sql(&workspace, "select 1 as id where false;"); + + assert_eq!( + result, + ExecuteStatementResult { + message: "0 rows".into(), + columns: vec!["id".into()], + ..Default::default() + } + ); +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_truncates_rows(test_db: PgPool) { + let workspace = get_statement_execution_workspace(&test_db, &allow_test_host(&test_db)); + + let (_, result) = execute_sql(&workspace, "select generate_series(1, 150) as n;"); + + assert!(result.truncated); + assert_eq!(result.rows.len(), EXECUTE_STATEMENT_MAX_ROWS); + assert_eq!(result.rows.last(), Some(&vec![Some("100".to_string())])); + assert_eq!(result.rows_affected, None); + assert_eq!(result.message, "Showing the first 100 rows (truncated)."); + + // The connection is still usable after the dropped rows. + let (_, result) = execute_sql(&workspace, "select 'next' as value;"); + assert_eq!(result.rows, [[Some("next".to_string())]]); +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn execute_statement_reports_rows_affected(test_db: PgPool) { + let workspace = get_statement_execution_workspace(&test_db, &allow_test_host(&test_db)); + + let (_, result) = execute_sql(&workspace, "create table executed (id int);"); + assert_eq!( + result, + ExecuteStatementResult { + message: "Successfully executed statement. Rows affected: 0".into(), + rows_affected: Some(0), + ..Default::default() + } + ); + + let (_, result) = execute_sql(&workspace, "insert into executed values (1), (2);"); + assert_eq!( + result, + ExecuteStatementResult { + message: "Successfully executed statement. Rows affected: 2".into(), + rows_affected: Some(2), + ..Default::default() + } + ); + + let (_, result) = execute_sql( + &workspace, + "delete from executed where id = 1 returning id;", + ); + assert_eq!(result.columns, ["id"]); + assert_eq!(result.rows, [[Some("1".to_string())]]); + assert_eq!(result.rows_affected, None); +} diff --git a/crates/pgls_workspace/src/workspace/server/statement_execution.rs b/crates/pgls_workspace/src/workspace/server/statement_execution.rs new file mode 100644 index 000000000..b348da8c0 --- /dev/null +++ b/crates/pgls_workspace/src/workspace/server/statement_execution.rs @@ -0,0 +1,89 @@ +use futures::TryStreamExt; +use sqlx::{ + Column, Either, Executor, PgPool, Row, Statement, ValueRef, + postgres::{PgColumn, PgRow}, +}; + +use crate::features::code_actions::{EXECUTE_STATEMENT_MAX_ROWS, ExecuteStatementResult}; + +/// Executes `sql` and collects the rows it returns, if any. +/// +/// The statement runs over the simple query protocol, so Postgres sends every value in its text +/// representation and no type needs decoding. +pub(super) async fn execute_statement( + pool: &PgPool, + sql: &str, +) -> Result { + let mut columns = None; + let mut rows = Vec::new(); + let mut rows_affected = 0; + let mut truncated = false; + + let mut results = sqlx::raw_sql(sql).fetch_many(pool); + while let Some(result) = results.try_next().await? { + match result { + Either::Left(result) => rows_affected += result.rows_affected(), + Either::Right(row) => { + if rows.len() == EXECUTE_STATEMENT_MAX_ROWS { + truncated = true; + break; + } + columns.get_or_insert_with(|| column_names(row.columns())); + rows.push(row_values(&row)); + } + } + } + drop(results); + + // Postgres sends no columns along with zero rows, so a query without rows looks like a command. + // Describing the statement tells them apart, unless it affected rows and is thus a command. + if columns.is_none() && rows_affected == 0 { + columns = pool + .prepare(sql) + .await + .ok() + .map(|statement| column_names(statement.columns())) + .filter(|columns| !columns.is_empty()); + } + + Ok(match columns { + Some(columns) => ExecuteStatementResult { + message: if truncated { + format!("Showing the first {EXECUTE_STATEMENT_MAX_ROWS} rows (truncated).") + } else if rows.len() == 1 { + "1 row".to_string() + } else { + format!("{} rows", rows.len()) + }, + columns, + rows, + truncated, + rows_affected: None, + }, + None => ExecuteStatementResult { + message: format!("Successfully executed statement. Rows affected: {rows_affected}"), + rows_affected: Some(rows_affected), + ..Default::default() + }, + }) +} + +fn column_names(columns: &[PgColumn]) -> Vec { + columns + .iter() + .map(|column| column.name().to_string()) + .collect() +} + +fn row_values(row: &PgRow) -> Vec> { + (0..row.len()) + .map(|index| { + let value = row.try_get_raw(index).ok()?; + if value.is_null() { + return None; + } + let bytes = value.as_bytes().unwrap_or_default(); + Some(String::from_utf8_lossy(bytes).into_owned()) + }) + .collect() +} diff --git a/docs/guides/configure_database.md b/docs/guides/configure_database.md index 8504104a7..2f850b943 100644 --- a/docs/guides/configure_database.md +++ b/docs/guides/configure_database.md @@ -113,6 +113,30 @@ Code actions that execute the statement under the cursor are disabled by default The patterns do not refer to schemas. To allow a single database only, name it, e.g. `"localhost/my_dev_db"`. +When no pattern matches, the code action is disabled and its reason names the setting. An invalid pattern is ignored with a warning in the log, and the reason lists it. + +### Statement Execution Results +The language server runs the statement with the LSP command `pgls.executeStatement`, shows a short summary with `window/showMessage`, and returns the result of `workspace/executeCommand`, so an editor extension can render the rows as a table: + +```json +{ + "message": "2 rows", + "columns": ["id", "name"], + "rows": [ + ["1", "Alice"], + ["2", null] + ], + "truncated": false +} +``` + +- `columns` and `rows` are filled for statements that return rows, such as `select`, `values`, `show`, `explain`, and statements with `returning`. +- Each value is a string in its Postgres text representation, as `psql` shows it, or `null` for SQL `NULL`. +- At most 100 rows are returned. `truncated` is `true` if the statement returned more. +- `rowsAffected` is set instead of `columns` and `rows` for statements that return no rows, such as `insert` without `returning` or `create table`. For example: `{ "message": "Successfully executed statement. Rows affected: 2", "columns": [], "rows": [], "truncated": false, "rowsAffected": 2 }`. +- If the statement is not executed, for example because statement execution is disabled, only `message` is meaningful: `columns` and `rows` are empty and `rowsAffected` is not set. +- If Postgres rejects the statement, the command fails with a JSON-RPC error carrying the Postgres error message. + ## Disabling Database Features If you prefer to work without a database connection, you can disable all database-related features: From 54744f4102f7a6d5da0a09ca56953ac58d7e067d Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 20 Oct 2026 07:35:57 +0000 Subject: [PATCH 2/3] fix(workspace): disable the database connection on an invalid connection string instead of panicking --- crates/pgls_cli/tests/assert_check.rs | 30 +++++ ...ck_fails_on_invalid_connection_string.snap | 19 +++ crates/pgls_lsp/src/session.rs | 15 +++ crates/pgls_lsp/tests/server.rs | 112 +++++++++++++++- crates/pgls_workspace/src/settings.rs | 120 +++++++++++++++--- crates/pgls_workspace/src/workspace/server.rs | 6 + docs/guides/configure_database.md | 2 + 7 files changed, 281 insertions(+), 23 deletions(-) create mode 100644 crates/pgls_cli/tests/snapshots/assert_check__check_fails_on_invalid_connection_string.snap diff --git a/crates/pgls_cli/tests/assert_check.rs b/crates/pgls_cli/tests/assert_check.rs index 53c48a6d3..7818863c1 100644 --- a/crates/pgls_cli/tests/assert_check.rs +++ b/crates/pgls_cli/tests/assert_check.rs @@ -270,6 +270,36 @@ fn check_fails_when_database_is_unreachable() { assert!(output.contains("database/connection"), "{output}"); } +#[cfg(target_os = "linux")] +#[test] +fn check_fails_on_invalid_connection_string() { + let output = run_check_without_db_env( + &[ + "--connection-string", + "postgres://postgres:hunter2@localhost:not-a-port/postgres", + UNKNOWN_COLUMN_PATH, + ], + &[], + ); + + assert!(!output.contains("hunter2"), "{output}"); + assert_snapshot!(output); +} + +#[cfg(target_os = "linux")] +#[test] +fn check_fails_on_invalid_database_url() { + let output = run_check_without_db_env( + &[UNKNOWN_COLUMN_PATH], + &[("DATABASE_URL", "host=localhost password=hunter2")], + ); + + assert!(output.starts_with("status: failure"), "{output}"); + assert!(output.contains("database/connection"), "{output}"); + assert!(output.contains("Invalid `db.connectionString`"), "{output}"); + assert!(!output.contains("hunter2"), "{output}"); +} + #[cfg(target_os = "linux")] #[test] fn check_without_database_succeeds_with_disable_db() { diff --git a/crates/pgls_cli/tests/snapshots/assert_check__check_fails_on_invalid_connection_string.snap b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_on_invalid_connection_string.snap new file mode 100644 index 000000000..a78684a9f --- /dev/null +++ b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_on_invalid_connection_string.snap @@ -0,0 +1,19 @@ +--- +source: crates/pgls_cli/tests/assert_check.rs +expression: output +--- +status: failure +stdout: +Checked 1 file in . No fixes applied. +Found 1 error. +stderr: +database/connection ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Invalid `db.connectionString`: invalid port number. Expected a URL like `postgres://user:password@host:5432/database`. + + i The checks that need the database were skipped. Fix the connection settings, or pass --disable-db or set db.disableConnection to check without a database. + + +check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Some errors were emitted while running checks. diff --git a/crates/pgls_lsp/src/session.rs b/crates/pgls_lsp/src/session.rs index 7807a87bb..2b367418e 100644 --- a/crates/pgls_lsp/src/session.rs +++ b/crates/pgls_lsp/src/session.rs @@ -663,6 +663,21 @@ impl Session { return ConfigurationStatus::Error; } + // An invalid connection string only disables the database features, + // so the user would not notice it otherwise. + if let Some(error) = configuration + .db + .as_ref() + .and_then(pgls_workspace::settings::connection_string_error) + { + self.client + .show_message( + MessageType::WARNING, + format!("{error} Database features are disabled."), + ) + .await; + } + let result = self.workspace.update_settings(UpdateSettingsParams { workspace_directory: self.fs.working_directory(), configuration, diff --git a/crates/pgls_lsp/tests/server.rs b/crates/pgls_lsp/tests/server.rs index ec2258ad7..2e360636d 100644 --- a/crates/pgls_lsp/tests/server.rs +++ b/crates/pgls_lsp/tests/server.rs @@ -48,8 +48,8 @@ use tower_lsp_server::ls_types::{ ClientCapabilities, DidChangeConfigurationParams, DidChangeTextDocumentParams, DidChangeWatchedFilesParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, FileChangeType, FileEvent, InitializeResult, InitializedParams, PublishDiagnosticsParams, - TextDocumentContentChangeEvent, TextDocumentIdentifier, TextDocumentItem, Uri, - VersionedTextDocumentIdentifier, + ShowMessageParams, TextDocumentContentChangeEvent, TextDocumentIdentifier, TextDocumentItem, + Uri, VersionedTextDocumentIdentifier, }; use tower_lsp_server::{jsonrpc::Request, ls_types::InitializeParams}; @@ -383,6 +383,7 @@ const CHANNEL_BUFFER_SIZE: usize = 8; #[derive(Debug, PartialEq, Eq)] enum ServerNotification { PublishDiagnostics(PublishDiagnosticsParams), + ShowMessage(ShowMessageParams), } /// Basic handler for requests and notifications coming from the server for tests @@ -408,6 +409,15 @@ where } } + if req.method() == "window/showMessage" { + let params = req.params().expect("invalid request"); + let message = from_value(params.clone()).expect("invalid params"); + match notify.send(ServerNotification::ShowMessage(message)).await { + Ok(_) => continue, + Err(_) => break, + } + } + let id = match req.id() { Some(id) => id, None => continue, @@ -580,11 +590,14 @@ async fn wait_for_diagnostics( ) -> Result { tokio::time::timeout(Duration::from_secs(5), async { loop { - let Some(ServerNotification::PublishDiagnostics(params)) = receiver.next().await else { - bail!("the server closed the connection"); - }; - if params.version == Some(version) { - return Ok(params); + match receiver.next().await { + Some(ServerNotification::PublishDiagnostics(params)) + if params.version == Some(version) => + { + return Ok(params); + } + Some(_) => {} + None => bail!("the server closed the connection"), } } }) @@ -664,6 +677,7 @@ async fn client_configuration_overrides_are_sticky_and_clearable(test_db: PgPool return true; } } + Some(ServerNotification::ShowMessage(_)) => {} None => return false, } } @@ -1223,6 +1237,90 @@ async fn test_issue_271(test_db: PgPool) -> Result<()> { Ok(()) } +/// An invalid connection string disables the database features with a warning instead of crashing +/// the server, on every configuration transition. +#[tokio::test] +async fn invalid_connection_string_warns_instead_of_crashing() -> Result<()> { + let factory = ServerFactory::default(); + let mut fs = MemoryFileSystem::default(); + fs.insert( + url!("postgres-language-server.jsonc") + .to_file_path() + .unwrap() + .into_owned(), + "{}".to_string(), + ); + + let (service, client) = factory + .create_with_fs(None, DynRef::Owned(Box::new(fs))) + .into_inner(); + + let (stream, sink) = client.split(); + let mut server = Server::new(service); + + let (sender, mut receiver) = channel(CHANNEL_BUFFER_SIZE); + let reader = tokio::spawn(client_handler(stream, sink, sender)); + + server.initialize().await?; + server.initialized().await?; + server.open_document("select 1 from;").await?; + drain_notifications(&mut receiver); + + async fn wait_for_warning(receiver: &mut Receiver) -> Option { + tokio::time::timeout(Duration::from_secs(5), async { + loop { + match receiver.next().await { + Some(ServerNotification::ShowMessage(params)) + if params.typ == lsp::MessageType::WARNING => + { + return Some(params.message); + } + Some(_) => {} + None => return None, + } + } + }) + .await + .ok() + .flatten() + } + + server + .set_configuration_overrides(json!({ + "overrides": { + "db": { "connectionString": "postgres://postgres:hunter2@localhost:not-a-port/postgres" } + } + })) + .await?; + + let warning = wait_for_warning(&mut receiver) + .await + .expect("expected a warning about the connection string"); + assert_eq!( + warning, + "Invalid `db.connectionString`: invalid port number. Expected a URL like `postgres://user:password@host:5432/database`. Database features are disabled." + ); + + // A reload with the invalid connection string warns again, and the server keeps working. + server.change_watched_configuration_file().await?; + assert!(wait_for_warning(&mut receiver).await.is_some()); + + server.replace_document(1, "select 1 from;").await?; + let diagnostics = wait_for_diagnostics(&mut receiver, 1).await?; + assert!( + diagnostics + .diagnostics + .iter() + .any(|diagnostic| diagnostic.message.contains("syntax error")), + "{diagnostics:?}" + ); + + server.shutdown().await?; + reader.abort(); + + Ok(()) +} + #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] async fn test_execute_statement(test_db: PgPool) -> Result<()> { let factory = ServerFactory::default(); diff --git a/crates/pgls_workspace/src/settings.rs b/crates/pgls_workspace/src/settings.rs index 5068ebabc..e346b815b 100644 --- a/crates/pgls_workspace/src/settings.rs +++ b/crates/pgls_workspace/src/settings.rs @@ -746,6 +746,8 @@ pub struct DatabaseSettings { pub allow_statement_executions: bool, /// The patterns of `db.allowStatementExecutionsAgainst` that are not valid globs. pub invalid_statement_execution_patterns: Vec, + /// Why `db.connectionString` is invalid. The connection is disabled then. + pub connection_string_error: Option, } impl DatabaseSettings { @@ -795,6 +797,7 @@ impl std::fmt::Debug for DatabaseSettings { "invalid_statement_execution_patterns", &self.invalid_statement_execution_patterns, ) + .field("connection_string_error", &self.connection_string_error) .finish() } } @@ -812,6 +815,7 @@ impl Default for DatabaseSettings { conn_timeout_secs: Duration::from_secs(10), allow_statement_executions: true, invalid_statement_execution_patterns: Vec::new(), + connection_string_error: None, } } } @@ -829,12 +833,7 @@ impl From for DatabaseSettings { } }); - // "host" OR "connectionString" is the minimum required setting for database features - // to be enabled. - let disable_connection = value.disable_connection.is_some_and(|disabled| disabled); - let enable_connection = - (connection_string.is_some() || value.host.as_ref().is_some()) && !disable_connection; - + let has_host = value.host.is_some(); #[cfg_attr(not(feature = "db"), allow(unused_mut))] let mut database = value.database.unwrap_or(d.database); #[cfg_attr(not(feature = "db"), allow(unused_mut))] @@ -844,21 +843,38 @@ impl From for DatabaseSettings { #[cfg_attr(not(feature = "db"), allow(unused_mut))] let mut username = value.username.unwrap_or(d.username); - // Parse connection string to extract host/port/username/database (DB mode only) + let disable_connection = value.disable_connection.is_some_and(|disabled| disabled); + + // Parse connection string to extract host/port/username/database (DB mode only). An + // invalid one disables the connection instead of failing the whole configuration. + #[cfg_attr(not(feature = "db"), allow(unused_mut))] + let mut connection_string_error: Option = None; #[cfg(feature = "db")] if let Some(uri) = connection_string.as_ref() { - let opts = PgConnectOptions::from_str(uri) - .unwrap_or_else(|err| panic!("Invalid db.connectionString provided: {err}")); - - // we only extract the values we need for statement execution checks and connection key - host = opts.get_host().to_string(); - port = opts.get_port(); - username = opts.get_username().to_string(); - if let Some(db) = opts.get_database() { - database = db.to_string(); + match parse_connection_string(uri) { + Ok(opts) => { + // we only extract the values we need for statement execution checks and connection key + host = opts.get_host().to_string(); + port = opts.get_port(); + username = opts.get_username().to_string(); + if let Some(db) = opts.get_database() { + database = db.to_string(); + } + } + Err(error) if !disable_connection => { + warn!("{error} The database connection is disabled."); + connection_string_error = Some(error); + } + Err(_) => {} } } + // "host" OR "connectionString" is the minimum required setting for database features + // to be enabled. + let enable_connection = (connection_string.is_some() || has_host) + && !disable_connection + && connection_string_error.is_none(); + // An invalid pattern is ignored rather than failing the whole configuration; the reason a // statement cannot be executed names it. let target = format!("{host}/{database}"); @@ -898,10 +914,41 @@ impl From for DatabaseSettings { allow_statement_executions, invalid_statement_execution_patterns, + connection_string_error, } } } +/// Parses `db.connectionString`. The error says why it is invalid without repeating it, as it may +/// contain a password. +#[cfg(feature = "db")] +fn parse_connection_string(uri: &str) -> Result { + PgConnectOptions::from_str(uri).map_err(|err| { + let reason = match err { + sqlx::Error::Configuration(source) => source.to_string(), + err => err.to_string(), + }; + format!( + "Invalid `db.connectionString`: {reason}. Expected a URL like `postgres://user:password@host:5432/database`." + ) + }) +} + +/// Why the `db.connectionString` of `configuration` is invalid, if it is. The database connection +/// is disabled while it is. +pub fn connection_string_error(configuration: &PartialDatabaseConfiguration) -> Option { + if configuration.disable_connection == Some(true) { + return None; + } + #[cfg(feature = "db")] + if let Some(uri) = configuration.connection_string.as_deref().map(str::trim) + && !uri.is_empty() + { + return parse_connection_string(uri).err(); + } + None +} + /// Filesystem settings for the entire workspace #[derive(Debug)] pub struct FilesSettings { @@ -1103,4 +1150,45 @@ mod tests { assert!(!config.allow_statement_executions) } + + #[cfg(feature = "db")] + #[test] + fn invalid_connection_string_disables_the_connection() { + let partial_config = PartialDatabaseConfiguration { + connection_string: Some( + "postgres://postgres:hunter2@localhost:not-a-port/test-db".to_string(), + ), + allow_statement_executions_against: Some(StringSet::from_iter(vec![String::from( + "*/*", + )])), + ..Default::default() + }; + + let error = super::connection_string_error(&partial_config); + let config = DatabaseSettings::from(partial_config); + + assert!(!config.enable_connection); + assert_eq!(config.connection_string_error, error); + assert_eq!( + error.as_deref(), + Some( + "Invalid `db.connectionString`: invalid port number. Expected a URL like `postgres://user:password@host:5432/database`." + ) + ); + } + + #[cfg(feature = "db")] + #[test] + fn disabled_connection_ignores_invalid_connection_string() { + let partial_config = PartialDatabaseConfiguration { + connection_string: Some("host=localhost".to_string()), + disable_connection: Some(true), + ..Default::default() + }; + + assert_eq!(super::connection_string_error(&partial_config), None); + let config = DatabaseSettings::from(partial_config); + assert!(!config.enable_connection); + assert_eq!(config.connection_string_error, None); + } } diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index cef3436ac..63b9d0f0a 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -1301,6 +1301,12 @@ impl Workspace for WorkspaceServer { return Ok(CheckDatabaseConnectionResult::default()); }; + if let Some(error) = &settings.db.connection_string_error { + return Ok(CheckDatabaseConnectionResult { + error: Some(error.clone()), + }); + } + let result = self .connection .try_with_pool(&settings.db, |pool| self.snapshot.load(pool).map(|_| ())); diff --git a/docs/guides/configure_database.md b/docs/guides/configure_database.md index 2f850b943..9a6bc7273 100644 --- a/docs/guides/configure_database.md +++ b/docs/guides/configure_database.md @@ -62,6 +62,8 @@ To check without a database, disable the connection with `--disable-db` or `"dis The language server keeps working without the database when the connection fails, and retries it in the background. +An invalid `connectionString` (or `DATABASE_URL`), such as one with a non-numeric port, disables the database connection instead of stopping the language server, which shows a warning. `postgres-language-server check` reports it as a `database/connection` error, like a failed connection. + ## Setting the connection from the editor An editor extension can set the connection for a running language server session with the LSP request `pgls/set_configuration_overrides`, which takes any configuration and applies it on top of the configuration file: From 36b098e500f21f862889efdb7eef2f4cd7022013 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sat, 24 Oct 2026 06:51:58 +0000 Subject: [PATCH 3/3] fix(workspace): match the port in allowStatementExecutionsAgainst The patterns were matched against host/database only, so the documented localhost/* also allowed a production database reached through a port forward, an SSH tunnel or a proxy on another local port. A pattern that names a port, such as 127.0.0.1:54322/*, is now matched against host:port/database and only allows that port. A pattern without a port keeps matching host/database and allows every port. The docs use port-specific patterns, the disabled reason suggests one, and DatabaseSettings::default() no longer allows statement executions. --- crates/pgls_configuration/src/database.rs | 3 + crates/pgls_workspace/src/settings.rs | 122 +++++++++++++++++- .../src/workspace/server.tests.rs | 2 +- docs/configuration.md | 4 +- docs/guides/configure_database.md | 14 +- docs/reference/cli.md | 9 +- docs/schema.json | 1 + .../backend-jsonrpc/src/workspace.ts | 3 + .../backend-jsonrpc/src/workspace.ts | 3 + 9 files changed, 144 insertions(+), 17 deletions(-) diff --git a/crates/pgls_configuration/src/database.rs b/crates/pgls_configuration/src/database.rs index 2e650031d..9012bc957 100644 --- a/crates/pgls_configuration/src/database.rs +++ b/crates/pgls_configuration/src/database.rs @@ -43,6 +43,9 @@ pub struct DatabaseConfiguration { #[partial(bpaf(long("database")))] pub database: String, + /// Glob patterns of the databases that code actions may execute statements against, matched + /// against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched + /// against `host/database` and allows every port. #[partial(bpaf(long("allow_statement_executions_against")))] pub allow_statement_executions_against: StringSet, diff --git a/crates/pgls_workspace/src/settings.rs b/crates/pgls_workspace/src/settings.rs index e346b815b..3fe6c6d94 100644 --- a/crates/pgls_workspace/src/settings.rs +++ b/crates/pgls_workspace/src/settings.rs @@ -758,9 +758,19 @@ impl DatabaseSettings { } let host = &self.host; + let port = self.port; let database = &self.database; + // A pattern can't name the port of an IPv6 address, see `pattern_names_port`. + let (target, pattern) = if host.contains(':') { + (format!("[{host}]:{port}/{database}"), format!("{host}/*")) + } else { + ( + format!("{host}:{port}/{database}"), + format!("{host}:{port}/*"), + ) + }; let mut reason = format!( - "Statement execution is disabled for `{host}/{database}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{host}/*\"]." + "Statement execution is disabled for `{target}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{pattern}\"]." ); if !self.invalid_statement_execution_patterns.is_empty() { let patterns = self @@ -813,7 +823,7 @@ impl Default for DatabaseSettings { password: None, database: "postgres".to_string(), conn_timeout_secs: Duration::from_secs(10), - allow_statement_executions: true, + allow_statement_executions: false, invalid_statement_execution_patterns: Vec::new(), connection_string_error: None, } @@ -876,8 +886,10 @@ impl From for DatabaseSettings { && connection_string_error.is_none(); // An invalid pattern is ignored rather than failing the whole configuration; the reason a - // statement cannot be executed names it. + // statement cannot be executed names it. A pattern that names a port only matches that + // port, any other pattern matches every port. let target = format!("{host}/{database}"); + let target_with_port = format!("{host}:{port}/{database}"); let mut allow_statement_executions = false; let mut invalid_statement_execution_patterns = Vec::new(); for pattern in value @@ -886,7 +898,14 @@ impl From for DatabaseSettings { .flat_map(|set| set.iter()) { match Glob::new(pattern) { - Ok(glob) => allow_statement_executions |= glob.compile_matcher().is_match(&target), + Ok(glob) => { + let target = if pattern_names_port(pattern) { + &target_with_port + } else { + &target + }; + allow_statement_executions |= glob.compile_matcher().is_match(target); + } Err(err) => { warn!( "Ignoring invalid pattern `{pattern}` in `db.allowStatementExecutionsAgainst`: {err}" @@ -919,6 +938,14 @@ impl From for DatabaseSettings { } } +/// Whether a pattern of `db.allowStatementExecutionsAgainst` names a port, as `localhost:54322/*` +/// does. That is the case if its host, the part before the first `/`, has exactly one `:`, so a +/// pattern for an IPv6 address like `::1/*` names no port. +fn pattern_names_port(pattern: &str) -> bool { + let host = pattern.split('/').next().unwrap_or(pattern); + host.matches(':').count() == 1 +} + /// Parses `db.connectionString`. The error says why it is invalid without repeating it, as it may /// contain a password. #[cfg(feature = "db")] @@ -1066,7 +1093,7 @@ mod tests { assert_eq!( config.statement_execution_disabled_reason().as_deref(), Some( - "Statement execution is disabled for `production/test-db`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"production/*\"]." + "Statement execution is disabled for `production:5432/test-db`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"production:5432/*\"]." ) ); } @@ -1191,4 +1218,89 @@ mod tests { assert!(!config.enable_connection); assert_eq!(config.connection_string_error, None); } + + fn allows(patterns: &[&str], host: &str, port: u16, database: &str) -> bool { + DatabaseSettings::from(PartialDatabaseConfiguration { + allow_statement_executions_against: Some(StringSet::from_iter( + patterns.iter().map(|pattern| pattern.to_string()), + )), + host: Some(host.into()), + port: Some(port), + database: Some(database.into()), + ..Default::default() + }) + .allow_statement_executions + } + + #[test] + fn patterns_with_a_port_only_match_that_port() { + assert!(allows( + &["127.0.0.1:54322/*"], + "127.0.0.1", + 54322, + "postgres" + )); + assert!(!allows( + &["127.0.0.1:54322/*"], + "127.0.0.1", + 5432, + "postgres" + )); + assert!(allows(&["localhost:*/dev"], "localhost", 6543, "dev")); + assert!(!allows(&["localhost:*/dev"], "localhost", 6543, "prod")); + assert!(allows(&["*:5432/*"], "db.internal", 5432, "app")); + } + + #[test] + fn patterns_without_a_port_match_every_port() { + assert!(allows(&["localhost/*"], "localhost", 5432, "postgres")); + assert!(allows(&["localhost/*"], "localhost", 15432, "postgres")); + assert!(allows(&["*"], "production", 5432, "postgres")); + assert!(allows(&["::1/*"], "::1", 5432, "postgres")); + } + + #[test] + fn disabled_reason_suggests_a_pattern_with_the_port() { + let config = DatabaseSettings::from(PartialDatabaseConfiguration { + host: Some("::1".into()), + port: Some(5433), + database: Some("postgres".into()), + ..Default::default() + }); + + assert_eq!( + config.statement_execution_disabled_reason().as_deref(), + Some( + "Statement execution is disabled for `[::1]:5433/postgres`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"::1/*\"]." + ) + ); + } + + #[cfg(feature = "db")] + #[test] + fn connection_string_port_is_matched() { + let allows = |connection_string: &str| { + DatabaseSettings::from(PartialDatabaseConfiguration { + connection_string: Some(connection_string.to_string()), + allow_statement_executions_against: Some(StringSet::from_iter([String::from( + "localhost:54322/*", + )])), + ..Default::default() + }) + .allow_statement_executions + }; + + assert!(allows( + "postgres://postgres:postgres@localhost:54322/postgres" + )); + // A production database reached through a tunnel on another local port. + assert!(!allows( + "postgres://postgres:postgres@localhost:15432/postgres" + )); + } + + #[test] + fn statement_executions_are_disallowed_by_default() { + assert!(!DatabaseSettings::default().allow_statement_executions); + } } diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index cadb826cb..2258d6063 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -1906,7 +1906,7 @@ async fn execute_statement_is_disabled_without_matching_pattern(test_db: PgPool) .unwrap() .to_string(); let reason = format!( - "Statement execution is disabled for `{host}/{database}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{host}/*\"]." + "Statement execution is disabled for `{host}:5432/{database}`. Add a matching pattern to `db.allowStatementExecutionsAgainst`, e.g. [\"{host}:5432/*\"]." ); assert_eq!(disabled_reason.as_deref(), Some(reason.as_str())); assert_eq!( diff --git a/docs/configuration.md b/docs/configuration.md index 6ad504e87..c38266c63 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -162,7 +162,7 @@ Some tools that the Postgres Language Server provides are implemented as mere in "password": "postgres", "database": "postgres", "connTimeoutSecs": 10, - "allowStatementExecutionsAgainst": ["127.0.0.1/*", "localhost/*"] + "allowStatementExecutionsAgainst": ["127.0.0.1:5432/*", "localhost:5432/*"] } } ``` @@ -175,7 +175,7 @@ individual fields. The URI takes precedence over any other connection fields. { "db": { "connectionString": "postgres://postgres:postgres@localhost:5432/postgres?sslmode=disable", - "allowStatementExecutionsAgainst": ["localhost/*"] + "allowStatementExecutionsAgainst": ["localhost:5432/*"] } } ``` diff --git a/docs/guides/configure_database.md b/docs/guides/configure_database.md index 9a6bc7273..781392777 100644 --- a/docs/guides/configure_database.md +++ b/docs/guides/configure_database.md @@ -30,9 +30,9 @@ Configure database connection details in your `postgres-language-server.jsonc` f "database": "your_database_name", // Connection timeout in seconds (default: 10) "connTimeoutSecs": 10, - // Glob patterns matched against `host/database`; code actions may execute + // Glob patterns matched against `host:port/database`; code actions may execute // statements only when one of them matches (default: [], never) - "allowStatementExecutionsAgainst": ["localhost/*", "127.0.0.1/*"], + "allowStatementExecutionsAgainst": ["localhost:5432/*", "127.0.0.1:5432/*"], // Completely disable database features (default: false) "disableConnection": false } @@ -102,18 +102,20 @@ GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA public TO postgres_language_server; ``` ### Statement Execution Control -Code actions that execute the statement under the cursor are disabled by default. To enable them, list glob patterns in `allowStatementExecutionsAgainst`. Each pattern is matched against `host/database` of the connection (taken from the connection string when one is set), and statements can only be executed when one of them matches: +Code actions that execute the statement under the cursor are disabled by default. To enable them, list glob patterns in `allowStatementExecutionsAgainst`. Each pattern is matched against `host:port/database` of the connection (taken from the connection string when one is set), and statements can only be executed when one of them matches: ```json { "db": { - // any database on the local machine - "allowStatementExecutionsAgainst": ["localhost/*", "127.0.0.1/*"] + // any database of the local Postgres on port 5432, and of a local Supabase project + "allowStatementExecutionsAgainst": ["127.0.0.1:5432/*", "127.0.0.1:54322/*"] } } ``` -The patterns do not refer to schemas. To allow a single database only, name it, e.g. `"localhost/my_dev_db"`. +Name the port. A port forward, an SSH tunnel, `cloud-sql-proxy` or `fly proxy` makes a remote database reachable on `localhost` too, on another port. A pattern without a port, such as `localhost/*`, is matched against `host/database` and allows every port, including those. + +The patterns do not refer to schemas. To allow a single database only, name it, e.g. `"localhost:5432/my_dev_db"`. A pattern can't name the port of an IPv6 address: `::1/*` matches every port of `::1`. When no pattern matches, the code action is disabled and its reason names the setting. An invalid pattern is ignored with a warning in the log, and the reason lists it. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 6e1cc95cb..73da341d8 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -179,7 +179,8 @@ Lints your database schema. The password to connect to the database. Can also be set via the `PGPASSWORD` environment variable. - **` --database`**=_`ARG`_ — The name of the database. Can also be set via the `PGDATABASE` environment variable. -- **` --allow_statement_executions_against`**=_`ARG`_ +- **` --allow_statement_executions_against`**=_`ARG`_ — + Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port. - **` --conn_timeout_secs`**=_`ARG`_ — The connection timeout in seconds. @@ -305,7 +306,8 @@ Runs everything to the requested files. The password to connect to the database. Can also be set via the `PGPASSWORD` environment variable. - **` --database`**=_`ARG`_ — The name of the database. Can also be set via the `PGDATABASE` environment variable. -- **` --allow_statement_executions_against`**=_`ARG`_ +- **` --allow_statement_executions_against`**=_`ARG`_ — + Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port. - **` --conn_timeout_secs`**=_`ARG`_ — The connection timeout in seconds. @@ -449,7 +451,8 @@ Formats the requested files. The password to connect to the database. Can also be set via the `PGPASSWORD` environment variable. - **` --database`**=_`ARG`_ — The name of the database. Can also be set via the `PGDATABASE` environment variable. -- **` --allow_statement_executions_against`**=_`ARG`_ +- **` --allow_statement_executions_against`**=_`ARG`_ — + Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port. - **` --conn_timeout_secs`**=_`ARG`_ — The connection timeout in seconds. diff --git a/docs/schema.json b/docs/schema.json index 68ca485ad..1eb1094d0 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -372,6 +372,7 @@ "type": "object", "properties": { "allowStatementExecutionsAgainst": { + "description": "Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port.", "anyOf": [ { "$ref": "#/definitions/StringSet" diff --git a/packages/@postgres-language-server/backend-jsonrpc/src/workspace.ts b/packages/@postgres-language-server/backend-jsonrpc/src/workspace.ts index 90572e043..2a2dfa906 100644 --- a/packages/@postgres-language-server/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgres-language-server/backend-jsonrpc/src/workspace.ts @@ -392,6 +392,9 @@ export interface PartialConfiguration { * The configuration of the database connection. */ export interface PartialDatabaseConfiguration { + /** + * Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port. + */ allowStatementExecutionsAgainst?: StringSet; /** * The connection timeout in seconds. diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index 90572e043..2a2dfa906 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -392,6 +392,9 @@ export interface PartialConfiguration { * The configuration of the database connection. */ export interface PartialDatabaseConfiguration { + /** + * Glob patterns of the databases that code actions may execute statements against, matched against `host:port/database`. A pattern without a port, e.g. `localhost/*`, is matched against `host/database` and allows every port. + */ allowStatementExecutionsAgainst?: StringSet; /** * The connection timeout in seconds.