fix: preserve allOf aliases and singleton maps - #138
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate unresolved lowering issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves allOf lowering for aliases, singleton maps, and closed empty objects, with added regression coverage and documentation.
Changes:
- Preserves map and equivalent composite aliases.
- Handles closed empty objects consistently.
- Adds fixtures, generated outputs, runtime tests, and coverage updates.
Review findings: Critical (2 votes): schema.rs can create duplicate definitions for inline map value schemas. Moderate (1 vote): all_of.rs does not preserve singleton wrapper aliases.
File summaries
| File | Summary |
|---|---|
docs/design.md |
Documents updated allOf behavior. |
crates/oapi-codegen/tests/generated/allof_merge.rs |
Updates generated regression models. |
crates/oapi-codegen/tests/generated.rs |
Adds serialization and constraint tests. |
crates/oapi-codegen/tests/fixtures/allof_merge.yaml |
Adds map, alias, and closed-object fixtures. |
crates/oapi-codegen/tests/coverage.rs |
Accepts empty braced structs. |
crates/oapi-codegen/src/lower/schema.rs |
Lowers singleton maps and closed objects; contains the critical duplicate-definition issue. |
crates/oapi-codegen/src/lower/paths.rs |
Tests inline map body lowering. |
crates/oapi-codegen/src/lower/all_of.rs |
Reuses equivalent composites; contains the moderate singleton-alias issue. |
crates/oapi-codegen/src/coverage.rs |
Removes obsolete closed-map warnings. |
Review details
Suppressed comments (1)
crates/oapi-codegen/src/lower/all_of.rs:189
- This guard only recognizes resolved composites with more than one
allOfmember. An alias declared asAliasA: { allOf: [{ $ref: Composite }] }resolves to a one-member wrapper, so two equivalent wrapper aliases skip this fast path;resolve()then unwraps into the multi-memberCompositeand errors with “overlapping composed properties”. Treat any equal resolvedallOfschema as reusable (the equality already protects against differing metadata), so these aliases are preserved too.
if matches!(&resolved_left.schema_kind, SchemaKind::AllOf { all_of } if all_of.len() > 1)
&& resolved_left == resolved_right
- Files reviewed: 8/9 changed files
- Comments generated: 1
- 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.
🟡 Changes recommended
A critical unresolved allOf alias comparison issue can cause valid singleton-wrapper aliases to fail during resolution.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Closed-object lowering can accept unsatisfiable schemas, and the related documentation remains contradictory.
Review details
Suppressed comments (3)
crates/oapi-codegen/src/lower/schema.rs:244
- This new branch sends empty closed objects to
object_to_struct, but that builder only iterates declared properties. A schema such as{type: object, additionalProperties: false, required: [missing]}therefore emitsstruct X {}and accepts{}, even though the closed schema requires an undeclared property and is unsatisfiable. Validate/reject undeclared required names before emitting this struct; the singletonallOfpath already rejects non-emptyrequiredfor the same reason.
if obj.properties.is_empty() && !matches!(obj.additional_properties, Some(AdditionalProperties::Any(false))) {
crates/oapi-codegen/src/lower/schema.rs:822
- The analogous inline branch has the same correctness gap: empty closed objects with
requirednames not present inpropertiesare lowered to an empty struct, so{}deserializes even though the schema cannot be satisfied. Please apply the same undeclared-required validation here beforeobject_to_structis called.
if obj.properties.is_empty() && !matches!(obj.additional_properties, Some(AdditionalProperties::Any(false))) {
docs/design.md:360
- This new statement leaves the later default-handling documentation at
docs/design.md:688contradictory: it still says an empty object is always represented as a map, while this change makes an empty closed object (additionalProperties: false) a struct. Please qualify that existing statement with the closed-object exception.
Empty closed objects remain structs and reject additional properties.
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
🎉 This PR is included in version 1.4.0-dev.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.