Skip to content

C#: Add response checks for batch for every command #3803

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions csharp/sources/Valkey.Glide/BaseClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ 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);
object?[]? value = HandleServerResponse<object?[]?>(await message, true);
return batch.ConvertResponse(value);

// All memory allocated is auto-freed by `using` operator
}
Expand Down Expand Up @@ -165,7 +166,7 @@ protected internal static T HandleServerResponse<R, T>(IntPtr response, bool isN
return null;
#pragma warning restore CS8603 // Possible null reference return.
}
throw new Exception($"Unexpected return type from Glide: got null expected {typeof(T).GetRealTypeName()}");
throw new RequestException($"Unexpected return type from Glide: got null expected {typeof(T).GetRealTypeName()}");
}
return value is R
? converter((value as R)!)
Expand Down
83 changes: 63 additions & 20 deletions csharp/sources/Valkey.Glide/Pipeline/BaseBatch.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0

using System.Diagnostics;

using Valkey.Glide.Internals;

using static Valkey.Glide.Commands.Options.InfoOptions;
using static Valkey.Glide.Errors;

using RequestType = Valkey.Glide.Internals.FFI.RequestType;

Expand All @@ -25,45 +28,85 @@ namespace Valkey.Glide.Pipeline;
public abstract class BaseBatch<T>(bool isAtomic) : IBatch where T : BaseBatch<T>
{
private readonly List<FFI.Cmd> _commands = [];
private readonly List<Func<object?, object?>> _converters = [];

internal bool IsAtomic { get; private set; } = isAtomic;

internal FFI.Batch ToFFI() => new([.. _commands], IsAtomic);

/// <inheritdoc cref="IBatch.CustomCommand(GlideString[])" />
public T CustomCommand(GlideString[] args)
/// <summary>
/// Convert a response reseived from the server.
/// </summary>
internal object?[]? ConvertResponse(object?[]? response)
{
_commands.Add(new(RequestType.CustomCommand, args));
return (T)this;
if (response is null)
{
return null;
}

Debug.Assert(response?.Length == _converters.Count, "Converteds misaligned");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
Debug.Assert(response?.Length == _converters.Count, "Converteds misaligned");
Debug.Assert(response?.Length == _converters.Count, "Converters misaligned");


for (int i = 0; i < response?.Length; i++)
{
response[i] = _converters[i](response[i]);
}
return response;
}

/// <inheritdoc cref="IBatch.Get(GlideString)" />
public T Get(GlideString key)
/// <summary>
/// Create a type checker for the response value.
/// </summary>
/// <typeparam name="V">Expected value type</typeparam>
protected Func<object?, object?> MakeTypeChecker<V>(bool isNullable = false) where V : class?
=> MakeTypeCheckerAndConverter<V, V>(r => r, isNullable);

/// <summary>
/// Create a function which checks response value type and converts it.
/// </summary>
/// <typeparam name="V">Expected value type in server response</typeparam>
/// <typeparam name="R">Resulting value type after conversion</typeparam>
protected Func<object?, object?> MakeTypeCheckerAndConverter<V, R>(Func<V, R> converter, bool isNullable = false) where V : class? where R : class?
=> value =>
{
#pragma warning disable IDE0046 // Convert to conditional expression
if (value is null)
{
return isNullable
? null
: (object)new RequestException($"Unexpected return type from Glide: got null expected {typeof(V).GetRealTypeName()}");
}
#pragma warning restore IDE0046 // Convert to conditional expression
return value is V
? converter((value as V)!)
: new RequestException($"Unexpected return type from Glide: got {value?.GetType().GetRealTypeName()} expected {typeof(V).GetRealTypeName()}");
};

internal T AddCmd(FFI.Cmd cmd, Func<object?, object?> converter)
{
_commands.Add(new(RequestType.Get, [key]));
_commands.Add(cmd);
_converters.Add(converter);
return (T)this;
}

/// <inheritdoc cref="IBatch.CustomCommand(GlideString[])" />
public T CustomCommand(GlideString[] args)
=> AddCmd(new(RequestType.CustomCommand, args), MakeTypeChecker<object>(true));

/// <inheritdoc cref="IBatch.Get(GlideString)" />
public T Get(GlideString key)
=> AddCmd(new(RequestType.Get, [key]), MakeTypeChecker<GlideString>(true));

/// <inheritdoc cref="IBatch.Set(GlideString, GlideString)" />
public T Set(GlideString key, GlideString value)
{
_commands.Add(new(RequestType.Set, [key, value]));
return (T)this;
}
=> AddCmd(new(RequestType.Set, [key, value]), MakeTypeChecker<string>());

/// <inheritdoc cref="IBatch.Info()" />
public T Info()
{
_commands.Add(new(RequestType.Info, []));
return (T)this;
}
public T Info() => Info([]);

/// <inheritdoc cref="IBatch.Info(Section[])" />
public T Info(Section[] sections)
{
_commands.Add(new(RequestType.Info, sections.ToGlideStrings()));
return (T)this;
}
=> AddCmd(new(RequestType.Info, sections.ToGlideStrings()),
MakeTypeCheckerAndConverter<GlideString, string>(gs => gs.ToString()));

IBatch IBatch.CustomCommand(GlideString[] args) => CustomCommand(args);
IBatch IBatch.Get(GlideString key) => Get(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,19 +105,19 @@ public async Task BatchWithSingleNodeRoute(bool isAtomic)
ClusterBatch batch = new ClusterBatch(isAtomic).Info([Section.REPLICATION]);

object?[]? res = await client.Exec(batch, true, new(route: new SlotKeyRoute("abc", SlotType.Primary)));
Assert.Contains("role:master", res![0] as gs);
Assert.Contains("role:master", res![0] as string);

res = await client.Exec(batch, true, new(route: new SlotKeyRoute("abc", SlotType.Replica)));
Assert.Contains("role:slave", res![0] as gs);
Assert.Contains("role:slave", res![0] as string);

res = await client.Exec(batch, true, new(route: new SlotIdRoute(42, SlotType.Primary)));
Assert.Contains("role:master", res![0] as gs);
Assert.Contains("role:master", res![0] as string);

res = await client.Exec(batch, true, new(route: new SlotIdRoute(42, SlotType.Replica)));
Assert.Contains("role:slave", res![0] as gs);
Assert.Contains("role:slave", res![0] as string);

res = await client.Exec(batch, true, new(route: new ByAddressRoute(TestConfiguration.CLUSTER_HOSTS[0].host, TestConfiguration.CLUSTER_HOSTS[0].port)));
Assert.Contains("# Replication", res![0] as gs);
Assert.Contains("# Replication", res![0] as string);
}

[Fact]
Expand Down
Loading