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!( diff --git a/usecases/service_loader/src/lib.rs b/usecases/service_loader/src/lib.rs index 5190544..30e129a 100644 --- a/usecases/service_loader/src/lib.rs +++ b/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>, + } + + 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" + ); + } +} 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" }), + ] + ); + } + } }