fix: (.NET) Improve json De/serialization #1138
Conversation
Replaces switch on JsonTokenType with a recursive method using JsonElement.ValueKind for more robust and accurate type inference. This improves handling of nested objects and arrays, and unifies the logic for converting JSON values to .NET types.
Simplifies and standardizes the deserialization of model properties from dictionaries, removing special handling for JsonElement and streamlining array and primitive type conversions. This improves code readability and maintainability in generated model classes.
Fellmonkey
changed the title
Updated the From method in the model template to check for the existence of optional properties in the input map before assigning values. This prevents errors when optional properties are missing from the input dictionary. (for examle in model: User, :-/ )
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR refactors JSON serialization and deserialization in the .NET SDK template to fix critical bugs and improve robustness, particularly addressing infinite recursion issues and type inference problems.
- Fixed infinite recursion bug in
ObjectToInferredTypesConverterthat causedStackOverflowException - Improved JSON type inference by switching from
JsonTokenTypetoJsonElement.ValueKind - Streamlined model deserialization by removing special
JsonElementhandling and standardizing type conversions
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/dotnet/Package/Role.cs.twig | Updated namespace to use dynamic spec title |
| templates/dotnet/Package/Models/Model.cs.twig | Simplified model deserialization logic, removed JsonElement handling |
| templates/dotnet/Package/Models/InputFile.cs.twig | Updated namespace import to use dynamic spec title |
| templates/dotnet/Package/Exception.cs.twig | Added blank line formatting |
| templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig | Complete rewrite to fix recursion bug and improve type inference |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fellmonkey
force-pushed
the
improve-json-serialization
branch
from
cf7eb57 to
b071cbc
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughObjectToInferredTypesConverter.Read now returns Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/dotnet/Package/Query.cs.twig (1)
153-159: Validate deserialization in Or/And to avoid nulls in ValuesJsonSerializer.Deserialize can return null on malformed input; currently this would silently serialize null entries. Fail fast with a clear error.
public static string Or(List<string> queries) { - return new Query("or", null, queries.Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions)).ToList()).ToString(); + var parsed = queries + .Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions) ?? throw new JsonException("Invalid query JSON in Or()")) + .ToList(); + return new Query("or", null, parsed).ToString(); } public static string And(List<string> queries) { - return new Query("and", null, queries.Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions)).ToList()).ToString(); + var parsed = queries + .Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions) ?? throw new JsonException("Invalid query JSON in And()")) + .ToList(); + return new Query("and", null, parsed).ToString(); }templates/dotnet/Package/Extensions/Extensions.cs.twig (1)
15-38: Correct query encoding and culture-invariant formattingEncoding the entire string via Uri.EscapeUriString risks malformed queries and culture-specific number formats. Encode components with Uri.EscapeDataString and format numbers with InvariantCulture; normalize booleans to lowercase.
- public static string ToQueryString(this Dictionary<string, object?> parameters) - { - var query = new List<string>(); - - foreach (var kvp in parameters) - { - switch (kvp.Value) - { - case null: - continue; - case IList list: - foreach (var item in list) - { - query.Add($"{kvp.Key}[]={item}"); - } - break; - default: - query.Add($"{kvp.Key}={kvp.Value.ToString()}"); - break; - } - } - - return Uri.EscapeUriString(string.Join("&", query)); - } + public static string ToQueryString(this Dictionary<string, object?> parameters) + { + string Encode(object? v) => + v switch + { + null => string.Empty, + bool b => b ? "true" : "false", + IFormattable f => f.ToString(null, System.Globalization.CultureInfo.InvariantCulture), + _ => v.ToString() ?? string.Empty + }; + + var parts = new List<string>(); + + foreach (var kvp in parameters) + { + switch (kvp.Value) + { + case null: + continue; + case IList list when kvp.Key is not null: + foreach (var item in list) + { + parts.Add($"{kvp.Key}[]={Uri.EscapeDataString(Encode(item))}"); + } + break; + default: + parts.Add($"{kvp.Key}={Uri.EscapeDataString(Encode(kvp.Value))}"); + break; + } + } + + return string.Join("&", parts); + }
♻️ Duplicate comments (2)
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig (1)
72-73: EOF newline restored.Matches prior request to add terminal newline.
templates/dotnet/Package/Models/Model.cs.twig (1)
51-51: Remove null-forgiving and tame the long LINQ chain.ToList() never returns null; the bang is unnecessary. Also, this line is hard to read—prior feedback still applies.
- ((IEnumerable<object>)map["{{ property.name }}"]).Select(x => {% if property.items.type == "string" %}x?.ToString(){% elseif property.items.type == "integer" %}{% if not property.required %}x == null ? (long?)null : {% endif %}Convert.ToInt64(x){% elseif property.items.type == "number" %}{% if not property.required %}x == null ? (double?)null : {% endif %}Convert.ToDouble(x){% elseif property.items.type == "boolean" %}{% if not property.required %}x == null ? (bool?)null : {% endif %}(bool)x{% else %}x{% endif %}).{% if property.items.type == "string" and property.required %}Where(x => x != null).{% endif %}ToList()! + ((IEnumerable<object>)map["{{ property.name }}"]).Select(x => {% if property.items.type == "string" %}x?.ToString(){% elseif property.items.type == "integer" %}{% if not property.required %}x == null ? (long?)null : {% endif %}Convert.ToInt64(x){% elseif property.items.type == "number" %}{% if not property.required %}x == null ? (double?)null : {% endif %}Convert.ToDouble(x){% elseif property.items.type == "boolean" %}{% if not property.required %}x == null ? (bool?)null : {% endif %}(bool)x{% else %}x{% endif %}).{% if property.items.type == "string" and property.required %}Where(x => x != null).{% endif %}ToList()Optional: mirror the safe-cast pattern from Lines 45-48 for optionals.
🧹 Nitpick comments (8)
templates/dotnet/Package/Query.cs.twig (1)
25-42: Broaden handling of enumerable values in constructorToday only IList is expanded; IEnumerable (e.g., LINQ results, HashSet) are treated as a single value. Safely broaden to IEnumerable while avoiding strings.
public Query(string method, string? attribute, object? values) { this.Method = method; this.Attribute = attribute; - if (values is IList valuesList) + if (values is IList valuesList) { this.Values = new List<object>(); foreach (var value in valuesList) { this.Values.Add(value); // Automatically boxes if value is a value type } } - else if (values != null) + else if (values is IEnumerable enumerable && values is not string) + { + this.Values = new List<object>(); + foreach (var value in enumerable) + { + this.Values.Add(value!); + } + } + else if (values != null) { this.Values = new List<object> { values }; } }templates/dotnet/Package/Extensions/Extensions.cs.twig (3)
1-5: Add missing using for CultureInfo (if not already imported elsewhere)Required by the proposed InvariantCulture formatting.
using System; using System.Collections; using System.Collections.Generic; using System.Text.Json; +using System.Globalization;
40-41: Make mappings readonlyThis is a constant lookup table; prevent accidental mutation.
- private static IDictionary<string, string> _mappings = new Dictionary<string, string>(StringComparer.InvariantCultureIgnoreCase) { + private static readonly IDictionary<string, string> _mappings = new Dictionary<string, string>(StringComparer.InvariantCultureIgnoreCase) {
607-612: Use nameof in ArgumentNullExceptionMinor clarity/readability improvement.
- if (extension == null) - { - throw new ArgumentNullException("extension"); - } + if (extension == null) + { + throw new ArgumentNullException(nameof(extension)); + }templates/dotnet/Package/Models/InputFile.cs.twig (1)
14-21: Guard against invalid pathsOptional: validate input to avoid surprising runtime errors and provide clearer messages.
- public static InputFile FromPath(string path) => new InputFile + public static InputFile FromPath(string path) + { + if (string.IsNullOrWhiteSpace(path)) + throw new ArgumentException("Path must be a non-empty string.", nameof(path)); + return new InputFile { Path = path, Filename = System.IO.Path.GetFileName(path), MimeType = path.GetMimeType(), SourceType = "path" - }; + }; + }templates/dotnet/Package/Exception.cs.twig (1)
22-25: Constructor overload looks good; consider serialization support and parity overloads.Nice addition. Two optional tweaks:
- Add [Serializable] + protected (SerializationInfo, StreamingContext) ctor for Exception best practices.
- Consider an overload that also accepts code/type/response with an inner exception if those are commonly set when wrapping.
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig (2)
38-44: Prefer DateTimeOffset (or skip auto date coercion) to preserve offsets.Parsing string → DateTime can lose timezone info or affect round-tripping when models expect strings. Either:
- Try DateTimeOffset first, then DateTime, else keep string.
- Or, keep all strings as strings (let models decide). First option shown below.
- case JsonValueKind.String: - if (element.TryGetDateTime(out DateTime datetime)) - { - return datetime; - } - return element.GetString(); + case JsonValueKind.String: + if (element.TryGetDateTimeOffset(out DateTimeOffset dto)) + { + return dto; + } + if (element.TryGetDateTime(out DateTime dt)) + { + return dt; + } + return element.GetString();
45-51: Avoid precision loss for large/monetary numbers.Fall back to decimal before double to preserve precision; Convert.* in models will still handle decimal.
case JsonValueKind.Number: if (element.TryGetInt64(out long l)) { return l; } - return element.GetDouble(); + if (element.TryGetDecimal(out decimal dec)) + { + return dec; + } + return element.GetDouble();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig(1 hunks)templates/dotnet/Package/Exception.cs.twig(1 hunks)templates/dotnet/Package/Extensions/Extensions.cs.twig(1 hunks)templates/dotnet/Package/Models/InputFile.cs.twig(2 hunks)templates/dotnet/Package/Models/Model.cs.twig(1 hunks)templates/dotnet/Package/Models/UploadProgress.cs.twig(1 hunks)templates/dotnet/Package/Query.cs.twig(1 hunks)templates/dotnet/Package/Role.cs.twig(2 hunks)
🔇 Additional comments (3)
templates/dotnet/Package/Models/UploadProgress.cs.twig (1)
1-26: LGTM — formatting onlyNo functional changes. Safe.
templates/dotnet/Package/Role.cs.twig (1)
1-92: LGTM — namespace templatingNamespace templating aligns with the rest of the PR; no functional changes to Role API.
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig (1)
10-16: Good fix: eliminates recursion/StackOverflow and handles nulls cleanly.Using JsonDocument + a single pass is safer and clearer. No issues here.
Improves the From() method in Model.cs.twig to handle nullable and array properties more robustly, using helper macros for parsing arrays and sub-schemas. This change ensures correct handling of optional fields and type conversions, reducing runtime errors and improving code maintainability. Also removes an unnecessary blank line in ServiceTemplate.cs.twig.
|
Hello @ChiragAgg5k, I made some minor adjustments, wrote some tests, and noticed that two arguments that are required in the code in the API are actually missing. It would be best to make them null-safe or see why the API is not returning them. 
|
Fields with null values in multipart are now omitted (so they don't turn into empty strings).
|
Fields with null values in multipart are now omitted (so they don't turn into empty strings). |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/dotnet/Package/Client.cs.twig (1)
457-495: Fix chunking: off‑by‑one and buffer misuse cause data corruption.
- bytes case drops 1 byte per chunk (ChunkSize-1).
- stream case ignores the actual bytes read and sends the entire buffer.
- Content-Range end and offset advance don’t reflect the actual bytes sent.
These bugs can corrupt uploads. Use the actual bytes to send per chunk.
- while (offset < size) + while (offset < size) { - switch(input.SourceType) - { - case "path": - case "stream": - var stream = input.Data as Stream; - if (stream == null) - throw new InvalidOperationException("Stream data is null"); - stream.Seek(offset, SeekOrigin.Begin); - await stream.ReadAsync(buffer, 0, ChunkSize); - break; - case "bytes": - buffer = ((byte[])input.Data) - .Skip((int)offset) - .Take((int)Math.Min(size - offset, ChunkSize - 1)) - .ToArray(); - break; - } - - var content = new MultipartFormDataContent { - { new ByteArrayContent(buffer), paramName, input.Filename } - }; + var remaining = (int)Math.Min(size - offset, ChunkSize); + int bytesToSend = remaining; + HttpContent filePart; + switch (input.SourceType) + { + case "path": + case "stream": + var stream = input.Data as Stream; + if (stream == null) + throw new InvalidOperationException("Stream data is null"); + stream.Seek(offset, SeekOrigin.Begin); + var bytesRead = 0; + while (bytesRead < remaining) + { + var read = await stream.ReadAsync(buffer, bytesRead, remaining - bytesRead); + if (read == 0) break; + bytesRead += read; + } + bytesToSend = bytesRead; + filePart = new ByteArrayContent(buffer, 0, bytesToSend); + break; + case "bytes": + var data = input.Data as byte[]; + if (data == null) + throw new InvalidOperationException("Byte array data is null"); + filePart = new ByteArrayContent(data, (int)offset, remaining); + break; + default: + throw new InvalidOperationException($"Unsupported source type: {input.SourceType}"); + } + + var content = new MultipartFormDataContent { + { filePart, paramName, input.Filename } + }; parameters[paramName] = content; - headers["Content-Range"] = - $"bytes {offset}-{Math.Min(offset + ChunkSize - 1, size - 1)}/{size}"; + var end = Math.Min(offset + bytesToSend - 1, size - 1); + headers["Content-Range"] = $"bytes {offset}-{end}/{size}"; result = await Call<Dictionary<string, object?>>( method: "POST", path, headers, parameters ); - offset += ChunkSize; + offset += bytesToSend;
🧹 Nitpick comments (3)
templates/dotnet/Package/Client.cs.twig (3)
157-176: Skip-nulls is good; also support arbitrary file param names (not only "file").Current logic only treats key "file". If an API uses a different param name (e.g., "image"), the file won’t be added. Detect MultipartFormDataContent by type instead of key so uploads work regardless of parameter name.
foreach (var parameter in parameters) { - if (parameter.Value == null) continue; - if (parameter.Key == "file") - { - var fileContent = parameters["file"] as MultipartFormDataContent; - if (fileContent != null) - { - form.Add(fileContent.First()!); - } - } + if (parameter.Value == null) continue; + if (parameter.Value is MultipartFormDataContent fileContent) + { + var part = fileContent.FirstOrDefault(); + if (part != null) + { + form.Add(part); + } + continue; + } else if (parameter.Value is IEnumerable<object> enumerable) { var list = new List<object>(enumerable); for (int index = 0; index < list.Count; index++) { form.Add(new StringContent(list[index]?.ToString() ?? string.Empty), $"{parameter.Key}[{index}]"); } } else { form.Add(new StringContent(parameter.Value?.ToString() ?? string.Empty), parameter.Key); } }
189-203: Avoid growing Accept headers on the shared HttpClient; set per-request instead.Each call adds another Accept value to DefaultRequestHeaders and never clears it. Prefer setting Accept only on the request (already done below) and skip touching DefaultRequestHeaders here.
foreach (var header in _headers) { - if (header.Key.Equals("content-type", StringComparison.OrdinalIgnoreCase)) - { - _http.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue(header.Value)); - } - else + if (!header.Key.Equals("content-type", StringComparison.OrdinalIgnoreCase)) { if (_http.DefaultRequestHeaders.Contains(header.Key)) { _http.DefaultRequestHeaders.Remove(header.Key); } _http.DefaultRequestHeaders.Add(header.Key, header.Value); } }
142-144: Avoid trailing “?” when no GET parameters
ToQueryString already skips nulls—only prefix with “?” if it returns non-empty:var rawQuery = parameters.ToQueryString(); var queryString = methodGet && rawQuery.Length > 0 ? "?" + rawQuery : string.Empty;
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
107-109:ToMapnullifies optional sub-modelsOptional nested models (and arrays of models) can legitimately stay
nullafterFrom(...), butToMap()invokes.Select(...)/.ToMap()unconditionally, so the first access on a null property throws aNullReferenceException. Please add null-conditionals so optional sub-models remainnullwhen re-serialized instead of crashing.- { "{{ property.name }}", {% if property.sub_schema %}{% if property.type == 'array' %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}.Select(it => it.ToMap()){% else %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}.ToMap(){% endif %}{% else %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}{% endif %}{{ ' }' }}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %} + { "{{ property.name }}", {% if property.sub_schema %}{% if property.type == 'array' %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.Select(it => it.ToMap()){% else %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.ToMap(){% endif %}{% else %}{{ _self.property_name(definition, property) | overrideProperty(definition.name) }}{% endif %}{{ ' }' }}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
43-43: Critical: TryGetValue guard still missing null check.Past reviews flagged that
TryGetValuesucceeds even when the value is explicitlynull. The current guard will execute conversion logic on null values, causing:
Convert.ToInt64(null)→0(notnull)- Optional arrays become
[]instead ofnull- Loss of distinction between
nulland default valuesThis breaks nullable property semantics.
Apply this diff to add the null guard:
- {%- if not property.required -%}map.TryGetValue("{{ property.name }}", out var {{ v }}) ? {% endif %} + {%- if not property.required -%}map.TryGetValue("{{ property.name }}", out var {{ v }}) && {{ v }} is not null ? {% endif %}This ensures conversion logic only runs when the value is present AND non-null.
🧹 Nitpick comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
89-93: Consider ISO 8601 format for DateTime values.Past reviews noted that if the converter produces
DateTimeorDateTimeOffsetobjects, callingToString()is culture-dependent and may not round-trip correctly.For string properties that might contain DateTime values, consider using
ToString("O")for ISO 8601 format:{%- if not property.required -%} {{ v }} switch { DateTimeOffset dto => dto.ToString("O"), DateTime dt => dt.ToUniversalTime().ToString("O"), _ => {{ v }}?.ToString() } {%- else -%} {{ mapAccess | raw }} switch { DateTimeOffset dto => dto.ToString("O"), DateTime dt => dt.ToUniversalTime().ToString("O"), _ => {{ mapAccess | raw }}.ToString() } {%- endif %}However, this may not be necessary if the converter always produces strings for string properties. Verify converter behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
templates/dotnet/Package/Client.cs.twig(1 hunks)templates/dotnet/Package/Models/Model.cs.twig(4 hunks)templates/dotnet/Package/Query.cs.twig(1 hunks)templates/dotnet/Package/Services/ServiceTemplate.cs.twig(0 hunks)
💤 Files with no reviewable changes (1)
- templates/dotnet/Package/Services/ServiceTemplate.cs.twig
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/dotnet/Package/Client.cs.twig
- templates/dotnet/Package/Query.cs.twig
🔇 Additional comments (7)
templates/dotnet/Package/Models/Model.cs.twig (7)
10-10: LGTM: Consistent use of DefinitionClass.The class declaration, constructor, and static From method correctly use the {{ DefinitionClass }} template variable consistently.
Also applies to: 21-21, 38-38
52-58: LGTM: Safe casting for optional sub-schemas.The optional branch correctly uses safe casting with pattern matching:
as Dictionary<string, object>preventsInvalidCastException- Pattern match
is { } objensures non-null before callingFrom- Returns
nullwhen value is missing or wrong typeThe required branch uses direct casting, which will fail fast on malformed data—acceptable for required properties.
60-70: LGTM: Enum handling correctly checks for null.The optional enum branch correctly guards against null values:
- Uses
TryGetValue- Explicitly checks
enumRaw{{ loop.index }} == null- Returns
nullif key missing or value is nullNote: This branch uses its own variable (
enumRaw{{ loop.index }}) instead of the{{ v }}pattern from line 40, which is slightly inconsistent but functionally correct.
98-98: LGTM: Correct null coalescing for optional properties.The
: nullsuffix correctly completes the ternary operator for optional properties. Once the null guard on line 43 is fixed, this will properly returnnullfor:
- Missing keys
- Keys with null values
72-77: Ensure parse_primitive_array handles null safely.The
parse_primitive_arraymacro definition wasn’t found in the templates—please locate and verify it:
- accepts null input without throwing
- uses safe casts (e.g.,
as IEnumerable<object>)- returns null for optional properties when input is null
82-87: Boolean cast safety verified
TheObjectToInferredTypesConverterreturnstrue/falseforJsonValueKind.True/JsonValueKind.FalseandnullforJsonValueKind.Null, so(bool?)vwill only ever receive aboolornull. No changes required.
46-50: Verify parse_subschema_array is null-safe and uses safe castsOptional branch passes
v(may be null); confirm the macro:
- accepts null and returns null for optional properties,
- uses safe casts (e.g.,
as+ pattern-match like(... as IEnumerable<object>) is { } arr ? ...) instead of direct casts,- or, if the macro can't be changed, add a guard at templates/dotnet/Package/Models/Model.cs.twig (lines 46–50) to check/cast
vbefore calling the macro and return null when absent.
Introduces a new parse_value Twig function in DotNet.php to centralize and simplify value parsing logic for model properties. Updates Model.cs.twig to use this function, reducing template complexity and improving maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
44-46: Guard “present-but-null” optionals; avoid running conversion on null.
TryGetValue(...) ? ... : nullstill executes the conversion when the key exists with a null value, which then NREs in array branches. Require non-null before parsing.- {%- if not property.required -%}map.TryGetValue("{{ property.name }}", out var {{ v }}) ? {% endif -%} + {%- if not property.required -%}map.TryGetValue("{{ property.name }}", out var {{ v }}) && {{ v }} is not null ? {% endif -%} {{ parse_value(property, mapAccess, v) }} - {%- if not property.required %} : null{% endif -%} + {%- if not property.required %} : null{% endif -%}
🧹 Nitpick comments (5)
src/SDK/Language/DotNet.php (4)
470-470: Doc typo: “sub_scheme” → “sub_schema”.Fix the comment to match the function name and usage.
- * get sub_scheme, property_name and parse_value functions + * get sub_schema, property_name and parse_value functions
568-575: Use Convert for booleans; avoid invalid casts.Direct casts can throw if the backend returns “true”/“false” strings. Use
Convert.ToBooleanfor both required and optional.- if ($required) { - return "({$typeName}){$mapAccess}"; - } - return "({$typeName}?){$v}"; + if ($required) { + return "Convert.ToBoolean({$mapAccess})"; + } + return "{$v} is null ? (bool?)null : (bool?)Convert.ToBoolean({$v})";
532-536: Enum optional: minor nullability/culture nit.
ToString()on non-string may be culture-sensitive (numbers/dates). If enums are string-backed, considerConvert.ToString(..., CultureInfo.InvariantCulture)for consistency.- return "{$v} == null ? null : new {$enumClass}({$v}.ToString())"; + return "{$v} == null ? null : new {$enumClass}(Convert.ToString({$v}, System.Globalization.CultureInfo.InvariantCulture))";
578-582: String required: avoid “type name” strings for complex values.
{$mapAccess}.ToString()will produce type names for objects/arrays (e.g.,System.Collections.Generic.Dictionary...). If the backend can return non-strings here, considerConvert.ToString(..., InvariantCulture)or validate type before ToString.- return "{$mapAccess}.ToString()"; + return "Convert.ToString({$mapAccess}, System.Globalization.CultureInfo.InvariantCulture)";templates/dotnet/Package/Models/Model.cs.twig (1)
39-46: Constructor init via From(...): consider consistent Nullable flow for arrays.This relies on
parse_valueto return null for optionals. Ensure the array branch inparse_valueis updated to return null (not[]) whenvis null or not anIEnumerable<object>, to preserve semantics and avoid silent defaulting.Run the scan in my earlier comment and re-generate a sample model to validate that:
- Optional arrays missing or null in JSON produce
nullproperties.- Optional arrays with null elements don’t throw and omit nulls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SDK/Language/DotNet.php(3 hunks)templates/dotnet/Package/Models/Model.cs.twig(3 hunks)
Remove null-forgiving operator (!) from optional array mappings and use null-safe casting to preserve null vs empty semantics in generated models.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/SDK/Language/DotNet.php (2)
538-555: Critical: Null elements silently converted to defaults or cause exceptions.Past reviews identified that this code:
- Required arrays: Direct cast throws if source is null.
- Null elements:
Convert.ToInt64(null)returns0andConvert.ToDouble(null)returns0.0, silently converting nulls to default values.- Boolean/string:
(bool)xand.ToString()throw whenxis null.This can introduce incorrect data or runtime exceptions.
Filter null elements and add safe conversions:
if ($property['type'] === 'array') { $itemsType = $property['items']['type'] ?? 'object'; - $src = $required ? $mapAccess : $v; - $arraySource = $required - ? "((IEnumerable<object>){$src})" - : "({$src} as IEnumerable<object>)?"; $selectExpression = match($itemsType) { - 'string' => 'x.ToString()', - 'integer' => 'Convert.ToInt64(x)', - 'number' => 'Convert.ToDouble(x)', - 'boolean' => '(bool)x', + 'string' => 'x?.ToString() ?? string.Empty', + 'integer' => 'Convert.ToInt64(x)', + 'number' => 'Convert.ToDouble(x)', + 'boolean' => 'Convert.ToBoolean(x)', default => 'x' }; - return "{$arraySource}.Select(x => {$selectExpression}).ToList()"; + if ($required) { + return "((IEnumerable<object>){$mapAccess})" + . ".Where(x => x != null)" + . ".Select(x => {$selectExpression})" + . ".ToList()"; + } + return "({$v} as IEnumerable<object>)?" + . ".Where(x => x != null)" + . ".Select(x => {$selectExpression})" + . ".ToList()"; }Based on past review comments.
514-519: Critical: Unsafe dictionary cast and missing null guards in sub-schema array mapping.This code has three issues flagged in previous reviews:
- Required arrays: Direct cast
((IEnumerable<object>){$mapAccess})will throw if source is null.- Element casting:
(Dictionary<string, object>)itthrowsInvalidCastExceptionif any element is null or not a dictionary.- Optional arrays: While
?.Selecthandles null source correctly, null elements inside the array are not filtered.Apply a safe mapping pattern that filters null elements and validates types:
- if ($property['type'] === 'array') { - $arraySource = $required - ? "((IEnumerable<object>){$mapAccess})" - : "({$v} as IEnumerable<object>)?"; - return "{$arraySource}.Select(it => {$subSchema}.From(map: (Dictionary<string, object>)it)).ToList()"; + if ($property['type'] === 'array') { + if ($required) { + return "((IEnumerable<object>){$mapAccess})" + . ".Where(it => it is Dictionary<string, object>)" + . ".Select(it => {$subSchema}.From(map: (Dictionary<string, object>)it))" + . ".ToList()"; + } + return "({$v} as IEnumerable<object>)?" + . ".Where(it => it is Dictionary<string, object>)" + . ".Select(it => {$subSchema}.From(map: (Dictionary<string, object>)it))" + . ".ToList()";Based on past review comments.
🧹 Nitpick comments (1)
src/SDK/Language/DotNet.php (1)
557-565: Use TryParse for numeric conversions to avoid runtime exceptions.
At src/SDK/Language/DotNet.php lines 557–565, the generatedConvert.ToInt64({var})/Convert.ToDouble({var})calls will throwFormatExceptionorInvalidCastExceptionon non-numeric input. Consider emittingInt64.TryParse/Double.TryParsepatterns to validate and handle invalid values with clearer logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SDK/Language/DotNet.php(3 hunks)
🔇 Additional comments (2)
src/SDK/Language/DotNet.php (2)
470-470: LGTM!Documentation correctly updated to reflect the addition of the
parse_valuefunction.
497-497: LGTM!Correctly marks the function output as safe HTML for Twig template rendering.
Adds conditional import of the Enums namespace in the Model.cs.twig template only when the model definition contains enum properties. This prevents unnecessary imports and improves template clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
53-53: Use safe cast for additionalProperties to prevent InvalidCastException.The direct cast
(Dictionary<string, object>)dataValuewill throwInvalidCastExceptionifdataValueis null or not a dictionary. Use a safe cast with null handling.Apply this diff to add safe casting:
- data: map.TryGetValue("data", out var dataValue) ? (Dictionary<string, object>)dataValue : map + data: map.TryGetValue("data", out var dataValue) && dataValue is Dictionary<string, object> dict ? dict : map
🧹 Nitpick comments (1)
templates/dotnet/Package/Models/Model.cs.twig (1)
1-85: Architecture: Dictionary-based deserialization improves robustness.The refactoring from
JsonElement-based to dictionary-based deserialization aligns well with the PR objectives of eliminating JsonElement leakage and streamlining model deserialization. The introduction of theDefinitionClassalias and theparse_valuefunction-based approach improves template maintainability and consistency.Key improvements:
- Centralized value parsing logic via
parse_value- Safer optional property handling with
TryGetValue- Elimination of JsonElement special-casing
- Consistent class naming via
DefinitionClassaliasHowever, ensure that:
- The
parse_valuefunction correctly handles null values for optional properties (see verification request in lines 43-48)- Safe casting is applied for
additionalPropertiesdeserialization (see issue flagged in line 53)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/dotnet/Package/Models/Model.cs.twig(2 hunks)
🔇 Additional comments (6)
templates/dotnet/Package/Models/Model.cs.twig (6)
1-1: LGTM: Clean alias for maintainability.Introducing the
DefinitionClassalias improves template readability and ensures consistent class naming throughout the generated code.
7-9: LGTM: Conditional import reduces clutter.The conditional import of the Enums namespace only when needed is a good practice that keeps generated code clean.
13-13: LGTM: Consistent use of DefinitionClass alias.The class declaration, property declarations, and constructor signature correctly use the
DefinitionClassalias, maintaining consistency throughout the template.Also applies to: 17-17, 24-26
57-66: LGTM: ToMap method handles all property types correctly.The
ToMapmethod correctly handles sub-schemas (both objects and arrays), enums, and regular properties with appropriate transformations and null-safety operators.
67-84: LGTM: ConvertTo methods enable flexible type conversion.The
ConvertTomethods correctly delegate to user-provided conversion functions, enabling flexible handling of additional properties and nested structures.
43-48: Confirm null preservation in parse_value
Themap.TryGetValue(...)guard still invokesparse_valueif the stored value isnull, which may yield default primitives instead ofnull.parse_valueis implemented outside this template—please verify it returnsnullfor null inputs or add an explicit null check around the call.
|
@ChiragAgg5k, good evening. |
|
@Fellmonkey waiting on jake's review, will merge then 👍🏻 |
It's strange here. I checked GetDB and GetCache server definitely doesn't return any keys to the HealthStatus model. We need to either edit the model specifications or see why the sever isn't returning this data. |
|
@Fellmonkey oh sorry missed your comment there, best to check in the appwrite codebase on why those values are not being returned, or if the model requires some adjustments i did some testing myself and it seems to work just fine for me: 
|
|
@ChiragAgg5k It really works But I don't understand what the problem is with the SDK; the Dart server has the same problem. 
I made some changes in Dart, because otherwise an error would occur: type 'Null' is not a subtype of type 'int' |
|
@Fellmonkey interesting it doesnt work in dart, would be great to figure out why. can you please create a seperate issue for tracking this 🙏 |
What does this PR do?
This PR refactors the JSON serialization and deserialization logic in the .NET SDK template to improve robustness and fix critical issues:
Key Changes:
Fixed infinite recursion bug in
ObjectToInferredTypesConverter: Replaced the problematic approach of usingJsonSerializer.Deserializerecursively within the converter itself, which causedStackOverflowExceptionfor nested objects and arrays.Improved JSON type inference: Switched from
JsonTokenTypetoJsonElement.ValueKindfor more accurate and reliable type detection, providing better handling of all JSON value types including null and undefined values.Eliminates the risk of leaking JsonElement instances into the resulting object graph, simplifying model deserialization and removing the need for special handling of JsonElement in generated code.
Streamlined model deserialization: Simplified the generated model deserialization logic by removing special handling for
JsonElementobjects and standardizing type conversions, making the generated code more readable and maintainable.Enhanced error handling: Added proper error handling with descriptive exceptions for unsupported JSON value kinds.
Test Plan
Testing the ObjectToInferredTypesConverter fix:
Testing model deserialization changes:
Related PRs and Issues
This PR addresses potential runtime crashes and improves the overall reliability of JSON handling in generated .NET SDKs. The changes are particularly important for applications that work with complex nested JSON structures from API responses.
Related to issues with incorrect type mapping, JsonElement leakage, and runtime errors during deserialization of complex/nested JSON structures.
Have you read the Contributing Guidelines on issues?
YES
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style