Skip to content

fix: (.NET) Improve json De/serialization #1138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+62 −37
Open

fix: (.NET) Improve json De/serialization #1138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a48f7a
Refactor object inference in JSON converter
Fellmonkey Aug 2, 2025
b44f0b1
Refactor model deserialization logic in C# template
Fellmonkey Aug 2, 2025
4c6b9f7
Handle optional properties in model From method
Fellmonkey Aug 9, 2025
b071cbc
synchronization with the Unity template
Fellmonkey Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 46 additions & 18 deletions templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,60 @@ namespace {{ spec.title | caseUcfirst }}.Converters
{
public class ObjectToInferredTypesConverter : JsonConverter<object>
{
public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
switch (reader.TokenType)
using (JsonDocument document = JsonDocument.ParseValue(ref reader))
{
case JsonTokenType.True:
return true;
case JsonTokenType.False:
return false;
case JsonTokenType.Number:
if (reader.TryGetInt64(out long l))
return ConvertElement(document.RootElement);
}
}

private object? ConvertElement(JsonElement element)
{
switch (element.ValueKind)
{
case JsonValueKind.Object:
var dictionary = new Dictionary<string, object?>();
foreach (var property in element.EnumerateObject())
{
return l;
dictionary[property.Name] = ConvertElement(property.Value);
}
return dictionary;

case JsonValueKind.Array:
var list = new List<object?>();
foreach (var item in element.EnumerateArray())
{
list.Add(ConvertElement(item));
}
return reader.GetDouble();
case JsonTokenType.String:
if (reader.TryGetDateTime(out DateTime datetime))
return list;

case JsonValueKind.String:
if (element.TryGetDateTime(out DateTime datetime))
{
return datetime;
}
return reader.GetString()!;
case JsonTokenType.StartObject:
return JsonSerializer.Deserialize<Dictionary<string, object>>(ref reader, options)!;
case JsonTokenType.StartArray:
return JsonSerializer.Deserialize<object[]>(ref reader, options)!;
return element.GetString();

case JsonValueKind.Number:
if (element.TryGetInt64(out long l))
{
return l;
}
return element.GetDouble();

case JsonValueKind.True:
return true;

case JsonValueKind.False:
return false;

case JsonValueKind.Null:
case JsonValueKind.Undefined:
return null;

default:
return JsonDocument.ParseValue(ref reader).RootElement.Clone();
throw new JsonException($"Unsupported JsonValueKind: {element.ValueKind}");
}
}

Expand Down
2 changes: 1 addition & 1 deletion templates/dotnet/Package/Exception.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ namespace {{spec.title | caseUcfirst}}
this.Type = type;
this.Response = response;
}

public {{spec.title | caseUcfirst}}Exception(string message, Exception inner)
: base(message, inner)
{
}
}
}

2 changes: 1 addition & 1 deletion templates/dotnet/Package/Extensions/Extensions.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,4 +624,4 @@ namespace {{ spec.title | caseUcfirst }}.Extensions
return GetMimeTypeFromExtension(System.IO.Path.GetExtension(path));
}
}
}
}
4 changes: 2 additions & 2 deletions templates/dotnet/Package/Models/InputFile.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
using System.IO;
using Appwrite.Extensions;
using {{ spec.title | caseUcfirst }}.Extensions;

namespace {{ spec.title | caseUcfirst }}.Models
{
Expand Down Expand Up @@ -38,4 +38,4 @@ namespace {{ spec.title | caseUcfirst }}.Models
SourceType = "bytes"
};
}
}
}
19 changes: 8 additions & 11 deletions templates/dotnet/Package/Models/Model.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %}
{% macro property_name(definition, property) %}{{ property.name | caseUcfirst | removeDollarSign | escapeKeyword }}{% endmacro %}

using System;
using System.Linq;
using System.Collections.Generic;
Expand Down Expand Up @@ -40,31 +39,29 @@ namespace {{ spec.title | caseUcfirst }}.Models
public static {{ definition.name | caseUcfirst | overrideIdentifier }} From(Dictionary<string, object> map) => new {{ definition.name | caseUcfirst | overrideIdentifier }}(
{%~ for property in definition.properties %}
{{ property.name | caseCamel | escapeKeyword | removeDollarSign }}:{{' '}}
{%- if not property.required -%}map.ContainsKey("{{ property.name }}") ? {% endif %}
{%- if property.sub_schema %}
{%- if property.type == 'array' -%}
map["{{ property.name }}"] is JsonElement jsonArray{{ loop.index }} ? jsonArray{{ loop.index }}.Deserialize<List<Dictionary<string, object>>>()!.Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: it)).ToList() : ((IEnumerable<Dictionary<string, object>>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: it)).ToList()
((IEnumerable<object>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it)).ToList()
{%- else -%}
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: map["{{ property.name }}"] is JsonElement jsonObj{{ loop.index }} ? jsonObj{{ loop.index }}.Deserialize<Dictionary<string, object>>()! : (Dictionary<string, object>)map["{{ property.name }}"])
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)map["{{ property.name }}"])
{%- endif %}
Comment on lines +42 to 48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Optional key guard misses the “present-but-null” case (can throw).

