fix(adapters): resolve parameter/requestBody refs + reject import collisions (OAI-04, OAI-05)
- index components/parameters + requestBodies in OpenAPISpec; resolve bare $ref parameter and requestBody entries through the cycle-guarded resolver; unresolvable refs abort spec parse loudly - reject path placeholders with no matching input-schema property at registration (no more silently percent-encoded literal placeholders) - reject duplicate operationIds and path+method routes in one import batch instead of silent last-write-wins registration - also reject a parameter named 'body' shadowed by requestBody (OAI-07 adjacency, same code path) Verified: cargo test (243), --all-features (322), clippy -D warnings (default + all-features), fmt --check, doc --no-deps
This commit is contained in:
@@ -11,6 +11,7 @@
|
||||
//!
|
||||
//! [ADR-051]: https://docs.rs/alkhttp (docs/architecture/decisions)
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use alkcall::client::{AdapterError, OperationAdapter};
|
||||
@@ -29,6 +30,63 @@ use super::forward::{forward, forward_stream, HttpServiceConfig};
|
||||
use super::openapi_spec::{OpenAPISpec, Operation};
|
||||
use crate::client::SharedHttpClient;
|
||||
|
||||
const GATEWAY_BODY_KEY: &str = "body";
|
||||
|
||||
fn unbound_placeholders(path_template: &str, input_schema: &Value) -> Vec<String> {
|
||||
let mut unbound = Vec::new();
|
||||
let mut rest = path_template;
|
||||
while let Some(start) = rest.find('{') {
|
||||
let Some(end_rel) = rest[start..].find('}') else {
|
||||
break;
|
||||
};
|
||||
let name = &rest[start + 1..start + end_rel];
|
||||
if !name.is_empty() {
|
||||
let properties = input_schema
|
||||
.get("properties")
|
||||
.and_then(|p| p.as_object())
|
||||
.map(|p| p.keys().cloned().collect::<Vec<_>>())
|
||||
.unwrap_or_default();
|
||||
if !properties.iter().any(|k| k == name) {
|
||||
unbound.push(name.to_string());
|
||||
}
|
||||
}
|
||||
rest = &rest[start + end_rel + 1..];
|
||||
}
|
||||
unbound
|
||||
}
|
||||
|
||||
fn collision_message(batch: &str, kind: &str, first_path: &str) -> AdapterError {
|
||||
AdapterError::SchemaParse {
|
||||
message: format!(
|
||||
"duplicate {kind} in import batch: `{batch}` (first registered from \
|
||||
{first_path}; the registry would silently replace the earlier \
|
||||
registration — disambiguate the operationIds or paths)"
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
fn reject_collisions(
|
||||
op_ids: Vec<String>,
|
||||
paths: Vec<String>,
|
||||
routes: Vec<(String, String)>,
|
||||
) -> Result<(), AdapterError> {
|
||||
assert_eq!(op_ids.len(), routes.len());
|
||||
assert_eq!(op_ids.len(), paths.len());
|
||||
let mut seen_names: HashMap<&str, &str> = HashMap::new();
|
||||
let mut seen_routes: HashMap<&(String, String), &str> = HashMap::new();
|
||||
for ((op_id, path), route) in op_ids.iter().zip(paths.iter()).zip(routes.iter()) {
|
||||
if let Some(first_path) = seen_names.get(op_id.as_str()) {
|
||||
return Err(collision_message(op_id, "operationId", first_path));
|
||||
}
|
||||
if let Some(first_path) = seen_routes.get(route) {
|
||||
return Err(collision_message(op_id, "path+method", first_path));
|
||||
}
|
||||
seen_names.insert(op_id, path);
|
||||
seen_routes.insert(route, path);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub struct FromOpenAPI {
|
||||
spec: OpenAPISpec,
|
||||
config: HttpServiceConfig,
|
||||
@@ -87,6 +145,15 @@ impl FromOpenAPI {
|
||||
Some(s) => self.spec.resolve_refs_recursive(s)?,
|
||||
None => serde_json::json!({"type": "string"}),
|
||||
};
|
||||
if param.name == GATEWAY_BODY_KEY && op.request_body.is_some() {
|
||||
return Err(AdapterError::SchemaParse {
|
||||
message: format!(
|
||||
"parameter named `{GATEWAY_BODY_KEY}` collides with the gateway's \
|
||||
requestBody placeholder key; the parameter would be diverted into \
|
||||
the request body at call time — rename the parameter"
|
||||
),
|
||||
});
|
||||
}
|
||||
properties.insert(param.name.clone(), schema);
|
||||
if param.required {
|
||||
required.push(param.name.clone());
|
||||
@@ -167,13 +234,12 @@ impl FromOpenAPI {
|
||||
qualified_name,
|
||||
op_type,
|
||||
Visibility::Internal,
|
||||
input_schema,
|
||||
input_schema.clone(),
|
||||
output_schema,
|
||||
error_schemas,
|
||||
AccessControl::default(),
|
||||
None,
|
||||
);
|
||||
|
||||
let path_template = path.to_string();
|
||||
let method_upper = method.to_ascii_uppercase();
|
||||
let auth_scheme = self.config.auth.clone();
|
||||
@@ -186,6 +252,20 @@ impl FromOpenAPI {
|
||||
.iter()
|
||||
.map(|e| (e.http_status.unwrap_or(0), e.code.clone()))
|
||||
.collect();
|
||||
let unbound = unbound_placeholders(&path_template, &input_schema);
|
||||
if !unbound.is_empty() {
|
||||
return Err(AdapterError::SchemaParse {
|
||||
message: format!(
|
||||
"path {method} {path_template} declares placeholder(s) {} with no \
|
||||
matching parameter in the operation's resolved input schema (an \
|
||||
unresolved parameter$requestBodies $ref, or a parameter missing \
|
||||
`name`/`in`); the placeholder would otherwise render as a literal \
|
||||
`{}` path segment",
|
||||
unbound.join(", "),
|
||||
unbound[0]
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
let handler = if op_type == OperationType::Sub {
|
||||
let stream_handler =
|
||||
@@ -259,12 +339,24 @@ impl FromOpenAPI {
|
||||
impl OperationAdapter for FromOpenAPI {
|
||||
async fn import(&self) -> Result<Vec<HandlerRegistration>, AdapterError> {
|
||||
let mut bundles = Vec::new();
|
||||
let mut op_ids = Vec::new();
|
||||
let mut paths = Vec::new();
|
||||
let mut routes = Vec::new();
|
||||
for (path, item) in &self.spec.paths {
|
||||
for (method, op) in &item.operations {
|
||||
let registration = self.build_registration(method, path, op)?;
|
||||
let name = registration.spec.name.clone();
|
||||
let qualified_op_id = name
|
||||
.rsplit_once('/')
|
||||
.map(|(_, id)| id.to_string())
|
||||
.unwrap_or_else(|| name.clone());
|
||||
op_ids.push(qualified_op_id);
|
||||
paths.push(path.clone());
|
||||
routes.push((path.clone(), method.to_ascii_uppercase()));
|
||||
bundles.push(registration);
|
||||
}
|
||||
}
|
||||
reject_collisions(op_ids, paths, routes)?;
|
||||
Ok(bundles)
|
||||
}
|
||||
}
|
||||
@@ -395,7 +487,10 @@ mod tests {
|
||||
let doc = r#"{
|
||||
"openapi": "3.0.0",
|
||||
"info": { "title": "T", "version": "1" },
|
||||
"paths": { "/users/{id}/posts": { "get": { "responses": { "200": { "content": { "application/json": { "schema": {} } } } } } } }
|
||||
"paths": { "/users/{id}/posts": { "get": {
|
||||
"parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}}],
|
||||
"responses": { "200": { "content": { "application/json": { "schema": {} } } } }
|
||||
} } }
|
||||
}"#;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let adapter = adapter(spec, config("svc", "https://api.example.com", None));
|
||||
@@ -403,6 +498,217 @@ mod tests {
|
||||
assert_eq!(bundles[0].spec.name, "svc/get_users_posts");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn duplicate_operation_ids_rejected_at_import() {
|
||||
let doc = r#"{
|
||||
"openapi": "3.0.0",
|
||||
"info": {"title": "T", "version": "1"},
|
||||
"paths": {
|
||||
"/a/x": {"get": {"operationId": "dup", "responses": {"200": {"content": {"application/json": {"schema": {}}}}}}},
|
||||
"/b/y": {"get": {"operationId": "dup", "responses": {"200": {"content": {"application/json": {"schema": {}}}}}}}
|
||||
}
|
||||
}"#;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let result = adapter(spec, config("svc", "https://x", None))
|
||||
.import()
|
||||
.await;
|
||||
match result {
|
||||
Err(AdapterError::SchemaParse { message }) => {
|
||||
assert!(
|
||||
message.contains("duplicate operationId"),
|
||||
"message was: {message}"
|
||||
);
|
||||
assert!(message.contains("dup"), "message was: {message}");
|
||||
}
|
||||
Ok(bundles) => panic!(
|
||||
"expected duplicate-operationId rejection, got {} bundles",
|
||||
bundles.len()
|
||||
),
|
||||
Err(e) => panic!("expected SchemaParse, got {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn generated_id_collision_between_template_and_literal_path_rejected() {
|
||||
let doc = r#"{
|
||||
"openapi": "3.0.0",
|
||||
"info": {"title": "T", "version": "1"},
|
||||
"paths": {
|
||||
"/x/{id}/y": {"get": {"parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}}], "responses": {"200": {"content": {"application/json": {"schema": {}}}}}}},
|
||||
"/x/y": {"get": {"responses": {"200": {"content": {"application/json": {"schema": {}}}}}}}
|
||||
}
|
||||
}"#;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let result = adapter(spec, config("svc", "https://x", None))
|
||||
.import()
|
||||
.await;
|
||||
match result {
|
||||
Err(AdapterError::SchemaParse { message }) => {
|
||||
assert!(
|
||||
message.contains("duplicate operationId"),
|
||||
"message was: {message}"
|
||||
);
|
||||
assert!(message.contains("get_x_y"), "message was: {message}");
|
||||
}
|
||||
Ok(bundles) => panic!(
|
||||
"expected generated-id collision rejection, got {} bundles",
|
||||
bundles.len()
|
||||
),
|
||||
Err(e) => panic!("expected SchemaParse, got {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn route_collision_check_rejects_duplicate_path_method() {
|
||||
let result = reject_collisions(
|
||||
vec!["a".to_string(), "b".to_string()],
|
||||
vec!["/shared".to_string(), "/other".to_string()],
|
||||
vec![
|
||||
("/shared".to_string(), "GET".to_string()),
|
||||
("/shared".to_string(), "GET".to_string()),
|
||||
],
|
||||
);
|
||||
match result {
|
||||
Err(AdapterError::SchemaParse { message }) => {
|
||||
assert!(
|
||||
message.contains("duplicate path+method"),
|
||||
"message was: {message}"
|
||||
);
|
||||
}
|
||||
Ok(()) => panic!("expected path+method collision rejection"),
|
||||
other => panic!("expected SchemaParse, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unresolved_path_placeholder_fails_import_loudly() {
|
||||
let doc = r#"{
|
||||
"openapi": "3.0.0",
|
||||
"info": { "title": "T", "version": "1" },
|
||||
"paths": { "/users/{id}/posts": { "get": { "responses": { "200": { "content": { "application/json": { "schema": {} } } } } } } }
|
||||
}"#;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let adapter = adapter(spec, config("svc", "https://api.example.com", None));
|
||||
match adapter.import().await {
|
||||
Err(AdapterError::SchemaParse { message }) => {
|
||||
assert!(message.contains("placeholder"), "message was: {message}");
|
||||
assert!(message.contains("id"), "message was: {message}");
|
||||
}
|
||||
Ok(bundles) => panic!(
|
||||
"expected unresolved-placeholder import error, got {} bundles",
|
||||
bundles.len()
|
||||
),
|
||||
Err(e) => panic!("expected SchemaParse, got {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refd_parameters_and_request_body_resolve_into_op_schema() {
|
||||
let doc = r##"{
|
||||
"openapi": "3.0.0",
|
||||
"info": {"title": "T", "version": "1"},
|
||||
"components": {
|
||||
"schemas": {
|
||||
"Widget": {"type": "object", "properties": {"name": {"type": "string"}}}
|
||||
},
|
||||
"parameters": {
|
||||
"Id": {
|
||||
"name": "id",
|
||||
"in": "path",
|
||||
"required": true,
|
||||
"schema": {"type": "string"}
|
||||
}
|
||||
},
|
||||
"requestBodies": {
|
||||
"WidgetInput": {
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {"$ref": "#/components/schemas/Widget"}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"paths": {
|
||||
"/widgets/{id}": {"put": {
|
||||
"operationId": "replaceWidget",
|
||||
"parameters": [{"$ref": "#/components/parameters/Id"}],
|
||||
"requestBody": {"$ref": "#/components/requestBodies/WidgetInput"},
|
||||
"responses": {"200": {"content": {"application/json": {"schema": {}}}}}
|
||||
}}
|
||||
}
|
||||
}"##;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let bundles = adapter(spec, config("svc", "https://x", None))
|
||||
.import()
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(bundles.len(), 1);
|
||||
let props = bundles[0]
|
||||
.spec
|
||||
.input_schema
|
||||
.get("properties")
|
||||
.unwrap()
|
||||
.as_object()
|
||||
.unwrap();
|
||||
assert!(props.contains_key("id"), "ref'd path param present");
|
||||
let body = props.get("body").unwrap();
|
||||
assert_eq!(body["type"], "object");
|
||||
assert!(
|
||||
body.get("properties")
|
||||
.and_then(|p| p.as_object())
|
||||
.unwrap()
|
||||
.contains_key("name"),
|
||||
"recursive $ref (requestBody -> schemas/Widget) fully expanded, not empty"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn self_referential_schema_behind_refd_parameter_fails_cleanly_not_abort() {
|
||||
let doc = r##"{
|
||||
"openapi": "3.0.0",
|
||||
"info": {"title": "T", "version": "1"},
|
||||
"components": {
|
||||
"schemas": {
|
||||
"Node": {"type": "object", "properties": {
|
||||
"next": {"$ref": "#/components/schemas/Node"}
|
||||
}}
|
||||
},
|
||||
"requestBodies": {
|
||||
"NodeInput": {
|
||||
"content": {
|
||||
"application/json": {"schema": {"$ref": "#/components/requestBodies/NodeInput"}}
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"paths": {
|
||||
"/nodes": {"post": {
|
||||
"operationId": "createNode",
|
||||
"requestBody": {"$ref": "#/components/requestBodies/NodeInput"},
|
||||
"responses": {"201": {"content": {"application/json": {"schema": {}}}}}
|
||||
}}
|
||||
}
|
||||
}"##;
|
||||
let spec = OpenAPISpec::from_json(doc).unwrap();
|
||||
let result = adapter(spec, config("svc", "https://x", None))
|
||||
.import()
|
||||
.await;
|
||||
match result {
|
||||
Err(AdapterError::SchemaParse { message }) => {
|
||||
assert!(
|
||||
message.contains("circular $ref"),
|
||||
"clean resolver error, message was: {message}"
|
||||
);
|
||||
}
|
||||
Ok(bundles) => panic!(
|
||||
"expected clean circular-$ref error, got {} bundles",
|
||||
bundles.len()
|
||||
),
|
||||
Err(e) => panic!("expected SchemaParse, got {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn op_type_detection() {
|
||||
let get_doc = r#"{"openapi":"3.0.0","info":{"title":"T","version":"1"},"paths":{"/g":{"get":{"operationId":"g","responses":{"200":{"content":{"application/json":{"schema":{}}}}}}}}}"#;
|
||||
|
||||
Reference in New Issue
Block a user