From 2a3b38fc6a3a8c599c8c882544de506dd965554f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 13:20:27 +0000 Subject: [PATCH 1/4] fix(service_loader): stop silently swallowing credential/config parse errors load() used if-let-Ok on load_credentials/load_configuration, discarding any error the same way whether the file was simply absent (fine) or present but malformed (a real bug that should surface). Now only a missing-file io::ErrorKind::NotFound is swallowed; everything else propagates. Fixes #17 --- usecases/service_loader/src/lib.rs | 116 +++++++++++++++++++++++++++-- 1 file changed, 110 insertions(+), 6 deletions(-) diff --git a/usecases/service_loader/src/lib.rs b/usecases/service_loader/src/lib.rs index 5190544..f993338 100644 --- a/usecases/service_loader/src/lib.rs +++ b/usecases/service_loader/src/lib.rs @@ -181,15 +181,18 @@ impl ServiceLoader { let mut value = load_service(fetcher, only_manifest)?; if !only_manifest && value.v1().manifest.v2().has_swagger() { - let creds = load_credentials(fetcher); - if let Ok(creds) = creds { - output.handle_credentials(id, creds)?; + match load_credentials(fetcher) { + Ok(creds) => output.handle_credentials(id, creds)?, + Err(error::ServiceLoader::Io { source }) if source.kind() == io::ErrorKind::NotFound => {} + Err(err) => return Err(err), } if merge_overrides { - let config = load_configuration(fetcher); - if let Ok(config) = config { - merge(&mut value, &config)?; + match load_configuration(fetcher) { + Ok(config) => merge(&mut value, &config)?, + Err(error::ServiceLoader::Io { source }) + if source.kind() == io::ErrorKind::NotFound => {} + Err(err) => return Err(err), } } } @@ -206,3 +209,104 @@ impl Default for ServiceLoader { Self::new() } } + +#[cfg(test)] +mod test { + #![allow(clippy::restriction, clippy::pedantic)] + + use std::cell::RefCell; + use std::collections::HashMap; + + use super::*; + + #[derive(Default)] + struct MockFetcher { + docs: RefCell>, + } + + impl MockFetcher { + fn with(self, location: &str, content: &str) -> Self { + self.docs + .borrow_mut() + .insert(location.to_owned(), content.to_owned()); + self + } + } + + impl Fetcher>> for MockFetcher { + fn fetch(&self, location: &str) -> io::Result>> { + self.docs + .borrow() + .get(location) + .map(|doc| io::Cursor::new(doc.clone().into_bytes())) + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "not found")) + } + } + + #[derive(Default)] + struct MockOutput { + credentials: Option, + } + + impl LoaderOutput for MockOutput { + fn handle_service(&mut self, _id: &str, _service: VersionedServiceTree) -> error::Result<()> { + Ok(()) + } + + fn handle_credentials( + &mut self, + _id: &str, + credentials: Authentication, + ) -> error::Result<()> { + self.credentials = Some(credentials); + Ok(()) + } + } + + fn manifest_with_swagger() -> String { + r#"{"v2":{"swagger":{"source":"openapi"}}}"#.to_owned() + } + + #[test] + fn load_skips_missing_credentials_and_config_files() { + let openapi_doc = include_str!("loaders/openapi/stubs/basic_root.yaml"); + let fetcher = MockFetcher::default() + .with(constants::MANIFEST_LOCATION, &manifest_with_swagger()) + .with("openapi", openapi_doc); + let mut output = MockOutput::default(); + + let result = ServiceLoader::new().load("svc", &fetcher, &mut output, true, false); + + assert!(result.is_ok(), "expected Ok, got {result:?}"); + assert!(output.credentials.is_none()); + } + + #[test] + fn load_propagates_malformed_credentials_instead_of_swallowing_them() { + let openapi_doc = include_str!("loaders/openapi/stubs/basic_root.yaml"); + let fetcher = MockFetcher::default() + .with(constants::MANIFEST_LOCATION, &manifest_with_swagger()) + .with("openapi", openapi_doc) + .with(constants::CREDENTIALS_LOCATION, "not valid json{{{"); + let mut output = MockOutput::default(); + + let result = ServiceLoader::new().load("svc", &fetcher, &mut output, false, false); + + assert!(result.is_err(), "expected malformed credentials to surface as an error, got Ok"); + assert!(output.credentials.is_none()); + } + + #[test] + fn load_propagates_malformed_config_instead_of_swallowing_it() { + let openapi_doc = include_str!("loaders/openapi/stubs/basic_root.yaml"); + let fetcher = MockFetcher::default() + .with(constants::MANIFEST_LOCATION, &manifest_with_swagger()) + .with("openapi", openapi_doc) + .with(constants::CONFIG_LOCATION, "not valid json{{{"); + let mut output = MockOutput::default(); + + let result = ServiceLoader::new().load("svc", &fetcher, &mut output, true, false); + + assert!(result.is_err(), "expected malformed config to surface as an error, got Ok"); + } +} From 23a816b975f52003d27031741bd0025c2d3e15c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 13:25:26 +0000 Subject: [PATCH 2/4] style: run cargo fmt CI's fmt check flagged the previous commit's formatting. --- usecases/service_loader/src/lib.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/usecases/service_loader/src/lib.rs b/usecases/service_loader/src/lib.rs index f993338..30e129a 100644 --- a/usecases/service_loader/src/lib.rs +++ b/usecases/service_loader/src/lib.rs @@ -183,7 +183,8 @@ impl ServiceLoader { if !only_manifest && value.v1().manifest.v2().has_swagger() { match load_credentials(fetcher) { Ok(creds) => output.handle_credentials(id, creds)?, - Err(error::ServiceLoader::Io { source }) if source.kind() == io::ErrorKind::NotFound => {} + Err(error::ServiceLoader::Io { source }) + if source.kind() == io::ErrorKind::NotFound => {} Err(err) => return Err(err), } @@ -249,7 +250,11 @@ mod test { } impl LoaderOutput for MockOutput { - fn handle_service(&mut self, _id: &str, _service: VersionedServiceTree) -> error::Result<()> { + fn handle_service( + &mut self, + _id: &str, + _service: VersionedServiceTree, + ) -> error::Result<()> { Ok(()) } @@ -292,7 +297,10 @@ mod test { let result = ServiceLoader::new().load("svc", &fetcher, &mut output, false, false); - assert!(result.is_err(), "expected malformed credentials to surface as an error, got Ok"); + assert!( + result.is_err(), + "expected malformed credentials to surface as an error, got Ok" + ); assert!(output.credentials.is_none()); } @@ -307,6 +315,9 @@ mod test { let result = ServiceLoader::new().load("svc", &fetcher, &mut output, true, false); - assert!(result.is_err(), "expected malformed config to surface as an error, got Ok"); + assert!( + result.is_err(), + "expected malformed config to surface as an error, got Ok" + ); } } From 45e075c0b06a7d5282d7339363a9fb24ae4d9192 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 13:32:22 +0000 Subject: [PATCH 3/4] refactor(service_loader,service_writer): dedupe 8-arm per-HTTP-verb logic collect_operations() hand-duplicated the same get/post/put/patch/delete/ head/options/trace block eight times; replaced with a loop over a verb-to-HttpMethodType table. service_writer::handle_path_items() had the mirror-image 8-arm match building the JSON path-item key; replaced with EnumFull's descriptor name (already used elsewhere in this file for InType), lowercased. Also folded the near-identical allOf/anyOf/oneOf branches in handle_schema() into one handle_composed_schema() helper. Added regression tests for handle_path_items (grouping by path/verb, rejecting an unset method) and handle_schema's composed-schema output. Fixes #6 --- .../service_loader/src/loaders/openapi/mod.rs | 155 +++----------- usecases/service_writer/src/lib.rs | 196 ++++++++++++------ 2 files changed, 167 insertions(+), 184 deletions(-) diff --git a/usecases/service_loader/src/loaders/openapi/mod.rs b/usecases/service_loader/src/loaders/openapi/mod.rs index 780e58a..9c6a165 100644 --- a/usecases/service_loader/src/loaders/openapi/mod.rs +++ b/usecases/service_loader/src/loaders/openapi/mod.rs @@ -74,6 +74,19 @@ fn get_server(server: &serde_json::Value) -> error::Result { required_field(server, "url") } +/// The `OpenAPI` path-item verbs [`collect_operations`] recognizes, paired +/// with the [`service::operation::HttpMethodType`] each one maps to. +const HTTP_METHODS: &[(&str, service::operation::HttpMethodType)] = &[ + ("get", service::operation::HttpMethodType::GET), + ("post", service::operation::HttpMethodType::POST), + ("put", service::operation::HttpMethodType::PUT), + ("patch", service::operation::HttpMethodType::PATCH), + ("delete", service::operation::HttpMethodType::DELETE), + ("head", service::operation::HttpMethodType::HEAD), + ("options", service::operation::HttpMethodType::OPTIONS), + ("trace", service::operation::HttpMethodType::TRACE), +]; + /// Resolves `item` (a path item, possibly a `$ref`) and converts each /// HTTP-verb entry it defines (get/post/put/patch/delete/head/options/ /// trace) into an `(operationId, Operation)` pair via [`handle_operation`], @@ -99,132 +112,22 @@ fn collect_operations( let mut result = Vec::new(); - if let Some(op) = item.get("get") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::GET.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("post") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::POST.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("put") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::PUT.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("patch") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::PATCH.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("delete") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::DELETE.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("head") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::HEAD.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("options") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::OPTIONS.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); - } - - if let Some(op) = item.get("trace") { - let mut common_op = service::Operation::new(); - common_op.path = path.to_owned(); - common_op.method = service::operation::HttpMethodType::TRACE.into(); - handle_operation( - op, - &mut common_op, - root, - fetcher, - cache, - schemas, - &common_params, - )?; - result.push((required_field(op, "operationId")?, common_op)); + for &(verb, method) in HTTP_METHODS { + if let Some(op) = item.get(verb) { + let mut common_op = service::Operation::new(); + common_op.path = path.to_owned(); + common_op.method = method.into(); + handle_operation( + op, + &mut common_op, + root, + fetcher, + cache, + schemas, + &common_params, + )?; + result.push((required_field(op, "operationId")?, common_op)); + } } Ok(result) diff --git a/usecases/service_writer/src/lib.rs b/usecases/service_writer/src/lib.rs index e525939..8295472 100644 --- a/usecases/service_writer/src/lib.rs +++ b/usecases/service_writer/src/lib.rs @@ -190,38 +190,20 @@ fn handle_path_items( let path_item = path_item .as_object_mut() .ok_or_else(|| error::ServiceWriter::InvalidType("Object".into()))?; - let path_item = match operation.method.enum_value() { - Ok(service::operation::HttpMethodType::GET) => path_item - .entry(String::from("get")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::POST) => path_item - .entry(String::from("post")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::PUT) => path_item - .entry(String::from("put")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::PATCH) => path_item - .entry(String::from("patch")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::DELETE) => path_item - .entry(String::from("delete")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::HEAD) => path_item - .entry(String::from("head")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::OPTIONS) => path_item - .entry(String::from("options")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), - Ok(service::operation::HttpMethodType::TRACE) => path_item - .entry(String::from("trace")) - .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())), + + let verb = match operation.method.enum_value() { Ok(service::operation::HttpMethodType::HTTP_METHOD_TYPE_NONE) | Err(_) => { return Err(error::ServiceWriter::Unimplemented( "Non Supported HTTP VERB".into(), )) } + Ok(method) => method.descriptor().name().to_lowercase(), }; + let path_item = path_item + .entry(verb) + .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())); + let path_item = path_item .as_object_mut() .ok_or_else(|| error::ServiceWriter::InvalidType("Object".into()))?; @@ -434,43 +416,13 @@ fn handle_schema( } } &Some(service::schema::Value::AllOf(ref values)) => { - let values: error::Result> = values - .schema - .iter() - .map(|common_schema| { - let mut schema = serde_json::Map::new(); - handle_schema(&mut schema, common_schema)?; - Ok(serde_json::Value::Object(schema)) - }) - .collect(); - - sink.insert("allOf".into(), values?.into()); + handle_composed_schema(sink, "allOf", values)?; } &Some(service::schema::Value::AnyOf(ref values)) => { - let values: error::Result> = values - .schema - .iter() - .map(|common_schema| { - let mut schema = serde_json::Map::new(); - handle_schema(&mut schema, common_schema)?; - Ok(serde_json::Value::Object(schema)) - }) - .collect(); - - sink.insert("anyOf".into(), values?.into()); + handle_composed_schema(sink, "anyOf", values)?; } &Some(service::schema::Value::OneOf(ref values)) => { - let values: error::Result> = values - .schema - .iter() - .map(|common_schema| { - let mut schema = serde_json::Map::new(); - handle_schema(&mut schema, common_schema)?; - Ok(serde_json::Value::Object(schema)) - }) - .collect(); - - sink.insert("oneOf".into(), values?.into()); + handle_composed_schema(sink, "oneOf", values)?; } _ => {} } @@ -478,6 +430,27 @@ fn handle_schema( Ok(()) } +/// Writes a `allOf`/`anyOf`/`oneOf` composition's branches (recursively +/// handled via [`handle_schema`]) into `sink` under `key`. +fn handle_composed_schema( + sink: &mut serde_json::Map, + key: &str, + composed: &service::ComposedSchema, +) -> error::Result<()> { + let values: error::Result> = composed + .schema + .iter() + .map(|common_schema| { + let mut schema = serde_json::Map::new(); + handle_schema(&mut schema, common_schema)?; + Ok(serde_json::Value::Object(schema)) + }) + .collect(); + + sink.insert(key.into(), values?.into()); + Ok(()) +} + #[cfg(test)] mod tests { use protobuf::EnumOrUnknown; @@ -500,4 +473,111 @@ mod tests { "expected an unrecognized parameter location to error instead of writing a bogus \"in\" value, got {result:?}" ); } + + #[test] + fn handle_path_items_groups_operations_by_path_and_lowercased_verb() { + let mut operations = HashMap::new(); + operations.insert( + "getThing".to_owned(), + service::Operation { + path: "/thing".to_owned(), + method: service::operation::HttpMethodType::GET.into(), + ..Default::default() + }, + ); + operations.insert( + "createThing".to_owned(), + service::Operation { + path: "/thing".to_owned(), + method: service::operation::HttpMethodType::POST.into(), + ..Default::default() + }, + ); + + let mut paths = serde_json::Map::new(); + handle_path_items(&mut paths, &operations).unwrap(); + + let path_item = paths.get("/thing").unwrap().as_object().unwrap(); + assert_eq!( + path_item.get("get").and_then(|op| op.get("operationId")), + Some(&serde_json::Value::from("getThing")) + ); + assert_eq!( + path_item.get("post").and_then(|op| op.get("operationId")), + Some(&serde_json::Value::from("createThing")) + ); + } + + #[test] + fn handle_path_items_rejects_an_unset_http_method() { + let mut operations = HashMap::new(); + operations.insert( + "mystery".to_owned(), + service::Operation { + path: "/thing".to_owned(), + ..Default::default() + }, + ); + + let mut paths = serde_json::Map::new(); + let result = handle_path_items(&mut paths, &operations); + + assert!( + matches!(result, Err(error::ServiceWriter::Unimplemented(_))), + "expected an unset HTTP method to error, got {result:?}" + ); + } + + #[test] + fn handle_schema_writes_composed_schema_branches_under_the_matching_key() { + let ref_branch = |name: &str| service::Schema { + value: Some(service::schema::Value::Ref(name.to_owned())), + ..Default::default() + }; + + for (value, key) in [ + ( + service::schema::Value::AllOf(service::ComposedSchema { + schema: vec![ref_branch("A"), ref_branch("B")], + ..Default::default() + }), + "allOf", + ), + ( + service::schema::Value::AnyOf(service::ComposedSchema { + schema: vec![ref_branch("A"), ref_branch("B")], + ..Default::default() + }), + "anyOf", + ), + ( + service::schema::Value::OneOf(service::ComposedSchema { + schema: vec![ref_branch("A"), ref_branch("B")], + ..Default::default() + }), + "oneOf", + ), + ] { + let source = service::Schema { + value: Some(value), + ..Default::default() + }; + + let mut sink = serde_json::Map::new(); + handle_schema(&mut sink, &source).unwrap(); + + let branches = sink + .get(key) + .unwrap_or_else(|| panic!("expected a \"{key}\" key in {sink:?}")) + .as_array() + .unwrap(); + assert_eq!( + *branches, + vec![ + serde_json::json!({ "$ref": "A" }), + serde_json::json!({ "$ref": "B" }), + ] + ); + } + } } From 1f1eafe66974c22a35c2b742159210467a0da1a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 14:18:51 +0000 Subject: [PATCH 4/4] test(api_caller): drain response bodies to fix connection-reuse test flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both connection-pooling tests sent two requests and asserted the second reused the first's pooled connection, but never read the first response's body. reqwest only returns a connection to its pool once the body is fully drained, so the assertion raced that release against the second request — observed failing in CI (jhamill34/api-tools#84) with an unrelated diff. Reading the body via .bytes() before the next request removes the race. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2 --- runners/api_caller/src/lib.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/runners/api_caller/src/lib.rs b/runners/api_caller/src/lib.rs index 9a06d2f..ecc62fc 100644 --- a/runners/api_caller/src/lib.rs +++ b/runners/api_caller/src/lib.rs @@ -1164,15 +1164,22 @@ mod tests { let (log, _log_handle) = LogWriter::spawn(tempfile::tempfile().unwrap()); let caller = APICaller::new(log); + // The response body must be fully drained before the connection is + // returned to reqwest's pool — leaving it unread races the second + // request against that release and flakes the assertion below. caller .client .get(format!("{base_url}/ping")) .send() + .unwrap() + .bytes() .unwrap(); caller .client .get(format!("{base_url}/ping")) .send() + .unwrap() + .bytes() .unwrap(); assert_eq!( @@ -1188,17 +1195,25 @@ mod tests { let (log, _log_handle) = LogWriter::spawn(tempfile::tempfile().unwrap()); let caller = AsyncAPICaller::new(log); + // See the sync test above: draining the body is what guarantees the + // connection is idle-pooled before the next request is sent. caller .client .get(format!("{base_url}/ping")) .send() .await + .unwrap() + .bytes() + .await .unwrap(); caller .client .get(format!("{base_url}/ping")) .send() .await + .unwrap() + .bytes() + .await .unwrap(); assert_eq!(