fix(service_loader): stop silently swallowing credential/config parse errors

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!(

usecases/service_loader/src/lib.rs

@@ -181,15 +181,19 @@ 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 +210,114 @@ 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<HashMap<String, String>>,
+    }
+
+    impl MockFetcher {
+        fn with(self, location: &str, content: &str) -> Self {
+            self.docs
+                .borrow_mut()
+                .insert(location.to_owned(), content.to_owned());
+            self
+        }
+    }
+
+    impl Fetcher<io::Cursor<Vec<u8>>> for MockFetcher {
+        fn fetch(&self, location: &str) -> io::Result<io::Cursor<Vec<u8>>> {
+            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<Authentication>,
+    }
+
+    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"
+        );
+    }
+}

usecases/service_loader/src/loaders/openapi/mod.rs

@@ -74,6 +74,19 @@ fn get_server(server: &serde_json::Value) -> error::Result<String> {
     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<R: io::Read>(
 
     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)