diff --git a/crates/pgls_cli/src/commands/check.rs b/crates/pgls_cli/src/commands/check.rs index 0a2c81c7a..1caaa7b6c 100644 --- a/crates/pgls_cli/src/commands/check.rs +++ b/crates/pgls_cli/src/commands/check.rs @@ -73,14 +73,14 @@ pub fn check( let execution = ExecutionConfig::new(mode, max_diagnostics) .with_error_on_warnings(cli_options.error_on_warnings); - let database_error = check_database_connection(&session)?; + let database_errors = check_database_connection(&session)?; if let Some(stdin_path) = args.stdin_file_path.as_deref() { - let database_unavailable = database_error.is_some(); + let database_unavailable = !database_errors.is_empty(); print_diagnostics( session.console(), configuration_errors .into_iter() - .chain(database_error) + .chain(database_errors) .collect(), ); let payload = read_stdin_payload(stdin_path, session.console(), "check")?; @@ -91,7 +91,7 @@ pub fn check( Ok(()) } else { let mut report: Report = run_files(&mut session, &execution, paths)?; - if let Some(error) = database_error { + for error in database_errors.into_iter().rev() { report.prepend_diagnostic(error); } for error in configuration_errors.into_iter().rev() { @@ -106,15 +106,17 @@ pub fn check( /// A configured database that cannot be reached is an error: the checks that need it would /// otherwise be skipped silently. -fn check_database_connection(session: &CliSession) -> Result, CliDiagnostic> { +fn check_database_connection(session: &CliSession) -> Result, CliDiagnostic> { let result = session .workspace() .check_database_connection(CheckDatabaseConnectionParams { project_key: session.project_key(), })?; Ok(result - .error - .map(|message| Error::from(DatabaseUnavailable::new(message)))) + .errors + .into_iter() + .map(|message| Error::from(DatabaseUnavailable::new(message))) + .collect()) } fn resolve_paths( diff --git a/crates/pgls_cli/tests/assert_check.rs b/crates/pgls_cli/tests/assert_check.rs index af3592ac7..34b80c819 100644 --- a/crates/pgls_cli/tests/assert_check.rs +++ b/crates/pgls_cli/tests/assert_check.rs @@ -742,6 +742,49 @@ fn relative_paths_are_relative_to_the_configuration_file() { ); } +/// `check` verifies the databases of all configuration files at the same time, and reports each +/// one it can't reach with its address and the folder that uses it. +#[cfg(target_os = "linux")] +#[test] +fn check_reports_every_unreachable_database_at_once() { + let unreachable = |port: u16| { + format!( + r#"{{ "db": {{ "host": "10.255.255.1", "port": {port}, "username": "postgres", "password": "postgres", "connTimeoutSecs": 2 }} }}"# + ) + }; + let (one, two, three) = (unreachable(5001), unreachable(5002), unreachable(5003)); + let root = write_tree(&[ + ( + "postgres-language-server.jsonc", + r#"{ "db": { "disableConnection": true } }"#, + ), + ("db1/postgres-language-server.jsonc", &one), + ("db2/postgres-language-server.jsonc", &two), + ("db3/postgres-language-server.jsonc", &three), + ("db1/a.sql", "select 1;\n"), + ("db2/a.sql", "select 1;\n"), + ("db3/a.sql", "select 1;\n"), + ]); + + let start = std::time::Instant::now(); + let (success, output) = check_in(root.path(), &[], &[]); + let elapsed = start.elapsed(); + assert!(!success, "{output}"); + for (port, folder) in [(5001, "db1"), (5002, "db2"), (5003, "db3")] { + let address = format!("postgres@10.255.255.1:{port}/postgres, the database of "); + assert!( + output.lines().any(|line| line.contains(&address) + && line.contains(&root.path().join(folder).display().to_string())), + "the database of {folder} is not reported:\n{output}" + ); + } + // One connection timeout for the check, not one per database. + assert!( + elapsed < std::time::Duration::from_secs(5), + "the connection checks ran one after the other: {elapsed:?}" + ); +} + /// A relative path in the arguments is relative to the working directory, like any other path /// typed on the command line, not to the folder of the configuration file. #[test] @@ -944,7 +987,7 @@ fn normalize_durations(input: &str) -> String { } fn normalize_output(status: ExitStatus, stdout: &str, stderr: &str) -> String { - let normalized_stdout = normalize_durations(stdout); + let normalized_stdout = normalize_test_environment(&normalize_durations(stdout)); let normalized_stderr = normalize_diagnostics(stderr); let status_label = if status.success() { "success" @@ -958,8 +1001,28 @@ fn normalize_output(status: ExitStatus, stdout: &str, stderr: &str) -> String { ) } +/// Replaces the address of the database in connection errors, which names the database that +/// `sqlx::test` creates, and the working directory. +fn normalize_test_environment(input: &str) -> String { + const PREFIX: &str = "Could not connect to "; + let mut output = String::new(); + let mut rest = input; + while let Some(start) = rest.find(PREFIX) { + output.push_str(&rest[..start + PREFIX.len()]); + output.push_str(""); + rest = &rest[start + PREFIX.len()..]; + let end = rest.find([',', ' ']).unwrap_or(rest.len()); + rest = &rest[end..]; + } + output.push_str(rest); + match std::env::current_dir() { + Ok(cwd) => output.replace(&cwd.display().to_string(), ""), + Err(_) => output, + } +} + fn normalize_diagnostics(input: &str) -> String { - let normalized = normalize_durations(input); + let normalized = normalize_test_environment(&normalize_durations(input)); let mut lines = normalized.lines().peekable(); let mut diagnostic_sections: Vec = Vec::new(); let mut other_lines: Vec = Vec::new(); diff --git a/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap index 564d59b16..cc95b7358 100644 --- a/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap +++ b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap @@ -9,7 +9,7 @@ Found 1 error. stderr: database/connection ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Database error: password authentication failed for user "postgres" + × Could not connect to , the database of : Database error: password authentication failed for user "postgres" 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. diff --git a/crates/pgls_lsp/src/session.rs b/crates/pgls_lsp/src/session.rs index d3b23f92f..438a8a7a7 100644 --- a/crates/pgls_lsp/src/session.rs +++ b/crates/pgls_lsp/src/session.rs @@ -594,7 +594,7 @@ impl Session { project_key: self.project_key(&pgls_path), }, ); - if connection.is_ok_and(|connection| connection.error.is_none()) + if connection.is_ok_and(|connection| connection.errors.is_empty()) && self .editor_features_without_database .swap(false, Ordering::Relaxed) diff --git a/crates/pgls_workspace/src/features/diagnostics.rs b/crates/pgls_workspace/src/features/diagnostics.rs index 3b7cdc299..e8aaf1fc1 100644 --- a/crates/pgls_workspace/src/features/diagnostics.rs +++ b/crates/pgls_workspace/src/features/diagnostics.rs @@ -31,8 +31,8 @@ pub struct CheckDatabaseConnectionParams { #[derive(Debug, Default, serde::Serialize, serde::Deserialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct CheckDatabaseConnectionResult { - /// Why the configured database cannot be used, if it cannot. - pub error: Option, + /// Why the configured databases cannot be used, one message per database that can't. + pub errors: Vec, } #[derive(Debug, serde::Serialize, serde::Deserialize)] diff --git a/crates/pgls_workspace/src/settings.rs b/crates/pgls_workspace/src/settings.rs index 734295247..db3ee67a2 100644 --- a/crates/pgls_workspace/src/settings.rs +++ b/crates/pgls_workspace/src/settings.rs @@ -133,11 +133,15 @@ impl WorkspaceSettings { self.data.get(key).map(|project| project.settings_for(path)) } - /// The root settings and the nested settings of a project. - pub fn get_all_settings(&self, key: ProjectKey) -> impl Iterator { + /// The root settings and the nested settings of a project, with the folders they apply to. + pub fn get_all_settings(&self, key: ProjectKey) -> impl Iterator { self.data.get(key).into_iter().flat_map(|project| { - std::iter::once(&project.settings) - .chain(project.nested.iter().map(|(_, settings)| settings)) + std::iter::once((project.path.as_path(), &project.settings)).chain( + project + .nested + .iter() + .map(|(folder, settings)| (folder.as_path(), settings)), + ) }) } @@ -769,6 +773,7 @@ impl Default for TypecheckSettings { pub const DEFAULT_DATABASE_PASSWORD: &str = "postgres"; /// Database settings for the entire workspace +#[derive(Clone)] pub struct DatabaseSettings { pub enable_connection: bool, pub connection_string: Option, diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index 0e401366f..4ca151863 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -86,6 +86,10 @@ mod async_helper; #[cfg(feature = "db")] mod connection_key; #[cfg(feature = "db")] +use crate::settings::DatabaseSettings; +#[cfg(feature = "db")] +use connection_key::ConnectionKey; +#[cfg(feature = "db")] mod connection_manager; pub(crate) mod document; mod expand_star; @@ -1799,29 +1803,78 @@ impl Workspace for WorkspaceServer { &self, params: CheckDatabaseConnectionParams, ) -> Result { - // Every configuration file of the project can have its own database, so check all of them. - let settings = self.workspaces(); - for settings in settings.as_ref().get_all_settings(params.project_key) { + // Every configuration file of the project can have its own database, so check each of + // them once, with the folders that use it. + let mut connections: Vec<(DatabaseSettings, Vec)> = Vec::new(); + let mut errors = Vec::new(); + for (folder, settings) in self + .workspaces() + .as_ref() + .get_all_settings(params.project_key) + { + // An invalid connection string disables the connection, so say why. if let Some(error) = &settings.db.connection_string_error { - return Ok(CheckDatabaseConnectionResult { - error: Some(error.clone()), - }); + if !errors.contains(error) { + errors.push(error.clone()); + } + continue; + } + if !settings.db.enable_connection { + continue; + } + let existing = connections.iter_mut().find(|(db, _)| { + ConnectionKey::from(db) == ConnectionKey::from(&settings.db) + && db.connection_string == settings.db.connection_string + && db.password == settings.db.password + && db.conn_timeout_secs == settings.db.conn_timeout_secs + }); + match existing { + Some((_, folders)) => folders.push(folder.to_path_buf()), + None => connections.push((settings.db.clone(), vec![folder.to_path_buf()])), } + } + + // Concurrently, so that unreachable databases time out together rather than one after + // the other. + let results: Vec<_> = std::thread::scope(|scope| { + let checks: Vec<_> = connections + .iter() + .map(|(db, _)| { + scope.spawn(|| { + self.connection + .try_with_pool(db, |pool| self.snapshot.load(pool).map(|_| ())) + }) + }) + .collect(); + checks + .into_iter() + .map(|check| check.join().expect("a connection check panicked")) + .collect() + }); - let result = self - .connection - .try_with_pool(&settings.db, |pool| self.snapshot.load(pool).map(|_| ())); + for ((db, folders), result) in connections.iter().zip(results) { match result { None | Some(Ok(())) => {} Some(Err(err @ WorkspaceError::DatabaseConnectionError(_))) => { - return Ok(CheckDatabaseConnectionResult { - error: Some(err.to_string()), - }); + let folders: Vec<_> = folders + .iter() + .filter(|folder| !folder.as_os_str().is_empty()) + .map(|folder| folder.display().to_string()) + .collect(); + let used_by = if folders.is_empty() { + String::new() + } else { + format!(", the database of {}", folders.join(", ")) + }; + errors.push(format!( + "Could not connect to {}@{}:{}/{}{used_by}: {err}", + db.username, db.host, db.port, db.database + )); } Some(Err(err)) => return Err(err), } } - Ok(CheckDatabaseConnectionResult::default()) + Ok(CheckDatabaseConnectionResult { errors }) } #[cfg(not(feature = "db"))] diff --git a/crates/pgls_workspace/src/workspace/server/snapshot_manager.rs b/crates/pgls_workspace/src/workspace/server/snapshot_manager.rs index 37727cfff..5345534f3 100644 --- a/crates/pgls_workspace/src/workspace/server/snapshot_manager.rs +++ b/crates/pgls_workspace/src/workspace/server/snapshot_manager.rs @@ -1,5 +1,8 @@ use std::sync::{Arc, RwLock}; +#[cfg(feature = "db")] +use std::sync::Mutex; + use pgls_catalog::Snapshot; use crate::WorkspaceError; @@ -12,6 +15,10 @@ use std::collections::HashMap; #[cfg(feature = "db")] use super::{async_helper::run_async, connection_key::ConnectionKey}; +/// The snapshot of a connection, once it loaded. +#[cfg(feature = "db")] +type SnapshotSlot = Arc>>>; + /// Manages database snapshot storage and retrieval. /// /// In db mode: supports loading from database connections and/or JSON. @@ -26,9 +33,10 @@ use super::{async_helper::run_async, connection_key::ConnectionKey}; /// - `load()` - Load schema from database connection /// - `clear_all_connections()` - Clear the schemas of all connections pub struct SnapshotManager { - /// Connection-based snapshots (db mode only) + /// Connection-based snapshots (db mode only). Each connection has its own lock, held while + /// its snapshot loads, so that a slow or unreachable database doesn't block the others. #[cfg(feature = "db")] - db_schemas: RwLock>>, + db_schemas: RwLock>, /// JSON-loaded schema (available in both modes) schema: RwLock>>, @@ -77,29 +85,26 @@ impl SnapshotManager { pub fn load(&self, pool: &PgPool) -> Result, WorkspaceError> { let key: ConnectionKey = pool.into(); - // Try read lock first for cache hit - if let Ok(schemas) = self.db_schemas.read() - && let Some(cache) = schemas.get(&key) - { - return Ok(Arc::clone(cache)); - } - - // Cache miss - need write lock to insert - let mut schemas = self.db_schemas.write().unwrap(); + // The map is only locked to find the connection's slot, never during network I/O. + let existing = self.db_schemas.read().unwrap().get(&key).cloned(); + let slot = match existing { + Some(slot) => slot, + None => Arc::clone(self.db_schemas.write().unwrap().entry(key).or_default()), + }; - // Double-check after acquiring write lock - if let Some(cache) = schemas.get(&key) { + // Callers for the same connection wait for one load instead of starting their own. + let mut snapshot = slot.lock().unwrap(); + if let Some(cache) = snapshot.as_ref() { return Ok(Arc::clone(cache)); } - // Load the snapshot from the database + // Load the snapshot from the database. A failure leaves the slot empty for a retry. let pool_clone = pool.clone(); - let snapshot = Arc::new(run_async( + let loaded = Arc::new(run_async( async move { Snapshot::load(&pool_clone).await }, )??); - - schemas.insert(key, snapshot.clone()); - Ok(snapshot) + *snapshot = Some(Arc::clone(&loaded)); + Ok(loaded) } /// Clear all connection-based snapshots.