Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion crates/oapi-codegen/src/console.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,12 @@ fn hints_for(err: &Error) -> Vec<String> {
.to_owned(),
];
}
Error::UnsupportedSchema { reason, .. } if reason.contains("the union holds") => {
return vec![
"A `oneOf` becomes an untagged enum. Serde reads the variants in order and takes the first that fits, so a repeated type is unreachable. Remove the repeated member."
.to_owned(),
];
}
Error::UnsupportedSchema { reason, .. } if reason.contains("`enum`") => {
return vec![
"An `enum` names each value once. Remove the repeat.".to_owned(),
Expand Down Expand Up @@ -572,7 +578,17 @@ mod tests {
"the `uniqueItems` rule does not reach the type this field holds",
"numbers, strings, or booleans",
),
("the `enum` gives `1` more than once", "Remove the repeat"),
("the `enum` gives `1` more than once", "names each value once"),
(
"the union holds `Cat` twice, as `Cat` and as `Cat2`",
"Remove the repeated member",
),
// A schema named `enum` puts that word in the union message too.
// The union arm must still win.
(
"the union holds `enum` twice, as `Enum` and as `Enum2`",
"Remove the repeated member",
),
];
for (reason, wanted) in cases {
let err = Error::UnsupportedSchema {
Expand Down
27 changes: 27 additions & 0 deletions crates/oapi-codegen/src/ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,33 @@ impl RustType {
);
}

/// The type as text, for an error message.
///
/// Emission goes through `emit::emit_type`, which builds tokens. This gives
/// a reader the same type in a message that the lowering stage can write,
/// where no token stream exists yet.
pub fn label(&self) -> String {
return match self {
RustType::Bool => "bool".to_owned(),
RustType::I32 => "i32".to_owned(),
RustType::I64 => "i64".to_owned(),
RustType::F64 => "f64".to_owned(),
RustType::String => "String".to_owned(),
RustType::Value => "serde_json::Value".to_owned(),
RustType::Date => "chrono::NaiveDate".to_owned(),
RustType::DateTime => "chrono::DateTime<chrono::Utc>".to_owned(),
RustType::Uuid => "uuid::Uuid".to_owned(),
RustType::Bytes => "Vec<u8>".to_owned(),
RustType::Vec(inner) => format!("Vec<{}>", inner.label()),
RustType::Map(inner) => format!("std::collections::HashMap<String, {}>", inner.label()),
RustType::Option(inner) => format!("Option<{}>", inner.label()),
RustType::Boxed(inner) => format!("Box<{}>", inner.label()),
RustType::Named(name) => name.clone(),
RustType::External { module, name } => format!("{module}::{name}"),
RustType::Verbatim { text, .. } => text.clone(),
};
}

/// An `x-rust-type` target whose `x-rust-derive` is absent, so it claims all
/// three non-serde traits. The common case, and the shape every test that
/// does not test this feature wants.
Expand Down
59 changes: 57 additions & 2 deletions crates/oapi-codegen/src/lower/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ impl Mapper<'_> {
Some(disc) if !disc.mapping.is_empty() => self.union_variants_from_mapping(disc)?,
Some(_) | None => self.union_variants_from_members(name, members)?,
};
check_variant_types(name, &variants)?;
return Ok(Enum {
name: self.type_name_ident(name),
doc: doc_of(data),
Expand Down Expand Up @@ -495,10 +496,23 @@ impl Mapper<'_> {
}
}
ReferenceOr::Item(schema) => {
let hint = format!("{name}_variant_{index}");
// An `x-rust-name` on the member names both the variant and
// the type the member hoists, so the two agree. Without it
// the position is the only thing that tells members apart.
let hint = match extension_str(&schema.schema_data, X_RUST_NAME) {
Some(custom) => custom.to_owned(),
None => format!("{name}_variant_{index}"),
};
let ty = self.type_from_schema(&hint, schema)?;
// A scalar member hoists no type, so the hint reaches
// nothing. Name the variant after the type it holds, which
// says more than the position does.
let seed = match scalar_variant_name(&ty) {
Some(scalar) if !schema.schema_data.extensions.contains_key(X_RUST_NAME) => scalar.to_owned(),
Some(_) | None => hint,
};
Comment thread
dotkas marked this conversation as resolved.
Outdated
UnionVariant {
name: crate::naming::deconflict_ident(to_ident(&hint, Case::Pascal), &mut seen),
name: crate::naming::deconflict_ident(to_ident(&seed, Case::Pascal), &mut seen),
ty,
}
}
Expand Down Expand Up @@ -790,6 +804,46 @@ fn integer_variant_name(value: i64) -> String {
return format!("value_{value}");
}

/// The variant name for a union member that lowers to a scalar.
///
/// A scalar hoists no type of its own, so the position hint names nothing. The
/// type is the only thing that tells one scalar member from another.
fn scalar_variant_name(ty: &RustType) -> Option<&'static str> {
return match ty {
RustType::Bool => Some("Bool"),
RustType::I32 => Some("I32"),
RustType::I64 => Some("I64"),
RustType::F64 => Some("F64"),
RustType::String => Some("String"),
_ => None,
};
}

