feat: allOf semantics - #136
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved findings remain, including one critical compile issue and four moderate correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements bounded allOf intersection semantics with merged constraints, documentation, and expanded tests.
Changes:
- Adds recursive object merging and scalar constraint intersection.
- Integrates
allOfhandling into schema lowering. - Expands fixtures, generated models, runtime tests, and documentation.
File summaries
| File | Summary |
|---|---|
docs/design.md |
Documents new allOf semantics and limitations. |
crates/oapi-codegen/tests/generated/allof_merge.rs |
Updates generated merge models. |
crates/oapi-codegen/tests/generated.rs |
Adds runtime assertions for merge behavior. |
crates/oapi-codegen/tests/fixtures/allof_merge.yaml |
Adds allOf intersection fixtures. |
crates/oapi-codegen/src/lower/schema.rs |
Integrates merging into schema lowering. |
crates/oapi-codegen/src/lower/mod.rs |
Registers the lowering module. |
crates/oapi-codegen/src/lower/all_of.rs |
Implements intersection and validation logic. |
crates/oapi-codegen/src/coverage.rs |
Removes obsolete allOf warning coverage. |
Review details
Suppressed comments (2)
crates/oapi-codegen/src/lower/all_of.rs:169
- This fast path only accepts identical references, so two identical inline composite definitions still go through
resolve; a multi-memberallOfthen fails the[member]match and is rejected. That contradicts the documented requirement that composite overlaps are allowed when their definitions are identical, and rejects a property declared identically in two members. Short-circuit identical multi-memberallOfitems before unwrapping them.
if left == right && matches!(left, ReferenceOr::Reference { .. }) {
return Ok(left.clone());
}
crates/oapi-codegen/src/lower/all_of.rs:310
- When the right-hand enum contains a duplicate that is also present on the left,
retainremoves the duplicate by projecting the left vector, so an unsupported member such as["a", "a", "b"]is silently normalized and generation succeeds. The normal enum lowering rejects repeated values; validate duplicates in both input enums before intersecting them soallOfcannot bypass that validation.
fn narrow_enum<T: Clone + PartialEq>(path: &str, left: &mut Vec<T>, right: &[T]) -> Result<()> {
if left.is_empty() {
left.extend_from_slice(right);
return Ok(());
}
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Four moderate findings in all_of.rs remain unresolved.
Review details
Suppressed comments (4)
crates/oapi-codegen/src/lower/all_of.rs:169
- The equality fast path is restricted to
$refs, so identical inline composite schemas are still rejected even though the design promises that composite overlaps with identical definitions are supported. Two duplicate properties with the same multi-memberallOfreachresolve, which rejects the composition instead of returning the unchanged definition. Apply this fast path to every identical definition, not only references.
if left == right && matches!(left, ReferenceOr::Reference { .. }) {
return Ok(left.clone());
}
crates/oapi-codegen/src/lower/all_of.rs:216
- The unsupported enum-overlap check does not include
format.combine_keywordcan retain a format from the other member, butstring_enumemits a plain string enum and never validates that format; for example,enum: ["not-a-uuid"]intersected withformat: uuidgenerates a type that accepts a value rejected by theallOf. Reject enum/format overlaps or add an equivalent validator before emission.
if !a.enumeration.is_empty()
&& (nullable || a.pattern.is_some() || a.min_length.is_some() || a.max_length.is_some())
{
crates/oapi-codegen/src/lower/all_of.rs:183
- This only rejects a custom type when the underlying schema kinds differ. If both overlapping properties carry the same
x-rust-typebut have different scalar constraints (for example, one hasminLengthand the othermaxLength), the merge proceeds; laterconstraints_ofignores all constraints on anx-rust-type, so the generated field accepts values that violate theallOfintersection. Custom-type overlaps with non-identical constraints need to be rejected or handled through the documented warning path rather than silently widened.
if left.schema_data.extensions.contains_key("x-rust-type") && left.schema_kind != right.schema_kind {
return Err(unsupported(path, "custom-type property constraints differ"));
}
crates/oapi-codegen/src/lower/all_of.rs:187
- This guard only runs when the two
SchemaKinds differ, but enum overlaps normally have the sameSchemaKind::Type(String/Integer). As a result,narrow_enumfilters the values while leavingx-enum-varnames/x-enumNamespositional metadata untouched; for example,["a", "b"]intersected with["b"]makes the survivingbuse the name at index 0. That silently generates the wrong Rust variant, despite the documented rule that positional-name enum intersections are unsupported. Reject these overlaps whenever positional names are present, or remap the names by wire value.
if left.schema_kind != right.schema_kind
&& ["x-enum-varnames", "x-enumNames"]
.iter()
.any(|key| return left.schema_data.extensions.contains_key(*key))
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the shared depth-budget issue and the documented format and enum-ordering inconsistencies.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
crates/oapi-codegen/src/lower/all_of.rs:322
- Because
retainpreserves the left-hand enum order, swapping theallOfmembers can change which Rust variant gets each wire value when two common strings normalize to the same identifier (for example,a-banda_b).string_enumdeconflicts variants in that order, so an order-equivalent schema can expose a different public API; canonicalize the narrowed order or reject these order-sensitive overlaps.
if !right.is_empty() {
left.retain(|value| return right.contains(value));
docs/design.md:364
- This contract is broader than the implementation:
lower/all_of.rsrejects formats for string-enum intersections, but the integer branch combines equal integer formats and narrows the enum, so twoint32enum definitions with different value sets still generate successfully. Either reject formatted integer enum overlaps too or narrow this sentence to the formats that are actually unsupported.
nullability, formats, scalar constraints, or positional variant names are unsupported.
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
|
🎉 This PR is included in version 1.4.0-dev.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
🔵 Needs a closer look
Resolve aliased allOf equivalence and preserve map lowering for empty-property objects.
Review details
Suppressed comments (2)
crates/oapi-codegen/src/lower/all_of.rs:139
- This only treats identical
ReferenceOrtokens as an identical composite. If two properties reach the same multi-memberallOfthrough different$refaliases (for exampleAandB: {$ref: A}),resolveenters this branch and rejects the valid overlap because the resolved composition has more than one member. Compare/canonicalize the resolved definitions before requiring a single-member wrapper; otherwise renaming or chaining a schema reference changes whether generation succeeds.
let [member] = all_of.as_slice() else {
return Err(unsupported(path, "overlapping composed properties are not supported"));
crates/oapi-codegen/src/lower/schema.rs:463
- This new guard sends every inline object through
merge_all_of, including objects with no named properties. Before this change,type_from_schemausedinline_object_typefor that shape and lowered it to a map; nowallOf: [{type: object, additionalProperties: {type: string}}]is rejected bycollect, and an unconstrained empty object becomes an empty named struct. Keep the existing fast path for empty-property objects (or otherwise preserve their map lowering).
ReferenceOr::Item(schema) if matches!(schema.schema_kind, SchemaKind::Type(Type::Object(_))) => {
return Ok(None);
}
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.