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