fix: handle Object[] in .NET array deserialization #1218
Conversation
Fixed an issue where array deserialization in Model.cs.twig failed when System.Text.Json creates Object[] instead of List<T>. Added a new ToList<T> extension method that properly handles JsonElement, Object[], List<T>, and IEnumerable<T> types, ensuring robust array deserialization across all scenarios.
Updated DotNet.php to return the element type instead of List<T> wrapper, and simplified Model.cs.twig to use typeName directly with the ToList<T> extension method.
|
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. WalkthroughAdds Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
templates/dotnet/Package/Extensions/Extensions.cs.twig (2)
16-26: Add null check for input value.The method should handle
nullinput explicitly rather than throwingNullReferenceExceptionfrom pattern matching.Apply this diff to add null handling:
public static List<T> ToList<T>(object value) { + if (value == null) + { + throw new ArgumentNullException(nameof(value), "Cannot convert null to List<T>"); + } + return value switch { JsonElement jsonElement => jsonElement.Deserialize<List<T>>()!, object[] objArray => objArray.Cast<T>().ToList(), List<T> list => list, IEnumerable<T> enumerable => enumerable.ToList(), _ => throw new InvalidCastException($"Cannot convert {value.GetType()} to List<{typeof(T)}>") }; }
21-21: Document the casting behavior for object[] to List.The
Cast<T>()operation will throwInvalidCastExceptionif any array element cannot be cast toT. This is appropriate for catching type mismatches early, but consider documenting this behavior or adding a try-catch for clearer error messages.- object[] objArray => objArray.Cast<T>().ToList(), + object[] objArray => objArray.Cast<T>().ToList(), // Throws InvalidCastException if elements cannot be cast to T
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/DotNet.php(1 hunks)templates/dotnet/Package/Extensions/Extensions.cs.twig(2 hunks)templates/dotnet/Package/Models/Model.cs.twig(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/DotNet.php (5)
src/SDK/Language/Python.php (1)
getTypeName(236-264)src/SDK/Language/Kotlin.php (1)
getTypeName(107-130)src/SDK/Language/Dart.php (1)
getTypeName(128-161)src/SDK/Language/Deno.php (1)
getTypeName(133-155)src/SDK/Language/Go.php (1)
getTypeName(139-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Python310)
- GitHub Check: dotnet (server)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Node18)
- GitHub Check: swift (server)
- GitHub Check: kotlin (server)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: flutter (client)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: android (client)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (4)
templates/dotnet/Package/Models/Model.cs.twig (3)
8-8: LGTM! Extension namespace import is necessary.The import enables the use of the new
ToList<T>extension method throughout the model deserialization logic.
45-45: Array deserialization logic correctly uses Extensions.ToList.The change to use
Extensions.ToList<Dictionary<string, object>>properly handles the case where System.Text.Json producesObject[]instead ofList<T>, addressing the core issue described in the PR.
62-62: Verify typeName filter returns element type consistently.This line depends on the
typeNamefilter (which callsgetTypeName) returning just the element type (e.g.,string) rather than the full list type (e.g.,List<string>). Ensure thegetTypeNamechange insrc/SDK/Language/DotNet.phpis correct and that property declarations are not broken.As noted in the review of
src/SDK/Language/DotNet.php, verify that thesub_schemafunction properly wraps array types inList<>for property declarations, as the deserialization logic here depends ongetTypeNamereturning element types.src/SDK/Language/DotNet.php (1)
180-182: Verify generated C# model array property declarations useList<ElementType>
No.csmodel files were found in the repository to confirm this change. Manually verify in the generated code that array properties are declared asList<…>rather than the raw element type.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
templates/dotnet/Package/Extensions/Extensions.cs.twig (1)
23-26: Cast may throw InvalidCastException for incompatible element types.When
objArraycontains elements that cannot be cast toT,Cast<T>()will throwInvalidCastExceptionat enumeration time, which could provide a less clear error message than the explicit check at line 38.Consider adding validation before casting to provide a clearer error message for type mismatches.
Apply this diff to add validation:
if (value is object[] objArray) { + if (objArray.Length > 0 && objArray.Any(item => item != null && !(item is T))) + { + throw new InvalidCastException($"Cannot convert object[] with incompatible elements to List<{typeof(T)}>"); + } return objArray.Cast<T>().ToList(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/DotNet.php(1 hunks)templates/dotnet/Package/Extensions/Extensions.cs.twig(2 hunks)templates/dotnet/Package/Models/Model.cs.twig(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/DotNet.php (8)
src/SDK/Language/Kotlin.php (1)
getTypeName(107-130)src/SDK/Language/Swift.php (1)
getTypeName(302-326)src/SDK/Language/Dart.php (1)
getTypeName(128-161)src/SDK/Language/Web.php (1)
getTypeName(180-243)src/SDK/Language/CLI.php (1)
getTypeName(352-376)src/SDK/Language/Go.php (1)
getTypeName(139-160)src/SDK/Language/Node.php (1)
getTypeName(15-78)src/SDK/Language/PHP.php (1)
getTypeName(253-271)
🔇 Additional comments (5)
templates/dotnet/Package/Models/Model.cs.twig (3)
8-8: LGTM!The addition of the Extensions namespace import enables access to the new
ToList<T>extension method used throughout the model deserialization logic.
62-62:typeNamefilter correctly returns element types for arrays
TYPE_ARRAY delegates to getTypeName($parameter['array']), and getTypeName maps to the primitive or model type.
45-45: ConfirmFrommethod parameter name
Verify that the generatedFrom(Dictionary<string, object>)method declares its dictionary parameter asmap; using.From(map: it)will fail if the parameter name differs.src/SDK/Language/DotNet.php (2)
174-186: Review getParamExample compatibility with the type change.The
getParamExamplemethod at lines 241-311 references$this->getTypeName($param['array'])for array types (lines 274, 276). With the new behavior wheregetTypeNamereturns element types instead ofList<T>, verify that the example generation still produces valid C# code.Based on the code at lines 274-276, it constructs
new List<{elementType}>(), which should still work correctly since it now uses the element type directly.
180-182: Update theparametermacro to emitList<T>for arrays
Intemplates/dotnet/base/utils.twig, the line{{ parameter | typeName }}now yields only the element type for arrays. Wrap array parameters in
List<{{ elementType }}>(orList<object>when no inner type) so generated method signatures remain correct.
critical_fixes_required
ChiragAgg5k
force-pushed
the
fix-dotnet-array-deserialization
branch
from
203e7f0 to
aac6cc3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes array deserialization issues in .NET code generation where System.Text.Json creates Object[] instead of List, causing InvalidCastException. The solution introduces a centralized extension method to handle various array input types and simplifies model property initialization.
- Added ConvertToList extension method to handle JsonElement, Object[], List, and IEnumerable inputs
- Updated Model.cs.twig to use the new extension method for consistent array property handling
- Simplified array property type handling by removing List wrapper logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| templates/dotnet/Package/Extensions/Extensions.cs.twig | Added ConvertToList extension method for robust array conversion |
| templates/dotnet/Package/Models/Model.cs.twig | Updated array property initialization to use ConvertToList extension |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@ChiragAgg5k it's because of the converter old variant (current in main) case JsonTokenType.StartArray:
return JsonSerializer.Deserialize<object[]>(ref reader, options)!;in pr case JsonValueKind.Array:
var list = new List<object?>();
foreach (var item in element.EnumerateArray())
{
list.Add(ConvertElement(item));
}
return list;Although the current solution is universal, I will add a revised version to my PR. |
3 participants
Summary
Fixed an issue where array deserialization in Model.cs.twig failed when System.Text.Json creates Object[] instead of List.
Problem
In Model.cs.twig, the array deserialization logic didn't handle Object[] properly. When System.Text.Json deserializes a dictionary with arrays, it creates Object[], but the code tried to directly cast to List, causing InvalidCastException.
Solution
ToList<T>extension method in Extensions.cs.twig that properly handles:Changes
templates/dotnet/Package/Extensions/Extensions.cs.twig: Added ToList extension methodtemplates/dotnet/Package/Models/Model.cs.twig: Updated array deserialization to use ToListsrc/SDK/Language/DotNet.php: Simplified getTypeName to return element typesTest plan
Summary by CodeRabbit
Refactor
Bug Fixes