If the key exists with null, casts below will NRE/ICE. Guard for null too.

-                {%- if not property.required -%}map.ContainsKey("{{ property.name }}") ? {% endif %}
+                {%- if not property.required -%}map.ContainsKey("{{ property.name }}") && map["{{ property.name }}"] != null ? {% endif %}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{%- if not property.required -%}map.ContainsKey("{{ property.name }}") ? {% endif %}
{%- if property.sub_schema %}
{%- if property.type == 'array' -%}
map["{{ property.name }}"] is JsonElement jsonArray{{ loop.index }} ? jsonArray{{ loop.index }}.Deserialize<List<Dictionary<string, object>>>()!.Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: it)).ToList() : ((IEnumerable<Dictionary<string, object>>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: it)).ToList()
((IEnumerable<object>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it)).ToList()
{%- else -%}
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: map["{{ property.name }}"] is JsonElement jsonObj{{ loop.index }} ? jsonObj{{ loop.index }}.Deserialize<Dictionary<string, object>>()! : (Dictionary<string, object>)map["{{ property.name }}"])
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)map["{{ property.name }}"])
{%- endif %}
{%- if not property.required -%}map.ContainsKey("{{ property.name }}") && map["{{ property.name }}"] != null ? {% endif %}
{%- if property.sub_schema %}
{%- if property.type == 'array' -%}
((IEnumerable<object>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it)).ToList()
{%- else -%}
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)map["{{ property.name }}"])
{%- endif %}
🤖 Prompt for AI Agents 
In templates/dotnet/Package/Models/Model.cs.twig around lines 42 to 48, the
optional-key guard only checks map.ContainsKey("{{ property.name }}") and will
still attempt casts when the key is present but null; update the guard to also
verify the value is not null (e.g., check map.ContainsKey(...) && map["{{
property.name }}"] != null or use TryGetValue to obtain the value and test for
null) before performing any casts or Select/ToList calls, and apply this null
check to both the array and object branches so null values do not cause NRE/ICE.

Comment on lines +45 to 48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Safer casts for sub-schemas (avoid InvalidCast when value types deviate).

Use safe casts for optionals to prevent InvalidCastException when value is null/mismatched.

-                        ((IEnumerable<object>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it)).ToList()
+                        ((map["{{ property.name }}"] as IEnumerable<object>) ?? Array.Empty<object>())
+                            .Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it))
+                            .ToList()
-                        {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)map["{{ property.name }}"])
+                        (map["{{ property.name }}"] as Dictionary<string, object>) is { } obj
+                            ? {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: obj)
+                            : null

Note: The second change pairs with the Line 42 null-guard; harmless for required props, safer for optionals.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
((IEnumerable<object>)map["{{ property.name }}"]).Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it)).ToList()
{%- else -%}
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: map["{{ property.name }}"] is JsonElement jsonObj{{ loop.index }} ? jsonObj{{ loop.index }}.Deserialize<Dictionary<string, object>>()! : (Dictionary<string, object>)map["{{ property.name }}"])
{{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)map["{{ property.name }}"])
{%- endif %}
((map["{{ property.name }}"] as IEnumerable<object>) ?? Array.Empty<object>())
.Select(it => {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: (Dictionary<string, object>)it))
.ToList()
{%- else -%}
(map["{{ property.name }}"] as Dictionary<string, object>) is { } obj
? {{ property.sub_schema | caseUcfirst | overrideIdentifier }}.From(map: obj)
: null
{%- endif %}
🤖 Prompt for AI Agents 
In templates/dotnet/Package/Models/Model.cs.twig around lines 45-48, avoid
direct casts that throw InvalidCastException by using safe casts and null
guards: replace ((IEnumerable<object>)map["{{ property.name }}"]) with a safe
cast (as IEnumerable<object>) and handle null by either using ?.Select(...)
followed by ToList() or coalescing to an empty sequence before mapping;
similarly replace (Dictionary<string, object>)map["{{ property.name }}"] with a
safe cast (as Dictionary<string, object>) and only call {{ property.sub_schema |
caseUcfirst | overrideIdentifier }}.From when the result is non-null (or pass a
null-safe value), keeping the existing Line 42 null-guard pairing for optionals.

{%- else %}
{%- if property.type == 'array' -%}
map["{{ property.name }}"] is JsonElement jsonArrayProp{{ loop.index }} ? jsonArrayProp{{ loop.index }}.Deserialize<{{ property | typeName }}>()! : ({{ property | typeName }})map["{{ property.name }}"]
((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()!
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null-forgiving operator ! at the end of ToList()! could hide potential null reference exceptions. If the enumerable or its elements could be null, this could cause runtime errors.

Suggested change
((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()!
((map["{{ property.name }}"] as IEnumerable<object>)?.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() ?? new List<{% if property.items.type == "string" %}string{% elseif property.items.type == "integer" %}long{% elseif property.items.type == "number" %}double{% elseif property.items.type == "boolean" %}bool{% else %}object{% endif %}>()

Copilot uses AI. Check for mistakes.

{%- else %}
{%- if property.type == "integer" or property.type == "number" %}
{%- if not property.required -%}map["{{ property.name }}"] == null ? null :{% endif %}Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(map["{{ property.name }}"])
{%- if not property.required -%}map["{{ property.name }}"] == null ? null : {% endif %}Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(map["{{ property.name }}"])
{%- else %}
{%- if property.type == "boolean" -%}
({{ property | typeName }}{% if not property.required %}?{% endif %})map["{{ property.name }}"]
{%- else %}
{%- if not property.required -%}
map.TryGetValue("{{ property.name }}", out var {{ property.name | caseCamel | escapeKeyword | removeDollarSign }}) ? {{ property.name | caseCamel | escapeKeyword | removeDollarSign }}?.ToString() : null
{%- else -%}
map["{{ property.name }}"].ToString()
{%- endif %}
{%- else -%}
map["{{ property.name }}"]{% if not property.required %}?{% endif %}.ToString()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Preserve ISO strings when converter produced DateTime/Offset.

If the converter inferred DateTime/Offset, ToString() becomes culture-dependent. Emit ISO 8601 for round-trip.

-                                map["{{ property.name }}"]{% if not property.required %}?{% endif %}.ToString()
+                                map["{{ property.name }}"] switch
+                                {
+                                    DateTimeOffset dto => dto.ToString("O"),
+                                    DateTime dt => dt.ToUniversalTime().ToString("O"),
+                                    _ => map["{{ property.name }}"]{% if not property.required %}?{% endif %}.ToString()
+                                }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
map["{{ property.name }}"]{% if not property.required %}?{% endif %}.ToString()
map["{{ property.name }}"]{% if not property.required %}?{% endif %} switch
{
DateTimeOffset dto => dto.ToString("O"),
DateTime dt => dt.ToUniversalTime().ToString("O"),
_ => map["{{ property.name }}"]{% if not property.required %}?{% endif %}.ToString()
}
🤖 Prompt for AI Agents 
In templates/dotnet/Package/Models/Model.cs.twig around line 59, the template
calls ToString() on values that a converter may have produced as
DateTime/DateTimeOffset, which is culture-dependent; update the template to emit
an ISO 8601 round-trip representation by using ToString("o") for DateTime and
DateTimeOffset cases (respecting the existing nullability ? operator if
present). Detect the property type (e.g., property.type == "DateTime" or
property.type == "DateTimeOffset" or equivalent metadata your generator
provides) and replace ToString() with ToString("o") only for those types,
leaving other types unchanged.

{%- endif %}
{%~ endif %}
{%~ endif %}
{%~ endif %}
{%- if not property.required %} : null{% endif %}
{%- if not loop.last or (loop.last and definition.additionalProperties) %},
{%~ endif %}
{%~ endfor %}
Expand Down
2 changes: 1 addition & 1 deletion templates/dotnet/Package/Models/UploadProgress.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ namespace {{ spec.title | caseUcfirst }}
ChunksUploaded = chunksUploaded;
}
}
}
}
2 changes: 1 addition & 1 deletion templates/dotnet/Package/Query.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,4 @@ namespace {{ spec.title | caseUcfirst }}
return new Query("and", null, queries.Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions)).ToList()).ToString();
}
}
}
}
4 changes: 2 additions & 2 deletions templates/dotnet/Package/Role.cs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
namespace Appwrite
namespace {{ spec.title | caseUcfirst }}
{
/// <summary>
/// Helper class to generate role strings for Permission.
Expand Down Expand Up @@ -89,4 +89,4 @@ namespace Appwrite
return $"label:{name}";
}
}
}
}