/// Reject a union that holds one type more than once.
///
/// The emitted enum is `#[serde(untagged)]`. Serde reads the variants in order
/// and takes the first that fits, so a repeated type makes the later variant
/// unreachable. A value built with that variant comes back as the earlier one,
/// which changes the value and reports nothing.
fn check_variant_types(name: &str, variants: &[UnionVariant]) -> Result<()> {
let mut diagnostics = crate::lower::validate::Diagnostics::new();
for (index, variant) in variants.iter().enumerate() {
let Some(earlier) = variants.iter().take(index).find(|other| return other.ty == variant.ty) else {
continue;
};
diagnostics.push(Error::UnsupportedSchema {
path: name.to_owned(),
reason: format!(
"the union holds `{}` twice, as `{}` and as `{}`",
variant.ty.label(),
earlier.name.logical(),
variant.name.logical()
),
});
}
return diagnostics.into_result();
}

/// Map an integer `format` to a Rust type.
pub(crate) fn integer_format_type(format: &VariantOrUnknownOrEmpty<IntegerFormat>) -> RustType {
let ty = match format {
Expand All @@ -801,6 +855,7 @@ pub(crate) fn integer_format_type(format: &VariantOrUnknownOrEmpty<IntegerFormat
}

/// Extract a string-valued extension (for example `x-rust-type`) from schema data.
/// Extract a string-valued extension (for example `x-rust-name`) from schema data.
fn extension_str<'a>(data: &'a SchemaData, key: &str) -> Option<&'a str> {
let value = data.extensions.get(key)?;
return value.as_str();
Expand Down
11 changes: 11 additions & 0 deletions crates/oapi-codegen/tests/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ const TEST_TABLE: &[Feature] = &[
status: Status::Supported,
fixture: Some("oneof_untagged"),
},
Feature {
element: "schema.oneOf.variant-naming",
status: Status::Supported,
fixture: Some("oneof_variant_naming"),
},
Feature {
element: "schema.oneOf.duplicate-variant-type",
status: Status::Unsupported,
fixture: Some("unsupported_duplicate_union_variant"),
},
Feature {
element: "schema.oneOf.discriminator",
status: Status::Supported,
Expand Down Expand Up @@ -704,6 +714,7 @@ generated_tests!(
object_optional_required,
oneof_discriminator,
oneof_untagged,
oneof_variant_naming,
primitive_scalars,
recursive_schema,
ref_local,
Expand Down
56 changes: 56 additions & 0 deletions crates/oapi-codegen/tests/fixtures/oneof_variant_naming.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
openapi: "3.0.3"
info:
title: how an inline oneOf member gets its name
version: "1.0.0"
paths: {}
components:
schemas:
# A scalar member hoists no type, so the position names nothing a reader can
# use. Each variant takes the name of the type it holds.
Scalars:
oneOf:
- type: string
- type: integer
- type: boolean
# An object member hoists a struct, and the two names must agree. Without an
# override the position is the only thing that tells the members apart.
Objects:
oneOf:
- type: object
required: [meow]
properties:
meow:
type: string
- type: object
required: [bark]
properties:
bark:
type: string
# `x-rust-name` on an inline member names the variant and the hoisted type.
Named:
oneOf:
- x-rust-name: Cat
type: object
required: [meow]
properties:
meow:
type: string
- x-rust-name: Dog
type: object
required: [bark]
properties:
bark:
type: string
# A member list that mixes the three cases above.
Mixed:
oneOf:
- type: string
- type: array
items:
type: string
- x-rust-name: Detail
type: object
required: [note]
properties:
note:
type: string
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
openapi: "3.0.3"
info:
title: a oneOf that holds one type twice
version: "1.0.0"
paths: {}
components:
schemas:
Cat:
type: object
required: [meow]
properties:
meow:
type: string
# Both members lower to `Cat`. The emitted enum is untagged, so serde reads
# the variants in order and takes the first that fits. The second variant
# never matches, and a value built with it comes back as the first.
Pet:
oneOf:
- $ref: "#/components/schemas/Cat"
- $ref: "#/components/schemas/Cat"
2 changes: 2 additions & 0 deletions crates/oapi-codegen/tests/generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ mod generated {
pub mod oneof_discriminator;
#[path = "oneof_untagged.rs"]
pub mod oneof_untagged;
#[path = "oneof_variant_naming.rs"]
pub mod oneof_variant_naming;
#[path = "prelude_result_name.rs"]
pub mod prelude_result_name;
#[path = "prelude_value_names.rs"]
Expand Down
64 changes: 64 additions & 0 deletions crates/oapi-codegen/tests/generated/oneof_variant_naming.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Code generated by oapi-codegen-rust. DO NOT EDIT.
#![allow(
dead_code,
clippy::all,
clippy::pedantic,
clippy::nursery,
clippy::restriction,
reason = "generated code, not first-party source"
)]

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum Scalars {
String(String),
I64(i64),
Bool(bool),
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum Objects {
ObjectsVariant0(ObjectsVariant0),
ObjectsVariant1(ObjectsVariant1),
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum Named {
Cat(Cat),
Dog(Dog),
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum Mixed {
String(String),
MixedVariant1(Vec<String>),
Detail(Detail),
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub struct ObjectsVariant0 {
pub meow: String,
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub struct ObjectsVariant1 {
pub bark: String,
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub struct Cat {
pub meow: String,
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub struct Dog {
pub bark: String,
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub struct Detail {
pub note: String,
}
2 changes: 1 addition & 1 deletion crates/oapi-codegen/tests/generated/recursive_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub struct Kid {
#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum Expression {
ExpressionVariant0(String),
String(String),
Expression(Box<Expression>),
}

Expand Down
29 changes: 28 additions & 1 deletion docs/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ An inline schema carries no name of its own, so `x-rust-name` on that schema has
nothing to override. The remedy acts on the component schema that encloses it, or it
moves the inline schema into a component of its own. This matches Go's
`oapi-codegen`, which documents `x-go-name` on a component schema and on a property,
and not on an inline schema.
and not on an inline schema. A member of a `oneOf` list is the one exception,
because that member becomes a variant that needs a name of its own. See
[Union variants](#union-variants).
Comment thread
dotkas marked this conversation as resolved.
Outdated

A per-operation type can take the name of a model. A schema named `<Op>Response` is
the common case.
Expand Down Expand Up @@ -348,6 +350,31 @@ reason: the literal does not fit.
A `number` or `boolean` `enum` still lowers to a bare `f64` or `bool`. A float is
not a legal discriminant, and a boolean enum names nothing useful.

## A union cannot hold one type twice

A `oneOf` or an `anyOf` becomes an enum with `#[serde(untagged)]`, so no tag
appears on the wire and serde picks a variant by shape. It reads the variants in
declaration order and takes the first that fits.

That order makes a repeated type unreachable. A union whose members give `Cat`
twice compiles, and the second variant never matches: a value built with it
serializes like the first and reads back as the first. The value changes, and
nothing reports it. So a repeated type is an error, and the message names the two
variants that share it.

The check reads the lowered Rust type, not the schema. Two members that differ in
the document but reach one type still collide, which is the case that matters,
because the wire is all serde sees.

The check does not read shapes. Two distinct types with the same fields still
shadow each other at run time, and the generator accepts them. Deciding that in
general means comparing every optional field and every subset, so the generator
draws the line at a repeated type, where the fault is exact.

A variant takes its name from `x-rust-name`, else from the type a `$ref` names,
else from the type an inline scalar holds, else from its position. `x-rust-name`
on a member names the hoisted type too, so the variant and its payload agree.

## Value constraints are checked when the value comes in

A schema can narrow the values it accepts with `pattern`, `minimum`, `minItems`,
Expand Down
Loading
Loading