Skip to content

fix(api-call): apply headers, error on non-2xx, GraphQL errors, output shape (DEVX-546)#97

Open
smukherjee-godaddy wants to merge 2 commits into
godaddy:rust-portfrom
smukherjee-godaddy:DEVX-546-api-call-fixes
Open

fix(api-call): apply headers, error on non-2xx, GraphQL errors, output shape (DEVX-546)#97
smukherjee-godaddy wants to merge 2 commits into
godaddy:rust-portfrom
smukherjee-godaddy:DEVX-546-api-call-fixes

Conversation

@smukherjee-godaddy

Copy link
Copy Markdown
Contributor

Problem

gddy api call had five defects that made it unreliable — and left the GraphQL commerce domains (catalog-products, taxes) impossible to call at all:

  1. Headers dropped--header KEY:VALUE was parsed but never attached to the request. Endpoints that require a header (the GraphQL commerce domains need x-store-id) could not be called.
  2. Non-2xx returned as success — any non-2xx response was returned as a successful command result carrying the error body, so scripts/agents saw "success" on a 4xx/5xx.
  3. Non-JSON coerced to null — a plain-text/HTML error body was silently turned into null, discarding the detail.
  4. GraphQL errors swallowed — a GraphQL 200 response with a top-level errors array was reported as success.
  5. Output shape drift — result was { status, body } instead of the documented { endpoint, method, status, status_text, data }.

Fix

  • Headers: apply every --header to the request (split on the first colon so values may contain colons; trimmed).
  • Non-2xx: return an error (non-zero exit) with the status line + truncated response body, instead of a success envelope.
  • Non-JSON: preserve the raw UTF-8 text; only truly empty/binary bodies become null.
  • GraphQL: detect the GraphQL commerce surfaces by path (graphql/subgraph) and surface a non-empty errors[] as an error.
  • Output shape: { endpoint, method, status, status_text, data } (+ headers when --include).

Extracted split_header, parse_response_body, and graphql_errors as pure helpers with unit tests. The existing 403-with-required-scopes hint is preserved ahead of the generic non-2xx handler.

Impact

  • The GraphQL commerce domains (catalog-products, taxes) are now callable via --header x-store-id: <uuid>.
  • Non-2xx and GraphQL errors fail loudly instead of masquerading as success.
  • Response detail is never silently lost.

Scope / dependency

This fixes correctness at the api call layer. The final envelope wrapping — success as { ok, command, result, next_actions } on stdout and errors on stdout — is the output-contract work (DEVX-542 / DEVX-543); this PR composes with it and does not duplicate it.

Companion to DEVX-548 (catalog $ref resolution): that made the commerce schemas resolvable; this makes the endpoints callable.

Testing

  • cargo test api_explorer — 8 pass (3 new: split_header_*, parse_response_body_*, graphql_errors_*)
  • cargo clippy --bin gddy -- -D warnings — clean
  • cargo fmt --check — clean

Not covered here: a full httpmock + mock-credential integration test of the success path (api call requires a token) — that's the T4 test in the test-layer ticket.

…t shape (DEVX-546)

`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) <noreply@anthropic.com>
Comment thread rust/src/api_explorer/mod.rs Outdated
Comment on lines +694 to +697
// 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to reference prior bugs; not useful for future readers.

Suggested change
// 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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 68ec959. The comment now documents only the current header application behavior.

@jpage-godaddy jpage-godaddy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nit here and the rest looks good.

Remove historical bug context from the inline comment so it only documents the current behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants