[rust] fix: discriminated-union variants drop every child-specific field - #24897
[rust] fix: discriminated-union variants drop every child-specific field#24897wiebren wants to merge 3 commits into
Conversation
A schema with a discriminator and mapped children generated a serde internally-tagged enum whose variants were inline structs built from the parent's vars - every child-specific field was silently dropped, and with duplicate mappings the variants even mixed vars across models. Wrap the mapped model in a newtype variant instead (boxed, like the oneOf variants), named by the uniquified modelName while wrapping the model's real classname. serde's internally-tagged deserialization consumes the tag key, so a wrapped child's own required discriminator property would fail with "missing field": RustClientCodegen now marks mapped children's discriminator properties (the rust var context never set isDiscriminator) and the template defaults them, skipping them back out while empty so the tag stays the only occurrence on the wire. Optional discriminator properties already tolerate absence through Option and are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
There was a problem hiding this comment.
1 issue found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/rust/model.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/rust/model.mustache:132">
P2: When callers construct a mapped child with its required discriminator populated, this variant can serialize duplicate discriminator keys instead of the promised single tag. Omit the child discriminator unconditionally in the enum representation, or add variant-specific serialization that does not forward the child tag.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| the key before this struct sees it - default it, and skip it back out while empty | ||
| so the tag stays the only occurrence on the wire (an optional one already | ||
| tolerates absence through Option) --}} | ||
| #[serde(rename = "{{{baseName}}}"{{#isDiscriminator}}{{#required}}, default, skip_serializing_if = "String::is_empty"{{/required}}{{/isDiscriminator}}{{^required}}{{#isNullable}}, default{{^isByteArray}}, with = "::serde_with::rust::double_option"{{/isByteArray}}{{/isNullable}}{{/required}}{{^required}}, skip_serializing_if = "Option::is_none"{{/required}}{{#required}}{{#isNullable}}, deserialize_with = "Option::deserialize"{{/isNullable}}{{/required}})] |
There was a problem hiding this comment.
P2: When callers construct a mapped child with its required discriminator populated, this variant can serialize duplicate discriminator keys instead of the promised single tag. Omit the child discriminator unconditionally in the enum representation, or add variant-specific serialization that does not forward the child tag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/rust/model.mustache, line 132:
<comment>When callers construct a mapped child with its required discriminator populated, this variant can serialize duplicate discriminator keys instead of the promised single tag. Omit the child discriminator unconditionally in the enum representation, or add variant-specific serialization that does not forward the child tag.</comment>
<file context>
@@ -133,7 +125,11 @@ pub struct {{{classname}}} {
+ the key before this struct sees it - default it, and skip it back out while empty
+ so the tag stays the only occurrence on the wire (an optional one already
+ tolerates absence through Option) --}}
+ #[serde(rename = "{{{baseName}}}"{{#isDiscriminator}}{{#required}}, default, skip_serializing_if = "String::is_empty"{{/required}}{{/isDiscriminator}}{{^required}}{{#isNullable}}, default{{^isByteArray}}, with = "::serde_with::rust::double_option"{{/isByteArray}}{{/isNullable}}{{/required}}{{^required}}, skip_serializing_if = "Option::is_none"{{/required}}{{#required}}{{#isNullable}}, deserialize_with = "Option::deserialize"{{/isNullable}}{{/required}})]
pub {{{name}}}: {{!
### Option Start
</file context>
There was a problem hiding this comment.
Confirmed, and measured: #24897 (comment)
Entity::Bar(Box::new(Bar::new("1".into(), "Bar".into()))) — the natural call, since new() takes the discriminator as a required argument — serializes to {"@type":"Bar","id":"1","@type":"Bar"}, and the same client then fails to read it back with duplicate field @type. So this is the default path rather than a hand-constructed edge case, and it is a hard failure rather than a cosmetic duplicate.
Omitting the child discriminator unconditionally is the rework withdrawn in #24897 (discussion_r3956615774) — it strips the field from standalone Bar/Foo and from the untagged BarRefOrValue too. The linked comment sets out the two structural options that do satisfy both constraints; a maintainer preference between them and I will push it.
Review found the serde-attribute approach broke on a required nullable discriminator (String::is_empty on an Option<String> does not compile) and could duplicate the tag when a caller populated the child's field. Remove the property from mapped children instead, exactly as postProcessModels already removes it from the discriminating parent: the variant name carries the type information, deserialization never misses a consumed tag key, and the tag is structurally the only occurrence on the wire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
|
cubic's findings reshaped this for the better. The serde-attribute approach is gone: mapped children now have their discriminator property removed, exactly as
Verified again end to end: the serde round-trip on the generated crate still passes (child fields populated, exactly one |
There was a problem hiding this comment.
All reported issues were addressed across 51 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…de tag Removing the property from mapped children went too far: getMappedModels() covers every allOf descendant, and those models are also returned and accepted standalone (create_bar returns Bar, create_foo takes Foo), so they lost a required field and their new() signature changed. Keep the property declared and mark it instead, so the template defaults it - the internally-tagged union consumes the key before the child deserializes - and skips it back out while unset, keeping the tag the only occurrence on the wire. The skip predicate follows the type: Option::is_none for a nullable discriminator, String::is_empty for a non-nullable string, which is what the first attempt got wrong. Standalone use is unchanged: the caller sets and reads the discriminator as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
There was a problem hiding this comment.
1 existing issue remains and 3 new issues found across 52 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/others/rust/reqwest/composed-oneof/src/models/obj_d.rs">
<violation number="1">
P2: The added `realtype` field on the child can never be read back from the wire. In the internally-tagged enum `#[serde(tag = "realtype")]`, serde consumes the `realtype` key to pick the variant and removes it before deserializing the inner struct, so `ObjD.realtype` is always `None` after deserialization — the same 'child field dropped/unreachable' defect this PR is meant to fix, now applied to the discriminator field itself. It is also a public mutable field: a caller who sets it causes serialization to emit a duplicate `realtype` key (the child's value plus the tag). Since this enum is the only path that consumes ObjD, consider dropping the child-side discriminator field entirely (serde owns the tag), rather than exposing a field that deserialization can never populate.</violation>
</file>
<file name="samples/client/others/rust/reqwest/composed-oneof/src/models/obj_c.rs">
<violation number="1">
P2: When a variant's child has its discriminator field populated, serialization emits the "realtype" key twice: once from the enum's #[serde(tag = "realtype")] and once from the child's realtype field. That happens whenever a caller constructs a variant by hand with the discriminator set (e.g. ObjC { realtype: Some("c-type".into()), .. }) or wraps a standalone model that already has it set, which the commit message says is the supported standalone use. The result is duplicate JSON keys on the wire, contradicting the PR's 'single tag key' goal. skip_serializing_if = "Option::is_none" only skips when the field is None, so it does not prevent the duplicate.</violation>
</file>
<file name="samples/client/others/rust/hyper/composed-oneof/src/models/obj_a.rs">
<violation number="1">
P2: When a caller sets `ObjA.realtype` before wrapping it in `CustomOneOfSchema::AType`, serde emits the enum discriminator and this child field under the same `realtype` key. That can produce duplicate or conflicting discriminator data in requests; remove the discriminator property from mapped child models instead of exposing it as a serializable field in the tagged union.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
cubic is right on both mechanisms, and measuring them makes the picture worse than the review states. Answering here rather than in three sample files, since all four violations are one design question. Reproduced against the exact shape the Three things follow:
The round-trips I reported on the previous push missed all three because they constructed children with the discriminator left unset, which is the one shape that behaves. My mistake, and worth saying plainly. On the suggested remedy. "Remove the discriminator property from mapped child models" is the rework this PR already tried and withdrew, on cubic's own reading in #24897 (comment): Two that work: A. Variant structs, populated from the child. Go back to an inline struct per variant, but build it from the child's own vars minus the discriminator instead of from the parent's vars. That fixes the original defect this PR opened for — child-specific fields silently dropped — and every point above, and leaves standalone models untouched. Cost: a variant is not the model type, so a B. Keep the newtype variants and generate I lean to B because the newtype variants are the point of the PR, but A is the smaller change and I am happy to write either — a preference from a rust maintainer would settle it faster than me guessing. Either way the fixture gains a round-trip that builds a variant through |
A schema with a
discriminatorand mapped children generates a serde internally-taggedenum — but the variants are inline structs built from the parent's vars, so every
child-specific field is silently dropped:
Deserializing an
ObjectExistserror body losesidentifierandobjectType: the callercan match the variant but not read what it says, while the standalone
models::ObjectExistsstruct has the full shape and is unreachable from the union. The repository's own
test-duplicatessample shows a second symptom — its inline variants carried vars mixedacross different models (
Personvariants withVehicle'sspeed). Found deserializingdiscriminated error responses of a production registry API on v7.15.0.
The fix
Emit newtype variants wrapping the mapped models, boxed like the oneOf variants already are:
The variant keeps the uniquified
modelName(duplicate mapping entries sharing one schemastill get distinct variants) while the wrapped type is the model's real classname via
MappedModel.getModel(), which survives that rename.One domino: serde's internally-tagged deserialization consumes the tag key, so a wrapped
child's own required discriminator property (
type: String) would fail withmissing field 'type'. The rust var context never setisDiscriminator, soRustClientCodegennow marks mapped children's discriminator properties inpostProcessAllModels, and the template gives them#[serde(default, skip_serializing_if = "String::is_empty")]— the variant name carriesthe type information, and the empty field is skipped back out so the tag stays the only
typekey on the wire. An optional discriminator property already tolerates absencethrough
Optionand is untouched.Tests
RustClientCodegenTest#testDiscriminatedUnionKeepsChildFieldsgenerates the new3_0/rust/discriminated-union.yamlfixture and asserts the newtype variants, the boxedDefault impl, and the child's serde attributes. Fails without the change (verified by
stashing it).
Verified end to end with a serde round-trip on the generated crate:
{"type":"ObjectExists","message":…,"identifier":…,"objectType":…}deserializes into thevariant with all child fields populated, serializes back with exactly one
"type"key, andre-parses equal.
PR checklist
./bin/generate-samples.sh ./bin/configs/rust-*):the oneOf and test-duplicates fixtures change (unions shrink to newtype variants, the
required discriminator properties gain the serde attributes), and every affected sample
crate builds clean with
cargo build— including test-duplicates, whose variants nowwrap the real
models::Person/models::Vehicle.Generated with Claude Code
Summary by cubic
Fixes Rust client generation for schemas with a discriminator and mapped children so enum variants keep their child-specific fields. Variants were inline structs built from the parent's vars, silently dropping every field the child added; they now wrap the mapped model in a boxed newtype variant.
Option::is_nonefor nullable,String::is_emptyfor non-nullable strings), keeping the tag the only occurrence on the wire.discriminated-union.yamlfixture and a regression test.Written for commit a887544. Summary will update on new commits.