fix(cli): fail check on database login errors and read ~/.pgpass

crates/pgls_cli/src/commands/check.rs

@@ -1,14 +1,16 @@
 use crate::cli_options::CliOptions;
 use crate::commands::get_files_to_process_with_cli_options;
+use crate::diagnostics::DatabaseUnavailable;
 use crate::execute::{StdinPayload, run_files, run_stdin};
 use crate::reporter::Report;
 use crate::{CliDiagnostic, CliSession, VcsIntegration};
 use crate::{ExecutionConfig, ExecutionMode, VcsTargeting};
 use pgls_configuration::PartialConfiguration;
 use pgls_console::Console;
-use pgls_diagnostics::category;
+use pgls_diagnostics::{Error, category};
 use pgls_fs::FileSystem;
 use pgls_workspace::DynRef;
+use pgls_workspace::features::diagnostics::CheckDatabaseConnectionParams;
 use std::ffi::OsString;
 
 pub struct CheckArgs {
@@ -50,14 +52,29 @@ pub fn check(
         let payload = read_stdin_payload(stdin_path, session.console())?;
         run_stdin(&mut session, &execution, payload)
     } else {
-        let report: Report = run_files(&mut session, &execution, paths)?;
+        let database_error = check_database_connection(&session)?;
+        let mut report: Report = run_files(&mut session, &execution, paths)?;
+        if let Some(error) = database_error {
+            report.prepend_diagnostic(error);
+        }
 
         let exit_result = enforce_exit_codes(cli_options, &report);
         session.report("check", cli_options, &report)?;
         exit_result
     }
 }
 
+/// 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<Option<Error>, CliDiagnostic> {
+    let result = session
+        .workspace()
+        .check_database_connection(CheckDatabaseConnectionParams::default())?;
+    Ok(result
+        .error
+        .map(|message| Error::from(DatabaseUnavailable::new(message))))
+}
+
 fn resolve_paths(
     fs: &DynRef<'_, dyn FileSystem>,
     configuration: &PartialConfiguration,

crates/pgls_cli/src/diagnostics.rs

@@ -163,6 +163,41 @@ pub struct CheckError {
     message: MessageAndDescription,
 }
 
+/// The configured database could not be used, so the checks that need it did not run.
+#[derive(Debug, Diagnostic)]
+#[diagnostic(category = "database/connection", severity = Error)]
+pub struct DatabaseUnavailable {
+    #[message]
+    #[description]
+    message: MessageAndDescription,
+
+    #[advice]
+    advice: DatabaseUnavailableAdvice,
+}
+
+impl DatabaseUnavailable {
+    pub fn new(message: impl Into<String>) -> Self {
+        Self {
+            message: MessageAndDescription::from(message.into()),
+            advice: DatabaseUnavailableAdvice,
+        }
+    }
+}
+
+#[derive(Debug)]
+struct DatabaseUnavailableAdvice;
+
+impl Advices for DatabaseUnavailableAdvice {
+    fn record(&self, visitor: &mut dyn Visit) -> std::io::Result<()> {
+        visitor.record_log(
+            LogCategory::Info,
+            &markup! {
+                "The checks that need the database were skipped. Fix the connection settings, or pass "<Emphasis>"--disable-db"</Emphasis>" or set "<Emphasis>"db.disableConnection"</Emphasis>" to check without a database."
+            },
+        )
+    }
+}
+
 #[derive(Debug, Diagnostic)]
 #[diagnostic(
     severity = Error,

crates/pgls_cli/src/reporter/mod.rs

@@ -99,6 +99,14 @@ impl Report {
             traversal,
         }
     }
