From 35796f10d4b1224ceb14acad4b72ff2f67a1fc10 Mon Sep 17 00:00:00 2001 From: Kaspar Lyngsie Date: Sun, 23 Aug 2026 16:08:52 +0200 Subject: [PATCH 1/4] feat: implement readonly and writeonly --- crates/oapi-codegen/src/emit/mod.rs | 9 +- crates/oapi-codegen/src/emit/models.rs | 2 + crates/oapi-codegen/src/emit/usage.rs | 2 + crates/oapi-codegen/src/ir.rs | 48 ++ crates/oapi-codegen/src/lib.rs | 6 + crates/oapi-codegen/src/loader.rs | 148 ++++++ crates/oapi-codegen/src/lower/direction.rs | 499 ++++++++++++++++++ crates/oapi-codegen/src/lower/mod.rs | 2 + crates/oapi-codegen/src/lower/paths.rs | 25 +- crates/oapi-codegen/src/lower/recurse.rs | 2 + crates/oapi-codegen/src/lower/schema.rs | 31 ++ crates/oapi-codegen/tests/coverage.rs | 57 +- .../fixtures/combined_read_write_only.yaml | 89 ++++ .../tests/fixtures/read_write_only.yaml | 51 ++ .../fixtures/read_write_only_conflict.yaml | 16 + .../fixtures/schemas/compose_marked.yaml | 23 + ...ver_unsupported_xfile_direction_split.yaml | 18 + crates/oapi-codegen/tests/generated.rs | 70 +++ .../generated/combined_read_write_only.rs | 358 +++++++++++++ .../tests/generated/read_write_only.rs | 71 +++ .../tests/generated/server_auth.rs | 6 +- docs/design.md | 43 +- 22 files changed, 1561 insertions(+), 15 deletions(-) create mode 100644 crates/oapi-codegen/src/lower/direction.rs create mode 100644 crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml create mode 100644 crates/oapi-codegen/tests/fixtures/read_write_only.yaml create mode 100644 crates/oapi-codegen/tests/fixtures/read_write_only_conflict.yaml create mode 100644 crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml create mode 100644 crates/oapi-codegen/tests/fixtures/server_unsupported_xfile_direction_split.yaml create mode 100644 crates/oapi-codegen/tests/generated/combined_read_write_only.rs create mode 100644 crates/oapi-codegen/tests/generated/read_write_only.rs diff --git a/crates/oapi-codegen/src/emit/mod.rs b/crates/oapi-codegen/src/emit/mod.rs index a2422d1..b63f5de 100644 --- a/crates/oapi-codegen/src/emit/mod.rs +++ b/crates/oapi-codegen/src/emit/mod.rs @@ -300,13 +300,12 @@ fn render_body(items: &[TokenStream]) -> Result { } /// Render a doc attribute, or nothing when there is no documentation. +/// +/// The text is split on its line breaks, so a multi-line `description` prints as +/// a run of `///` lines rather than one `/** */` block. pub(crate) fn doc_attr(doc: &Option) -> TokenStream { let tokens = match doc { - Some(text) => { - // Leading space matches the `/// text` desugaring rustfmt produces. - let spaced = format!(" {text}"); - quote! { #[doc = #spaced] } - } + Some(text) => doc_lines(std::slice::from_ref(text)), None => quote! {}, }; return tokens; diff --git a/crates/oapi-codegen/src/emit/models.rs b/crates/oapi-codegen/src/emit/models.rs index 7868ad8..3c17576 100644 --- a/crates/oapi-codegen/src/emit/models.rs +++ b/crates/oapi-codegen/src/emit/models.rs @@ -516,6 +516,7 @@ fn emit_alias(alias: &Alias) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::ir::Access; use crate::naming::Case; use crate::naming::to_ident; @@ -536,6 +537,7 @@ mod tests { serde_skip: false, default: Some(DefaultValue::Int(10)), constraints: None, + access: Access::ReadWrite, }], additional_properties: None, deny_unknown_fields: false, diff --git a/crates/oapi-codegen/src/emit/usage.rs b/crates/oapi-codegen/src/emit/usage.rs index 9b04593..4092232 100644 --- a/crates/oapi-codegen/src/emit/usage.rs +++ b/crates/oapi-codegen/src/emit/usage.rs @@ -427,6 +427,7 @@ fn struct_field_names(strukt: &Struct, out: &mut Vec) { #[cfg(test)] mod tests { use super::*; + use crate::ir::Access; use crate::ir::Alias; use crate::ir::Body; use crate::ir::Field; @@ -457,6 +458,7 @@ mod tests { serde_skip: false, default: None, constraints: None, + access: Access::ReadWrite, }; } diff --git a/crates/oapi-codegen/src/ir.rs b/crates/oapi-codegen/src/ir.rs index bc1c396..ab7941f 100644 --- a/crates/oapi-codegen/src/ir.rs +++ b/crates/oapi-codegen/src/ir.rs @@ -100,6 +100,54 @@ pub struct Field { /// The generator checks these on the way in, so the check runs only where /// the code deserializes. A response the server writes is not checked. pub constraints: Option, + /// Which direction of an exchange carries the property, from `readOnly` and + /// `writeOnly`. + pub access: Access, +} + +/// One direction of an exchange. +/// +/// A request travels from the client to the server. A response travels back. +/// Which serde trait that needs depends on the side, so the direction is kept +/// separate from the trait: a server deserializes a request and serializes a +/// response, and a client does the opposite. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[non_exhaustive] +pub enum Direction { + /// The payload an operation reads: parameters and the request body. + Request, + /// The payload an operation writes: the response body and its headers. + Response, +} + +/// Which direction of an exchange carries a property. +/// +/// OpenAPI marks a property `readOnly` when a response may carry it and a +/// request must not, and `writeOnly` for the opposite. The mark names a +/// direction and not a value, so one struct cannot state both. [`Access`] +/// records the mark, and [`crate::lower::direction`] builds the two shapes a +/// marked model needs. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +#[non_exhaustive] +pub enum Access { + /// Both directions carry the property. + #[default] + ReadWrite, + /// `readOnly: true`: a response carries the property, a request must not. + ReadOnly, + /// `writeOnly: true`: a request carries the property, a response must not. + WriteOnly, +} + +impl Access { + /// Whether `direction` carries a property with this access. + pub fn carried_by(self, direction: Direction) -> bool { + return match (self, direction) { + (Access::ReadWrite, _) => true, + (Access::ReadOnly, Direction::Response) | (Access::WriteOnly, Direction::Request) => true, + (Access::ReadOnly, Direction::Request) | (Access::WriteOnly, Direction::Response) => false, + }; + } } /// A numeric bound, in the form the document writes it. diff --git a/crates/oapi-codegen/src/lib.rs b/crates/oapi-codegen/src/lib.rs index 5bbff09..cd59650 100644 --- a/crates/oapi-codegen/src/lib.rs +++ b/crates/oapi-codegen/src/lib.rs @@ -101,6 +101,10 @@ fn lower_spec(spec_path: &Path, config: &Config) -> Result { .unwrap_or(crate::config::DEFAULT_RESPONSE_SUFFIX); let mut service = lower::generate_service(&spec, &config.import_mapping, response_type_suffix)?; lower::rewrite_service(&mut service, names.renames()); + // Before pruning, so a shape no operation reaches is dropped with the + // unused models, and before the name checks, so a shape's name is + // checked like any other. + lower::split_by_direction(&mut module, Some(&mut service)); if !config.output_options.skip_prune { lower::prune_unused_models(&mut module, &service); } @@ -128,6 +132,7 @@ fn lower_spec(spec_path: &Path, config: &Config) -> Result { } // Models-only generation prunes nothing, so the module holds every schema and // every collision reports. + lower::split_by_direction(&mut module, None); names.check_emitted(&module)?; lower::check_duplicate_models(&module)?; lower::check_prelude_shadowing(&module, emit::Targets::default())?; @@ -216,6 +221,7 @@ pub fn generate_models_string(spec_path: &Path) -> Result { let spec = Spec::load(spec_path)?; let names = lower::type_renames(&spec, None)?; let mut module = lower::generate_models(&spec, &names)?; + lower::split_by_direction(&mut module, None); // Every schema becomes an item here, so every collision reaches the file. names.check_emitted(&module)?; lower::check_duplicate_models(&module)?; diff --git a/crates/oapi-codegen/src/loader.rs b/crates/oapi-codegen/src/loader.rs index 352b68a..7caf152 100644 --- a/crates/oapi-codegen/src/loader.rs +++ b/crates/oapi-codegen/src/loader.rs @@ -16,6 +16,8 @@ use openapiv3::Schema; use crate::error::Error; use crate::error::Result; +use crate::lower::direction::REQUEST_SUFFIX; +use crate::lower::direction::RESPONSE_SUFFIX; /// Maximum `$ref` chain length before bailing out (cycle guard). const MAX_REF_DEPTH: usize = 32; @@ -441,11 +443,26 @@ impl Spec { /// `type-name-suffix` it takes from its own config. This run cannot see that /// config, so it emits the plain name and reaches whichever of the two /// schemas kept it. That builds, and it carries the wrong type. + /// + /// A schema the direction pass splits is an error for the same reason. The + /// other run emits two names there and this one cannot tell which of the two + /// an `import-mapping` reference means. pub fn external_schema_name(&self, file: &str, name: &str, reference: &str) -> Result { let entry = self.component_schema(Some(file), reference, name)?; let chosen = external_name_of(&entry, name)?; let doc = self.document_for(file)?; let schemas = doc.components.as_ref().map(|components| return &components.schemas); + if direction_split_schemas(&doc).contains(name) { + return Err(Error::UnsupportedRef { + reference: reference.to_owned(), + reason: format!( + "`{name}` in `{file}` marks a property `readOnly` or `writeOnly`, so the run that \ + generates that file emits it as `{name}{REQUEST_SUFFIX}` and `{name}{RESPONSE_SUFFIX}`. \ + This run cannot tell which of the two an `import-mapping` reference means. Declare \ + the schema in this document instead, or drop the mark" + ), + }); + } for (other, other_entry) in schemas.into_iter().flatten() { if other == name { continue; @@ -466,6 +483,137 @@ impl Spec { } } +/// The schemas in `doc` that the direction pass splits into a request shape and +/// a response shape. +/// +/// A schema qualifies when it marks anything inside it `readOnly` or +/// `writeOnly`, or when it reaches such a schema through a same-document `$ref`. +/// The reference walk runs from a marked schema up to the schemas naming it, +/// because a holder's field type is what changes per direction. +/// +/// This reads the document alone. Both runs that compose a crate must reach the +/// same verdict for the same file, and only one of the two holds the operations. +fn direction_split_schemas(doc: &OpenAPI) -> std::collections::BTreeSet { + let mut marked = std::collections::BTreeSet::new(); + let Some(components) = doc.components.as_ref() else { + return marked; + }; + let mut referrers: HashMap> = HashMap::new(); + for (name, entry) in &components.schemas { + let ReferenceOr::Item(schema) = entry else { + continue; + }; + if schema_marks_a_direction(schema) { + marked.insert(name.clone()); + } + let mut targets = Vec::new(); + schema_local_refs(schema, &mut targets); + for target in targets { + referrers.entry(target).or_default().push(name.clone()); + } + } + + let mut stack: Vec = marked.iter().cloned().collect(); + while let Some(name) = stack.pop() { + let Some(parents) = referrers.get(&name) else { + continue; + }; + for parent in parents { + if marked.insert(parent.clone()) { + stack.push(parent.clone()); + } + } + } + return marked; +} + +/// Whether a schema, or any schema written inside it, sets `readOnly` or +/// `writeOnly`. +fn schema_marks_a_direction(schema: &Schema) -> bool { + if schema.schema_data.read_only || schema.schema_data.write_only { + return true; + } + let mut found = false; + walk_inline_schemas(schema, &mut |inner| { + found = found || inner.schema_data.read_only || inner.schema_data.write_only; + }); + return found; +} + +/// The same-document component schemas a schema names, at any depth. +fn schema_local_refs(schema: &Schema, out: &mut Vec) { + let mut collect = |entry: &ReferenceOr| { + if let ReferenceOr::Reference { reference } = entry + && ref_file_part(reference).is_none() + && let Some(name) = ref_component_name(reference, "schemas") + { + out.push(name.to_owned()); + } + }; + for entry in inline_members(schema) { + collect(&entry); + } + walk_inline_schemas(schema, &mut |inner| { + for entry in inline_members(inner) { + collect(&entry); + } + }); +} + +/// Apply `visit` to every schema written inline within `schema`, at any depth. +fn walk_inline_schemas(schema: &Schema, visit: &mut impl FnMut(&Schema)) { + for entry in inline_members(schema) { + if let ReferenceOr::Item(inner) = entry { + visit(&inner); + walk_inline_schemas(&inner, visit); + } + } +} + +/// The schemas one schema holds directly: its properties, its element type, its +/// `additionalProperties`, and its composition members. +fn inline_members(schema: &Schema) -> Vec> { + let unbox = |entry: &ReferenceOr>| { + return match entry { + ReferenceOr::Item(inner) => ReferenceOr::Item((**inner).clone()), + ReferenceOr::Reference { reference } => ReferenceOr::Reference { + reference: reference.clone(), + }, + }; + }; + let additional = |entry: &Option| { + return match entry { + Some(openapiv3::AdditionalProperties::Schema(inner)) => vec![(**inner).clone()], + Some(openapiv3::AdditionalProperties::Any(_)) | None => Vec::new(), + }; + }; + let mut members = Vec::new(); + match &schema.schema_kind { + openapiv3::SchemaKind::Type(openapiv3::Type::Object(object)) => { + members.extend(object.properties.values().map(&unbox)); + members.extend(additional(&object.additional_properties)); + } + openapiv3::SchemaKind::Type(openapiv3::Type::Array(array)) => { + members.extend(array.items.as_ref().map(&unbox)); + } + openapiv3::SchemaKind::Type(_) => {} + openapiv3::SchemaKind::OneOf { one_of } => members.extend(one_of.iter().cloned()), + openapiv3::SchemaKind::AllOf { all_of } => members.extend(all_of.iter().cloned()), + openapiv3::SchemaKind::AnyOf { any_of } => members.extend(any_of.iter().cloned()), + openapiv3::SchemaKind::Not { not } => members.push((**not).clone()), + openapiv3::SchemaKind::Any(any) => { + members.extend(any.properties.values().map(&unbox)); + members.extend(additional(&any.additional_properties)); + members.extend(any.items.as_ref().map(&unbox)); + members.extend(any.one_of.iter().cloned()); + members.extend(any.all_of.iter().cloned()); + members.extend(any.any_of.iter().cloned()); + members.extend(any.not.as_ref().map(|not| return (**not).clone())); + } + } + return members; +} + /// The name a schema in a referenced document declares for itself, honouring /// `x-rust-name`. fn external_name_of(entry: &ReferenceOr, name: &str) -> Result { diff --git a/crates/oapi-codegen/src/lower/direction.rs b/crates/oapi-codegen/src/lower/direction.rs new file mode 100644 index 0000000..7f489a8 --- /dev/null +++ b/crates/oapi-codegen/src/lower/direction.rs @@ -0,0 +1,499 @@ +//! Splitting a model into the request shape and the response shape that its +//! `readOnly` and `writeOnly` properties describe. +//! +//! OpenAPI marks a property `readOnly` when a response may carry it and a +//! request must not, and `writeOnly` for the opposite. The mark names a +//! direction, not a value, so one struct cannot state both: the same `Order` +//! type reaches a request body and a response body, and a serde attribute that +//! is right for one is wrong for the other. A server deserializes a request and +//! serializes a response, and a client does the reverse, so an attribute cannot +//! even be chosen per target. +//! +//! So a marked model becomes two models. `Order` with a `readOnly` `id` emits +//! `OrderRequest`, which has no `id`, and `OrderResponse`, which has one. Each +//! API position then names the shape its direction carries: a request body, a +//! parameter and a multipart part take the request shape, and a response body +//! and a response header take the response shape. +//! +//! The split spreads along model references. A model holding a marked model +//! cannot keep one name either, because its field type differs per direction, +//! so `Envelope { order: Order }` becomes `EnvelopeRequest { order: OrderRequest }` +//! and `EnvelopeResponse { order: OrderResponse }`. A model that no mark reaches +//! keeps its name, so a document that uses neither keyword generates exactly +//! what it generated before. +//! +//! The split reads the marks only. It does not read how the operations use a +//! model, so the two names a schema takes stay the same when an operation is +//! added or removed. A shape that no operation reaches is then dropped by +//! [`crate::lower::prune`], the same way any unused model is. + +use std::collections::BTreeSet; +use std::collections::HashMap; + +use crate::ir::Alias; +use crate::ir::Direction; +use crate::ir::Enum; +use crate::ir::EnumKind; +use crate::ir::Item; +use crate::ir::Module; +use crate::ir::RequestPayload; +use crate::ir::ResponseBody; +use crate::ir::RustType; +use crate::ir::Service; +use crate::ir::Struct; +use crate::ir::UnionVariant; +use crate::naming::Case; +use crate::naming::RustIdent; +use crate::naming::to_ident; + +/// The suffix the request shape of a split model takes. +pub const REQUEST_SUFFIX: &str = "Request"; +/// The suffix the response shape of a split model takes. +pub const RESPONSE_SUFFIX: &str = "Response"; + +/// Replace every model a direction mark reaches with its request shape and its +/// response shape, and point each API position at the shape its direction +/// carries. +/// +/// `service` is `None` for a models-only run, which has no operation to give a +/// direction. Both shapes are emitted there, because a models crate is consumed +/// by a run that does know the direction. +/// +/// A document that marks no property leaves both the module and the service +/// untouched. +pub fn split_by_direction(module: &mut Module, service: Option<&mut Service>) { + let split = split_models(module); + if split.is_empty() { + return; + } + let mut items = Vec::with_capacity(module.items.len() + split.len()); + for item in module.items.drain(..) { + if split.contains(item.name()) { + items.push(project_item(&item, Direction::Request, &split)); + items.push(project_item(&item, Direction::Response, &split)); + } else { + items.push(item); + } + } + module.items = items; + if let Some(service) = service { + project_service(service, &split); + } +} + +/// The name a model takes in one direction. +pub fn projected_name(name: &str, direction: Direction) -> RustIdent { + let suffix = match direction { + Direction::Request => REQUEST_SUFFIX, + Direction::Response => RESPONSE_SUFFIX, + }; + return to_ident(&format!("{name} {suffix}"), Case::Pascal); +} + +/// The models that must be split: the ones marking a property, plus every model +/// that reaches one of those through a field, a variant, or an alias target. +/// +/// The walk runs up the reference graph, from a marked model to the models +/// naming it, because a holder's field type is what changes per direction. +fn split_models(module: &Module) -> BTreeSet { + let mut marked: BTreeSet = module + .items + .iter() + .filter(|item| return marks_a_direction(item)) + .map(|item| return item.name().to_owned()) + .collect(); + if marked.is_empty() { + return marked; + } + + let mut referrers: HashMap> = HashMap::new(); + for item in &module.items { + for target in item_references(item) { + referrers.entry(target).or_default().push(item.name().to_owned()); + } + } + + let mut stack: Vec = marked.iter().cloned().collect(); + while let Some(name) = stack.pop() { + let Some(parents) = referrers.get(&name) else { + continue; + }; + for parent in parents { + if marked.insert(parent.clone()) { + stack.push(parent.clone()); + } + } + } + return marked; +} + +/// Whether an item declares a property that only one direction carries. +fn marks_a_direction(item: &Item) -> bool { + let Item::Struct(strukt) = item else { + return false; + }; + return strukt.fields.iter().any(|field| { + return field.access != crate::ir::Access::ReadWrite; + }); +} + +/// The names of the generated models an item references, canonicalized the way +/// [`Item::name`] spells them. +fn item_references(item: &Item) -> Vec { + let mut names = Vec::new(); + match item { + Item::Struct(strukt) => { + for field in &strukt.fields { + collect_named(&field.ty, &mut names); + } + if let Some(additional) = &strukt.additional_properties { + collect_named(additional, &mut names); + } + } + Item::Enum(enom) => { + if let EnumKind::Union(variants) = &enom.kind { + for variant in variants { + collect_named(&variant.ty, &mut names); + } + } + } + Item::Alias(alias) => collect_named(&alias.ty, &mut names), + } + return names; +} + +/// Collect the model a type expression names, looking through the wrappers. +/// +/// A [`RustType::Named`] still holds the schema name the document wrote, so it +/// is run through [`to_ident`] to match the item names the graph is keyed by. +fn collect_named(ty: &RustType, out: &mut Vec) { + match ty { + RustType::Named(name) => out.push(to_ident(name, Case::Pascal).logical().to_owned()), + RustType::Vec(inner) | RustType::Map(inner) | RustType::Option(inner) | RustType::Boxed(inner) => { + collect_named(inner, out); + } + _ => {} + } +} + +/// Build one direction's shape of an item: its name takes the direction's +/// suffix, the properties the other direction carries are dropped, and every +/// reference to a split model points at that model's shape. +fn project_item(item: &Item, direction: Direction, split: &BTreeSet) -> Item { + return match item { + Item::Struct(strukt) => Item::Struct(Struct { + name: projected_name(strukt.name.logical(), direction), + doc: projected_doc(&strukt.doc, strukt.name.logical(), direction), + fields: strukt + .fields + .iter() + .filter(|field| return field.access.carried_by(direction)) + .map(|field| { + let mut projected = field.clone(); + projected.ty = project_type(&field.ty, direction, split); + return projected; + }) + .collect(), + additional_properties: strukt + .additional_properties + .as_ref() + .map(|ty| return project_type(ty, direction, split)), + ..strukt.clone() + }), + Item::Enum(enom) => Item::Enum(Enum { + name: projected_name(enom.name.logical(), direction), + doc: projected_doc(&enom.doc, enom.name.logical(), direction), + kind: match &enom.kind { + EnumKind::Union(variants) => EnumKind::Union( + variants + .iter() + .map(|variant| { + return UnionVariant { + name: variant.name.clone(), + ty: project_type(&variant.ty, direction, split), + }; + }) + .collect(), + ), + other => other.clone(), + }, + ..enom.clone() + }), + Item::Alias(alias) => Item::Alias(Alias { + name: projected_name(alias.name.logical(), direction), + doc: projected_doc(&alias.doc, alias.name.logical(), direction), + ty: project_type(&alias.ty, direction, split), + ..alias.clone() + }), + }; +} + +/// Add a line naming the direction a shape carries, after whatever the schema +/// `description` already says. +/// +/// A reader meets two types where the document declares one schema, so the +/// generated file states which of the two this is and where the name came from. +fn projected_doc(doc: &Option, name: &str, direction: Direction) -> Option { + let note = match direction { + Direction::Request => format!("The request shape of `{name}`. A `readOnly` property is not part of it."), + Direction::Response => format!("The response shape of `{name}`. A `writeOnly` property is not part of it."), + }; + return Some(match doc { + Some(text) => format!("{text}\n\n{note}"), + None => note, + }); +} + +/// Rewrite a type expression so a reference to a split model names that model's +/// shape for `direction`. Anything else is left as it is. +fn project_type(ty: &RustType, direction: Direction, split: &BTreeSet) -> RustType { + return match ty { + RustType::Named(name) => { + let canonical = to_ident(name, Case::Pascal); + if split.contains(canonical.logical()) { + RustType::Named(projected_name(canonical.logical(), direction).logical().to_owned()) + } else { + ty.clone() + } + } + RustType::Vec(inner) => RustType::Vec(Box::new(project_type(inner, direction, split))), + RustType::Map(inner) => RustType::Map(Box::new(project_type(inner, direction, split))), + RustType::Option(inner) => RustType::Option(Box::new(project_type(inner, direction, split))), + RustType::Boxed(inner) => RustType::Boxed(Box::new(project_type(inner, direction, split))), + other => other.clone(), + }; +} + +/// Point every API position at the shape of the direction it carries, and drop +/// the multipart parts a request must not send. +fn project_service(service: &mut Service, split: &BTreeSet) { + for operation in &mut service.operations { + for param in &mut operation.path_params { + param.ty = project_type(¶m.ty, Direction::Request, split); + } + if let Some(query) = &mut operation.query { + project_struct(query, Direction::Request, split); + } + if let Some(headers) = &mut operation.headers { + for param in &mut headers.params { + param.ty = project_type(¶m.ty, Direction::Request, split); + } + } + if let Some(cookies) = &mut operation.cookies { + for param in &mut cookies.params { + param.ty = project_type(¶m.ty, Direction::Request, split); + } + } + if let Some(request) = &mut operation.request { + match request { + RequestPayload::Single(body) => body.ty = project_type(&body.ty, Direction::Request, split), + RequestPayload::Multipart(multipart) => { + for field in &mut multipart.fields { + field.ty = project_type(&field.ty, Direction::Request, split); + } + } + RequestPayload::Negotiated(negotiated) => { + for variant in &mut negotiated.variants { + variant.body.ty = project_type(&variant.body.ty, Direction::Request, split); + } + } + } + } + for case in &mut operation.responses { + for header in &mut case.headers { + header.ty = project_type(&header.ty, Direction::Response, split); + } + match &mut case.body { + Some(ResponseBody::Single(body)) => body.ty = project_type(&body.ty, Direction::Response, split), + Some(ResponseBody::Negotiated(negotiated)) => { + for variant in &mut negotiated.variants { + variant.body.ty = project_type(&variant.body.ty, Direction::Response, split); + } + } + None => {} + } + } + } +} + +/// Rewrite the type of every field of a per-operation struct. +fn project_struct(strukt: &mut Struct, direction: Direction, split: &BTreeSet) { + for field in &mut strukt.fields { + field.ty = project_type(&field.ty, direction, split); + } + if let Some(additional) = &mut strukt.additional_properties { + *additional = project_type(additional, direction, split); + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use super::*; + use crate::ir::Access; + use crate::ir::Field; + use crate::loader::Spec; + + /// Lower an inline document's schemas and split them, which is the pipeline + /// a models-only run does. + fn split_yaml(yaml: &str) -> Module { + let doc: openapiv3::OpenAPI = serde_yaml::from_str(yaml).expect("parse spec"); + let spec = Spec::from_parts(doc, PathBuf::from("inline.yaml")); + let names = crate::lower::rename::type_renames(&spec, None).expect("resolve names"); + let mut module = crate::lower::generate_models(&spec, &names).expect("lower schemas"); + split_by_direction(&mut module, None); + return module; + } + + /// The item names a module declares, in emission order. + fn names(module: &Module) -> Vec { + return module.items.iter().map(|item| return item.name().to_owned()).collect(); + } + + /// The field names of the struct called `name`. + fn fields(module: &Module, name: &str) -> Vec { + for item in &module.items { + if let Item::Struct(strukt) = item + && strukt.name.logical() == name + { + return strukt + .fields + .iter() + .map(|field| return field.name.logical().to_owned()) + .collect(); + } + } + panic!("no struct named `{name}`"); + } + + const PREAMBLE: &str = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths: {}\ncomponents:\n schemas:\n"; + + #[test] + fn a_document_with_no_mark_is_left_alone() { + let module = split_yaml(&format!( + "{PREAMBLE} Widget:\n type: object\n properties:\n name:\n type: string\n" + )); + assert_eq!(names(&module), vec!["Widget"]); + } + + #[test] + fn a_marked_property_leaves_the_shape_the_other_direction_carries() { + let module = split_yaml(&format!( + "{PREAMBLE} Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n secret:\n type: string\n writeOnly: true\n email:\n type: string\n" + )); + assert_eq!(names(&module), vec!["AccountRequest", "AccountResponse"]); + assert_eq!(fields(&module, "AccountRequest"), vec!["secret", "email"]); + assert_eq!(fields(&module, "AccountResponse"), vec!["id", "email"]); + } + + #[test] + fn a_mark_on_a_referenced_schema_reaches_the_property_that_names_it() { + let module = split_yaml(&format!( + "{PREAMBLE} Id:\n type: string\n readOnly: true\n Account:\n type: object\n properties:\n id:\n $ref: '#/components/schemas/Id'\n email:\n type: string\n" + )); + assert_eq!(fields(&module, "AccountRequest"), vec!["email"]); + assert_eq!(fields(&module, "AccountResponse"), vec!["id", "email"]); + } + + #[test] + fn an_all_of_member_carries_its_marks_into_the_merged_shape() { + let module = split_yaml(&format!( + "{PREAMBLE} Timestamps:\n type: object\n properties:\n createdAt:\n type: string\n readOnly: true\n Account:\n allOf:\n - $ref: '#/components/schemas/Timestamps'\n - type: object\n properties:\n email:\n type: string\n" + )); + assert_eq!(fields(&module, "AccountRequest"), vec!["email"]); + assert_eq!(fields(&module, "AccountResponse"), vec!["created_at", "email"]); + } + + #[test] + fn a_holder_of_a_split_model_splits_and_names_the_matching_shape() { + let module = split_yaml(&format!( + "{PREAMBLE} Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n Envelope:\n type: object\n properties:\n account:\n $ref: '#/components/schemas/Account'\n" + )); + assert_eq!( + names(&module), + vec![ + "AccountRequest", + "AccountResponse", + "EnvelopeRequest", + "EnvelopeResponse" + ] + ); + let request = module + .items + .iter() + .find(|item| return item.name() == "EnvelopeRequest") + .expect("request shape"); + let Item::Struct(strukt) = request else { + panic!("expected a struct"); + }; + assert_eq!( + strukt.fields.first().map(|field| return field.ty.label()), + Some("Option".to_owned()) + ); + } + + #[test] + fn both_marks_on_one_property_are_rejected() { + let doc: openapiv3::OpenAPI = serde_yaml::from_str(&format!( + "{PREAMBLE} Account:\n type: object\n properties:\n secret:\n type: string\n readOnly: true\n writeOnly: true\n" + )) + .expect("parse spec"); + let spec = Spec::from_parts(doc, PathBuf::from("inline.yaml")); + let names = crate::lower::rename::type_renames(&spec, None).expect("resolve names"); + let error = crate::lower::generate_models(&spec, &names).expect_err("both marks must be rejected"); + assert!( + format!("{error}").contains("`readOnly` and `writeOnly` are both set"), + "unexpected message: {error}" + ); + } + + #[test] + fn a_name_that_is_not_pascal_case_still_matches_the_split_set() { + // A `RustType::Named` holds the schema name the document wrote, so the + // lookup canonicalizes it. Without that, a reference to `order-item` + // would miss the `OrderItem` entry and keep the unsplit name. + let module = split_yaml(&format!( + "{PREAMBLE} order-item:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n Basket:\n type: object\n properties:\n item:\n $ref: '#/components/schemas/order-item'\n" + )); + assert!(names(&module).contains(&"OrderItemRequest".to_owned())); + let Some(Item::Struct(basket)) = module.items.iter().find(|item| return item.name() == "BasketRequest") else { + panic!("no request shape for `Basket`"); + }; + assert_eq!( + basket.fields.first().map(|field| return field.ty.label()), + Some("Option".to_owned()) + ); + } + + #[test] + fn access_reports_the_direction_that_carries_the_property() { + let cases = [ + (Access::ReadWrite, true, true), + (Access::ReadOnly, false, true), + (Access::WriteOnly, true, false), + ]; + for (access, request, response) in cases { + assert_eq!(access.carried_by(Direction::Request), request, "{access:?} request"); + assert_eq!(access.carried_by(Direction::Response), response, "{access:?} response"); + } + } + + #[test] + fn a_field_with_no_mark_keeps_the_default_access() { + let field = Field { + name: to_ident("name", Case::Snake), + rename: None, + doc: None, + deprecated: None, + ty: RustType::String, + required: true, + omit_empty: None, + serde_skip: false, + default: None, + constraints: None, + access: Access::default(), + }; + assert_eq!(field.access, Access::ReadWrite); + } +} diff --git a/crates/oapi-codegen/src/lower/mod.rs b/crates/oapi-codegen/src/lower/mod.rs index 0e82332..f6106b0 100644 --- a/crates/oapi-codegen/src/lower/mod.rs +++ b/crates/oapi-codegen/src/lower/mod.rs @@ -10,6 +10,7 @@ pub(crate) mod constraints; /// Lowering of a schema `default`. Used by [`schema`] and [`paths`] only. pub(crate) mod default; +pub mod direction; /// Readers for the `x-` extensions the generator understands. pub(crate) mod extension; pub mod paths; @@ -21,6 +22,7 @@ pub mod security; pub mod servers; pub mod validate; +pub use crate::lower::direction::split_by_direction; pub use crate::lower::paths::generate_service; pub use crate::lower::prune::prune_unused_models; pub use crate::lower::recurse::box_recursive_types; diff --git a/crates/oapi-codegen/src/lower/paths.rs b/crates/oapi-codegen/src/lower/paths.rs index c7b56f7..a0199fe 100644 --- a/crates/oapi-codegen/src/lower/paths.rs +++ b/crates/oapi-codegen/src/lower/paths.rs @@ -50,6 +50,7 @@ use openapiv3::ReferenceOr; use openapiv3::RequestBody; use openapiv3::Response as OasResponse; use openapiv3::Schema; +use openapiv3::SchemaData; use openapiv3::SchemaKind; use openapiv3::StatusCode; use openapiv3::Type; @@ -440,6 +441,9 @@ impl Lowerer<'_> { serde_skip: false, default, constraints, + // A parameter only ever travels in a request, so a direction mark on + // its schema states nothing the generator can act on. + access: crate::ir::Access::ReadWrite, }; crate::lower::constraints::check_constraints(&field)?; return Ok(field); @@ -1178,7 +1182,12 @@ impl Lowerer<'_> { return name == wire_name; }); let (kind, nullable) = match property { - ReferenceOr::Item(schema) => (schema.schema_kind.clone(), schema.schema_data.nullable), + ReferenceOr::Item(schema) => { + if !multipart_part_is_sent(&schema.schema_data, path, method, wire_name)? { + continue; + } + (schema.schema_kind.clone(), schema.schema_data.nullable) + } ReferenceOr::Reference { reference } => { if ref_file_part(reference).is_some() { return Err(Error::UnsupportedOperation { @@ -1190,6 +1199,9 @@ impl Lowerer<'_> { }); } let resolved = self.spec.resolve_schema(None, reference)?; + if !multipart_part_is_sent(&resolved.schema_data, path, method, wire_name)? { + continue; + } (resolved.schema_kind, resolved.schema_data.nullable) } }; @@ -1551,6 +1563,17 @@ impl Lowerer<'_> { } } +/// Whether a `multipart/form-data` part travels in a request. +/// +/// A multipart body is a request body only, so a `readOnly` part is left out of +/// the generated extractor. `writeOnly` states that a request carries the part, +/// which is what a multipart part already does. +fn multipart_part_is_sent(data: &SchemaData, path: &str, method: &str, wire_name: &str) -> Result { + let at = format!("{method} {path} multipart field `{wire_name}`"); + let access = crate::lower::schema::access_of(data, &at)?; + return Ok(access.carried_by(crate::ir::Direction::Request)); +} + /// Derive the trait method name: an explicit `x-rust-name`, else the /// `operationId`, else a name synthesised from the method and path (for example /// `get /v1/widgets` -> `get_v1_widgets`). diff --git a/crates/oapi-codegen/src/lower/recurse.rs b/crates/oapi-codegen/src/lower/recurse.rs index 69058c1..a46d738 100644 --- a/crates/oapi-codegen/src/lower/recurse.rs +++ b/crates/oapi-codegen/src/lower/recurse.rs @@ -426,6 +426,7 @@ fn canonical(name: &str) -> String { #[cfg(test)] mod tests { use super::*; + use crate::ir::Access; use crate::ir::Alias; use crate::ir::Enum; use crate::ir::Field; @@ -449,6 +450,7 @@ mod tests { serde_skip: false, default: None, constraints: None, + access: Access::ReadWrite, }], additional_properties: None, deny_unknown_fields: false, diff --git a/crates/oapi-codegen/src/lower/schema.rs b/crates/oapi-codegen/src/lower/schema.rs index cb74c7c..141b1f5 100644 --- a/crates/oapi-codegen/src/lower/schema.rs +++ b/crates/oapi-codegen/src/lower/schema.rs @@ -15,6 +15,7 @@ use openapiv3::VariantOrUnknownOrEmpty; use crate::error::Error; use crate::error::Result; +use crate::ir::Access; use crate::ir::Alias; use crate::ir::Deprecation; use crate::ir::Enum; @@ -329,6 +330,16 @@ impl Mapper<'_> { None => to_ident(wire, Case::Snake), }; let rename = crate::naming::rename_for(wire, &ident); + let access = match prop { + ReferenceOr::Item(schema) => access_of(&schema.schema_data, &at)?, + // A `$ref` property carries no sibling keyword in OpenAPI 3.0, so the + // mark can only sit on the target. The checks below read the target + // the same way. + ReferenceOr::Reference { reference } => match self.spec.resolve(reference) { + Ok(target) => access_of(&target.schema_data, &at)?, + Err(_) => Access::ReadWrite, + }, + }; let constraints = match prop { ReferenceOr::Item(schema) => crate::lower::constraints::constraints_of(schema), // The alias a `$ref` makes carries no serde attribute, so the field @@ -350,6 +361,7 @@ impl Mapper<'_> { serde_skip, default, constraints, + access, }; crate::lower::constraints::check_constraints(&field)?; return Ok(field); @@ -1064,6 +1076,25 @@ fn verbatim_type(data: &SchemaData, verbatim: &str, path: &str) -> Result Result { + return match (data.read_only, data.write_only) { + (true, true) => Err(Error::UnsupportedSchema { + path: at.to_owned(), + reason: "`readOnly` and `writeOnly` are both set, so no request and no response could \ + carry the property. Set at most one of the two." + .to_owned(), + }), + (true, false) => Ok(Access::ReadOnly), + (false, true) => Ok(Access::WriteOnly), + (false, false) => Ok(Access::ReadWrite), + }; +} + /// Derive a `#[deprecated]` annotation from `deprecated: true` and an optional /// `x-deprecated-reason` note. Returns `None` unless the schema is deprecated, /// so a lone `x-deprecated-reason` is a no-op (matching `oapi-codegen`). diff --git a/crates/oapi-codegen/tests/coverage.rs b/crates/oapi-codegen/tests/coverage.rs index e614917..205391e 100644 --- a/crates/oapi-codegen/tests/coverage.rs +++ b/crates/oapi-codegen/tests/coverage.rs @@ -333,13 +333,18 @@ const TEST_TABLE: &[Feature] = &[ }, Feature { element: "meta.readOnly", - status: Status::Ignored, - fixture: None, + status: Status::Supported, + fixture: Some("read_write_only"), }, Feature { element: "meta.writeOnly", - status: Status::Ignored, - fixture: None, + status: Status::Supported, + fixture: Some("read_write_only"), + }, + Feature { + element: "meta.readOnly.with.writeOnly", + status: Status::Unsupported, + fixture: Some("read_write_only_conflict"), }, Feature { element: "meta.example", @@ -555,6 +560,7 @@ const SERVER_UNSUPPORTED_FIXTURES: &[&str] = &[ "server_unsupported_xfile_missing_component", "server_unsupported_xfile_missing_schema", "server_unsupported_xfile_name_clash", + "server_unsupported_xfile_direction_split", "server_unsupported_xfile_no_import_mapping", "server_unsupported_xfile_object_path_param", "server_unsupported_object_response_header", @@ -601,6 +607,7 @@ const CLIENT_UNSUPPORTED_FIXTURES: &[&str] = &[ /// same per-operation types alongside the component models. const COMBINED_FIXTURES: &[&str] = &[ "combined_keyword_operations", + "combined_read_write_only", "combined_prelude_value_names", "combined_server_client", "combined_response_name_collision", @@ -742,6 +749,7 @@ generated_tests!( compose_shared, primitive_scalars, recursive_schema, + read_write_only, ref_local, integer_enum, string_enum, @@ -777,6 +785,10 @@ fn server_config() -> oapi_codegen::Config { "schemas/compose_clash.yaml".to_owned(), "crate::generated::compose_clash".to_owned(), ); + import_mapping.insert( + "schemas/compose_marked.yaml".to_owned(), + "crate::generated::compose_marked".to_owned(), + ); return oapi_codegen::Config { generate: oapi_codegen::config::Generate { std_http_server: true, @@ -887,6 +899,42 @@ fn server_unsupported_features_are_rejected() { } } +/// A property that sets both direction marks is rejected **for that**, and not +/// for whatever else the lowering pass trips over first. +/// +/// `unsupported_features_are_rejected` asserts `is_err()` only, so it passes on +/// any error at all. The OpenAPI specification calls the combination invalid, so +/// the message must name both keywords and leave the author with one remedy. +#[test] +fn a_property_that_sets_both_direction_marks_names_both_in_the_message() { + let fixture = tests_dir().join("fixtures").join("read_write_only_conflict.yaml"); + let error = oapi_codegen::generate_models_string(&fixture).expect_err("both marks must be rejected"); + let message = format!("{error}"); + assert!( + message.contains("`readOnly` and `writeOnly` are both set"), + "the message must name both keywords: {message}", + ); +} + +/// A cross-file reference to a schema the direction pass splits is rejected, and +/// the message names the two shapes the other run emits. +/// +/// The run that writes the operations resolves the reference by name alone, so +/// it cannot tell which shape the author meant. Emitting the plain name compiles +/// nothing, because the models crate declares neither. +#[test] +fn a_cross_file_reference_to_a_split_schema_is_rejected() { + let fixture = tests_dir() + .join("fixtures") + .join("server_unsupported_xfile_direction_split.yaml"); + let error = oapi_codegen::generate(&fixture, &server_config()).expect_err("a split target must be rejected"); + let message = format!("{error}"); + assert!( + message.contains("ParcelRequest") && message.contains("ParcelResponse"), + "the message must name both shapes: {message}", + ); +} + /// Unused component schemas are pruned by default, but retained when /// `output-options.skip-prune` is set — mirroring `oapi-codegen`'s pruning. #[test] @@ -1276,6 +1324,7 @@ macro_rules! combined_generated_tests { combined_generated_tests!( combined_keyword_operations, + combined_read_write_only, combined_prelude_value_names, combined_server_client, combined_response_name_collision, diff --git a/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml b/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml new file mode 100644 index 0000000..b4cb144 --- /dev/null +++ b/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml @@ -0,0 +1,89 @@ +openapi: 3.0.3 +info: + title: Combined Accounts API + version: 1.0.0 +paths: + /accounts: + post: + operationId: createAccount + summary: Create an account. + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/Account" + responses: + "201": + description: The created account. + content: + application/json: + schema: + $ref: "#/components/schemas/Account" + get: + operationId: listAccounts + summary: List every account. + responses: + "200": + description: The accounts, wrapped in a page. + content: + application/json: + schema: + $ref: "#/components/schemas/AccountPage" + /accounts/{accountId}/avatar: + put: + operationId: uploadAvatar + summary: Replace an account avatar. + parameters: + - name: accountId + in: path + required: true + schema: + type: string + requestBody: + required: true + content: + multipart/form-data: + schema: + $ref: "#/components/schemas/Avatar" + responses: + "204": + description: The avatar was stored. +components: + schemas: + Account: + description: An account, used as a request body and as a response body. + type: object + required: [id, email, password] + properties: + id: + description: Assigned by the server, so only a response carries it. + type: string + readOnly: true + email: + type: string + password: + description: Sent when the account is created, and never read back. + type: string + writeOnly: true + AccountPage: + description: A response-only holder of a split model. + type: object + required: [items] + properties: + items: + type: array + items: + $ref: "#/components/schemas/Account" + Avatar: + description: A multipart body, which only a request carries. + type: object + required: [image] + properties: + image: + type: string + format: binary + checksum: + description: The server computes this, so the extractor leaves it out. + type: string + readOnly: true diff --git a/crates/oapi-codegen/tests/fixtures/read_write_only.yaml b/crates/oapi-codegen/tests/fixtures/read_write_only.yaml new file mode 100644 index 0000000..8ac1394 --- /dev/null +++ b/crates/oapi-codegen/tests/fixtures/read_write_only.yaml @@ -0,0 +1,51 @@ +openapi: "3.0.3" +info: + title: Read-only and write-only properties + version: "1.0.0" +paths: {} +components: + schemas: + Account: + description: A schema whose marks split it into a request shape and a response shape. + type: object + required: [id, email, password, nickname] + properties: + id: + description: Assigned by the server, so only a response carries it. + type: string + format: uuid + readOnly: true + createdAt: + type: string + format: date-time + readOnly: true + email: + type: string + password: + description: Sent when the account is created, and never read back. + type: string + writeOnly: true + nickname: + type: string + Envelope: + description: A holder splits too, because its field type differs per direction. + type: object + required: [account] + properties: + account: + $ref: "#/components/schemas/Account" + accounts: + type: array + items: + $ref: "#/components/schemas/Account" + AccountList: + description: An alias to a split model splits as well. + type: array + items: + $ref: "#/components/schemas/Account" + Untouched: + description: No mark reaches this schema, so it keeps its name. + type: object + properties: + label: + type: string diff --git a/crates/oapi-codegen/tests/fixtures/read_write_only_conflict.yaml b/crates/oapi-codegen/tests/fixtures/read_write_only_conflict.yaml new file mode 100644 index 0000000..37b8d30 --- /dev/null +++ b/crates/oapi-codegen/tests/fixtures/read_write_only_conflict.yaml @@ -0,0 +1,16 @@ +openapi: "3.0.3" +info: + title: Both direction marks on one property + version: "1.0.0" +paths: {} +components: + schemas: + Account: + type: object + required: [secret] + properties: + secret: + description: No request and no response could carry this property. + type: string + readOnly: true + writeOnly: true diff --git a/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml b/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml new file mode 100644 index 0000000..188fc2f --- /dev/null +++ b/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml @@ -0,0 +1,23 @@ +openapi: 3.0.3 +info: + title: shared models that a direction mark splits + version: "1" +paths: {} +components: + schemas: + # `Parcel` marks a property `readOnly`, so the run that writes the models + # emits `ParcelRequest` and `ParcelResponse` and no `Parcel`. + # + # The run that writes the operations resolves an `import-mapping` reference + # by name alone. It cannot tell which of the two shapes the reference means, + # so it rejects the reference instead of emitting a name the models crate + # never declares. + Parcel: + type: object + required: [id, weight_kg] + properties: + id: + type: string + readOnly: true + weight_kg: + type: number diff --git a/crates/oapi-codegen/tests/fixtures/server_unsupported_xfile_direction_split.yaml b/crates/oapi-codegen/tests/fixtures/server_unsupported_xfile_direction_split.yaml new file mode 100644 index 0000000..7ee6ee0 --- /dev/null +++ b/crates/oapi-codegen/tests/fixtures/server_unsupported_xfile_direction_split.yaml @@ -0,0 +1,18 @@ +openapi: 3.0.3 +info: + title: unsupported cross-file schema ref (a direction mark splits the target) + version: "1" +paths: + /parcels: + post: + operationId: acceptParcel + summary: Accept a parcel. + requestBody: + required: true + content: + application/json: + schema: + $ref: "schemas/compose_marked.yaml#/components/schemas/Parcel" + responses: + "201": + description: The parcel was accepted. diff --git a/crates/oapi-codegen/tests/generated.rs b/crates/oapi-codegen/tests/generated.rs index df3fcda..a10e20b 100644 --- a/crates/oapi-codegen/tests/generated.rs +++ b/crates/oapi-codegen/tests/generated.rs @@ -34,6 +34,8 @@ mod generated { pub mod combined_keyword_operations; #[path = "combined_prelude_value_names.rs"] pub mod combined_prelude_value_names; + #[path = "combined_read_write_only.rs"] + pub mod combined_read_write_only; #[path = "combined_response_name_collision.rs"] pub mod combined_response_name_collision; #[path = "combined_server_client.rs"] @@ -81,6 +83,8 @@ mod generated { pub mod prelude_value_names; #[path = "primitive_scalars.rs"] pub mod primitive_scalars; + #[path = "read_write_only.rs"] + pub mod read_write_only; #[path = "recursive_schema.rs"] pub mod recursive_schema; #[path = "ref_local.rs"] @@ -249,6 +253,72 @@ fn optional_field_is_skipped_when_none() { assert_eq!(json, r#"{"id":"u1"}"#); } +#[test] +fn a_direction_mark_splits_a_model_into_two_shapes() { + use generated::read_write_only; + + // The request shape drops what only a response carries, so a payload the + // client builds cannot state a server-assigned value. + let request = read_write_only::AccountRequest { + email: "a@example.com".to_owned(), + password: "hunter2".to_owned(), + nickname: "ann".to_owned(), + }; + let json = serde_json::to_string(&request).expect("serialize"); + assert_eq!( + json, + r#"{"email":"a@example.com","password":"hunter2","nickname":"ann"}"# + ); + + // The response shape drops what only a request carries, and reading a + // response that still holds it ignores the extra key. + let response: read_write_only::AccountResponse = serde_json::from_str( + r#"{"id":"3a1f9b0e-1c1a-4d0f-8b6f-2f9a4c5d6e70","email":"a@example.com","nickname":"ann","password":"hunter2"}"#, + ) + .expect("deserialize"); + assert_eq!(response.email, "a@example.com"); + let json = serde_json::to_string(&response).expect("serialize"); + assert!( + !json.contains("password"), + "response shape wrote a write-only property: {json}" + ); +} + +#[test] +fn a_holder_of_a_split_model_names_the_shape_of_its_direction() { + use generated::read_write_only; + + let envelope = read_write_only::EnvelopeRequest { + account: read_write_only::AccountRequest { + email: "a@example.com".to_owned(), + password: "hunter2".to_owned(), + nickname: "ann".to_owned(), + }, + accounts: None, + }; + let json = serde_json::to_string(&envelope).expect("serialize"); + assert!( + !json.contains("\"id\""), + "request shape wrote a read-only property: {json}" + ); + + let list: read_write_only::AccountListResponse = serde_json::from_str( + r#"[{"id":"3a1f9b0e-1c1a-4d0f-8b6f-2f9a4c5d6e70","email":"a@example.com","nickname":"ann"}]"#, + ) + .expect("deserialize"); + assert_eq!(list.len(), 1); +} + +#[test] +fn a_read_only_multipart_part_is_left_out_of_the_extractor() { + use generated::combined_read_write_only; + + // `Avatar.checksum` is `readOnly`, and a multipart body only travels in a + // request, so the extractor never reads that part. + let body = combined_read_write_only::UploadAvatarMultipart { image: vec![1, 2, 3] }; + assert_eq!(body.image, vec![1, 2, 3]); +} + #[test] fn generated_server_trait_implements_and_routes() { use generated::server_petstore; diff --git a/crates/oapi-codegen/tests/generated/combined_read_write_only.rs b/crates/oapi-codegen/tests/generated/combined_read_write_only.rs new file mode 100644 index 0000000..1ab6229 --- /dev/null +++ b/crates/oapi-codegen/tests/generated/combined_read_write_only.rs @@ -0,0 +1,358 @@ +// Code generated by oapi-codegen-rust. DO NOT EDIT. +#![allow( + dead_code, + unused_imports, + clippy::all, + clippy::pedantic, + clippy::nursery, + clippy::restriction, + reason = "generated code, not first-party source" +)] + +/// An account, used as a request body and as a response body. +/// +/// The request shape of `Account`. A `readOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountRequest { + pub email: String, + /// Sent when the account is created, and never read back. + pub password: String, +} + +/// An account, used as a request body and as a response body. +/// +/// The response shape of `Account`. A `writeOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountResponse { + /// Assigned by the server, so only a response carries it. + pub id: String, + pub email: String, +} + +/// A response-only holder of a split model. +/// +/// The response shape of `AccountPage`. A `writeOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountPageResponse { + pub items: Vec, +} + +/// List every account. +#[derive(Debug, Clone, PartialEq)] +pub enum ListAccountsResponse { + /// The accounts, wrapped in a page. + Ok(AccountPageResponse), +} + +/// Create an account. +#[derive(Debug, Clone, PartialEq)] +pub enum CreateAccountResponse { + /// The created account. + Created(AccountResponse), +} + +#[derive(Debug, Clone)] +pub struct UploadAvatarMultipart { + pub image: Vec, +} + +/// Replace an account avatar. +#[derive(Debug, Clone, PartialEq)] +pub enum UploadAvatarResponse { + /// The avatar was stored. + NoContent, +} + +impl axum::extract::FromRequest for UploadAvatarMultipart +where + S: Send + Sync, +{ + type Rejection = (axum::http::StatusCode, String); + async fn from_request( + request: axum::extract::Request, + state: &S, + ) -> Result { + let mut multipart = >::from_request(request, state) + .await + .map_err(|error| { + return (axum::http::StatusCode::BAD_REQUEST, error.to_string()); + })?; + let mut image: Option> = None; + while let Some(field) = multipart + .next_field() + .await + .map_err(|error| { + return (axum::http::StatusCode::BAD_REQUEST, error.to_string()); + })? + { + let field_name = field.name().map(|name| return name.to_owned()); + match field_name.as_deref() { + Some("image") => { + let value = field + .bytes() + .await + .map_err(|error| { + return ( + axum::http::StatusCode::BAD_REQUEST, + error.to_string(), + ); + })?; + image = Some(value.to_vec()); + } + _ => {} + } + } + return Ok(Self { + image: image + .ok_or(( + axum::http::StatusCode::BAD_REQUEST, + "missing required multipart field `image`".to_owned(), + ))?, + }); + } +} + +/// Server behaviour: implement one method per operation. +pub trait Api: Clone + Send + Sync + 'static { + /// List every account. + fn list_accounts( + &self, + ) -> impl std::future::Future + Send; + /// Create an account. + fn create_account( + &self, + body: AccountRequest, + ) -> impl std::future::Future + Send; + /// Replace an account avatar. + fn upload_avatar( + &self, + account_id: String, + body: UploadAvatarMultipart, + ) -> impl std::future::Future + Send; +} + +impl axum::response::IntoResponse for ListAccountsResponse { + fn into_response(self) -> axum::response::Response { + match self { + ListAccountsResponse::Ok(body) => { + const STATUS: axum::http::StatusCode = match axum::http::StatusCode::from_u16( + 200, + ) { + Ok(status) => status, + Err(_) => panic!("oapi-codegen emitted an invalid HTTP status code"), + }; + (STATUS, axum::Json(body)).into_response() + } + } + } +} + +impl axum::response::IntoResponse for CreateAccountResponse { + fn into_response(self) -> axum::response::Response { + match self { + CreateAccountResponse::Created(body) => { + const STATUS: axum::http::StatusCode = match axum::http::StatusCode::from_u16( + 201, + ) { + Ok(status) => status, + Err(_) => panic!("oapi-codegen emitted an invalid HTTP status code"), + }; + (STATUS, axum::Json(body)).into_response() + } + } + } +} + +impl axum::response::IntoResponse for UploadAvatarResponse { + fn into_response(self) -> axum::response::Response { + match self { + UploadAvatarResponse::NoContent => { + const STATUS: axum::http::StatusCode = match axum::http::StatusCode::from_u16( + 204, + ) { + Ok(status) => status, + Err(_) => panic!("oapi-codegen emitted an invalid HTTP status code"), + }; + STATUS.into_response() + } + } + } +} + +/// Build an axum `Router` that dispatches each route to `api`. +pub fn router(api: T) -> axum::Router { + axum::Router::new() + .route( + "/accounts", + axum::routing::get(list_accounts_handler::) + .post(create_account_handler::), + ) + .route( + "/accounts/{accountId}/avatar", + axum::routing::put(upload_avatar_handler::), + ) + .with_state(api) +} + +async fn list_accounts_handler( + axum::extract::State(api): axum::extract::State, +) -> ListAccountsResponse { + api.list_accounts().await +} + +async fn create_account_handler( + axum::extract::State(api): axum::extract::State, + axum::Json(body): axum::Json, +) -> CreateAccountResponse { + api.create_account(body).await +} + +async fn upload_avatar_handler( + axum::extract::State(api): axum::extract::State, + axum::extract::Path(account_id): axum::extract::Path, + body: UploadAvatarMultipart, +) -> UploadAvatarResponse { + api.upload_avatar(account_id, body).await +} + +/// Errors returned by the generated client. +#[derive(Debug)] +pub enum ClientError { + /// The `reqwest` request failed to send or complete, including any + /// body decoding `reqwest` performs internally (such as JSON). + Http(reqwest::Error), + /// The server returned a status code the operation does not declare. + UnexpectedStatus(reqwest::StatusCode), + /// The response `Content-Type` matched none of the representations the + /// operation declares for its status. + UnexpectedContentType(String), + /// The response cannot be decoded: a body that failed to + /// deserialize (for example malformed form-urlencoded content), or a + /// required response header that was missing or unparsable. + Decode(String), +} +impl std::fmt::Display for ClientError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ClientError::Http(error) => return write!(f, "HTTP request failed: {error}"), + ClientError::UnexpectedStatus(status) => { + return write!(f, "unexpected response status: {status}"); + } + ClientError::UnexpectedContentType(content_type) => { + return write!(f, "unexpected response content type: {content_type}"); + } + ClientError::Decode(message) => { + return write!(f, "failed to decode response: {message}"); + } + } + } +} +impl std::error::Error for ClientError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ClientError::Http(error) => return Some(error), + ClientError::UnexpectedStatus(_) + | ClientError::UnexpectedContentType(_) + | ClientError::Decode(_) => return None, + } + } +} +impl From for ClientError { + fn from(error: reqwest::Error) -> Self { + return ClientError::Http(error); + } +} + +const PATH_PARAM_ENCODE_SET: &percent_encoding::AsciiSet = &percent_encoding::NON_ALPHANUMERIC + .remove(b'-') + .remove(b'.') + .remove(b'_') + .remove(b'~'); + +/// A blocking HTTP client for the API. +/// +/// `base_url` is used as a prefix for every request path and must not +/// carry a trailing slash (for example `https://api.example.com`). +#[derive(Debug, Clone)] +pub struct Client { + base_url: String, + http: reqwest::blocking::Client, +} + +impl Client { + /// Build a client targeting `base_url` with a default blocking + /// `reqwest::blocking::Client`. + pub fn new(base_url: impl Into) -> Result { + let http = reqwest::blocking::Client::builder().build()?; + return Ok(Self { + base_url: base_url.into(), + http, + }); + } + /// Build a client targeting `base_url` with a caller-provided + /// `reqwest::blocking::Client` (for example preconfigured with timeouts). + pub fn with_client( + base_url: impl Into, + http: reqwest::blocking::Client, + ) -> Self { + return Self { + base_url: base_url.into(), + http, + }; + } + /// List every account. + pub fn list_accounts(&self) -> Result { + let url = format!("{}/accounts", self.base_url); + let response = self.http.request(reqwest::Method::GET, url).send()?; + let status = response.status(); + if status.as_u16() == 200 { + let body: AccountPageResponse = response.json()?; + return Ok(ListAccountsResponse::Ok(body)); + } + return Err(ClientError::UnexpectedStatus(status)); + } + /// Create an account. + pub fn create_account( + &self, + body: AccountRequest, + ) -> Result { + let url = format!("{}/accounts", self.base_url); + let mut request = self.http.request(reqwest::Method::POST, url); + request = request.json(&body); + let response = request.send()?; + let status = response.status(); + if status.as_u16() == 201 { + let body: AccountResponse = response.json()?; + return Ok(CreateAccountResponse::Created(body)); + } + return Err(ClientError::UnexpectedStatus(status)); + } + /// Replace an account avatar. + pub fn upload_avatar( + &self, + account_id: String, + body: UploadAvatarMultipart, + ) -> Result { + let url = format!( + "{}/accounts/{}/avatar", self.base_url, + percent_encoding::utf8_percent_encode(account_id.as_str(), + PATH_PARAM_ENCODE_SET) + ); + let mut request = self.http.request(reqwest::Method::PUT, url); + let mut form = reqwest::blocking::multipart::Form::new(); + form = form + .part( + "image", + reqwest::blocking::multipart::Part::bytes(body.image).file_name("image"), + ); + request = request.multipart(form); + let response = request.send()?; + let status = response.status(); + if status.as_u16() == 204 { + return Ok(UploadAvatarResponse::NoContent); + } + return Err(ClientError::UnexpectedStatus(status)); + } +} diff --git a/crates/oapi-codegen/tests/generated/read_write_only.rs b/crates/oapi-codegen/tests/generated/read_write_only.rs new file mode 100644 index 0000000..df484d6 --- /dev/null +++ b/crates/oapi-codegen/tests/generated/read_write_only.rs @@ -0,0 +1,71 @@ +// Code generated by oapi-codegen-rust. DO NOT EDIT. +#![allow( + dead_code, + unused_imports, + clippy::all, + clippy::pedantic, + clippy::nursery, + clippy::restriction, + reason = "generated code, not first-party source" +)] + +/// A schema whose marks split it into a request shape and a response shape. +/// +/// The request shape of `Account`. A `readOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountRequest { + pub email: String, + /// Sent when the account is created, and never read back. + pub password: String, + pub nickname: String, +} + +/// A schema whose marks split it into a request shape and a response shape. +/// +/// The response shape of `Account`. A `writeOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountResponse { + /// Assigned by the server, so only a response carries it. + pub id: uuid::Uuid, + #[serde(rename = "createdAt", skip_serializing_if = "Option::is_none")] + pub created_at: Option>, + pub email: String, + pub nickname: String, +} + +/// A holder splits too, because its field type differs per direction. +/// +/// The request shape of `Envelope`. A `readOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct EnvelopeRequest { + pub account: AccountRequest, + #[serde(skip_serializing_if = "Option::is_none")] + pub accounts: Option>, +} + +/// A holder splits too, because its field type differs per direction. +/// +/// The response shape of `Envelope`. A `writeOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct EnvelopeResponse { + pub account: AccountResponse, + #[serde(skip_serializing_if = "Option::is_none")] + pub accounts: Option>, +} + +/// An alias to a split model splits as well. +/// +/// The request shape of `AccountList`. A `readOnly` property is not part of it. +pub type AccountListRequest = Vec; + +/// An alias to a split model splits as well. +/// +/// The response shape of `AccountList`. A `writeOnly` property is not part of it. +pub type AccountListResponse = Vec; + +/// No mark reaches this schema, so it keeps its name. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct Untouched { + #[serde(skip_serializing_if = "Option::is_none")] + pub label: Option, +} diff --git a/crates/oapi-codegen/tests/generated/server_auth.rs b/crates/oapi-codegen/tests/generated/server_auth.rs index 7651650..37ffbed 100644 --- a/crates/oapi-codegen/tests/generated/server_auth.rs +++ b/crates/oapi-codegen/tests/generated/server_auth.rs @@ -34,9 +34,9 @@ pub enum GetReportsResponse { Ok(Message), } -/** Fetch session data, which an API key cookie protects. - -The cookie is set at login and expires with the session.*/ +/// Fetch session data, which an API key cookie protects. +/// +/// The cookie is set at login and expires with the session. #[derive(Debug, Clone, PartialEq)] pub enum GetSessionResponse { /// The session data. diff --git a/docs/design.md b/docs/design.md index 22fa51f..797ddea 100644 --- a/docs/design.md +++ b/docs/design.md @@ -512,8 +512,47 @@ does the same. `minProperties` on a schema that names its properties, and names the way out, because a rule the document states and the code drops is worse than no rule at all. -`readOnly` and `writeOnly` are read by nothing yet. They mark a direction, not a -value, and one struct cannot be both. +## A direction mark splits a model in two + +`readOnly` and `writeOnly` mark a direction, not a value. A `readOnly` property +travels in a response, and a request must not send it. A `writeOnly` property +travels in a request, and a response must not send it. A property that sets both +marks is an error, because no direction is left to carry it. + +One struct cannot hold both statements. The same `Order` type reaches a request +body and a response body, so a serde attribute that is right for one is wrong for +the other. The side does not settle it either. A server reads a request and +writes a response, and a client does the reverse. + +So a marked model becomes two models. `Order` with a `readOnly` `id` gives +`OrderRequest`, which has no `id`, and `OrderResponse`, which has one. A request +body, a parameter, and a multipart part take the request shape. A response body +and a response header take the response shape. + +The split spreads along model references. A model that holds a marked model +cannot keep one name either, because its field type differs per direction. So +`Envelope { order: Order }` gives `EnvelopeRequest` with an `OrderRequest` field +and `EnvelopeResponse` with an `OrderResponse` field. A model that no mark +reaches keeps its name, and a document that uses neither keyword generates what +it generated before. + +The split reads the marks alone. It does not read how the operations use a model, +so the two names stay the same when an operation is added or removed. A shape +that no operation reaches is then dropped with the other unused models. + +The two names go through the same checks as any other type name. A schema named +`GetWidget` gives a `GetWidgetResponse` shape, which the response enum of a +`getWidget` operation also claims. That is reported, and `x-rust-name` or +`output-options.response-type-suffix` gives the way out. + +A `required` property needs no separate rule. The shape that must not send the +property does not declare it at all, so the requirement applies only where the +property exists. This is what the OpenAPI specification states. + +An `import-mapping` reference to a marked schema is an error. Two runs make a +composed crate, and only one of them holds the operations. The other run emits +two names for that schema, and a reference by name alone cannot say which of the +two it means. ## A `default` removes the `Option` From cdf77e19c988442ff70b72c0131d93872e451d7e Mon Sep 17 00:00:00 2001 From: Kaspar Lyngsie Date: Sun, 23 Aug 2026 16:36:46 +0200 Subject: [PATCH 2/4] fix: changes after canary tests --- crates/oapi-codegen/src/emit/mod.rs | 6 +- crates/oapi-codegen/src/emit/usage.rs | 36 +- crates/oapi-codegen/src/ir.rs | 17 +- crates/oapi-codegen/src/lib.rs | 5 +- crates/oapi-codegen/src/loader.rs | 10 +- crates/oapi-codegen/src/lower/direction.rs | 370 +++++++++++------- crates/oapi-codegen/src/lower/paths.rs | 10 +- crates/oapi-codegen/src/lower/schema.rs | 10 +- .../fixtures/combined_read_write_only.yaml | 49 +++ crates/oapi-codegen/tests/generated.rs | 30 ++ .../generated/combined_read_write_only.rs | 118 +++++- docs/design.md | 58 +-- 12 files changed, 497 insertions(+), 222 deletions(-) diff --git a/crates/oapi-codegen/src/emit/mod.rs b/crates/oapi-codegen/src/emit/mod.rs index b63f5de..5d69842 100644 --- a/crates/oapi-codegen/src/emit/mod.rs +++ b/crates/oapi-codegen/src/emit/mod.rs @@ -11,7 +11,7 @@ mod operation; mod package; mod reqwest; mod servers; -mod usage; +pub(crate) mod usage; use std::collections::HashMap; @@ -301,8 +301,8 @@ fn render_body(items: &[TokenStream]) -> Result { /// Render a doc attribute, or nothing when there is no documentation. /// -/// The text is split on its line breaks, so a multi-line `description` prints as -/// a run of `///` lines rather than one `/** */` block. +/// This splits the text on its line breaks, so a multi-line `description` prints +/// as a run of `///` lines and not one `/** */` block. pub(crate) fn doc_attr(doc: &Option) -> TokenStream { let tokens = match doc { Some(text) => doc_lines(std::slice::from_ref(text)), diff --git a/crates/oapi-codegen/src/emit/usage.rs b/crates/oapi-codegen/src/emit/usage.rs index 4092232..56d6693 100644 --- a/crates/oapi-codegen/src/emit/usage.rs +++ b/crates/oapi-codegen/src/emit/usage.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; use crate::emit::Targets; use crate::emit::models::ModelDerives; use crate::emit::models::SerdeDerives; +use crate::ir::Direction; use crate::ir::ForeignDerives; use crate::ir::Item; use crate::ir::Module; @@ -32,11 +33,11 @@ use crate::naming::to_ident; /// Whether a model is reachable as a request payload and/or a response payload. #[derive(Debug, Default, Clone, Copy)] -struct Usage { +pub(crate) struct Usage { /// Reachable from a request body or request-input struct. - request: bool, + pub(crate) request: bool, /// Reachable from a response body. - response: bool, + pub(crate) response: bool, } /// Compute the derive set for every generated model, keyed by its logical name. @@ -46,14 +47,7 @@ struct Usage { pub(crate) fn model_derives(module: &Module, service: &Service, targets: Targets) -> HashMap { let adjacency = adjacency(module); let foreign = foreign_derives(module, &adjacency); - let mut usage: HashMap = HashMap::new(); - - for name in request_seeds(service) { - mark(&adjacency, &name, &mut usage, Direction::Request); - } - for name in response_seeds(service) { - mark(&adjacency, &name, &mut usage, Direction::Response); - } + let usage = direction_usage(module, service); // Union of both keys. A model can be constrained by a foreign type without // being reachable from any operation, and the other way round, so taking only @@ -264,11 +258,19 @@ fn item_types(item: &Item) -> Vec { return types; } -/// The direction a seed propagates. -#[derive(Debug, Clone, Copy)] -enum Direction { - Request, - Response, +/// Which direction reaches each model, keyed by its logical name. +/// +/// An absent name is reached by no operation, which happens under `skip-prune`. +pub(crate) fn direction_usage(module: &Module, service: &Service) -> HashMap { + let adjacency = adjacency(module); + let mut usage: HashMap = HashMap::new(); + for name in request_seeds(service) { + mark(&adjacency, &name, &mut usage, Direction::Request); + } + for name in response_seeds(service) { + mark(&adjacency, &name, &mut usage, Direction::Response); + } + return usage; } /// Mark `start` and every model reachable from it with `direction`, following @@ -301,7 +303,7 @@ fn mark( /// Build the model-reference graph: each item name mapped to the names of the /// generated models it references through its fields, variants, or alias target. -fn adjacency(module: &Module) -> HashMap> { +pub(crate) fn adjacency(module: &Module) -> HashMap> { let mut graph = HashMap::with_capacity(module.items.len()); for item in &module.items { graph.insert(item.name().to_owned(), item_references(item)); diff --git a/crates/oapi-codegen/src/ir.rs b/crates/oapi-codegen/src/ir.rs index ab7941f..a3b6ad2 100644 --- a/crates/oapi-codegen/src/ir.rs +++ b/crates/oapi-codegen/src/ir.rs @@ -108,9 +108,8 @@ pub struct Field { /// One direction of an exchange. /// /// A request travels from the client to the server. A response travels back. -/// Which serde trait that needs depends on the side, so the direction is kept -/// separate from the trait: a server deserializes a request and serializes a -/// response, and a client does the opposite. +/// The direction does not name a serde trait, because a server deserializes a +/// request and serializes a response, and a client does the opposite. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum Direction { @@ -122,20 +121,18 @@ pub enum Direction { /// Which direction of an exchange carries a property. /// -/// OpenAPI marks a property `readOnly` when a response may carry it and a -/// request must not, and `writeOnly` for the opposite. The mark names a -/// direction and not a value, so one struct cannot state both. [`Access`] -/// records the mark, and [`crate::lower::direction`] builds the two shapes a -/// marked model needs. +/// `readOnly` gives [`Access::ReadOnly`] and `writeOnly` gives +/// [`Access::WriteOnly`]. [`crate::lower::direction`] then drops a property that +/// the direction of a shape does not carry. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] #[non_exhaustive] pub enum Access { /// Both directions carry the property. #[default] ReadWrite, - /// `readOnly: true`: a response carries the property, a request must not. + /// A response carries the property, and a request must not. ReadOnly, - /// `writeOnly: true`: a request carries the property, a response must not. + /// A request carries the property, and a response must not. WriteOnly, } diff --git a/crates/oapi-codegen/src/lib.rs b/crates/oapi-codegen/src/lib.rs index cd59650..3213328 100644 --- a/crates/oapi-codegen/src/lib.rs +++ b/crates/oapi-codegen/src/lib.rs @@ -101,9 +101,8 @@ fn lower_spec(spec_path: &Path, config: &Config) -> Result { .unwrap_or(crate::config::DEFAULT_RESPONSE_SUFFIX); let mut service = lower::generate_service(&spec, &config.import_mapping, response_type_suffix)?; lower::rewrite_service(&mut service, names.renames()); - // Before pruning, so a shape no operation reaches is dropped with the - // unused models, and before the name checks, so a shape's name is - // checked like any other. + // Runs before the prune pass, which drops a shape no operation reaches, + // and before the name checks, which then see the projected names. lower::split_by_direction(&mut module, Some(&mut service)); if !config.output_options.skip_prune { lower::prune_unused_models(&mut module, &service); diff --git a/crates/oapi-codegen/src/loader.rs b/crates/oapi-codegen/src/loader.rs index 7caf152..d927cf5 100644 --- a/crates/oapi-codegen/src/loader.rs +++ b/crates/oapi-codegen/src/loader.rs @@ -445,8 +445,8 @@ impl Spec { /// schemas kept it. That builds, and it carries the wrong type. /// /// A schema the direction pass splits is an error for the same reason. The - /// other run emits two names there and this one cannot tell which of the two - /// an `import-mapping` reference means. + /// other run emits two names there, and this one cannot tell which of the + /// two an `import-mapping` reference means. pub fn external_schema_name(&self, file: &str, name: &str, reference: &str) -> Result { let entry = self.component_schema(Some(file), reference, name)?; let chosen = external_name_of(&entry, name)?; @@ -488,11 +488,9 @@ impl Spec { /// /// A schema qualifies when it marks anything inside it `readOnly` or /// `writeOnly`, or when it reaches such a schema through a same-document `$ref`. -/// The reference walk runs from a marked schema up to the schemas naming it, -/// because a holder's field type is what changes per direction. /// -/// This reads the document alone. Both runs that compose a crate must reach the -/// same verdict for the same file, and only one of the two holds the operations. +/// This reads the document alone, because both runs that compose a crate must +/// reach the same verdict. Only one of the two holds the operations. fn direction_split_schemas(doc: &OpenAPI) -> std::collections::BTreeSet { let mut marked = std::collections::BTreeSet::new(); let Some(components) = doc.components.as_ref() else { diff --git a/crates/oapi-codegen/src/lower/direction.rs b/crates/oapi-codegen/src/lower/direction.rs index 7f489a8..05460c1 100644 --- a/crates/oapi-codegen/src/lower/direction.rs +++ b/crates/oapi-codegen/src/lower/direction.rs @@ -1,39 +1,27 @@ -//! Splitting a model into the request shape and the response shape that its -//! `readOnly` and `writeOnly` properties describe. +//! The request shape and the response shape that `readOnly` and `writeOnly` +//! describe. //! -//! OpenAPI marks a property `readOnly` when a response may carry it and a -//! request must not, and `writeOnly` for the opposite. The mark names a -//! direction, not a value, so one struct cannot state both: the same `Order` -//! type reaches a request body and a response body, and a serde attribute that -//! is right for one is wrong for the other. A server deserializes a request and -//! serializes a response, and a client does the reverse, so an attribute cannot -//! even be chosen per target. +//! OpenAPI marks a property `readOnly` when a response carries it and a request +//! must not, and `writeOnly` for the opposite. The mark names a direction, not a +//! value, so one struct cannot state both. //! -//! So a marked model becomes two models. `Order` with a `readOnly` `id` emits -//! `OrderRequest`, which has no `id`, and `OrderResponse`, which has one. Each -//! API position then names the shape its direction carries: a request body, a -//! parameter and a multipart part take the request shape, and a response body -//! and a response header take the response shape. +//! Each model therefore drops the properties that its direction does not carry. +//! A model that one direction reaches keeps its name. A model that both +//! directions reach becomes `Request` and `Response`, and so does +//! every model that both directions reach and that references it. //! -//! The split spreads along model references. A model holding a marked model -//! cannot keep one name either, because its field type differs per direction, -//! so `Envelope { order: Order }` becomes `EnvelopeRequest { order: OrderRequest }` -//! and `EnvelopeResponse { order: OrderResponse }`. A model that no mark reaches -//! keeps its name, so a document that uses neither keyword generates exactly -//! what it generated before. -//! -//! The split reads the marks only. It does not read how the operations use a -//! model, so the two names a schema takes stay the same when an operation is -//! added or removed. A shape that no operation reaches is then dropped by -//! [`crate::lower::prune`], the same way any unused model is. +//! `docs/design.md` states the rest of the reasoning. +use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; +use crate::emit::usage; use crate::ir::Alias; use crate::ir::Direction; use crate::ir::Enum; use crate::ir::EnumKind; +use crate::ir::Field; use crate::ir::Item; use crate::ir::Module; use crate::ir::RequestPayload; @@ -51,33 +39,44 @@ pub const REQUEST_SUFFIX: &str = "Request"; /// The suffix the response shape of a split model takes. pub const RESPONSE_SUFFIX: &str = "Response"; -/// Replace every model a direction mark reaches with its request shape and its -/// response shape, and point each API position at the shape its direction -/// carries. +/// How one direction-sensitive model is projected. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Projection { + /// Both directions reach the model, so it becomes two items with suffixed + /// names. + Split, + /// One direction reaches the model, so it keeps its name and drops only the + /// properties that direction does not carry. + Single(Direction), +} + +/// Project every model a direction mark reaches onto the direction that carries +/// it, and point each API position at the shape it takes. /// /// `service` is `None` for a models-only run, which has no operation to give a -/// direction. Both shapes are emitted there, because a models crate is consumed -/// by a run that does know the direction. +/// direction. The pass splits every marked model there, because the run that +/// consumes those models does know the direction. /// -/// A document that marks no property leaves both the module and the service -/// untouched. +/// The pass leaves a document that marks no property untouched. pub fn split_by_direction(module: &mut Module, service: Option<&mut Service>) { - let split = split_models(module); - if split.is_empty() { + let projections = projections(module, service.as_deref()); + if projections.is_empty() { return; } - let mut items = Vec::with_capacity(module.items.len() + split.len()); + let mut items = Vec::with_capacity(module.items.len() + projections.len()); for item in module.items.drain(..) { - if split.contains(item.name()) { - items.push(project_item(&item, Direction::Request, &split)); - items.push(project_item(&item, Direction::Response, &split)); - } else { - items.push(item); + match projections.get(item.name()) { + Some(Projection::Split) => { + items.push(project_item(&item, Direction::Request, &projections)); + items.push(project_item(&item, Direction::Response, &projections)); + } + Some(Projection::Single(direction)) => items.push(project_item(&item, *direction, &projections)), + None => items.push(item), } } module.items = items; if let Some(service) = service { - project_service(service, &split); + project_service(service, &projections); } } @@ -90,12 +89,37 @@ pub fn projected_name(name: &str, direction: Direction) -> RustIdent { return to_ident(&format!("{name} {suffix}"), Case::Pascal); } -/// The models that must be split: the ones marking a property, plus every model -/// that reaches one of those through a field, a variant, or an alias target. +/// How each direction-sensitive model is projected, keyed by its logical name. +fn projections(module: &Module, service: Option<&Service>) -> BTreeMap { + let sensitive = sensitive_models(module); + let Some(service) = service else { + return sensitive + .into_iter() + .map(|name| return (name, Projection::Split)) + .collect(); + }; + + let usage = usage::direction_usage(module, service); + return sensitive + .into_iter() + .map(|name| { + let used = usage.get(&name).copied().unwrap_or_default(); + let projection = match (used.request, used.response) { + (true, false) => Projection::Single(Direction::Request), + (false, true) => Projection::Single(Direction::Response), + _ => Projection::Split, + }; + return (name, projection); + }) + .collect(); +} + +/// The models a direction mark reaches: the ones that mark a property, and +/// every model that references one of those. /// -/// The walk runs up the reference graph, from a marked model to the models -/// naming it, because a holder's field type is what changes per direction. -fn split_models(module: &Module) -> BTreeSet { +/// The walk starts at a marked model and follows the reference graph in +/// reverse. The field type of a holder is what changes per direction. +fn sensitive_models(module: &Module) -> BTreeSet { let mut marked: BTreeSet = module .items .iter() @@ -107,9 +131,9 @@ fn split_models(module: &Module) -> BTreeSet { } let mut referrers: HashMap> = HashMap::new(); - for item in &module.items { - for target in item_references(item) { - referrers.entry(target).or_default().push(item.name().to_owned()); + for (name, targets) in usage::adjacency(module) { + for target in targets { + referrers.entry(target).or_default().push(name.clone()); } } @@ -137,72 +161,46 @@ fn marks_a_direction(item: &Item) -> bool { }); } -/// The names of the generated models an item references, canonicalized the way -/// [`Item::name`] spells them. -fn item_references(item: &Item) -> Vec { - let mut names = Vec::new(); - match item { - Item::Struct(strukt) => { - for field in &strukt.fields { - collect_named(&field.ty, &mut names); - } - if let Some(additional) = &strukt.additional_properties { - collect_named(additional, &mut names); - } - } - Item::Enum(enom) => { - if let EnumKind::Union(variants) = &enom.kind { - for variant in variants { - collect_named(&variant.ty, &mut names); - } - } - } - Item::Alias(alias) => collect_named(&alias.ty, &mut names), - } - return names; -} - -/// Collect the model a type expression names, looking through the wrappers. +/// Build the shape of an item for one direction. /// -/// A [`RustType::Named`] still holds the schema name the document wrote, so it -/// is run through [`to_ident`] to match the item names the graph is keyed by. -fn collect_named(ty: &RustType, out: &mut Vec) { - match ty { - RustType::Named(name) => out.push(to_ident(name, Case::Pascal).logical().to_owned()), - RustType::Vec(inner) | RustType::Map(inner) | RustType::Option(inner) | RustType::Boxed(inner) => { - collect_named(inner, out); +/// The name takes the suffix of the direction only when both directions reach +/// the model. The shape drops the properties that the other direction carries, +/// and points every reference to a split model at the shape of that model. +fn project_item(item: &Item, direction: Direction, projections: &BTreeMap) -> Item { + let split = matches!(projections.get(item.name()), Some(Projection::Split)); + let name_of = |name: &RustIdent| { + if split { + return projected_name(name.logical(), direction); } - _ => {} - } -} - -/// Build one direction's shape of an item: its name takes the direction's -/// suffix, the properties the other direction carries are dropped, and every -/// reference to a split model points at that model's shape. -fn project_item(item: &Item, direction: Direction, split: &BTreeSet) -> Item { + return name.clone(); + }; return match item { - Item::Struct(strukt) => Item::Struct(Struct { - name: projected_name(strukt.name.logical(), direction), - doc: projected_doc(&strukt.doc, strukt.name.logical(), direction), - fields: strukt + Item::Struct(strukt) => { + let fields: Vec = strukt .fields .iter() .filter(|field| return field.access.carried_by(direction)) .map(|field| { let mut projected = field.clone(); - projected.ty = project_type(&field.ty, direction, split); + projected.ty = project_type(&field.ty, direction, projections); return projected; }) - .collect(), - additional_properties: strukt - .additional_properties - .as_ref() - .map(|ty| return project_type(ty, direction, split)), - ..strukt.clone() - }), + .collect(); + let dropped = fields.len() < strukt.fields.len(); + Item::Struct(Struct { + name: name_of(&strukt.name), + doc: projected_doc(&strukt.doc, strukt.name.logical(), direction, split, dropped), + fields, + additional_properties: strukt + .additional_properties + .as_ref() + .map(|ty| return project_type(ty, direction, projections)), + ..strukt.clone() + }) + } Item::Enum(enom) => Item::Enum(Enum { - name: projected_name(enom.name.logical(), direction), - doc: projected_doc(&enom.doc, enom.name.logical(), direction), + name: name_of(&enom.name), + doc: projected_doc(&enom.doc, enom.name.logical(), direction, split, false), kind: match &enom.kind { EnumKind::Union(variants) => EnumKind::Union( variants @@ -210,7 +208,7 @@ fn project_item(item: &Item, direction: Direction, split: &BTreeSet) -> .map(|variant| { return UnionVariant { name: variant.name.clone(), - ty: project_type(&variant.ty, direction, split), + ty: project_type(&variant.ty, direction, projections), }; }) .collect(), @@ -220,23 +218,35 @@ fn project_item(item: &Item, direction: Direction, split: &BTreeSet) -> ..enom.clone() }), Item::Alias(alias) => Item::Alias(Alias { - name: projected_name(alias.name.logical(), direction), - doc: projected_doc(&alias.doc, alias.name.logical(), direction), - ty: project_type(&alias.ty, direction, split), + name: name_of(&alias.name), + doc: projected_doc(&alias.doc, alias.name.logical(), direction, split, false), + ty: project_type(&alias.ty, direction, projections), ..alias.clone() }), }; } -/// Add a line naming the direction a shape carries, after whatever the schema -/// `description` already says. +/// Add a line about the direction, after the schema `description`. /// -/// A reader meets two types where the document declares one schema, so the -/// generated file states which of the two this is and where the name came from. -fn projected_doc(doc: &Option, name: &str, direction: Direction) -> Option { - let note = match direction { - Direction::Request => format!("The request shape of `{name}`. A `readOnly` property is not part of it."), - Direction::Response => format!("The response shape of `{name}`. A `writeOnly` property is not part of it."), +/// A split shape always takes the line, because a reader meets two types where +/// the document declares one schema. A model that keeps its name takes the line +/// only when the projection drops a property. So a mark that costs a shape +/// nothing leaves the generated file as it was. +fn projected_doc(doc: &Option, name: &str, direction: Direction, split: bool, dropped: bool) -> Option { + let note = match (split, direction) { + (true, Direction::Request) => { + format!("The request shape of `{name}`. A `readOnly` property is not part of it.") + } + (true, Direction::Response) => { + format!("The response shape of `{name}`. A `writeOnly` property is not part of it.") + } + (false, _) if !dropped => return doc.clone(), + (false, Direction::Request) => { + "Only a request carries this model, so a `readOnly` property is not part of it.".to_owned() + } + (false, Direction::Response) => { + "Only a response carries this model, so a `writeOnly` property is not part of it.".to_owned() + } }; return Some(match doc { Some(text) => format!("{text}\n\n{note}"), @@ -244,70 +254,74 @@ fn projected_doc(doc: &Option, name: &str, direction: Direction) -> Opti }); } -/// Rewrite a type expression so a reference to a split model names that model's -/// shape for `direction`. Anything else is left as it is. -fn project_type(ty: &RustType, direction: Direction, split: &BTreeSet) -> RustType { +/// Rewrite a type expression so a reference to a split model names the shape of +/// that model for `direction`. This leaves any other type as it is. +fn project_type(ty: &RustType, direction: Direction, projections: &BTreeMap) -> RustType { return match ty { RustType::Named(name) => { + // A `RustType::Named` holds the schema name the document wrote, so the + // lookup canonicalizes it. Without that, a reference to `order-item` + // would miss the `OrderItem` entry and keep the unsplit name. let canonical = to_ident(name, Case::Pascal); - if split.contains(canonical.logical()) { - RustType::Named(projected_name(canonical.logical(), direction).logical().to_owned()) - } else { - ty.clone() + match projections.get(canonical.logical()) { + Some(Projection::Split) => { + RustType::Named(projected_name(canonical.logical(), direction).logical().to_owned()) + } + _ => ty.clone(), } } - RustType::Vec(inner) => RustType::Vec(Box::new(project_type(inner, direction, split))), - RustType::Map(inner) => RustType::Map(Box::new(project_type(inner, direction, split))), - RustType::Option(inner) => RustType::Option(Box::new(project_type(inner, direction, split))), - RustType::Boxed(inner) => RustType::Boxed(Box::new(project_type(inner, direction, split))), + RustType::Vec(inner) => RustType::Vec(Box::new(project_type(inner, direction, projections))), + RustType::Map(inner) => RustType::Map(Box::new(project_type(inner, direction, projections))), + RustType::Option(inner) => RustType::Option(Box::new(project_type(inner, direction, projections))), + RustType::Boxed(inner) => RustType::Boxed(Box::new(project_type(inner, direction, projections))), other => other.clone(), }; } /// Point every API position at the shape of the direction it carries, and drop /// the multipart parts a request must not send. -fn project_service(service: &mut Service, split: &BTreeSet) { +fn project_service(service: &mut Service, projections: &BTreeMap) { for operation in &mut service.operations { for param in &mut operation.path_params { - param.ty = project_type(¶m.ty, Direction::Request, split); + param.ty = project_type(¶m.ty, Direction::Request, projections); } if let Some(query) = &mut operation.query { - project_struct(query, Direction::Request, split); + project_struct(query, Direction::Request, projections); } if let Some(headers) = &mut operation.headers { for param in &mut headers.params { - param.ty = project_type(¶m.ty, Direction::Request, split); + param.ty = project_type(¶m.ty, Direction::Request, projections); } } if let Some(cookies) = &mut operation.cookies { for param in &mut cookies.params { - param.ty = project_type(¶m.ty, Direction::Request, split); + param.ty = project_type(¶m.ty, Direction::Request, projections); } } if let Some(request) = &mut operation.request { match request { - RequestPayload::Single(body) => body.ty = project_type(&body.ty, Direction::Request, split), + RequestPayload::Single(body) => body.ty = project_type(&body.ty, Direction::Request, projections), RequestPayload::Multipart(multipart) => { for field in &mut multipart.fields { - field.ty = project_type(&field.ty, Direction::Request, split); + field.ty = project_type(&field.ty, Direction::Request, projections); } } RequestPayload::Negotiated(negotiated) => { for variant in &mut negotiated.variants { - variant.body.ty = project_type(&variant.body.ty, Direction::Request, split); + variant.body.ty = project_type(&variant.body.ty, Direction::Request, projections); } } } } for case in &mut operation.responses { for header in &mut case.headers { - header.ty = project_type(&header.ty, Direction::Response, split); + header.ty = project_type(&header.ty, Direction::Response, projections); } match &mut case.body { - Some(ResponseBody::Single(body)) => body.ty = project_type(&body.ty, Direction::Response, split), + Some(ResponseBody::Single(body)) => body.ty = project_type(&body.ty, Direction::Response, projections), Some(ResponseBody::Negotiated(negotiated)) => { for variant in &mut negotiated.variants { - variant.body.ty = project_type(&variant.body.ty, Direction::Response, split); + variant.body.ty = project_type(&variant.body.ty, Direction::Response, projections); } } None => {} @@ -317,12 +331,12 @@ fn project_service(service: &mut Service, split: &BTreeSet) { } /// Rewrite the type of every field of a per-operation struct. -fn project_struct(strukt: &mut Struct, direction: Direction, split: &BTreeSet) { +fn project_struct(strukt: &mut Struct, direction: Direction, projections: &BTreeMap) { for field in &mut strukt.fields { - field.ty = project_type(&field.ty, direction, split); + field.ty = project_type(&field.ty, direction, projections); } if let Some(additional) = &mut strukt.additional_properties { - *additional = project_type(additional, direction, split); + *additional = project_type(additional, direction, projections); } } @@ -335,8 +349,8 @@ mod tests { use crate::ir::Field; use crate::loader::Spec; - /// Lower an inline document's schemas and split them, which is the pipeline - /// a models-only run does. + /// Lower the schemas of an inline document and split them, the way a + /// models-only run does. fn split_yaml(yaml: &str) -> Module { let doc: openapiv3::OpenAPI = serde_yaml::from_str(yaml).expect("parse spec"); let spec = Spec::from_parts(doc, PathBuf::from("inline.yaml")); @@ -346,6 +360,42 @@ mod tests { return module; } + /// Lower an inline document with its operations and split them, the way a + /// server or client run does. + fn split_service_yaml(yaml: &str) -> Module { + let doc: openapiv3::OpenAPI = serde_yaml::from_str(yaml).expect("parse spec"); + let spec = Spec::from_parts(doc, PathBuf::from("inline.yaml")); + let names = crate::lower::rename::type_renames(&spec, None).expect("resolve names"); + let mut module = crate::lower::generate_models(&spec, &names).expect("lower schemas"); + let mut service = + crate::lower::generate_service(&spec, &Default::default(), "Response").expect("lower operations"); + crate::lower::rewrite_service(&mut service, names.renames()); + split_by_direction(&mut module, Some(&mut service)); + return module; + } + + /// The doc text of the item called `name`. + fn doc(module: &Module, name: &str) -> Option { + for item in &module.items { + if item.name() == name { + return match item { + Item::Struct(strukt) => strukt.doc.clone(), + Item::Enum(enom) => enom.doc.clone(), + Item::Alias(alias) => alias.doc.clone(), + }; + } + } + panic!("no item named `{name}`"); + } + + /// A document whose only operation returns `Account`, so no request reaches + /// it. + fn response_only(schemas: &str) -> String { + return format!( + "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n get:\n operationId: listAccounts\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\ncomponents:\n schemas:\n{schemas}" + ); + } + /// The item names a module declares, in emission order. fn names(module: &Module) -> Vec { return module.items.iter().map(|item| return item.name().to_owned()).collect(); @@ -369,6 +419,40 @@ mod tests { const PREAMBLE: &str = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths: {}\ncomponents:\n schemas:\n"; + #[test] + fn one_direction_keeps_the_name_and_drops_nothing_it_carries() { + let module = split_service_yaml(&response_only( + " Account:\n description: An account.\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n", + )); + assert_eq!(names(&module), vec!["Account"]); + assert_eq!(fields(&module, "Account"), vec!["id", "email"]); + assert_eq!(doc(&module, "Account"), Some("An account.".to_owned())); + } + + #[test] + fn one_direction_keeps_the_name_and_drops_what_it_cannot_carry() { + let module = split_service_yaml(&response_only( + " Account:\n type: object\n properties:\n email:\n type: string\n secret:\n type: string\n writeOnly: true\n", + )); + assert_eq!(names(&module), vec!["Account"]); + assert_eq!(fields(&module, "Account"), vec!["email"]); + assert_eq!( + doc(&module, "Account"), + Some("Only a response carries this model, so a `writeOnly` property is not part of it.".to_owned()) + ); + } + + #[test] + fn a_holder_that_one_direction_reaches_keeps_its_name_and_names_the_split_shape() { + let yaml = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n post:\n operationId: createAccount\n requestBody:\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Page'\ncomponents:\n schemas:\n Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n Page:\n type: object\n properties:\n items:\n type: array\n items:\n $ref: '#/components/schemas/Account'\n"; + let module = split_service_yaml(yaml); + assert_eq!(names(&module), vec!["AccountRequest", "AccountResponse", "Page"]); + let Some(Item::Struct(page)) = module.items.iter().find(|item| return item.name() == "Page") else { + panic!("no struct named `Page`"); + }; + assert_eq!(page.fields[0].ty.label(), "Option>"); + } + #[test] fn a_document_with_no_mark_is_left_alone() { let module = split_yaml(&format!( diff --git a/crates/oapi-codegen/src/lower/paths.rs b/crates/oapi-codegen/src/lower/paths.rs index a0199fe..d28b96d 100644 --- a/crates/oapi-codegen/src/lower/paths.rs +++ b/crates/oapi-codegen/src/lower/paths.rs @@ -441,8 +441,8 @@ impl Lowerer<'_> { serde_skip: false, default, constraints, - // A parameter only ever travels in a request, so a direction mark on - // its schema states nothing the generator can act on. + // A parameter travels in a request only, so a direction mark on its + // schema states nothing the generator can act on. access: crate::ir::Access::ReadWrite, }; crate::lower::constraints::check_constraints(&field)?; @@ -1565,9 +1565,9 @@ impl Lowerer<'_> { /// Whether a `multipart/form-data` part travels in a request. /// -/// A multipart body is a request body only, so a `readOnly` part is left out of -/// the generated extractor. `writeOnly` states that a request carries the part, -/// which is what a multipart part already does. +/// A multipart body is a request body only, so the generator leaves a `readOnly` +/// part out of the extractor. `writeOnly` states what a multipart part already +/// does. fn multipart_part_is_sent(data: &SchemaData, path: &str, method: &str, wire_name: &str) -> Result { let at = format!("{method} {path} multipart field `{wire_name}`"); let access = crate::lower::schema::access_of(data, &at)?; diff --git a/crates/oapi-codegen/src/lower/schema.rs b/crates/oapi-codegen/src/lower/schema.rs index 141b1f5..3156b50 100644 --- a/crates/oapi-codegen/src/lower/schema.rs +++ b/crates/oapi-codegen/src/lower/schema.rs @@ -332,9 +332,8 @@ impl Mapper<'_> { let rename = crate::naming::rename_for(wire, &ident); let access = match prop { ReferenceOr::Item(schema) => access_of(&schema.schema_data, &at)?, - // A `$ref` property carries no sibling keyword in OpenAPI 3.0, so the - // mark can only sit on the target. The checks below read the target - // the same way. + // A `$ref` property carries no sibling keyword in OpenAPI 3.0, so + // the mark can only sit on the target. ReferenceOr::Reference { reference } => match self.spec.resolve(reference) { Ok(target) => access_of(&target.schema_data, &at)?, Err(_) => Access::ReadWrite, @@ -1078,9 +1077,8 @@ fn verbatim_type(data: &SchemaData, verbatim: &str, path: &str) -> Result Result { return match (data.read_only, data.write_only) { (true, true) => Err(Error::UnsupportedSchema { diff --git a/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml b/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml index b4cb144..1b09f14 100644 --- a/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml +++ b/crates/oapi-codegen/tests/fixtures/combined_read_write_only.yaml @@ -30,6 +30,32 @@ paths: application/json: schema: $ref: "#/components/schemas/AccountPage" + /accounts/search: + post: + operationId: searchAccounts + summary: Search accounts. + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/AccountFilter" + responses: + "204": + description: The search was accepted. + /audit: + get: + operationId: listAudit + summary: List the audit log. + responses: + "200": + description: The audit entries. + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/AuditEntry" /accounts/{accountId}/avatar: put: operationId: uploadAvatar @@ -75,6 +101,29 @@ components: type: array items: $ref: "#/components/schemas/Account" + AccountFilter: + description: A request-only model that marks a property `readOnly`. + type: object + required: [email] + properties: + email: + type: string + matched: + description: Only a response could carry this, so the request drops it. + type: integer + format: int32 + readOnly: true + AuditEntry: + description: A response-only model that marks a property `writeOnly`. + type: object + required: [action] + properties: + action: + type: string + token: + description: Only a request could carry this, so the response drops it. + type: string + writeOnly: true Avatar: description: A multipart body, which only a request carries. type: object diff --git a/crates/oapi-codegen/tests/generated.rs b/crates/oapi-codegen/tests/generated.rs index a10e20b..84dd363 100644 --- a/crates/oapi-codegen/tests/generated.rs +++ b/crates/oapi-codegen/tests/generated.rs @@ -309,6 +309,36 @@ fn a_holder_of_a_split_model_names_the_shape_of_its_direction() { assert_eq!(list.len(), 1); } +#[test] +fn a_model_that_one_direction_reaches_keeps_its_name() { + use generated::combined_read_write_only; + + // `AccountPage` is response-only, so it keeps its name and only its field + // type follows the split of `Account`. + let page = combined_read_write_only::AccountPage { + items: vec![combined_read_write_only::AccountResponse { + id: "3a1f9b0e-1c1a-4d0f-8b6f-2f9a4c5d6e70".to_owned(), + email: "a@example.com".to_owned(), + }], + }; + assert_eq!(page.items.len(), 1); + + // `AuditEntry` is response-only and marks `token` `writeOnly`, so it keeps + // its name and drops that property. + let entry: combined_read_write_only::AuditEntry = + serde_json::from_str(r#"{"action":"login","token":"t"}"#).expect("deserialize"); + let json = serde_json::to_string(&entry).expect("serialize"); + assert_eq!(json, r#"{"action":"login"}"#); + + // `AccountFilter` is request-only and marks `matched` `readOnly`, so it + // keeps its name and drops that property. + let filter = combined_read_write_only::AccountFilter { + email: "a@example.com".to_owned(), + }; + let json = serde_json::to_string(&filter).expect("serialize"); + assert_eq!(json, r#"{"email":"a@example.com"}"#); +} + #[test] fn a_read_only_multipart_part_is_left_out_of_the_extractor() { use generated::combined_read_write_only; diff --git a/crates/oapi-codegen/tests/generated/combined_read_write_only.rs b/crates/oapi-codegen/tests/generated/combined_read_write_only.rs index 1ab6229..7eb09c8 100644 --- a/crates/oapi-codegen/tests/generated/combined_read_write_only.rs +++ b/crates/oapi-codegen/tests/generated/combined_read_write_only.rs @@ -30,18 +30,32 @@ pub struct AccountResponse { } /// A response-only holder of a split model. -/// -/// The response shape of `AccountPage`. A `writeOnly` property is not part of it. #[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] -pub struct AccountPageResponse { +pub struct AccountPage { pub items: Vec, } +/// A request-only model that marks a property `readOnly`. +/// +/// Only a request carries this model, so a `readOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AccountFilter { + pub email: String, +} + +/// A response-only model that marks a property `writeOnly`. +/// +/// Only a response carries this model, so a `writeOnly` property is not part of it. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] +pub struct AuditEntry { + pub action: String, +} + /// List every account. #[derive(Debug, Clone, PartialEq)] pub enum ListAccountsResponse { /// The accounts, wrapped in a page. - Ok(AccountPageResponse), + Ok(AccountPage), } /// Create an account. @@ -51,6 +65,20 @@ pub enum CreateAccountResponse { Created(AccountResponse), } +/// Search accounts. +#[derive(Debug, Clone, PartialEq)] +pub enum SearchAccountsResponse { + /// The search was accepted. + NoContent, +} + +/// List the audit log. +#[derive(Debug, Clone, PartialEq)] +pub enum ListAuditResponse { + /// The audit entries. + Ok(Vec), +} + #[derive(Debug, Clone)] pub struct UploadAvatarMultipart { pub image: Vec, @@ -125,6 +153,13 @@ pub trait Api: Clone + Send + Sync + 'static { &self, body: AccountRequest, ) -> impl std::future::Future + Send; + /// Search accounts. + fn search_accounts( + &self, + body: AccountFilter, + ) -> impl std::future::Future + Send; + /// List the audit log. + fn list_audit(&self) -> impl std::future::Future + Send; /// Replace an account avatar. fn upload_avatar( &self, @@ -165,6 +200,38 @@ impl axum::response::IntoResponse for CreateAccountResponse { } } +impl axum::response::IntoResponse for SearchAccountsResponse { + fn into_response(self) -> axum::response::Response { + match self { + SearchAccountsResponse::NoContent => { + const STATUS: axum::http::StatusCode = match axum::http::StatusCode::from_u16( + 204, + ) { + Ok(status) => status, + Err(_) => panic!("oapi-codegen emitted an invalid HTTP status code"), + }; + STATUS.into_response() + } + } + } +} + +impl axum::response::IntoResponse for ListAuditResponse { + fn into_response(self) -> axum::response::Response { + match self { + ListAuditResponse::Ok(body) => { + const STATUS: axum::http::StatusCode = match axum::http::StatusCode::from_u16( + 200, + ) { + Ok(status) => status, + Err(_) => panic!("oapi-codegen emitted an invalid HTTP status code"), + }; + (STATUS, axum::Json(body)).into_response() + } + } + } +} + impl axum::response::IntoResponse for UploadAvatarResponse { fn into_response(self) -> axum::response::Response { match self { @@ -189,6 +256,8 @@ pub fn router(api: T) -> axum::Router { axum::routing::get(list_accounts_handler::) .post(create_account_handler::), ) + .route("/accounts/search", axum::routing::post(search_accounts_handler::)) + .route("/audit", axum::routing::get(list_audit_handler::)) .route( "/accounts/{accountId}/avatar", axum::routing::put(upload_avatar_handler::), @@ -209,6 +278,19 @@ async fn create_account_handler( api.create_account(body).await } +async fn search_accounts_handler( + axum::extract::State(api): axum::extract::State, + axum::Json(body): axum::Json, +) -> SearchAccountsResponse { + api.search_accounts(body).await +} + +async fn list_audit_handler( + axum::extract::State(api): axum::extract::State, +) -> ListAuditResponse { + api.list_audit().await +} + async fn upload_avatar_handler( axum::extract::State(api): axum::extract::State, axum::extract::Path(account_id): axum::extract::Path, @@ -308,7 +390,7 @@ impl Client { let response = self.http.request(reqwest::Method::GET, url).send()?; let status = response.status(); if status.as_u16() == 200 { - let body: AccountPageResponse = response.json()?; + let body: AccountPage = response.json()?; return Ok(ListAccountsResponse::Ok(body)); } return Err(ClientError::UnexpectedStatus(status)); @@ -329,6 +411,32 @@ impl Client { } return Err(ClientError::UnexpectedStatus(status)); } + /// Search accounts. + pub fn search_accounts( + &self, + body: AccountFilter, + ) -> Result { + let url = format!("{}/accounts/search", self.base_url); + let mut request = self.http.request(reqwest::Method::POST, url); + request = request.json(&body); + let response = request.send()?; + let status = response.status(); + if status.as_u16() == 204 { + return Ok(SearchAccountsResponse::NoContent); + } + return Err(ClientError::UnexpectedStatus(status)); + } + /// List the audit log. + pub fn list_audit(&self) -> Result { + let url = format!("{}/audit", self.base_url); + let response = self.http.request(reqwest::Method::GET, url).send()?; + let status = response.status(); + if status.as_u16() == 200 { + let body: Vec = response.json()?; + return Ok(ListAuditResponse::Ok(body)); + } + return Err(ClientError::UnexpectedStatus(status)); + } /// Replace an account avatar. pub fn upload_avatar( &self, diff --git a/docs/design.md b/docs/design.md index 797ddea..abb86bc 100644 --- a/docs/design.md +++ b/docs/design.md @@ -512,33 +512,43 @@ does the same. `minProperties` on a schema that names its properties, and names the way out, because a rule the document states and the code drops is worse than no rule at all. -## A direction mark splits a model in two +## A direction mark projects a model onto its direction `readOnly` and `writeOnly` mark a direction, not a value. A `readOnly` property travels in a response, and a request must not send it. A `writeOnly` property travels in a request, and a response must not send it. A property that sets both marks is an error, because no direction is left to carry it. -One struct cannot hold both statements. The same `Order` type reaches a request -body and a response body, so a serde attribute that is right for one is wrong for -the other. The side does not settle it either. A server reads a request and -writes a response, and a client does the reverse. - -So a marked model becomes two models. `Order` with a `readOnly` `id` gives -`OrderRequest`, which has no `id`, and `OrderResponse`, which has one. A request -body, a parameter, and a multipart part take the request shape. A response body -and a response header take the response shape. - -The split spreads along model references. A model that holds a marked model -cannot keep one name either, because its field type differs per direction. So -`Envelope { order: Order }` gives `EnvelopeRequest` with an `OrderRequest` field -and `EnvelopeResponse` with an `OrderResponse` field. A model that no mark -reaches keeps its name, and a document that uses neither keyword generates what -it generated before. - -The split reads the marks alone. It does not read how the operations use a model, -so the two names stay the same when an operation is added or removed. A shape -that no operation reaches is then dropped with the other unused models. +A serde attribute cannot state this. The same `Order` type can reach a request +body and a response body, so an attribute that is right for one is wrong for the +other. The side does not settle it either. A server reads a request and writes a +response, and a client does the reverse. + +So each model drops the properties that its direction does not carry. When one +direction reaches a model, the model keeps its name. An `Order` that only a +response carries keeps the name `Order` and drops each `writeOnly` property. A +`readOnly` property costs that model nothing, so a read-only API generates what +it generated before the marks were there. + +When both directions reach a model, one name is not enough. `Order` then becomes +`OrderRequest`, which has no `readOnly` property, and `OrderResponse`, which has +no `writeOnly` property. A parameter, a request body, and a multipart part take +the request shape. A response body and a response header take the response +shape. + +The split spreads along model references. A model that holds a split model and +that both directions reach cannot keep one name either, because its field type +differs per direction. A holder that one direction reaches keeps its name, and +only its field type follows the split. So a response-only `Page` keeps the name +`Page` and holds an `OrderResponse`. + +A models-only run splits every marked model, because it has no operation to give +a direction. The run that consumes those models does know the direction and +picks the shape it needs. + +A new operation can rename a model. A `POST` that sends an `Order` gives that +model a second direction, so the one name becomes two. The model gains a second +shape at that point, so the two names report what the document now states. The two names go through the same checks as any other type name. A schema named `GetWidget` gives a `GetWidgetResponse` shape, which the response enum of a @@ -550,9 +560,9 @@ property does not declare it at all, so the requirement applies only where the property exists. This is what the OpenAPI specification states. An `import-mapping` reference to a marked schema is an error. Two runs make a -composed crate, and only one of them holds the operations. The other run emits -two names for that schema, and a reference by name alone cannot say which of the -two it means. +composed crate, and only one of them holds the operations. The other run is a +models-only run, so it emits two names for that schema. A reference by name +alone cannot say which of the two it means. ## A `default` removes the `Option` From 17024b3967c8f8b474578aa88c128b0db0f5fbf8 Mon Sep 17 00:00:00 2001 From: Kaspar Lyngsie Date: Sun, 23 Aug 2026 19:15:02 +0200 Subject: [PATCH 3/4] fix: pr comments --- crates/oapi-codegen/src/loader.rs | 109 +++++++++++++++--- crates/oapi-codegen/src/lower/direction.rs | 60 +++++++--- crates/oapi-codegen/tests/coverage.rs | 9 +- .../fixtures/schemas/compose_marked.yaml | 4 +- .../tests/generated/read_write_only.rs | 8 +- 5 files changed, 157 insertions(+), 33 deletions(-) diff --git a/crates/oapi-codegen/src/loader.rs b/crates/oapi-codegen/src/loader.rs index d927cf5..97ba460 100644 --- a/crates/oapi-codegen/src/loader.rs +++ b/crates/oapi-codegen/src/loader.rs @@ -452,12 +452,14 @@ impl Spec { let chosen = external_name_of(&entry, name)?; let doc = self.document_for(file)?; let schemas = doc.components.as_ref().map(|components| return &components.schemas); + let ident = crate::naming::to_ident(&chosen, crate::naming::Case::Pascal); if direction_split_schemas(&doc).contains(name) { + let shape = ident.logical(); return Err(Error::UnsupportedRef { reference: reference.to_owned(), reason: format!( "`{name}` in `{file}` marks a property `readOnly` or `writeOnly`, so the run that \ - generates that file emits it as `{name}{REQUEST_SUFFIX}` and `{name}{RESPONSE_SUFFIX}`. \ + generates that file emits it as `{shape}{REQUEST_SUFFIX}` and `{shape}{RESPONSE_SUFFIX}`. \ This run cannot tell which of the two an `import-mapping` reference means. Declare \ the schema in this document instead, or drop the mark" ), @@ -468,7 +470,6 @@ impl Spec { continue; } let taken = external_name_of(other_entry, other)?; - let ident = crate::naming::to_ident(&chosen, crate::naming::Case::Pascal); if crate::naming::to_ident(&taken, crate::naming::Case::Pascal).logical() == ident.logical() { return Err(Error::UnsupportedRef { reference: reference.to_owned(), @@ -486,8 +487,12 @@ impl Spec { /// The schemas in `doc` that the direction pass splits into a request shape and /// a response shape. /// -/// A schema qualifies when it marks anything inside it `readOnly` or -/// `writeOnly`, or when it reaches such a schema through a same-document `$ref`. +/// A schema qualifies when a property inside it carries a direction mark, or +/// when it reaches such a schema through a same-document `$ref`. +/// +/// A mark on a schema itself does not qualify that schema. The direction pass +/// splits a model only when one of its properties is directional, so a marked +/// primitive keeps its one name and only its referrers split. /// /// This reads the document alone, because both runs that compose a crate must /// reach the same verdict. Only one of the two holds the operations. @@ -496,12 +501,22 @@ fn direction_split_schemas(doc: &OpenAPI) -> std::collections::BTreeSet let Some(components) = doc.components.as_ref() else { return marked; }; + + let mut directional = std::collections::BTreeSet::new(); + for (name, entry) in &components.schemas { + if let ReferenceOr::Item(schema) = entry + && (schema.schema_data.read_only || schema.schema_data.write_only) + { + directional.insert(name.clone()); + } + } + let mut referrers: HashMap> = HashMap::new(); for (name, entry) in &components.schemas { let ReferenceOr::Item(schema) = entry else { continue; }; - if schema_marks_a_direction(schema) { + if schema_marks_a_direction(schema, &directional) { marked.insert(name.clone()); } let mut targets = Vec::new(); @@ -525,19 +540,55 @@ fn direction_split_schemas(doc: &OpenAPI) -> std::collections::BTreeSet return marked; } -/// Whether a schema, or any schema written inside it, sets `readOnly` or -/// `writeOnly`. -fn schema_marks_a_direction(schema: &Schema) -> bool { - if schema.schema_data.read_only || schema.schema_data.write_only { - return true; - } - let mut found = false; +/// Whether a property of `schema`, or of a schema written inside it, carries a +/// direction mark. +/// +/// `directional` holds the component schemas that set `readOnly` or `writeOnly` +/// on themselves. A property that names one of those carries the mark too. +fn schema_marks_a_direction(schema: &Schema, directional: &std::collections::BTreeSet) -> bool { + let mut found = declares_a_directional_property(schema, directional); walk_inline_schemas(schema, &mut |inner| { - found = found || inner.schema_data.read_only || inner.schema_data.write_only; + found = found || declares_a_directional_property(inner, directional); }); return found; } +/// Whether one schema declares a property that only one direction carries. +fn declares_a_directional_property(schema: &Schema, directional: &std::collections::BTreeSet) -> bool { + return object_properties(schema).iter().any(|entry| { + return match entry { + ReferenceOr::Item(inner) => inner.schema_data.read_only || inner.schema_data.write_only, + ReferenceOr::Reference { reference } => { + return ref_file_part(reference).is_none() + && ref_component_name(reference, "schemas").is_some_and(|name| return directional.contains(name)); + } + }; + }); +} + +/// The schemas one schema declares as properties. +fn object_properties(schema: &Schema) -> Vec> { + let unbox = |entry: &ReferenceOr>| { + return match entry { + ReferenceOr::Item(inner) => ReferenceOr::Item((**inner).clone()), + ReferenceOr::Reference { reference } => ReferenceOr::Reference { + reference: reference.clone(), + }, + }; + }; + return match &schema.schema_kind { + openapiv3::SchemaKind::Type(openapiv3::Type::Object(object)) => { + return object.properties.values().map(&unbox).collect(); + } + openapiv3::SchemaKind::Any(any) => return any.properties.values().map(&unbox).collect(), + openapiv3::SchemaKind::Type(_) + | openapiv3::SchemaKind::OneOf { .. } + | openapiv3::SchemaKind::AllOf { .. } + | openapiv3::SchemaKind::AnyOf { .. } + | openapiv3::SchemaKind::Not { .. } => Vec::new(), + }; +} + /// The same-document component schemas a schema names, at any depth. fn schema_local_refs(schema: &Schema, out: &mut Vec) { let mut collect = |entry: &ReferenceOr| { @@ -997,4 +1048,36 @@ mod tests { openapiv3::SchemaKind::Type(openapiv3::Type::Integer(_)) )); } + + /// A mark on a schema itself splits the schemas that name it, not the schema. + /// + /// The direction pass splits a model only when one of its properties is + /// directional, so a marked primitive stays one type alias. Reporting it as + /// split rejects an `import-mapping` reference that resolves. + #[test] + fn a_schema_level_mark_splits_only_the_referrers() { + let doc: OpenAPI = serde_yaml::from_str( + "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths: {}\ncomponents:\n schemas:\n Marked:\n type: string\n readOnly: true\n Holder:\n type: object\n properties:\n a:\n $ref: '#/components/schemas/Marked'\n Bystander:\n type: object\n properties:\n b:\n type: string\n", + ) + .expect("parse doc"); + + let split = direction_split_schemas(&doc); + assert!(split.contains("Holder"), "a property that names a marked schema splits"); + assert!(!split.contains("Marked"), "a marked primitive keeps its one name"); + assert!(!split.contains("Bystander"), "an unrelated schema keeps its one name"); + } + + /// A mark on a property splits the model that declares it, and every model + /// that reaches it. + #[test] + fn a_property_mark_splits_the_declaring_model_and_its_referrers() { + let doc: OpenAPI = serde_yaml::from_str( + "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths: {}\ncomponents:\n schemas:\n Leaf:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n Parent:\n type: object\n properties:\n leaf:\n $ref: '#/components/schemas/Leaf'\n", + ) + .expect("parse doc"); + + let split = direction_split_schemas(&doc); + assert!(split.contains("Leaf"), "the model that declares the mark splits"); + assert!(split.contains("Parent"), "a model that reaches the mark splits"); + } } diff --git a/crates/oapi-codegen/src/lower/direction.rs b/crates/oapi-codegen/src/lower/direction.rs index 05460c1..ebc38ae 100644 --- a/crates/oapi-codegen/src/lower/direction.rs +++ b/crates/oapi-codegen/src/lower/direction.rs @@ -228,23 +228,29 @@ fn project_item(item: &Item, direction: Direction, projections: &BTreeMap, name: &str, direction: Direction, split: bool, dropped: bool) -> Option { - let note = match (split, direction) { - (true, Direction::Request) => { + let note = match (split, dropped, direction) { + (true, true, Direction::Request) => { format!("The request shape of `{name}`. A `readOnly` property is not part of it.") } - (true, Direction::Response) => { + (true, true, Direction::Response) => { format!("The response shape of `{name}`. A `writeOnly` property is not part of it.") } - (false, _) if !dropped => return doc.clone(), - (false, Direction::Request) => { + (true, false, Direction::Request) => format!("The request shape of `{name}`."), + (true, false, Direction::Response) => format!("The response shape of `{name}`."), + (false, false, _) => return doc.clone(), + (false, true, Direction::Request) => { "Only a request carries this model, so a `readOnly` property is not part of it.".to_owned() } - (false, Direction::Response) => { + (false, true, Direction::Response) => { "Only a response carries this model, so a `writeOnly` property is not part of it.".to_owned() } }; @@ -278,8 +284,7 @@ fn project_type(ty: &RustType, direction: Direction, projections: &BTreeMap) { for operation in &mut service.operations { for param in &mut operation.path_params { @@ -442,9 +447,36 @@ mod tests { ); } + /// A split shape names the keyword only when it drops a property. + /// + /// A holder splits because it references a split model. It drops nothing, so + /// a line about a `readOnly` property would state a fault that is not there. + #[test] + fn a_split_shape_that_drops_nothing_states_the_direction_alone() { + let module = split_yaml(&format!( + "{PREAMBLE} Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n Envelope:\n type: object\n properties:\n account:\n $ref: '#/components/schemas/Account'\n" + )); + assert_eq!( + doc(&module, "AccountRequest"), + Some("The request shape of `Account`. A `readOnly` property is not part of it.".to_owned()) + ); + assert_eq!( + doc(&module, "AccountResponse"), + Some("The response shape of `Account`.".to_owned()), + "the response drops nothing, because no property is `writeOnly`", + ); + assert_eq!( + doc(&module, "EnvelopeRequest"), + Some("The request shape of `Envelope`.".to_owned()) + ); + assert_eq!( + doc(&module, "EnvelopeResponse"), + Some("The response shape of `Envelope`.".to_owned()) + ); + } + #[test] - fn a_holder_that_one_direction_reaches_keeps_its_name_and_names_the_split_shape() { - let yaml = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n post:\n operationId: createAccount\n requestBody:\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Page'\ncomponents:\n schemas:\n Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n Page:\n type: object\n properties:\n items:\n type: array\n items:\n $ref: '#/components/schemas/Account'\n"; + fn a_holder_that_one_direction_reaches_keeps_its_name_and_names_the_split_shape() { let yaml = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n post:\n operationId: createAccount\n requestBody:\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Page'\ncomponents:\n schemas:\n Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n Page:\n type: object\n properties:\n items:\n type: array\n items:\n $ref: '#/components/schemas/Account'\n"; let module = split_service_yaml(yaml); assert_eq!(names(&module), vec!["AccountRequest", "AccountResponse", "Page"]); let Some(Item::Struct(page)) = module.items.iter().find(|item| return item.name() == "Page") else { diff --git a/crates/oapi-codegen/tests/coverage.rs b/crates/oapi-codegen/tests/coverage.rs index 205391e..7017487 100644 --- a/crates/oapi-codegen/tests/coverage.rs +++ b/crates/oapi-codegen/tests/coverage.rs @@ -922,6 +922,9 @@ fn a_property_that_sets_both_direction_marks_names_both_in_the_message() { /// The run that writes the operations resolves the reference by name alone, so /// it cannot tell which shape the author meant. Emitting the plain name compiles /// nothing, because the models crate declares neither. +/// +/// The target carries an `x-rust-name`, so the message must name the shapes that +/// the chosen name builds, not the shapes that the schema name builds. #[test] fn a_cross_file_reference_to_a_split_schema_is_rejected() { let fixture = tests_dir() @@ -930,9 +933,13 @@ fn a_cross_file_reference_to_a_split_schema_is_rejected() { let error = oapi_codegen::generate(&fixture, &server_config()).expect_err("a split target must be rejected"); let message = format!("{error}"); assert!( - message.contains("ParcelRequest") && message.contains("ParcelResponse"), + message.contains("ShipmentRequest") && message.contains("ShipmentResponse"), "the message must name both shapes: {message}", ); + assert!( + !message.contains("ParcelRequest") && !message.contains("ParcelResponse"), + "the message must not name a shape the models run never emits: {message}", + ); } /// Unused component schemas are pruned by default, but retained when diff --git a/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml b/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml index 188fc2f..5a89035 100644 --- a/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml +++ b/crates/oapi-codegen/tests/fixtures/schemas/compose_marked.yaml @@ -6,13 +6,15 @@ paths: {} components: schemas: # `Parcel` marks a property `readOnly`, so the run that writes the models - # emits `ParcelRequest` and `ParcelResponse` and no `Parcel`. + # emits `ShipmentRequest` and `ShipmentResponse` and no `Shipment`. The + # `x-rust-name` key sets the base name that the two suffixes extend. # # The run that writes the operations resolves an `import-mapping` reference # by name alone. It cannot tell which of the two shapes the reference means, # so it rejects the reference instead of emitting a name the models crate # never declares. Parcel: + x-rust-name: Shipment type: object required: [id, weight_kg] properties: diff --git a/crates/oapi-codegen/tests/generated/read_write_only.rs b/crates/oapi-codegen/tests/generated/read_write_only.rs index df484d6..f1bba31 100644 --- a/crates/oapi-codegen/tests/generated/read_write_only.rs +++ b/crates/oapi-codegen/tests/generated/read_write_only.rs @@ -35,7 +35,7 @@ pub struct AccountResponse { /// A holder splits too, because its field type differs per direction. /// -/// The request shape of `Envelope`. A `readOnly` property is not part of it. +/// The request shape of `Envelope`. #[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] pub struct EnvelopeRequest { pub account: AccountRequest, @@ -45,7 +45,7 @@ pub struct EnvelopeRequest { /// A holder splits too, because its field type differs per direction. /// -/// The response shape of `Envelope`. A `writeOnly` property is not part of it. +/// The response shape of `Envelope`. #[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)] pub struct EnvelopeResponse { pub account: AccountResponse, @@ -55,12 +55,12 @@ pub struct EnvelopeResponse { /// An alias to a split model splits as well. /// -/// The request shape of `AccountList`. A `readOnly` property is not part of it. +/// The request shape of `AccountList`. pub type AccountListRequest = Vec; /// An alias to a split model splits as well. /// -/// The response shape of `AccountList`. A `writeOnly` property is not part of it. +/// The response shape of `AccountList`. pub type AccountListResponse = Vec; /// No mark reaches this schema, so it keeps its name. From a1e1ec7267ae007a2538269930b238849424108c Mon Sep 17 00:00:00 2001 From: Kaspar Lyngsie Date: Sun, 23 Aug 2026 20:13:12 +0200 Subject: [PATCH 4/4] fix: fixing high finding --- crates/oapi-codegen/src/loader.rs | 50 +++++++++++++++++----- crates/oapi-codegen/src/lower/direction.rs | 3 +- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/crates/oapi-codegen/src/loader.rs b/crates/oapi-codegen/src/loader.rs index 97ba460..c063827 100644 --- a/crates/oapi-codegen/src/loader.rs +++ b/crates/oapi-codegen/src/loader.rs @@ -513,16 +513,24 @@ fn direction_split_schemas(doc: &OpenAPI) -> std::collections::BTreeSet let mut referrers: HashMap> = HashMap::new(); for (name, entry) in &components.schemas { - let ReferenceOr::Item(schema) = entry else { - continue; - }; - if schema_marks_a_direction(schema, &directional) { - marked.insert(name.clone()); - } - let mut targets = Vec::new(); - schema_local_refs(schema, &mut targets); - for target in targets { - referrers.entry(target).or_default().push(name.clone()); + match entry { + ReferenceOr::Item(schema) => { + if schema_marks_a_direction(schema, &directional) { + marked.insert(name.clone()); + } + let mut targets = Vec::new(); + schema_local_refs(schema, &mut targets); + for target in targets { + referrers.entry(target).or_default().push(name.clone()); + } + } + // A component declared as a `$ref` is an alias. An alias to a split + // model splits as well, so it has to reach the closure below. + ReferenceOr::Reference { reference } => { + if let Some(target) = ref_target_name(reference) { + referrers.entry(target.to_owned()).or_default().push(name.clone()); + } + } } } @@ -1067,6 +1075,28 @@ mod tests { assert!(!split.contains("Bystander"), "an unrelated schema keeps its one name"); } + /// A component declared as a `$ref` is an alias, and an alias to a split + /// model splits as well. + /// + /// Missing one emits a reference to a name the models run never writes, so + /// the composed crate does not build. + #[test] + fn an_alias_to_a_split_model_splits_as_well() { + let doc: OpenAPI = serde_yaml::from_str( + "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths: {}\ncomponents:\n schemas:\n Marked:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n Alias:\n $ref: '#/components/schemas/Marked'\n Plain:\n type: object\n properties:\n a:\n type: string\n PlainAlias:\n $ref: '#/components/schemas/Plain'\n", + ) + .expect("parse doc"); + + let split = direction_split_schemas(&doc); + assert!(split.contains("Marked"), "the model that declares the mark splits"); + assert!(split.contains("Alias"), "an alias to a split model splits"); + assert!(!split.contains("Plain"), "an unmarked model keeps its one name"); + assert!( + !split.contains("PlainAlias"), + "an alias to an unmarked model keeps its one name" + ); + } + /// A mark on a property splits the model that declares it, and every model /// that reaches it. #[test] diff --git a/crates/oapi-codegen/src/lower/direction.rs b/crates/oapi-codegen/src/lower/direction.rs index ebc38ae..ff82fb4 100644 --- a/crates/oapi-codegen/src/lower/direction.rs +++ b/crates/oapi-codegen/src/lower/direction.rs @@ -476,7 +476,8 @@ mod tests { } #[test] - fn a_holder_that_one_direction_reaches_keeps_its_name_and_names_the_split_shape() { let yaml = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n post:\n operationId: createAccount\n requestBody:\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Page'\ncomponents:\n schemas:\n Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n Page:\n type: object\n properties:\n items:\n type: array\n items:\n $ref: '#/components/schemas/Account'\n"; + fn a_holder_that_one_direction_reaches_keeps_its_name_and_names_the_split_shape() { + let yaml = "openapi: 3.0.3\ninfo:\n title: t\n version: '1'\npaths:\n /accounts:\n post:\n operationId: createAccount\n requestBody:\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Account'\n responses:\n '200':\n description: ok\n content:\n application/json:\n schema:\n $ref: '#/components/schemas/Page'\ncomponents:\n schemas:\n Account:\n type: object\n properties:\n id:\n type: string\n readOnly: true\n email:\n type: string\n Page:\n type: object\n properties:\n items:\n type: array\n items:\n $ref: '#/components/schemas/Account'\n"; let module = split_service_yaml(yaml); assert_eq!(names(&module), vec!["AccountRequest", "AccountResponse", "Page"]); let Some(Item::Struct(page)) = module.items.iter().find(|item| return item.name() == "Page") else {