[csharp][generichost] Correct error message on serialization - #24933
Merged
wing328 merged 2 commits intoSep 15, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
15 issues found across 1350 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="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/ReadOnlyFirst.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/ReadOnlyFirst.cs:195">
P3: The serialization path now throws JsonException for a null non-nullable property, but the matching deserialization path in the same converter (Read method) still throws ArgumentNullException for the identical condition. Callers handling this error must catch different exception types depending on whether they serialize or deserialize. Consider throwing the same exception type (JsonException) in the Read method as well for consistency.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/RolesReportsHash.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/RolesReportsHash.cs:195">
P3: The Write path now throws JsonException for a null value on a non-nullable property, but the matching Read path in this same file (lines 162/165) still throws ArgumentNullException for the identical condition. Deserialization of a null token for these optional non-nullable properties reaches that throw, so callers get an inconsistent and misleading exception type on the read side. Consider using JsonException there too for consistency.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/List.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/List.cs:178">
P3: The write path now throws JsonException for a set-but-null non-nullable property, but the symmetric deserialization path (Read, line 148) still throws ArgumentNullException for the identical condition. Both handle the same "non-nullable property is null" case, so callers get different exception types depending on direction. Make both consistent (JsonException is the more appropriate type for a JSON (de)serialization error); the read template JsonConverter.mustache would need the matching change.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/CopyActivity.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/CopyActivity.cs:175">
P2: Removing this check means a null `CopyActivitytt` is now silently serialized as `"copyActivitytt": null` instead of throwing. NRT annotations are compile-time only and are not enforced at runtime, so a null can still reach serialization (e.g. `new CopyActivity(null)`, reflection, or a derived type). The deserialization path still rejects null for this property, so serialization and deserialization now disagree on the same required, non-nullable contract. If the goal was to fix the error message, keep the guard and use the correct message rather than dropping validation.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/AnyOfNoCompare/src/Org.OpenAPITools/Model/Apple.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/AnyOfNoCompare/src/Org.OpenAPITools/Model/Apple.cs:179">
P3: WriteProperties now throws JsonException, but the matching Read path in the same converter still throws ArgumentNullException for the same non-nullable property. A caller handling serialization errors now must catch JsonException for writes and ArgumentNullException for reads of the same class. Align both paths on JsonException (the System.Text.Json convention for converter errors) for consistency.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/Descendant2.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/Descendant2.cs:181">
P2: Removing these guards makes the write path trust NRT, but AlternativeName and Confidentiality are mutable `{ get; set; }` auto-properties without the C# `required` modifier, so a null can still reach WriteProperties (via setter, reflection, or a construction path). With the guard gone, `writer.WriteString(...)` emits JSON `null` silently for a required, non-nullable property, while this same file's Read method still rejects that exact input with "Property is not nullable for class Descendant2." Write now accepts what Read rejects, so a malformed payload leaves the service with no error. Keep a write-time guard for required non-nullable reference-type properties with the corrected "not nullable" message, or enforce the contract (e.g., `required` properties) so the NRT assumption actually holds.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/SimpleQuadrilateral.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/SimpleQuadrilateral.cs:192">
P2: For required non-nullable string properties, the removed guards mean a null value now serializes as `"quadrilateralType": null` (Utf8JsonWriter.WriteString writes JSON null) instead of throwing. The PR premise "we can trust the NRT" only holds when nullable reference types are enforced, but this generated project's csproj does not enable `<Nullable>`, and these are public setters users can assign null through. Keep a runtime guard for required reference-type properties with the corrected "not nullable" message, or enable NRT so the trust premise actually holds.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/Whale.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/Whale.cs:213">
P2: Removing this guard means a required non-nullable field no longer fails fast at the serialization boundary. `ClassName` has a public setter, so a null can still reach `WriteProperties` at runtime (e.g. `whale.ClassName = null` with a suppressed warning or `new Whale(null!)`); previously this threw a clear `ArgumentNullException`, now `WriteString("className", whale.ClassName)` silently emits `"className": null`. The NRT guarantee only holds for well-typed code, so confirm the intended behavior for a null required value is to write null rather than throw.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/TestDescendants.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/TestDescendants.cs:219">
P2: Removing this guard means a required, non-nullable property can now be silently serialized as null. The NRT guarantee does not hold here: AlternativeName has a public setter and the constructor accepts the value without validation, so null can reach the writer at runtime (e.g. `new TestDescendants(null)` or `model.AlternativeName = null`). Previously this threw ArgumentNullException; now it writes `"alternativeName": null` into the JSON, which a compliant server rejects as a missing required field. If the intent is to keep trusting NRT for required fields, consider at least validating the constructor parameter, since the setter makes runtime null reachable.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/BasquePig.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/BasquePig.cs:173">
P2: Removing this guard means a required non-nullable property (ClassName) that is null at runtime is now silently serialized as `"className":null`, which violates the schema (required + not nullable) and is rejected by servers, instead of failing fast with an exception. The "trust the NRT" premise does not hold at runtime: `ClassName` has a public setter and NRT is a compile-time-only annotation, so null can still reach serialization through user mutation, reflection, or consumers without nullable analysis. Note the generated template still guards optional non-nullable properties but now skips required ones—the exact properties that most need protection. Consider keeping a guard with the corrected message ("Property is not nullable...") rather than removing it.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/IsoscelesTriangle.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Model/IsoscelesTriangle.cs:185">
P2: When ShapeType or TriangleType is null at serialization time (reachable via the public setter, an object initializer, or reflection despite the NRT annotation), WriteProperties now silently writes "shapeType":null / "triangleType":null instead of throwing. That produces schema-invalid JSON for required non-nullable properties, and it is inconsistent with the optional-property path, which still throws JsonException for a null non-nullable value. Consider keeping a guard for required non-nullable properties but with the corrected "not nullable" message rather than removing it entirely.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/RequiredClass.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/RequiredClass.cs:2233">
P2: Removing the null guards on required non-nullable properties means a null value now serializes silently as JSON `null` instead of being detected. NRT annotations are compile-time hints only and are not enforced at runtime, so a required property can still be null (e.g. `new Pet(null, null)` or an object initializer). For reference-type required fields such as `RequiredNotnullableArrayOfString`, `JsonSerializer.Serialize(writer, ...)` writes the `null` token, and for required strings `Utf8JsonWriter.WriteString("name", null)` also emits `null` (verified against the .NET runtime source), so the output violates the required/non-nullable contract without any error. This contradicts the very check you are keeping for optional non-nullable properties in the same method (the new `JsonException` throws). Consider keeping a null guard for required properties but with the corrected "property is not nullable" message.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/FormatTest.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/FormatTest.cs:949">
P2: The required non-nullable guards for `FormatTest.Byte` and `FormatTest.Password` were removed, so a null value is now silently serialized as JSON `null` for these required properties instead of throwing (same for `IsoscelesTriangle.ShapeType`/`TriangleType`). This relies on NRT, which is compile-time only: `new FormatTest { Byte = null }` or `f.Byte = null;` compile (with a warning) because the setters are public and mutable, so a null can still reach `writer.WriteString("password", formatTest.Password)` / `JsonSerializer.Serialize(writer, formatTest.Byte, ...)` and emit an invalid `"byte": null` for a required field. It is also inconsistent with the retained optional-property guards on the surrounding lines, which still throw `JsonException` for the same set-but-null condition, and with the Read converter (JsonConverter.mustache:297) which still rejects missing/required values.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/csharp/libraries/generichost/JsonConverter.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/csharp/libraries/generichost/JsonConverter.mustache:459">
P2: Required non-nullable reference-type properties lose their serialization null guard entirely. The old code checked `prop == null` for required properties too; now the guard is emitted only inside `{{^required}}`. NRT is compile-time only, and the generated model exposes public setters (and nulls can come from reflection or `null!`), so a required property can be null at runtime. Serializing it now silently writes a JSON null (for strings) or throws a confusing NullReferenceException deeper in the write path instead of the previous clear ArgumentNullException. Consider keeping the null check for required properties (it was not the source of the wrong error message being fixed), or otherwise document the deliberate loss of this runtime check.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/AppleReq.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/AppleReq.cs:190">
P2: Removing the null guard for the required `Cultivar` property lets serialization silently emit `"cultivar": null` instead of throwing. `Cultivar` has a public setter and an unvalidated constructor, so it can be null at runtime, and `Utf8JsonWriter.WriteString(propertyName, null)` writes a JSON null rather than failing. This now contradicts the deserializer, which still throws "Property is not nullable" for a null cultivar. Consider keeping a guard that throws `JsonException` for the null case so invalid state is reported instead of serialized.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
3 existing issues remain and no new issues found across 1508 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Re-trigger cubic
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.
The error message incorrectly said property is required instead of property is not nullable. Also changed to only check ^required because we can trust the NRT. Also removed the reference type check because the validation is good on nullable value types as well.
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
Summary by cubic
Fixes the misleading serialization error message in the C# generichost client when writing null to a non-nullable property.
ArgumentNullExceptiontoJsonException.Written for commit 1d1cb76. Summary will update on new commits.