C#: Add response checks for batch for every command (valkey-io#4236)

* Add type check for batch

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: Edward Liang <76571219+edlng@users.noreply.github.com>

Author: Yury-Fridlyand

csharp/sources/Valkey.Glide/BaseClient.Commands.cs

@@ -0,0 +1,15 @@
+// Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
+
+using Valkey.Glide.Commands;
+using Valkey.Glide.Internals;
+
+namespace Valkey.Glide;
+
+public abstract partial class BaseClient : IStringBaseCommands
+{
+    public async Task<string> Set(GlideString key, GlideString value)
+        => await Command(Request.Set(key, value));
+
+    public async Task<GlideString?> Get(GlideString key)
+        => await Command(Request.Get(key));
+}

csharp/sources/Valkey.Glide/BaseClient.cs

@@ -2,28 +2,21 @@
 
 using System.Runtime.InteropServices;
 
-using Valkey.Glide.Commands;
 using Valkey.Glide.Internals;
 using Valkey.Glide.Pipeline;
 
 using static Valkey.Glide.ConnectionConfiguration;
 using static Valkey.Glide.Errors;
 using static Valkey.Glide.Internals.FFI;
+using static Valkey.Glide.Internals.ResponseConverters;
 using static Valkey.Glide.Internals.ResponseHandler;
 using static Valkey.Glide.Pipeline.Options;
-using static Valkey.Glide.Route;
 
 namespace Valkey.Glide;
 
