From c9defdbe0b7e7306e179104fb4fe669514034c05 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:02:28 +0100 Subject: [PATCH 01/12] Document PostgreSQL ClickPipe TLS setup --- README.md | 21 +++++++++- crates/clickhousectl/src/cloud/clickpipes.rs | 41 +++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 5bfc06f1..b2574922 100644 --- a/README.md +++ b/README.md @@ -772,7 +772,9 @@ clickhousectl cloud clickpipe create kinesis \ --database default --table events \ --column "event_id:Int64" --column "name:String" -# From PostgreSQL (CDC) +# From PostgreSQL (CDC) with a publicly trusted TLS certificate +# TLS and certificate verification are enabled by default. The certificate +# hostname defaults to --host. clickhousectl cloud clickpipe create postgres \ --name my-pg-pipe \ --host db.example.com --pg-database mydb \ @@ -780,6 +782,14 @@ clickhousectl cloud clickpipe create postgres \ --table-mapping "public.users:public_users" \ --table-mapping "public.orders:public_orders" +# From PostgreSQL with a private or self-signed certificate +clickhousectl cloud clickpipe create postgres \ + --name my-private-pg-pipe \ + --host db.private.example.com --pg-database mydb \ + --username "$POSTGRES_USERNAME" --password "$POSTGRES_PASSWORD" \ + --ca-certificate ./postgres-ca.pem \ + --table-mapping "public.users:public_users" + # From MySQL (CDC) # --server-id sets the replication server ID (useful when multiple pipes read # from the same MySQL instance, or to avoid colliding with existing replicas) @@ -805,6 +815,15 @@ clickhousectl cloud clickpipe create bigquery \ --table-mapping "dataset.table:target_table" ``` +Before creating a PostgreSQL CDC ClickPipe: + +- Make the source reachable from ClickPipes and allow the [ClickPipes static IPs](https://clickhouse.com/docs/integrations/clickpipes/networking/static-ips) for your service region. +- Enable logical replication on PostgreSQL. +- Create a publication that includes every source table passed with `--table-mapping`. +- Grant the ClickPipes user schema `USAGE`, table `SELECT`, and replication privileges. + +See the [PostgreSQL ClickPipes setup guide](https://clickhouse.com/docs/integrations/clickpipes/postgres) for provider-specific prerequisites or the [generic PostgreSQL source setup](https://clickhouse.com/docs/integrations/clickpipes/postgres/source/generic) for self-hosted and other providers. `--ca-certificate` is needed only when the source certificate is signed by a private CA or is self-signed. Use `--tls-host` when the hostname in that certificate differs from `--host`. + Use `clickhousectl cloud clickpipe create --help` for the full list of options per source type. #### Discovering a source schema (beta) diff --git a/crates/clickhousectl/src/cloud/clickpipes.rs b/crates/clickhousectl/src/cloud/clickpipes.rs index 5d885b2c..aabde579 100644 --- a/crates/clickhousectl/src/cloud/clickpipes.rs +++ b/crates/clickhousectl/src/cloud/clickpipes.rs @@ -328,6 +328,10 @@ pub enum ClickPipeCreateCommands { Kinesis(KinesisCreateArgs), /// Create a ClickPipe from PostgreSQL + #[command( + long_about = "Create a ClickPipe from PostgreSQL.\n\nTLS and certificate verification are enabled by default. Omit --ca-certificate when the source certificate has a publicly trusted chain. For a private or self-signed source certificate, pass its PEM CA bundle with --ca-certificate. Certificate hostname verification uses --host unless --tls-host is provided.", + after_long_help = "PostgreSQL ClickPipes setup:\n https://clickhouse.com/docs/integrations/clickpipes/postgres\nGeneric PostgreSQL source setup:\n https://clickhouse.com/docs/integrations/clickpipes/postgres/source/generic\nClickPipes networking and static IPs:\n https://clickhouse.com/docs/integrations/clickpipes/networking/static-ips" + )] Postgres(PostgresCreateArgs), /// Create a ClickPipe from MySQL @@ -718,11 +722,11 @@ pub struct PostgresCreateArgs { #[arg(long, required_if_eq("auth", "IAM_ROLE"))] pub iam_role: Option, - /// TLS hostname + /// Certificate hostname to verify (defaults to --host) #[arg(long)] pub tls_host: Option, - /// Path to CA certificate file + /// Path to a PEM CA bundle for a private or self-signed source certificate #[arg(long)] pub ca_certificate: Option, @@ -2322,6 +2326,39 @@ mod tests { ); } + #[test] + fn postgres_create_help_explains_tls_defaults_and_prerequisites() { + let error = Cli::try_parse_from([ + "clickhousectl", + "cloud", + "clickpipe", + "create", + "postgres", + "--help", + ]) + .err() + .expect("--help should stop parsing"); + assert_eq!(error.kind(), clap::error::ErrorKind::DisplayHelp); + + let help = error.to_string(); + for expected in [ + "TLS and certificate verification are enabled by default", + "Omit --ca-certificate when the source certificate has a publicly trusted chain", + "private or self-signed source certificate", + "Certificate hostname to verify (defaults to --host)", + "https://clickhouse.com/docs/integrations/clickpipes/postgres", + "https://clickhouse.com/docs/integrations/clickpipes/postgres/source/generic", + "https://clickhouse.com/docs/integrations/clickpipes/networking/static-ips", + ] { + assert!(help.contains(expected), "missing {expected:?} in help:\n{help}"); + } + assert!(!help.contains("--disable-tls"), "unexpected TLS bypass flag"); + assert!( + !help.contains("--skip-cert-verification"), + "unexpected certificate bypass flag" + ); + } + fn assert_object_storage_value(flag: &str, value: &str) { if flag == "--format" { parse_clickpipe(&[ From eca772b4987f40e667030944f4af482278b6274d Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:03:33 +0100 Subject: [PATCH 02/12] Guide PostgreSQL ClickPipe TLS failures --- crates/clickhousectl/src/cloud/clickpipes.rs | 36 +++++++++++- .../tests/cli_request_shape_test.rs | 57 +++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/crates/clickhousectl/src/cloud/clickpipes.rs b/crates/clickhousectl/src/cloud/clickpipes.rs index aabde579..8b155b38 100644 --- a/crates/clickhousectl/src/cloud/clickpipes.rs +++ b/crates/clickhousectl/src/cloud/clickpipes.rs @@ -1809,11 +1809,35 @@ async fn clickpipe_create_postgres( let org_id = resolve_org_id(client, args.org_id.as_deref()).await?; let clickpipe = client .create_clickpipe(&org_id, &args.service_id, &request) - .await?; + .await + .map_err(add_postgres_tls_guidance)?; print_created(&clickpipe, json)?; Ok(()) } +fn add_postgres_tls_guidance(mut error: CloudError) -> CloudError { + let message = error.message.to_ascii_lowercase(); + let hint = if message.contains("x509: certificate signed by unknown authority") { + Some("Provide the PostgreSQL source's PEM CA bundle with --ca-certificate .") + } else if message.contains("x509: hostname mismatch") + || (message.contains("x509: certificate is valid for ") && message.contains(", not ")) + || (message.contains("x509: cannot validate certificate for ") + && message.contains(" because it doesn't contain any ip sans")) + { + Some( + "Set --tls-host to a DNS name covered by the PostgreSQL source certificate (it defaults to --host).", + ) + } else { + None + }; + + if let Some(hint) = hint { + error.message.push_str("\n\nHint: "); + error.message.push_str(hint); + } + error +} + fn build_postgres_request( args: &PostgresCreateArgs, ) -> CloudResult { @@ -2350,9 +2374,15 @@ mod tests { "https://clickhouse.com/docs/integrations/clickpipes/postgres/source/generic", "https://clickhouse.com/docs/integrations/clickpipes/networking/static-ips", ] { - assert!(help.contains(expected), "missing {expected:?} in help:\n{help}"); + assert!( + help.contains(expected), + "missing {expected:?} in help:\n{help}" + ); } - assert!(!help.contains("--disable-tls"), "unexpected TLS bypass flag"); + assert!( + !help.contains("--disable-tls"), + "unexpected TLS bypass flag" + ); assert!( !help.contains("--skip-cert-verification"), "unexpected certificate bypass flag" diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 7c412ce8..e08b87f3 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -2350,6 +2350,63 @@ fn postgres_args_minimal() -> Vec { .collect() } +async fn invoke_postgres_create_api_error(message: &str) -> std::process::Output { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/organizations/org/services/svc-id/clickpipes")) + .respond_with(ResponseTemplate::new(400).set_body_json(serde_json::json!({ + "status": 400, + "error": message, + "requestId": "stub-postgres-create-error", + }))) + .mount(&mock) + .await; + + let args = postgres_args_minimal(); + let arg_refs: Vec<&str> = args.iter().map(String::as_str).collect(); + invoke_cli_with_cloud_credentials(&mock, &arg_refs) +} + +#[tokio::test] +async fn postgres_unknown_authority_error_preserves_detail_and_names_ca_flag() { + let api_error = "BAD_REQUEST: failed to establish connection: tls: failed to verify certificate: x509: certificate signed by unknown authority"; + let output = invoke_postgres_create_api_error(api_error).await; + + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!( + "Error: {api_error}\n\nHint: Provide the PostgreSQL source's PEM CA bundle with --ca-certificate .\n" + ) + ); +} + +#[tokio::test] +async fn postgres_hostname_mismatch_error_preserves_detail_and_names_tls_host_flag() { + let api_error = "BAD_REQUEST: failed to establish connection: tls: failed to verify certificate: x509: certificate is valid for pg.example.com, not db.example.com"; + let output = invoke_postgres_create_api_error(api_error).await; + + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!( + "Error: {api_error}\n\nHint: Set --tls-host to a DNS name covered by the PostgreSQL source certificate (it defaults to --host).\n" + ) + ); +} + +#[tokio::test] +async fn postgres_unrelated_connection_error_has_no_tls_hint() { + let api_error = "BAD_REQUEST: failed to establish connection: connection refused"; + let output = invoke_postgres_create_api_error(api_error).await; + + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!("Error: {api_error}\n") + ); +} + #[tokio::test] async fn postgres_invalid_inputs_fail_before_cloud_api_dispatch() { let without_org = || { From 7048364e99774d4a318163b25a05a8d574cd05c6 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 10:12:02 +0100 Subject: [PATCH 03/12] Document PostgreSQL ClickPipe publication --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b2574922..df5e08ff 100644 --- a/README.md +++ b/README.md @@ -779,6 +779,7 @@ clickhousectl cloud clickpipe create postgres \ --name my-pg-pipe \ --host db.example.com --pg-database mydb \ --username "$POSTGRES_USERNAME" --password "$POSTGRES_PASSWORD" \ + --publication-name clickpipes \ --table-mapping "public.users:public_users" \ --table-mapping "public.orders:public_orders" @@ -787,6 +788,7 @@ clickhousectl cloud clickpipe create postgres \ --name my-private-pg-pipe \ --host db.private.example.com --pg-database mydb \ --username "$POSTGRES_USERNAME" --password "$POSTGRES_PASSWORD" \ + --publication-name clickpipes \ --ca-certificate ./postgres-ca.pem \ --table-mapping "public.users:public_users" @@ -819,7 +821,7 @@ Before creating a PostgreSQL CDC ClickPipe: - Make the source reachable from ClickPipes and allow the [ClickPipes static IPs](https://clickhouse.com/docs/integrations/clickpipes/networking/static-ips) for your service region. - Enable logical replication on PostgreSQL. -- Create a publication that includes every source table passed with `--table-mapping`. +- Create a publication named `clickpipes` that includes every source table passed with `--table-mapping`. - Grant the ClickPipes user schema `USAGE`, table `SELECT`, and replication privileges. See the [PostgreSQL ClickPipes setup guide](https://clickhouse.com/docs/integrations/clickpipes/postgres) for provider-specific prerequisites or the [generic PostgreSQL source setup](https://clickhouse.com/docs/integrations/clickpipes/postgres/source/generic) for self-hosted and other providers. `--ca-certificate` is needed only when the source certificate is signed by a private CA or is self-signed. Use `--tls-host` when the hostname in that certificate differs from `--host`. From c305b50381bf883fd25f5be947281564e43d039f Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:52:55 +0100 Subject: [PATCH 04/12] Exercise PostgreSQL ClickPipe creation through CLI --- Cargo.lock | 1 + crates/clickhouse-cloud-api/Cargo.toml | 1 + .../tests/clickpipes/postgres_cdc_test.rs | 175 +++++++++++------- 3 files changed, 115 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c5b3205..3596f81e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -985,6 +985,7 @@ dependencies = [ "rustls-pemfile", "serde", "serde_json", + "tempfile", "thiserror", "tokio", "tokio-postgres", diff --git a/crates/clickhouse-cloud-api/Cargo.toml b/crates/clickhouse-cloud-api/Cargo.toml index 92faacdc..6995d5ba 100644 --- a/crates/clickhouse-cloud-api/Cargo.toml +++ b/crates/clickhouse-cloud-api/Cargo.toml @@ -45,6 +45,7 @@ rcgen = "0.14.8" rustls = { version = "0.23.40", default-features = false, features = ["ring", "std", "tls12"] } rustls-native-certs = "0.8.3" rustls-pemfile = "2.2.0" +tempfile = "3.27.0" tokio = { version = "1", features = ["macros", "rt-multi-thread"] } tokio-postgres = "0.7.17" tokio-postgres-rustls = "0.13.0" diff --git a/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs b/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs index 82f5018e..c09f4753 100644 --- a/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs +++ b/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs @@ -1,7 +1,16 @@ +//! Managed-Postgres CDC E2E. Resource setup, state polling, and cleanup use the +//! API client, while both ClickPipe creation attempts run the built CLI binary. +//! +//! The managed Postgres fixture has a private CA. Mandatory CI has no +//! publicly-trusted PostgreSQL fixture, so successful creation without an +//! uploaded CA remains a residual gap. + #[path = "../common/mod.rs"] mod common; mod support; +use std::path::PathBuf; +use std::process::Command; use std::str::FromStr; use std::time::Duration; @@ -19,16 +28,24 @@ const POST_SEED_ROW_COUNT: i64 = 8; const DEFAULT_CLICKPIPE_READY_TIMEOUT_SECS: u64 = 600; const DEFAULT_CDC_LAG_TIMEOUT_SECS: u64 = 300; +const CLICKHOUSECTL_BINARY_ENV: &str = "CLICKHOUSECTL_TEST_BINARY"; +const PRIVATE_CA_API_ERROR: &str = "x509: certificate signed by unknown authority"; +const PRIVATE_CA_HINT: &str = + "Hint: Provide the PostgreSQL source's PEM CA bundle with --ca-certificate ."; #[tokio::test] #[ignore = "requires live ClickHouse Cloud credentials and provisions real resources"] -async fn cloud_clickpipe_postgres_cdc() -> TestResult<()> { +async fn cloud_clickpipe_postgres_cli_cdc() -> TestResult<()> { // rustls 0.23 requires a default CryptoProvider be installed before any // ClientConfig is constructed. install_default returns an Err if one was // already set by another test in the same process, which is harmless here. let _ = rustls::crypto::ring::default_provider().install_default(); let ctx = TestContext::from_env()?; + let clickhousectl = clickhousectl_binary()?; + let cli_workspace = tempfile::tempdir()?; + let cli_home = cli_workspace.path().join("home"); + std::fs::create_dir(&cli_home)?; let clickpipe_ready_timeout = duration_from_env_or( "CLICKHOUSE_CLOUD_TEST_TIMEOUT_CLICKPIPE_READY_SECS", DEFAULT_CLICKPIPE_READY_TIMEOUT_SECS, @@ -42,7 +59,7 @@ async fn cloud_clickpipe_postgres_cdc() -> TestResult<()> { let mut cleanup = CleanupRegistry::default(); let test_result = async { - log_run_header("cloud_clickpipe_postgres_cdc", &ctx); + log_run_header("cloud_clickpipe_postgres_cli_cdc", &ctx); let mut failures = FailureRecorder::default(); // ── Preflight ─────────────────────────────────────────────── @@ -252,81 +269,98 @@ async fn cloud_clickpipe_postgres_cdc() -> TestResult<()> { let pg_ca_pem = client .postgres_service_certs_get(&ctx.org_id, &postgres_id) - .await - .ok(); + .await?; + if pg_ca_pem.trim().is_empty() { + return Err("postgres CA endpoint returned an empty bundle".into()); + } + let pg_ca_path = cli_workspace.path().join("postgres-ca.pem"); + std::fs::write(&pg_ca_path, pg_ca_pem.as_bytes())?; let pg_client = connect_postgres( &pg_hostname, pg_port, &pg_database, &pg_username, &pg_password, - pg_ca_pem.as_deref(), + Some(&pg_ca_pem), ) .await?; configure_pg_for_cdc(&pg_client).await?; - // ── Create ClickPipe ──────────────────────────────────────── + // ── Create ClickPipe through the real CLI ─────────────────── log_phase("Create ClickPipe"); - let pipe_request = ClickPipePostRequest { - name: format!("cdc-{}", ctx.run_id), - destination: ClickPipeMutateDestination { - // Postgres is a "database pipe" — only `database` is valid at - // the top level. The per-mapping `targetTable` carries the - // destination table name. - database: "default".to_string(), - ..Default::default() - }, - source: ClickPipePostSource { - postgres: Some(ClickPipeMutatePostgresSource { - authentication: ClickPipeMutatePostgresSourceAuthentication::Basic, - ca_certificate: pg_ca_pem.clone(), - credentials: PLAIN { - username: pg_username.clone(), - password: pg_password.clone(), - }, - database: pg_database.clone(), - host: pg_hostname.clone(), - port: pg_port as i64, - settings: ClickPipePostgresPipeSettings { - // `cdc` does snapshot + ongoing replication. The API - // manages the replication slot itself in this mode and - // rejects an explicit replicationSlotName. - replication_mode: ClickPipePostgresPipeSettingsReplicationmode::Cdc, - publication_name: Some(PUBLICATION.to_string()), - // The API rejects 0 for numeric fields ("Value must be >= 1"); - // the Default impl gives every i64 a 0, so we set sensible - // values explicitly. (Same issue exists in the CLI's - // clickpipe create postgres handler — track separately.) - sync_interval_seconds: Some(60), - pull_batch_size: Some(100_000), - initial_load_parallelism: Some(4), - snapshot_num_rows_per_partition: Some(100_000), - snapshot_number_of_parallel_tables: Some(4), - ..Default::default() - }, - table_mappings: vec![ClickPipePostgresPipeTableMapping { - source_schema_name: SOURCE_SCHEMA.to_string(), - source_table: SOURCE_TABLE.to_string(), - target_table: TARGET_TABLE.to_string(), - table_engine: - ClickPipePostgresPipeTableMappingTableengine::ReplacingMergeTree, - ..Default::default() - }], - r#type: Some(ClickPipeMutatePostgresSourceType::Postgres), - ..Default::default() - }), - ..Default::default() - }, - ..Default::default() + let run_cli_create = |name: &str, + ca_path: Option<&std::path::Path>| + -> TestResult { + let mut command = Command::new(&clickhousectl); + command + .env("DO_NOT_TRACK", "1") + .env("HOME", &cli_home) + .current_dir(cli_workspace.path()) + .arg("cloud"); + if let Some(base_url) = std::env::var("CLICKHOUSE_CLOUD_API_BASE_URL") + .ok() + .filter(|value| !value.is_empty()) + { + command.args(["--url", &base_url]); + } + command + .arg("--json") + .args(["clickpipe", "create", "postgres"]) + .arg(&clickhouse_id) + .args(["--name", name]) + .args(["--host", &pg_hostname]) + .arg("--port") + .arg(pg_port.to_string()) + .args(["--pg-database", &pg_database]) + .args(["--username", &pg_username]) + .args(["--password", &pg_password]) + .args([ + "--table-mapping", + &format!("{SOURCE_SCHEMA}.{SOURCE_TABLE}:{TARGET_TABLE}"), + ]) + .args(["--publication-name", PUBLICATION]) + .args(["--org-id", &ctx.org_id]); + if let Some(path) = ca_path { + command.arg("--ca-certificate").arg(path); + } + Ok(command.output()?) }; - let pipe = client - .click_pipe_create(&ctx.org_id, &clickhouse_id, &pipe_request) - .await? - .result - .ok_or("clickpipe create returned no result")?; + let without_ca = run_cli_create(&format!("private-ca-check-{}", ctx.run_id), None)?; + if without_ca.status.code() != Some(1) { + return Err(format!( + "clickhousectl without --ca-certificate exited {:?}, expected 1", + without_ca.status.code() + ) + .into()); + } + let without_ca_stderr = String::from_utf8_lossy(&without_ca.stderr); + if !without_ca_stderr.contains(PRIVATE_CA_API_ERROR) { + return Err(format!( + "private-CA failure did not preserve API detail `{PRIVATE_CA_API_ERROR}`: {without_ca_stderr}" + ) + .into()); + } + if !without_ca_stderr.contains(PRIVATE_CA_HINT) { + return Err(format!( + "private-CA failure did not include the CA flag guidance: {without_ca_stderr}" + ) + .into()); + } + + let with_ca = run_cli_create(&format!("cdc-{}", ctx.run_id), Some(&pg_ca_path))?; + if !with_ca.status.success() { + return Err(format!( + "clickhousectl with --ca-certificate exited {:?}: {}", + with_ca.status.code(), + String::from_utf8_lossy(&with_ca.stderr) + ) + .into()); + } + let pipe: ClickPipe = serde_json::from_slice(&with_ca.stdout) + .map_err(|error| format!("clickhousectl returned invalid ClickPipe JSON: {error}"))?; let clickpipe_id = clickpipe_id(&pipe)?; cleanup.register_clickpipe(clickhouse_id.clone(), clickpipe_id.clone()); eprintln!(" provisioned clickpipe id "); @@ -472,6 +506,23 @@ fn log_step(message: &str) { eprintln!(" step: {message}"); } +fn clickhousectl_binary() -> TestResult { + let configured = std::env::var_os(CLICKHOUSECTL_BINARY_ENV).ok_or_else(|| { + format!( + "{CLICKHOUSECTL_BINARY_ENV} must point to a built clickhousectl binary; run `cargo build -p clickhousectl` first" + ) + })?; + let binary = std::fs::canonicalize(configured)?; + if !binary.is_file() { + return Err(format!( + "{CLICKHOUSECTL_BINARY_ENV} does not point to a file: {}", + binary.display() + ) + .into()); + } + Ok(binary) +} + fn duration_from_env_or(name: &str, default_secs: u64) -> TestResult { match std::env::var(name) { Ok(value) => Ok(Duration::from_secs(value.parse()?)), From e411c3c1c99db6a0ac8588ac4a4b62f20c3a9c8b Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:52:59 +0100 Subject: [PATCH 05/12] Run CLI-backed ClickPipe test in live CI --- .github/workflows/cloud-integration.yml | 8 ++++++++ AGENTS.md | 2 +- crates/clickhouse-cloud-api/README.md | 5 ++++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cloud-integration.yml b/.github/workflows/cloud-integration.yml index 36db509a..12631548 100644 --- a/.github/workflows/cloud-integration.yml +++ b/.github/workflows/cloud-integration.yml @@ -254,10 +254,18 @@ jobs: needs.plan.outputs.organization == 'true' }} run: cargo test -p clickhouse-cloud-api --test integration_org_test -- --ignored --nocapture + - name: Build CLI for ClickPipe Postgres CDC integration test + if: >- + ${{ !cancelled() && + needs.plan.outputs.clickpipes == 'true' }} + run: cargo build -p clickhousectl + - name: Run ClickPipe Postgres CDC integration test if: >- ${{ !cancelled() && needs.plan.outputs.clickpipes == 'true' }} + env: + CLICKHOUSECTL_TEST_BINARY: ${{ github.workspace }}/target/debug/clickhousectl run: cargo test -p clickhouse-cloud-api --test clickpipe_postgres_cdc_test -- --ignored --nocapture # ClickPipe create smoke tests run against a long-lived ClickHouse Cloud diff --git a/AGENTS.md b/AGENTS.md index 9ff0aef8..6c13a7e1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -186,7 +186,7 @@ Real cloud integration tests, 100% OpenAPI spec coverage. Cost is not a reason t - `tests/common/support.rs` — generic test infra (polling, logging, env helpers, ClickHouse provisioning & cleanup, HTTP query helper). Used by every integration binary. Call `Client` directly from Rust. - `tests/integration_test.rs`, `tests/integration_postgres_test.rs`, `tests/integration_org_test.rs` — cloud-service, Postgres-service, and organization lifecycle tests. -- `tests/clickpipes/` — ClickPipes E2E suite, including external cloud services. Only Postgres CDC (uses ClickHouse & Postgres inside ClickHouse Cloud) is run in CI. Tests for third party services must be executed manually. CI also optionally runs `clickpipe_smoke_test` against a long-lived service when the `CLICKHOUSE_CLOUD_TEST_CLICKPIPE_SERVICE_ID` repo variable is set (see `.github/workflows/cloud-integration.yml`); the step is skipped when the variable is unset. +- `tests/clickpipes/` — ClickPipes E2E suite, including external cloud services. Only Postgres CDC (uses ClickHouse & Postgres inside ClickHouse Cloud and the built `clickhousectl` binary for ClickPipe creation) is run in CI. Tests for third party services must be executed manually. CI also optionally runs `clickpipe_smoke_test` against a long-lived service when the `CLICKHOUSE_CLOUD_TEST_CLICKPIPE_SERVICE_ID` repo variable is set (see `.github/workflows/cloud-integration.yml`); the step is skipped when the variable is unset. - `spec_coverage_test.rs`: runs the shared analyzer against the vendored OpenAPI snapshot and requires an actionable-drift-free report. - Labeled internal PRs classify the exact base-to-head diff with `scripts/classify-cloud-integration.py` and run only affected `service`, `postgres`, `organization`, and `clickpipes` suites. New or renamed API source/test files must be added to its explicit mappings; unknown paths fail closed to all suites. Scheduled runs still select all suites, while manual runs use the requested scope. diff --git a/crates/clickhouse-cloud-api/README.md b/crates/clickhouse-cloud-api/README.md index ba8ce1ab..5cd82d34 100644 --- a/crates/clickhouse-cloud-api/README.md +++ b/crates/clickhouse-cloud-api/README.md @@ -80,12 +80,15 @@ cargo test --test clickpipe_kinesis_test -- --ignored --nocapture # per-s cargo test --test clickpipe_mongo_test -- --ignored --nocapture # per-source: MongoDB cargo test --test clickpipe_mysql_test -- --ignored --nocapture # per-source: MySQL cargo test --test clickpipe_postgres_ec2_test -- --ignored --nocapture # per-source: Postgres-on-EC2 -cargo test --test clickpipe_postgres_cdc_test -- --ignored --nocapture # CHC-managed Postgres CDC +cargo build -p clickhousectl +CLICKHOUSECTL_TEST_BINARY=../../target/debug/clickhousectl cargo test --test clickpipe_postgres_cdc_test -- --ignored --nocapture # CHC-managed Postgres CDC through the CLI cargo test --test clickpipe_smoke_test -- --ignored --nocapture # create-only smoke against a shared service ``` All require `CLICKHOUSE_CLOUD_API_KEY`, `CLICKHOUSE_CLOUD_API_SECRET`, `CLICKHOUSE_CLOUD_TEST_ORG_ID`, `CLICKHOUSE_CLOUD_TEST_PROVIDER`, and `CLICKHOUSE_CLOUD_TEST_REGION` in the environment, and are wired into the scheduled `Cloud Integration` GitHub Actions workflow. The ClickPipes E2E suites additionally need AWS credentials and an `eu-west-1` region quota; `clickpipe_smoke_test` reads a pre-provisioned service ID from `CLICKHOUSE_CLOUD_TEST_CLICKPIPE_SERVICE_ID`. +The managed-Postgres CDC test fetches its private CA, writes it to a temporary file, and exercises failed CLI creation without `--ca-certificate` followed by successful snapshot and CDC with the flag. The mandatory CI fixtures do not currently include a PostgreSQL source with a publicly trusted certificate, so successful creation without an uploaded CA remains untested. + Applying `run-cloud-integration` to an eligible PR selects suites from that PR's merge-base-to-head diff; known changes without a live suite finish without entering the environment-bearing job, while unknown API source or test paths select all suites. Changes to the classifier, its tests, or the workflow also select all suites through an independent workflow guard. Manual `Cloud Integration` dispatches accept `scope=all`, `service`, `postgres`, `organization`, or `clickpipes`. The focused scopes run only their corresponding suite; `clickpipes` runs Postgres CDC plus the fixture-gated smoke test, while `all` runs all four mandatory suites plus that optional smoke test. Because stacked-PR diffs exclude inherited changes, manually run `scope=all` against the top stack branch for full-stack validation. `spec_coverage_test` sends the checked-in sources and snapshot through the From 58d912443a636401560b1a76a7b4338ec1c7e111 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 17:29:03 +0100 Subject: [PATCH 06/12] Verify CLI-created tables without FINAL --- .../clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs b/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs index c09f4753..bbdccce5 100644 --- a/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs +++ b/crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs @@ -664,7 +664,7 @@ impl ClickHouseQuery { async fn count_rows(&self, table: &str) -> TestResult { let body = self .run_query(&format!( - "SELECT count() FROM default.{table} FINAL FORMAT TabSeparated" + "SELECT count() FROM default.{table} FORMAT TabSeparated" )) .await?; Ok(body.trim().parse::()?) From 32fb4c25654fe1f3ad75d72bc0c5bf2dacccbbb1 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 12:53:39 +0100 Subject: [PATCH 07/12] Fix local server start help quotes --- crates/clickhousectl/src/local/cli.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 5935de7a..b2210d92 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -188,7 +188,7 @@ CONTEXT FOR AGENTS: Additional clickhouse-server arguments must follow `--`. Related: `clickhousectl local server list` to see servers, `clickhousectl local server stop [name]` to stop one.")] Start { - /// Server name (default: \"default\", or random if default is already running) + /// Server name (default: "default", or random if default is already running) #[arg(value_name = "NAME", conflicts_with = "name_flag")] name: Option, @@ -530,6 +530,22 @@ mod tests { assert_eq!(config_file.as_deref(), Some("analytics")); } + #[test] + fn server_start_help_renders_default_name_quotes_without_escaping() { + let help = Cli::try_parse_from(["clickhousectl", "local", "server", "start", "--help"]) + .err() + .expect("help should exit through clap") + .to_string(); + + assert!( + help.contains( + r#" [NAME] Server name (default: "default", or random if default is already running)"# + ), + "{help}" + ); + assert!(!help.contains(r#"\"default\""#), "{help}"); + } + #[test] fn parses_server_start_config_file_legacy_alias() { let LocalCommands::Server { From d788938a471c756ed6adfc404235fc3f0446ce40 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 13:19:00 +0100 Subject: [PATCH 08/12] Add local teardown name compatibility --- crates/clickhousectl/src/local/cli.rs | 153 +++++++++++++++++++++++--- crates/clickhousectl/src/local/mod.rs | 5 +- 2 files changed, 141 insertions(+), 17 deletions(-) diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index b2210d92..b5a14210 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -253,8 +253,9 @@ CONTEXT FOR AGENTS: /// Stop a running server by name #[command(after_help = "\ CONTEXT FOR AGENTS: - Stops a ClickHouse server. The name defaults to \"default\"; use `clickhousectl local server list` - to find other server names. + Stops a ClickHouse server. The name defaults to \"default\"; pass it positionally to select + another server (e.g., `server stop dev`). The older `--name dev` form remains accepted, but + cannot be combined with a positional name. Use `clickhousectl local server list` to find names. Sends SIGTERM first, then SIGKILL if the process doesn't exit gracefully. The server's data and metadata are preserved so it remains visible in `server list`. Restart with `clickhousectl local server start `. @@ -263,8 +264,12 @@ CONTEXT FOR AGENTS: Related: `clickhousectl local server list` to see servers.")] Stop { /// Name of the server to stop (default: "default") - #[arg(default_value = "default")] - name: String, + #[arg(value_name = "NAME", conflicts_with = "name_flag")] + name: Option, + + /// Compatibility form for the server name; prefer positional NAME + #[arg(long = "name", value_name = "NAME", conflicts_with = "name")] + name_flag: Option, /// System-wide maintenance only: stop a server from any project. You almost certainly want the default project-scoped stop instead. #[arg(long)] @@ -295,12 +300,17 @@ CONTEXT FOR AGENTS: CONTEXT FOR AGENTS: Permanently deletes a server's data directory. The server must be stopped first. This is irreversible — all data for this server instance will be lost. - The name defaults to \"default\". + The name defaults to \"default\"; pass it positionally to select another server. The older + `--name dev` form remains accepted, but cannot be combined with a positional name. Related: `clickhousectl local server stop [name]` to stop first, `clickhousectl local server list` to see servers.")] Remove { /// Name of the server to remove (default: "default") - #[arg(default_value = "default")] - name: String, + #[arg(value_name = "NAME", conflicts_with = "name_flag")] + name: Option, + + /// Compatibility form for the server name; prefer positional NAME + #[arg(long = "name", value_name = "NAME", conflicts_with = "name")] + name_flag: Option, }, /// Write ClickHouse connection env vars to a .env file @@ -707,25 +717,136 @@ mod tests { } #[test] - fn server_stop_name_defaults_to_default() { + fn server_stop_omission_stays_explicit() { let LocalCommands::Server { - command: ServerCommands::Stop { name, .. }, + command: ServerCommands::Stop { + name, name_flag, .. + }, } = local_command(&["server", "stop"]) else { panic!("expected server stop"); }; - assert_eq!(name, "default"); + assert_eq!(name, None); + assert_eq!(name_flag, None); } #[test] - fn server_remove_name_defaults_to_default() { + fn server_remove_omission_stays_explicit() { let LocalCommands::Server { - command: ServerCommands::Remove { name }, + command: ServerCommands::Remove { name, name_flag }, } = local_command(&["server", "remove"]) else { panic!("expected server remove"); }; - assert_eq!(name, "default"); + assert_eq!(name, None); + assert_eq!(name_flag, None); + } + + #[test] + fn server_stop_accepts_both_name_forms_before_trailing_options() { + let LocalCommands::Server { + command: + ServerCommands::Stop { + name, + name_flag, + global, + project, + }, + } = local_command(&[ + "server", + "stop", + "analytics", + "--global", + "--project", + "/tmp/project", + ]) + else { + panic!("expected server stop"); + }; + assert_eq!(name.as_deref(), Some("analytics")); + assert_eq!(name_flag, None); + assert!(global); + assert_eq!(project.as_deref(), Some("/tmp/project")); + + let LocalCommands::Server { + command: + ServerCommands::Stop { + name, + name_flag, + global, + project, + }, + } = local_command(&[ + "server", + "stop", + "--name", + "analytics", + "--global", + "--project", + "/tmp/project", + ]) + else { + panic!("expected server stop"); + }; + assert_eq!(name, None); + assert_eq!(name_flag.as_deref(), Some("analytics")); + assert!(global); + assert_eq!(project.as_deref(), Some("/tmp/project")); + } + + #[test] + fn server_remove_accepts_both_name_forms_before_trailing_options() { + let LocalCommands::Server { + command: ServerCommands::Remove { name, name_flag }, + } = local_command(&["server", "remove", "analytics", "--json"]) + else { + panic!("expected server remove"); + }; + assert_eq!(name.as_deref(), Some("analytics")); + assert_eq!(name_flag, None); + + let LocalCommands::Server { + command: ServerCommands::Remove { name, name_flag }, + } = local_command(&["server", "remove", "--name", "analytics", "--json"]) + else { + panic!("expected server remove"); + }; + assert_eq!(name, None); + assert_eq!(name_flag.as_deref(), Some("analytics")); + } + + #[test] + fn server_stop_name_forms_conflict() { + let error = Cli::try_parse_from([ + "clickhousectl", + "local", + "server", + "stop", + "existing", + "--name", + "other", + ]) + .err() + .expect("name forms should conflict"); + assert_eq!(error.kind(), clap::error::ErrorKind::ArgumentConflict); + assert!(error.to_string().contains("cannot be used with")); + } + + #[test] + fn server_remove_name_forms_conflict() { + let error = Cli::try_parse_from([ + "clickhousectl", + "local", + "server", + "remove", + "existing", + "--name", + "other", + ]) + .err() + .expect("name forms should conflict"); + assert_eq!(error.kind(), clap::error::ErrorKind::ArgumentConflict); + assert!(error.to_string().contains("cannot be used with")); } #[test] @@ -769,15 +890,15 @@ mod tests { else { panic!("expected server stop"); }; - assert_eq!(name, "analytics"); + assert_eq!(name.as_deref(), Some("analytics")); let LocalCommands::Server { - command: ServerCommands::Remove { name }, + command: ServerCommands::Remove { name, .. }, } = local_command(&["server", "remove", "analytics"]) else { panic!("expected server remove"); }; - assert_eq!(name, "analytics"); + assert_eq!(name.as_deref(), Some("analytics")); let LocalCommands::Postgres { command: PostgresCommands::Stop { name, .. }, diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index dccae726..5e07f2d5 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -714,9 +714,11 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> } ServerCommands::Stop { name, + name_flag, global, project, } => { + let name = name.or(name_flag).unwrap_or_else(|| "default".to_string()); if global { stop_server_global(&name, project.as_deref(), json) } else { @@ -772,7 +774,8 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> password, database, } => dotenv_server(name.as_deref(), local, user, password, database, json), - ServerCommands::Remove { name } => { + ServerCommands::Remove { name, name_flag } => { + let name = name.or(name_flag).unwrap_or_else(|| "default".to_string()); server::validate_server_name(&name)?; // Recover orphaned servers so we correctly detect a running From 41af11c9f32e85afaf6cd1f7de8886ba68d9da92 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 13:19:11 +0100 Subject: [PATCH 09/12] Test local teardown name forms end to end --- .../tests/local_server_stopped_test.rs | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/crates/clickhousectl/tests/local_server_stopped_test.rs b/crates/clickhousectl/tests/local_server_stopped_test.rs index c91e3bed..21cdc7f6 100644 --- a/crates/clickhousectl/tests/local_server_stopped_test.rs +++ b/crates/clickhousectl/tests/local_server_stopped_test.rs @@ -31,6 +31,11 @@ fn write_server_metadata(project: &Path, pid: u32) -> PathBuf { metadata } +fn create_stopped_server(project: &Path, name: &str) { + std::fs::create_dir_all(project.join(".clickhouse/servers").join(name).join("data")) + .expect("create stopped server data dir"); +} + fn run(project: &Path, home: &Path, args: &[&str]) -> Output { Command::new(clickhousectl_binary()) .env("DO_NOT_TRACK", "1") @@ -97,6 +102,80 @@ impl Drop for ProcessGuard { } } +#[test] +fn stop_and_remove_accept_name_forms_omission_and_trailing_json() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let cases = [ + ( + "stop-positional", + ["local", "server", "stop", "stop-positional", "--json"].as_slice(), + ), + ( + "stop-flag", + ["local", "server", "stop", "--name", "stop-flag", "--json"].as_slice(), + ), + ("default", ["local", "server", "stop", "--json"].as_slice()), + ( + "remove-positional", + ["local", "server", "remove", "remove-positional", "--json"].as_slice(), + ), + ( + "remove-flag", + [ + "local", + "server", + "remove", + "--name", + "remove-flag", + "--json", + ] + .as_slice(), + ), + ( + "default", + ["local", "server", "remove", "--json"].as_slice(), + ), + ]; + + for (name, args) in cases { + create_stopped_server(project.path(), name); + let output = run(project.path(), home.path(), args); + assert!( + output.status.success(), + "args: {args:?}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let body: Value = serde_json::from_slice(&output.stdout).expect("parse command JSON"); + assert_eq!(body["name"], name, "args: {args:?}"); + } +} + +#[test] +fn stop_and_remove_reject_positional_and_name_flag_together() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + + for command in ["stop", "remove"] { + let output = run( + project.path(), + home.path(), + &[ + "local", + "server", + command, + "positional", + "--name", + "flagged", + ], + ); + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("cannot be used with"), "{stderr}"); + assert!(stderr.contains("--name "), "{stderr}"); + } +} + #[test] fn stopping_clickhouse_retains_metadata_and_lists_it_as_stopped() { let project = tempfile::tempdir().expect("create project tempdir"); From ed222df0b5994fb7d4b24fb41aaa135e21525b43 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:03:56 +0100 Subject: [PATCH 10/12] Validate local client selectors --- crates/clickhousectl/src/local/cli.rs | 140 ++++++++++++++++-- .../tests/local_client_selectors_test.rs | 110 ++++++++++++++ 2 files changed, 241 insertions(+), 9 deletions(-) create mode 100644 crates/clickhousectl/tests/local_client_selectors_test.rs diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index b5a14210..4a9f7565 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -98,14 +98,15 @@ CONTEXT FOR AGENTS: Two connection modes: 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. + 2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults + to localhost (for example, `local client --port 9000`); a missing port defaults to 9000. + Named and direct selectors cannot be combined. --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) @@ -113,7 +114,7 @@ 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..))] port: Option, /// Execute a SQL query @@ -422,14 +423,16 @@ CONTEXT FOR AGENTS: 1. Named server: `clickhousectl local postgres client --name dev` — looks up the host port 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`. + 2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults + to 127.0.0.1; a missing port defaults to 5432. + Named and direct selectors cannot be combined. 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 @@ -441,7 +444,7 @@ 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..))] port: Option, /// Execute a single SQL query @@ -485,13 +488,42 @@ mod tests { use clap::Parser; fn local_command(args: &[&str]) -> LocalCommands { + try_local_command(args).unwrap() + } + + fn try_local_command(args: &[&str]) -> Result { let mut argv = vec!["clickhousectl", "local"]; argv.extend_from_slice(args); - let cli = Cli::try_parse_from(argv).unwrap(); + let cli = Cli::try_parse_from(argv)?; let Commands::Local(local) = cli.command else { panic!("expected local command"); }; - local.command + Ok(local.command) + } + + fn client_selectors( + postgres: bool, + selectors: &[&str], + ) -> (Option, Option, Option) { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend_from_slice(selectors); + + match local_command(&args) { + LocalCommands::Client { + name, host, port, .. + } + | LocalCommands::Postgres { + command: + PostgresCommands::Client { + name, host, port, .. + }, + } => (name, host, port), + _ => panic!("expected local client command"), + } } #[test] @@ -508,6 +540,96 @@ mod tests { assert!(help.contains("`clickhouse format`"), "{help}"); } + #[test] + fn client_selector_matrix_accepts_unambiguous_modes_and_port_bounds() { + let cases = [ + (&[][..], None, None, None), + (&["--name", "dev"][..], Some("dev"), None, None), + ( + &["--host", "db.example"][..], + None, + Some("db.example"), + None, + ), + (&["--port", "1"][..], None, None, Some(1)), + (&["--port", "65535"][..], None, None, Some(65535)), + ( + &["--host", "db.example", "--port", "9440"][..], + None, + Some("db.example"), + Some(9440), + ), + ( + &["--port", "9440", "--host", "db.example"][..], + None, + Some("db.example"), + Some(9440), + ), + ]; + + for postgres in [false, true] { + for (selectors, expected_name, expected_host, expected_port) in cases { + let (name, host, port) = client_selectors(postgres, 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 client_selector_matrix_rejects_named_and_direct_modes_in_either_order() { + let cases = [ + &["--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"][..], + &["--host", "db.example", "--port", "9000", "--name", "dev"][..], + ]; + + for postgres in [false, true] { + for selectors in cases { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend_from_slice(selectors); + let error = try_local_command(&args) + .err() + .expect("selectors should conflict"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ArgumentConflict, + "selectors: {selectors:?}" + ); + } + } + } + + #[test] + fn client_ports_reject_zero_and_nonnumeric_values() { + for postgres in [false, true] { + for port in ["0", "not-a-port"] { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend(["--port", port]); + let error = try_local_command(&args) + .err() + .expect("port should be invalid"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ValueValidation, + "port: {port}" + ); + } + } + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) 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..245d4015 --- /dev/null +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -0,0 +1,110 @@ +//! End-to-end coverage for local client selector validation (issue #466). + +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 install_fake_clickhouse(home: &Path) { + let binary = home + .join(".clickhouse/versions") + .join(VERSION) + .join("clickhouse"); + std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version dir"); + std::fs::write(&binary, b"#!/bin/sh\nprintf '%s\\n' \"$@\"\n").expect("write fake ClickHouse"); + let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(binary, permissions).expect("make fake ClickHouse executable"); + std::fs::write(home.join(".clickhouse/default"), VERSION).expect("write default version"); +} + +fn run(project: &Path, home: &Path, args: &[&str]) -> Output { + Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .current_dir(project) + .args(args) + .output() + .expect("run clickhousectl") +} + +#[test] +fn clickhouse_direct_client_defaults_missing_host_or_port() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path()); + + let cases = [ + ( + &["local", "client", "--host", "db.example"][..], + &["client", "--host", "db.example", "--port", "9000"][..], + ), + ( + &["local", "client", "--port", "1"][..], + &["client", "--host", "localhost", "--port", "1"][..], + ), + ( + &["local", "client", "--port", "65535"][..], + &["client", "--host", "localhost", "--port", "65535"][..], + ), + ]; + + for (args, expected) in cases { + let output = run(project.path(), home.path(), args); + assert!( + output.status.success(), + "args: {args:?}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let forwarded: Vec<_> = String::from_utf8(output.stdout) + .expect("fake ClickHouse output should be UTF-8") + .lines() + .map(str::to_string) + .collect(); + assert_eq!(forwarded, expected, "args: {args:?}"); + } +} + +#[test] +fn invalid_client_selectors_are_usage_errors_before_resolution() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let cases = [ + &["local", "client", "--name", "dev", "--host", "db.example"][..], + &["local", "client", "--port", "9000", "--name", "dev"][..], + &["local", "client", "--port", "0"][..], + &[ + "local", + "postgres", + "client", + "--host", + "db.example", + "--name", + "dev", + ][..], + &[ + "local", "postgres", "client", "--name", "dev", "--port", "5432", + ][..], + &["local", "postgres", "client", "--port", "0"][..], + ]; + + for args in cases { + let output = run(project.path(), home.path(), args); + assert_eq!(output.status.code(), Some(2), "args: {args:?}"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("error:"), + "args: {args:?}\nstderr: {stderr}" + ); + assert!( + !stderr.contains("No default version configured") + && !stderr.contains("Server 'dev' not found"), + "args: {args:?}\nstderr: {stderr}" + ); + } +} From ffaf4cb131295eacee96ee67b9cfd3193547a34a Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 16:42:56 +0100 Subject: [PATCH 11/12] Allow direct client binary selection --- README.md | 5 + crates/clickhousectl/src/error.rs | 15 + crates/clickhousectl/src/local/cli.rs | 113 ++++++- crates/clickhousectl/src/local/mod.rs | 24 +- .../tests/local_client_selectors_test.rs | 281 +++++++++++++++++- 5 files changed, 421 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index df5e08ff..cc6a7a29 100644 --- a/README.md +++ b/README.md @@ -224,8 +224,13 @@ 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 --version 26.8.1.1760 # Use an exact installed client binary ``` +Named connections and direct connections select the client binary differently. A named connection uses the version recorded in the managed server's metadata, regardless of the global default. A direct connection (`--host`, `--port`, or both) uses the exact installed version passed with `--version`; if that flag is omitted, it uses the version selected by `local use`. + +Direct connections never infer a binary from the contents of `~/.clickhouse/versions`. With no valid default, omitting `--version` fails whether zero, one, or multiple versions are installed. Use `local list` to find an exact installed version, then pass it with `--version` or select it globally with `local use`. A missing explicit version and a default that points to a removed version both fail with repair instructions. Direct `--version` selection neither installs a binary nor changes the default. + ### Creating and managing ClickHouse servers Start and manage ClickHouse server instances. Each server gets its own isolated data directory at `.clickhouse/servers//data/`. diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index 10df5f1b..8ab93232 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -22,6 +22,21 @@ pub enum Error { #[error("No default version set. Run: clickhousectl local use ")] NoDefaultVersion, + #[error( + "No ClickHouse client version selected for the direct connection. Pass `--version `, or set a default with `clickhousectl local use ` (see `clickhousectl local list`)." + )] + DirectClientVersionRequired, + + #[error( + "ClickHouse client version {0} is not installed. Run `clickhousectl local install {0}`, or choose an exact version from `clickhousectl local list`." + )] + ClientVersionNotInstalled(String), + + #[error( + "Default ClickHouse version {0} is not installed. Repair it with `clickhousectl local use `, or pass `--version ` for this direct connection." + )] + StaleClientDefault(String), + #[error("Version {0} is already installed")] VersionAlreadyInstalled(String), diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 4a9f7565..af365d15 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -41,7 +41,8 @@ CONTEXT FOR AGENTS: /// Set the default version #[command(after_help = "\ CONTEXT FOR AGENTS: - Sets the default ClickHouse version used by `clickhousectl local client` and `clickhousectl local server`. + Sets the default ClickHouse version used by direct `clickhousectl local client` connections + when --version is omitted, and by `clickhousectl local server`. Accepts version specs: \"latest\" (recommended), \"stable\", \"lts\", partial like \"25.12\", or exact like \"25.12.5.44\". Auto-installs the version if not already present. Also creates `~/.local/bin/clickhouse` as a symlink to the version's binary so the `clickhouse` command is on PATH. Pass --no-global to skip. @@ -93,28 +94,44 @@ CONTEXT FOR AGENTS: Init, /// Connect to a running ClickHouse server with clickhouse-client - #[command(after_help = "\ + #[command( + group(clap::ArgGroup::new("direct").args(["host", "port"]).multiple(true)), + after_help = "\ CONTEXT FOR AGENTS: - Two connection modes: + Connection and local binary selection are separate: 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. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults to localhost (for example, `local client --port 9000`); a missing port defaults to 9000. - Named and direct selectors cannot be combined. + Pass --version with a direct connection to select an exact installed client binary without + changing the default. Otherwise direct mode uses the configured default; it never guesses + from installed versions. Use `local list` to find exact installed versions. + Named mode always uses the managed server's recorded version. --version is direct-only, and + named and direct selectors cannot be combined. --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.")] + 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, conflicts_with_all = ["host", "port"])] + #[arg(long, short, conflicts_with_all = ["host", "port", "version"])] name: Option, + /// Exact installed ClickHouse version for a direct connection; does not change the default + #[arg(long, short = 'v', requires = "direct")] + version: Option, + /// Host to connect to (bypasses local server lookup) - #[arg(long)] + #[arg(long, group = "direct")] host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))] + #[arg( + long, + short, + value_parser = clap::value_parser!(u16).range(1..), + group = "direct" + )] port: Option, /// Execute a SQL query @@ -630,6 +647,86 @@ mod tests { } } + #[test] + fn clickhouse_direct_client_version_selector_matrix() { + let cases = [ + ( + &["--host", "db.example", "--version", "25.12.9.61"][..], + "25.12.9.61", + ), + (&["--port", "9000", "-v", "26.1.2.3"][..], "26.1.2.3"), + ( + &[ + "--version", + "26.2.3.4", + "--host", + "db.example", + "--port", + "9440", + ][..], + "26.2.3.4", + ), + ]; + + for (selectors, expected_version) in cases { + let mut args = vec!["client"]; + args.extend_from_slice(selectors); + let LocalCommands::Client { version, .. } = local_command(&args) else { + panic!("expected local client command"); + }; + assert_eq!( + version.as_deref(), + Some(expected_version), + "selectors: {selectors:?}" + ); + } + } + + #[test] + fn clickhouse_client_version_requires_direct_connection() { + for selectors in [ + &["--version", "25.12.9.61"][..], + &["--name", "dev", "--version", "25.12.9.61"][..], + &["--version", "25.12.9.61", "--name", "dev"][..], + ] { + let mut args = vec!["client"]; + args.extend_from_slice(selectors); + let error = try_local_command(&args) + .err() + .expect("binary version should require direct mode"); + assert!( + matches!( + error.kind(), + clap::error::ErrorKind::MissingRequiredArgument + | clap::error::ErrorKind::ArgumentConflict + ), + "selectors: {selectors:?}\n{error}" + ); + } + } + + #[test] + fn clickhouse_client_help_separates_connection_and_binary_selection() { + let help = Cli::try_parse_from(["clickhousectl", "local", "client", "--help"]) + .err() + .expect("help should exit through clap") + .to_string(); + + assert!( + help.contains("Connection and local binary selection are separate"), + "{help}" + ); + assert!( + help.contains("Exact installed ClickHouse version"), + "{help}" + ); + assert!(help.contains("does not change the default"), "{help}"); + assert!( + help.contains("Named mode always uses the managed server's recorded version"), + "{help}" + ); + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 5e07f2d5..7d1cefc3 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -38,12 +38,13 @@ pub async fn run(cmd: LocalCommands, json: bool) -> Result<()> { } LocalCommands::Client { name, + version, host, port, query, queries_file, args, - } => run_client(name, host, port, query, queries_file, args), + } => run_client(name, version, host, port, query, queries_file, args), LocalCommands::Server { command } => run_server_commands(command, json).await, LocalCommands::Postgres { command } => postgres::run(command, json).await, } @@ -253,6 +254,7 @@ fn which(json: bool) -> Result<()> { fn run_client( name: Option, + version: Option, host: Option, port: Option, query: Option, @@ -264,7 +266,7 @@ fn run_client( let (resolved_host, tcp_port, version) = if host.is_some() || port.is_some() { let h = host.unwrap_or_else(|| "localhost".to_string()); let p = port.unwrap_or(9000); - let v = version_manager::get_default_version()?; + let v = resolve_direct_client_version(version.as_deref())?; (h, p, v) } else { let server_name = name.as_deref().unwrap_or("default"); @@ -313,6 +315,24 @@ fn run_client( Err(Error::Exec(err.to_string())) } +fn resolve_direct_client_version(version: Option<&str>) -> Result { + if let Some(version) = version { + let installed = version_manager::list_installed_versions()?; + return installed + .iter() + .any(|installed| installed == version) + .then(|| version.to_string()) + .ok_or_else(|| Error::ClientVersionNotInstalled(version.to_string())); + } + + match version_manager::get_default_version() { + Ok(version) => Ok(version), + Err(Error::NoDefaultVersion) => Err(Error::DirectClientVersionRequired), + Err(Error::VersionNotFound(version)) => Err(Error::StaleClientDefault(version)), + Err(error) => Err(error), + } +} + #[allow(clippy::too_many_arguments)] async fn start_server( name: Option, diff --git a/crates/clickhousectl/tests/local_client_selectors_test.rs b/crates/clickhousectl/tests/local_client_selectors_test.rs index 245d4015..43a1d979 100644 --- a/crates/clickhousectl/tests/local_client_selectors_test.rs +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -1,26 +1,58 @@ -//! End-to-end coverage for local client selector validation (issue #466). +//! End-to-end coverage for local client selectors (issues #466 and #469). +use serde_json::json; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::process::{Command, Output}; -const VERSION: &str = "25.12.9.61"; +const VERSION_A: &str = "25.12.9.61"; +const VERSION_B: &str = "26.1.2.3"; +const MISSING_VERSION: &str = "24.8.99.1"; fn clickhousectl_binary() -> PathBuf { PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) } -fn install_fake_clickhouse(home: &Path) { +fn install_fake_clickhouse(home: &Path, version: &str) { let binary = home .join(".clickhouse/versions") - .join(VERSION) + .join(version) .join("clickhouse"); std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version dir"); - std::fs::write(&binary, b"#!/bin/sh\nprintf '%s\\n' \"$@\"\n").expect("write fake ClickHouse"); + std::fs::write( + &binary, + format!("#!/bin/sh\nprintf 'binary={version}\\n'\nprintf '%s\\n' \"$@\"\n"), + ) + .expect("write fake ClickHouse"); let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); permissions.set_mode(0o755); std::fs::set_permissions(binary, permissions).expect("make fake ClickHouse executable"); - std::fs::write(home.join(".clickhouse/default"), VERSION).expect("write default version"); +} + +fn set_default(home: &Path, version: &str) { + let base = home.join(".clickhouse"); + std::fs::create_dir_all(&base).expect("create ClickHouse home"); + std::fs::write(base.join("default"), version).expect("write default version"); +} + +fn write_server_metadata(project: &Path, name: &str, version: &str, tcp_port: u16) { + let servers = project.join(".clickhouse/servers"); + std::fs::create_dir_all(&servers).expect("create servers dir"); + std::fs::write( + servers.join(format!("{name}.json")), + serde_json::to_vec_pretty(&json!({ + "name": name, + "pid": std::process::id(), + "version": version, + "http_port": 8123, + "tcp_port": tcp_port, + "started_at": "1700000000", + "cwd": project.display().to_string(), + "engine": "clickhouse" + })) + .unwrap(), + ) + .expect("write server metadata"); } fn run(project: &Path, home: &Path, args: &[&str]) -> Output { @@ -33,11 +65,28 @@ fn run(project: &Path, home: &Path, args: &[&str]) -> Output { .expect("run clickhousectl") } +fn assert_client(output: Output, version: &str, expected_args: &[&str]) { + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let lines: Vec<_> = String::from_utf8(output.stdout) + .expect("fake ClickHouse output should be UTF-8") + .lines() + .map(str::to_string) + .collect(); + let mut expected = vec![format!("binary={version}")]; + expected.extend(expected_args.iter().map(|arg| (*arg).to_string())); + assert_eq!(lines, expected); +} + #[test] fn clickhouse_direct_client_defaults_missing_host_or_port() { let project = tempfile::tempdir().expect("create project tempdir"); let home = tempfile::tempdir().expect("create home tempdir"); - install_fake_clickhouse(home.path()); + install_fake_clickhouse(home.path(), VERSION_A); + set_default(home.path(), VERSION_A); let cases = [ ( @@ -64,12 +113,227 @@ fn clickhouse_direct_client_defaults_missing_host_or_port() { let forwarded: Vec<_> = String::from_utf8(output.stdout) .expect("fake ClickHouse output should be UTF-8") .lines() + .skip(1) .map(str::to_string) .collect(); assert_eq!(forwarded, expected, "args: {args:?}"); } } +#[test] +fn direct_client_without_explicit_version_uses_only_a_valid_default() { + struct Case { + installed: &'static [&'static str], + default: Option<&'static str>, + expected_binary: Option<&'static str>, + expected_error: Option<&'static str>, + } + + let cases = [ + Case { + installed: &[], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A], + default: Some(VERSION_A), + expected_binary: Some(VERSION_A), + expected_error: None, + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(VERSION_B), + expected_binary: Some(VERSION_B), + expected_error: None, + }, + Case { + installed: &[VERSION_A], + default: Some(MISSING_VERSION), + expected_binary: None, + expected_error: Some("Default ClickHouse version 24.8.99.1 is not installed"), + }, + ]; + + for case in cases { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + for version in case.installed { + install_fake_clickhouse(home.path(), version); + } + if let Some(version) = case.default { + set_default(home.path(), version); + } + + let output = run( + project.path(), + home.path(), + &["local", "client", "--host", "db.example"], + ); + if let Some(version) = case.expected_binary { + assert_client( + output, + version, + &["client", "--host", "db.example", "--port", "9000"], + ); + } else { + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains(case.expected_error.unwrap()), + "installed: {:?}, default: {:?}\nstderr: {stderr}", + case.installed, + case.default + ); + assert!( + stderr.contains("clickhousectl local use") + || stderr.contains("clickhousectl local list"), + "stderr should be actionable: {stderr}" + ); + } + } +} + +#[test] +fn direct_client_explicit_version_matrix_is_installed_only_and_preserves_default() { + struct Case { + installed: &'static [&'static str], + default: Option<&'static str>, + requested: &'static str, + expected_binary: Option<&'static str>, + } + + let cases = [ + Case { + installed: &[], + default: None, + requested: VERSION_A, + expected_binary: None, + }, + Case { + installed: &[VERSION_A], + default: None, + requested: VERSION_A, + expected_binary: Some(VERSION_A), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(VERSION_A), + requested: VERSION_B, + expected_binary: Some(VERSION_B), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(MISSING_VERSION), + requested: VERSION_B, + expected_binary: Some(VERSION_B), + }, + Case { + installed: &[VERSION_A], + default: Some(VERSION_A), + requested: VERSION_B, + expected_binary: None, + }, + ]; + + for case in cases { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + for version in case.installed { + install_fake_clickhouse(home.path(), version); + } + if let Some(version) = case.default { + set_default(home.path(), version); + } + + let output = run( + project.path(), + home.path(), + &[ + "local", + "client", + "--port", + "9440", + "--version", + case.requested, + ], + ); + if let Some(version) = case.expected_binary { + assert_client( + output, + version, + &["client", "--host", "localhost", "--port", "9440"], + ); + } else { + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains(&format!( + "ClickHouse client version {} is not installed", + case.requested + )), + "stderr: {stderr}" + ); + assert!( + stderr.contains(&format!("clickhousectl local install {}", case.requested)) + && stderr.contains("clickhousectl local list"), + "stderr should be actionable: {stderr}" + ); + } + + let default_file = home.path().join(".clickhouse/default"); + match case.default { + Some(expected) => assert_eq!( + std::fs::read_to_string(default_file).unwrap(), + expected, + "explicit selection must preserve the default" + ), + None => assert!( + !default_file.exists(), + "explicit selection must not create a default" + ), + } + } +} + +#[test] +fn named_client_uses_recorded_version_even_with_a_stale_default() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path(), VERSION_A); + install_fake_clickhouse(home.path(), VERSION_B); + set_default(home.path(), MISSING_VERSION); + write_server_metadata(project.path(), "dev", VERSION_B, 9440); + + let output = run( + project.path(), + home.path(), + &["local", "client", "--name", "dev"], + ); + assert_client( + output, + VERSION_B, + &["client", "--host", "localhost", "--port", "9440"], + ); + assert_eq!( + std::fs::read_to_string(home.path().join(".clickhouse/default")).unwrap(), + MISSING_VERSION + ); +} + #[test] fn invalid_client_selectors_are_usage_errors_before_resolution() { let project = tempfile::tempdir().expect("create project tempdir"); @@ -78,6 +342,9 @@ fn invalid_client_selectors_are_usage_errors_before_resolution() { &["local", "client", "--name", "dev", "--host", "db.example"][..], &["local", "client", "--port", "9000", "--name", "dev"][..], &["local", "client", "--port", "0"][..], + &["local", "client", "--version", VERSION_A][..], + &["local", "client", "--name", "dev", "--version", VERSION_A][..], + &["local", "client", "--version", VERSION_A, "--name", "dev"][..], &[ "local", "postgres", From 67746531a01c0888deb42d008978318b08236d52 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 19:16:36 +0100 Subject: [PATCH 12/12] Preserve local client query multiplicity --- README.md | 5 +- crates/clickhousectl/src/local/cli.rs | 129 +++++++++++++++- crates/clickhousectl/src/local/mod.rs | 12 +- .../tests/local_client_selectors_test.rs | 143 +++++++++++++++++- 4 files changed, 274 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index cc6a7a29..534ec6f6 100644 --- a/README.md +++ b/README.md @@ -222,11 +222,14 @@ postgres/ clickhousectl local client # Connects to "default" server 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 --query "SELECT 1" --query "SELECT 2" # Run queries in order +clickhousectl local client --queries-file schema.sql seed.sql # Run query files in order clickhousectl local client --host remote-host --port 9000 # Connect to a specific host/port clickhousectl local client --host remote-host --version 26.8.1.1760 # Use an exact installed client binary ``` +`--query` can be repeated. `--queries-file` accepts multiple paths after one flag and can also be repeated. Their values are forwarded in order. The native client does not allow inline queries and query files in the same invocation, so `local client` rejects that combination as a usage error instead of reordering it. Arguments after `--` are forwarded after these generated query arguments. + Named connections and direct connections select the client binary differently. A named connection uses the version recorded in the managed server's metadata, regardless of the global default. A direct connection (`--host`, `--port`, or both) uses the exact installed version passed with `--version`; if that flag is omitted, it uses the version selected by `local use`. Direct connections never infer a binary from the contents of `~/.clickhouse/versions`. With no valid default, omitting `--version` fails whether zero, one, or multiple versions are installed. Use `local list` to find an exact installed version, then pass it with `--version` or select it globally with `local use`. A missing explicit version and a default that points to a removed version both fail with repair instructions. Direct `--version` selection neither installs a binary nor changes the default. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index af365d15..a61693dd 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -108,7 +108,9 @@ CONTEXT FOR AGENTS: from installed versions. Use `local list` to find exact installed versions. Named mode always uses the managed server's recorded version. --version is direct-only, and named and direct selectors cannot be combined. - --query and --queries-file execute SQL inline or from a file. + Repeat --query to execute multiple inline queries. --queries-file accepts multiple paths after + one flag and can also be repeated. Inline queries and query files cannot be combined, matching + the native client. 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." )] @@ -134,13 +136,13 @@ CONTEXT FOR AGENTS: )] port: Option, - /// Execute a SQL query - #[arg(long, short)] - query: Option, + /// Execute a SQL query; repeat for multiple queries + #[arg(long, short, conflicts_with = "queries_file")] + query: Vec, - /// Execute queries from a SQL file - #[arg(long)] - queries_file: Option, + /// Execute queries from SQL files; accepts multiple paths or repeated flags + #[arg(long, num_args = 1.., conflicts_with = "query")] + queries_file: Vec, /// Additional arguments to pass to clickhouse-client #[arg(trailing_var_arg = true, allow_hyphen_values = true)] @@ -727,6 +729,119 @@ mod tests { ); } + #[test] + fn clickhouse_client_queries_preserve_empty_values_repeats_and_order() { + let LocalCommands::Client { + query, + queries_file, + args, + .. + } = local_command(&["client"]) + else { + panic!("expected local client command"); + }; + assert!(query.is_empty()); + assert!(queries_file.is_empty()); + assert!(args.is_empty()); + + let LocalCommands::Client { + query, + queries_file, + args, + .. + } = local_command(&[ + "client", + "--query", + "SELECT 1", + "-q", + "", + "--query", + "SELECT 3", + "--", + "--format", + "JSONEachRow", + ]) + else { + panic!("expected local client command"); + }; + assert_eq!(query, ["SELECT 1", "", "SELECT 3"]); + assert!(queries_file.is_empty()); + assert_eq!(args, ["--format", "JSONEachRow"]); + } + + #[test] + fn clickhouse_client_query_files_preserve_empty_values_repeats_and_order() { + let LocalCommands::Client { + query, + queries_file, + args, + .. + } = local_command(&[ + "client", + "--queries-file", + "schema.sql", + "seed.sql", + "--queries-file", + "", + "verify.sql", + "--", + "--echo", + ]) + else { + panic!("expected local client command"); + }; + assert!(query.is_empty()); + assert_eq!(queries_file, ["schema.sql", "seed.sql", "", "verify.sql"]); + assert_eq!(args, ["--echo"]); + } + + #[test] + fn clickhouse_client_rejects_combined_query_sources_in_either_order() { + for args in [ + &[ + "client", + "--query", + "SELECT 1", + "--queries-file", + "queries.sql", + ][..], + &[ + "client", + "--queries-file", + "queries.sql", + "--query", + "SELECT 1", + ][..], + ] { + let error = try_local_command(args) + .err() + .expect("query sources should conflict"); + assert_eq!(error.kind(), clap::error::ErrorKind::ArgumentConflict); + let message = error.to_string(); + assert!(message.contains("--query"), "{message}"); + assert!(message.contains("--queries-file"), "{message}"); + assert!(message.contains("cannot be used with"), "{message}"); + } + } + + #[test] + fn clickhouse_client_query_help_documents_native_multiplicity_and_exclusion() { + let help = Cli::try_parse_from(["clickhousectl", "local", "client", "--help"]) + .err() + .expect("help should exit through clap") + .to_string(); + + assert!(help.contains("repeat for multiple queries"), "{help}"); + assert!( + help.contains("accepts multiple paths or repeated flags"), + "{help}" + ); + assert!( + help.contains("Inline queries and query files cannot be combined"), + "{help}" + ); + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 7d1cefc3..f67c93a3 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -257,8 +257,8 @@ fn run_client( version: Option, host: Option, port: Option, - query: Option, - queries_file: Option, + query: Vec, + queries_file: Vec, args: Vec, ) -> Result<()> { // If --host or --port is set, connect directly (bypass local server lookup). @@ -298,12 +298,12 @@ fn run_client( .arg("--port") .arg(tcp_port.to_string()); - if let Some(q) = &query { - cmd.arg("--query").arg(q); + for query in query { + cmd.arg("--query").arg(query); } - if let Some(f) = &queries_file { - cmd.arg("--queries-file").arg(f); + if !queries_file.is_empty() { + cmd.arg("--queries-file").args(queries_file); } cmd.args(&args); diff --git a/crates/clickhousectl/tests/local_client_selectors_test.rs b/crates/clickhousectl/tests/local_client_selectors_test.rs index 43a1d979..5f7be35c 100644 --- a/crates/clickhousectl/tests/local_client_selectors_test.rs +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -1,4 +1,4 @@ -//! End-to-end coverage for local client selectors (issues #466 and #469). +//! End-to-end coverage for local client selectors and query inputs (issues #466, #469, and #470). use serde_json::json; use std::os::unix::fs::PermissionsExt; @@ -81,6 +81,147 @@ fn assert_client(output: Output, version: &str, expected_args: &[&str]) { assert_eq!(lines, expected); } +#[test] +fn clickhouse_client_preserves_native_query_argv_across_supported_versions() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path(), VERSION_A); + install_fake_clickhouse(home.path(), VERSION_B); + + let cases = [ + ( + &[][..], + &["client", "--host", "db.example", "--port", "9000"][..], + ), + ( + &["--query", "SELECT 1"][..], + &[ + "client", + "--host", + "db.example", + "--port", + "9000", + "--query", + "SELECT 1", + ][..], + ), + ( + &[ + "--query", + "SELECT 1", + "-q", + "", + "--query", + "SELECT 3", + "--", + "--format", + "JSONEachRow", + ][..], + &[ + "client", + "--host", + "db.example", + "--port", + "9000", + "--query", + "SELECT 1", + "--query", + "", + "--query", + "SELECT 3", + "--format", + "JSONEachRow", + ][..], + ), + ( + &["--queries-file", "schema.sql"][..], + &[ + "client", + "--host", + "db.example", + "--port", + "9000", + "--queries-file", + "schema.sql", + ][..], + ), + ( + &[ + "--queries-file", + "schema.sql", + "seed.sql", + "--queries-file", + "", + "verify.sql", + "--", + "--echo", + ][..], + &[ + "client", + "--host", + "db.example", + "--port", + "9000", + "--queries-file", + "schema.sql", + "seed.sql", + "", + "verify.sql", + "--echo", + ][..], + ), + ]; + + for version in [VERSION_A, VERSION_B] { + for (input, expected) in cases { + let mut args = vec![ + "local", + "client", + "--host", + "db.example", + "--version", + version, + ]; + args.extend_from_slice(input); + assert_client(run(project.path(), home.path(), &args), version, expected); + } + } +} + +#[test] +fn combined_clickhouse_client_query_sources_are_usage_errors_before_exec() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let cases = [ + &[ + "local", + "client", + "--query", + "SELECT 1", + "--queries-file", + "queries.sql", + ][..], + &[ + "local", + "client", + "--queries-file", + "queries.sql", + "--query", + "SELECT 1", + ][..], + ]; + + for args in cases { + let output = run(project.path(), home.path(), args); + assert_eq!(output.status.code(), Some(2), "args: {args:?}"); + assert!(output.stdout.is_empty(), "child must not run: {args:?}"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("--query"), "stderr: {stderr}"); + assert!(stderr.contains("--queries-file"), "stderr: {stderr}"); + assert!(stderr.contains("cannot be used with"), "stderr: {stderr}"); + } +} + #[test] fn clickhouse_direct_client_defaults_missing_host_or_port() { let project = tempfile::tempdir().expect("create project tempdir");