[csharp][generichost] Improve date deserialization - #25022
Draft
devhl-labs wants to merge 2 commits into
Draft
devhl-labs wants to merge 2 commits into
devhl-labs 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.
All reported issues were addressed across 169 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
6 issues found across 171 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/petstore/csharp/generichost/net9/OneOf/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/OneOf/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs:33">
P2: `JsonSerializer.Deserialize<DateTime>` removes the converter's UTC normalization: offset-less values become `Unspecified` and offset values become local. Preserve `AdjustToUniversal | AssumeUniversal` so downstream timestamp handling does not reinterpret API values.</violation>
<violation number="2" location="samples/client/petstore/csharp/generichost/net9/OneOf/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs:47">
P2: This delegation changes the serialized wire format from fixed seven-digit fractions to trimmed fractions, such as `.0000000Z` becoming `Z`. Preserve the explicit format unless this breaking payload change is intentional and documented.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs:33">
P1: This delegation rejects the compact `yyyyMMddTHHmmss...` timestamps that the converter previously accepted. Preserve those formats before falling back to the built-in parser, or explicitly document this breaking wire-format change.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs:47">
P2: This rejects values that the converter itself serializes: `Write` emits no timezone suffix for `DateTimeKind.Unspecified`, while this branch rejects the resulting offset-less timestamp and the fallback array is empty. Preserve the existing offset-less compatibility path or serialize unspecified values with an explicit UTC offset so converter round-trips do not throw.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs:48">
P3: `string value = reader.GetString()` runs before the `TryGetDateTime` fast path even though the standard RFC 3339-with-offset case returns immediately without using `value`. Move it below the fast-path check so the common path avoids decoding and allocating the string. `TryGetDateTime` operates on the current token without advancing the reader, so the reorder is behavior-neutral.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs:33">
P3: The nullable converters now delegate `Read`/`Write` to `JsonSerializer.Deserialize<DateTime>(ref reader, options)` / `Serialize(writer, dateTimeValue.Value, options)`, so their parsing and output behavior is fully determined by whether the non-nullable converter happens to be registered in `options`. This works in the generated clients (both converter types are registered together), but the removed public `Formats` on the nullable converters means users who customize the JSON options and register only the nullable converters silently lose the format-based fallback and get plain ISO-only parsing. Instantiate the non-nullable converter explicitly (e.g. `new DateTimeJsonConverter().Read(...)`) so the nullable path no longer depends on the options registry.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return result; | ||
|
|
||
| return null; | ||
| return JsonSerializer.Deserialize<DateTime>(ref reader, options); |
Contributor
There was a problem hiding this comment.
P1: This delegation rejects the compact yyyyMMddTHHmmss... timestamps that the converter previously accepted. Preserve those formats before falling back to the built-in parser, or explicitly document this breaking wire-format change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs, line 33:
<comment>This delegation rejects the compact `yyyyMMddTHHmmss...` timestamps that the converter previously accepted. Preserve those formats before falling back to the built-in parser, or explicitly document this breaking wire-format change.</comment>
<file context>
@@ -54,16 +30,7 @@ public class DateTimeNullableJsonConverter : JsonConverter<DateTime?>
- return result;
-
- throw new JsonException("The JSON value is not a valid date-time.");
+ return JsonSerializer.Deserialize<DateTime>(ref reader, options);
}
</file context>
| writer.WriteNullValue(); | ||
| else | ||
| writer.WriteStringValue(dateTimeValue.Value.ToString("yyyy'-'MM'-'dd'T'HH':'mm':'ss'.'fffffffK", CultureInfo.InvariantCulture)); | ||
| JsonSerializer.Serialize(writer, dateTimeValue.Value, options); |
Contributor
There was a problem hiding this comment.
P2: This delegation changes the serialized wire format from fixed seven-digit fractions to trimmed fractions, such as .0000000Z becoming Z. Preserve the explicit format unless this breaking payload change is intentional and documented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/net9/OneOf/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs, line 47:
<comment>This delegation changes the serialized wire format from fixed seven-digit fractions to trimmed fractions, such as `.0000000Z` becoming `Z`. Preserve the explicit format unless this breaking payload change is intentional and documented.</comment>
<file context>
@@ -77,7 +44,7 @@ public override void Write(Utf8JsonWriter writer, DateTime? dateTimeValue, JsonS
writer.WriteNullValue();
else
- writer.WriteStringValue(dateTimeValue.Value.ToString("yyyy'-'MM'-'dd'T'HH':'mm':'ss'.'fffffffK", CultureInfo.InvariantCulture));
+ JsonSerializer.Serialize(writer, dateTimeValue.Value, options);
}
}
</file context>
Suggested change
| JsonSerializer.Serialize(writer, dateTimeValue.Value, options); | |
| writer.WriteStringValue(dateTimeValue.Value.ToString("yyyy'-'MM'-'dd'T'HH':'mm':'ss'.'fffffffK", System.Globalization.CultureInfo.InvariantCulture)); |
| return result; | ||
|
|
||
| return null; | ||
| return JsonSerializer.Deserialize<DateTime>(ref reader, options); |
Contributor
There was a problem hiding this comment.
P2: JsonSerializer.Deserialize<DateTime> removes the converter's UTC normalization: offset-less values become Unspecified and offset values become local. Preserve AdjustToUniversal | AssumeUniversal so downstream timestamp handling does not reinterpret API values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/net9/OneOf/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs, line 33:
<comment>`JsonSerializer.Deserialize<DateTime>` removes the converter's UTC normalization: offset-less values become `Unspecified` and offset values become local. Preserve `AdjustToUniversal | AssumeUniversal` so downstream timestamp handling does not reinterpret API values.</comment>
<file context>
@@ -54,16 +30,7 @@ public class DateTimeNullableJsonConverter : JsonConverter<DateTime?>
- return result;
-
- throw new JsonException("The JSON value is not a valid date-time.");
+ return JsonSerializer.Deserialize<DateTime>(ref reader, options);
}
</file context>
| // System.Text.Json also accepts offset-less ISO 8601 values, but OpenAPI | ||
| // date-time values follow RFC 3339, which requires an offset. Let those | ||
| // non-standard values fall through so custom formats can explicitly permit them. | ||
| if (reader.TryGetDateTime(out DateTime dateTime) && dateTime.Kind != DateTimeKind.Unspecified) |
Contributor
There was a problem hiding this comment.
P2: This rejects values that the converter itself serializes: Write emits no timezone suffix for DateTimeKind.Unspecified, while this branch rejects the resulting offset-less timestamp and the fallback array is empty. Preserve the existing offset-less compatibility path or serialize unspecified values with an explicit UTC offset so converter round-trips do not throw.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/net9/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs, line 47:
<comment>This rejects values that the converter itself serializes: `Write` emits no timezone suffix for `DateTimeKind.Unspecified`, while this branch rejects the resulting offset-less timestamp and the fallback array is empty. Preserve the existing offset-less compatibility path or serialize unspecified values with an explicit UTC offset so converter round-trips do not throw.</comment>
<file context>
@@ -52,15 +36,24 @@ public class DateTimeJsonConverter : JsonConverter<DateTime>
+ // System.Text.Json also accepts offset-less ISO 8601 values, but OpenAPI
+ // date-time values follow RFC 3339, which requires an offset. Let those
+ // non-standard values fall through so custom formats can explicitly permit them.
+ if (reader.TryGetDateTime(out DateTime dateTime) && dateTime.Kind != DateTimeKind.Unspecified)
+ return dateTime.ToUniversalTime();
+
</file context>
| // System.Text.Json also accepts offset-less ISO 8601 values, but OpenAPI | ||
| // date-time values follow RFC 3339, which requires an offset. Let those | ||
| // non-standard values fall through so custom formats can explicitly permit them. | ||
| if (reader.TryGetDateTime(out DateTime dateTime) && dateTime.Kind != DateTimeKind.Unspecified) |
Contributor
There was a problem hiding this comment.
P3: string value = reader.GetString() runs before the TryGetDateTime fast path even though the standard RFC 3339-with-offset case returns immediately without using value. Move it below the fast-path check so the common path avoids decoding and allocating the string. TryGetDateTime operates on the current token without advancing the reader, so the reorder is behavior-neutral.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/net4.7/Petstore/src/Org.OpenAPITools/Client/DateTimeJsonConverter.cs, line 48:
<comment>`string value = reader.GetString()` runs before the `TryGetDateTime` fast path even though the standard RFC 3339-with-offset case returns immediately without using `value`. Move it below the fast-path check so the common path avoids decoding and allocating the string. `TryGetDateTime` operates on the current token without advancing the reader, so the reorder is behavior-neutral.</comment>
<file context>
@@ -54,15 +37,24 @@ public class DateTimeJsonConverter : JsonConverter<DateTime>
+ // System.Text.Json also accepts offset-less ISO 8601 values, but OpenAPI
+ // date-time values follow RFC 3339, which requires an offset. Let those
+ // non-standard values fall through so custom formats can explicitly permit them.
+ if (reader.TryGetDateTime(out DateTime dateTime) && dateTime.Kind != DateTimeKind.Unspecified)
+ return dateTime.ToUniversalTime();
+
</file context>
| return result; | ||
|
|
||
| return null; | ||
| return JsonSerializer.Deserialize<DateTime>(ref reader, options); |
Contributor
There was a problem hiding this comment.
P3: The nullable converters now delegate Read/Write to JsonSerializer.Deserialize<DateTime>(ref reader, options) / Serialize(writer, dateTimeValue.Value, options), so their parsing and output behavior is fully determined by whether the non-nullable converter happens to be registered in options. This works in the generated clients (both converter types are registered together), but the removed public Formats on the nullable converters means users who customize the JSON options and register only the nullable converters silently lose the format-based fallback and get plain ISO-only parsing. Instantiate the non-nullable converter explicitly (e.g. new DateTimeJsonConverter().Read(...)) so the nullable path no longer depends on the options registry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs, line 33:
<comment>The nullable converters now delegate `Read`/`Write` to `JsonSerializer.Deserialize<DateTime>(ref reader, options)` / `Serialize(writer, dateTimeValue.Value, options)`, so their parsing and output behavior is fully determined by whether the non-nullable converter happens to be registered in `options`. This works in the generated clients (both converter types are registered together), but the removed public `Formats` on the nullable converters means users who customize the JSON options and register only the nullable converters silently lose the format-based fallback and get plain ISO-only parsing. Instantiate the non-nullable converter explicitly (e.g. `new DateTimeJsonConverter().Read(...)`) so the nullable path no longer depends on the options registry.</comment>
<file context>
@@ -54,16 +30,7 @@ public class DateTimeNullableJsonConverter : JsonConverter<DateTime?>
- return result;
-
- throw new JsonException("The JSON value is not a valid date-time.");
+ return JsonSerializer.Deserialize<DateTime>(ref reader, options);
}
</file context>
devhl-labs
marked this pull request as draft
September 27, 2026 23:06
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
1 participant
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.
Summary
Normalize JSON date and date-time deserialization in the C# Generic Host client and ensure invalid non-null values fail consistently with
JsonException.Previously, nullable converters could silently return
nullfor malformed values, making an invalid server response indistinguishable from an actual JSONnull. Parsing was also limited to a fixed set of exact formats, rejecting valid RFC 3339 timestamps with more than seven fractional digits.Behavior
nullremains valid for nullable date and date-time properties.JsonExceptionfor nullable and non-nullable properties.System.Text.Jsonand normalized to UTC.date-timefollows RFC 3339, which requires an offset.date-time: RFC 3339 values withZor a numeric offset.date:yyyy-MM-dd.The customizable format partial remains available for users who intentionally need to permit compatibility formats, including offset-less values. Such values are interpreted as UTC through the existing fallback parsing policy.
Nullable converters now handle only JSON
nullthemselves. Non-null values are delegated through the configuredJsonSerializerOptions, ensuring nullable and non-nullable properties use the same converter behavior without bypassing the configured serialization pipeline.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
Improves the C# generichost date and date-time JSON converters so invalid values throw a descriptive
JsonExceptioninstead of a genericNotSupportedExceptionor silently returning null. Generated samples are updated.Behavior changes
JsonExceptionwith a clear message instead ofNotSupportedExceptionor returningnull.yyyyMMddvariants; offset-less ISO values fall back to the custom formats.Written for commit dcc02cc. Summary will update on new commits.