Split the empty-result error into by-hash and by-subject variants - #262
Open
thedavidmeister wants to merge 1 commit into
Open
Split the empty-result error into by-hash and by-subject variants#262thedavidmeister wants to merge 1 commit into
thedavidmeister wants to merge 1 commit into
Conversation
Closes #179 MetaboardSubgraphClientError::Empty(String) rendered "no data for metahash {0}" for both queries, so an empty subject query labelled the subject value as a metahash. Split it the way RequestErrorByHash / RequestErrorBySubject already are, so each message names the key it actually queried on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 49 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 #179
The defect
MetaboardSubgraphClientError::Empty(String)carried one message for two different query keys:get_metabytes_by_hashfills it with the metahash, butget_metabytes_by_subjectfills it withsubject.0, so an empty subject query renderedSubgraph query returned no data for metahash 0x315where0x315was the subject argument.The fix
Split it the way
RequestErrorByHash/RequestErrorBySubjectin the same enum already are:Each construction site now names the key it actually queried on. The one consumer,
DotrainSourceV1::fetch_by_subject, matchedEmpty(_)to map an empty result toOk(None); it now matchesEmptyBySubject { .. }, the only empty variant that method can produce.Public API change:
Empty(String)is gone fromrain-metaboard-subgraph's public error enum, replaced by the two variants above. That is the "per-key variant" option the issue's triage framing named; the alternative it named — neutral wording (for query key {0}) — keeps the variant but gives up what the intent oracle asks for, a message that identifies what was queried. No shim is kept for the old variant. Version bumps are the release workflow's job, so none here.QA
metaboard_client::tests::test_get_metabytes_by_subject_empty— it now asserts the renderedDisplaystring, which is where the defect lived (the payload was already the subject on base, so the previous payload-only assertion could not see it). It cannot be run verbatim on base, becauseEmptyBySubjectdoes not exist there; base behaviour was reproduced in-tree by mutation, below, and the test fails on it with the issue's exact reported string.test_get_metabytes_by_hash_emptyalso gained aDisplayassertion but is NOT discriminating for this issue — by-hash rendering was already correct on base; it is a regression guard for the split.crates/metaboard/src/metaboard_client.rs:27#[error("...no data for subject {subject}")]-> base's wording#[error("...no data for metahash {subject}")]-> killed bytest_get_metabytes_by_subject_empty:left: "Subgraph query returned no data for metahash 0x315",right: "Subgraph query returned no data for subject 0x315".crates/metaboard/src/metaboard_client.rs:115constructionEmptyBySubject { subject: subject.0.clone() }->EmptyByHash { metahash: subject.0.clone() }-> killed bytest_get_metabytes_by_subject_empty, same left/right.crates/cli/src/meta/types/dotrain/source_v1.rs:59armEmptyBySubject { .. }->EmptyByHash { .. }-> killed by the pre-existingsource_v1::tests::test_fetch_by_subject_not_found(assertion failed: result.is_ok()), so the retargeted arm is covered.RequestErrorByHashsays "metahash" andRequestErrorBySubjectsays "subject" for the same two keys. The expected strings were written from that pair, not read back off the#[error]attributes under test.EmptyDisplaylabels a subject value as a metahash — and it is covered. Its open triage note ("the variant shape is a public API") is a design call, ruled here in favour of the per-key split with the reasoning in the Public API change paragraph above, for a reviewer to override.Run locally in the flake shell:
cargo fmt --check,cargo clippy -p rain-metaboard-subgraph -p rain-metadata --all-targets -- -D warnings,cargo test -p rain-metaboard-subgraph --lib(12 passed) andcargo test -p rain-metadata --lib source_v1(15 passed). The full suite was not run locally; CI covers it.