Implement GetServer(RedisKey, ...) #2936
Conversation
mgravell
commented
Jul 30, 2025
mgravell
commented
- implement GetServer API from New API proposal: improve support for pinned server ad-hoc commands #2932, and tests
- implement memoization on GetServer when asyncState is null
- fixup NotImplementedException in dummy places
- implement memoization on GetServer when asyncState is null - fixup NotImplementedException in dummy places
| /// <param name="args">The arguments to pass for the command.</param> | ||
| /// <returns>A dynamic representation of the command's result.</returns> | ||
| /// <remarks>This API should be considered an advanced feature; inappropriate use can be harmful.</remarks> | ||
| RedisResult Execute(int? database, string command, params object[] args); |
There was a problem hiding this comment.
Should database be nullable here? If you're using this overload, null should never be acceptable right? I'm curious if that removes the need for RS0026/RS0027 suppression
There was a problem hiding this comment.
Discussed in more depth in #2932, but short answer: "it is intentional"; null here means "behave the same as GetDatabase(...) does if you don't add a default" - very intentionally this is a database-oriented command, but using the configured default database. The parameters are ordered this way to avoid any ambiguity vs the command, i.e. it can't be optional because it is first, and we need command, and if we reversed the order it would be ambiguous vs string, params object[].
| } | ||
|
|
||
| public RedisResult Execute(int? database, string command, params object[] args) | ||
| => Execute(database, command, args, flags: CommandFlags.None); |
There was a problem hiding this comment.
I wonder if we should add this overload at all, given you can do [arg1, arg2, arg3] etc. so cleanly in current C#. Perhaps we stop adding these types of overloads?
There was a problem hiding this comment.
agreed and done; that removed the need for #pragma, too; win:win
|
Main feedback: the RS0026/RS0027 suppression gives me pause here, since that's commonly a source of feedback especially with |
|
OK to tick with the |
| /// <param name="flags">The flags to use for this operation.</param> | ||
| /// <returns>A dynamic representation of the command's result.</returns> | ||
| /// <remarks>This API should be considered an advanced feature; inappropriate use can be harmful.</remarks> | ||
| RedisResult Execute(int? database, string command, ICollection<object> args, CommandFlags flags = CommandFlags.None); |
There was a problem hiding this comment.
Is the pragma needed here, if database isn't nullable?
|
Removed; still Todo: think more about the dB Vs non-db execute naming |
