From f32b354f9bd300e8a9a7ad9b6ea2316c2b17e8ed Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 05:43:43 +0900 Subject: [PATCH] fix(api): close OpenAPI schema fidelity gaps --- crates/api-macros-impl/src/lib.rs | 17 +--- crates/api-macros/src/openapi.rs | 74 +++++++++++++---- crates/api-macros/tests/openapi.rs | 125 ++++++++++++++++++++++++++++- 3 files changed, 184 insertions(+), 32 deletions(-) diff --git a/crates/api-macros-impl/src/lib.rs b/crates/api-macros-impl/src/lib.rs index fa6b49782..79b46eaa7 100644 --- a/crates/api-macros-impl/src/lib.rs +++ b/crates/api-macros-impl/src/lib.rs @@ -1301,9 +1301,8 @@ fn openapi_adapter_tokens( Location::Header => "header", Location::Body => unreachable!(), }; - let required = matches!(parameter.location, Location::Path) || !is_option_type(ty); Some(quote! { - operation.parameter::<#ty>(#name, #location, #required)?; + operation.parameter::<#ty>(#name, #location)?; }) }); let request = operation.request_body.as_ref().map(|ty| { @@ -1351,23 +1350,11 @@ fn openapi_adapter_tokens( > { let mut builder = #api_crate::openapi::OpenApiBuilder::new(info)?; #(#operations)* - ::core::result::Result::Ok(builder.finish()) + builder.finish() } } } -fn is_option_type(ty: &Type) -> bool { - let Type::Path(path) = ty else { - return false; - }; - path.qself.is_none() - && path - .path - .segments - .last() - .is_some_and(|segment| segment.ident == "Option") -} - fn expand_api(api: ApiDefinition) -> syn::Result { let api_crate = api_crate_path()?; let reqwest_adapter = reqwest_adapter_tokens(&api, &api_crate); diff --git a/crates/api-macros/src/openapi.rs b/crates/api-macros/src/openapi.rs index 0908a83f4..19ff1f5d0 100644 --- a/crates/api-macros/src/openapi.rs +++ b/crates/api-macros/src/openapi.rs @@ -26,6 +26,12 @@ const MAX_SAFE_INTEGER: i64 = 9_007_199_254_740_991; /// The default component name delegates to [`JsonSchema::schema_name`]; types /// may override it to provide a stable public wire name. pub trait OpenApiSchema: JsonSchema { + /// Whether a parameter of this type must be present on the wire. + /// + /// This semantic hook intentionally lives on the resolved type, so aliases + /// such as `type Header = Option` retain optionality. + const PARAMETER_REQUIRED: bool = true; + fn openapi_schema_name() -> Cow<'static, str> { Self::schema_name() } @@ -39,7 +45,9 @@ macro_rules! impl_openapi_schema { impl_openapi_schema!((), bool, String, char, i8, i16, i32, u8, u16, u32, f32, f64); -impl OpenApiSchema for Option {} +impl OpenApiSchema for Option { + const PARAMETER_REQUIRED: bool = false; +} impl OpenApiSchema for Vec {} impl OpenApiSchema for Box {} impl OpenApiSchema for std::sync::Arc {} @@ -184,7 +192,7 @@ impl OpenApiBuilder { }) } - pub fn finish(self) -> OpenApiDocument { + pub fn finish(self) -> Result { let paths = self .paths .into_iter() @@ -195,12 +203,14 @@ impl OpenApiBuilder { .collect::>(); let schemas = self.schemas.into_iter().collect::>(); - OpenApiDocument(canonicalize(json!({ + let document = canonicalize(json!({ "openapi": "3.1.0", "info": self.info, "paths": paths, "components": { "schemas": schemas }, - }))) + })); + validate_component_references(&document)?; + Ok(OpenApiDocument(document)) } fn schema_ref(&mut self) -> Result { @@ -268,7 +278,6 @@ impl OpenApiOperation<'_> { &mut self, name: &'static str, location: &'static str, - required: bool, ) -> Result<(), OpenApiError> { if !matches!(location, "path" | "query" | "header") { return Err(OpenApiError::InvalidContract(format!( @@ -276,12 +285,7 @@ impl OpenApiOperation<'_> { self.operation_id ))); } - if location == "path" && !required { - return Err(OpenApiError::InvalidContract(format!( - "path parameter `{name}` for `{}` must be required", - self.operation_id - ))); - } + let required = location == "path" || T::PARAMETER_REQUIRED; let schema = self.parent.schema_ref::()?; self.parameters.push(json!({ "name": name, @@ -396,6 +400,46 @@ fn validate_component_name(name: &str) -> Result<(), OpenApiError> { Ok(()) } +fn validate_component_references(document: &Value) -> Result<(), OpenApiError> { + let schemas = document["components"]["schemas"] + .as_object() + .ok_or_else(|| OpenApiError::InvalidContract("missing component schema map".to_owned()))?; + let mut references = Vec::new(); + collect_references(document, &mut references); + for reference in references { + let Some(name) = reference.strip_prefix("#/components/schemas/") else { + return Err(OpenApiError::InvalidContract(format!( + "unsupported non-component schema reference `{reference}`" + ))); + }; + if !schemas.contains_key(name) { + return Err(OpenApiError::InvalidContract(format!( + "unresolved OpenAPI component reference `{reference}`" + ))); + } + } + Ok(()) +} + +fn collect_references<'a>(value: &'a Value, references: &mut Vec<&'a str>) { + match value { + Value::Object(object) => { + if let Some(reference) = object.get("$ref").and_then(Value::as_str) { + references.push(reference); + } + for value in object.values() { + collect_references(value, references); + } + } + Value::Array(values) => { + for value in values { + collect_references(value, references); + } + } + _ => {} + } +} + fn rewrite_definition_refs(value: &mut Value) { match value { Value::Object(object) => { @@ -421,11 +465,11 @@ fn validate_schema(value: &Value, component: &str) -> Result<(), OpenApiError> { object.get("format").and_then(Value::as_str), Some("int8" | "uint8" | "int16" | "uint16" | "int32" | "uint32") ); - let minimum = object.get("minimum").and_then(Value::as_i64); - let maximum = object.get("maximum").and_then(Value::as_u64); + let minimum = object.get("minimum").and_then(Value::as_f64); + let maximum = object.get("maximum").and_then(Value::as_f64); if !safe_format - && (minimum.is_none_or(|value| value < -MAX_SAFE_INTEGER) - || maximum.is_none_or(|value| value > MAX_SAFE_INTEGER as u64)) + && (minimum.is_none_or(|value| value < -(MAX_SAFE_INTEGER as f64)) + || maximum.is_none_or(|value| value > MAX_SAFE_INTEGER as f64)) { return Err(OpenApiError::InvalidContract(format!( "integer schema in `{component}` exceeds the JSON safe-integer range; expose a bounded wire integer" diff --git a/crates/api-macros/tests/openapi.rs b/crates/api-macros/tests/openapi.rs index 874ee000f..0e659fbc3 100644 --- a/crates/api-macros/tests/openapi.rs +++ b/crates/api-macros/tests/openapi.rs @@ -6,7 +6,7 @@ use api_macros::{ openapi::{OpenApiInfo, OpenApiSchema}, }; use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_json::{Value, json}; fn nullable_string_schema(generator: &mut schemars::SchemaGenerator) -> schemars::Schema { @@ -20,10 +20,36 @@ pub struct GeoPoint { } impl OpenApiSchema for GeoPoint {} +/// A custom Serde wire representation admitted only after an explicit, +/// matching schema hook and `OpenApiSchema` attestation. +#[derive(Debug, JsonSchema)] +#[schemars(transparent)] +pub struct WireSlug(String); + +impl Serialize for WireSlug { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.0) + } +} + +impl<'de> Deserialize<'de> for WireSlug { + fn deserialize>(deserializer: D) -> Result { + String::deserialize(deserializer).map(Self) + } +} +impl OpenApiSchema for WireSlug {} + #[derive(Debug, Deserialize, JsonSchema, Serialize)] pub struct CreateWidget { + #[schemars(length(min = 1, max = 64))] name: String, + #[schemars(range(min = 1, max = 1_000))] count: u32, + #[schemars(range(min = -100, max = 100))] + bounded_balance: i64, + #[schemars(length(min = 1, max = 8))] + tags: Vec, + slug: WireSlug, /// Missing and explicit null are separate wire states. #[schemars(default, schema_with = "nullable_string_schema")] optional_label: Option, @@ -47,11 +73,14 @@ pub struct Widget { impl OpenApiSchema for Widget {} #[derive(Debug, Deserialize, JsonSchema, Serialize)] +#[serde(tag = "code", content = "details")] pub enum PublicError { Invalid { message: String }, } impl OpenApiSchema for PublicError {} +type OptionalRequestId = Option; + #[api(openapi)] pub trait FixtureApi { #[post( @@ -64,7 +93,7 @@ pub trait FixtureApi { &self, #[path] widget_id: u32, #[query] lookup: Lookup, - #[header("x-request-id")] request_id: Option, + #[header("x-request-id")] request_id: OptionalRequestId, #[body] request: CreateWidget, ) -> Result; @@ -105,6 +134,41 @@ fn output_is_deterministic_and_accepted_by_an_independent_parser() { ); assert!(!first.contains("generatedAt")); assert!(!first.contains("#/$defs/")); + + let schemas = value["components"]["schemas"] + .as_object() + .expect("component map"); + let mut references = Vec::new(); + collect_schema_references(&value, &mut references); + assert!(!references.is_empty()); + for reference in references { + let name = reference + .strip_prefix("#/components/schemas/") + .expect("only local component references are emitted"); + assert!( + schemas.contains_key(name), + "unresolved reference {reference}" + ); + } +} + +fn collect_schema_references<'a>(value: &'a Value, references: &mut Vec<&'a str>) { + match value { + Value::Object(object) => { + if let Some(reference) = object.get("$ref").and_then(Value::as_str) { + references.push(reference); + } + for value in object.values() { + collect_schema_references(value, references); + } + } + Value::Array(values) => { + for value in values { + collect_schema_references(value, references); + } + } + _ => {} + } } #[test] @@ -156,6 +220,7 @@ fn operations_and_components_preserve_the_wire_contract() { "Lookup", "PublicError", "Widget", + "WireSlug", "uint32", "Nullable_string", ] { @@ -169,6 +234,33 @@ fn operations_and_components_preserve_the_wire_contract() { schemas["CreateWidget"]["properties"]["location"]["$ref"], "#/components/schemas/GeoPoint" ); + assert_eq!( + schemas["CreateWidget"]["properties"]["slug"]["$ref"], + "#/components/schemas/WireSlug" + ); + assert_eq!( + schemas["CreateWidget"]["properties"]["name"]["minLength"], + 1 + ); + assert_eq!( + schemas["CreateWidget"]["properties"]["name"]["maxLength"], + 64 + ); + assert_eq!( + schemas["CreateWidget"]["properties"]["count"]["maximum"], + 1_000 + ); + assert_eq!( + schemas["CreateWidget"]["properties"]["bounded_balance"]["minimum"], + -100 + ); + assert_eq!( + schemas["CreateWidget"]["properties"]["bounded_balance"]["maximum"], + 100 + ); + assert_eq!(schemas["CreateWidget"]["properties"]["tags"]["minItems"], 1); + assert_eq!(schemas["CreateWidget"]["properties"]["tags"]["maxItems"], 8); + assert!(schemas["PublicError"]["oneOf"].is_array()); let required = schemas["CreateWidget"]["required"] .as_array() @@ -220,6 +312,20 @@ pub enum AmbiguousWireEnum { } impl OpenApiSchema for AmbiguousWireEnum {} +#[derive(Debug)] +pub struct DanglingReference; + +impl JsonSchema for DanglingReference { + fn schema_name() -> std::borrow::Cow<'static, str> { + "DanglingReference".into() + } + + fn json_schema(_generator: &mut schemars::SchemaGenerator) -> schemars::Schema { + schemars::json_schema!({ "$ref": "#/$defs/Missing" }) + } +} +impl OpenApiSchema for DanglingReference {} + #[test] fn schema_name_collisions_and_ambiguous_wire_shapes_fail_closed() { use api_macros::openapi::{OpenApiBuilder, OpenApiError}; @@ -254,6 +360,21 @@ fn schema_name_collisions_and_ambiguous_wire_shapes_fail_closed() { .response::(200, "application/json", "ok") .expect_err("untagged/ambiguous unions must fail closed"); assert!(error.to_string().contains("untagged")); + + let mut dangling = OpenApiBuilder::new(info).expect("builder"); + { + let mut operation = dangling + .operation("GET", "/dangling", "dangling") + .expect("operation"); + operation + .response::(200, "application/json", "ok") + .expect("schema registration succeeds before graph validation"); + operation.finish().expect("operation"); + } + let error = dangling + .finish() + .expect_err("dangling component references must fail closed"); + assert!(error.to_string().contains("unresolved")); } #[test]