Skip to content

Make CynicClientError::Empty reachable for missing-data responses - #264

Open
thedavidmeister wants to merge 1 commit into
mainfrom
2026-08-25-issue-178
Open

Make CynicClientError::Empty reachable for missing-data responses#264
thedavidmeister wants to merge 1 commit into
mainfrom
2026-08-25-issue-178

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Closes #178.

The defect

CynicClient::query matched on errors first and fell back to
data.ok_or(CynicClientError::Empty). cynic's GraphQlResponse deserializer
rejects any body where both data and errors are absent ("Either data or
errors must be present in a GraphQL response"), so errors: None already
implied data: Some(_) and the Empty arm was unreachable — a missing-data
body surfaced as CynicClientError::Request (reqwest Decode) instead.
Confirmed against the cynic 3.11.0 that Cargo.lock pins: src/result.rs, the
data.is_none() && errors.is_none() guard in the hand-written Deserialize
impl.

The one body shape that passes that deserializer while carrying neither an
error nor data is an empty errors array. The old match sent it down the
Some(errors) arm to GraphqlError([]) — an error whose Display renders
"Graphql errors: " with nothing after it.

The fix

Match on the (data, errors) pair and guard the errors arm on a non-empty
list:

  • non-empty errors -> GraphqlError (unchanged)
  • data present -> Ok(data), so errors: [] beside real data is no longer
    reported as an error carrying no messages
  • no data -> Empty, now reachable and testable

Behaviour is unchanged for every shape a spec-conforming server sends (the
GraphQL spec forbids an empty errors list). Only the empty-errors shape
moves, and it moves off an error that said nothing.

test_get_metabytes_by_hash_null_data_is_request_decode_error still passes
unchanged — {"data": null} with no errors key is still rejected at the
deserializer. Its doc comment lost the now-false claim that Empty is
unreachable from query.

QA

  • Discriminating tests: test_get_metabytes_by_hash_null_data_empty_errors_is_empty, test_get_metabytes_by_hash_empty_errors_returns_data — each fails on base (checked out origin/main's crates/metaboard/src/cynic_client.rs over this branch's tests and ran cargo test -p rain-metaboard-subgraph --lib: 12 passed, both new tests failed, the first on unexpected result: Err(RequestErrorByHash { .., source: GraphqlError([]) }))
  • Mutations applied: cynic_client.rs (None, _) => Err(CynicClientError::Empty)Err(CynicClientError::GraphqlError(vec![])) → killed by test_get_metabytes_by_hash_null_data_empty_errors_is_empty (this is the equivalent mutant issue CynicClientError::Empty is unreachable from CynicClient::query: missing/null data surfaces as a Request decode error #178 names; it now dies); cynic_client.rs (_, Some(errors)) if !errors.is_empty() → guard dropped, (_, Some(errors)) → killed by both new tests
  • Oracle: the GraphQL spec (errors must be a non-empty list when present, and must be absent when no errors were raised, so errors: [] carries no error) plus cynic 3.11.0's GraphQlResponse deserializer read directly from the vendored source, which decides which bodies reach query's match at all — neither derived from this repo's implementation
  • Category check: issue asks for one of two dispositions for the dead Empty arm — remove/document it as defensive, or surface missing-data deliberately. Took the second: the arm is now live for the empty-errors body, so nothing is dead and nothing needs a defensive comment. No panic path was introduced, which the first disposition would have needed since GraphQlResponse::data is Option<T> and the None case must produce some value.

cargo test -p rain-metaboard-subgraph --lib: 14 passed, 0 failed. cargo fmt --check and cargo clippy -p rain-metaboard-subgraph --all-targets -- -D warnings clean.

🤖 Generated with Claude Code


rainix-rs / static / rs-static fails on this branch and on main at the same
step — the pre-commit rustfmt hook exiting "Failed to find targets" — a
repo-wide rainix hook bug, not this change. cargo clippy --all-targets --all-features -- -D warnings -D clippy::all ran clean in that same job before
the hook failed. Every other check is green.

Closes #178.

An empty `errors` array is the one body shape that passes cynic's
"either data or errors must be present" deserializer while carrying
neither an error nor data. The old match sent it to `GraphqlError([])`,
an error that renders with no messages, and left `data.ok_or(Empty)`
unreachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 47 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0ceaa9c6-9759-48e1-9e33-fb23a55aa315

📥 Commits

Reviewing files that changed from the base of the PR and between 45ca96c and 437fa03.

📒 Files selected for processing (2)
  • crates/metaboard/src/cynic_client.rs
  • crates/metaboard/src/metaboard_client.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

CynicClientError::Empty is unreachable from CynicClient::query: missing/null data surfaces as a Request decode error

1 participant