Make CynicClientError::Empty reachable for missing-data responses - #264
Open
thedavidmeister wants to merge 1 commit into
Open
Make CynicClientError::Empty reachable for missing-data responses#264thedavidmeister wants to merge 1 commit into
thedavidmeister wants to merge 1 commit into
Conversation
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>
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #178.
The defect
CynicClient::querymatched onerrorsfirst and fell back todata.ok_or(CynicClientError::Empty). cynic'sGraphQlResponsedeserializerrejects any body where both
dataanderrorsare absent ("Either data orerrors must be present in a GraphQL response"), so
errors: Nonealreadyimplied
data: Some(_)and theEmptyarm was unreachable — a missing-databody surfaced as
CynicClientError::Request(reqwest Decode) instead.Confirmed against the cynic 3.11.0 that
Cargo.lockpins:src/result.rs, thedata.is_none() && errors.is_none()guard in the hand-writtenDeserializeimpl.
The one body shape that passes that deserializer while carrying neither an
error nor data is an empty
errorsarray. The old match sent it down theSome(errors)arm toGraphqlError([])— an error whoseDisplayrenders"Graphql errors: "with nothing after it.The fix
Match on the
(data, errors)pair and guard the errors arm on a non-emptylist:
errors->GraphqlError(unchanged)datapresent ->Ok(data), soerrors: []beside real data is no longerreported as an error carrying no messages
data->Empty, now reachable and testableBehaviour is unchanged for every shape a spec-conforming server sends (the
GraphQL spec forbids an empty
errorslist). Only the empty-errorsshapemoves, and it moves off an error that said nothing.
test_get_metabytes_by_hash_null_data_is_request_decode_errorstill passesunchanged —
{"data": null}with noerrorskey is still rejected at thedeserializer. Its doc comment lost the now-false claim that
Emptyisunreachable from
query.QA
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 outorigin/main'scrates/metaboard/src/cynic_client.rsover this branch's tests and rancargo test -p rain-metaboard-subgraph --lib: 12 passed, both new tests failed, the first onunexpected result: Err(RequestErrorByHash { .., source: GraphqlError([]) }))cynic_client.rs(None, _) => Err(CynicClientError::Empty)→Err(CynicClientError::GraphqlError(vec![]))→ killed bytest_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 testserrorsmust be a non-empty list when present, and must be absent when no errors were raised, soerrors: []carries no error) plus cynic 3.11.0'sGraphQlResponsedeserializer read directly from the vendored source, which decides which bodies reachquery's match at all — neither derived from this repo's implementationEmptyarm — remove/document it as defensive, or surface missing-data deliberately. Took the second: the arm is now live for the empty-errorsbody, so nothing is dead and nothing needs a defensive comment. No panic path was introduced, which the first disposition would have needed sinceGraphQlResponse::dataisOption<T>and theNonecase must produce some value.cargo test -p rain-metaboard-subgraph --lib: 14 passed, 0 failed.cargo fmt --checkandcargo clippy -p rain-metaboard-subgraph --all-targets -- -D warningsclean.🤖 Generated with Claude Code
rainix-rs / static / rs-staticfails on this branch and onmainat the samestep — the pre-commit
rustfmthook exiting "Failed to find targets" — arepo-wide rainix hook bug, not this change.
cargo clippy --all-targets --all-features -- -D warnings -D clippy::allran clean in that same job beforethe hook failed. Every other check is green.