+
+    /// Adds a diagnostic that does not belong to a single file, ahead of the file diagnostics.
+    pub fn prepend_diagnostic(&mut self, diagnostic: Error) {
+        let (errors, warnings) = count_levels(std::slice::from_ref(&diagnostic));
+        self.errors += errors;
+        self.warnings += warnings;
+        self.diagnostics.insert(0, diagnostic);
+    }
 }
 
 pub trait ReportWriter {

crates/pgls_cli/tests/assert_check.rs

@@ -222,6 +222,187 @@ async fn check_accepts_tls_connection_strings(test_db: PgPool) {
     std::fs::remove_file(sql_file).expect("failed to remove temporary SQL file");
 }
 
+#[cfg(target_os = "linux")]
+const UNKNOWN_COLUMN_PATH: &str = "tests/fixtures/unknown_column.sql";
+
+#[cfg(target_os = "linux")]
+#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")]
+async fn check_fails_when_database_login_fails(test_db: PgPool) {
+    let opts = test_db.connect_options();
+    let port = opts.get_port().to_string();
+
+    assert_snapshot!(run_check_without_db_env(
+        &[
+            "--host",
+            opts.get_host(),
+            "--port",
+            &port,
+            "--username",
+            "postgres",
+            "--password",
+            "wrong-password",
+            "--database",
+            opts.get_database().unwrap_or("postgres"),
+            UNKNOWN_COLUMN_PATH,
+        ],
+        &[],
+    ));
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn check_fails_when_database_is_unreachable() {
+    // Nothing listens on port 1.
+    let output = run_check_without_db_env(
+        &[
+            "--host",
+            "127.0.0.1",
+            "--port",
+            "1",
+            "--conn_timeout_secs",
+            "1",
+            UNKNOWN_COLUMN_PATH,
+        ],
+        &[],
+    );
+
+    assert!(output.starts_with("status: failure"), "{output}");
+    assert!(output.contains("database/connection"), "{output}");
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn check_without_database_succeeds_with_disable_db() {
+    let output = run_check_without_db_env(
+        &[
+            "--host",
+            "127.0.0.1",
+            "--port",
+            "1",
+            "--conn_timeout_secs",
+            "1",
+            "--disable-db",
+            UNKNOWN_COLUMN_PATH,
+        ],
+        &[],
+    );
+
+    assert!(output.starts_with("status: success"), "{output}");
+    assert!(!output.contains("database/connection"), "{output}");
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn check_without_database_succeeds_with_disable_connection_in_config() {
+    let mut cmd = cargo_bin_cmd!("postgres-language-server");
+    let output = cmd
+        .env_remove("DATABASE_URL")
+        .env_remove("PGHOST")
+        .args([
+            "check",
+            "--config-path",
+            "tests/fixtures/disable_connection/postgres-language-server.jsonc",
+            "--log-level",
+            "none",
+            UNKNOWN_COLUMN_PATH,
+        ])
+        .output()
+        .expect("failed to run CLI");
+    let output = normalize_output(
+        output.status,
+        &String::from_utf8_lossy(&output.stdout),
+        &String::from_utf8_lossy(&output.stderr),
+    );
+
+    assert!(output.starts_with("status: success"), "{output}");
+    assert!(!output.contains("database/connection"), "{output}");
+}
+
+#[cfg(target_os = "linux")]
+#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")]
+async fn check_reads_the_password_from_pgpassfile(test_db: PgPool) {
+    use std::os::unix::fs::PermissionsExt;
+
+    let opts = test_db.connect_options();
+    let host = opts.get_host().to_string();
+    let port = opts.get_port().to_string();
+    let database = opts.get_database().unwrap_or("postgres").to_string();
+    let role = format!("pgls_pgpass_{}", std::process::id());
+    sqlx::query(&format!("drop role if exists {role}"))
+        .execute(&test_db)
+        .await
+        .unwrap();
+    sqlx::query(&format!(
+        "create role {role} login password 'pgpass:secret'"
+    ))
+    .execute(&test_db)
+    .await
+    .unwrap();
+
+    let passfile = std::env::temp_dir().join(format!("{role}.pgpass"));
+    std::fs::write(
+        &passfile,
+        format!("# comment\n{host}:{port}:{database}:{role}:pgpass\\:secret\n"),
+    )
+    .unwrap();
+    std::fs::set_permissions(&passfile, std::fs::Permissions::from_mode(0o600)).unwrap();
+
+    let output = run_check_without_db_env(
+        &[
+            "--host",
+            &host,
+            "--port",
+            &port,
+            "--username",
+            &role,
+            "--database",
+            &database,
+            UNKNOWN_COLUMN_PATH,
+        ],
+        &[("PGPASSFILE", passfile.to_str().unwrap())],
+    );
+
+    std::fs::remove_file(&passfile).unwrap();
+    sqlx::query(&format!("drop role {role}"))
+        .execute(&test_db)
+        .await
+        .unwrap();
+
+    // The column check needs the database schema, so it only runs after a successful login.
+    assert!(!output.contains("database/connection"), "{output}");
+    assert!(output.contains("lint/unknownColumn"), "{output}");
+}
+
+/// Runs `check` with the database settings given in `args` only, without the connection
+/// settings of the environment running the tests.
+#[cfg(target_os = "linux")]
+fn run_check_without_db_env(args: &[&str], envs: &[(&str, &str)]) -> String {
+    let mut cmd = cargo_bin_cmd!("postgres-language-server");
+    for var in [
+        "DATABASE_URL",
+        "PGHOST",
+        "PGPORT",
+        "PGUSER",
+        "PGPASSWORD",
+        "PGDATABASE",
+    ] {
+        cmd.env_remove(var);
+    }
+    // Keep the passfile of the user running the tests out of it.
+    cmd.env("PGPASSFILE", "/nonexistent/pgpass");
+    cmd.envs(envs.iter().copied());
+
+    let mut full_args = vec!["check", "--config-path", CONFIG_PATH, "--log-level", "none"];
+    full_args.extend_from_slice(args);
+    let output = cmd.args(full_args).output().expect("failed to run CLI");
+
+    normalize_output(
+        output.status,
+        &String::from_utf8_lossy(&output.stdout),
+        &String::from_utf8_lossy(&output.stderr),
+    )
+}
+
 fn run_check(args: &[&str]) -> String {
     let mut full_args = vec!["--config-path", CONFIG_PATH, "--log-level", "none"];
     full_args.extend_from_slice(args);

crates/pgls_cli/tests/fixtures/disable_connection/postgres-language-server.jsonc

@@ -0,0 +1,10 @@
+{
+  "$schema": "https://pg-language-server.com/schema/postgres-language-server.schema.json",
+  "db": {
+    // Nothing listens on port 1.
+    "host": "127.0.0.1",
+    "port": 1,
+    "connTimeoutSecs": 1,
+    "disableConnection": true
+  }
+}

crates/pgls_cli/tests/fixtures/unknown_column.sql

@@ -0,0 +1 @@
+select no_such_column from pg_catalog.pg_class;

crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap

@@ -0,0 +1,19 @@
+---
+source: crates/pgls_cli/tests/assert_check.rs
+expression: "run_check_without_db_env(&[\"--host\", opts.get_host(), \"--port\", &port,\n\"--username\", \"postgres\", \"--password\", \"wrong-password\", \"--database\",\nopts.get_database().unwrap_or(\"postgres\"), UNKNOWN_COLUMN_PATH,], &[],)"
+---
+status: failure
+stdout:
+Checked 1 file in <duration>. No fixes applied.
+Found 1 error.
+stderr:
+database/connection ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × 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.
+
+
+check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × Some errors were emitted while running checks.

crates/pgls_configuration/src/database.rs

@@ -51,7 +51,7 @@ pub struct DatabaseConfiguration {
     pub conn_timeout_secs: u16,
 
     /// Actively disable all database-related features.
-    #[partial(bpaf(long("disable-db"), switch, fallback(Some(false))))]
+    #[partial(bpaf(long("disable-db"), req_flag(true)))]
     #[partial(cfg_attr(feature = "schema", schemars(skip)))]
     pub disable_connection: bool,
 }

crates/pgls_env/src/lib.rs

@@ -41,6 +41,7 @@ pub struct PgLSEnv {
     pub pgport: PgLSEnvVariable,
     pub pguser: PgLSEnvVariable,
     pub pgpassword: PgLSEnvVariable,
+    pub pgpassfile: PgLSEnvVariable,
     pub pgdatabase: PgLSEnvVariable,
 
     // DEPRECATED - kept for backward compatibility
@@ -83,6 +84,10 @@ impl PgLSEnv {
                 "PGPASSWORD",
                 "The password to connect to the database.",
             ),
+            pgpassfile: PgLSEnvVariable::new(
+                "PGPASSFILE",
+                "The password file to look up the password in when none is configured. Default: `~/.pgpass`.",
+            ),
             pgdatabase: PgLSEnvVariable::new(
                 "PGDATABASE",
                 "The name of the database to connect to.",
@@ -181,7 +186,13 @@ impl Display for PgLSEnv {
         };
 
         let sensitive = [&self.database_url, &self.pgpassword];
-        let non_sensitive = [&self.pghost, &self.pgport, &self.pguser, &self.pgdatabase];
+        let non_sensitive = [
+            &self.pghost,
+            &self.pgport,
+            &self.pguser,
+            &self.pgpassfile,
+            &self.pgdatabase,
+        ];
 
         for var in sensitive {
             match var.value() {

crates/pgls_lsp/src/server.rs

@@ -507,6 +507,7 @@ impl ServerFactory {
         workspace_method!(builder, change_file);
         workspace_method!(builder, close_file);
         workspace_method!(builder, pull_file_diagnostics);
+        workspace_method!(builder, check_database_connection);
         workspace_method!(builder, get_completions);
         workspace_method!(builder, register_project_folder);
         workspace_method!(builder, unregister_project_folder);