From fdfba4c80a672df3262eb251253e3093cec4a960 Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:24:36 -0500 Subject: [PATCH 1/6] refactor(sync): one retry loop, not four MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit push_create_inner, push_put, push_delete and pull_list each carried their own copy of the engine's resilience discipline — refresh-on-401 once, exponential backoff on transient failures, mark offline, give up past the cap. Twenty-five identical lines, four times, differing only in which PdsClient method sat in the middle. That is the kind of duplication that does not stay identical. A change to the backoff, or to what counts as retryable, had four places to land and no way to notice a missed one — on the path where getting it wrong means either hammering a PDS or silently dropping a publish. `with_resilience` takes the call to retry and owns everything around it; the four methods become one line each. It re-invokes `op` per attempt against a client rebuilt from the possibly-rotated session, which is why it is `Fn` and why the record-carrying callers clone. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-sync/src/sync.rs | 111 ++++++++---------------------- 1 file changed, 27 insertions(+), 84 deletions(-) diff --git a/crates/navigator-sync/src/sync.rs b/crates/navigator-sync/src/sync.rs index a5c991ff..faec1f28 100644 --- a/crates/navigator-sync/src/sync.rs +++ b/crates/navigator-sync/src/sync.rs @@ -111,65 +111,16 @@ impl AsyncSync { rkey: &str, record: serde_json::Value, ) -> Result { - let mut refreshed = false; - let mut attempt = 0u32; - loop { - let client = PdsClient::from_session(self.http.clone(), &self.session)?; - match client.put_record(collection, rkey, record.clone()).await { - Ok(r) => { - self.online.store(true, Ordering::Relaxed); - return Ok(r); - } - Err(SyncError::Unauthorized) if !refreshed => { - self.session = refresh(&self.http, &self.session).await?; - self.tokens.save(&self.did, &self.session)?; - refreshed = true; - } - Err(e) if e.is_transient() && attempt < self.policy.max_retries => { - self.online.store(false, Ordering::Relaxed); - tokio::time::sleep(self.policy.backoff(attempt)).await; - attempt += 1; - } - Err(e) => { - if e.is_transient() { - self.online.store(false, Ordering::Relaxed); - } - return Err(e); - } - } - } + let record = &record; + self.with_resilience(|c| async move { c.put_record(collection, rkey, record.clone()).await }) + .await } /// Delete a record at `rkey` (`deleteRecord`) — the orphan-prune path. Same refresh-on-401 + /// transient backoff discipline as [`push_put`](Self::push_put). pub async fn push_delete(&mut self, collection: &str, rkey: &str) -> Result<(), SyncError> { - let mut refreshed = false; - let mut attempt = 0u32; - loop { - let client = PdsClient::from_session(self.http.clone(), &self.session)?; - match client.delete_record(collection, rkey).await { - Ok(()) => { - self.online.store(true, Ordering::Relaxed); - return Ok(()); - } - Err(SyncError::Unauthorized) if !refreshed => { - self.session = refresh(&self.http, &self.session).await?; - self.tokens.save(&self.did, &self.session)?; - refreshed = true; - } - Err(e) if e.is_transient() && attempt < self.policy.max_retries => { - self.online.store(false, Ordering::Relaxed); - tokio::time::sleep(self.policy.backoff(attempt)).await; - attempt += 1; - } - Err(e) => { - if e.is_transient() { - self.online.store(false, Ordering::Relaxed); - } - return Err(e); - } - } - } + self.with_resilience(|c| async move { c.delete_record(collection, rkey).await }) + .await } /// Fetch one page of the account's own records in `collection` (`listRecords`, for a PULL). Same @@ -179,33 +130,8 @@ impl AsyncSync { collection: &str, cursor: Option<&str>, ) -> Result<(Vec, Option), SyncError> { - let mut refreshed = false; - let mut attempt = 0u32; - loop { - let client = PdsClient::from_session(self.http.clone(), &self.session)?; - match client.list_records(collection, cursor).await { - Ok(r) => { - self.online.store(true, Ordering::Relaxed); - return Ok(r); - } - Err(SyncError::Unauthorized) if !refreshed => { - self.session = refresh(&self.http, &self.session).await?; - self.tokens.save(&self.did, &self.session)?; - refreshed = true; - } - Err(e) if e.is_transient() && attempt < self.policy.max_retries => { - self.online.store(false, Ordering::Relaxed); - tokio::time::sleep(self.policy.backoff(attempt)).await; - attempt += 1; - } - Err(e) => { - if e.is_transient() { - self.online.store(false, Ordering::Relaxed); - } - return Err(e); - } - } - } + self.with_resilience(|c| async move { c.list_records(collection, cursor).await }) + .await } async fn push_create_inner( @@ -214,14 +140,31 @@ impl AsyncSync { record: serde_json::Value, rkey: Option<&str>, ) -> Result { + let record = &record; + self.with_resilience(|c| async move { c.create_record(collection, record.clone(), rkey).await }) + .await + } + + /// Run one PDS call under the engine's whole resilience discipline, retrying `op` against a + /// freshly built client until it succeeds or gives up. This is the *only* place the policy + /// lives — every public method above is a one-liner over it, so refresh-on-401, backoff, and + /// the offline flag can never drift apart between the create/put/delete/list paths. + /// + /// `op` is re-invoked per attempt (hence `Fn`, and hence the callers cloning their record), + /// because a retry needs a client rebuilt from the possibly-rotated session. + async fn with_resilience(&mut self, op: F) -> Result + where + F: Fn(PdsClient) -> Fut, + Fut: std::future::Future>, + { let mut refreshed = false; let mut attempt = 0u32; loop { let client = PdsClient::from_session(self.http.clone(), &self.session)?; - match client.create_record(collection, record.clone(), rkey).await { - Ok(r) => { + match op(client).await { + Ok(v) => { self.online.store(true, Ordering::Relaxed); - return Ok(r); + return Ok(v); } // Token expired/revoked: refresh once, persist, and retry immediately. Err(SyncError::Unauthorized) if !refreshed => { From 7e6cfa74c83f5f7a384b601b6675720f11ea77ca Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:25:13 -0500 Subject: [PATCH 2/6] refactor(cli): `?`, for a function that returns an exit code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `navigator ` is a function returning the process exit status, so `?` was unavailable and each fallible step wrote its own four-line `match` to turn an error into a code. Seventy of them had accumulated in one file. Reading a command meant reading past them to find the three or four things it actually does. Two error types reached those matches, and they differ only in whether the message has been printed: the helpers here print their own and hand back a code, `App` returns an AppError nobody has shown the user yet. An `ExitCode` trait is that seam, so `cli_try!` needs one arm and covers both. Sixty-four blocks collapse to one line each. `require_subject` takes the other twelve: the "no subject with identifier" not-found block, copied verbatim wherever a `--subject` command started up. No behaviour change — `report` still exists for the `.map_err(report)?` sites inside the Result-returning helpers, and now just calls through to the same printing. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-ui/src/cli.rs | 437 ++++++++++----------------------- 1 file changed, 125 insertions(+), 312 deletions(-) diff --git a/crates/navigator-ui/src/cli.rs b/crates/navigator-ui/src/cli.rs index f0823a24..dff40ae1 100644 --- a/crates/navigator-ui/src/cli.rs +++ b/crates/navigator-ui/src/cli.rs @@ -16,6 +16,44 @@ use navigator_app::{AnalysisStep, App, DnaType}; use navigator_domain::du_domain::ids::SampleGuid; use navigator_domain::workspace::NewProject; +/// The exit status a failed CLI step ends its command with. +/// +/// Each `navigator ` runs as a function returning the process exit code, so `?` is not +/// available and every fallible step needs its error turned into a code. Two kinds of error reach +/// that point and they differ only in whether the message has been printed yet: the helpers in this +/// module print their own and hand back the code, while [`App`] returns an `AppError` nobody has +/// shown the user. This is the seam between them, so [`cli_try!`] needs only one arm. +trait ExitCode { + fn exit_code(self) -> i32; +} + +impl ExitCode for i32 { + fn exit_code(self) -> i32 { + self + } +} + +impl ExitCode for navigator_app::AppError { + fn exit_code(self) -> i32 { + eprintln!("error: {self}"); + 1 + } +} + +/// Unwrap a CLI step, or end the command with the exit status its failure implies. +/// +/// This is `?` for a function returning `i32` instead of `Result`. It replaced ~50 hand-written +/// four-line `match` blocks, which between them were most of what stood in the way of reading a +/// command as the short sequence of steps it actually is. +macro_rules! cli_try { + ($e:expr) => { + match $e { + Ok(v) => v, + Err(e) => return ExitCode::exit_code(e), + } + }; +} + #[derive(Parser)] #[command( name = "navigator", @@ -527,21 +565,12 @@ pub fn run(command: Command) -> i32 { /// Resolve subjects against the AppView samples API and attach the authoritative INSDC accessions. async fn backfill_accessions(args: AccessionArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let project_id = match resolve_project_filter(&app, args.project.as_ref()).await { - Ok(v) => v, - Err(c) => return c, - }; - let r = match app - .backfill_accessions(project_id, args.apply, args.all, args.limit) - .await - { - Ok(r) => r, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let project_id = cli_try!(resolve_project_filter(&app, args.project.as_ref()).await); + let r = cli_try!( + app.backfill_accessions(project_id, args.apply, args.all, args.limit) + .await + ); if args.json { println!("{}", serde_json::to_string_pretty(&r).unwrap_or_default()); } else { @@ -573,18 +602,9 @@ async fn backfill_accessions(args: AccessionArgs) -> i32 { /// Attach public-catalog external ids derivable from provenance across the workspace (or a project). async fn backfill_catalog_ids(args: CatalogArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let project_id = match resolve_project_filter(&app, args.project.as_ref()).await { - Ok(v) => v, - Err(c) => return c, - }; - let r = match app.backfill_catalog_ids(project_id, args.apply).await { - Ok(r) => r, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let project_id = cli_try!(resolve_project_filter(&app, args.project.as_ref()).await); + let r = cli_try!(app.backfill_catalog_ids(project_id, args.apply).await); if args.json { println!("{}", serde_json::to_string_pretty(&r).unwrap_or_default()); } else { @@ -609,10 +629,7 @@ async fn backfill_catalog_ids(args: CatalogArgs) -> i32 { /// Sign in via OAuth (browser + loopback callback) and persist the session for later subcommands. async fn login(args: LoginArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); eprintln!("Opening browser to sign in as {}…", args.handle); match app.login(&args.handle).await { Ok(did) => { @@ -625,10 +642,7 @@ async fn login(args: LoginArgs) -> i32 { /// Delete orphaned alignment records from the signed-in account's PDS (dry-run unless `--apply`). async fn prune_orphans(args: PruneArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); let report = match app.prune_orphan_alignments(args.apply).await { Ok(r) => r, Err(navigator_app::AppError::NotAuthenticated) => { @@ -663,19 +677,10 @@ async fn prune_orphans(args: PruneArgs) -> i32 { /// Backfill the standardized-test-label read-profile fields across the workspace (or one project). async fn backfill_profiles(args: BackfillArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let project_id = match resolve_project_filter(&app, args.project.as_ref()).await { - Ok(v) => v, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let project_id = cli_try!(resolve_project_filter(&app, args.project.as_ref()).await); - let r = match app.backfill_read_profiles(project_id, args.rescan).await { - Ok(r) => r, - Err(e) => return report(e), - }; + let r = cli_try!(app.backfill_read_profiles(project_id, args.rescan).await); if args.json { println!("{}", serde_json::to_string_pretty(&r).unwrap_or_default()); @@ -710,20 +715,11 @@ async fn rebuild_signatures(args: RebuildArgs) -> i32 { eprintln!("error: --dry-run and --include-unknown only apply with --stale-tree"); return 2; } - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); - let project_id = match resolve_project_filter(&app, args.project.as_ref()).await { - Ok(v) => v, - Err(c) => return c, - }; + let project_id = cli_try!(resolve_project_filter(&app, args.project.as_ref()).await); - let bios = match app.list_all_biosamples().await { - Ok(v) => v, - Err(e) => return report(e), - }; + let bios = cli_try!(app.list_all_biosamples().await); // The staleness selector: which subjects were placed against a tree other than today's. Held as // guids rather than folded into `wanted` because that set is matched against donor identifiers @@ -732,14 +728,8 @@ async fn rebuild_signatures(args: RebuildArgs) -> i32 { // Two independent symptoms of the same thing, unioned: a *source call* stamped with another // tree, and a *derived consensus* naming a branch this tree does not carry. The second can // be true while every call beneath it is current, so neither selector subsumes the other. - let by_fingerprint = match app.subjects_placed_against_another_tree(args.include_unknown).await { - Ok(v) => v, - Err(e) => return report(e), - }; - let off_tree = match app.subjects_labelled_off_tree().await { - Ok(v) => v, - Err(e) => return report(e), - }; + let by_fingerprint = cli_try!(app.subjects_placed_against_another_tree(args.include_unknown).await); + let off_tree = cli_try!(app.subjects_labelled_off_tree().await); let mut set: std::collections::HashSet = by_fingerprint.iter().copied().collect(); let also = off_tree.iter().filter(|g| !set.contains(g)).count(); set.extend(off_tree); @@ -863,20 +853,11 @@ async fn rebuild_signatures(args: RebuildArgs) -> i32 { } async fn reingest_external(args: ReingestArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); - let project_id = match resolve_project_filter(&app, args.project.as_ref()).await { - Ok(v) => v, - Err(c) => return c, - }; + let project_id = cli_try!(resolve_project_filter(&app, args.project.as_ref()).await); - let bios = match app.list_all_biosamples().await { - Ok(v) => v, - Err(e) => return report(e), - }; + let bios = cli_try!(app.list_all_biosamples().await); let (mut subjects, mut y_total, mut mt_total, mut failed) = (0usize, 0usize, 0usize, 0usize); for b in &bios { @@ -909,22 +890,9 @@ async fn reingest_external(args: ReingestArgs) -> i32 { } async fn compare_callers(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; - let runs = match app.list_sequence_runs(guid).await { - Ok(v) => v, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); + let runs = cli_try!(app.list_sequence_runs(guid).await); let mut alns = Vec::new(); for r in &runs { match app.list_alignments(r.id).await { @@ -970,10 +938,7 @@ async fn compare_callers(args: ShowArgs) -> i32 { /// Time the per-alignment analysis steps (the GUI Full Analysis path) to profile where time goes. async fn analyze(args: AnalyzeArgs) -> i32 { use std::time::Instant; - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); let id = args.alignment; // The step list comes from `App::plan_full_analysis` — the same one the GUI's Full Analysis @@ -1108,12 +1073,26 @@ async fn resolve_project_filter(app: &App, name: Option<&String>) -> Result Result, i32> { let all = app.list_all_biosamples().await.map_err(report)?; Ok(all.into_iter().find(|b| b.donor_identifier == donor).map(|b| b.guid)) } +/// The subject with this exact donor identifier — the opening move of every `--subject` command. +/// A missing subject is a plain user error, so the message is printed here and the caller just +/// propagates the code. +async fn require_subject(app: &App, donor: &str) -> Result { + match find_subject(app, donor).await? { + Some(g) => Ok(g), + None => { + eprintln!("error: no subject with identifier \"{donor}\""); + Err(1) + } + } +} + /// Find a project id by exact name, or create it. async fn find_or_create_project(app: &App, name: &str) -> Result { let overview = app.project_overview().await.map_err(report)?; @@ -1131,16 +1110,15 @@ async fn find_or_create_project(app: &App, name: &str) -> Result { Ok(p.id) } +/// Print an [`App`] error and yield the failure exit code. Kept as a free function for the +/// `.map_err(report)?` sites inside the `Result`-returning helpers; steps in a command body use +/// [`cli_try!`] instead. fn report(e: navigator_app::AppError) -> i32 { - eprintln!("error: {e}"); - 1 + e.exit_code() } async fn ingest(args: IngestArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); // Resolve the project first so subject creation can attach to it. let project_id = match &args.project { @@ -1318,14 +1296,8 @@ async fn ingest(args: IngestArgs) -> i32 { } async fn subjects(args: ProbeArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let bios = match app.list_all_biosamples().await { - Ok(v) => v, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let bios = cli_try!(app.list_all_biosamples().await); let overview = app.project_overview().await.unwrap_or_default(); let project_name = |id: Option| -> Option { id.and_then(|pid| { @@ -1391,18 +1363,8 @@ async fn subjects(args: ProbeArgs) -> i32 { } async fn debug_place(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); match app.debug_y_placement(guid).await { Ok(trace) => { println!("{trace}"); @@ -1416,18 +1378,8 @@ async fn debug_place(args: ShowArgs) -> i32 { } async fn debug_descent(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); match app.debug_y_descent(guid).await { Ok(trace) => { println!("{trace}"); @@ -1441,10 +1393,7 @@ async fn debug_descent(args: ShowArgs) -> i32 { } async fn debug_calls(args: DebugCallsArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); // Resolve the target alignment: explicit --alignment wins; otherwise pick from the subject // (prefer a CHM13/HiFi alignment, else the first). let alignment_id = match args.alignment { @@ -1454,14 +1403,7 @@ async fn debug_calls(args: DebugCallsArgs) -> i32 { eprintln!("error: provide --alignment or --subject "); return 2; }; - let guid = match find_subject(&app, subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{subject}\""); - return 1; - } - Err(c) => return c, - }; + let guid = cli_try!(require_subject(&app, subject).await); match app.pick_y_debug_alignment(guid).await { Ok(Some(id)) => id, Ok(None) => { @@ -1493,10 +1435,7 @@ async fn private_y_batch(app: &App, project: &str, force: bool) -> i32 { let Ok(Some(pid)) = resolve_project_filter(app, Some(&project.to_string())).await else { return 1; }; - let members = match app.list_biosamples(pid).await { - Ok(v) => v, - Err(e) => return report(e), - }; + let members = cli_try!(app.list_biosamples(pid).await); let (mut done, mut skipped, mut failed, mut no_aln) = (0usize, 0usize, 0usize, 0usize); let mut missing = 0usize; let (mut novel, mut publishable) = (0usize, 0usize); @@ -1596,10 +1535,7 @@ async fn publish_origins(args: PublishOriginsArgs) -> i32 { return 2; } }; - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); let report = match app.publish_ancestral_origins(lineage, !args.apply).await { Ok(r) => r, Err(e) => { @@ -1624,10 +1560,7 @@ async fn publish_origins(args: PublishOriginsArgs) -> i32 { } async fn private_y(args: PrivateYArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); if let Some(project) = args.project.as_deref() { return private_y_batch(&app, project, args.force).await; } @@ -1638,14 +1571,7 @@ async fn private_y(args: PrivateYArgs) -> i32 { eprintln!("error: provide --alignment or --subject "); return 2; }; - let guid = match find_subject(&app, subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{subject}\""); - return 1; - } - Err(c) => return c, - }; + let guid = cli_try!(require_subject(&app, subject).await); match app.pick_y_debug_alignment(guid).await { Ok(Some(id)) => id, Ok(None) => { @@ -1724,10 +1650,7 @@ async fn private_y(args: PrivateYArgs) -> i32 { /// Per-marker branch report over a Y/mtDNA node's descendant subtree — table / `--tsv` / `--json`. async fn branch_report(args: BranchReportArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); let dna = match args.tree.to_ascii_lowercase().as_str() { "y" | "ydna" | "y-dna" => DnaType::Y, "mt" | "mtdna" | "mt-dna" => DnaType::Mt, @@ -1743,14 +1666,7 @@ async fn branch_report(args: BranchReportArgs) -> i32 { eprintln!("error: provide --alignment or --subject "); return 2; }; - let guid = match find_subject(&app, subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{subject}\""); - return 1; - } - Err(c) => return c, - }; + let guid = cli_try!(require_subject(&app, subject).await); // Y and mt want different alignments — a Big-Y run carries no chrM reads. match app.pick_alignment_for(guid, dna).await { Ok(Some(id)) => id, @@ -1859,22 +1775,9 @@ async fn branch_report(args: BranchReportArgs) -> i32 { /// Deep-ancestry stability report — see [`navigator_app::App::ancient_ancestry_stability`]. async fn debug_ancient(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; - let rows = match app.ancient_ancestry_stability(guid).await { - Ok(r) => r, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); + let rows = cli_try!(app.ancient_ancestry_stability(guid).await); if args.json { println!("{}", serde_json::to_string_pretty(&rows).unwrap_or_default()); return 0; @@ -1909,22 +1812,9 @@ async fn debug_ancient(args: ShowArgs) -> i32 { /// Tier B archaic segments — see [`navigator_app::App::call_archaic_segments_for_subject`]. async fn archaic_segments(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; - let r = match app.call_archaic_segments_for_subject(guid).await { - Ok(r) => r, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); + let r = cli_try!(app.call_archaic_segments_for_subject(guid).await); if args.json { println!("{}", serde_json::to_string_pretty(&r).unwrap_or_default()); return 0; @@ -1942,27 +1832,11 @@ async fn archaic_segments(args: ShowArgs) -> i32 { /// Archaic (Neanderthal / Denisovan) Tier-A marker count — see /// [`navigator_app::App::estimate_archaic_from_consensus`]. async fn archaic(args: ArchaicArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); let r = match args.alignment { - Some(aln) => match app.archaic_for_alignment(guid, aln).await { - Ok(r) => r, - Err(e) => return report(e), - }, - None => match app.estimate_archaic_from_consensus(guid).await { - Ok(r) => r, - Err(e) => return report(e), - }, + Some(aln) => cli_try!(app.archaic_for_alignment(guid, aln).await), + None => cli_try!(app.estimate_archaic_from_consensus(guid).await), }; if args.json { println!("{}", serde_json::to_string_pretty(&r).unwrap_or_default()); @@ -1995,22 +1869,9 @@ async fn archaic(args: ArchaicArgs) -> i32 { /// Deep (ancient) ancestry via qpAdm — see [`navigator_app::App::estimate_deep_ancestry`]. async fn deep_ancestry(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; - let result = match app.estimate_deep_ancestry(guid).await { - Ok(r) => r, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); + let result = cli_try!(app.estimate_deep_ancestry(guid).await); match result { None => { println!( @@ -2039,18 +1900,8 @@ async fn deep_ancestry(args: ShowArgs) -> i32 { /// Panel batch-process mode — see [`navigator_app::App::genotype_panel_for_alignment`]. async fn genotype_panel(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); // Best-callable alignment + chips/VCFs (which fold in during the refresh) — one decode per subject. match app.genotype_panel_for_subject(guid).await { Ok(Some((aln, sites))) => { @@ -2066,18 +1917,8 @@ async fn genotype_panel(args: ShowArgs) -> i32 { } async fn show(args: ShowArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let guid = match find_subject(&app, &args.subject).await { - Ok(Some(g)) => g, - Ok(None) => { - eprintln!("error: no subject with identifier \"{}\"", args.subject); - return 1; - } - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let guid = cli_try!(require_subject(&app, &args.subject).await); let runs = app.list_sequence_runs(guid).await.unwrap_or_default(); let strs = app.list_str_profiles(guid).await.unwrap_or_default(); @@ -2206,18 +2047,9 @@ async fn doctor(args: DoctorArgs) -> i32 { } } } else { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let id = match resolve_alignment(&app, args.subject.as_deref(), args.alignment).await { - Ok(id) => id, - Err(c) => return c, - }; - match app.diagnose_alignment(id).await { - Ok(r) => r, - Err(e) => return report(e), - } + let app = cli_try!(open(args.db).await); + let id = cli_try!(resolve_alignment(&app, args.subject.as_deref(), args.alignment).await); + cli_try!(app.diagnose_alignment(id).await) }; if args.json { @@ -2266,18 +2098,12 @@ async fn resolve_alignment(app: &App, subject: Option<&str>, explicit: Option i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let alignment_id = match resolve_alignment(&app, args.subject.as_deref(), args.alignment).await { - Ok(id) => id, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); + let alignment_id = cli_try!(resolve_alignment(&app, args.subject.as_deref(), args.alignment).await); let scope = args.contig.clone().unwrap_or_else(|| "whole genome".into()); eprintln!("calling de-novo diploid variants on alignment #{alignment_id} ({scope})…"); - let vcf = match args.contig { + let vcf = cli_try!(match args.contig { Some(contig) => { app.diploid_vcf(alignment_id, contig, navigator_app::CancelToken::none()) .await @@ -2286,11 +2112,7 @@ async fn call(args: CallArgs) -> i32 { app.diploid_vcf_genome(alignment_id, navigator_app::CancelToken::none()) .await } - }; - let vcf = match vcf { - Ok(v) => v, - Err(e) => return report(e), - }; + }); // Summary to stderr (records, of which multiallelic) so a redirected stdout stays pure VCF. let records: Vec<&str> = vcf.lines().filter(|l| !l.starts_with('#')).collect(); @@ -2314,10 +2136,7 @@ async fn call(args: CallArgs) -> i32 { } async fn lift_vcf(args: LiftVcfArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; + let app = cli_try!(open(args.db).await); let source = match args .from .clone() @@ -2368,14 +2187,8 @@ async fn lift_vcf(args: LiftVcfArgs) -> i32 { } async fn projects(args: ProbeArgs) -> i32 { - let app = match open(args.db).await { - Ok(a) => a, - Err(c) => return c, - }; - let overview = match app.project_overview().await { - Ok(v) => v, - Err(e) => return report(e), - }; + let app = cli_try!(open(args.db).await); + let overview = cli_try!(app.project_overview().await); if args.json { let arr: Vec<_> = overview .iter() From 0e5538eccd41218c8fc94a519595500048ba0999 Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:26:07 -0500 Subject: [PATCH 3/6] refactor(ui): the widget idioms every tab was re-deriving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small shapes had been copied around the tabs rather than named: - the "does this alignment have a file we can walk" lookup, five times across two files, each spelling out the same find-then-map-then-unwrap_or(false); - the cancel button, four times, including its non-obvious detail — it disables itself once clicked, because cancellation is cooperative and a live button made a working cancel look ignored. That reasoning was written out in one copy and absent from the other three; - the destructive-action button (filled DANGER, white label), five times. They move to chrome.rs beside `tr`, where the next tab can find them. The analysis modal keeps its own cancel: it defers the click out of the closure and adds a spinner, so it is a genuine variant rather than another copy. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-ui/src/ui/central.rs | 8 +----- crates/navigator-ui/src/ui/chrome.rs | 36 +++++++++++++++++++++++++++ crates/navigator-ui/src/ui/detail.rs | 12 +-------- crates/navigator-ui/src/ui/ibd.rs | 26 +++---------------- crates/navigator-ui/src/ui/modals.rs | 32 +++--------------------- crates/navigator-ui/src/ui/sources.rs | 33 +++--------------------- 6 files changed, 49 insertions(+), 98 deletions(-) diff --git a/crates/navigator-ui/src/ui/central.rs b/crates/navigator-ui/src/ui/central.rs index f05f681d..e7723e66 100644 --- a/crates/navigator-ui/src/ui/central.rs +++ b/crates/navigator-ui/src/ui/central.rs @@ -733,13 +733,7 @@ impl NavigatorApp { ui.label(egui::RichText::new(bio.sex.as_deref().unwrap_or("Unknown")).weak()); }); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - if ui - .add( - egui::Button::new(egui::RichText::new(self.tr("common.delete")).color(egui::Color32::WHITE)) - .fill(DANGER), - ) - .clicked() - { + if self.danger_button(ui, "common.delete") { self.confirm_delete = Some(guid); } ui.menu_button(self.tr("common.clearData"), |ui| { diff --git a/crates/navigator-ui/src/ui/chrome.rs b/crates/navigator-ui/src/ui/chrome.rs index 286043ea..0b3dd5e6 100644 --- a/crates/navigator-ui/src/ui/chrome.rs +++ b/crates/navigator-ui/src/ui/chrome.rs @@ -39,6 +39,42 @@ impl NavigatorApp { crate::i18n::tr(self.lang, key) } + /// Whether the loaded alignment has a BAM/CRAM path on record — the gate on every control that + /// would have to walk reads. An alignment we can't find is treated as having no file, so a + /// stale id disables the button rather than launching a walk that would fail. + pub(crate) fn alignment_has_bam(&self, alignment_id: i64) -> bool { + self.alignments + .iter() + .any(|a| a.id == alignment_id && a.bam_path.is_some()) + } + + /// The cancel control shown beside a running analysis: it disables itself once clicked. + /// + /// The disable matters — cancellation is cooperative and doesn't take effect until the walk + /// reaches its next check, so a live button invited repeat clicks and made a working cancel + /// look ignored. Every place that can start a walk needs the same behaviour, so it lives here + /// rather than being re-derived per tab. + pub(crate) fn cancel_button(&mut self, ui: &mut egui::Ui) { + let requested = self.cancelling; + let label = if requested { + self.tr("analysis.cancelling") + } else { + self.tr("common.cancel") + }; + if ui.add_enabled(!requested, egui::Button::new(label)).clicked() { + self.cancelling = true; + let _ = self.tx.send(Command::CancelAnalysis); + self.status = self.tr("analysis.cancelling").to_string(); + } + } + + /// A destructive-action button (filled [`DANGER`], white label) — the visual promise that this + /// one is not like the others. Returns whether it was clicked. + pub(crate) fn danger_button(&self, ui: &mut egui::Ui, key: &'static str) -> bool { + ui.add(egui::Button::new(egui::RichText::new(self.tr(key)).color(egui::Color32::WHITE)).fill(DANGER)) + .clicked() + } + /// The loaded subject for `guid`, from whichever list holds it: `all_biosamples` (the workspace /// table) or `samples` (the current project's members). pub(crate) fn find_subject(&self, guid: SampleGuid) -> Option<&Biosample> { diff --git a/crates/navigator-ui/src/ui/detail.rs b/crates/navigator-ui/src/ui/detail.rs index d82a177b..4728b01c 100644 --- a/crates/navigator-ui/src/ui/detail.rs +++ b/crates/navigator-ui/src/ui/detail.rs @@ -1771,17 +1771,7 @@ impl NavigatorApp { } } if self.analyzing { - let requested = self.cancelling; - let label = if requested { - self.tr("analysis.cancelling") - } else { - self.tr("common.cancel") - }; - if ui.add_enabled(!requested, egui::Button::new(label)).clicked() { - self.cancelling = true; - let _ = self.tx.send(Command::CancelAnalysis); - self.status = self.tr("analysis.cancelling").to_string(); - } + self.cancel_button(ui); } if ui.button(self.tr("projects.exportCsv")).clicked() { let csv = navigator_app::report_csv(&self.project_report); diff --git a/crates/navigator-ui/src/ui/ibd.rs b/crates/navigator-ui/src/ui/ibd.rs index 623c8d31..f3ef8dbd 100644 --- a/crates/navigator-ui/src/ui/ibd.rs +++ b/crates/navigator-ui/src/ui/ibd.rs @@ -337,12 +337,7 @@ impl NavigatorApp { /// mtDNA haplogroup assigned directly from the alignment's chrM — the standalone counterpart /// to the Y-DNA section's "Assign Y haplogroup". pub(crate) fn mt_haplogroup_section(&mut self, ui: &mut egui::Ui, alignment_id: i64) { - let has_bam = self - .alignments - .iter() - .find(|a| a.id == alignment_id) - .map(|a| a.bam_path.is_some()) - .unwrap_or(false); + let has_bam = self.alignment_has_bam(alignment_id); ui.horizontal(|ui| { if ui .add_enabled(has_bam, egui::Button::new(self.tr("btn.assignMt"))) @@ -367,12 +362,7 @@ impl NavigatorApp { /// De-novo haploid SNP calls for a specific `contig` (chrY on the Y-DNA tab, chrM on mtDNA). pub(crate) fn denovo_section(&mut self, ui: &mut egui::Ui, alignment_id: i64, contig: &str) { // Reference is resolved from the build on demand, so only the BAM is required. - let has_bam = self - .alignments - .iter() - .find(|a| a.id == alignment_id) - .map(|a| a.bam_path.is_some()) - .unwrap_or(false); + let has_bam = self.alignment_has_bam(alignment_id); ui.horizontal(|ui| { let ready = has_bam && !self.running_denovo; @@ -388,17 +378,7 @@ impl NavigatorApp { } if self.running_denovo { ui.spinner(); - let requested = self.cancelling; - let label = if requested { - self.tr("analysis.cancelling") - } else { - self.tr("common.cancel") - }; - if ui.add_enabled(!requested, egui::Button::new(label)).clicked() { - self.cancelling = true; - let _ = self.tx.send(Command::CancelAnalysis); - self.status = self.tr("analysis.cancelling").to_string(); - } + self.cancel_button(ui); } if !has_bam { ui.label(egui::RichText::new("(no BAM/CRAM recorded)").weak()); diff --git a/crates/navigator-ui/src/ui/modals.rs b/crates/navigator-ui/src/ui/modals.rs index 33567dde..25825403 100644 --- a/crates/navigator-ui/src/ui/modals.rs +++ b/crates/navigator-ui/src/ui/modals.rs @@ -1009,13 +1009,7 @@ impl NavigatorApp { ui.label(egui::RichText::new(self.tr("delete.note")).weak().small()); ui.add_space(12.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - if ui - .add( - egui::Button::new(egui::RichText::new(self.tr("common.delete")).color(egui::Color32::WHITE)) - .fill(DANGER), - ) - .clicked() - { + if self.danger_button(ui, "common.delete") { let _ = self.tx.send(Command::DeleteBiosample(guid)); if self.selected_sample == Some(guid) { self.selected_sample = None; @@ -1049,13 +1043,7 @@ impl NavigatorApp { ui.label(egui::RichText::new(self.tr("clear.note")).weak().small()); ui.add_space(12.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - if ui - .add( - egui::Button::new(egui::RichText::new(self.tr("common.clearData")).color(egui::Color32::WHITE)) - .fill(DANGER), - ) - .clicked() - { + if self.danger_button(ui, "common.clearData") { let _ = self.tx.send(Command::ClearBiosampleData(guid)); close = true; } @@ -1247,13 +1235,7 @@ impl NavigatorApp { ui.label(egui::RichText::new(self.tr("delete.dataNote")).weak().small()); ui.add_space(12.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - if ui - .add( - egui::Button::new(egui::RichText::new(self.tr("common.delete")).color(egui::Color32::WHITE)) - .fill(DANGER), - ) - .clicked() - { + if self.danger_button(ui, "common.delete") { let _ = self.tx.send(target.command()); close = true; } @@ -1587,13 +1569,7 @@ impl NavigatorApp { ui.label(egui::RichText::new(self.tr("editProject.deleteNote")).weak().small()); ui.add_space(12.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - if ui - .add( - egui::Button::new(egui::RichText::new(self.tr("common.delete")).color(egui::Color32::WHITE)) - .fill(DANGER), - ) - .clicked() - { + if self.danger_button(ui, "common.delete") { let _ = self.tx.send(Command::DeleteProject(id)); if self.selected_project == Some(id) { self.selected_project = None; diff --git a/crates/navigator-ui/src/ui/sources.rs b/crates/navigator-ui/src/ui/sources.rs index e5ac3002..cd2b9943 100644 --- a/crates/navigator-ui/src/ui/sources.rs +++ b/crates/navigator-ui/src/ui/sources.rs @@ -322,12 +322,7 @@ impl NavigatorApp { /// Inferred sex + read-level QC metrics for a single alignment. pub(crate) fn sex_metrics_section(&mut self, ui: &mut egui::Ui, alignment_id: i64) { - let has_bam = self - .alignments - .iter() - .find(|a| a.id == alignment_id) - .map(|a| a.bam_path.is_some()) - .unwrap_or(false); + let has_bam = self.alignment_has_bam(alignment_id); ui.horizontal(|ui| { if ui @@ -363,17 +358,7 @@ impl NavigatorApp { // SV walks every read in the file, so it is one of the runs a user most wants to stop. // It had no cancel control at all until the walkers became cancellable. if self.running_sv { - let requested = self.cancelling; - let label = if requested { - self.tr("analysis.cancelling") - } else { - self.tr("common.cancel") - }; - if ui.add_enabled(!requested, egui::Button::new(label)).clicked() { - self.cancelling = true; - let _ = self.tx.send(Command::CancelAnalysis); - self.status = self.tr("analysis.cancelling").to_string(); - } + self.cancel_button(ui); } if !has_bam { ui.label(self.tr("hint.noBamPath")); @@ -620,12 +605,7 @@ impl NavigatorApp { /// mtDNA heteroplasmy scan for an alignment (chrM pileup → mixed positions). Results /// feed the mtDNA reconciliation record's heteroplasmy observations. pub(crate) fn heteroplasmy_section(&mut self, ui: &mut egui::Ui, alignment_id: i64) { - let has_bam = self - .alignments - .iter() - .find(|a| a.id == alignment_id) - .map(|a| a.bam_path.is_some()) - .unwrap_or(false); + let has_bam = self.alignment_has_bam(alignment_id); ui.horizontal(|ui| { if ui @@ -663,12 +643,7 @@ impl NavigatorApp { /// Y-haplogroup assignment for an alignment (calls chrY tree positions; FTDNA tree). pub(crate) fn y_haplogroup_section(&mut self, ui: &mut egui::Ui, alignment_id: i64) { - let has_bam = self - .alignments - .iter() - .find(|a| a.id == alignment_id) - .map(|a| a.bam_path.is_some()) - .unwrap_or(false); + let has_bam = self.alignment_has_bam(alignment_id); ui.horizontal(|ui| { if ui From ed44273f7e026e74c4fcf61f118603bbcf45d3dd Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:26:55 -0500 Subject: [PATCH 4/6] refactor(domain): constructors for the workspace aggregates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Biosample, SequenceRun and Alignment are mostly Option fields that are None until some later pass populates them — the struct docs say so outright. Every construction site still had to write the whole column of `None`s, 91 of them across the workspace, and the two or three fields that carried the actual intent were buried in the middle of it. Each type gets a `new` taking exactly the fields with no sensible empty value. Sites that know more say so with functional-update syntax, which puts the interesting fields first and the defaults behind a `..`: an alignment with a BAM path now reads as one. Left alone deliberately: the three row-to-domain mappers in the store. There, every field is assigned from a column and the exhaustive literal is the point — adding a column should fail to compile until the mapping handles it. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-app/src/commands.rs | 5 +- crates/navigator-app/src/import_unified.rs | 36 +- crates/navigator-app/src/lib.rs | 9 +- crates/navigator-app/src/publish.rs | 21 +- crates/navigator-app/tests/app.rs | 544 ++---------------- crates/navigator-domain/src/workspace.rs | 104 ++++ crates/navigator-store/src/artifact.rs | 57 +- .../navigator-store/src/biosample_project.rs | 17 +- crates/navigator-store/src/external_id.rs | 17 +- crates/navigator-store/src/ftdna_member.rs | 17 +- crates/navigator-store/src/mdka.rs | 17 +- crates/navigator-store/tests/store.rs | 217 ++----- crates/navigator-ui/src/ui/detail.rs | 15 +- crates/navigator-ui/src/worker.rs | 77 +-- 14 files changed, 237 insertions(+), 916 deletions(-) diff --git a/crates/navigator-app/src/commands.rs b/crates/navigator-app/src/commands.rs index f235ece6..c44bfca1 100644 --- a/crates/navigator-app/src/commands.rs +++ b/crates/navigator-app/src/commands.rs @@ -67,13 +67,10 @@ impl App { } } let b = Biosample { - guid: SampleGuid(Uuid::new_v4()), sample_accession, - donor_identifier: donor_identifier.into(), - description: None, - center_name: None, sex, project_id, + ..Biosample::new(SampleGuid(Uuid::new_v4()), donor_identifier) }; biosample::create(self.store.pool(), &b).await?; Ok(b) diff --git a/crates/navigator-app/src/import_unified.rs b/crates/navigator-app/src/import_unified.rs index 251c8789..783f53bf 100644 --- a/crates/navigator-app/src/import_unified.rs +++ b/crates/navigator-app/src/import_unified.rs @@ -128,15 +128,9 @@ impl App { let run = self .record_sequence_run(NewSequenceRun { - biosample_guid, - platform_name, instrument_model, - test_type, library_layout: stats.as_ref().and_then(|s| s.library_layout.clone()), - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, + ..NewSequenceRun::new(biosample_guid, platform_name, test_type) }) .await?; @@ -368,18 +362,8 @@ impl App { { Some(r) => r, None => { - self.record_sequence_run(NewSequenceRun { - biosample_guid, - platform_name: "UNKNOWN".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) - .await? + self.record_sequence_run(NewSequenceRun::new(biosample_guid, "UNKNOWN", "WGS")) + .await? } }; @@ -775,18 +759,8 @@ impl App { { Some(r) => r, None => { - self.record_sequence_run(NewSequenceRun { - biosample_guid: biosample.guid, - platform_name: "UNKNOWN".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) - .await? + self.record_sequence_run(NewSequenceRun::new(biosample.guid, "UNKNOWN", "WGS")) + .await? } }; diff --git a/crates/navigator-app/src/lib.rs b/crates/navigator-app/src/lib.rs index 3f3ad913..a9016d3a 100644 --- a/crates/navigator-app/src/lib.rs +++ b/crates/navigator-app/src/lib.rs @@ -4092,15 +4092,8 @@ mod publish_tests { let b = app.add_biosample(None, "S1", None, None).await.unwrap(); let run = app .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), instrument_model: Some("NovaSeq".into()), - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, + ..NewSequenceRun::new(b.guid, "ILLUMINA", "WGS") }) .await .unwrap(); diff --git a/crates/navigator-app/src/publish.rs b/crates/navigator-app/src/publish.rs index fbff5bea..4c3a713f 100644 --- a/crates/navigator-app/src/publish.rs +++ b/crates/navigator-app/src/publish.rs @@ -486,29 +486,12 @@ mod tests { async fn alignment_with_test_type(app: &App, test_type: &str) -> i64 { let b = app.add_biosample(None, "yscoped", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: test_type.into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", test_type)) .await .unwrap(); app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "synthetic".into(), - variant_caller: None, bam_path: Some("/nonexistent.cram".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "synthetic") }) .await .unwrap() diff --git a/crates/navigator-app/tests/app.rs b/crates/navigator-app/tests/app.rs index 605887c4..ecf5be19 100644 --- a/crates/navigator-app/tests/app.rs +++ b/crates/navigator-app/tests/app.rs @@ -388,17 +388,7 @@ async fn validate_hg002_haplogroups() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app @@ -525,17 +515,7 @@ async fn validate_gfx_chm13_haplogroups() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "PACBIO".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "PACBIO", "WGS")) .await .unwrap(); // mt now needs the CHM13 reference (to self-generate the rCRS↔chrM map): resolve it @@ -649,17 +629,7 @@ async fn validate_gfx_decodingus_y() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "PACBIO".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "PACBIO", "WGS")) .await .unwrap(); let aln = app @@ -720,17 +690,7 @@ async fn gvcf_y_placement_smoke() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app @@ -796,30 +756,14 @@ async fn gvcf_fast_path_matches_cram_walk() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa-mem".into(), - variant_caller: None, bam_path: Some(cram), reference_path: Some(reference), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "bwa-mem") }) .await .unwrap() @@ -872,30 +816,13 @@ async fn analysis_provenance_roundtrips_and_defaults_full_walk() { let app = app().await; let b = app.add_biosample(None, "PROV", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "x".into(), - variant_caller: None, bam_path: Some("/x.cram".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "x") }) .await .unwrap() @@ -935,30 +862,13 @@ async fn save_analysis_no_downgrade_keeps_the_fuller_result() { let app = app().await; let b = app.add_biosample(None, "NODG", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "x".into(), - variant_caller: None, bam_path: Some("/x.cram".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "x") }) .await .unwrap() @@ -1093,30 +1003,14 @@ async fn assign_haplogroup_from_alignment_calls_and_ranks() { let dir = fixtures(); let b = app.add_biosample(None, "HG002", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chrM".into(), - aligner: "synthetic".into(), - variant_caller: None, bam_path: Some(dir.join("coverage.bam").to_string_lossy().into_owned()), reference_path: Some(dir.join("ref.fa").to_string_lossy().into_owned()), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chrM", "synthetic") }) .await .unwrap() @@ -1248,33 +1142,13 @@ async fn add_data_imports_completegenomics_master_var() { async fn alignment_id(app: &App) -> i64 { let b = app.add_biosample(None, "HG002", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); - app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chrM-fixture".into(), - aligner: "synthetic".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }) - .await - .unwrap() - .id + app.record_alignment(NewAlignment::new(run.id, "chrM-fixture", "synthetic")) + .await + .unwrap() + .id } #[tokio::test] @@ -1305,30 +1179,17 @@ async fn command_flow_and_overview() { // chain a run + alignment off the first sample let run = app .record_sequence_run(NewSequenceRun { - biosample_guid: b1.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), library_layout: Some("PAIRED".into()), total_reads: Some(8_000_000), pf_reads_aligned: Some(7_956_881), mean_read_length: Some(148.0), mean_insert_size: Some(580.7), + ..NewSequenceRun::new(b1.guid, "ILLUMINA", "WGS") }) .await .unwrap(); let aln = app - .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }) + .record_alignment(NewAlignment::new(run.id, "chm13v2.0", "bwa")) .await .unwrap(); assert_eq!(aln.sequence_run_id, run.id); @@ -1355,31 +1216,11 @@ async fn typed_analysis_artifact_round_trips_and_versions() { let app = app().await; let b = app.add_biosample(None, "HG002", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app - .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }) + .record_alignment(NewAlignment::new(run.id, "chm13v2.0", "bwa")) .await .unwrap(); @@ -1480,30 +1321,13 @@ async fn run_denovo_caller_persists_snp_calls() { async fn diploid_alignment(app: &App) -> i64 { let b = app.add_biosample(None, "diploid", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let bam = fixtures().join("diploid.bam").to_string_lossy().into_owned(); app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chr1".into(), - aligner: "synthetic".into(), - variant_caller: None, bam_path: Some(bam), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chr1", "synthetic") }) .await .unwrap() @@ -1920,17 +1744,7 @@ async fn assign_y_haplogroup_lifts_grch38_tree_onto_chm13_alignment() { let dir = fixtures(); let b = app.add_biosample(None, "HG002", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app @@ -2003,29 +1817,13 @@ async fn analyze_project_runs_coverage_and_attempts_y_per_sample() { .unwrap(); let b = app.add_biosample(Some(p.id), "S1", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "X".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "X", "WGS")) .await .unwrap(); app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "x".into(), - variant_caller: None, bam_path: Some(dir.join("coverage.cram").to_string_lossy().into_owned()), reference_path: Some(dir.join("ref.fa").to_string_lossy().into_owned()), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "x") }) .await .unwrap(); @@ -2204,29 +2002,13 @@ async fn compare_mt_grch38_vs_chm13() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "X".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "X", "WGS")) .await .unwrap(); app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: build.into(), - aligner: "x".into(), - variant_caller: None, bam_path: Some(bam), reference_path: reference, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, build, "x") }) .await .unwrap() @@ -2304,30 +2086,13 @@ async fn sex_and_read_metrics_persist_and_reload() { let app = app().await; let b = app.add_biosample(None, "sx", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "synthetic".into(), - variant_caller: None, bam_path: Some(fixtures().join("sex.bam").to_string_lossy().into_owned()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "synthetic") }) .await .unwrap() @@ -2353,30 +2118,14 @@ async fn gfx_sex_is_male() { let app = app().await; let b = app.add_biosample(None, "GFX0457637", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "PACBIO_SMRT".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "PACBIO_SMRT", "WGS")) .await .unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "pbmm2".into(), - variant_caller: None, bam_path: Some(bam), reference_path: std::env::var("GFX_CHM13_REF").ok(), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "pbmm2") }) .await .unwrap() @@ -2399,17 +2148,7 @@ async fn cached_artifact_invalidated_when_source_file_changes() { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); @@ -2418,15 +2157,8 @@ async fn cached_artifact_invalidated_when_source_file_changes() { std::fs::write(&bam, b"original").unwrap(); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some(bam.to_string_lossy().into_owned()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap() @@ -2758,31 +2490,11 @@ async fn deleting_run_purges_derived_haplogroup_and_consensus() { let app = app().await; let b = app.add_biosample(None, "103589", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "Targeted Y".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "Targeted Y")) .await .unwrap(); let aln = app - .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "unknown".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }) + .record_alignment(NewAlignment::new(run.id, "GRCh38", "unknown")) .await .unwrap(); @@ -2847,31 +2559,15 @@ async fn branch_report_genotypes_the_mt_subtree_end_to_end() { let dir = fixtures(); let b = app.add_biosample(None, "S-mt", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); // GRCh38 build → chrM is rCRS-direct (no liftover), so tree positions query chrM as-is. let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem".into(), - variant_caller: None, bam_path: Some(dir.join("coverage.bam").to_string_lossy().into_owned()), reference_path: Some(dir.join("ref.fa").to_string_lossy().into_owned()), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem") }) .await .unwrap() @@ -2945,17 +2641,7 @@ async fn mt_alignment_pick_skips_a_y_only_run() { // A Big-Y (Y-only) run, recorded first so it's a candidate for both pickers. let y_run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "BIG_Y_700".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "BIG_Y_700")) .await .unwrap(); let y_aln = app @@ -2976,30 +2662,13 @@ async fn mt_alignment_pick_skips_a_y_only_run() { // A whole-genome run that does carry chrM. let wgs_run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let wgs_aln = app .record_alignment(NewAlignment { - sequence_run_id: wgs_run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem".into(), - variant_caller: None, bam_path: Some("/nonexistent.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(wgs_run.id, "GRCh38", "bwa-mem") }) .await .unwrap() @@ -3138,29 +2807,12 @@ mod full_analysis_plan { .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/nonexistent/plan.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap() @@ -3346,17 +2998,7 @@ async fn subject_with_run( .await .unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); (b, run) @@ -3371,15 +3013,8 @@ async fn registering_a_realignment_is_additive_and_records_its_source() { let source = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/tmp/vendor.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap(); @@ -3431,15 +3066,8 @@ async fn realigning_to_the_same_build_is_refused() { let source = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "minimap2".into(), - variant_caller: None, bam_path: Some("/tmp/already.cram".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "minimap2") }) .await .unwrap(); @@ -3466,15 +3094,8 @@ async fn derived_alignments_are_discoverable_from_their_source() { let source = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/tmp/vendor.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap(); @@ -3486,15 +3107,11 @@ async fn derived_alignments_are_discoverable_from_their_source() { let derived = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "minimap2".into(), - variant_caller: None, bam_path: Some("/tmp/realigned.cram".into()), reference_path: Some("/tmp/chm13.fa".into()), - content_sha256: None, derived_from_alignment_id: Some(source.id), derivation: Some("realign:minimap2-sr".into()), + ..NewAlignment::new(run.id, "chm13v2.0", "minimap2") }) .await .unwrap(); @@ -3521,15 +3138,8 @@ async fn existing_alignments_read_back_as_originals() { let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/tmp/x.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap(); @@ -3551,15 +3161,8 @@ async fn a_realigned_alignment_becomes_the_subjects_default() { let source = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/tmp/vendor.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap(); @@ -3572,15 +3175,11 @@ async fn a_realigned_alignment_becomes_the_subjects_default() { let realigned = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "minimap2".into(), - variant_caller: None, bam_path: Some("/tmp/realigned.cram".into()), reference_path: Some("/tmp/chm13.fa".into()), - content_sha256: None, derived_from_alignment_id: Some(source.id), derivation: Some("realign:minimap2-sr".into()), + ..NewAlignment::new(run.id, "chm13v2.0", "minimap2") }) .await .unwrap(); @@ -3618,63 +3217,34 @@ async fn a_project_batch_skips_what_it_would_refuse() { // Eligible: an off-build alignment with a file. let eligible = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh38".into(), - aligner: "bwa-mem2".into(), - variant_caller: None, bam_path: Some("/tmp/a.bam".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "GRCh38", "bwa-mem2") }) .await .unwrap(); // Skipped: already on the target build. app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "minimap2".into(), - variant_caller: None, bam_path: Some("/tmp/b.cram".into()), - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "minimap2") }) .await .unwrap(); // Skipped: no file to read. - app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "GRCh37".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }) - .await - .unwrap(); + app.record_alignment(NewAlignment::new(run.id, "GRCh37", "bwa")) + .await + .unwrap(); let queue = app.realignable_in_project(project.id, "chm13v2.0").await.unwrap(); assert_eq!(queue, vec![eligible.id]); // Once it has been realigned, a second batch has nothing left to do. app.record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "minimap2".into(), - variant_caller: None, bam_path: Some("/tmp/c.cram".into()), - reference_path: None, - content_sha256: None, derived_from_alignment_id: Some(eligible.id), derivation: Some("realign:minimap2-sr".into()), + ..NewAlignment::new(run.id, "chm13v2.0", "minimap2") }) .await .unwrap(); diff --git a/crates/navigator-domain/src/workspace.rs b/crates/navigator-domain/src/workspace.rs index c5e89bfa..9d831b69 100644 --- a/crates/navigator-domain/src/workspace.rs +++ b/crates/navigator-domain/src/workspace.rs @@ -37,6 +37,27 @@ pub struct Biosample { pub project_id: Option, } +impl Biosample { + /// A biosample with only its identity set — everything descriptive left unpopulated. + /// + /// This and its siblings ([`SequenceRun::new`], [`NewSequenceRun::new`], [`Alignment::new`], + /// [`NewAlignment::new`]) take exactly the fields that have no sensible empty value. They exist + /// so the callers that genuinely only know the identity — fixtures, and imports that fill the + /// rest in later — stop restating a column of `None`s. Callers that do know more should say so + /// with functional-update syntax: `Biosample { sex: Some("M".into()), ..Biosample::new(g, id) }`. + pub fn new(guid: SampleGuid, donor_identifier: impl Into) -> Self { + Biosample { + guid, + sample_accession: None, + donor_identifier: donor_identifier.into(), + description: None, + center_name: None, + sex: None, + project_id: None, + } + } +} + /// A sequencing run for a biosample, with summary read metrics as flat fields. /// /// The lab/instrument identity block (`instrument_id`/`sample_name`/`library_id`/`platform_unit`/ @@ -77,6 +98,37 @@ pub struct SequenceRun { } impl SequenceRun { + /// A run with only the fields the database requires — the whole metrics and lab-identity block + /// left `None`, which is exactly its state until an analysis pass fills it in. See + /// [`Biosample::new`] for why these constructors exist. + pub fn new( + id: i64, + biosample_guid: SampleGuid, + platform_name: impl Into, + test_type: impl Into, + ) -> Self { + SequenceRun { + id, + biosample_guid, + platform_name: platform_name.into(), + instrument_model: None, + test_type: test_type.into(), + library_layout: None, + total_reads: None, + pf_reads_aligned: None, + mean_read_length: None, + mean_insert_size: None, + total_bases: None, + read_type: None, + sequencing_facility: None, + instrument_id: None, + sample_name: None, + library_id: None, + platform_unit: None, + flowcell_id: None, + } + } + /// The standardized, vendor-neutral test label (`WGS150 45Gbases`, `HiFi 90Gbases`, `BigY-700`), /// or `None` when this isn't a yield/product test we standardize (chips, panels) — the caller /// falls back to the raw `test_type`. See [`du_domain::testprofile`]. @@ -104,6 +156,23 @@ pub struct NewSequenceRun { pub mean_insert_size: Option, } +impl NewSequenceRun { + /// A run to insert, with only the required fields set. See [`Biosample::new`]. + pub fn new(biosample_guid: SampleGuid, platform_name: impl Into, test_type: impl Into) -> Self { + NewSequenceRun { + biosample_guid, + platform_name: platform_name.into(), + instrument_model: None, + test_type: test_type.into(), + library_layout: None, + total_reads: None, + pf_reads_aligned: None, + mean_read_length: None, + mean_insert_size: None, + } + } +} + /// An alignment of a sequence run to a reference build. `bam_path`/`reference_path` /// locate the files so analysis can be run directly from the record. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -133,6 +202,24 @@ pub struct Alignment { } impl Alignment { + /// An alignment with only the fields the database requires — no file paths, no caller, and no + /// derivation, i.e. an original rather than something Navigator produced. See + /// [`Biosample::new`]. + pub fn new(id: i64, sequence_run_id: i64, reference_build: impl Into, aligner: impl Into) -> Self { + Alignment { + id, + sequence_run_id, + reference_build: reference_build.into(), + aligner: aligner.into(), + variant_caller: None, + bam_path: None, + reference_path: None, + content_sha256: None, + derived_from_alignment_id: None, + derivation: None, + } + } + /// Whether Navigator produced this alignment from another one, rather than importing it. /// /// The distinction is user-facing: a derived alignment can be deleted and rebuilt from its @@ -159,6 +246,23 @@ pub struct NewAlignment { pub derivation: Option, } +impl NewAlignment { + /// An alignment to insert, with only the required fields set. See [`Biosample::new`]. + pub fn new(sequence_run_id: i64, reference_build: impl Into, aligner: impl Into) -> Self { + NewAlignment { + sequence_run_id, + reference_build: reference_build.into(), + aligner: aligner.into(), + variant_caller: None, + bam_path: None, + reference_path: None, + content_sha256: None, + derived_from_alignment_id: None, + derivation: None, + } + } +} + /// A persisted analysis result, keyed by `(alignment, kind, algorithm_version)`. The /// version is part of the key so a cache entry is invalidated when the algorithm /// changes (plan §6 cache-versioning fix). `payload` is JSON of the result type. diff --git a/crates/navigator-store/src/artifact.rs b/crates/navigator-store/src/artifact.rs index 7fd82818..e4d9b5c5 100644 --- a/crates/navigator-store/src/artifact.rs +++ b/crates/navigator-store/src/artifact.rs @@ -243,57 +243,20 @@ mod tests { async fn subject(pool: &SqlitePool, donor: &str) -> SampleGuid { let guid = SampleGuid(uuid::Uuid::new_v4()); - crate::biosample::create( - pool, - &Biosample { - guid, - sample_accession: None, - donor_identifier: donor.into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }, - ) - .await - .unwrap(); + crate::biosample::create(pool, &Biosample::new(guid, donor)) + .await + .unwrap(); guid } async fn alignment(pool: &SqlitePool, guid: SampleGuid) -> i64 { - let run = crate::sequence_run::create( - pool, - &NewSequenceRun { - biosample_guid: guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }, - ) - .await - .unwrap(); - crate::alignment::create( - pool, - &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap() - .id + let run = crate::sequence_run::create(pool, &NewSequenceRun::new(guid, "ILLUMINA", "WGS")) + .await + .unwrap(); + crate::alignment::create(pool, &NewAlignment::new(run.id, "chm13v2.0", "bwa")) + .await + .unwrap() + .id } async fn full_coverage(pool: &SqlitePool, aln: i64) { diff --git a/crates/navigator-store/src/biosample_project.rs b/crates/navigator-store/src/biosample_project.rs index 0d82bd16..6fdb3522 100644 --- a/crates/navigator-store/src/biosample_project.rs +++ b/crates/navigator-store/src/biosample_project.rs @@ -124,20 +124,9 @@ mod tests { async fn seed_biosample(pool: &SqlitePool, donor: &str) -> SampleGuid { let guid = SampleGuid(uuid::Uuid::new_v4()); - crate::biosample::create( - pool, - &Biosample { - guid, - sample_accession: None, - donor_identifier: donor.into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }, - ) - .await - .unwrap(); + crate::biosample::create(pool, &Biosample::new(guid, donor)) + .await + .unwrap(); guid } diff --git a/crates/navigator-store/src/external_id.rs b/crates/navigator-store/src/external_id.rs index 9f24a9b5..32aca759 100644 --- a/crates/navigator-store/src/external_id.rs +++ b/crates/navigator-store/src/external_id.rs @@ -107,20 +107,9 @@ mod tests { async fn seed(pool: &SqlitePool, donor: &str) -> SampleGuid { let guid = SampleGuid(uuid::Uuid::new_v4()); - crate::biosample::create( - pool, - &Biosample { - guid, - sample_accession: None, - donor_identifier: donor.into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }, - ) - .await - .unwrap(); + crate::biosample::create(pool, &Biosample::new(guid, donor)) + .await + .unwrap(); guid } diff --git a/crates/navigator-store/src/ftdna_member.rs b/crates/navigator-store/src/ftdna_member.rs index 5385b9ba..4aca340c 100644 --- a/crates/navigator-store/src/ftdna_member.rs +++ b/crates/navigator-store/src/ftdna_member.rs @@ -80,20 +80,9 @@ mod tests { async fn seed(pool: &SqlitePool) -> SampleGuid { let guid = SampleGuid(uuid::Uuid::new_v4()); - crate::biosample::create( - pool, - &Biosample { - guid, - sample_accession: None, - donor_identifier: "GFX".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }, - ) - .await - .unwrap(); + crate::biosample::create(pool, &Biosample::new(guid, "GFX")) + .await + .unwrap(); guid } diff --git a/crates/navigator-store/src/mdka.rs b/crates/navigator-store/src/mdka.rs index e2f76355..331b4217 100644 --- a/crates/navigator-store/src/mdka.rs +++ b/crates/navigator-store/src/mdka.rs @@ -156,20 +156,9 @@ mod tests { async fn seed(pool: &SqlitePool) -> SampleGuid { let guid = SampleGuid(uuid::Uuid::new_v4()); - crate::biosample::create( - pool, - &Biosample { - guid, - sample_accession: None, - donor_identifier: "GFX".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }, - ) - .await - .unwrap(); + crate::biosample::create(pool, &Biosample::new(guid, "GFX")) + .await + .unwrap(); guid } diff --git a/crates/navigator-store/tests/store.rs b/crates/navigator-store/tests/store.rs index b8eca879..e70c584c 100644 --- a/crates/navigator-store/tests/store.rs +++ b/crates/navigator-store/tests/store.rs @@ -74,15 +74,12 @@ async fn foreign_keys_are_enforced() { // sequence_run referencing a non-existent biosample must fail. let run = NewSequenceRun { - biosample_guid: SampleGuid(Uuid::new_v4()), - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), library_layout: Some("PAIRED".into()), total_reads: Some(8_000_000), pf_reads_aligned: Some(7_956_881), mean_read_length: Some(148.0), mean_insert_size: Some(580.7), + ..NewSequenceRun::new(SampleGuid(Uuid::new_v4()), "ILLUMINA", "WGS") }; assert!(sequence_run::create(s.pool(), &run).await.is_err()); } @@ -96,15 +93,13 @@ async fn run_alignment_chain_persists() { let run = sequence_run::create( s.pool(), &NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), instrument_model: Some("HiSeq 2500".into()), - test_type: "WGS".into(), library_layout: Some("PAIRED".into()), total_reads: Some(8_000_000), pf_reads_aligned: Some(7_956_881), mean_read_length: Some(148.0), mean_insert_size: Some(580.7), + ..NewSequenceRun::new(b.guid, "ILLUMINA", "WGS") }, ) .await @@ -169,15 +164,8 @@ async fn run_alignment_chain_persists() { let aln = alignment::create( s.pool(), &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa-mem 0.7.19".into(), variant_caller: Some("navigator-haploid".into()), - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chm13v2.0", "bwa-mem 0.7.19") }, ) .await @@ -243,38 +231,12 @@ async fn clear_data_resets_subject_but_keeps_the_biosample() { let s = store().await; let b = sample(None); biosample::create(s.pool(), &b).await.unwrap(); - let run = sequence_run::create( - s.pool(), - &NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }, - ) - .await - .unwrap(); - let aln = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap(); + let run = sequence_run::create(s.pool(), &NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) + .await + .unwrap(); + let aln = alignment::create(s.pool(), &NewAlignment::new(run.id, "chm13v2.0", "bwa")) + .await + .unwrap(); artifact::upsert( s.pool(), aln.id, @@ -337,38 +299,12 @@ async fn artifact_upsert_replaces_same_version_and_keeps_distinct_versions() { let s = store().await; let b = sample(None); biosample::create(s.pool(), &b).await.unwrap(); - let run = sequence_run::create( - s.pool(), - &NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }, - ) - .await - .unwrap(); - let aln = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap(); + let run = sequence_run::create(s.pool(), &NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) + .await + .unwrap(); + let aln = alignment::create(s.pool(), &NewAlignment::new(run.id, "chm13v2.0", "bwa")) + .await + .unwrap(); // Same (kind, version) upserts in place. artifact::upsert( @@ -426,38 +362,12 @@ async fn delete_cascades_run_to_alignments_and_artifacts() { let s = store().await; let b = sample(None); biosample::create(s.pool(), &b).await.unwrap(); - let run = sequence_run::create( - s.pool(), - &NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }, - ) - .await - .unwrap(); - let aln = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap(); + let run = sequence_run::create(s.pool(), &NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) + .await + .unwrap(); + let aln = alignment::create(s.pool(), &NewAlignment::new(run.id, "chm13v2.0", "bwa")) + .await + .unwrap(); artifact::upsert( s.pool(), aln.id, @@ -473,22 +383,9 @@ async fn delete_cascades_run_to_alignments_and_artifacts() { .unwrap(); // Deleting a single alignment removes its artifacts but leaves the run. - let aln2 = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: run.id, - reference_build: "grch38".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap(); + let aln2 = alignment::create(s.pool(), &NewAlignment::new(run.id, "grch38", "bwa")) + .await + .unwrap(); artifact::upsert( s.pool(), aln2.id, @@ -564,34 +461,14 @@ async fn set_sequence_run_reparents_an_alignment() { let b = sample(None); biosample::create(s.pool(), &b).await.unwrap(); let mk_run = |layout: &str| NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), library_layout: Some(layout.to_string()), - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, + ..NewSequenceRun::new(b.guid, "ILLUMINA", "WGS") }; let primary = sequence_run::create(s.pool(), &mk_run("A")).await.unwrap(); let secondary = sequence_run::create(s.pool(), &mk_run("B")).await.unwrap(); - let aln = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: secondary.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) - .await - .unwrap(); + let aln = alignment::create(s.pool(), &NewAlignment::new(secondary.id, "chm13v2.0", "bwa")) + .await + .unwrap(); artifact::upsert( s.pool(), aln.id, @@ -839,40 +716,14 @@ async fn bulk_loaders_match_the_per_item_queries() { let b = sample(None); biosample::create(s.pool(), &b).await.unwrap(); guids.push(b.guid); - let run = sequence_run::create( - s.pool(), - &NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }, - ) - .await - .unwrap(); - // Two alignments on the middle subject, so grouping by subject is actually exercised. - for _ in 0..if i == 1 { 2 } else { 1 } { - let aln = alignment::create( - s.pool(), - &NewAlignment { - sequence_run_id: run.id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }, - ) + let run = sequence_run::create(s.pool(), &NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); + // Two alignments on the middle subject, so grouping by subject is actually exercised. + for _ in 0..if i == 1 { 2 } else { 1 } { + let aln = alignment::create(s.pool(), &NewAlignment::new(run.id, "chm13v2.0", "bwa")) + .await + .unwrap(); for kind in ["coverage", "sex"] { artifact::upsert( s.pool(), diff --git a/crates/navigator-ui/src/ui/detail.rs b/crates/navigator-ui/src/ui/detail.rs index 4728b01c..072aeca5 100644 --- a/crates/navigator-ui/src/ui/detail.rs +++ b/crates/navigator-ui/src/ui/detail.rs @@ -2639,17 +2639,10 @@ impl NavigatorApp { .clicked() { let platform = opt(&self.forms.run_platform).unwrap_or_else(|| "UNKNOWN".into()); - let _ = self.tx.send(Command::AddRun(NewSequenceRun { - biosample_guid: guid, - platform_name: platform, - instrument_model: None, - test_type: self.forms.run_test_type.clone(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - })); + let test_type = self.forms.run_test_type.clone(); + let _ = self + .tx + .send(Command::AddRun(NewSequenceRun::new(guid, platform, test_type))); self.forms.run_platform.clear(); } }); diff --git a/crates/navigator-ui/src/worker.rs b/crates/navigator-ui/src/worker.rs index f2fda9ee..31f08ab1 100644 --- a/crates/navigator-ui/src/worker.rs +++ b/crates/navigator-ui/src/worker.rs @@ -3739,31 +3739,15 @@ mod tests { let app = app().await; let b = app.add_biosample(None, "HG002", None, None).await.unwrap(); let run = app - .record_sequence_run(NewSequenceRun { - biosample_guid: b.guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }) + .record_sequence_run(NewSequenceRun::new(b.guid, "ILLUMINA", "WGS")) .await .unwrap(); let fixtures = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../navigator-analysis/tests/fixtures"); let aln = app .record_alignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "chrM".into(), - aligner: "synthetic".into(), - variant_caller: None, bam_path: Some(fixtures.join("coverage.bam").to_string_lossy().into_owned()), reference_path: Some(fixtures.join("ref.fa").to_string_lossy().into_owned()), - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, + ..NewAlignment::new(run.id, "chrM", "synthetic") }) .await .unwrap(); @@ -3914,17 +3898,7 @@ mod tests { // add a run -> RunsChanged(sample) match handle( &app, - Command::AddRun(NewSequenceRun { - biosample_guid: guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }), + Command::AddRun(NewSequenceRun::new(guid, "ILLUMINA", "WGS")), &CancelToken::none(), ) .await @@ -3940,17 +3914,7 @@ mod tests { // add an alignment -> AlignmentsChanged(run) match handle( &app, - Command::AddAlignment(NewAlignment { - sequence_run_id: run_id, - reference_build: "chm13v2.0".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }), + Command::AddAlignment(NewAlignment::new(run_id, "chm13v2.0", "bwa")), &CancelToken::none(), ) .await @@ -4020,17 +3984,7 @@ mod tests { // adding dependent data makes delete refuse with a conflict match handle( &app, - Command::AddRun(NewSequenceRun { - biosample_guid: guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, - total_reads: None, - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, - }), + Command::AddRun(NewSequenceRun::new(guid, "ILLUMINA", "WGS")), &CancelToken::none(), ) .await @@ -4302,15 +4256,8 @@ mod tests { match handle( &app, Command::AddRun(NewSequenceRun { - biosample_guid: guid, - platform_name: "ILLUMINA".into(), - instrument_model: None, - test_type: "WGS".into(), - library_layout: None, total_reads: Some(1_000), - pf_reads_aligned: None, - mean_read_length: None, - mean_insert_size: None, + ..NewSequenceRun::new(guid, "ILLUMINA", "WGS") }), &CancelToken::none(), ) @@ -4357,17 +4304,7 @@ mod tests { match handle( &app, - Command::AddAlignment(NewAlignment { - sequence_run_id: run.id, - reference_build: "grch38".into(), - aligner: "bwa".into(), - variant_caller: None, - bam_path: None, - reference_path: None, - content_sha256: None, - derived_from_alignment_id: None, - derivation: None, - }), + Command::AddAlignment(NewAlignment::new(run.id, "grch38", "bwa")), &CancelToken::none(), ) .await From 7ac0cbf44616e059e0003d8c128f1dca22fbe556 Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:28:20 -0500 Subject: [PATCH 5/6] refactor(app): one way to talk to the AppView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three clients reached the Edge API independently — IBD exchange, social, recruitment — and each arrived at the same two shapes: a POST whose body carries the device-key signature, and a replay-guarded signed GET with did/ts/sig on the query. `social_post` and `exchange_post` were byte-for-byte identical; the doc comment on one even said it mirrored the other, which is a duplication noticed and then kept. `social_get` and `exchange_get_poll` differed only by a closure. sync.rs and matching.rs open-coded the same request a fourth and fifth time, and sixteen sites spelled out the same reqwest-error mapping by hand. appview.rs is all of it: the URL, the two request shapes, the transport-error mapping, and the non-2xx classification. `exchange_get_poll` survives as a one-liner because the exchange endpoints all sign the same canonical poll string — that, and not the HTTP, was its content. fetch_exchange_key and the ibd/suggestions poll keep their own bodies: one treats 404 as absence, the other retries a 403 while the AppView verifies a freshly registered device key. Both now build their URL and map their errors through the shared helpers. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-app/src/appview.rs | 107 +++++++++++++++++++++++ crates/navigator-app/src/ibd_exchange.rs | 67 +++----------- crates/navigator-app/src/lib.rs | 16 +--- crates/navigator-app/src/matching.rs | 26 +----- crates/navigator-app/src/recruitment.rs | 8 +- crates/navigator-app/src/social.rs | 69 ++------------- crates/navigator-app/src/sync.rs | 28 ++---- 7 files changed, 141 insertions(+), 180 deletions(-) create mode 100644 crates/navigator-app/src/appview.rs diff --git a/crates/navigator-app/src/appview.rs b/crates/navigator-app/src/appview.rs new file mode 100644 index 00000000..926fce29 --- /dev/null +++ b/crates/navigator-app/src/appview.rs @@ -0,0 +1,107 @@ +//! The one way Navigator talks to the AppView's `/api/v1/*` Edge API. +//! +//! Three clients grew here independently — IBD exchange, social, recruitment — and each arrived at +//! the same two shapes: an unauthenticated-looking POST whose body carries the device-key +//! signature, and a replay-guarded signed GET whose `did`/`ts`/`sig` ride on the query string. The +//! IBD and social versions were byte-for-byte identical, and the remaining one-off calls in +//! `sync.rs` / `matching.rs` open-coded the same thing a fourth and fifth time. They are all this +//! module now, so the error mapping, the signing-query layout, and the non-2xx classification are +//! decided once. +//! +//! What travels: a DID, a timestamp, a signature, and whatever the caller chose to send. Never +//! genotypes, never coordinates. + +use super::*; + +/// A transport failure (connection refused, timeout, TLS) on an AppView call. +/// +/// The AppView is reached with a bare `reqwest` client rather than through the sync engine, but a +/// network failure means the same thing either way, so it lands in the same error variant the PDS +/// paths use and the offline indicator already understands. +pub(crate) fn transport(e: reqwest::Error) -> AppError { + AppError::Sync(navigator_sync::SyncError::from(e)) +} + +/// Classify a non-2xx AppView response into a user-facing [`AppError::AppView`]. Consumes `resp` to +/// read the body (so capture the status first at the call site if it is also needed). +pub(crate) async fn status_error(api: &str, resp: reqwest::Response) -> AppError { + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + match status.as_u16() { + 403 => AppError::AppView(format!( + "{api}: device key not yet registered or verified by the AppView (403)" + )), + 422 => AppError::AppView(format!( + "{api}: request rejected, likely clock skew (422) — check the system clock" + )), + _ => AppError::AppView(format!("{api}: {status}: {body}")), + } +} + +impl App { + /// The absolute URL of an `/api/v1/` endpoint on the configured AppView. + pub(crate) fn appview_url(&self, path: &str) -> String { + format!("{}/api/v1/{path}", decodingus_appview_url()) + } + + /// POST a JSON body to an `/api/v1/` endpoint and return the decoded response. + /// + /// The signature (and the DID it is over) belongs in `body` — these endpoints authenticate the + /// device key per call, not the HTTP request — so this deliberately takes an already-signed + /// body rather than signing on the caller's behalf: the canonical string differs per endpoint + /// and only the caller knows it. + pub(crate) async fn appview_post( + &self, + path: &str, + body: serde_json::Value, + ) -> Result { + let resp = self + .auth + .http + .post(self.appview_url(path)) + .json(&body) + .send() + .await + .map_err(transport)?; + if !resp.status().is_success() { + return Err(status_error(path, resp).await); + } + resp.json().await.map_err(transport) + } + + /// Device-key-signed GET to an `/api/v1/` endpoint, decoded into `T`. + /// + /// `build_msg(did, ts)` produces the canonical string to sign — the one thing that varies + /// between a poll, a thread read, and an exchange pull. `did`/`ts`/`sig` plus `extra` go on the + /// query; the timestamp is what makes the signature replay-guarded. + pub(crate) async fn appview_get_signed( + &self, + path: &str, + build_msg: F, + extra: &[(&str, &str)], + ) -> Result + where + T: serde::de::DeserializeOwned, + F: Fn(&str, i64) -> String, + { + let did = self.current_account().ok_or(AppError::NotAuthenticated)?; + let dev = self.ensure_device_key().await?; + let ts = Utc::now().timestamp(); + let sig = dev.sign(&build_msg(&did, ts)); + let ts_s = ts.to_string(); + let mut query: Vec<(&str, &str)> = vec![("did", did.as_str()), ("ts", ts_s.as_str()), ("sig", sig.as_str())]; + query.extend_from_slice(extra); + let resp = self + .auth + .http + .get(self.appview_url(path)) + .query(&query) + .send() + .await + .map_err(transport)?; + if !resp.status().is_success() { + return Err(status_error(path, resp).await); + } + resp.json().await.map_err(transport) + } +} diff --git a/crates/navigator-app/src/ibd_exchange.rs b/crates/navigator-app/src/ibd_exchange.rs index 868f0cc8..805b9bec 100644 --- a/crates/navigator-app/src/ibd_exchange.rs +++ b/crates/navigator-app/src/ibd_exchange.rs @@ -19,7 +19,7 @@ impl App { let ts = Utc::now().timestamp(); let sig = dev.sign_fresh(ts, &exchange::messages::publickey(&did, &pub_b64, None)); let body = serde_json::json!({ "did": did, "x25519_pub": pub_b64, "ts": ts, "signature": sig }); - let v = self.exchange_post("exchange/key", body).await?; + let v = self.appview_post("exchange/key", body).await?; let _ = v; // { did, status: "published" } Ok(ik) } @@ -27,7 +27,7 @@ impl App { /// Fetch a peer's published X25519 public key (STANDARD base64), or `None` if they haven't /// published one. Public read — no signature. pub async fn fetch_exchange_key(&self, did: &str) -> Result, AppError> { - let url = format!("{}/api/v1/exchange/key", decodingus_appview_url()); + let url = self.appview_url("exchange/key"); let resp = self .auth .http @@ -35,17 +35,14 @@ impl App { .query(&[("did", did)]) .send() .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; + .map_err(appview::transport)?; if resp.status().as_u16() == 404 { return Ok(None); } if !resp.status().is_success() { - return Err(appview_status_error("exchange/key", resp).await); + return Err(appview::status_error("exchange/key", resp).await); } - let v: serde_json::Value = resp - .json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; + let v: serde_json::Value = resp.json().await.map_err(appview::transport)?; Ok(v.get("x25519_pub").and_then(|x| x.as_str()).map(str::to_string)) } @@ -77,7 +74,7 @@ impl App { "ts": ts, "signature": sig, }); - self.exchange_post("exchange/request", body).await?; + self.appview_post("exchange/request", body).await?; Ok(request_uri) } @@ -95,7 +92,7 @@ impl App { "ts": ts, "signature": sig, }); - let v = self.exchange_post("exchange/consent", body).await?; + let v = self.appview_post("exchange/consent", body).await?; Ok(ConsentOutcome { status: v .get("status") @@ -189,7 +186,7 @@ impl App { "ts": ts, "signature": sig, }); - let v = self.exchange_post("exchange/relay", body).await?; + let v = self.appview_post("exchange/relay", body).await?; Ok(v.get("id").and_then(|x| x.as_i64()).unwrap_or_default()) } @@ -225,7 +222,7 @@ impl App { let ts = Utc::now().timestamp(); let sig = dev.sign_fresh(ts, &exchange::messages::ack(&did, envelope_id)); let body = serde_json::json!({ "envelope_id": envelope_id, "did": did, "ts": ts, "signature": sig }); - self.exchange_post("exchange/ack", body).await.map(|_| ()) + self.appview_post("exchange/ack", body).await.map(|_| ()) } /// Establish a shared session key for a consent-ready session: publish/load our identity key, @@ -581,50 +578,12 @@ impl App { Ok(()) } - /// POST a JSON body to an `/api/v1/` exchange endpoint, mapping non-2xx to an AppView error. - async fn exchange_post(&self, path: &str, body: serde_json::Value) -> Result { - let url = format!("{}/api/v1/{path}", decodingus_appview_url()); - let resp = self - .auth - .http - .post(&url) - .json(&body) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error(path, resp).await); - } - resp.json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e))) - } - /// Issue a device-key-signed `exchange-poll` GET to an `/api/v1/` endpoint, with `extra` - /// query params appended. Shared by incoming / pending / relay-pull. + /// query params appended. Shared by incoming / pending / relay-pull — the exchange endpoints + /// all sign the same canonical poll string, so this is the only thing they add over + /// [`App::appview_get_signed`]. async fn exchange_get_poll(&self, path: &str, extra: &[(&str, &str)]) -> Result { - let did = self.current_account().ok_or(AppError::NotAuthenticated)?; - let dev = self.ensure_device_key().await?; - let url = format!("{}/api/v1/{path}", decodingus_appview_url()); - let ts = Utc::now().timestamp(); - let sig = dev.sign(&exchange::messages::poll(&did, ts)); - let ts_s = ts.to_string(); - let mut query: Vec<(&str, &str)> = vec![("did", did.as_str()), ("ts", ts_s.as_str()), ("sig", sig.as_str())]; - query.extend_from_slice(extra); - let resp = self - .auth - .http - .get(&url) - .query(&query) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error(path, resp).await); - } - resp.json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e))) + self.appview_get_signed(path, exchange::messages::poll, extra).await } /// Enqueue the anchor records every child record references: the subject's biosample summary diff --git a/crates/navigator-app/src/lib.rs b/crates/navigator-app/src/lib.rs index a9016d3a..574a3139 100644 --- a/crates/navigator-app/src/lib.rs +++ b/crates/navigator-app/src/lib.rs @@ -876,21 +876,6 @@ fn parse_ibd_signals(v: &serde_json::Value) -> Vec { } } -/// Classify a non-2xx AppView response into a user-facing [`AppError::AppView`]. Consumes -/// `resp` to read the body (so capture the status first at the call site if also needed). -async fn appview_status_error(api: &str, resp: reqwest::Response) -> AppError { - let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); - match status.as_u16() { - 403 => AppError::AppView(format!( - "{api}: device key not yet registered or verified by the AppView (403)" - )), - 422 => AppError::AppView(format!( - "{api}: request rejected, likely clock skew (422) — check the system clock" - )), - _ => AppError::AppView(format!("{api}: {status}: {body}")), - } -} pub use navigator_analysis::ibd_attest::{IbdAttestation, IbdExchangeMsg, IbdSite}; use navigator_domain::bisdna; pub use navigator_domain::brief::{ @@ -2887,6 +2872,7 @@ pub struct RefBuildStatus { } mod analysis; +mod appview; pub use analysis::AnalysisStep; mod auth; mod blocktree; diff --git a/crates/navigator-app/src/matching.rs b/crates/navigator-app/src/matching.rs index 245e3305..e2520480 100644 --- a/crates/navigator-app/src/matching.rs +++ b/crates/navigator-app/src/matching.rs @@ -322,18 +322,7 @@ impl App { "ts": ts, "signature": sig, }); - let url = format!("{}/api/v1/ibd/dismiss", decodingus_appview_url()); - let resp = self - .auth - .http - .post(&url) - .json(&body) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error("ibd/dismiss", resp).await); - } + self.appview_post("ibd/dismiss", body).await?; Ok(()) } @@ -372,18 +361,7 @@ impl App { "ts": ts, "signature": sig, }); - let url = format!("{}/api/v1/ibd/attest", decodingus_appview_url()); - let resp = self - .auth - .http - .post(&url) - .json(&body) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error("ibd/attest", resp).await); - } + self.appview_post("ibd/attest", body).await?; Ok(()) } diff --git a/crates/navigator-app/src/recruitment.rs b/crates/navigator-app/src/recruitment.rs index f4b5030c..4a14b881 100644 --- a/crates/navigator-app/src/recruitment.rs +++ b/crates/navigator-app/src/recruitment.rs @@ -2,7 +2,7 @@ //! social roadmap 3c) — the **response** side: list the caller's open invitations and accept/decline //! them. Campaign creation stays on the AppView web flow (it's gated to a group-project admin, which //! the Navigator can't yet act as). Device-key-signed like the social/exchange clients; reuses the -//! shared [`social_post`](App::social_post) / [`social_get`](App::social_get) transport. Invitations +//! shared [`appview_post`](App::appview_post) / [`appview_get_signed`](App::appview_get_signed) transport. Invitations //! also arrive as SYSTEM notifications, so this pairs with the Community → Notifications surface. use super::*; @@ -25,7 +25,9 @@ impl App { #[serde(default)] items: Vec, } - let r: Resp = self.social_get("recruitment/invitations", messages::poll, &[]).await?; + let r: Resp = self + .appview_get_signed("recruitment/invitations", messages::poll, &[]) + .await?; Ok(r.items) } @@ -43,7 +45,7 @@ impl App { "ts": ts, "signature": sig, }); - let v = self.social_post("recruitment/respond", body).await?; + let v = self.appview_post("recruitment/respond", body).await?; Ok(v.get("changed").and_then(|x| x.as_bool()).unwrap_or(false)) } } diff --git a/crates/navigator-app/src/social.rs b/crates/navigator-app/src/social.rs index d692f491..4eaf8b2f 100644 --- a/crates/navigator-app/src/social.rs +++ b/crates/navigator-app/src/social.rs @@ -131,7 +131,7 @@ impl App { #[serde(default)] items: Vec, } - let r: Resp = self.social_get("social/threads", messages::poll, &[]).await?; + let r: Resp = self.appview_get_signed("social/threads", messages::poll, &[]).await?; Ok(r.items) } @@ -144,7 +144,7 @@ impl App { } let path = format!("social/thread/{conversation_id}"); let r: Resp = self - .social_get(&path, |d, ts| messages::thread_read(d, conversation_id, ts), &[]) + .appview_get_signed(&path, |d, ts| messages::thread_read(d, conversation_id, ts), &[]) .await?; Ok(r.items) } @@ -176,7 +176,7 @@ impl App { if let Some(s) = subject { b["subject"] = serde_json::json!(s); } - let v = self.social_post("social/thread", b).await?; + let v = self.appview_post("social/thread", b).await?; Ok(v.get("conversation_id") .and_then(|x| x.as_str()) .unwrap_or_default() @@ -187,7 +187,7 @@ impl App { /// Read the community feed: announcements + community posts + federated mirror. pub async fn community_feed(&self) -> Result { - self.social_get("social/feed", messages::poll, &[]).await + self.appview_get_signed("social/feed", messages::poll, &[]).await } /// Post to the community feed (optionally tagged with a `topic`, or as a reply to `parent`); @@ -210,7 +210,7 @@ impl App { if let Some(p) = parent { b["parent_post_id"] = serde_json::json!(p); } - let v = self.social_post("social/post", b).await?; + let v = self.appview_post("social/post", b).await?; Ok(v.get("id").and_then(|x| x.as_str()).unwrap_or_default().to_string()) } @@ -247,7 +247,8 @@ impl App { /// The signed-in account's notifications + unread count. pub async fn notifications(&self) -> Result { - self.social_get("social/notifications", messages::poll, &[]).await + self.appview_get_signed("social/notifications", messages::poll, &[]) + .await } /// Mark one notification read (`id = Some`) or all (`id = None`); returns how many were marked. @@ -260,63 +261,9 @@ impl App { if let Some(i) = id { b["id"] = serde_json::json!(i); } - let v = self.social_post("social/notifications/read", b).await?; + let v = self.appview_post("social/notifications/read", b).await?; Ok(v.get("marked").and_then(|x| x.as_i64()).unwrap_or(0)) } - - // ---- transport helpers (mirror the IBD exchange client) ---------------- - - /// POST a JSON body to a `/api/v1/<…>` endpoint, mapping non-2xx to an AppView error. Shared by - /// the social client and the recruitment Edge client (`recruitment.rs`). - pub(crate) async fn social_post(&self, path: &str, body: serde_json::Value) -> Result { - let url = format!("{}/api/v1/{path}", decodingus_appview_url()); - let resp = self - .auth - .http - .post(&url) - .json(&body) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error(path, resp).await); - } - resp.json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e))) - } - - /// Device-key-signed GET to a `/api/v1/<…>` endpoint. `build_msg(did, ts)` produces the - /// canonical string to sign (poll or thread-read); `did`/`ts`/`sig` + `extra` go on the query. - /// Shared by the social client and the recruitment Edge client (`recruitment.rs`). - pub(crate) async fn social_get(&self, path: &str, build_msg: F, extra: &[(&str, &str)]) -> Result - where - T: serde::de::DeserializeOwned, - F: Fn(&str, i64) -> String, - { - let did = self.current_account().ok_or(AppError::NotAuthenticated)?; - let dev = self.ensure_device_key().await?; - let url = format!("{}/api/v1/{path}", decodingus_appview_url()); - let ts = Utc::now().timestamp(); - let sig = dev.sign(&build_msg(&did, ts)); - let ts_s = ts.to_string(); - let mut query: Vec<(&str, &str)> = vec![("did", did.as_str()), ("ts", ts_s.as_str()), ("sig", sig.as_str())]; - query.extend_from_slice(extra); - let resp = self - .auth - .http - .get(&url) - .query(&query) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error(path, resp).await); - } - resp.json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e))) - } } #[cfg(test)] diff --git a/crates/navigator-app/src/sync.rs b/crates/navigator-app/src/sync.rs index a6cbbee8..b8031df3 100644 --- a/crates/navigator-app/src/sync.rs +++ b/crates/navigator-app/src/sync.rs @@ -387,7 +387,7 @@ impl App { pub async fn ibd_suggestions(&self) -> Result, AppError> { let did = self.current_account().ok_or(AppError::NotAuthenticated)?; let key = self.ensure_device_key().await?; - let url = format!("{}/api/v1/ibd/suggestions", decodingus_appview_url()); + let url = self.appview_url("ibd/suggestions"); let mut attempt = 0u32; loop { @@ -402,13 +402,10 @@ impl App { .query(&[("did", did.as_str()), ("ts", ts.as_str()), ("sig", sig.as_str())]) .send() .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; + .map_err(appview::transport)?; let status = resp.status(); if status.is_success() { - let body: serde_json::Value = resp - .json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; + let body: serde_json::Value = resp.json().await.map_err(appview::transport)?; return Ok(parse_ibd_suggestions(&body)); } if status.as_u16() == 403 && attempt < DEVICE_KEY_INGEST_RETRIES { @@ -416,7 +413,7 @@ impl App { attempt += 1; continue; } - return Err(appview_status_error("ibd/suggestions", resp).await); + return Err(appview::status_error("ibd/suggestions", resp).await); } } @@ -431,7 +428,6 @@ impl App { pub async fn ibd_introduce(&self, suggested_sample_guid: &str) -> Result { let did = self.current_account().ok_or(AppError::NotAuthenticated)?; let key = self.ensure_device_key().await?; - let url = format!("{}/api/v1/ibd/introduce", decodingus_appview_url()); let ts = Utc::now().timestamp(); let sig = key.sign_fresh(ts, &format!("ibd-introduce\n{did}\n{suggested_sample_guid}")); // The AppView's IntroduceBody deserializes plain snake_case (no serde rename), and @@ -442,21 +438,7 @@ impl App { "ts": ts, "signature": sig, }); - let resp = self - .auth - .http - .post(&url) - .json(&body) - .send() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; - if !resp.status().is_success() { - return Err(appview_status_error("ibd/introduce", resp).await); - } - let v: serde_json::Value = resp - .json() - .await - .map_err(|e| AppError::Sync(navigator_sync::SyncError::from(e)))?; + let v = self.appview_post("ibd/introduce", body).await?; let request_uri = v .get("requestUri") .or_else(|| v.get("request_uri")) From 40bd3e972c027a86defc85b711b224a02150fd2e Mon Sep 17 00:00:00 2001 From: James Kane Date: Mon, 17 Aug 2026 08:27:29 -0500 Subject: [PATCH 6/6] fix(store): one signature-keyed cache, and stop leaking the ones nobody listed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit consensus_archaic, consensus_roh, consensus_painting and consensus_archaic_segments were the same module four times over — same struct, same upsert/get/delete, same test — with a table name and three column names swapped. Roughly 420 lines to say one thing. The copies had already cost something. Both places that purge a subject's derived results enumerate the cache tables by hand, and each had fallen behind the set by a different amount: - `biosample::clear_data`, the "reset this subject's analysis" path, only ever named consensus_painting. ROH and both archaic caches survived a clear, still keyed to a consensus signature that no longer existed. - `purge_alignment_derived` named three of the four and left the Tier B archaic segments behind, keyed to an alignment that had just been deleted. Both now iterate `sig_cache::ALL`, which is the list. That is the fix; the deduplication is what makes the list exist. The schema is untouched — the column names still differ per table for historical reasons, so each cache carries its own and `get` aliases them back to a common shape. The tests run over ALL, including one that the four copies could not have written: that writing one cache does not disturb another sharing a column name. Co-Authored-By: Claude Opus 5 (1M context) --- crates/navigator-app/src/commands.rs | 8 +- crates/navigator-app/src/haplogroup.rs | 122 ++++++----- crates/navigator-app/src/lib.rs | 8 +- crates/navigator-store/src/biosample.rs | 19 +- .../navigator-store/src/consensus_archaic.rs | 104 ---------- .../src/consensus_archaic_segments.rs | 107 ---------- .../navigator-store/src/consensus_painting.rs | 103 ---------- crates/navigator-store/src/consensus_roh.rs | 104 ---------- crates/navigator-store/src/lib.rs | 5 +- crates/navigator-store/src/sig_cache.rs | 191 ++++++++++++++++++ 10 files changed, 280 insertions(+), 491 deletions(-) delete mode 100644 crates/navigator-store/src/consensus_archaic.rs delete mode 100644 crates/navigator-store/src/consensus_archaic_segments.rs delete mode 100644 crates/navigator-store/src/consensus_painting.rs delete mode 100644 crates/navigator-store/src/consensus_roh.rs create mode 100644 crates/navigator-store/src/sig_cache.rs diff --git a/crates/navigator-app/src/commands.rs b/crates/navigator-app/src/commands.rs index c44bfca1..b557c0af 100644 --- a/crates/navigator-app/src/commands.rs +++ b/crates/navigator-app/src/commands.rs @@ -375,9 +375,11 @@ impl App { for dna in ["Y", "Mt", "Auto"] { consensus_profile::delete(pool, biosample, dna).await?; } - consensus_painting::delete(pool, biosample).await?; - consensus_roh::delete(pool, biosample).await?; - consensus_archaic::delete(pool, biosample).await?; + // Every signature-keyed cache, from the one list — this used to name three of the four by + // hand and leave the Tier-B archaic segments behind, still keyed to a deleted alignment. + for cache in sig_cache::ALL { + cache.delete(pool, biosample).await?; + } // The audit log describes the consensus we just wiped; clear it so deleting the last run // can't leave a stale RUN_RECORDED history pointing at gone alignments. It is re-appended // when the consensus is next rebuilt from any remaining calls. diff --git a/crates/navigator-app/src/haplogroup.rs b/crates/navigator-app/src/haplogroup.rs index 0e3795d9..036285b9 100644 --- a/crates/navigator-app/src/haplogroup.rs +++ b/crates/navigator-app/src/haplogroup.rs @@ -2473,7 +2473,7 @@ impl App { /// DecodingUs Y-DNA tree-with-variants JSON from our AppView (`/api/v1/y-tree/full`), /// host from [`decodingus_appview_url`]. On-disk cached like the FTDNA tree. pub(crate) async fn fetch_decodingus_y_tree(&self) -> Result { - let url = format!("{}/api/v1/y-tree/full", decodingus_appview_url()); + let url = self.appview_url("y-tree/full"); self.fetch_tree(&url, "decodingus-ytree.json").await } @@ -2483,7 +2483,7 @@ impl App { /// (~577, plus local indels), so callers must remap onto rCRS via [`mt_tree_rcrs`]. On-disk /// cached like the other trees. pub(crate) async fn fetch_decodingus_mt_tree(&self) -> Result { - let url = format!("{}/api/v1/mt-tree/full", decodingus_appview_url()); + let url = self.appview_url("mt-tree/full"); self.fetch_tree(&url, "decodingus-mttree.json").await } @@ -2564,7 +2564,7 @@ impl App { /// cached like the trees (7-day TTL + offline fallback). Looked up locally so a batch import /// makes one network call, not one per sample. async fn fetch_lab_instruments(&self) -> Result, AppError> { - let url = format!("{}/api/v1/sequencer/lab-instruments", decodingus_appview_url()); + let url = self.appview_url("sequencer/lab-instruments"); let json = self.fetch_tree(&url, "sequencer-lab-instruments.json").await?; serde_json::from_str(&json).map_err(|e| AppError::Import(format!("parsing lab-instruments: {e}"))) } @@ -3324,13 +3324,13 @@ impl App { let Some(row) = consensus_profile::get(self.store.pool(), biosample_guid, "Auto").await? else { return Ok(None); }; - let Some(p) = consensus_painting::get(self.store.pool(), biosample_guid).await? else { + let Some(p) = sig_cache::PAINTING.get(self.store.pool(), biosample_guid).await? else { return Ok(None); }; - if p.consensus_sig != row.last_reconciled_at { + if p.sig != row.last_reconciled_at { return Ok(None); // painted from an older consensus — stale } - Ok(Some(parse_painting_json(&p.segments)?)) + Ok(Some(parse_painting_json(&p.payload)?)) } /// Paint each chromosome with local ancestry from the subject's **consensus** — no BAM walk. The @@ -3448,14 +3448,15 @@ impl App { }; // Cache keyed to the consensus signature so it's reused until the consensus is rebuilt. - consensus_painting::upsert( - self.store.pool(), - biosample_guid, - &sig, - &serde_json::to_string(&result)?, - &Utc::now().to_rfc3339(), - ) - .await?; + sig_cache::PAINTING + .upsert( + self.store.pool(), + biosample_guid, + &sig, + &serde_json::to_string(&result)?, + &Utc::now().to_rfc3339(), + ) + .await?; Ok(result) } @@ -3538,11 +3539,11 @@ impl App { let Some(row) = consensus_profile::get(self.store.pool(), biosample_guid, "Auto").await? else { return Ok(None); }; - let Some(r) = consensus_roh::get(self.store.pool(), biosample_guid).await? else { + let Some(r) = sig_cache::ROH.get(self.store.pool(), biosample_guid).await? else { return Ok(None); }; - if r.consensus_sig == row.last_reconciled_at { - Ok(Some(serde_json::from_str(&r.roh)?)) + if r.sig == row.last_reconciled_at { + Ok(Some(serde_json::from_str(&r.payload)?)) } else { Ok(None) // computed from an older consensus — stale } @@ -3561,9 +3562,9 @@ impl App { let sig = row.last_reconciled_at.clone(); // Cache hit (same consensus signature) → return without recomputing. - if let Some(r) = consensus_roh::get(self.store.pool(), biosample_guid).await? { - if r.consensus_sig == sig { - return Ok(serde_json::from_str(&r.roh)?); + if let Some(r) = sig_cache::ROH.get(self.store.pool(), biosample_guid).await? { + if r.sig == sig { + return Ok(serde_json::from_str(&r.payload)?); } } @@ -3583,14 +3584,15 @@ impl App { .await?; // Cache keyed to the consensus signature so it's reused until the consensus is rebuilt. - consensus_roh::upsert( - self.store.pool(), - biosample_guid, - &sig, - &serde_json::to_string(&result)?, - &Utc::now().to_rfc3339(), - ) - .await?; + sig_cache::ROH + .upsert( + self.store.pool(), + biosample_guid, + &sig, + &serde_json::to_string(&result)?, + &Utc::now().to_rfc3339(), + ) + .await?; Ok(result) } @@ -3674,15 +3676,18 @@ impl App { if !crate::ARCHAIC_SEGMENTS_ENABLED { return Ok(None); } - let Some(row) = consensus_archaic_segments::get(self.store.pool(), biosample_guid).await? else { + let Some(row) = sig_cache::ARCHAIC_SEGMENTS + .get(self.store.pool(), biosample_guid) + .await? + else { return Ok(None); }; let Some(aln) = self.alignment_with_diploid_calls(biosample_guid).await? else { return Ok(None); }; let contigs = crate::called_diploid_contigs(&self.store, aln).await?; - if row.source_sig == archaic_segment_sig(aln, &contigs) { - Ok(Some(serde_json::from_str(&row.segments)?)) + if row.sig == archaic_segment_sig(aln, &contigs) { + Ok(Some(serde_json::from_str(&row.payload)?)) } else { Ok(None) } @@ -3735,9 +3740,12 @@ impl App { // Computed from the contigs actually cached, so a later genome-wide pass invalidates a // partial result instead of inheriting it. let sig = archaic_segment_sig(aln, &crate::called_diploid_contigs(&self.store, aln).await?); - if let Some(row) = consensus_archaic_segments::get(self.store.pool(), biosample_guid).await? { - if row.source_sig == sig { - return Ok(serde_json::from_str(&row.segments)?); + if let Some(row) = sig_cache::ARCHAIC_SEGMENTS + .get(self.store.pool(), biosample_guid) + .await? + { + if row.sig == sig { + return Ok(serde_json::from_str(&row.payload)?); } } @@ -3833,14 +3841,15 @@ impl App { }) .await??; - consensus_archaic_segments::upsert( - self.store.pool(), - biosample_guid, - &sig, - &serde_json::to_string(&result)?, - &Utc::now().to_rfc3339(), - ) - .await?; + sig_cache::ARCHAIC_SEGMENTS + .upsert( + self.store.pool(), + biosample_guid, + &sig, + &serde_json::to_string(&result)?, + &Utc::now().to_rfc3339(), + ) + .await?; Ok(result) } @@ -4013,13 +4022,13 @@ impl App { let Some(row) = consensus_profile::get(self.store.pool(), biosample_guid, "Auto").await? else { return Ok(None); }; - let Some(r) = consensus_archaic::get(self.store.pool(), biosample_guid).await? else { + let Some(r) = sig_cache::ARCHAIC.get(self.store.pool(), biosample_guid).await? else { return Ok(None); }; // Prefix match: the stored sig is ":", so a consensus change or a panel // rebuild both read as stale. - if r.consensus_sig.starts_with(&row.last_reconciled_at) { - Ok(Some(serde_json::from_str(&r.archaic)?)) + if r.sig.starts_with(&row.last_reconciled_at) { + Ok(Some(serde_json::from_str(&r.payload)?)) } else { Ok(None) // computed from an older consensus — stale } @@ -4058,9 +4067,9 @@ impl App { let panel_fingerprint = navigator_analysis::manifest::sha256_hex(&bytes); let sig = format!("{}:{}", row.last_reconciled_at, &panel_fingerprint[..16]); - if let Some(r) = consensus_archaic::get(self.store.pool(), biosample_guid).await? { - if r.consensus_sig == sig { - return Ok(serde_json::from_str(&r.archaic)?); + if let Some(r) = sig_cache::ARCHAIC.get(self.store.pool(), biosample_guid).await? { + if r.sig == sig { + return Ok(serde_json::from_str(&r.payload)?); } } let panel = ArchaicMarkerPanel::from_bytes(&bytes)?; @@ -4106,14 +4115,15 @@ impl App { result.cohort = Some(cohort); } - consensus_archaic::upsert( - self.store.pool(), - biosample_guid, - &sig, - &serde_json::to_string(&result)?, - &Utc::now().to_rfc3339(), - ) - .await?; + sig_cache::ARCHAIC + .upsert( + self.store.pool(), + biosample_guid, + &sig, + &serde_json::to_string(&result)?, + &Utc::now().to_rfc3339(), + ) + .await?; Ok(result) } diff --git a/crates/navigator-app/src/lib.rs b/crates/navigator-app/src/lib.rs index 574a3139..9cbb657e 100644 --- a/crates/navigator-app/src/lib.rs +++ b/crates/navigator-app/src/lib.rs @@ -905,10 +905,10 @@ pub use navigator_store::ibd_exchange::StoredIbdExchange; pub use navigator_store::ibd_request::StoredIbdRequest; pub use navigator_store::source_file::SourceFile; use navigator_store::{ - alignment, ancestry_result, artifact, biosample, biosample_project, chip_profile, consensus_archaic, - consensus_archaic_segments, consensus_painting, consensus_profile, consensus_roh, haplogroup_call, mdka, - mtdna as mtdna_store, project, reconciliation as recon_store, sequence_run, source_file, str_profile, sync_history, - sync_outbox, sync_state, variant_set, variant_set_genotype, variant_set_private_y, Store, StoreError, + alignment, ancestry_result, artifact, biosample, biosample_project, chip_profile, consensus_profile, + haplogroup_call, mdka, mtdna as mtdna_store, project, reconciliation as recon_store, sequence_run, sig_cache, + source_file, str_profile, sync_history, sync_outbox, sync_state, variant_set, variant_set_genotype, + variant_set_private_y, Store, StoreError, }; use serde::de::DeserializeOwned; use serde::Serialize; diff --git a/crates/navigator-store/src/biosample.rs b/crates/navigator-store/src/biosample.rs index 7e1750a3..4eccab7f 100644 --- a/crates/navigator-store/src/biosample.rs +++ b/crates/navigator-store/src/biosample.rs @@ -151,9 +151,10 @@ pub async fn clear_home_project(pool: &SqlitePool, project_id: i64) -> Result Result<(), StoreError> { let g = guid.0.to_string(); let mut tx = pool.begin().await?; @@ -194,11 +195,14 @@ pub async fn clear_data(pool: &SqlitePool, guid: SampleGuid) -> Result<(), Store .bind(&g) .execute(&mut *tx) .await?; - // Biosample-keyed derived + imported tables (the biosample row itself is kept). + // Biosample-keyed derived + imported tables (the biosample row itself is kept). The + // signature-keyed caches come from `sig_cache::ALL` rather than being listed here, because when + // they were listed by hand this loop only ever named `consensus_painting` — ROH and both + // archaic caches survived a "clear this subject's data", still keyed to a consensus signature + // that no longer existed. for table in [ "haplogroup_call", "consensus_profile", - "consensus_painting", "reconciliation_override", "reconciliation_audit", "ancestry_result", @@ -207,7 +211,10 @@ pub async fn clear_data(pool: &SqlitePool, guid: SampleGuid) -> Result<(), Store "str_profile", "variant_set", "chip_profile", - ] { + ] + .into_iter() + .chain(crate::sig_cache::ALL.iter().map(|c| c.table())) + { sqlx::query(&format!("DELETE FROM {table} WHERE biosample_guid = ?")) .bind(&g) .execute(&mut *tx) diff --git a/crates/navigator-store/src/consensus_archaic.rs b/crates/navigator-store/src/consensus_archaic.rs deleted file mode 100644 index 26601fa9..00000000 --- a/crates/navigator-store/src/consensus_archaic.rs +++ /dev/null @@ -1,104 +0,0 @@ -//! Cached archaic (Neanderthal / Denisovan) Tier-A marker count per subject, keyed to the autosomal -//! consensus signature it was computed from. The app upserts on (re)compute and reads back when the signature still -//! matches the current consensus — otherwise it recomputes. One row per biosample. Mirrors -//! [`crate::consensus_painting`]. - -use du_domain::ids::SampleGuid; -use sqlx::SqlitePool; - -use crate::StoreError; - -/// A stored archaic marker count result: the consensus signature it was computed from + the full result (opaque JSON). -#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] -pub struct StoredArchaic { - pub biosample_guid: String, - pub consensus_sig: String, - pub archaic: String, - pub computed_at: String, -} - -/// Insert or replace the cached archaic marker count result for a biosample. -pub async fn upsert( - pool: &SqlitePool, - guid: SampleGuid, - consensus_sig: &str, - archaic: &str, - computed_at: &str, -) -> Result<(), StoreError> { - sqlx::query( - "INSERT INTO consensus_archaic (biosample_guid, consensus_sig, archaic, computed_at) \ - VALUES (?, ?, ?, ?) \ - ON CONFLICT(biosample_guid) DO UPDATE SET \ - consensus_sig = excluded.consensus_sig, archaic = excluded.archaic, computed_at = excluded.computed_at", - ) - .bind(guid.0.to_string()) - .bind(consensus_sig) - .bind(archaic) - .bind(computed_at) - .execute(pool) - .await?; - Ok(()) -} - -/// The cached archaic marker count result for a biosample, if one exists (caller checks the signature for staleness). -pub async fn get(pool: &SqlitePool, guid: SampleGuid) -> Result, StoreError> { - let row: Option = sqlx::query_as("SELECT * FROM consensus_archaic WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .fetch_optional(pool) - .await?; - Ok(row) -} - -/// Remove a biosample's cached archaic marker count result. -pub async fn delete(pool: &SqlitePool, guid: SampleGuid) -> Result { - let affected = sqlx::query("DELETE FROM consensus_archaic WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .execute(pool) - .await? - .rows_affected(); - Ok(affected > 0) -} - -#[cfg(test)] -mod tests { - use super::*; - use uuid::Uuid; - - #[tokio::test] - async fn upsert_get_delete_round_trip() { - let pool = crate::Store::open_in_memory().await.unwrap(); - let g = SampleGuid(Uuid::new_v4()); - let bio = navigator_domain::workspace::Biosample { - guid: g, - sample_accession: None, - donor_identifier: "S1".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }; - crate::biosample::create(pool.pool(), &bio).await.unwrap(); - - assert!(get(pool.pool(), g).await.unwrap().is_none()); - upsert(pool.pool(), g, "2026-07-22T00:00:00Z", "{}", "2026-07-22T01:00:00Z") - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-07-22T00:00:00Z"); - // Upsert replaces (a recompute after a consensus rebuild). - upsert( - pool.pool(), - g, - "2026-07-23T00:00:00Z", - r#"{"segments":[]}"#, - "2026-07-23T01:00:00Z", - ) - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-07-23T00:00:00Z"); - assert_eq!(got.archaic, r#"{"segments":[]}"#); - assert!(delete(pool.pool(), g).await.unwrap()); - assert!(get(pool.pool(), g).await.unwrap().is_none()); - } -} diff --git a/crates/navigator-store/src/consensus_archaic_segments.rs b/crates/navigator-store/src/consensus_archaic_segments.rs deleted file mode 100644 index 5c5401f8..00000000 --- a/crates/navigator-store/src/consensus_archaic_segments.rs +++ /dev/null @@ -1,107 +0,0 @@ -//! Cached **Tier B** archaic SEGMENT calls per subject. -//! -//! Keyed to the alignment they were called from rather than to the autosomal consensus: segments -//! come from genome-wide de-novo diploid calls on one alignment, whereas the consensus only carries -//! the 1240k panel loci. `source_sig` is the alignment id plus the caller's genotype version, so -//! re-calling with a newer caller invalidates the cache. Mirrors [`crate::consensus_archaic`]. - -use du_domain::ids::SampleGuid; -use sqlx::SqlitePool; - -use crate::StoreError; - -/// A stored segments marker count result: the consensus signature it was computed from + the full result (opaque JSON). -#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] -pub struct StoredArchaicSegments { - pub biosample_guid: String, - pub source_sig: String, - pub segments: String, - pub computed_at: String, -} - -/// Insert or replace the cached segments marker count result for a biosample. -pub async fn upsert( - pool: &SqlitePool, - guid: SampleGuid, - source_sig: &str, - segments: &str, - computed_at: &str, -) -> Result<(), StoreError> { - sqlx::query( - "INSERT INTO consensus_archaic_segments (biosample_guid, source_sig, segments, computed_at) \ - VALUES (?, ?, ?, ?) \ - ON CONFLICT(biosample_guid) DO UPDATE SET \ - source_sig = excluded.source_sig, segments = excluded.segments, computed_at = excluded.computed_at", - ) - .bind(guid.0.to_string()) - .bind(source_sig) - .bind(segments) - .bind(computed_at) - .execute(pool) - .await?; - Ok(()) -} - -/// The cached segments marker count result for a biosample, if one exists (caller checks the signature for staleness). -pub async fn get(pool: &SqlitePool, guid: SampleGuid) -> Result, StoreError> { - let row: Option = - sqlx::query_as("SELECT * FROM consensus_archaic_segments WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .fetch_optional(pool) - .await?; - Ok(row) -} - -/// Remove a biosample's cached segments marker count result. -pub async fn delete(pool: &SqlitePool, guid: SampleGuid) -> Result { - let affected = sqlx::query("DELETE FROM consensus_archaic_segments WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .execute(pool) - .await? - .rows_affected(); - Ok(affected > 0) -} - -#[cfg(test)] -mod tests { - use super::*; - use uuid::Uuid; - - #[tokio::test] - async fn upsert_get_delete_round_trip() { - let pool = crate::Store::open_in_memory().await.unwrap(); - let g = SampleGuid(Uuid::new_v4()); - let bio = navigator_domain::workspace::Biosample { - guid: g, - sample_accession: None, - donor_identifier: "S1".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }; - crate::biosample::create(pool.pool(), &bio).await.unwrap(); - - assert!(get(pool.pool(), g).await.unwrap().is_none()); - upsert(pool.pool(), g, "2026-07-22T00:00:00Z", "{}", "2026-07-22T01:00:00Z") - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.source_sig, "2026-07-22T00:00:00Z"); - // Upsert replaces (a recompute after a consensus rebuild). - upsert( - pool.pool(), - g, - "2026-07-23T00:00:00Z", - r#"{"segments":[]}"#, - "2026-07-23T01:00:00Z", - ) - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.source_sig, "2026-07-23T00:00:00Z"); - assert_eq!(got.segments, r#"{"segments":[]}"#); - assert!(delete(pool.pool(), g).await.unwrap()); - assert!(get(pool.pool(), g).await.unwrap().is_none()); - } -} diff --git a/crates/navigator-store/src/consensus_painting.rs b/crates/navigator-store/src/consensus_painting.rs deleted file mode 100644 index 27eb2964..00000000 --- a/crates/navigator-store/src/consensus_painting.rs +++ /dev/null @@ -1,103 +0,0 @@ -//! Cached chromosome painting (local-ancestry segments) per subject, keyed to the autosomal -//! consensus signature it was computed from. The app upserts on (re)paint and reads back when the -//! signature still matches the current consensus — otherwise it recomputes. One row per biosample. - -use du_domain::ids::SampleGuid; -use sqlx::SqlitePool; - -use crate::StoreError; - -/// A stored painting: the consensus signature it was painted from + the segments (opaque JSON). -#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] -pub struct StoredPainting { - pub biosample_guid: String, - pub consensus_sig: String, - pub segments: String, - pub painted_at: String, -} - -/// Insert or replace the cached painting for a biosample. -pub async fn upsert( - pool: &SqlitePool, - guid: SampleGuid, - consensus_sig: &str, - segments: &str, - painted_at: &str, -) -> Result<(), StoreError> { - sqlx::query( - "INSERT INTO consensus_painting (biosample_guid, consensus_sig, segments, painted_at) \ - VALUES (?, ?, ?, ?) \ - ON CONFLICT(biosample_guid) DO UPDATE SET \ - consensus_sig = excluded.consensus_sig, segments = excluded.segments, painted_at = excluded.painted_at", - ) - .bind(guid.0.to_string()) - .bind(consensus_sig) - .bind(segments) - .bind(painted_at) - .execute(pool) - .await?; - Ok(()) -} - -/// The cached painting for a biosample, if one exists (caller checks the signature for staleness). -pub async fn get(pool: &SqlitePool, guid: SampleGuid) -> Result, StoreError> { - let row: Option = sqlx::query_as("SELECT * FROM consensus_painting WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .fetch_optional(pool) - .await?; - Ok(row) -} - -/// Remove a biosample's cached painting. -pub async fn delete(pool: &SqlitePool, guid: SampleGuid) -> Result { - let affected = sqlx::query("DELETE FROM consensus_painting WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .execute(pool) - .await? - .rows_affected(); - Ok(affected > 0) -} - -#[cfg(test)] -mod tests { - use super::*; - use uuid::Uuid; - - #[tokio::test] - async fn upsert_get_delete_round_trip() { - let pool = crate::Store::open_in_memory().await.unwrap(); - let g = SampleGuid(Uuid::new_v4()); - let bio = navigator_domain::workspace::Biosample { - guid: g, - sample_accession: None, - donor_identifier: "S1".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }; - crate::biosample::create(pool.pool(), &bio).await.unwrap(); - - assert!(get(pool.pool(), g).await.unwrap().is_none()); - upsert(pool.pool(), g, "2026-06-15T00:00:00Z", "[]", "2026-06-15T01:00:00Z") - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-06-15T00:00:00Z"); - // Upsert replaces (a re-paint after a consensus rebuild). - upsert( - pool.pool(), - g, - "2026-06-16T00:00:00Z", - r#"[{"x":1}]"#, - "2026-06-16T01:00:00Z", - ) - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-06-16T00:00:00Z"); - assert_eq!(got.segments, r#"[{"x":1}]"#); - assert!(delete(pool.pool(), g).await.unwrap()); - assert!(get(pool.pool(), g).await.unwrap().is_none()); - } -} diff --git a/crates/navigator-store/src/consensus_roh.rs b/crates/navigator-store/src/consensus_roh.rs deleted file mode 100644 index 3fe7243d..00000000 --- a/crates/navigator-store/src/consensus_roh.rs +++ /dev/null @@ -1,104 +0,0 @@ -//! Cached runs-of-homozygosity (ROH) result per subject, keyed to the autosomal consensus signature -//! it was computed from. The app upserts on (re)compute and reads back when the signature still -//! matches the current consensus — otherwise it recomputes. One row per biosample. Mirrors -//! [`crate::consensus_painting`]. - -use du_domain::ids::SampleGuid; -use sqlx::SqlitePool; - -use crate::StoreError; - -/// A stored ROH result: the consensus signature it was computed from + the full result (opaque JSON). -#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] -pub struct StoredRoh { - pub biosample_guid: String, - pub consensus_sig: String, - pub roh: String, - pub computed_at: String, -} - -/// Insert or replace the cached ROH result for a biosample. -pub async fn upsert( - pool: &SqlitePool, - guid: SampleGuid, - consensus_sig: &str, - roh: &str, - computed_at: &str, -) -> Result<(), StoreError> { - sqlx::query( - "INSERT INTO consensus_roh (biosample_guid, consensus_sig, roh, computed_at) \ - VALUES (?, ?, ?, ?) \ - ON CONFLICT(biosample_guid) DO UPDATE SET \ - consensus_sig = excluded.consensus_sig, roh = excluded.roh, computed_at = excluded.computed_at", - ) - .bind(guid.0.to_string()) - .bind(consensus_sig) - .bind(roh) - .bind(computed_at) - .execute(pool) - .await?; - Ok(()) -} - -/// The cached ROH result for a biosample, if one exists (caller checks the signature for staleness). -pub async fn get(pool: &SqlitePool, guid: SampleGuid) -> Result, StoreError> { - let row: Option = sqlx::query_as("SELECT * FROM consensus_roh WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .fetch_optional(pool) - .await?; - Ok(row) -} - -/// Remove a biosample's cached ROH result. -pub async fn delete(pool: &SqlitePool, guid: SampleGuid) -> Result { - let affected = sqlx::query("DELETE FROM consensus_roh WHERE biosample_guid = ?") - .bind(guid.0.to_string()) - .execute(pool) - .await? - .rows_affected(); - Ok(affected > 0) -} - -#[cfg(test)] -mod tests { - use super::*; - use uuid::Uuid; - - #[tokio::test] - async fn upsert_get_delete_round_trip() { - let pool = crate::Store::open_in_memory().await.unwrap(); - let g = SampleGuid(Uuid::new_v4()); - let bio = navigator_domain::workspace::Biosample { - guid: g, - sample_accession: None, - donor_identifier: "S1".into(), - description: None, - center_name: None, - sex: None, - project_id: None, - }; - crate::biosample::create(pool.pool(), &bio).await.unwrap(); - - assert!(get(pool.pool(), g).await.unwrap().is_none()); - upsert(pool.pool(), g, "2026-07-22T00:00:00Z", "{}", "2026-07-22T01:00:00Z") - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-07-22T00:00:00Z"); - // Upsert replaces (a recompute after a consensus rebuild). - upsert( - pool.pool(), - g, - "2026-07-23T00:00:00Z", - r#"{"segments":[]}"#, - "2026-07-23T01:00:00Z", - ) - .await - .unwrap(); - let got = get(pool.pool(), g).await.unwrap().unwrap(); - assert_eq!(got.consensus_sig, "2026-07-23T00:00:00Z"); - assert_eq!(got.roh, r#"{"segments":[]}"#); - assert!(delete(pool.pool(), g).await.unwrap()); - assert!(get(pool.pool(), g).await.unwrap().is_none()); - } -} diff --git a/crates/navigator-store/src/lib.rs b/crates/navigator-store/src/lib.rs index cf95314f..ca6290c2 100644 --- a/crates/navigator-store/src/lib.rs +++ b/crates/navigator-store/src/lib.rs @@ -14,11 +14,7 @@ pub mod artifact; pub mod biosample; pub mod biosample_project; pub mod chip_profile; -pub mod consensus_archaic; -pub mod consensus_archaic_segments; -pub mod consensus_painting; pub mod consensus_profile; -pub mod consensus_roh; pub mod dm; pub mod error; pub mod external_id; @@ -32,6 +28,7 @@ pub mod mtdna; pub mod project; pub mod reconciliation; pub mod sequence_run; +pub mod sig_cache; pub mod source_file; pub mod str_profile; pub mod sync_history; diff --git a/crates/navigator-store/src/sig_cache.rs b/crates/navigator-store/src/sig_cache.rs new file mode 100644 index 00000000..53a87793 --- /dev/null +++ b/crates/navigator-store/src/sig_cache.rs @@ -0,0 +1,191 @@ +//! Signature-keyed result caches — one row per biosample, holding an opaque JSON result plus the +//! signature of the input it was computed from. +//! +//! Four tables share this exact shape, and the app uses them the same way every time: read the row, +//! compare its signature against what the current inputs hash to, and recompute on a mismatch. They +//! were four hand-copied modules until this one replaced them; the copies had already drifted (the +//! two purge paths in the app each forgot a different table), which is the argument for having one. +//! +//! The columns are *named* differently per table for historical reasons — `consensus_sig` vs +//! `source_sig`, `roh` vs `archaic` vs `segments`, `computed_at` vs `painted_at` — so each cache +//! carries its column names and `get` aliases them back to the common [`Cached`] shape. The schema +//! is untouched; only the Rust side is unified. + +use du_domain::ids::SampleGuid; +use sqlx::SqlitePool; + +use crate::StoreError; + +/// A cached result: the signature of the inputs it came from, the result itself as opaque JSON, +/// and when it was computed. The caller compares [`Cached::sig`] against the current inputs to +/// decide whether the payload is still good. +#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] +pub struct Cached { + pub biosample_guid: String, + pub sig: String, + pub payload: String, + pub computed_at: String, +} + +/// One signature-keyed cache table, identified by its name and its three non-key columns. +#[derive(Debug, Clone, Copy)] +pub struct SigCache { + /// The table name. A compile-time constant in every case — never caller input — so + /// interpolating it into SQL is safe. + table: &'static str, + sig_col: &'static str, + payload_col: &'static str, + at_col: &'static str, +} + +/// Cached chromosome painting (local-ancestry segments), keyed to the autosomal consensus's +/// `last_reconciled_at`. +pub const PAINTING: SigCache = SigCache::new("consensus_painting", "consensus_sig", "segments", "painted_at"); + +/// Cached runs-of-homozygosity result (segments + summary), keyed to the autosomal consensus. +pub const ROH: SigCache = SigCache::new("consensus_roh", "consensus_sig", "roh", "computed_at"); + +/// Cached archaic (Neanderthal / Denisovan) **Tier A** marker count, keyed to the autosomal +/// consensus. +pub const ARCHAIC: SigCache = SigCache::new("consensus_archaic", "consensus_sig", "archaic", "computed_at"); + +/// Cached archaic **Tier B** segment calls. Keyed to the *alignment* they were called from rather +/// than to the consensus: segments come from genome-wide de-novo diploid calls on one alignment, +/// whereas the consensus only carries the 1240k panel loci. The signature is the alignment id plus +/// the caller's genotype version, so re-calling with a newer caller invalidates the cache. +pub const ARCHAIC_SEGMENTS: SigCache = + SigCache::new("consensus_archaic_segments", "source_sig", "segments", "computed_at"); + +/// Every signature-keyed cache, in one list — so a purge that means "drop this subject's derived +/// results" drops *all* of them. Both purge paths used to enumerate tables by hand and both had +/// fallen behind the set. +pub const ALL: [SigCache; 4] = [PAINTING, ROH, ARCHAIC, ARCHAIC_SEGMENTS]; + +impl SigCache { + const fn new(table: &'static str, sig_col: &'static str, payload_col: &'static str, at_col: &'static str) -> Self { + SigCache { + table, + sig_col, + payload_col, + at_col, + } + } + + /// The table this cache lives in — for callers that must fold it into a wider delete inside + /// their own transaction, where [`SigCache::delete`]'s pool-level call would not enlist. + pub const fn table(&self) -> &'static str { + self.table + } + + /// Insert or replace this biosample's cached result. + pub async fn upsert( + &self, + pool: &SqlitePool, + guid: SampleGuid, + sig: &str, + payload: &str, + computed_at: &str, + ) -> Result<(), StoreError> { + let (table, s, p, a) = (self.table, self.sig_col, self.payload_col, self.at_col); + sqlx::query(&format!( + "INSERT INTO {table} (biosample_guid, {s}, {p}, {a}) VALUES (?, ?, ?, ?) \ + ON CONFLICT(biosample_guid) DO UPDATE SET \ + {s} = excluded.{s}, {p} = excluded.{p}, {a} = excluded.{a}" + )) + .bind(guid.0.to_string()) + .bind(sig) + .bind(payload) + .bind(computed_at) + .execute(pool) + .await?; + Ok(()) + } + + /// This biosample's cached result, if one exists. The caller checks [`Cached::sig`] for + /// staleness — a row here is not by itself a usable result. + pub async fn get(&self, pool: &SqlitePool, guid: SampleGuid) -> Result, StoreError> { + let (table, s, p, a) = (self.table, self.sig_col, self.payload_col, self.at_col); + let row: Option = sqlx::query_as(&format!( + "SELECT biosample_guid, {s} AS sig, {p} AS payload, {a} AS computed_at \ + FROM {table} WHERE biosample_guid = ?" + )) + .bind(guid.0.to_string()) + .fetch_optional(pool) + .await?; + Ok(row) + } + + /// Remove this biosample's cached result. `false` means there was nothing to remove. + pub async fn delete(&self, pool: &SqlitePool, guid: SampleGuid) -> Result { + let affected = sqlx::query(&format!("DELETE FROM {} WHERE biosample_guid = ?", self.table)) + .bind(guid.0.to_string()) + .execute(pool) + .await? + .rows_affected(); + Ok(affected > 0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use uuid::Uuid; + + /// Every cache round-trips, and upsert replaces rather than duplicating (a recompute after the + /// inputs changed). Running the same body over [`ALL`] is what keeps a newly added table from + /// silently going untested. + #[tokio::test] + async fn every_cache_round_trips_and_upsert_replaces() { + let pool = crate::Store::open_in_memory().await.unwrap(); + let g = SampleGuid(Uuid::new_v4()); + crate::biosample::create(pool.pool(), &navigator_domain::workspace::Biosample::new(g, "S1")) + .await + .unwrap(); + + for cache in ALL { + assert!(cache.get(pool.pool(), g).await.unwrap().is_none(), "{cache:?}"); + cache + .upsert(pool.pool(), g, "sig-1", "{}", "2026-07-22T01:00:00Z") + .await + .unwrap(); + let got = cache.get(pool.pool(), g).await.unwrap().unwrap(); + assert_eq!(got.sig, "sig-1", "{cache:?}"); + assert_eq!(got.payload, "{}", "{cache:?}"); + + cache + .upsert(pool.pool(), g, "sig-2", r#"{"segments":[]}"#, "2026-07-23T01:00:00Z") + .await + .unwrap(); + let got = cache.get(pool.pool(), g).await.unwrap().unwrap(); + assert_eq!(got.sig, "sig-2", "{cache:?}"); + assert_eq!(got.payload, r#"{"segments":[]}"#, "{cache:?}"); + assert_eq!(got.computed_at, "2026-07-23T01:00:00Z", "{cache:?}"); + + assert!(cache.delete(pool.pool(), g).await.unwrap(), "{cache:?}"); + assert!(cache.get(pool.pool(), g).await.unwrap().is_none(), "{cache:?}"); + } + } + + /// The caches are independent: writing one must not disturb another that happens to share a + /// column name (`segments` is `consensus_painting`'s *and* `consensus_archaic_segments`'s). + #[tokio::test] + async fn caches_do_not_alias_each_other() { + let pool = crate::Store::open_in_memory().await.unwrap(); + let g = SampleGuid(Uuid::new_v4()); + crate::biosample::create(pool.pool(), &navigator_domain::workspace::Biosample::new(g, "S1")) + .await + .unwrap(); + + PAINTING.upsert(pool.pool(), g, "sig-p", "painted", "t").await.unwrap(); + ARCHAIC_SEGMENTS + .upsert(pool.pool(), g, "sig-a", "called", "t") + .await + .unwrap(); + + assert_eq!(PAINTING.get(pool.pool(), g).await.unwrap().unwrap().payload, "painted"); + assert_eq!( + ARCHAIC_SEGMENTS.get(pool.pool(), g).await.unwrap().unwrap().payload, + "called" + ); + } +}