microsoft / kiota-serialization-json-dotnet

Kiota serialization provider implementation with System.Text.Json
https://aka.ms/kiota/docs
MIT License
23 stars 27 forks source link

Unbounded LINQ expansion in AOT scenarios #226

Closed filipnavara closed 6 months ago

filipnavara commented 6 months ago

The use of LINQ in enum parsing and serialization code is resulting in unbounded LINQ generic specialization expansion that results in crashing the NativeAOT compiler.

A full explanation of the issue is here.

The use of LINQ should be removed from the methods to make the use of Kiota and Microsoft.Graph SDK usable in NativeAOT scenarios.

While not a nice or optimal code I tested that the following diff (along with a similar change in Kiota Form serializer) is enough to stop the problem from occurring:

diff --git a/src/JsonParseNode.cs b/src/JsonParseNode.cs
index f91286e..cbb5229 100644
--- a/src/JsonParseNode.cs
+++ b/src/JsonParseNode.cs
@@ -218,21 +218,23 @@ namespace Microsoft.Kiota.Serialization.Json
 #endif
         {
             var rawValue = _jsonNode.GetString();
-            if(string.IsNullOrEmpty(rawValue)) return null;
+            if (string.IsNullOrEmpty(rawValue)) return null;

             var type = typeof(T);
             rawValue = ToEnumRawName<T>(rawValue!);
-            if(type.GetCustomAttributes<FlagsAttribute>().Any())
+            if (type.GetCustomAttributes<FlagsAttribute>().Any())
             {
-                return (T)(object)rawValue!
-                    .Split(',')
-                    .Select(x => Enum.TryParse<T>(x, true, out var result) ? result : (T?)null)
-                    .Where(x => !x.Equals(null))
-                    .Select(x => (int)(object)x!)
-                    .Sum();
+                string[] valueNames = rawValue!.Split(',');
+                int value = 0;
+                foreach (string valueName in valueNames)
+                {
+                    if (Enum.TryParse<T>(valueName, true, out var result))
+                        value |= (int)(object)result;
+                }
+                return (T)(object)value;
             }
             else
-                return Enum.TryParse<T>(rawValue, true,out var result) ? result : null;
+                return Enum.TryParse<T>(rawValue, true, out var result) ? result : null;
         }

         /// <summary>
diff --git a/src/JsonSerializationWriter.cs b/src/JsonSerializationWriter.cs
index 4674569..ef11aac 100644
--- a/src/JsonSerializationWriter.cs
+++ b/src/JsonSerializationWriter.cs
@@ -278,16 +278,21 @@ namespace Microsoft.Kiota.Serialization.Json
             if(value.HasValue)
             {
                 if(typeof(T).GetCustomAttributes<FlagsAttribute>().Any())
-                    WriteStringValue(null,
+                {
+                    var values =
 #if NET5_0_OR_GREATER
-                        Enum.GetValues<T>()
+                        Enum.GetValues<T>();
 #else
-                        Enum.GetValues(typeof(T))
-                            .Cast<T>()
+                        Enum.GetValues(typeof(T)).Cast<T>();
 #endif
-                            .Where(x => value.Value.HasFlag(x))
-                            .Select(GetEnumName)
-                            .Aggregate((x, y) => $"{x},{y}"));
+                    List<string> valueNames = new List<string>();
+                    foreach (var x in values)
+                    {
+                        if (value.Value.HasFlag(x) && GetEnumName(x) is string valueName)
+                            valueNames.Add(valueName);
+                    }
+                    WriteStringValue(null, string.Join(",", valueNames));
+                }
                 else WriteStringValue(null, GetEnumName(value.Value));
             }
         }

The code can be written in more optimal fashion. Hence, why I submit an issue and not a PR.

baywet commented 6 months ago

Hi @filipnavara Thanks for using the Microsoft Graph SDK and for bringing this up. I think it'd be ok to open a PR at this stage, and we can discuss optimizations on the PR. For the second method, it'd probably be better to use a span or a string builder instead of a list of strings if we're talking optimization.

filipnavara commented 6 months ago

For the second method, it'd probably be better to use a span or a string builder instead of a list of strings if we're talking optimization.

Definitely :-) I just wrote the simplest possible implementation to test how far can I get with fixing just the few methods.

am11 commented 6 months ago

Copilot optimized the patches like this: first patch

@@ -218,21 +218,23 @@ namespace Microsoft.Kiota.Serialization.Json
 #endif
         {
             var rawValue = _jsonNode.GetString();
             if (string.IsNullOrEmpty(rawValue)) return null;

             var type = typeof(T);
             rawValue = ToEnumRawName<T>(rawValue!);
             if (type.GetCustomAttributes<FlagsAttribute>().Any())
             {
-                return (T)(object)rawValue!
-                    .Split(',')
-                    .Select(x => Enum.TryParse<T>(x, true, out var result) ? result : (T?)null)
-                    .Where(x => !x.Equals(null))
-                    .Select(x => (int)(object)x!)
-                    .Sum();
+                ReadOnlySpan<char> valueSpan = rawValue.AsSpan();
+                int value = 0;
+                while (valueSpan.Length > 0)
+                {
+                    int commaIndex = valueSpan.IndexOf(',');
+                    ReadOnlySpan<char> valueNameSpan = commaIndex < 0 ? valueSpan : valueSpan.Slice(0, commaIndex);
+                    if (Enum.TryParse<T>(valueNameSpan.ToString(), true, out var result))
+                        value |= (int)(object)result;
+                    valueSpan = commaIndex < 0 ? ReadOnlySpan<char>.Empty : valueSpan.Slice(commaIndex + 1);
+                }
+                return (T)(object)value;
             }
             else
                 return Enum.TryParse<T>(rawValue, true, out var result) ? result : null;
         }

second patch

@@ -278,16 +278,21 @@ namespace Microsoft.Kiota.Serialization.Json
             if(value.HasValue)
             {
                 if(typeof(T).GetCustomAttributes<FlagsAttribute>().Any())
-                    WriteStringValue(null,
+                {
+                    var values =
 #if NET5_0_OR_GREATER
-                        Enum.GetValues<T>()
+                        Enum.GetValues<T>();
 #else
-                        Enum.GetValues(typeof(T))
-                            .Cast<T>()
+                        Enum.GetValues(typeof(T)).Cast<T>();
 #endif
-                            .Where(x => value.Value.HasFlag(x))
-                            .Select(GetEnumName)
-                            .Aggregate((x, y) => $"{x},{y}"));
+                    StringBuilder valueNames = new StringBuilder();
+                    foreach (var x in values)
+                    {
+                        if (value.Value.HasFlag(x) && GetEnumName(x) is string valueName)
+                            valueNames.Append(valueName).Append(',');
+                    }
+                    if (valueNames.Length > 0)
+                        valueNames.Length--; // Remove the last comma
+                    WriteStringValue(null, valueNames.ToString());
+                }
                 else WriteStringValue(null, GetEnumName(value.Value));
             }
         }