diff --git a/reflectapi-demo/src/tests/mod.rs b/reflectapi-demo/src/tests/mod.rs index c977656e..b54cd10b 100644 --- a/reflectapi-demo/src/tests/mod.rs +++ b/reflectapi-demo/src/tests/mod.rs @@ -38,6 +38,107 @@ fn write_openapi_spec() { insta::assert_snapshot!(s); } +#[test] +fn generated_openapi_spec_has_resolvable_refs() { + let (schema, _) = crate::builder().build().unwrap(); + + let s = reflectapi::codegen::openapi::generate( + &schema, + reflectapi::codegen::openapi::Config::default() + .exclude_tags(BTreeSet::from_iter(["internal".to_string()])), + ) + .unwrap(); + let spec: serde_json::Value = serde_json::from_str(&s).unwrap(); + + assert_no_embedded_json_schema_docs(&spec, "$".to_owned()); + assert_local_openapi_refs_resolve(&spec); +} + +fn assert_local_openapi_refs_resolve(spec: &serde_json::Value) { + let schemas = spec + .pointer("/components/schemas") + .and_then(serde_json::Value::as_object) + .expect("OpenAPI spec should have components.schemas"); + + let mut unresolved_refs = Vec::new(); + collect_unresolved_local_refs(spec, schemas, "$".to_owned(), &mut unresolved_refs); + + assert!( + unresolved_refs.is_empty(), + "OpenAPI spec contains unresolved local refs:\n{}", + unresolved_refs.join("\n") + ); +} + +fn collect_unresolved_local_refs( + value: &serde_json::Value, + schemas: &serde_json::Map, + path: String, + unresolved_refs: &mut Vec, +) { + match value { + serde_json::Value::Object(object) => { + if let Some(ref_path) = object.get("$ref").and_then(serde_json::Value::as_str) { + if let Some(component_name) = ref_path.strip_prefix("#/components/schemas/") { + if !schemas.contains_key(component_name) { + unresolved_refs.push(format!("{path}: {ref_path}")); + } + } else if ref_path.starts_with('#') { + unresolved_refs.push(format!("{path}: unsupported local ref {ref_path}")); + } + } + + for (key, child) in object { + collect_unresolved_local_refs( + child, + schemas, + format!("{path}.{}", key.replace('.', "\\.")), + unresolved_refs, + ); + } + } + serde_json::Value::Array(items) => { + for (index, child) in items.iter().enumerate() { + collect_unresolved_local_refs( + child, + schemas, + format!("{path}[{index}]"), + unresolved_refs, + ); + } + } + serde_json::Value::Null + | serde_json::Value::Bool(_) + | serde_json::Value::Number(_) + | serde_json::Value::String(_) => {} + } +} + +fn assert_no_embedded_json_schema_docs(value: &serde_json::Value, path: String) { + match value { + serde_json::Value::String(text) => { + assert!( + !text.contains("JSON schema"), + "OpenAPI spec contains embedded JSON schema docs at {path}" + ); + } + serde_json::Value::Object(object) => { + for (key, child) in object { + assert_no_embedded_json_schema_docs( + child, + format!("{path}.{}", key.replace('.', "\\.")), + ); + } + } + serde_json::Value::Array(items) => { + for (index, child) in items.iter().enumerate() { + assert_no_embedded_json_schema_docs(child, format!("{path}[{index}]")); + } + } + serde_json::Value::Null | serde_json::Value::Bool(_) | serde_json::Value::Number(_) => {} + } +} + #[test] fn write_rust_client() { let (schema, _) = crate::builder().build().unwrap(); diff --git a/reflectapi-demo/src/tests/serde.rs b/reflectapi-demo/src/tests/serde.rs index 7e7ba4dc..9de9b1c7 100644 --- a/reflectapi-demo/src/tests/serde.rs +++ b/reflectapi-demo/src/tests/serde.rs @@ -1077,6 +1077,20 @@ fn test_box_field_unwrapping() { assert_snapshot!(TreeNode); } +#[test] +fn test_box_root_openapi_unwrapping() { + #[derive( + serde::Serialize, serde::Deserialize, Debug, reflectapi::Input, reflectapi::Output, + )] + struct TreeNode { + label: String, + child: Option>, + } + + let schema = super::into_schema::>(); + insta::assert_json_snapshot!(reflectapi::codegen::openapi::Spec::from(&schema)); +} + #[test] fn test_nested_generic_containers() { #[derive( diff --git a/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__serde__box_root_openapi_unwrapping.snap b/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__serde__box_root_openapi_unwrapping.snap new file mode 100644 index 00000000..a93f1621 --- /dev/null +++ b/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__serde__box_root_openapi_unwrapping.snap @@ -0,0 +1,75 @@ +--- +source: reflectapi-demo/src/tests/serde.rs +assertion_line: 1091 +expression: "reflectapi::codegen::openapi::Spec::from(&schema)" +--- +{ + "openapi": "3.1.0", + "info": { + "title": "", + "description": "", + "version": "1.0.0" + }, + "paths": { + "/inout_test": { + "description": "", + "post": { + "operationId": "inout_test", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/reflectapi_demo.tests.serde.TreeNode" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "200 OK", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/reflectapi_demo.tests.serde.TreeNode" + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "reflectapi_demo.tests.serde.TreeNode": { + "type": "object", + "title": "reflectapi_demo.tests.serde.TreeNode", + "required": [ + "child", + "label" + ], + "properties": { + "child": { + "oneOf": [ + { + "description": "Null", + "type": "null" + }, + { + "$ref": "#/components/schemas/reflectapi_demo.tests.serde.TreeNode" + } + ] + }, + "label": { + "$ref": "#/components/schemas/std.string.String" + } + } + }, + "std.string.String": { + "description": "UTF-8 encoded string", + "type": "string" + } + } + } +} diff --git a/reflectapi/src/codegen/openapi.rs b/reflectapi/src/codegen/openapi.rs index 32d886c7..e5d708d9 100644 --- a/reflectapi/src/codegen/openapi.rs +++ b/reflectapi/src/codegen/openapi.rs @@ -441,7 +441,7 @@ impl Converter<'_> { tags: self.config.tags.clone(), info: Info { title: schema.name.clone(), - description: schema.description.clone(), + description: sanitize_description(&schema.description), version: "1.0.0".into(), }, paths: schema @@ -516,7 +516,7 @@ impl Converter<'_> { let operation = Operation { operation_id: f.name.clone(), tags: f.tags.clone(), - description: f.description.clone(), + description: sanitize_description(&f.description), deprecated: f.deprecated(), parameters: f .input_headers() @@ -535,7 +535,7 @@ impl Converter<'_> { }; PathItem { - description: f.description.clone(), + description: sanitize_description(&f.description), get: None, // If we do this the `operation_id` will not be unique // get: f.readonly.then(|| operation.clone()), @@ -597,7 +597,7 @@ impl Converter<'_> { field: &crate::Field, ) -> Property { Property { - description: field.description().to_owned(), + description: sanitize_description(field.description()), deprecated: field.deprecated(), schema: self.convert_type_ref(schema, kind, field.type_ref()), } @@ -609,6 +609,10 @@ impl Converter<'_> { kind: Kind, ty_ref: &crate::TypeReference, ) -> InlineOrRef { + if let Some(fallback) = primitive_fallback_type_ref(schema, kind, ty_ref) { + return self.convert_type_ref(schema, kind, &fallback); + } + let name = normalize(&ty_ref.name); let schema_ref = Ref::new(format!("#/components/schemas/{name}")); if self.components.schemas.contains_key(&name) { @@ -803,7 +807,7 @@ impl Converter<'_> { }; Schema::Flat(FlatSchema { - description: reflect_ty.description().to_owned(), + description: sanitize_description(reflect_ty.description()), ty, }) } @@ -871,7 +875,7 @@ impl Converter<'_> { let mut strukt = crate::Struct { name: variant.name().to_owned(), serde_name: variant.serde_name.to_owned(), - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), parameters: vec![], fields: variant.fields.clone(), transparent: false, @@ -902,7 +906,7 @@ impl Converter<'_> { properties: BTreeMap::from_iter([( variant.serde_name().to_owned(), Property { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), deprecated: false, schema: Inline(Schema::empty_object()), }, @@ -914,7 +918,7 @@ impl Converter<'_> { properties: BTreeMap::from_iter([( variant.serde_name().to_owned(), Property { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), deprecated: false, schema: Inline(Schema::empty_tuple()), }, @@ -922,7 +926,7 @@ impl Converter<'_> { }, reflectapi_schema::Fields::None => { return Inline(Schema::Const { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), value: variant.serde_name().to_owned(), }) } @@ -930,7 +934,7 @@ impl Converter<'_> { return Inline( FlatSchema { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), ty, } .into(), @@ -946,7 +950,7 @@ impl Converter<'_> { return Inline( FlatSchema { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), ty: Type::Object { title: fmt_type_ref(&type_ref), required: [variant.serde_name().to_owned()].into(), @@ -971,7 +975,7 @@ impl Converter<'_> { }; return Inline( FlatSchema { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), ty: Type::Object { title: fmt_type_ref(&type_ref), required: [tag.to_owned(), content.to_owned()].into(), @@ -1065,7 +1069,7 @@ impl Converter<'_> { return Inline( FlatSchema { - description: variant.description().to_owned(), + description: sanitize_description(variant.description()), ty, } .into(), @@ -1143,7 +1147,7 @@ impl Converter<'_> { required.insert(value.clone()); Ok(Inline(Schema::Flat(FlatSchema { - description: description.clone(), + description: sanitize_description(description), ty: Type::Object { title: String::new(), required, @@ -1160,7 +1164,7 @@ impl Converter<'_> { | Type::Tuple { .. } => Err(InvalidInternalTagError(schema.ty.clone())), Type::Map { .. } => todo!("map within newtype variant"), Type::Null => Ok(Inline(Schema::Flat(FlatSchema { - description: schema.description.to_owned(), + description: sanitize_description(&schema.description), ty: Type::Object { title: String::new(), required: [tag_name.to_owned()].into(), @@ -1179,7 +1183,7 @@ impl Converter<'_> { required, properties, } => Ok(Inline(Schema::Flat(FlatSchema { - description: schema.description.to_owned(), + description: sanitize_description(&schema.description), ty: Type::Object { title: title.to_owned(), required: required @@ -1256,7 +1260,7 @@ impl Converter<'_> { }; let s = FlatSchema { - description: strukt.description().to_owned(), + description: sanitize_description(strukt.description()), ty, } .into(); @@ -1281,6 +1285,112 @@ impl Converter<'_> { } } +fn primitive_fallback_type_ref( + schema: &crate::Schema, + kind: Kind, + ty_ref: &crate::TypeReference, +) -> Option { + let crate::Type::Primitive(primitive) = match kind { + Kind::Input => schema.input_types.get_type(&ty_ref.name), + Kind::Output => schema.output_types.get_type(&ty_ref.name), + }? + else { + return None; + }; + + let fallback = primitive.fallback()?.clone(); + // Only unwrap transparent pointer-shaped primitives whose fallback is just + // one of their generic parameters (Box, Rc, Arc, Cow, ...). + // Primitives with their own OpenAPI representation (usize, chrono::DateTime, + // HashSet -> Vec, ...) must keep their richer schema instead of being + // collapsed to the wire fallback. + if !primitive + .parameters + .iter() + .any(|param| param.name() == fallback.name) + { + return None; + } + + let subst = reflectapi_schema::mk_subst(&primitive.parameters, &ty_ref.arguments); + Some(fallback.subst(&subst)) +} + +fn sanitize_description(description: &str) -> String { + let Some(summary_start) = description.find("JSON schema") else { + return description.to_owned(); + }; + + let details_start = description[..summary_start] + .rfind("") else { + return description[..details_start].trim_end().to_owned(); + }; + let details_end = summary_start + details_end + "".len(); + + let mut sanitized = description[..details_start].trim_end().to_owned(); + let suffix = description[details_end..].trim_start(); + if !sanitized.is_empty() && !suffix.is_empty() { + sanitized.push_str("\n\n"); + } + sanitized.push_str(suffix); + + sanitize_description(&sanitized) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn json_schema_doc() -> &'static str { + r##"Public docs. + +
JSON schema + +```json +{ + "$ref": "#/components/schemas/InternalOnly" +} +``` +
+ +Trailing docs."## + } + + #[test] + fn strips_embedded_json_schema_blocks_from_descriptions() { + let mut schema = crate::Schema::new(); + schema.description = json_schema_doc().to_owned(); + + let mut strukt = crate::Struct::new("tier1::ContextWithDiagrams"); + strukt.description = json_schema_doc().to_owned(); + strukt.fields = reflectapi_schema::Fields::Named(vec![crate::Field { + description: json_schema_doc().to_owned(), + required: true, + ..crate::Field::new("name".to_owned(), "std::string::String".into()) + }]); + schema.input_types.insert_type(strukt.clone().into()); + schema.output_types.insert_type(strukt.into()); + schema.functions.push(crate::Function { + description: json_schema_doc().to_owned(), + input_type: Some("tier1::ContextWithDiagrams".into()), + output_type: crate::OutputType::Complete { + output_type: Some("tier1::ContextWithDiagrams".into()), + }, + ..crate::Function::new("test".to_owned()) + }); + + let spec_json = serde_json::to_string(&Spec::from(&schema)).unwrap(); + + assert!(!spec_json.contains("
")); + assert!(!spec_json.contains("InternalOnly")); + assert!(spec_json.contains("Public docs.")); + assert!(spec_json.contains("Trailing docs.")); + assert!(spec_json.contains("#/components/schemas/tier1.ContextWithDiagrams")); + } +} + fn normalize(name: impl AsRef) -> String { name.as_ref().replace("::", ".") }