diff --git a/.agents/skills/add-cli-command/references/cloud.md b/.agents/skills/add-cli-command/references/cloud.md index 4476fc4e..b8d726bd 100644 --- a/.agents/skills/add-cli-command/references/cloud.md +++ b/.agents/skills/add-cli-command/references/cloud.md @@ -5,8 +5,9 @@ its own PR (see `crates/clickhouse-cloud-api/AGENTS.md`). Everything for a command lives in its domain module, `src/cloud/.rs`: the clap definition, permissions, dispatch, the `CloudClient` wrapper, the request builder, the handler and the tests. Copy the pattern from -`src/cloud/services.rs`, but not from its query commands or from `src/cloud/postgres.rs`. Those call -`client.api()` directly. +`src/cloud/services.rs` or `src/cloud/postgres.rs`. Handlers never call `client.api()`. The one wrapper that +returns the raw library error is `run_service_query{,_bearer}` in `services.rs`: query paths stream the +response and classify the error themselves. Don't copy that for other endpoints. 1. **Definition.** Add the variant and its argument struct to the domain's command enum. For a new command group: @@ -41,6 +42,11 @@ dispatch, the `CloudClient` wrapper, the request builder, the handler and the te errors that need the org ID or lookup context, use `convert_error_for_organization(e, org_id)` or `convert_error_for_lookup(e, lookup)` instead. Use the library's types. These conversions also record the failure kind for telemetry, so don't build a `CloudError` by hand. + - `convert_error_for_lookup` reports any 400 or 404 over well-formed IDs as a missing resource. Use it only + when the path names the resource itself and the request has no other input, such as a get or delete. + - For a subresource, or a request with a body, a 404 can mean the subresource is absent and a 400 can be a + validation error. Confirm the parent with its own GET first, as `convert_postgres_by_id_result` in + `postgres.rs` does, and declare that GET as a `Conditional` permission. 6. **Request builder.** If the command sends a body, write `build__request(...)` returning the library's request struct. Unit-test it with minimal and maximal input, asserting on the struct's fields. 7. **Handler.** Call the builder, then the wrapper, then print: diff --git a/AGENTS.md b/AGENTS.md index 5a276fc4..aac8312b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,8 +39,9 @@ work on a branch, with an associated issue and a PR. - New Cloud handlers go through `CloudClient` wrapper methods co-located in each domain module, not `clickhouse_cloud_api::Client` directly. `src/cloud/client.rs` owns the core client, credential precedence, error conversion, and response unwrapping. -- Exception, do not copy: `src/cloud/postgres.rs` handlers and the query paths in `src/cloud/services.rs` still - call `client.api()` directly (`postgres.rs` also uses a local `unwrap_api` instead of `unwrap_response`). +- One deliberate exception to wrapper error conversion: `run_service_query{,_bearer}` in `src/cloud/services.rs` + return the raw library error and an unread `reqwest::Response`, because the query paths stream the body and + classify the error themselves before `convert_query_error`. Do not copy that shape for other endpoints. - Cloud handlers support `--json` unless there is good reason not to. JSON is emitted automatically when `--json` is passed or a coding agent is detected — `json_output()` in `main.rs` wraps `is_ai_agent::detect()`. - `CloudError.kind` decides the top-level error: `Auth` (401/403, missing credentials) → exit 4; `Usage` (input diff --git a/crates/clickhousectl/src/cloud/postgres.rs b/crates/clickhousectl/src/cloud/postgres.rs index de60888d..2bbbf6a4 100644 --- a/crates/clickhousectl/src/cloud/postgres.rs +++ b/crates/clickhousectl/src/cloud/postgres.rs @@ -1,6 +1,12 @@ use super::permissions::{Conditional, Declaration as Permission}; use clickhouse_cloud_api::meta::operations as op; +/// The GET that tells a missing service from a rejected request when a by-id +/// subresource or write fails with a 400 or 404 (see +/// `CloudClient::convert_postgres_by_id_result`). +const CONFIRM_POSTGRES_SERVICE: Conditional = + Conditional::new("If the ID is rejected", &[&op::POSTGRES_SERVICE_GET]); + // Declare every API call made by these workflows, including optional lookups. pub(super) const PERMISSIONS: &[Permission] = &[ Permission::api("postgres list", &[&op::POSTGRES_SERVICE_GET_LIST]), @@ -17,33 +23,46 @@ pub(super) const PERMISSIONS: &[Permission] = &[ &[&op::POSTGRES_SERVICE_GET, &op::POSTGRES_SERVICE_DELETE], ) .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), - Permission::api("postgres certs get", &[&op::POSTGRES_SERVICE_CERTS_GET]) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), - Permission::api("postgres config get", &[&op::POSTGRES_INSTANCE_CONFIG_GET]) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), + Permission::api("postgres certs get", &[&op::POSTGRES_SERVICE_CERTS_GET]).when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), + Permission::api("postgres config get", &[&op::POSTGRES_INSTANCE_CONFIG_GET]).when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), Permission::api( "postgres config replace", &[&op::POSTGRES_INSTANCE_CONFIG_POST], ) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), + .when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), Permission::api( "postgres config patch", &[&op::POSTGRES_INSTANCE_CONFIG_PATCH], ) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), + .when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), Permission::api( "postgres reset-password", &[&op::POSTGRES_SERVICE_SET_PASSWORD], ) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), + .when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), Permission::api( "postgres read-replica create", &[&op::POSTGRES_INSTANCE_CREATE_READ_REPLICA], ) - .when(&[Conditional::flag( - "source-name", - &[&op::POSTGRES_SERVICE_GET_LIST], - )]), + .when(&[ + Conditional::flag("source-name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), Permission::api("postgres metrics", &[&op::POSTGRES_INSTANCE_METRICS_GET]) .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), Permission::api( @@ -68,16 +87,24 @@ pub(super) const PERMISSIONS: &[Permission] = &[ Conditional::flag("source-name", &[&op::POSTGRES_SERVICE_GET_LIST]), Conditional::new("Reading access rules", &[&op::POSTGRES_SERVICE_GET]), Conditional::new("Applying access rules", &[&op::POSTGRES_SERVICE_PATCH]), + CONFIRM_POSTGRES_SERVICE, + ]), + Permission::api("postgres restart", &[&op::POSTGRES_SERVICE_PATCH_STATE]).when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), + Permission::api("postgres promote", &[&op::POSTGRES_SERVICE_PATCH_STATE]).when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, + ]), + Permission::api("postgres switchover", &[&op::POSTGRES_SERVICE_PATCH_STATE]).when(&[ + Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), + CONFIRM_POSTGRES_SERVICE, ]), - Permission::api("postgres restart", &[&op::POSTGRES_SERVICE_PATCH_STATE]) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), - Permission::api("postgres promote", &[&op::POSTGRES_SERVICE_PATCH_STATE]) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), - Permission::api("postgres switchover", &[&op::POSTGRES_SERVICE_PATCH_STATE]) - .when(&[Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST])]), Permission::api("postgres update", &[&op::POSTGRES_SERVICE_PATCH]).when(&[ Conditional::flag("name", &[&op::POSTGRES_SERVICE_GET_LIST]), Conditional::new("Without --clear-tags", &[&op::POSTGRES_SERVICE_GET]), + CONFIRM_POSTGRES_SERVICE, ]), ]; @@ -94,17 +121,19 @@ use clap::builder::TypedValueParser; use clap::{ArgGroup, Subcommand}; use clickhouse_cloud_api::RunPostgresQueryRequest; use clickhouse_cloud_api::models::{ - ApiResponse, PgBouncerConfig, PgConfig, PgConfigDefaultToastCompression, + PgBouncerConfig, PgConfig, PgConfigDefaultToastCompression, PgConfigDefaultTransactionIsolation, PgConfigEnableParallelHash, PgConfigIoMethod, PgConfigLogLockWaits, PgConfigSslMinProtocolVersion, PgConfigWalCompression, PgHaType, PgIdProperty, PgProvider, PgSize, PgVersion, PostgresBackup, PostgresInstanceConfig, + PostgresInstanceConfigResponse, PostgresInstanceUpdateConfigResponse, PostgresIpAccessListEntry, PostgresIpAccessListEntryResponse, PostgresIpAccessListPatch, PostgresIpAccessListRuleReference, PostgresLogEntry, PostgresLogsGetListSortorder, - PostgresMetrics, PostgresService, PostgresServiceListItem, PostgresServicePatchRequest, - PostgresServicePostRequest, PostgresServiceReadReplicaRequest, PostgresServiceRestoreRequest, - PostgresServiceSetPassword, PostgresServiceSetState, PostgresServiceSetStateCommand, - PostgresSlowQueryPattern, PostgresSlowQueryPatternDetail, ResourceTagsV1, - ResourceTagsV1Response, SlowQueryPatternsGetListSortby, SlowQueryPatternsGetListSortorder, + PostgresMetrics, PostgresService, PostgresServiceListItem, PostgresServicePasswordResource, + PostgresServicePatchRequest, PostgresServicePostRequest, PostgresServiceReadReplicaRequest, + PostgresServiceRestoreRequest, PostgresServiceSetPassword, PostgresServiceSetState, + PostgresServiceSetStateCommand, PostgresSlowQueryPattern, PostgresSlowQueryPatternDetail, + ResourceTagsV1, ResourceTagsV1Response, SlowQueryPatternsGetListSortby, + SlowQueryPatternsGetListSortorder, }; use futures_util::StreamExt; use serde::de::DeserializeOwned; @@ -1000,13 +1029,6 @@ pub async fn run(client: &CloudClient, command: PostgresCommands, json: bool) -> // Helpers // --------------------------------------------------------------------------- -/// Every Postgres path reports an empty body the same way the rest of the -/// cloud surface does, so moving one onto a `CloudClient` wrapper cannot -/// change the message a caller sees. -fn unwrap_api(resp: ApiResponse) -> CloudResult { - CloudClient::unwrap_response(resp) -} - fn parse_pg_size(value: &str) -> CloudResult { serde_json::from_value(serde_json::Value::String(value.to_string())) .map_err(|e| CloudError::new(format!("invalid size '{}': {}", value, e))) @@ -2044,12 +2066,7 @@ pub async fn postgres_list( json: bool, ) -> CloudResult<()> { let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_service_get_list(&org_id) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let items = unwrap_api(resp)?; + let items = client.list_postgres_services(&org_id).await?; let filtered: Vec = items .into_iter() .filter(|i| apply_filter(i, filters)) @@ -2279,12 +2296,7 @@ pub async fn postgres_create( let req = build_postgres_create_request(&opts)?; let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_service_create(&org_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let svc = unwrap_api(resp)?; + let svc = client.create_postgres_service(&org_id, &req).await?; if json { println!("{}", serde_json::to_string_pretty(&svc)?); @@ -2353,12 +2365,7 @@ pub async fn postgres_update( let tags = if opts.clear_tags { Some(Vec::new()) } else { - let current = client - .api() - .postgres_service_get(&org_id, postgres_id) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let current = unwrap_api(current)?; + let current = client.get_postgres_service(&org_id, postgres_id).await?; let add = parse_tags(opts.add_tag)?.unwrap_or_default(); Some(merge_response_tags(current.tags, &add, opts.remove_tag)?) }; @@ -2367,12 +2374,9 @@ pub async fn postgres_update( build_postgres_ip_access_list_patch(opts.add_ip_allow, opts.remove_ip_allow); let req = build_postgres_update_request(opts.name, size, ha_type, tags, ip_access_list); - let resp = client - .api() - .postgres_service_patch(&org_id, postgres_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let svc = unwrap_api(resp)?; + let svc = client + .patch_postgres_service(&org_id, postgres_id, &req) + .await?; if json { println!("{}", serde_json::to_string_pretty(&svc)?); @@ -2398,16 +2402,7 @@ pub async fn postgres_delete( // the resource object rather than `{"status":...,"requestId":...}` (#614). let svc = client.get_postgres_service(&org_id, postgres_id).await?; - client - .api() - .postgres_service_delete(&org_id, postgres_id) - .await - .map_err(|error| { - client.convert_error_for_lookup( - error, - ResourceLookup::in_org(ResourceKind::PostgresService, postgres_id, &org_id), - ) - })?; + client.delete_postgres_service(&org_id, postgres_id).await?; // The service is already gone by here, so a closed stdout must not turn a // completed deletion into a panic — see `print_line` and #598. @@ -2430,10 +2425,8 @@ pub async fn postgres_certs_get( ) -> CloudResult<()> { let org_id = resolve_org_id(client).await?; let pem = client - .api() - .postgres_service_certs_get(&org_id, postgres_id) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; + .get_postgres_service_certs(&org_id, postgres_id) + .await?; if let Some(path) = output { write_pem_file(path, &pem)?; @@ -2470,12 +2463,7 @@ pub async fn postgres_config_get( json: bool, ) -> CloudResult<()> { let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_instance_config_get(&org_id, postgres_id) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let cfg = unwrap_api(resp)?; + let cfg = client.get_postgres_config(&org_id, postgres_id).await?; if json { println!("{}", serde_json::to_string_pretty(&cfg)?); } else { @@ -2492,12 +2480,9 @@ pub async fn postgres_config_replace( ) -> CloudResult<()> { let cfg = instance_config_from_json(&load_json_file::(file)?)?; let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_instance_config_post(&org_id, postgres_id, &cfg) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let out = unwrap_api(resp)?; + let out = client + .replace_postgres_config(&org_id, postgres_id, &cfg) + .await?; if json { println!("{}", serde_json::to_string_pretty(&out)?); @@ -2536,12 +2521,9 @@ pub async fn postgres_config_patch( }; let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_instance_config_patch(&org_id, postgres_id, &cfg) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let out = unwrap_api(resp)?; + let out = client + .patch_postgres_config(&org_id, postgres_id, &cfg) + .await?; if json { println!("{}", serde_json::to_string_pretty(&out)?); @@ -2578,12 +2560,9 @@ pub async fn postgres_reset_password( let req = PostgresServiceSetPassword { password: pw.clone(), }; - let resp = client - .api() - .postgres_service_set_password(&org_id, postgres_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let out = unwrap_api(resp)?; + let out = client + .set_postgres_service_password(&org_id, postgres_id, &req) + .await?; // Emit what the user now needs to use: the API echoes the password back, but // fall back to the one we sent if the response omits it. let password = out.password.unwrap_or(pw); @@ -2629,12 +2608,9 @@ pub async fn postgres_read_replica_create( }; let org_id = resolve_org_id(client).await?; - let resp = client - .api() - .postgres_instance_create_read_replica(&org_id, postgres_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let svc = unwrap_api(resp)?; + let svc = client + .create_postgres_read_replica(&org_id, postgres_id, &req) + .await?; if json { println!("{}", serde_json::to_string_pretty(&svc)?); @@ -2689,12 +2665,9 @@ pub async fn postgres_restore( pg_config, pg_bouncer_config, }; - let resp = client - .api() - .postgres_instance_restore(&org_id, postgres_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let svc = unwrap_api(resp)?; + let svc = client + .restore_postgres_service(&org_id, postgres_id, &req) + .await?; let svc = apply_restored_access_rules(client, &org_id, svc, &desired, tags).await?; @@ -2789,13 +2762,9 @@ pub async fn postgres_state_change( json: bool, ) -> CloudResult<()> { let org_id = resolve_org_id(client).await?; - let req = PostgresServiceSetState { command: cmd }; - let resp = client - .api() - .postgres_service_patch_state(&org_id, postgres_id, &req) - .await - .map_err(|e| client.convert_error_for_organization(e, &org_id))?; - let svc = unwrap_api(resp)?; + let svc = client + .set_postgres_service_state(&org_id, postgres_id, cmd) + .await?; if json { println!("{}", serde_json::to_string_pretty(&svc)?); @@ -3343,21 +3312,215 @@ impl CloudClient { Self::unwrap_response(response) } - /// Update a Postgres service. - async fn patch_postgres_service( + /// Convert a failed by-id Postgres request whose path names a + /// subresource of the service or whose body carries user input. + /// + /// [`CloudClient::convert_error_for_lookup`] reads a 400 or 404 against + /// well-formed ids as a missing service. That is sound only when the + /// path names the service itself and nothing else in the request is + /// user input (`get`, `delete`). Here a 404 can mean the subresource is + /// absent and a 400 can be the body's validation failure, so the status + /// alone cannot tell. A GET of the service itself decides: when it also + /// rejects the well-formed ids, the service is missing and the original + /// error is reported as such; otherwise the original error stands. The + /// extra GET only happens after such a rejection. + async fn convert_postgres_by_id_result( &self, + result: Result, org_id: &str, postgres_id: &str, - request: &PostgresServicePatchRequest, + ) -> CloudResult { + let error = match result { + Ok(value) => return Ok(value), + Err(error) => error, + }; + let lookup = ResourceLookup::in_org(ResourceKind::PostgresService, postgres_id, org_id); + if lookup.rejected_well_formed_ids(&error) + && let Err(get_error) = self.api().postgres_service_get(org_id, postgres_id).await + && lookup.rejected_well_formed_ids(&get_error) + { + return Err(self.convert_error_for_lookup(error, lookup)); + } + Err(self.convert_error_for_organization(error, org_id)) + } + + /// Create a Postgres service. + async fn create_postgres_service( + &self, + org_id: &str, + request: &PostgresServicePostRequest, ) -> CloudResult { let response = self .api() - .postgres_service_patch(org_id, postgres_id, request) + .postgres_service_create(org_id, request) .await .map_err(|error| self.convert_error_for_organization(error, org_id))?; Self::unwrap_response(response) } + /// Delete a Postgres service. The endpoint returns no resource. + async fn delete_postgres_service(&self, org_id: &str, postgres_id: &str) -> CloudResult<()> { + self.api() + .postgres_service_delete(org_id, postgres_id) + .await + .map_err(|error| { + // This path identifies the Postgres service itself. + self.convert_error_for_lookup( + error, + ResourceLookup::in_org(ResourceKind::PostgresService, postgres_id, org_id), + ) + })?; + Ok(()) + } + + /// Fetch a Postgres service's CA certificate as PEM text. + async fn get_postgres_service_certs( + &self, + org_id: &str, + postgres_id: &str, + ) -> CloudResult { + self.convert_postgres_by_id_result( + self.api() + .postgres_service_certs_get(org_id, postgres_id) + .await, + org_id, + postgres_id, + ) + .await + } + + /// Fetch a Postgres service's runtime configuration. + async fn get_postgres_config( + &self, + org_id: &str, + postgres_id: &str, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_instance_config_get(org_id, postgres_id) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Replace a Postgres service's runtime configuration. + async fn replace_postgres_config( + &self, + org_id: &str, + postgres_id: &str, + config: &PostgresInstanceConfig, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_instance_config_post(org_id, postgres_id, config) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Merge settings into a Postgres service's runtime configuration. + async fn patch_postgres_config( + &self, + org_id: &str, + postgres_id: &str, + config: &PostgresInstanceConfig, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_instance_config_patch(org_id, postgres_id, config) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Set a Postgres service's password. + async fn set_postgres_service_password( + &self, + org_id: &str, + postgres_id: &str, + request: &PostgresServiceSetPassword, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_service_set_password(org_id, postgres_id, request) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Create a read replica of a Postgres service. + async fn create_postgres_read_replica( + &self, + org_id: &str, + postgres_id: &str, + request: &PostgresServiceReadReplicaRequest, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_instance_create_read_replica(org_id, postgres_id, request) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Restore a Postgres service to a point in time as a new service. + async fn restore_postgres_service( + &self, + org_id: &str, + postgres_id: &str, + request: &PostgresServiceRestoreRequest, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_instance_restore(org_id, postgres_id, request) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + + /// Update a Postgres service. + async fn patch_postgres_service( + &self, + org_id: &str, + postgres_id: &str, + request: &PostgresServicePatchRequest, + ) -> CloudResult { + let response = self + .convert_postgres_by_id_result( + self.api() + .postgres_service_patch(org_id, postgres_id, request) + .await, + org_id, + postgres_id, + ) + .await?; + Self::unwrap_response(response) + } + /// Fetch a single Postgres service. pub async fn get_postgres_service( &self, @@ -3412,10 +3575,14 @@ impl CloudClient { ) -> CloudResult { let request = PostgresServiceSetState { command }; let response = self - .api() - .postgres_service_patch_state(org_id, postgres_id, &request) - .await - .map_err(|error| self.convert_error_for_organization(error, org_id))?; + .convert_postgres_by_id_result( + self.api() + .postgres_service_patch_state(org_id, postgres_id, &request) + .await, + org_id, + postgres_id, + ) + .await?; Self::unwrap_response(response) } } diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index 0038f6bb..978a8d12 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -3518,7 +3518,7 @@ async fn probe_repaired_query_key( // Never probe production by accident: a control plane the query host // cannot be derived from (a local mock, an unusual base URL) means the // library would fall back to the production host. - if client.api().configured_query_host().is_none() { + if client.configured_query_host().is_none() { return Ok(skip_repair_verification( org_id, service_id, @@ -3555,7 +3555,7 @@ async fn probe_repaired_query_key( } let outcome = wait_for_query_endpoint_readiness(readiness, || { - client.api().run_query( + client.run_service_query( service_id, key_id, key_secret, @@ -3643,9 +3643,7 @@ async fn run_basic_service_query( confirmed_idle: bool, ) -> Result { run_service_query_with_wake(service_name, confirmed_idle, |wake| { - client - .api() - .run_query(service_id, key_id, key_secret, sql, database, format, wake) + client.run_service_query(service_id, key_id, key_secret, sql, database, format, wake) }) .await } @@ -3806,7 +3804,7 @@ async fn run_just_provisioned_service_query_or( on_timeout: impl FnOnce(Duration) -> CloudError, ) -> CloudResult { let confirmed_idle = wait_for_query_endpoint_readiness(readiness, || { - client.api().run_query( + client.run_service_query( target.service_id, key_id, key_secret, @@ -3944,7 +3942,7 @@ async fn service_query(client: &CloudClient, options: ServiceQueryOptions) -> Cl // possible on this path at all. failure::set_provisioning_state(ProvisioningState::Bearer); let run = |wake: bool| { - client.api().run_query_bearer( + client.run_service_query_bearer( &service_id, &sql, options.database.as_deref(), @@ -4601,6 +4599,58 @@ async fn service_prometheus( } impl CloudClient { + /// The Query API host the library would send queries to, when one is + /// configured or derivable from the control-plane URL. + fn configured_query_host(&self) -> Option { + self.api().configured_query_host() + } + + /// Send one statement to a service's Query API with an API key. + /// + /// Unlike every other wrapper, this returns the library error as-is and + /// the response unread: the caller streams the body, and the query paths + /// classify the raw error themselves (idle and stopped services, wake + /// retries, readiness polling, key rejections) before converting it + /// through `convert_query_error`, which records the telemetry failure. + #[allow(clippy::too_many_arguments)] + async fn run_service_query( + &self, + service_id: &str, + key_id: &str, + key_secret: &str, + sql: &str, + database: Option<&str>, + format: &str, + wake_service: bool, + ) -> Result { + self.api() + .run_query( + service_id, + key_id, + key_secret, + sql, + database, + format, + wake_service, + ) + .await + } + + /// [`Self::run_service_query`] with the client's OAuth bearer token, + /// returning the raw library error for the same reason. + async fn run_service_query_bearer( + &self, + service_id: &str, + sql: &str, + database: Option<&str>, + format: &str, + wake_service: bool, + ) -> Result { + self.api() + .run_query_bearer(service_id, sql, database, format, wake_service) + .await + } + pub async fn get_scaling_schedule( &self, org_id: &str, diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 45975946..efa84b5b 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -20390,6 +20390,217 @@ async fn by_id_read_in_json_mode_keeps_the_servers_message_for_a_malformed_id() } } +// ── Postgres by-id commands report a missing service like `get` (#1156) ── + +fn unknown_postgres_path(suffix: &str) -> String { + format!("/v1/organizations/{LOOKUP_ORG_ID}/postgres/{UNKNOWN_UUID}{suffix}") +} + +/// The not-found message `postgres get` prints for [`UNKNOWN_UUID`], ending in +/// the rejected request's own server message. +fn postgres_not_found_message(server_message: &str) -> String { + format!( + "No such Postgres service: {UNKNOWN_UUID} (organization {LOOKUP_ORG_ID}). \ + The API rejected the identifier: {server_message}" + ) +} + +fn assert_postgres_not_found_json(output: &std::process::Output, server_message: &str) { + assert_eq!(output.status.code(), Some(1)); + assert!(output.stdout.is_empty()); + let error = cloud_runtime_error(output); + assert_eq!(error["code"], "resource_not_found"); + assert_eq!( + error["command"], + format!("clickhousectl cloud postgres list --org-id {LOOKUP_ORG_ID}") + ); + assert_eq!(error["message"], postgres_not_found_message(server_message)); +} + +fn assert_postgres_not_found_human(output: &std::process::Output, server_message: &str) { + assert_eq!(output.status.code(), Some(1)); + assert!(output.stdout.is_empty()); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!("Error: {}\n", postgres_not_found_message(server_message)) + ); +} + +/// `postgres update` reads the service's tags before it patches. A missing +/// service must stop there with the same answer `postgres get` gives, and +/// must not send the PATCH. +#[tokio::test] +async fn postgres_update_of_a_missing_service_reads_as_not_found() { + let server_message = + format!("BAD_REQUEST: Invalid Postgres service id string:\"{UNKNOWN_UUID}\""); + let args = [ + "postgres", + "update", + UNKNOWN_UUID, + "--org-id", + LOOKUP_ORG_ID, + "--new-name", + "renamed", + ]; + for json in [false, true] { + let mock = MockServer::start().await; + mount_invalid_id_400( + &mock, + unknown_postgres_path(""), + "Postgres service", + UNKNOWN_UUID, + ) + .await; + + if json { + let output = invoke_cli_with_cloud_credentials(&mock, &args); + assert_postgres_not_found_json(&output, &server_message); + } else { + let output = invoke_cli_human(&mock, &args); + assert_postgres_not_found_human(&output, &server_message); + } + assert_eq!( + received_request_shape(&mock).await, + vec![("GET".to_string(), unknown_postgres_path(""))] + ); + } +} + +/// `--clear-tags` skips the tag read, so the PATCH is what fails. Its body is +/// user input, so the CLI only calls the service missing after a GET of the +/// service also rejects the id. +#[tokio::test] +async fn postgres_update_clear_tags_of_a_missing_service_reads_as_not_found() { + let mock = MockServer::start().await; + let server_message = + format!("BAD_REQUEST: Invalid Postgres service id string:\"{UNKNOWN_UUID}\""); + Mock::given(method("PATCH")) + .and(path(unknown_postgres_path(""))) + .respond_with(invalid_id_400_response("Postgres service", UNKNOWN_UUID)) + .expect(1) + .mount(&mock) + .await; + mount_invalid_id_400( + &mock, + unknown_postgres_path(""), + "Postgres service", + UNKNOWN_UUID, + ) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "postgres", + "update", + UNKNOWN_UUID, + "--org-id", + LOOKUP_ORG_ID, + "--clear-tags", + ], + ); + + assert_postgres_not_found_json(&output, &server_message); + assert_eq!( + received_request_shape(&mock).await, + vec![ + ("PATCH".to_string(), unknown_postgres_path("")), + ("GET".to_string(), unknown_postgres_path("")), + ] + ); +} + +fn postgres_config_patch_args(setting: &'static str) -> [&'static str; 8] { + [ + "postgres", + "config", + "patch", + UNKNOWN_UUID, + "--org-id", + LOOKUP_ORG_ID, + "--set", + setting, + ] +} + +/// A config PATCH for a deleted service answers 404; the confirming GET of +/// the service agrees, so the command reports the service as missing, in +/// both output modes. +#[tokio::test] +async fn postgres_config_patch_of_a_missing_service_reads_as_not_found() { + let server_message = "NOT_FOUND: Postgres service not found"; + let args = postgres_config_patch_args("max_connections=500"); + for json in [false, true] { + let mock = MockServer::start().await; + for (http_method, route) in [ + ("PATCH", unknown_postgres_path("/config")), + ("GET", unknown_postgres_path("")), + ] { + Mock::given(method(http_method)) + .and(path(route)) + .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": server_message, + }))) + .expect(1) + .mount(&mock) + .await; + } + + if json { + let output = invoke_cli_with_cloud_credentials(&mock, &args); + assert_postgres_not_found_json(&output, server_message); + } else { + let output = invoke_cli_human(&mock, &args); + assert_postgres_not_found_human(&output, server_message); + } + assert_eq!( + received_request_shape(&mock).await, + vec![ + ("PATCH".to_string(), unknown_postgres_path("/config")), + ("GET".to_string(), unknown_postgres_path("")), + ] + ); + } +} + +/// A config PATCH rejected for its body answers 400 too. The service exists, +/// so the server's validation message must reach the user unchanged rather +/// than being misreported as a missing service. +#[tokio::test] +async fn postgres_config_patch_validation_400_keeps_the_servers_message() { + let mock = MockServer::start().await; + Mock::given(method("PATCH")) + .and(path(unknown_postgres_path("/config"))) + .respond_with(ResponseTemplate::new(400).set_body_json(serde_json::json!({ + "status": 400, + "error": "BAD_REQUEST: max_connections must be at least 10", + }))) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("GET")) + .and(path(unknown_postgres_path(""))) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!({"result": {"id": UNKNOWN_UUID}})), + ) + .expect(1) + .mount(&mock) + .await; + + let output = + invoke_cli_with_cloud_credentials(&mock, &postgres_config_patch_args("max_connections=1")); + + assert_eq!(output.status.code(), Some(1)); + let error = cloud_runtime_error(&output); + assert_eq!( + error["message"], + "BAD_REQUEST: max_connections must be at least 10" + ); + assert_eq!(error["code"], "http_4xx"); +} + /// A `service delete` for an id no service has, with the organization id also /// a well-formed UUID so every path identifier passes the structural test. fn invoke_unknown_service_delete(mock: &MockServer, force: bool) -> std::process::Output {