-public abstract class BaseClient : IDisposable, IStringBaseCommands
+public abstract partial class BaseClient : IDisposable
 {
     #region public methods
-    public async Task<string> Set(GlideString key, GlideString value)
-        => await Command(RequestType.Set, [key, value], HandleOk);
-
-    public async Task<GlideString?> Get(GlideString key)
-        => await Command(RequestType.Get, [key], response => HandleServerResponse<GlideString>(response, true));
-
     public void Dispose()
     {
         GC.SuppressFinalize(this);
@@ -71,10 +64,17 @@ protected BaseClient()
 
     protected internal delegate T ResponseHandler<T>(IntPtr response);
 
-    internal async Task<T> Command<T>(RequestType requestType, GlideString[] arguments, ResponseHandler<T> responseHandler, Route? route = null) where T : class?
+    /// <summary>
+    /// </summary>
+    /// <typeparam name="R">Type received from server.</typeparam>
+    /// <typeparam name="T">Type we return to the user.</typeparam>
+    /// <param name="command"></param>
+    /// <param name="route"></param>
+    /// <returns></returns>
+    internal async Task<T> Command<R, T>(Request.Cmd<R, T> command, Route? route = null)
     {
         // 1. Create Cmd which wraps CmdInfo and manages all memory allocations
-        using Cmd cmd = new(requestType, arguments);
+        using Cmd cmd = command.ToFfi();
 
         // 2. Allocate memory for route
         using FFI.Route? ffiRoute = route?.ToFfi();
@@ -84,7 +84,15 @@ internal async Task<T> Command<T>(RequestType requestType, GlideString[] argumen
         CommandFfi(_clientPointer, (ulong)message.Index, cmd.ToPtr(), ffiRoute?.ToPtr() ?? IntPtr.Zero);
 
         // 4. Get a response and Handle it
-        return responseHandler(await message);
+        IntPtr response = await message;
+        try
+        {
+            return HandleServerValue(HandleResponse(response), command.IsNullable, command.Converter);
+        }
+        finally
+        {
+            FreeResponse(response);
+        }
 
         // All memory allocated is auto-freed by `using` operator
     }
@@ -102,79 +110,17 @@ internal async Task<T> Command<T>(RequestType requestType, GlideString[] argumen
         BatchFfi(_clientPointer, (ulong)message.Index, ffiBatch.ToPtr(), raiseOnError, ffiOptions?.ToPtr() ?? IntPtr.Zero);
 
         // 4. Get a response and Handle it
-        return HandleServerResponse<object?[]?>(await message, true);
-
-        // All memory allocated is auto-freed by `using` operator
-    }
-
-    protected internal static string HandleOk(IntPtr response)
-        => HandleServerResponse<string>(response, false);
-
-    protected internal static T HandleServerResponse<T>(IntPtr response, bool isNullable) where T : class?
-        => HandleServerResponse<T, T>(response, isNullable, o => o);
-
-    protected static ClusterValue<object?> HandleCustomCommandClusterResponse(IntPtr response, Route? route = null)
-        => HandleServerResponse<object, ClusterValue<object?>>(response, true, data
-            => (data is string str && str == "OK") || route is SingleNodeRoute || data is not Dictionary<GlideString, object?>
-                ? ClusterValue<object?>.OfSingleValue(data)
-                : ClusterValue<object?>.OfMultiValue((Dictionary<GlideString, object?>)data));
-
-    /// <summary>
-    /// Process and convert a server response that may be a multi-node response.
-    /// </summary>
-    /// <typeparam name="R">GLIDE's return type per node.</typeparam>
-    /// <typeparam name="T">Command's return type.</typeparam>
-    /// <param name="response"></param>
-    /// <param name="isNullable"></param>
-    /// <param name="converter">Function to convert <typeparamref name="R"/> to <typeparamref name="T"/>.</param>
-    protected static ClusterValue<T> HandleClusterValueResponse<R, T>(IntPtr response, bool isNullable, Route route, Func<R, T> converter) where T : class?
-        => HandleServerResponse<object, ClusterValue<T>>(response, isNullable, data => route is SingleNodeRoute
-            ? ClusterValue<T>.OfSingleValue(converter((R)data))
-            : ClusterValue<T>.OfMultiValue(((Dictionary<GlideString, object>)data).ConvertValues(converter)));
-
-    /// <summary>
-    /// Process and convert a cluster multi-node response.
-    /// </summary>
-    /// <typeparam name="R">GLIDE's return type per node.</typeparam>
-    /// <typeparam name="T">Command's return type.</typeparam>
-    /// <param name="response"></param>
-    /// <param name="converter">Function to convert <typeparamref name="R"/> to <typeparamref name="T"/>.</param>
-    protected static Dictionary<string, T> HandleMultiNodeResponse<R, T>(IntPtr response, Func<R, T> converter) where T : class?
-        => HandleServerResponse<Dictionary<GlideString, object>, Dictionary<string, T>>(response, false, dict => dict.DownCastKeys().ConvertValues(converter));
-
-    /// <summary>
-    /// Process and convert a server response.
-    /// </summary>
-    /// <typeparam name="R">GLIDE's return type.</typeparam>
-    /// <typeparam name="T">Command's return type.</typeparam>
-    /// <param name="response"></param>
-    /// <param name="isNullable"></param>
-    /// <param name="converter">Optional function to convert <typeparamref name="R" /> to <typeparamref name="T" />.</param>
-    /// <returns></returns>
-    /// <exception cref="Exception"></exception>
-    protected internal static T HandleServerResponse<R, T>(IntPtr response, bool isNullable, Func<R, T> converter) where T : class? where R : class?
-    {
+        IntPtr response = await message;
         try
         {
-            object? value = HandleResponse(response);
-            if (value is null)
-            {
-                if (isNullable)
-                {
-#pragma warning disable CS8603 // Possible null reference return.
-                    return null;
-#pragma warning restore CS8603 // Possible null reference return.
-                }
-                throw new Exception($"Unexpected return type from Glide: got null expected {typeof(T).GetRealTypeName()}");
-            }
-            return value is R
-                ? converter((value as R)!)
-                : throw new RequestException($"Unexpected return type from Glide: got {value?.GetType().GetRealTypeName()} expected {typeof(T).GetRealTypeName()}");
+            return batch.ConvertResponse(HandleServerValue(HandleResponse(response), true, (object?[]? o) => o));
         }
         finally
         {
             FreeResponse(response);
         }
+
+        // All memory allocated is auto-freed by `using` operator
     }
     #endregion protected methods
 

csharp/sources/Valkey.Glide/GlideClient.cs

@@ -2,13 +2,12 @@
 
 using Valkey.Glide.Commands;
 using Valkey.Glide.Commands.Options;
+using Valkey.Glide.Internals;
 using Valkey.Glide.Pipeline;
 
 using static Valkey.Glide.ConnectionConfiguration;
 using static Valkey.Glide.Pipeline.Options;
 
-using RequestType = Valkey.Glide.Internals.FFI.RequestType;
-
 namespace Valkey.Glide;
 
 // TODO add wiki link
@@ -67,12 +66,10 @@ public static async Task<GlideClient> CreateClient(StandaloneClientConfiguration
         => await Batch(batch, raiseOnError, options);
 
     public async Task<object?> CustomCommand(GlideString[] args)
-        => await Command(RequestType.CustomCommand, args, resp
-            => HandleServerResponse<object?>(resp, true));
+        => await Command(Request.CustomCommand(args));
 
     public async Task<string> Info() => await Info([]);
 
     public async Task<string> Info(InfoOptions.Section[] sections)
-        => await Command(RequestType.Info, sections.ToGlideStrings(), resp
-            => HandleServerResponse<GlideString, string>(resp, false, gs => gs.ToString()));
+        => await Command(Request.Info(sections));
 }

csharp/sources/Valkey.Glide/GlideClusterClient.cs

@@ -2,12 +2,13 @@
 
 using Valkey.Glide.Commands;
 using Valkey.Glide.Commands.Options;
+using Valkey.Glide.Internals;
 using Valkey.Glide.Pipeline;
 
 using static Valkey.Glide.ConnectionConfiguration;
 using static Valkey.Glide.Errors;
-using static Valkey.Glide.Internals.FFI;
 using static Valkey.Glide.Pipeline.Options;
+using static Valkey.Glide.Route;
 
 namespace Valkey.Glide;
 
@@ -66,20 +67,18 @@ public static async Task<GlideClusterClient> CreateClient(ClusterClientConfigura
             : await Batch(batch, raiseOnError, options);
 
     public async Task<ClusterValue<object?>> CustomCommand(GlideString[] args)
-        => await Command(RequestType.CustomCommand, args, resp => HandleCustomCommandClusterResponse(resp));
+        => await Command(Request.CustomCommand(args, resp => ResponseConverters.HandleCustomCommandClusterValue(resp)));
 
     public async Task<ClusterValue<object?>> CustomCommand(GlideString[] args, Route route)
-        => await Command(RequestType.CustomCommand, args, resp => HandleCustomCommandClusterResponse(resp, route), route);
+        => await Command(Request.CustomCommand(args, resp => ResponseConverters.HandleCustomCommandClusterValue(resp, route)), route);
 
     public async Task<Dictionary<string, string>> Info() => await Info([]);
 
     public async Task<Dictionary<string, string>> Info(InfoOptions.Section[] sections)
-        => await Command(RequestType.Info, sections.ToGlideStrings(), resp
-            => HandleMultiNodeResponse<GlideString, string>(resp, gs => gs.ToString()));
+        => await Command(Request.Info(sections).ToMultiNodeValue());
 
     public async Task<ClusterValue<string>> Info(Route route) => await Info([], route);
 
     public async Task<ClusterValue<string>> Info(InfoOptions.Section[] sections, Route route)
-        => await Command(RequestType.Info, sections.ToGlideStrings(), resp
-            => HandleClusterValueResponse<GlideString, string>(resp, false, route, gs => gs.ToString()), route);
+        => await Command(Request.Info(sections).ToClusterValue(route is SingleNodeRoute), route);
 }

csharp/sources/Valkey.Glide/Internals/Cmd.cs

@@ -0,0 +1,112 @@
+// Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
+
+using System.Diagnostics;
+
+using Valkey.Glide.Commands.Options;
+
+using static Valkey.Glide.Errors;
+using static Valkey.Glide.Internals.FFI;
+
+namespace Valkey.Glide.Internals;
+
+internal class Request // TODO naming
+{
+    internal interface ICmd
+    {
+        /// <summary>
+        /// Convert to an FFI-ready struct.
+        /// </summary>
+        Cmd ToFfi();
+        /// <summary>
+        /// Get untyped converted (used for batch).
+        /// </summary>
+        Func<object?, object?> GetConverter();
+    }
+
+    internal class Cmd<R, T> : ICmd
+    {
+        public readonly bool IsNullable;
+        public readonly Func<R, T> Converter;
+        public readonly RequestType Request;
+        public readonly ArgsArray ArgsArray;
+
+#pragma warning disable IDE0046 // Convert to conditional expression
+        public Func<object?, object?> GetConverter() => value =>
+        {
+            if (value is null)
+            {
+                if (IsNullable)
+                {
+                    return null;
+                }
+                throw new RequestException($"Unexpected return type from Glide: got null expected {typeof(T).GetRealTypeName()}");
+            }
+            if (value is RequestException)
+            {
+                return value;
+            }
+            Debug.Assert(value!.GetType() == typeof(R) || typeof(R).IsAssignableFrom(value!.GetType()),
+                $"Unexpected return type from Glide: got {value?.GetType().GetRealTypeName()} expected {typeof(R).GetRealTypeName()}");
+
+            return Converter((R)value!);
+        };
+#pragma warning restore IDE0046 // Convert to conditional expression
+
+        public Cmd ToFfi() => new(Request, ArgsArray.Args);
+
+        public new string ToString() => $"{Request} [{string.Join(' ', ArgsArray.Args.ToStrings())}]";
+
+        public Cmd(RequestType request, GlideString[] args, bool isNullable, Func<R, T> converter)
+        {
+            Request = request;
+            ArgsArray = new() { Args = args };
+            IsNullable = isNullable;
+            Converter = converter;
+        }
+
+        /// <summary>
+        /// Convert a command to one which handles a multi-node cluster value.
+        /// </summary>
+        public Cmd<Dictionary<GlideString, object>, Dictionary<string, T>> ToMultiNodeValue()
+            => new(Request, ArgsArray.Args, IsNullable, map => ResponseConverters.HandleMultiNodeValue(map, Converter));
+
+        /// <summary>
+        /// Convert a command to one which handles a <see cref="ClusterValue{T}" />.
+        /// </summary>
+        /// <param name="isSingleValue">Whether current command call returns a single value.</param>
+        public Cmd<object, ClusterValue<T>> ToClusterValue(bool isSingleValue)
+            => new(Request, ArgsArray.Args, IsNullable, ResponseConverters.MakeClusterValueHandler(Converter, isSingleValue));
+    }
+
+    internal record ArgsArray
+    {
+        public GlideString[] Args = [];
+    }
+
+    public static Cmd<object?, object?> CustomCommand(GlideString[] args)
+        => new(RequestType.CustomCommand, args, true, o => o);
+
+    public static Cmd<object?, T> CustomCommand<T>(GlideString[] args, Func<object?, T> converter) where T : class?
+        => new(RequestType.CustomCommand, args, true, converter);
+
+    public static Cmd<GlideString, string> Info(InfoOptions.Section[] sections)
+        => new(RequestType.Info, sections.ToGlideStrings(), false, gs => gs.ToString());
+
+    public static Cmd<GlideString, GlideString> Get(GlideString key)
+        => Simple<GlideString>(RequestType.Get, [key], true);
+
+    public static Cmd<string, string> Set(GlideString key, GlideString value)
+        => OK(RequestType.Set, [key, value]);
+
+    /// <summary>
+    /// Create a Cmd which returns OK
+    /// </summary>
+    private static Cmd<string, string> OK(RequestType request, GlideString[] args)
+        => Simple<string>(request, args);
+
+    /// <summary>
+    /// Create a Cmd which does not need type conversion
+    /// </summary>
+    private static Cmd<T, T> Simple<T>(RequestType request, GlideString[] args, bool isNullable = false)
+        => new(request, args, isNullable, o => o);
+}

csharp/sources/Valkey.Glide/Internals/Helpers.cs

@@ -15,9 +15,15 @@ public static Dictionary<string, T> DownCastKeys<T>(this Dictionary<GlideString,
     public static Dictionary<T, string> DownCastVals<T>(this Dictionary<T, GlideString> dict) where T : class
         => dict.Select(p => (p.Key, Value: p.Value.ToString())).ToDictionary(p => p.Key, p => p.Value);
 
-    // Convert values in a dictionary from T to V where input dict has type `Dictionary<K, object>`
-    public static Dictionary<K, V> ConvertValues<K, V, T>(this Dictionary<K, object> dict, Func<T, V> converter) where K : class
-        => dict.Select(p => (p.Key, Value: converter((T)p.Value))).ToDictionary(p => p.Key, p => p.Value);
+    // Convert values in a dictionary from V to T where input dict has type `Dictionary<K, V>` and returns `Dictionary<K, T>`
+    public static Dictionary<K, T> ConvertValues<K, V, T>(this Dictionary<K, V> dict, Func<V, T> converter) where K : notnull
+        => dict.Select(p => (p.Key, Value: converter(p.Value))).ToDictionary(p => p.Key, p => p.Value);
+
+    // Convert values in a dictionary from (object)V to T where input dict has type `Dictionary<K, V>` and returns `Dictionary<K, T>`
+    // We receive maps as `Dictionary<GlideString, object>` or `Dictionary<String, object>` from `ResponseHandler`. This converter
+    // assumes that all values in a map have the same type V.
+    public static Dictionary<K, T> ConvertValues<K, V, T>(this Dictionary<K, object> dict, Func<V, T> converter) where K : notnull
+        => dict.Select(p => (p.Key, Value: converter((V)p.Value))).ToDictionary(p => p.Key, p => p.Value);
 
     // Get type name in format like "Dictionary<GlideString, GlideString>" (not "Dictionary`2")
     public static string GetRealTypeName(this Type t)