-
Notifications
You must be signed in to change notification settings - Fork 543
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
[Compatibility] Added COMMAND GETKEYS and GETKEYSANDFLAGS command #888
base: main
Are you sure you want to change the base?
Changes from all commits
6460ad6
6a7f6b4
c6d0633
f5ec1cd
923664a
3516f7a
de1089c
fcca08b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// Licensed under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Runtime.CompilerServices; | ||
using System.Text; | ||
|
@@ -1149,6 +1150,230 @@ private bool NetworkCOMMAND_INFO() | |
return true; | ||
} | ||
|
||
/// <summary> | ||
/// Processes COMMAND GETKEYS subcommand. | ||
/// </summary> | ||
private bool NetworkCOMMAND_GETKEYS() | ||
{ | ||
if (parseState.Count == 0) | ||
{ | ||
return AbortWithWrongNumberOfArguments(nameof(RespCommand.COMMAND_GETKEYS)); | ||
} | ||
|
||
var cmdName = parseState.GetString(0).ToUpperInvariant(); | ||
bool cmdFound = RespCommandsInfo.TryGetRespCommandInfo(cmdName, out var cmdInfo, true, true, logger) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think includeSubCommands should be false (I don't believe sub-commands should have key specs - please verify) |
||
storeWrapper.customCommandManager.TryGetCustomCommandInfo(cmdName, out cmdInfo); | ||
|
||
if (!cmdFound) | ||
{ | ||
return AbortWithErrorMessage(CmdStrings.RESP_INVALID_COMMAND_SPECIFIED); | ||
} | ||
|
||
if (cmdInfo.KeySpecifications == null || cmdInfo.KeySpecifications.Length == 0) | ||
{ | ||
return AbortWithErrorMessage(CmdStrings.RESP_COMMAND_HAS_NO_KEY_ARGS); | ||
} | ||
|
||
var keyList = new List<byte[]>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have to allocate a byte[] for each key? The keys already exist in the parse state. |
||
foreach (var spec in cmdInfo.KeySpecifications) | ||
{ | ||
ExtractKeys(spec, keyList); | ||
} | ||
|
||
while (!RespWriteUtils.WriteArrayLength(keyList.Count, ref dcurr, dend)) | ||
SendAndReset(); | ||
|
||
foreach (var key in keyList) | ||
{ | ||
while (!RespWriteUtils.WriteBulkString(key, ref dcurr, dend)) | ||
SendAndReset(); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/// <summary> | ||
/// Processes COMMAND GETKEYSANDFLAGS subcommand. | ||
/// </summary> | ||
private bool NetworkCOMMAND_GETKEYSANDFLAGS() | ||
{ | ||
if (parseState.Count == 0) | ||
{ | ||
return AbortWithWrongNumberOfArguments(nameof(RespCommand.COMMAND_GETKEYSANDFLAGS)); | ||
} | ||
|
||
var cmdName = parseState.GetString(0).ToUpperInvariant(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See same comments as in GETKEYS |
||
bool cmdFound = RespCommandsInfo.TryGetRespCommandInfo(cmdName, out var cmdInfo, true, true, logger) || | ||
storeWrapper.customCommandManager.TryGetCustomCommandInfo(cmdName, out cmdInfo); | ||
|
||
if (!cmdFound) | ||
{ | ||
return AbortWithErrorMessage(CmdStrings.RESP_INVALID_COMMAND_SPECIFIED); | ||
} | ||
|
||
if (cmdInfo.KeySpecifications == null || cmdInfo.KeySpecifications.Length == 0) | ||
{ | ||
return AbortWithErrorMessage(CmdStrings.RESP_COMMAND_HAS_NO_KEY_ARGS); | ||
} | ||
|
||
var keyList = new List<byte[]>(); | ||
var flagsList = new List<string[]>(); | ||
|
||
foreach (var spec in cmdInfo.KeySpecifications) | ||
{ | ||
var keyCount = keyList.Count; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: just for clarity, should probably be named prevKeyCount |
||
ExtractKeys(spec, keyList); | ||
var flags = EnumUtils.GetEnumDescriptions(spec.Flags); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spec.respFormatFlags should already contain this info, you can create a public getter than returns this field. |
||
for (int i = keyCount; i < keyList.Count; i++) | ||
{ | ||
flagsList.Add(flags); | ||
} | ||
} | ||
|
||
while (!RespWriteUtils.WriteArrayLength(keyList.Count, ref dcurr, dend)) | ||
SendAndReset(); | ||
|
||
for (int i = 0; i < keyList.Count; i++) | ||
{ | ||
while (!RespWriteUtils.WriteArrayLength(2, ref dcurr, dend)) | ||
SendAndReset(); | ||
|
||
while (!RespWriteUtils.WriteBulkString(keyList[i], ref dcurr, dend)) | ||
SendAndReset(); | ||
|
||
while (!RespWriteUtils.WriteArrayLength(flagsList[i].Length, ref dcurr, dend)) | ||
SendAndReset(); | ||
|
||
foreach (var flag in flagsList[i]) | ||
{ | ||
while (!RespWriteUtils.WriteBulkString(Encoding.ASCII.GetBytes(flag), ref dcurr, dend)) | ||
SendAndReset(); | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private void ExtractKeys(RespCommandKeySpecification spec, List<byte[]> keyList) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I would probably do is have this as an extension method to SessionParseState (e.g. TryGetKeysFromKeySpecs) that takes an array of RespCommandKeySpecifications. |
||
{ | ||
int startIndex = 0; | ||
|
||
if (spec.BeginSearch is BeginSearchIndex bsIndex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can take advantage of the OO design here and handle each class-specific scenario in the class implementation itself. |
||
{ | ||
startIndex = bsIndex.Index; | ||
if (startIndex < 0) | ||
startIndex = parseState.Count + startIndex; | ||
} | ||
else if (spec.BeginSearch is BeginSearchKeyword bsKeyword) | ||
{ | ||
// Handle negative StartFrom by converting to positive index from end | ||
int searchStartIndex = bsKeyword.StartFrom; | ||
if (searchStartIndex < 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can probably avoid having 2 loops that do the same thing here, just have a condition for the index if the order is descending (this path is probably not as performance-critical). |
||
{ | ||
// Convert negative index to positive from end | ||
searchStartIndex = parseState.Count + searchStartIndex; | ||
|
||
// Search backwards from the calculated start position for the keyword | ||
for (int i = searchStartIndex; i >= 0; i--) | ||
{ | ||
if (parseState.GetArgSliceByRef(i).ReadOnlySpan.EqualsUpperCaseSpanIgnoringCase(Encoding.ASCII.GetBytes(bsKeyword.Keyword))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can store Encoding.ASCII.GetBytes(bsKeyword.Keyword) prior to the loop instead of re-evaluating it each time. |
||
{ | ||
startIndex = i + 1; | ||
break; | ||
} | ||
} | ||
} | ||
else | ||
{ | ||
// Search forwards from start position for the keyword | ||
for (int i = searchStartIndex; i < parseState.Count; i++) | ||
{ | ||
if (parseState.GetArgSliceByRef(i).ReadOnlySpan.EqualsUpperCaseSpanIgnoringCase(Encoding.ASCII.GetBytes(bsKeyword.Keyword))) | ||
{ | ||
startIndex = i + 1; | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
|
||
if (startIndex < 0 || startIndex >= parseState.Count) | ||
return; | ||
|
||
if (spec.FindKeys is FindKeysRange range) | ||
{ | ||
int lastKey; | ||
if (range.LastKey < 0) | ||
{ | ||
// For negative LastKey, calculate limit based on the factor | ||
int availableArgs = parseState.Count - startIndex; | ||
int limitFactor = range.Limit <= 1 ? availableArgs : availableArgs / range.Limit; | ||
|
||
// Calculate available slots based on keyStep | ||
int slotsAvailable = (limitFactor + range.KeyStep - 1) / range.KeyStep; | ||
lastKey = startIndex + (slotsAvailable * range.KeyStep) - range.KeyStep; | ||
} | ||
else | ||
{ | ||
lastKey = Math.Min(startIndex + range.LastKey, parseState.Count - 1); | ||
} | ||
|
||
for (int i = startIndex; i <= lastKey; i += range.KeyStep) | ||
{ | ||
if (i < parseState.Count) | ||
{ | ||
var value = parseState.GetArgSliceByRef(i).ToArray(); | ||
if (value.Length != 0) | ||
{ | ||
keyList.Add(value); | ||
} | ||
} | ||
} | ||
} | ||
else if (spec.FindKeys is FindKeysKeyNum keyNum) | ||
{ | ||
int numKeys = 0; | ||
int firstKey = startIndex + keyNum.FirstKey; | ||
|
||
// Handle negative FirstKey | ||
if (keyNum.FirstKey < 0) | ||
firstKey = parseState.Count + keyNum.FirstKey; | ||
|
||
// Get number of keys from the KeyNumIdx | ||
if (keyNum.KeyNumIdx >= 0) | ||
{ | ||
var keyNumPos = startIndex + keyNum.KeyNumIdx; | ||
if (keyNumPos < parseState.Count && parseState.TryGetInt(keyNumPos, out var count)) | ||
{ | ||
numKeys = count; | ||
} | ||
} | ||
else | ||
{ | ||
// Negative KeyNumIdx means count from the end | ||
var keyNumPos = parseState.Count + keyNum.KeyNumIdx; | ||
if (keyNumPos >= 0 && parseState.TryGetInt(keyNumPos, out var count)) | ||
{ | ||
numKeys = count; | ||
} | ||
} | ||
|
||
// Extract keys based on numKeys, firstKey, and keyStep | ||
if (numKeys > 0 && firstKey >= 0) | ||
{ | ||
for (int i = 0; i < numKeys && firstKey + i * keyNum.KeyStep < parseState.Count; i++) | ||
{ | ||
var keyIndex = firstKey + i * keyNum.KeyStep; | ||
var value = parseState.GetArgSliceByRef(keyIndex).ToArray(); | ||
if (value.Length != 0) | ||
{ | ||
keyList.Add(value); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
private bool NetworkECHO() | ||
{ | ||
if (parseState.Count != 1) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -233,7 +233,7 @@ private static IReadOnlyDictionary<string, RespCommandDocs> GetUpdatedCommandsDo | |
} | ||
|
||
// Update commands docs with commands to add | ||
foreach (var command in commandsToAdd.Keys) | ||
foreach (var command in commandsToAdd.Keys.Where(x => x.Command.StartsWith("COMMAND"))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mistake? |
||
{ | ||
RespCommandDocs baseCommandDocs; | ||
List<RespCommandDocs> updatedSubCommandsDocs; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for ToUpperInvariant(), the TryGet... methods already take care of that.