From 59904f25e6ebc7ca36026e312814de4669fd6c42 Mon Sep 17 00:00:00 2001 From: angela-helios Date: Thu, 30 Jul 2026 19:35:36 -0400 Subject: [PATCH] fix(ui): page the conformance self-fetch and never cache an empty registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-fetch asked for 10000 in one page and trusted the answer, but a server capping _count at 1000 silently truncated the registry — 1377 R4 SearchParameters served as 1000, and the viewer under-reported without a trace (#460). The fetch now follows next links (rebased onto the loopback base, since the advertised link carries the server's public base URL) and fails the whole fetch if any page fails, because a silently partial registry is the bug this exists to fix. The catalogs also stop caching an *empty success* (#462): storage seeds the conformance resources at startup, so an empty search answer means the backend's index has not caught up (a composite's ES sync), and pinning it kept the pages broken long after the sync landed. Empty is now served for its request only, like a failure already was. Closes #460 --- crates/ui/src/compartments.rs | 46 ++++++++- crates/ui/src/conformance.rs | 173 +++++++++++++++++++++++++++------ crates/ui/src/search_params.rs | 47 ++++++++- 3 files changed, 232 insertions(+), 34 deletions(-) diff --git a/crates/ui/src/compartments.rs b/crates/ui/src/compartments.rs index e858f93b0..7d122210b 100644 --- a/crates/ui/src/compartments.rs +++ b/crates/ui/src/compartments.rs @@ -95,8 +95,12 @@ impl CompartmentCatalog { defs.sort_by(|a, b| a.code.cmp(&b.code)); let built = Arc::new(defs); // A failed fetch is served empty for this request only — caching it - // would pin the page to the failure until restart. - if !fetch_ok { + // would pin the page to the failure until restart. An *empty success* + // is treated the same (#462): the server seeds the spec definitions + // at startup, so emptiness means the search path hasn't caught up + // (a composite backend's index still syncing), and caching it would + // keep the page broken long after the sync lands. + if !fetch_ok || built.is_empty() { return built; } self.cache @@ -530,6 +534,44 @@ mod tests { #[cfg(feature = "R4")] const R4: FhirVersion = FhirVersion::R4; + /// A source whose first fetch is empty and later fetches carry data, the + /// way a composite backend behaves while its search index still syncs. + struct WarmingSource(std::sync::atomic::AtomicUsize); + + #[async_trait::async_trait] + impl ConformanceSource for WarmingSource { + async fn fetch( + &self, + _rt: &str, + _v: FhirVersion, + _t: &str, + ) -> Result, String> { + let call = self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if call == 0 { + return Ok(Vec::new()); + } + Ok(vec![serde_json::json!({ + "resourceType": "CompartmentDefinition", + "url": "http://example.org/CompartmentDefinition/patient", + "status": "active", + "code": "Patient", + "search": true, + "resource": [] + })]) + } + } + + /// #462: an empty success must not be cached — the next request retries + /// and sees the definitions once the backend's index catches up. + #[tokio::test] + async fn empty_definitions_are_not_pinned() { + let catalog = CompartmentCatalog::new(Arc::new(WarmingSource(0.into()))); + let cold = catalog.definitions("t", FhirVersion::default()).await; + assert!(cold.is_empty(), "first fetch is empty"); + let warm = catalog.definitions("t", FhirVersion::default()).await; + assert_eq!(warm.len(), 1, "second request re-fetched"); + } + /// The R4 compartment definitions, parsed from the shipped `data/` bundle /// exactly as an HTTP fetch would deliver them — keeps the test offline. #[cfg(feature = "R4")] diff --git a/crates/ui/src/conformance.rs b/crates/ui/src/conformance.rs index 3cf62ba3c..1ca7ecc18 100644 --- a/crates/ui/src/conformance.rs +++ b/crates/ui/src/conformance.rs @@ -63,38 +63,75 @@ impl ConformanceSource for HttpConformanceSource { _version: FhirVersion, tenant: &str, ) -> Result, String> { - // A single page large enough to hold the whole conformance set (~1.4k - // SearchParameters per version): the UI needs the full list for its - // facets and rail, and paginates in-memory. Capped at 10000 — the - // Elasticsearch max_result_window — so the search also succeeds on - // backends that delegate search to ES. - let url = format!("{}/{}?_count=10000", self.base_url, resource_type); - let mut request = self - .client - .get(&url) - .header("Accept", "application/fhir+json"); - // Scope the self-call to the effective tenant (#344); an empty id means - // the server default and needs no header. - if !tenant.is_empty() { - request = request.header("X-Tenant-ID", tenant); + // Ask for everything at once, then follow `next` links for whatever + // the server's `_count` policy withheld (#460): the request says + // 10000, but a server capping at 1000 used to silently truncate the + // registry (1377 R4 SearchParameters served as 1000). The UI needs + // the full list for its facets and rail, and paginates in-memory. + let mut url = format!("{}/{}?_count=10000", self.base_url, resource_type); + let mut resources = Vec::new(); + // Generous page bound — only a runaway self-linking server hits it. + for _ in 0..100 { + let mut request = self + .client + .get(&url) + .header("Accept", "application/fhir+json"); + // Scope the self-call to the effective tenant (#344); an empty id + // means the server default and needs no header. + if !tenant.is_empty() { + request = request.header("X-Tenant-ID", tenant); + } + let request = self + .outbound_auth + .authorize(request, &self.base_url) + .await + .map_err(|e| format!("outbound auth failed: {e}"))?; + let response = request + .send() + .await + .map_err(|e| format!("request to {url} failed: {e}"))?; + if !response.status().is_success() { + // A failed page fails the fetch: serving a silently partial + // registry is exactly the bug this loop exists to fix. + return Err(format!("{url} returned {}", response.status())); + } + let bundle: Value = response + .json() + .await + .map_err(|e| format!("parsing {resource_type} bundle failed: {e}"))?; + resources.extend(extract_bundle_resources(&bundle)); + match next_link(&bundle) { + // The advertised link carries the server's own idea of its base + // URL, which need not be the loopback this client targets — + // keep the path + query, swap in our base. + Some(next) => url = rebase_link(&next, &self.base_url), + None => break, + } } - let request = self - .outbound_auth - .authorize(request, &self.base_url) - .await - .map_err(|e| format!("outbound auth failed: {e}"))?; - let response = request - .send() - .await - .map_err(|e| format!("request to {url} failed: {e}"))?; - if !response.status().is_success() { - return Err(format!("{url} returned {}", response.status())); + Ok(resources) + } +} + +/// The `next` page URL of a searchset Bundle, if any. +fn next_link(bundle: &Value) -> Option { + bundle + .get("link")? + .as_array()? + .iter() + .find(|l| l.get("relation").and_then(Value::as_str) == Some("next"))? + .get("url")? + .as_str() + .map(String::from) +} + +/// Points a server-advertised link at `base_url`, keeping its path and query. +fn rebase_link(link: &str, base_url: &str) -> String { + match link.find("://").and_then(|i| link[i + 3..].find('/')) { + Some(slash) => { + let i = link.find("://").unwrap() + 3; + format!("{}{}", base_url, &link[i + slash..]) } - let bundle: Value = response - .json() - .await - .map_err(|e| format!("parsing {resource_type} bundle failed: {e}"))?; - Ok(extract_bundle_resources(&bundle)) + None => link.to_string(), } } @@ -176,3 +213,79 @@ impl ConformanceSource for StaticConformanceSource { .unwrap_or_default()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn next_link_finds_the_next_relation() { + let bundle = serde_json::json!({ + "link": [ + {"relation": "self", "url": "http://s/SearchParameter?_count=1000"}, + {"relation": "next", "url": "http://s/SearchParameter?_count=1000&_offset=1000"} + ] + }); + assert_eq!( + next_link(&bundle).as_deref(), + Some("http://s/SearchParameter?_count=1000&_offset=1000") + ); + assert_eq!(next_link(&serde_json::json!({"link": []})), None); + } + + #[test] + fn rebase_link_swaps_the_advertised_base_for_ours() { + assert_eq!( + rebase_link( + "http://localhost:8080/SearchParameter?_offset=1000", + "http://127.0.0.1:9999" + ), + "http://127.0.0.1:9999/SearchParameter?_offset=1000" + ); + } + + /// #460: a server that caps `_count` answers in pages; the fetch must + /// follow `next` links and return the union, not the first page. + #[tokio::test] + async fn http_fetch_follows_next_links() { + use axum::{Router, extract::Query, routing::get}; + use std::collections::HashMap; + + async fn page(Query(q): Query>) -> axum::Json { + let offset: usize = q.get("_offset").map(|o| o.parse().unwrap()).unwrap_or(0); + let mut bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "searchset", + "entry": [{"resource": {"resourceType": "SearchParameter", "id": format!("sp-{offset}")}}] + }); + if offset == 0 { + // Advertise the next page under a base URL that is not the + // one the client dialed, like a server with a configured + // public base does. + bundle["link"] = serde_json::json!([ + {"relation": "next", "url": "http://advertised.invalid/SearchParameter?_offset=1"} + ]); + } + axum::Json(bundle) + } + + let app = Router::new().route("/SearchParameter", get(page)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + + let source = HttpConformanceSource::new( + format!("http://{addr}"), + std::sync::Arc::new(helios_auth::outbound::NoOpOutboundAuthProvider), + ); + let resources = source + .fetch("SearchParameter", FhirVersion::R4, "") + .await + .expect("fetch succeeds"); + let ids: Vec<_> = resources + .iter() + .map(|r| r["id"].as_str().unwrap().to_string()) + .collect(); + assert_eq!(ids, vec!["sp-0", "sp-1"]); + } +} diff --git a/crates/ui/src/search_params.rs b/crates/ui/src/search_params.rs index 0c578e255..f1c1b1ed8 100644 --- a/crates/ui/src/search_params.rs +++ b/crates/ui/src/search_params.rs @@ -59,8 +59,11 @@ impl SpCatalog { let built = Arc::new(fetch_snapshot(&*self.source, version, tenant).await); // A failed fetch (`spec_loaded == false`) is served degraded for this // request only — caching it would pin the page to the failure until - // restart. - if !built.spec_loaded { + // restart. An *empty success* gets the same treatment (#462): storage + // seeds the spec parameters at startup, so an empty registry means + // the search path hasn't caught up yet (a composite backend's index + // still syncing), not that there is nothing to show. + if !built.spec_loaded || built.params.is_empty() { return built; } // Another task may have raced us here; keep whichever landed first. @@ -695,6 +698,46 @@ fn page_links(page: usize, page_count: usize) -> Vec { mod tests { use super::*; + /// A source whose first fetch is empty and later fetches carry data, the + /// way a composite backend behaves while its search index still syncs. + struct WarmingSource(std::sync::atomic::AtomicUsize); + + #[async_trait::async_trait] + impl ConformanceSource for WarmingSource { + async fn fetch( + &self, + _rt: &str, + _v: FhirVersion, + _t: &str, + ) -> Result, String> { + let call = self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if call == 0 { + return Ok(Vec::new()); + } + Ok(vec![serde_json::json!({ + "resourceType": "SearchParameter", + "url": "http://example.org/SearchParameter/color", + "name": "color", + "code": "color", + "status": "active", + "type": "token", + "base": ["Patient"], + "expression": "Patient.extension.value" + })]) + } + } + + /// #462: an empty success must not be cached — the next request retries + /// and sees the data once the backend's index catches up. + #[tokio::test] + async fn an_empty_snapshot_is_not_pinned() { + let catalog = SpCatalog::new(Arc::new(WarmingSource(0.into()))); + let cold = catalog.snapshot("t", FhirVersion::default()).await; + assert!(cold.params.is_empty(), "first fetch is empty"); + let warm = catalog.snapshot("t", FhirVersion::default()).await; + assert_eq!(warm.params.len(), 1, "second request re-fetched"); + } + fn snapshot() -> VersionSnapshot { // Tests run with the crate as CWD; the workspace spec bundles live two // levels up. Build the snapshot from the bundle's raw resources exactly