diff --git a/src/cmd/issue/fixtures.rs b/src/cmd/issue/fixtures.rs new file mode 100644 index 0000000..bdd5bee --- /dev/null +++ b/src/cmd/issue/fixtures.rs @@ -0,0 +1,119 @@ +//! The captured records `read`'s tests are checked against. +//! +//! `cmd::pr::read::fixtures`'s reasoning, one collection over: these are real +//! bytes off real services, and the paragraphs saying what each capture holds +//! are what let an assertion counting six rows in one be trusted. They live +//! in a module of their own so there is one copy of those paragraphs rather +//! than one per test that reads a file. +//! +//! Every capture below was taken from a service that has never heard of atgc, +//! by an unauthenticated request anybody can repeat. What they are here to +//! pin is the gap between the lexicon and the wild: `sh.tangled.repo.issue` +//! types `repo` as a DID and marks `createdAt` required, and a live +//! collection is full of records that do neither. Before these files, that +//! was a claim four doc comments made and nothing checked. + +use serde_json::Value; + +/// `tangled.org/core`, which most of the captured issues are filed against +/// and which every repo-scoped assertion here is scoped to. +pub(super) const CORE_REPO: &str = "did:plc:j5hmlfdrwkvtxm7cjmu7j2is"; + +/// One page of one account's `sh.tangled.repo.issue` collection, which is +/// exactly what `issue list` reads. +/// +/// Ten records, all of them issues on [`CORE_REPO`], and they do not agree +/// about how to say so: six name it by DID, and four carry the pre-DID +/// spelling — `repo` holding the at-uri of the repo *record*, with the repo's +/// DID beside it in a `repoDid` property the lexicon has never had. Both +/// stamp spellings are here too, `…Z` and `…+03:00`, which is why nothing in +/// the listing compares `createdAt` as a string. +/// +/// ```text +/// curl 'https://woodear.us-west.host.bsky.network/xrpc/com.atproto.repo.listRecords\ +/// ?repo=did:plc:xasnlahkri4ewmbuzly2rlc5&collection=sh.tangled.repo.issue&limit=10\ +/// &cursor=3mmbnhtrah522' +/// ``` +pub(super) const PDS_ISSUES: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/pds_issues_page.json" +)); + +/// The index's answer about the same account, overlapping the page above. +/// +/// `sh.tangled.repo.listIssuesBy` wraps each record in the item envelope +/// `merge_indexed` reads — `state` and `commentCount` beside the record — and +/// it does not hand the record back untouched: a pre-DID record arrives with +/// its `repo` rewritten to the repo DID and its own `repoDid` dropped, under +/// the record's real CID. The two halves of a merged listing therefore +/// disagree about the same issue, which is what `Listed` reading its fields +/// off whichever row survived is for. +/// +/// ```text +/// curl 'https://api.tangled.org/xrpc/sh.tangled.repo.listIssuesBy\ +/// ?subject=did:plc:xasnlahkri4ewmbuzly2rlc5&limit=10&cursor=…' +/// ``` +pub(super) const BOBBIN_ISSUES: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/bobbin_list_issues_by.json" +)); + +/// A single issue record in the shape every current writer produces: `repo` a +/// DID, `createdAt` a UTC stamp, `mentions` and `references` present and +/// empty. Captured with `com.atproto.repo.getRecord`, so it carries the +/// `{uri, cid, value}` envelope `fetch_issue` reads. +pub(super) const NEW_RECORD: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/issue_new_record.json" +)); + +/// A single pre-DID record, and the densest one found in the wild: `repo` an +/// at-uri, `createdAt` the empty string, and two properties no version of the +/// lexicon has — `owner`, and `issueId` holding the appview's own issue +/// number. The number being *in* a record is worth knowing, since `issue +/// view 23` refuses on the grounds that it is nowhere; it is in this shape +/// and in nothing written since. +pub(super) const OLD_RECORD: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/issue_old_record.json" +)); + +/// One account's whole `sh.tangled.repo.issue.state` collection: ten ordinary +/// closes and two records that are neither dated nor pointed at anything — +/// `createdAt` absent and `issue` the empty string, both of which the lexicon +/// marks required. They are what `state_event` declines to read. +pub(super) const STATES: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/issue_states.json" +)); + +/// A second state collection, kept for one thing the first cannot show: live +/// close/reopen/close chains, each three records inside seven seconds. The +/// newest-wins rule has to settle those on the stamp and not on the order the +/// PDS listed them in. +pub(super) const STATES_REOPENED: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/issue_states_reopened.json" +)); + +fn array(fixture: &str, key: &str) -> Vec { + serde_json::from_str::(fixture).expect("fixture is JSON")[key] + .as_array() + .expect("fixture holds that array") + .clone() +} + +/// The `records` of a captured `listRecords` page. +pub(super) fn records(fixture: &str) -> Vec { + array(fixture, "records") +} + +/// The `items` of a captured Bobbin listing. +pub(super) fn items(fixture: &str) -> Vec { + array(fixture, "items") +} + +/// A captured `getRecord` response, whole. +pub(super) fn single(fixture: &str) -> Value { + serde_json::from_str(fixture).expect("fixture is JSON") +} diff --git a/src/cmd/issue/mod.rs b/src/cmd/issue/mod.rs index ecaaee2..6b78ad8 100644 --- a/src/cmd/issue/mod.rs +++ b/src/cmd/issue/mod.rs @@ -36,6 +36,9 @@ pub(crate) mod read; pub(crate) mod write; +#[cfg(test)] +mod fixtures; + /// The `atgc issue` verbs. #[derive(clap::Subcommand, Debug)] pub(crate) enum Command { diff --git a/src/cmd/issue/read.rs b/src/cmd/issue/read.rs index 42ee685..b056388 100644 --- a/src/cmd/issue/read.rs +++ b/src/cmd/issue/read.rs @@ -374,6 +374,35 @@ impl State { /// could not spin. const MAX_STATE_PAGES: usize = 50; +/// One `listRecords` row read as the issue it is about and the event it is, +/// or `None` for a record that names no issue. +/// +/// Everything but `issue` is read leniently, because a state record is +/// somebody else's write and a listing must not fail over one: an absent +/// `createdAt` becomes the empty string, which [`StateEvent::instant`] parses +/// to `None` and which therefore loses to every dated record rather than +/// winning; an absent `state` becomes a token [`IssueState::from_token`] +/// declines, so [`newest_state`] skips it. Both shapes are real — see +/// `fixtures::STATES`, where two of twelve captured records carry neither a +/// stamp nor a subject. +/// +/// `issue` is the one field with no lenient reading available. A record that +/// does not say which issue it is about cannot be filed under one, and the +/// empty string is that case rather than a subject: it matches no at-uri, so +/// keeping it would only mean carrying it to be discarded later. +fn state_event(record: &serde_json::Value) -> Option<(String, StateEvent)> { + let value = &record["value"]; + let issue = value["issue"].as_str().filter(|i| !i.is_empty())?; + Some(( + issue.to_string(), + StateEvent { + uri: record["uri"].as_str().unwrap_or_default().to_string(), + created_at: value["createdAt"].as_str().unwrap_or_default().to_string(), + state: value["state"].as_str().unwrap_or_default().to_string(), + }, + )) +} + /// Every state record in `did`'s PDS about any of `issues`, keyed by issue /// at-uri. /// @@ -421,26 +450,13 @@ async fn state_events_for( } for record in &records { - let Some(issue) = record["value"]["issue"].as_str() else { + let Some((issue, event)) = state_event(record) else { continue; }; - if !wanted.contains(issue) { + if !wanted.contains(issue.as_str()) { continue; } - found - .entry(issue.to_string()) - .or_default() - .push(StateEvent { - uri: record["uri"].as_str().unwrap_or_default().to_string(), - created_at: record["value"]["createdAt"] - .as_str() - .unwrap_or_default() - .to_string(), - state: record["value"]["state"] - .as_str() - .unwrap_or_default() - .to_string(), - }); + found.entry(issue).or_default().push(event); } let oldest_on_page = records @@ -1415,8 +1431,9 @@ async fn resolve_issue( #[cfg(test)] mod tests { - use super::{IssueRef, IssueState, Listed, State, StateEvent}; - use super::{classify_issue_ref, day, merge_indexed, names_repo, newest_state}; + use super::super::fixtures; + use super::{FetchedIssue, IssueRef, IssueState, Listed, State, StateEvent}; + use super::{classify_issue_ref, day, merge_indexed, names_repo, newest_state, state_event}; use serde_json::json; const ME: &str = "did:plc:nlzmjyfv6loqtxyzvdcznwgf"; @@ -1806,4 +1823,191 @@ mod tests { assert_eq!(untitled.title(), "(untitled)"); assert_eq!(untitled.created_at(), ""); } + + // ----------------------------------------------------------------------- + // Against captured records + // ----------------------------------------------------------------------- + // + // Everything above this line is hand-written JSON, which proves that the + // helpers do what they were written to do and nothing about whether the + // shapes they were written for are the shapes that exist. These read real + // records off `tests/fixtures/`; see `super::super::fixtures` for what + // each capture holds and where it came from. + + fn fetched(fixture: &str) -> FetchedIssue { + let captured = fixtures::single(fixture); + FetchedIssue { + uri: captured["uri"].as_str().unwrap().to_string(), + author: captured["uri"] + .as_str() + .unwrap() + .trim_start_matches("at://") + .split('/') + .next() + .unwrap() + .to_string(), + rkey: captured["uri"] + .as_str() + .unwrap() + .rsplit('/') + .next() + .unwrap() + .to_string(), + value: captured["value"].clone(), + cid: captured["cid"].as_str().map(str::to_string), + } + } + + /// What each reader on [`FetchedIssue`] answers for a record of each + /// shape, on the bytes rather than on a description of them. + /// + /// The pre-DID record is the one that matters. Its `repo` is an at-uri + /// whose authority is the repo *owner's* account, so `repo_did` answers + /// `None` rather than a plausible wrong DID — and its `createdAt` is + /// present and empty, which reads as no stamp rather than as a stamp of + /// nothing. Both are what the doc comments on those two methods claim, + /// and neither had a record behind it before this. + #[test] + fn the_shapes_a_live_issue_collection_actually_holds() { + let new = fetched(fixtures::NEW_RECORD); + assert_eq!(new.repo_did(), Some(fixtures::CORE_REPO)); + assert!(new.created_at().is_some()); + assert!(new.body().is_some()); + + let old = fetched(fixtures::OLD_RECORD); + assert_eq!( + old.repo_did(), + None, + "an at-uri in `repo` is not a repo DID and is not read as one" + ); + assert_eq!(old.created_at(), None, "an empty stamp is no stamp"); + assert_eq!(old.title(), "xyz"); + // The appview's own issue number, in a record — which `issue view 23` + // refuses on the grounds that it is in none. It is in this shape and + // in nothing written since; see `fixtures::OLD_RECORD`. + assert_eq!(old.value["issueId"], 2); + } + + /// The cost of `names_repo` being an exact DID comparison, counted on a + /// real collection. + /// + /// Every one of the ten captured records is an issue on `CORE_REPO`, and + /// the filter keeps six: the four that name the repo by at-uri are + /// dropped from a repo-scoped listing of the author's own PDS. The index + /// does not drop them, because it rewrites `repo` to the DID before + /// handing the record back — so `--source bobbin` shows issues that the + /// PDS half of the same command cannot, on records the account itself + /// wrote. + /// + /// Left as it is rather than fixed: matching the at-uri too would mean + /// resolving it, and the at-uri names the repo *record*, whose key is not + /// the repo name and whose authority is the owner. TODO.md carries the + /// entry. + #[test] + fn a_repo_scoped_pds_listing_drops_the_pre_did_records() { + let records = fixtures::records(fixtures::PDS_ISSUES); + assert_eq!(records.len(), 10, "the captured page changed"); + let kept = records + .iter() + .filter(|r| names_repo(r, fixtures::CORE_REPO)) + .count(); + assert_eq!(kept, 6); + + // The same four are on the page all the same, and say which repo + // they mean in a property the lexicon does not have. + let dropped: Vec<&serde_json::Value> = records + .iter() + .filter(|r| !names_repo(r, fixtures::CORE_REPO)) + .collect(); + assert_eq!(dropped.len(), 4); + for record in dropped { + assert!( + record["value"]["repo"] + .as_str() + .unwrap() + .starts_with("at://") + ); + assert_eq!(record["value"]["repoDid"], fixtures::CORE_REPO); + } + + let indexed = fixtures::items(fixtures::BOBBIN_ISSUES); + let seen = indexed + .iter() + .filter(|i| names_repo(i, fixtures::CORE_REPO)) + .count(); + assert_eq!(seen, 9, "the index's rewrite passes the same filter"); + } + + /// Every stamp on a captured page parses, in both spellings that page + /// carries. `Listed::instant` exists because one account's records come + /// back in whatever offset the writing client used, and a listing that + /// sorted them as strings would be sorting `…Z` against `…+03:00`. + #[test] + fn both_stamp_spellings_on_one_page_parse_to_an_instant() { + let rows: Vec = fixtures::records(fixtures::PDS_ISSUES) + .into_iter() + .map(|value| Listed { + uri: value["uri"].as_str().unwrap().to_string(), + value, + state: State::Unknown, + comments: 0, + indexed: false, + }) + .collect(); + assert!(rows.iter().any(|r| r.created_at().ends_with('Z'))); + assert!(rows.iter().any(|r| r.created_at().ends_with("+03:00"))); + for row in &rows { + assert!( + row.instant().is_some(), + "{} did not parse", + row.created_at() + ); + } + } + + /// A captured close/reopen/close chain, settled the way the appview + /// settles it. + /// + /// Seven seconds separate the three records, and the listing order is not + /// the answer: `newest_state` has to read the stamps. Fed in the order + /// the PDS returned them and again reversed, it says `closed` both times. + #[test] + fn a_captured_reopen_chain_settles_on_the_newest_record() { + const ISSUE: &str = + "at://did:plc:jge3zxi7lgrfnvhzcgrimeo7/sh.tangled.repo.issue/3msfd4c3gskmf"; + let mut events: Vec = fixtures::records(fixtures::STATES_REOPENED) + .iter() + .filter_map(state_event) + .filter(|(issue, _)| issue == ISSUE) + .map(|(_, event)| event) + .collect(); + assert_eq!(events.len(), 3, "the captured chain changed"); + assert_eq!(newest_state(&events), Some(IssueState::Closed)); + events.reverse(); + assert_eq!(newest_state(&events), Some(IssueState::Closed)); + } + + /// The two captured records that are not about anything. + /// + /// `issue` is the empty string and `createdAt` is absent, both of which + /// the lexicon marks required. `state_event` declines them, so they never + /// reach the sort — and a listing reading that PDS still answers for the + /// ten records beside them rather than failing over these two. + #[test] + fn a_state_record_that_names_no_issue_is_not_read() { + let records = fixtures::records(fixtures::STATES); + assert_eq!(records.len(), 12, "the captured page changed"); + let read: Vec<(String, StateEvent)> = records.iter().filter_map(state_event).collect(); + assert_eq!(read.len(), 10); + assert_eq!( + records + .iter() + .filter(|r| r["value"]["issue"] == "" && r["value"]["createdAt"].is_null()) + .count(), + 2 + ); + for (_, event) in &read { + assert!(event.instant().is_some(), "every readable record is dated"); + } + } }