From f3a8fce207198ed46d88d42dd9464839b243c800 Mon Sep 17 00:00:00 2001 From: "Sanket M." Date: Mon, 13 Jul 2026 21:32:40 +0530 Subject: [PATCH 1/2] fix(api-call): apply headers, error on non-2xx, GraphQL errors, output shape (DEVX-546) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gddy api call` had five defects that made it unreliable — and left the GraphQL commerce domains (catalog-products, taxes) uncallable: - Headers: `--header KEY:VALUE` was parsed but never attached to the request, so endpoints requiring a header (the GraphQL commerce domains need `x-store-id`) could not be called. Now applied (split on first colon, trimmed). - Non-2xx: any non-2xx status was returned as a success result carrying the error body. Now returns an error (non-zero exit) with the status line + truncated body. - Non-JSON body: was silently coerced to `null`. Now preserved as raw text. - GraphQL errors: a GraphQL 200 response with a top-level `errors` array was reported as success. Now detected (by graphql/subgraph path) and surfaced as an error. - Output shape: `{ status, body }` -> `{ endpoint, method, status, status_text, data }`, matching the documented contract. Extracted split_header / parse_response_body / graphql_errors as pure helpers with unit tests. cargo test/clippy/fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- rust/src/api_explorer/mod.rs | 124 +++++++++++++++++++++++++++++++++-- 1 file changed, 118 insertions(+), 6 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 9bb9297..4a181aa 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -196,6 +196,34 @@ fn string_list(args: &serde_json::Map, key: &str) -> Vec } } +/// Split a `--header` value of the form `KEY:VALUE` into trimmed parts. +/// Splits on the first colon only, so values may themselves contain colons +/// (e.g. a URL). Returns None when there is no colon. +fn split_header(raw: &str) -> Option<(&str, &str)> { + raw.split_once(':').map(|(k, v)| (k.trim(), v.trim())) +} + +/// Parse a response body as JSON when possible, otherwise preserve it as raw +/// UTF-8 text. A non-JSON body (plain text / HTML error page) must not be +/// silently dropped to `null` — only a truly empty or binary body becomes null. +fn parse_response_body(bytes: &[u8]) -> Value { + match serde_json::from_slice::(bytes) { + Ok(v) => v, + Err(_) => match std::str::from_utf8(bytes) { + Ok(s) if !s.is_empty() => json!(s), + _ => Value::Null, + }, + } +} + +/// A non-empty top-level GraphQL `errors` array, if present. GraphQL endpoints +/// return HTTP 200 even on failure and carry the failure here. +fn graphql_errors(body: &Value) -> Option<&Vec> { + body.get("errors") + .and_then(|e| e.as_array()) + .filter(|a| !a.is_empty()) +} + /// Union of user-supplied `--scope` flags and a matched endpoint's declared /// scopes, order-preserving and de-duplicated (flags first). fn merge_required_scopes(flag_scopes: Vec, endpoint_scopes: &[String]) -> Vec { @@ -663,6 +691,19 @@ fn call_command() -> RuntimeCommandSpec { .bearer_auth(&token) .header("x-request-id", uuid::Uuid::new_v4().to_string()); + // Apply user-supplied `--header KEY:VALUE` values (repeatable). These + // were parsed but never attached to the request, so endpoints that + // require a header — e.g. the GraphQL commerce domains need `x-store-id` + // — could not be called at all. + for h in string_list(&ctx.args, "header") { + let (key, val) = split_header(&h).ok_or_else(|| { + cli_engine::CliCoreError::message(format!( + "invalid header '{h}': expected KEY:VALUE" + )) + })?; + req = req.header(key, val); + } + if let Some(body) = request_body { req = req.json(&body); } @@ -676,7 +717,8 @@ fn call_command() -> RuntimeCommandSpec { .await .map_err(|e| cli_engine::CliCoreError::message(e.to_string()))?; - let status = resp.status(); + let status_code = resp.status(); + let status_text = status_code.canonical_reason().unwrap_or("").to_owned(); let response_headers_raw = resp.headers().clone(); let include_headers = ctx .args @@ -688,12 +730,12 @@ fn call_command() -> RuntimeCommandSpec { .await .map_err(|e| cli_engine::CliCoreError::message(e.to_string()))?; cli_engine::transport::debug_log_reqwest_response( - status, + status_code, &response_headers_raw, &body_bytes, ); - let status = status.as_u16(); + let status = status_code.as_u16(); let response_headers: Option> = if include_headers { Some( response_headers_raw @@ -705,11 +747,23 @@ fn call_command() -> RuntimeCommandSpec { None }; - let body: Value = serde_json::from_slice(&body_bytes).unwrap_or(json!(null)); + let body: Value = parse_response_body(&body_bytes); + + // GraphQL endpoints return HTTP 200 even on failure, carrying the error + // in a top-level `errors` array. Detect the GraphQL commerce surfaces by + // path and surface those errors instead of reporting a false success. + let is_graphql = endpoint.contains("graphql") || endpoint.contains("subgraph"); + if is_graphql && let Some(errors) = graphql_errors(&body) { + return Err(cli_engine::CliCoreError::message(format!( + "GraphQL request returned {} error(s):\n{}", + errors.len(), + serde_json::to_string_pretty(&json!(errors)).unwrap_or_default(), + ))); + } // Scope step-up already ran up front (the token was requested with // `required`). A 403 here means the granted token still lacks a - // required scope — surface it rather than silently returning the body. + // required scope — surface it with a re-login hint. if status == 403 && !required.is_empty() { // `auth login --scope` is append-style (one value per flag), so // repeat the flag rather than space-joining. @@ -725,6 +779,20 @@ fn call_command() -> RuntimeCommandSpec { ))); } + // Any other non-2xx is a failure, not a success envelope. Include the + // status line and (truncated) response body so the caller sees the detail + // instead of a success result that happens to carry an error payload. + if !(200..300).contains(&status) { + let detail: String = serde_json::to_string_pretty(&body) + .unwrap_or_else(|_| body.to_string()) + .chars() + .take(4000) + .collect(); + return Err(cli_engine::CliCoreError::message(format!( + "{status} {status_text}\n{detail}" + ))); + } + let query_path = ctx.args.get("query").and_then(|v| v.as_str()); let output = if let Some(path) = query_path { extract_json_path(&body, path) @@ -734,7 +802,14 @@ fn call_command() -> RuntimeCommandSpec { body }; - let mut result = json!({ "status": status, "body": output }); + // Identify the call and its outcome in the result envelope. + let mut result = json!({ + "endpoint": endpoint, + "method": method, + "status": status, + "status_text": status_text, + "data": output, + }); if let Some(headers) = response_headers { result["headers"] = Value::Object(headers); } @@ -781,6 +856,43 @@ mod tests { ); } + #[test] + fn split_header_trims_and_splits_on_first_colon() { + assert_eq!( + super::split_header("x-store-id: abc-123"), + Some(("x-store-id", "abc-123")) + ); + // Value may contain colons (e.g. a URL) — only the first colon splits. + assert_eq!( + super::split_header("Location: https://x/y"), + Some(("Location", "https://x/y")) + ); + assert_eq!(super::split_header("no-colon"), None); + } + + #[test] + fn parse_response_body_json_text_and_empty() { + use serde_json::json; + assert_eq!(super::parse_response_body(b"{\"a\":1}"), json!({"a":1})); + // Non-JSON is preserved as text, not dropped to null. + assert_eq!( + super::parse_response_body(b"plain text"), + json!("plain text") + ); + // Empty body is null. + assert_eq!(super::parse_response_body(b""), serde_json::Value::Null); + } + + #[test] + fn graphql_errors_detects_nonempty_array_only() { + use serde_json::json; + assert!( + super::graphql_errors(&json!({"data": null, "errors": [{"message": "x"}]})).is_some() + ); + assert!(super::graphql_errors(&json!({"data": {}, "errors": []})).is_none()); + assert!(super::graphql_errors(&json!({"data": {}})).is_none()); + } + #[test] fn string_list_handles_scalar_array_and_missing() { use serde_json::json; From 68ec959f80d7fb123ae0c3bb00d9995fd63d1be7 Mon Sep 17 00:00:00 2001 From: "Sanket M." Date: Wed, 15 Jul 2026 14:45:05 +0530 Subject: [PATCH 2/2] chore(api-call): simplify header application comment (DEVX-546) Remove historical bug context from the inline comment so it only documents the current behavior. --- rust/src/api_explorer/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 4a181aa..50af917 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -691,10 +691,7 @@ fn call_command() -> RuntimeCommandSpec { .bearer_auth(&token) .header("x-request-id", uuid::Uuid::new_v4().to_string()); - // Apply user-supplied `--header KEY:VALUE` values (repeatable). These - // were parsed but never attached to the request, so endpoints that - // require a header — e.g. the GraphQL commerce domains need `x-store-id` - // — could not be called at all. + // Apply user-supplied `--header KEY:VALUE` values (repeatable). for h in string_list(&ctx.args, "header") { let (key, val) = split_header(&h).ok_or_else(|| { cli_engine::CliCoreError::message(format!(