Repository navigation
fix(csharp): serialize object oneOf variants - #25549
Open
AndreyVMarkelov wants to merge 2 commits into
Open
AndreyVMarkelov wants to merge 2 commits into
AndreyVMarkelov wants to merge 2 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 63 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/csharp/generichost/net4.7/OneOf/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/OneOf/src/Org.OpenAPITools/Model/Fruit.cs:244">
P1: This new Banana serialization does not round-trip through the generated client: `count` makes `Fruit.Read` accept the object as an `Apple`, because `Apple.Read` ignores unknown properties and does not require `kind`. Validate each oneOf variant's required properties or otherwise select the matching variant before returning from `Fruit.Read`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/SourceGeneration/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/SourceGeneration/src/Org.OpenAPITools/Model/Fruit.cs:216">
P2: These independent branches serialize both variants when callers set both public properties, producing JSON that violates `Fruit`'s `oneOf` contract and may be deserialized as a different variant. Make the variant branches mutually exclusive, matching the Apple-first selection used by `Read`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:258">
P2: This lookup can select a user-supplied converter that supports `MixedSubId` but is not `MixedSubIdJsonConverter`, causing serialization to throw before writing the variant. Select the generated converter by its concrete type instead of casting the first converter that claims support.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| converter.WriteProperties(writer, fruit.Apple, jsonSerializerOptions); | ||
| } | ||
|
|
||
| if (fruit.Banana != null) |
Contributor
There was a problem hiding this comment.
P1: This new Banana serialization does not round-trip through the generated client: count makes Fruit.Read accept the object as an Apple, because Apple.Read ignores unknown properties and does not require kind. Validate each oneOf variant's required properties or otherwise select the matching variant before returning from Fruit.Read.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net4.7/OneOf/src/Org.OpenAPITools/Model/Fruit.cs, line 244:
<comment>This new Banana serialization does not round-trip through the generated client: `count` makes `Fruit.Read` accept the object as an `Apple`, because `Apple.Read` ignores unknown properties and does not require `kind`. Validate each oneOf variant's required properties or otherwise select the matching variant before returning from `Fruit.Read`.</comment>
<file context>
@@ -235,6 +235,24 @@ public override void Write(Utf8JsonWriter writer, Fruit fruit, JsonSerializerOpt
+ converter.WriteProperties(writer, fruit.Apple, jsonSerializerOptions);
+ }
+
+ if (fruit.Banana != null)
+ {
+ BananaJsonConverter converter = (BananaJsonConverter) jsonSerializerOptions.Converters.First(c => c.CanConvert(typeof(Banana)));
</file context>
Contributor
There was a problem hiding this comment.
24 issues found across 104 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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/csharp/generichost/net8/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:256">
P2: This multiple-variant check only covers `MixedSubId`, so invalid combinations involving the primitive `oneOf` members are accepted. Count or validate all five variant properties before serialization so every combination with more than one active variant raises `JsonException`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/FruitReq.cs:194">
P2: This path performs a full string-and-DOM round trip for every active variant even when the generated variant converter is already registered, adding avoidable CPU and allocations to every request containing `FruitReq`. Keep the direct `WriteProperties` fast path for generated converters and reserve the round trip for custom converters or otherwise unsupported converter cases.</violation>
</file>
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpGenericHostOneOfSerializationTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpGenericHostOneOfSerializationTest.java:28">
P2: This assertion no longer verifies serialization in both oneOf branches. A regression that removes the `FolderEntry` serialization while retaining the `FileEntry` call would still pass because `assertFileContains` searches the entire file; assert branch-specific generated snippets or the expected occurrence count instead.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:260">
P2: This guard does not enforce the oneOf invariant for the primitive alternatives exposed by `MixedOneOfContent`: setting `String` (or another primitive) together with `MixedSubId` still serializes only the object variant instead of throwing. Apply the conflict check to every oneOf alternative, or validate all variant properties before writing.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/FruitReq.cs:197">
P2: This round-trips every oneOf variant through a string and `JsonDocument` before writing it into the wrapper, adding avoidable CPU and allocations to every `FruitReq` serialization. Call the generated variant converter’s `WriteProperties` directly, retaining the round-trip only for genuinely custom converters that cannot flatten into the wrapper.</violation>
</file>
<file name="modules/openapi-generator/src/test/resources/3_0/csharp/oneof-runtime/OneOfSerializationRegressionTests.cs">
<violation number="1" location="modules/openapi-generator/src/test/resources/3_0/csharp/oneof-runtime/OneOfSerializationRegressionTests.cs:46">
P2: The custom- and derived-variant tests do not verify that wrapper properties remain serialized. Set `fruit.Color` and assert the `color` property in these converter-specific cases so regressions in the variant-merging path are detected.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:256">
P2: The new oneOf guard tracks only non-primitive variants, so it misses states where a primitive variant and `MixedSubId` are both set. Track every oneOf variant before writing properties; otherwise invalid wrapper state is silently serialized as only the object variant.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/NullReferenceTypes/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/NullReferenceTypes/src/Org.OpenAPITools/Model/FruitReq.cs:196">
P2: This adds a JSON string allocation, parse, DOM allocation, and property copy for every object `oneOf` serialization, even when the generated variant converter is already registered. Use the generated converter’s `WriteProperties` fast path and reserve the buffered round-trip for custom or otherwise unavailable converters.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Model/FruitReq.cs:194">
P2: `JsonDocument.Parse(string)` imposes its independent default maximum depth, so variants deeper than 64 fail even when `jsonSerializerOptions.MaxDepth` permits them. Pass matching `JsonDocumentOptions` with `MaxDepth = variantOptions.MaxDepth` for both branches, or avoid the intermediate JSON document.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/FormModels/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/FormModels/src/Org.OpenAPITools/Model/FruitReq.cs:194">
P2: This intermediate parse ignores `jsonSerializerOptions.MaxDepth`, so deeply nested oneOf variants fail at the wrapper even when the configured serializer allows them. Pass the effective serializer depth to `JsonDocument.Parse`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/NullReferenceTypes/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/NullReferenceTypes/src/Org.OpenAPITools/Model/Fruit.cs:222">
P2: This round-trip through a JSON string and `JsonDocument` runs even when the generated variant converter is already registered. Call the generated converter's `WriteProperties` directly for that case, and retain buffering only for custom or otherwise unavailable converters.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Model/Fruit.cs:220">
P2: This materializes and parses a complete JSON document for every oneOf serialization, adding avoidable allocations to every request. Use the generated variant converter's `WriteProperties` directly when available, and reserve the round-trip fallback for custom converters.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:254">
P2: This guard does not enforce oneOf exclusivity for this generated model: `String`, `Bool`, `Int`, and `Decimal` are also public alternatives, but only `MixedSubId` updates `oneOfValueWritten`. Track all alternatives or validate the complete set before writing, otherwise an object with multiple variants set is serialized without the promised `JsonException`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:253">
P2: `oneOfValueWritten` tracks only the non-primitive variant, so this mixed oneOf still accepts multiple active variants. For example, setting `String` and `MixedSubId` silently drops the scalar and writes only `MixedSubId`; track or validate every oneOf alternative before writing.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/Petstore/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/Petstore/src/Org.OpenAPITools/Model/FruitReq.cs:194">
P2: This adds a string allocation, JSON parse, DOM allocation, and property copy on every object `oneOf` serialization even when the generated converter is already registered. Call the generated converter's `WriteProperties` directly and reserve the round-trip for custom converters or missing registrations.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.8/FormModels/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.8/FormModels/src/Org.OpenAPITools/Model/Fruit.cs:220">
P2: This intermediate parse imposes a hard 64-level depth limit that can be lower than the caller's configured `JsonSerializerOptions.MaxDepth`. Pass matching `JsonDocumentOptions.MaxDepth` (or avoid reparsing the serialized variant) in both oneOf branches so deep models retain the configured limit.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.8/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.8/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:257">
P2: `oneOfValueWritten` only tracks `MixedSubId`, so it misses active `String`, `Bool`, `Int`, or `Decimal` oneOf properties. Those properties are public and can be set after deserialization, causing serialization to silently drop the extra value instead of throwing. Update the generated check to account for every oneOf member.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:257">
P2: This guard can never detect a primitive-plus-object combination because the primitive oneOf fields are not checked before `MixedSubId` sets `oneOfValueWritten`; such an object silently serializes only `MixedSubId`. Track every oneOf variant when enforcing exclusivity, or reject the combination before writing.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:257">
P2: This guard does not enforce oneOf exclusivity for `MixedOneOfContent`: primitive variants can be set alongside `MixedSubId` without changing `oneOfValueWritten`. Validate all oneOf slots, including the primitive branches, before writing the merged object.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:267">
P2: This round-trip does not preserve the caller's configured maximum depth: variants deeper than `JsonDocument`'s default limit fail before their properties are copied. Pass a `JsonDocumentOptions` with `MaxDepth` derived from `jsonSerializerOptions.MaxDepth`, or avoid the intermediate document and write the variant properties directly.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Model/FruitReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Model/FruitReq.cs:196">
P2: This round-trips every object variant through a string and `JsonDocument` even when its generated converter is already registered, adding avoidable allocation and parsing to every `FruitReq` serialization. Call the generated `WriteProperties` directly when available, and retain the round-trip only as the missing-converter fallback.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:254">
P2: The new oneOf validation does not cover the scalar alternatives in this wrapper. When a caller sets `MixedSubId` and one of `String`, `Bool`, `Int`, or `Decimal`, serialization proceeds with only `MixedSubId` instead of rejecting the invalid multiple-variant state; track every oneOf alternative (or explicitly validate the scalar members) before writing.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net8/SourceGeneration/src/Org.OpenAPITools/Model/Fruit.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net8/SourceGeneration/src/Org.OpenAPITools/Model/Fruit.cs:223">
P2: This buffers and parses every active variant even when the generated converter is already registered. Call the generated converter's `WriteProperties` directly on that path and reserve buffering for converters that cannot expose properties.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs:257">
P2: This multiple-variant check does not cover the scalar `oneOf` members in `MixedOneOfContent`. Check every active variant before serialization so assigning a scalar property together with `MixedSubId` raises `JsonException` instead of emitting an invalid partial value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| { | ||
| writer.WriteStartObject(); | ||
|
|
||
| bool oneOfValueWritten = false; |
Contributor
There was a problem hiding this comment.
P2: This multiple-variant check only covers MixedSubId, so invalid combinations involving the primitive oneOf members are accepted. Count or validate all five variant properties before serialization so every combination with more than one active variant raises JsonException.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net8/NullReferenceTypes/src/Org.OpenAPITools/Model/MixedOneOfContent.cs, line 256:
<comment>This multiple-variant check only covers `MixedSubId`, so invalid combinations involving the primitive `oneOf` members are accepted. Count or validate all five variant properties before serialization so every combination with more than one active variant raises `JsonException`.</comment>
<file context>
@@ -253,12 +253,29 @@ public override void Write(Utf8JsonWriter writer, MixedOneOfContent mixedOneOfCo
{
writer.WriteStartObject();
+ bool oneOfValueWritten = false;
if (mixedOneOfContent.MixedSubId != null)
{
</file context>
| variantOptions = new JsonSerializerOptions(jsonSerializerOptions); | ||
| variantOptions.Converters.Add(new AppleReqJsonConverter()); | ||
| } | ||
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) |
Contributor
There was a problem hiding this comment.
P2: This path performs a full string-and-DOM round trip for every active variant even when the generated variant converter is already registered, adding avoidable CPU and allocations to every request containing FruitReq. Keep the direct WriteProperties fast path for generated converters and reserve the round trip for custom converters or otherwise unsupported converter cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/FruitReq.cs, line 194:
<comment>This path performs a full string-and-DOM round trip for every active variant even when the generated variant converter is already registered, adding avoidable CPU and allocations to every request containing `FruitReq`. Keep the direct `WriteProperties` fast path for generated converters and reserve the round trip for custom converters or otherwise unsupported converter cases.</comment>
<file context>
@@ -178,18 +178,50 @@ public override void Write(Utf8JsonWriter writer, FruitReq fruitReq, JsonSeriali
+ variantOptions = new JsonSerializerOptions(jsonSerializerOptions);
+ variantOptions.Converters.Add(new AppleReqJsonConverter());
+ }
+ using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)))
+ {
+ if (document.RootElement.ValueKind != JsonValueKind.Object)
</file context>
| new DefaultGenerator().opts(config.toClientOptInput()).generate(); | ||
| assertFileContains(output.resolve("src/OneOfRepro/Model/Entry.cs"), | ||
| "if (entry.FileEntry != null)", | ||
| "JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)", |
Contributor
There was a problem hiding this comment.
P2: This assertion no longer verifies serialization in both oneOf branches. A regression that removes the FolderEntry serialization while retaining the FileEntry call would still pass because assertFileContains searches the entire file; assert branch-specific generated snippets or the expected occurrence count instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpGenericHostOneOfSerializationTest.java, line 28:
<comment>This assertion no longer verifies serialization in both oneOf branches. A regression that removes the `FolderEntry` serialization while retaining the `FileEntry` call would still pass because `assertFileContains` searches the entire file; assert branch-specific generated snippets or the expected occurrence count instead.</comment>
<file context>
@@ -25,8 +25,8 @@ public void testObjectOneOfWithoutDiscriminatorWritesVariant() throws Exception
assertFileContains(output.resolve("src/OneOfRepro/Model/Entry.cs"),
"if (entry.FileEntry != null)",
- "converter.WriteProperties(writer, entry.FileEntry, jsonSerializerOptions);",
+ "JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)",
"if (entry.FolderEntry != null)",
- "converter.WriteProperties(writer, entry.FolderEntry, jsonSerializerOptions);");
</file context>
| bool oneOfValueWritten = false; | ||
| if (mixedOneOfContent.MixedSubId != null) | ||
| { | ||
| if (oneOfValueWritten) |
Contributor
There was a problem hiding this comment.
P2: This guard does not enforce the oneOf invariant for the primitive alternatives exposed by MixedOneOfContent: setting String (or another primitive) together with MixedSubId still serializes only the object variant instead of throwing. Apply the conflict check to every oneOf alternative, or validate all variant properties before writing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/MixedOneOfContent.cs, line 260:
<comment>This guard does not enforce the oneOf invariant for the primitive alternatives exposed by `MixedOneOfContent`: setting `String` (or another primitive) together with `MixedSubId` still serializes only the object variant instead of throwing. Apply the conflict check to every oneOf alternative, or validate all variant properties before writing.</comment>
<file context>
@@ -254,12 +254,29 @@ public override void Write(Utf8JsonWriter writer, MixedOneOfContent mixedOneOfCo
{
- MixedSubIdJsonConverter converter = (MixedSubIdJsonConverter) jsonSerializerOptions.Converters.First(c => c.CanConvert(typeof(MixedSubId)));
- converter.WriteProperties(writer, mixedOneOfContent.MixedSubId, jsonSerializerOptions);
+ if (oneOfValueWritten)
+ throw new JsonException("Multiple oneOf variants are set for MixedOneOfContent.");
+ oneOfValueWritten = true;
</file context>
| variantOptions = new JsonSerializerOptions(jsonSerializerOptions); | ||
| variantOptions.Converters.Add(new AppleReqJsonConverter()); | ||
| } | ||
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) |
Contributor
There was a problem hiding this comment.
P2: This round-trips every oneOf variant through a string and JsonDocument before writing it into the wrapper, adding avoidable CPU and allocations to every FruitReq serialization. Call the generated variant converter’s WriteProperties directly, retaining the round-trip only for genuinely custom converters that cannot flatten into the wrapper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net9/SourceGeneration/src/Org.OpenAPITools/Model/FruitReq.cs, line 197:
<comment>This round-trips every oneOf variant through a string and `JsonDocument` before writing it into the wrapper, adding avoidable CPU and allocations to every `FruitReq` serialization. Call the generated variant converter’s `WriteProperties` directly, retaining the round-trip only for genuinely custom converters that cannot flatten into the wrapper.</comment>
<file context>
@@ -181,18 +181,50 @@ public override void Write(Utf8JsonWriter writer, FruitReq fruitReq, JsonSeriali
+ variantOptions = new JsonSerializerOptions(jsonSerializerOptions);
+ variantOptions.Converters.Add(new AppleReqJsonConverter());
+ }
+ using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)))
+ {
+ if (document.RootElement.ValueKind != JsonValueKind.Object)
</file context>
| variantOptions = new JsonSerializerOptions(jsonSerializerOptions); | ||
| variantOptions.Converters.Add(new MixedSubIdJsonConverter()); | ||
| } | ||
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) |
Contributor
There was a problem hiding this comment.
P2: This round-trip does not preserve the caller's configured maximum depth: variants deeper than JsonDocument's default limit fail before their properties are copied. Pass a JsonDocumentOptions with MaxDepth derived from jsonSerializerOptions.MaxDepth, or avoid the intermediate document and write the variant properties directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net8/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs, line 267:
<comment>This round-trip does not preserve the caller's configured maximum depth: variants deeper than `JsonDocument`'s default limit fail before their properties are copied. Pass a `JsonDocumentOptions` with `MaxDepth` derived from `jsonSerializerOptions.MaxDepth`, or avoid the intermediate document and write the variant properties directly.</comment>
<file context>
@@ -251,12 +251,29 @@ public override void Write(Utf8JsonWriter writer, MixedOneOfContent mixedOneOfCo
+ variantOptions = new JsonSerializerOptions(jsonSerializerOptions);
+ variantOptions.Converters.Add(new MixedSubIdJsonConverter());
+ }
+ using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)))
+ {
+ if (document.RootElement.ValueKind != JsonValueKind.Object)
</file context>
Suggested change
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) | |
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions), new JsonDocumentOptions { MaxDepth = jsonSerializerOptions.MaxDepth })) |
| variantOptions = new JsonSerializerOptions(jsonSerializerOptions); | ||
| variantOptions.Converters.Add(new AppleReqJsonConverter()); | ||
| } | ||
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) |
Contributor
There was a problem hiding this comment.
P2: This round-trips every object variant through a string and JsonDocument even when its generated converter is already registered, adding avoidable allocation and parsing to every FruitReq serialization. Call the generated WriteProperties directly when available, and retain the round-trip only as the missing-converter fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Model/FruitReq.cs, line 196:
<comment>This round-trips every object variant through a string and `JsonDocument` even when its generated converter is already registered, adding avoidable allocation and parsing to every `FruitReq` serialization. Call the generated `WriteProperties` directly when available, and retain the round-trip only as the missing-converter fallback.</comment>
<file context>
@@ -180,18 +180,50 @@ public override void Write(Utf8JsonWriter writer, FruitReq fruitReq, JsonSeriali
+ variantOptions = new JsonSerializerOptions(jsonSerializerOptions);
+ variantOptions.Converters.Add(new AppleReqJsonConverter());
+ }
+ using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)))
+ {
+ if (document.RootElement.ValueKind != JsonValueKind.Object)
</file context>
| { | ||
| writer.WriteStartObject(); | ||
|
|
||
| bool oneOfValueWritten = false; |
Contributor
There was a problem hiding this comment.
P2: The new oneOf validation does not cover the scalar alternatives in this wrapper. When a caller sets MixedSubId and one of String, Bool, Int, or Decimal, serialization proceeds with only MixedSubId instead of rejecting the invalid multiple-variant state; track every oneOf alternative (or explicitly validate the scalar members) before writing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Model/MixedOneOfContent.cs, line 254:
<comment>The new oneOf validation does not cover the scalar alternatives in this wrapper. When a caller sets `MixedSubId` and one of `String`, `Bool`, `Int`, or `Decimal`, serialization proceeds with only `MixedSubId` instead of rejecting the invalid multiple-variant state; track every oneOf alternative (or explicitly validate the scalar members) before writing.</comment>
<file context>
@@ -251,12 +251,29 @@ public override void Write(Utf8JsonWriter writer, MixedOneOfContent mixedOneOfCo
{
writer.WriteStartObject();
+ bool oneOfValueWritten = false;
if (mixedOneOfContent.MixedSubId != null)
{
</file context>
| variantOptions = new JsonSerializerOptions(jsonSerializerOptions); | ||
| variantOptions.Converters.Add(new AppleJsonConverter()); | ||
| } | ||
| using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions))) |
Contributor
There was a problem hiding this comment.
P2: This buffers and parses every active variant even when the generated converter is already registered. Call the generated converter's WriteProperties directly on that path and reserve buffering for converters that cannot expose properties.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net8/SourceGeneration/src/Org.OpenAPITools/Model/Fruit.cs, line 223:
<comment>This buffers and parses every active variant even when the generated converter is already registered. Call the generated converter's `WriteProperties` directly on that path and reserve buffering for converters that cannot expose properties.</comment>
<file context>
@@ -207,18 +207,50 @@ public override void Write(Utf8JsonWriter writer, Fruit fruit, JsonSerializerOpt
+ variantOptions = new JsonSerializerOptions(jsonSerializerOptions);
+ variantOptions.Converters.Add(new AppleJsonConverter());
+ }
+ using (JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(variant, variant.GetType(), variantOptions)))
+ {
+ if (document.RootElement.ValueKind != JsonValueKind.Object)
</file context>
| bool oneOfValueWritten = false; | ||
| if (mixedOneOfContent.MixedSubId != null) | ||
| { | ||
| if (oneOfValueWritten) |
Contributor
There was a problem hiding this comment.
P2: This multiple-variant check does not cover the scalar oneOf members in MixedOneOfContent. Check every active variant before serialization so assigning a scalar property together with MixedSubId raises JsonException instead of emitting an invalid partial value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Model/MixedOneOfContent.cs, line 257:
<comment>This multiple-variant check does not cover the scalar `oneOf` members in `MixedOneOfContent`. Check every active variant before serialization so assigning a scalar property together with `MixedSubId` raises `JsonException` instead of emitting an invalid partial value.</comment>
<file context>
@@ -251,12 +251,29 @@ public override void Write(Utf8JsonWriter writer, MixedOneOfContent mixedOneOfCo
{
- MixedSubIdJsonConverter converter = (MixedSubIdJsonConverter) jsonSerializerOptions.Converters.First(c => c.CanConvert(typeof(MixedSubId)));
- converter.WriteProperties(writer, mixedOneOfContent.MixedSubId, jsonSerializerOptions);
+ if (oneOfValueWritten)
+ throw new JsonException("Multiple oneOf variants are set for MixedOneOfContent.");
+ oneOfValueWritten = true;
</file context>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #25547
For C#
generichostobjectoneOfmodels without a discriminator, write the active variant's properties alongside properties declared on the wrapper.Tests
oneOfsample.{}.Summary by cubic
Fixes #25547: C#
generichostobjectoneOfmodels without a discriminator now serialize the active variant's properties alongside the wrapper's own properties, instead of producing{}.JsonExceptionwhen multipleoneOfvariants are set on the wrapper.JsonConverter.mustacheto emitWritePropertiescalls for non-primitiveoneOfvariants before wrapper properties.Written for commit 233f838. Summary will update on new commits.