From 985e08b0f70c30cef448ad83574bd3f9c528c22e Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 26 Aug 2026 15:23:34 +0100 Subject: [PATCH 1/2] Validate local client selectors --- README.md | 8 + crates/clickhousectl/src/local/cli.rs | 231 +++++++++++++++++- .../tests/local_client_selectors_test.rs | 186 ++++++++++++++ 3 files changed, 419 insertions(+), 6 deletions(-) create mode 100644 crates/clickhousectl/tests/local_client_selectors_test.rs diff --git a/README.md b/README.md index dece1393..a356f72e 100644 --- a/README.md +++ b/README.md @@ -224,8 +224,12 @@ clickhousectl local client --name dev # Connects to "dev" server clickhousectl local client --query "SHOW DATABASES" # Run a query clickhousectl local client --queries-file schema.sql # Run queries from a file clickhousectl local client --host remote-host --port 9000 # Connect to a specific host/port +clickhousectl local client --host remote-host # Direct mode; port defaults to 9000 +clickhousectl local client --port 19000 # Direct mode; host defaults to localhost ``` +`--name` selects a managed server and cannot be combined with direct `--host` or `--port` selectors. + ### Creating and managing ClickHouse servers Start and manage ClickHouse server instances. Each server gets its own isolated data directory at `.clickhouse/servers//data/`. @@ -323,6 +327,8 @@ clickhousectl local server list # Connect with psql (uses host psql if installed; otherwise falls back to docker exec) clickhousectl local postgres client --name dev clickhousectl local postgres client --name dev --query "SELECT 1" +clickhousectl local postgres client --host remote-host # Direct mode; port defaults to 5432 +clickhousectl local postgres client --port 55432 # Direct mode; connects locally # Write POSTGRES_HOST/PORT/USER/PASSWORD/DATABASE into .env.local clickhousectl local postgres dotenv --name dev --local @@ -336,6 +342,8 @@ clickhousectl local postgres remove # Remove "default" clickhousectl local postgres remove dev ``` +Postgres `--name` and `--version` select a managed instance and cannot be combined with direct `--host` or `--port` selectors. + The Postgres `dotenv` command includes the generated password. Do not commit its output; prefer `--local` when your application reads `.env.local`. `--env` accepts each valid `KEY=VALUE` key once. `POSTGRES_USER`, `POSTGRES_DB`, and `PGDATA` are generated by clickhousectl and cannot be supplied through `--env`; use `--user` or `--database` for the first two. For compatibility, `-e POSTGRES_PASSWORD=...` remains an alternative to `--password`, but combining the two or repeating `POSTGRES_PASSWORD` is an error. This guarantees that every generated variable appears exactly once in the container environment. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index ec04a857..854de661 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -214,13 +214,14 @@ CONTEXT FOR AGENTS: 1. Named server: `clickhousectl local client --name dev` — looks up port and version from a locally managed server started via `clickhousectl local server start`. Defaults to \"default\". 2. Explicit host/port: `clickhousectl local client --host myhost --port 9000` — connects to any - ClickHouse server directly, bypassing local server lookup. + ClickHouse server directly, bypassing local server lookup. Host-only uses port 9000; port-only + connects to localhost. Direct selectors cannot be combined with --name. --query and --queries-file execute SQL inline or from a file. Additional clickhouse-client args can be passed after --. Related: `clickhousectl local server start` to start a local server, `clickhousectl local server list` to see servers.")] Client { /// Server name to connect to (default: "default") - #[arg(long, short)] + #[arg(long, short, conflicts_with_all = ["host", "port"])] name: Option, /// Host to connect to (bypasses local server lookup) @@ -228,7 +229,11 @@ CONTEXT FOR AGENTS: host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short)] + #[arg( + long, + short, + value_parser = clap::value_parser!(u16).range(1..=65535) + )] port: Option, /// Execute a SQL query @@ -547,17 +552,19 @@ CONTEXT FOR AGENTS: and credentials from a locally managed Postgres started via `local postgres start`. Defaults to \"default\". 2. Explicit host/port: `clickhousectl local postgres client --host myhost --port 5432`. + Host-only uses port 5432; port-only connects to the local machine. Direct selectors cannot be + combined with --name or --version. If `psql` is on PATH on the host, it is execed directly. Otherwise, falls back to running `psql` inside the container via Docker exec (no host psql required). --query and --queries-file pass through to psql (-c / -f). Additional psql args can be passed after --.")] Client { /// Server name to connect to (default: "default") - #[arg(long, short)] + #[arg(long, short, conflicts_with_all = ["host", "port"])] name: Option, /// Postgres version to disambiguate when multiple share a name - #[arg(long, short = 'v')] + #[arg(long, short = 'v', conflicts_with_all = ["host", "port"])] version: Option, /// Host to connect to (bypasses local server lookup) @@ -565,7 +572,11 @@ CONTEXT FOR AGENTS: host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short)] + #[arg( + long, + short, + value_parser = clap::value_parser!(u16).range(1..=65535) + )] port: Option, /// Execute a single SQL query @@ -637,6 +648,14 @@ mod tests { .expect("invalid postgres start arguments should fail during clap parsing") } + fn local_parse_error(args: &[&str]) -> clap::Error { + let mut argv = vec!["clickhousectl", "local"]; + argv.extend_from_slice(args); + Cli::try_parse_from(argv) + .err() + .expect("invalid local arguments should fail during clap parsing") + } + #[test] fn parses_supported_clickhouse_version_forms_for_each_command() { for (input, expected) in [ @@ -725,6 +744,206 @@ mod tests { ); } + #[test] + fn clickhouse_client_parses_every_valid_selector_combination_and_order() { + type SelectorCase = ( + &'static [&'static str], + Option<&'static str>, + Option<&'static str>, + Option, + ); + let cases: &[SelectorCase] = &[ + (&[], None, None, None), + (&["--name", "dev"], Some("dev"), None, None), + (&["--host", "db.example"], None, Some("db.example"), None), + (&["--port", "1"], None, None, Some(1)), + ( + &["--host", "db.example", "--port", "65535"], + None, + Some("db.example"), + Some(65535), + ), + ( + &["--port", "65535", "--host", "db.example"], + None, + Some("db.example"), + Some(65535), + ), + ]; + + for (selectors, expected_name, expected_host, expected_port) in cases { + let args: Vec<&str> = ["client"] + .into_iter() + .chain(selectors.iter().copied()) + .collect(); + let LocalCommands::Client { + name, host, port, .. + } = local_command(&args) + else { + panic!("expected ClickHouse client for {selectors:?}"); + }; + assert_eq!(name.as_deref(), *expected_name, "selectors: {selectors:?}"); + assert_eq!(host.as_deref(), *expected_host, "selectors: {selectors:?}"); + assert_eq!(port, *expected_port, "selectors: {selectors:?}"); + } + } + + #[test] + fn clickhouse_client_rejects_named_and_direct_selectors_in_every_order() { + let conflicting: &[&[&str]] = &[ + &["--name", "dev", "--host", "db.example"], + &["--host", "db.example", "--name", "dev"], + &["--name", "dev", "--port", "9000"], + &["--port", "9000", "--name", "dev"], + &["--name", "dev", "--host", "db.example", "--port", "9000"], + &["--name", "dev", "--port", "9000", "--host", "db.example"], + &["--host", "db.example", "--name", "dev", "--port", "9000"], + &["--host", "db.example", "--port", "9000", "--name", "dev"], + &["--port", "9000", "--name", "dev", "--host", "db.example"], + &["--port", "9000", "--host", "db.example", "--name", "dev"], + ]; + + for selectors in conflicting { + let args: Vec<&str> = ["client"] + .into_iter() + .chain(selectors.iter().copied()) + .collect(); + let error = local_parse_error(&args); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ArgumentConflict, + "selectors: {selectors:?}" + ); + assert!(error.to_string().contains("--name"), "{error}"); + } + } + + #[test] + fn clickhouse_client_rejects_zero_and_nonnumeric_ports() { + for port in ["0", "not-a-port"] { + let error = local_parse_error(&["client", "--port", port]); + assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation); + assert!(error.to_string().contains("--port"), "{error}"); + } + } + + #[test] + fn clickhouse_client_preserves_passthrough_selector_like_arguments() { + let LocalCommands::Client { name, args, .. } = local_command(&[ + "client", + "--name", + "dev", + "--", + "--host", + "child-host", + "--port", + "0", + ]) else { + panic!("expected ClickHouse client"); + }; + + assert_eq!(name.as_deref(), Some("dev")); + assert_eq!(args, ["--host", "child-host", "--port", "0"]); + } + + #[test] + fn postgres_client_applies_named_and_direct_selector_validation() { + let valid: &[&[&str]] = &[ + &[], + &["--name", "dev"], + &["--version", "18"], + &["--name", "dev", "--version", "18"], + &["--version", "18", "--name", "dev"], + &["--host", "db.example"], + &["--port", "1"], + &["--host", "db.example", "--port", "65535"], + &["--port", "65535", "--host", "db.example"], + ]; + for selectors in valid { + let args: Vec<&str> = ["postgres", "client"] + .into_iter() + .chain(selectors.iter().copied()) + .collect(); + let LocalCommands::Postgres { + command: PostgresCommands::Client { .. }, + } = local_command(&args) + else { + panic!("expected Postgres client for {selectors:?}"); + }; + } + + let conflicting: &[&[&str]] = &[ + &["--name", "dev", "--host", "db.example"], + &["--host", "db.example", "--name", "dev"], + &["--name", "dev", "--port", "5432"], + &["--port", "5432", "--name", "dev"], + &["--version", "18", "--host", "db.example"], + &["--host", "db.example", "--version", "18"], + &["--version", "18", "--port", "5432"], + &["--port", "5432", "--version", "18"], + &[ + "--name", + "dev", + "--version", + "18", + "--host", + "db.example", + "--port", + "5432", + ], + &[ + "--port", + "5432", + "--host", + "db.example", + "--version", + "18", + "--name", + "dev", + ], + ]; + for selectors in conflicting { + let args: Vec<&str> = ["postgres", "client"] + .into_iter() + .chain(selectors.iter().copied()) + .collect(); + assert_eq!( + local_parse_error(&args).kind(), + clap::error::ErrorKind::ArgumentConflict, + "selectors: {selectors:?}" + ); + } + + for port in ["0", "not-a-port"] { + let error = local_parse_error(&["postgres", "client", "--port", port]); + assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation); + assert!(error.to_string().contains("--port"), "{error}"); + } + } + + #[test] + fn postgres_client_preserves_passthrough_selector_like_arguments() { + let LocalCommands::Postgres { + command: PostgresCommands::Client { name, args, .. }, + } = local_command(&[ + "postgres", + "client", + "--name", + "dev", + "--", + "--host", + "child-host", + "--port", + "0", + ]) + else { + panic!("expected Postgres client"); + }; + + assert_eq!(name.as_deref(), Some("dev")); + assert_eq!(args, ["--host", "child-host", "--port", "0"]); + } + #[test] fn install_help_covers_install_requirements() { let error = Cli::try_parse_from(["clickhousectl", "local", "install", "--help"]) diff --git a/crates/clickhousectl/tests/local_client_selectors_test.rs b/crates/clickhousectl/tests/local_client_selectors_test.rs new file mode 100644 index 00000000..6e0a98a2 --- /dev/null +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -0,0 +1,186 @@ +//! Subprocess coverage for local client selector validation and direct-mode defaults. + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +const VERSION: &str = "25.12.9.61"; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn write_arg_printer(path: &Path) { + std::fs::create_dir_all(path.parent().expect("fake child parent")) + .expect("create fake child directory"); + std::fs::write(path, b"#!/bin/sh\nprintf '%s\\n' \"$@\"\n").expect("write fake child"); + let mut permissions = std::fs::metadata(path) + .expect("read fake child metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(path, permissions).expect("make fake child executable"); +} + +fn run(project: &Path, home: &Path, path: Option<&Path>, args: &[&str]) -> Output { + let mut command = Command::new(clickhousectl_binary()); + command + .env_clear() + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .current_dir(project) + .args(args); + if let Some(path) = path { + command.env("PATH", path); + } + command.output().expect("run clickhousectl") +} + +fn assert_child_args(output: Output, expected: &[&str]) { + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(output.status.success(), "stderr: {stderr}"); + let args: Vec<&str> = std::str::from_utf8(&output.stdout) + .expect("child output is UTF-8") + .lines() + .collect(); + assert_eq!(args, expected); +} + +fn assert_usage_before_resolution(args: &[&str], expected: &[&str]) { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let output = run(project.path(), home.path(), None, args); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert_eq!(output.status.code(), Some(2), "stderr: {stderr}"); + for text in expected { + assert!(stderr.contains(text), "missing {text:?} in: {stderr}"); + } + assert!(!stderr.contains("No default version"), "{stderr}"); + assert!(!stderr.contains("Failed to execute"), "{stderr}"); + assert!( + !home.path().join(".clickhouse").exists(), + "parser error resolved a ClickHouse binary" + ); + assert!( + !project.path().join(".clickhouse").exists(), + "parser error resolved project state" + ); +} + +#[test] +fn invalid_clickhouse_selectors_fail_before_binary_or_project_resolution() { + for args in [ + &["local", "client", "--name", "dev", "--host", "remote"][..], + &["local", "client", "--host", "remote", "--name", "dev"], + &["local", "client", "--name", "dev", "--port", "9000"], + &["local", "client", "--port", "9000", "--name", "dev"], + ] { + assert_usage_before_resolution(args, &["--name", "cannot be used"]); + } + assert_usage_before_resolution( + &["local", "client", "--port", "0"], + &["invalid value", "--port"], + ); + assert_usage_before_resolution( + &["local", "client", "--port", "not-a-port"], + &["invalid value", "--port"], + ); +} + +#[test] +fn clickhouse_direct_selectors_reach_fake_child_with_documented_defaults() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let binary = home + .path() + .join(".clickhouse/versions") + .join(VERSION) + .join("clickhouse"); + write_arg_printer(&binary); + std::fs::write(home.path().join(".clickhouse/default"), VERSION) + .expect("write default version"); + + assert_child_args( + run( + project.path(), + home.path(), + None, + &["local", "client", "--host", "remote", "--query", "SELECT 1"], + ), + &[ + "client", "--host", "remote", "--port", "9000", "--query", "SELECT 1", + ], + ); + assert_child_args( + run( + project.path(), + home.path(), + None, + &["local", "client", "--port", "65535"], + ), + &["client", "--host", "localhost", "--port", "65535"], + ); +} + +#[test] +fn postgres_client_uses_the_same_validation_and_direct_mode_defaults() { + assert_usage_before_resolution( + &[ + "local", "postgres", "client", "--name", "dev", "--host", "remote", + ], + &["--name", "cannot be used"], + ); + assert_usage_before_resolution( + &[ + "local", + "postgres", + "client", + "--version", + "18", + "--port", + "5432", + ], + &["--version", "cannot be used"], + ); + assert_usage_before_resolution( + &["local", "postgres", "client", "--port", "0"], + &["invalid value", "--port"], + ); + + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let bin = home.path().join("bin"); + write_arg_printer(&bin.join("psql")); + + assert_child_args( + run( + project.path(), + home.path(), + Some(&bin), + &[ + "local", "postgres", "client", "--host", "remote", "--query", "SELECT 1", + ], + ), + &[ + "-h", "remote", "-p", "5432", "-U", "postgres", "-d", "postgres", "-c", "SELECT 1", + ], + ); + assert_child_args( + run( + project.path(), + home.path(), + Some(&bin), + &["local", "postgres", "client", "--port", "65535"], + ), + &[ + "-h", + "127.0.0.1", + "-p", + "65535", + "-U", + "postgres", + "-d", + "postgres", + ], + ); +} From 7f434f6ebe031e409f2d5dab8c970446ac65091c Mon Sep 17 00:00:00 2001 From: sdairs Date: Thu, 27 Aug 2026 07:49:18 +0100 Subject: [PATCH 2/2] Confirm selector validation review