Skip to content
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.

Sign up for GitHub

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

fix(cmd): args should be parsed before retrieving keys in COMMAND GETKEYS #2661

Merged
merged 3 commits into from
Nov 14, 2024
Merged

fix(cmd): args should be parsed before retrieving keys in COMMAND GETKEYS #2661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
004bf11
fix(cmd): args should be parsed before retrieving keys in COMMAND GET…
PragmaTwice Nov 14, 2024
ee76803
Merge branch 'unstable' into fix-server-getkeys
PragmaTwice Nov 14, 2024
abbe688
fix test cases
PragmaTwice Nov 14, 2024
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
8 changes: 5 additions & 3 deletions src/cluster/cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,9 +835,11 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons
redis::Connection *conn, lua::ScriptRunCtx *script_run_ctx) {
std::vector<int> key_indexes;

auto s = redis::CommandTable::GetKeysFromCommand(attributes, cmd_tokens);
if (!s) return Status::OK();
key_indexes = *s;
attributes->ForEachKeyRange(
[&](const std::vector<std::string> &, redis::CommandKeyRange key_range) {
key_range.ForEachKeyIndex([&](int i) { key_indexes.push_back(i); }, cmd_tokens.size());
},
cmd_tokens);

if (key_indexes.empty()) return Status::OK();

Expand Down
5 changes: 5 additions & 0 deletions src/commands/commander.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ StatusOr<std::vector<int>> CommandTable::GetKeysFromCommand(const CommandAttribu
return {Status::NotOK, "Invalid number of arguments specified for command"};
}

auto cmd = attributes->factory();
if (auto s = cmd->Parse(cmd_tokens); !s) {
return {Status::NotOK, "Invalid syntax found in this command arguments: " + s.Msg()};
}

Status status;
std::vector<int> key_indexes;

Expand Down
2 changes: 2 additions & 0 deletions src/commands/commander.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ struct CommandAttributes {
return {Status::NotOK, "key range is unavailable without command arguments"};
}

// the command arguments must be parsed and in valid syntax
// before this method is called, otherwise the behavior is UNDEFINED
template <typename F, typename G>
void ForEachKeyRange(F &&f, const std::vector<std::string> &args, G &&g) const {
if (key_range_.first_key > 0) {
Expand Down
14 changes: 7 additions & 7 deletions src/storage/scripting.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,13 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {
auto *srv = conn->GetServer();
Config *config = srv->GetConfig();

cmd->SetArgs(args);
auto s = cmd->Parse();
if (!s) {
PushError(lua, s.Msg().data());
return raise_error ? RaiseError(lua) : 1;
}

if (config->cluster_enabled) {
if (script_run_ctx->flags & ScriptFlagType::kScriptNoCluster) {
PushError(lua, "Can not run script on cluster, 'no-cluster' flag is set");
Expand Down Expand Up @@ -807,13 +814,6 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {
return raise_error ? RaiseError(lua) : 1;
}

cmd->SetArgs(args);
auto s = cmd->Parse();
if (!s) {
PushError(lua, s.Msg().data());
return raise_error ? RaiseError(lua) : 1;
}

std::string output;
// TODO: make it possible for multiple redis commands in lua script to use the same txn context.
{
Expand Down
16 changes: 8 additions & 8 deletions tests/gocase/unit/command/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func TestCommand(t *testing.T) {
})

t.Run("COMMAND GETKEYS ZMPOP", func(t *testing.T) {
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "ZMPOP", "2", "key1", "key2")
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "ZMPOP", "2", "key1", "key2", "min")
vs, err := r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
Expand All @@ -189,7 +189,7 @@ func TestCommand(t *testing.T) {
})

t.Run("COMMAND GETKEYS BZMPOP", func(t *testing.T) {
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "BZMPOP", "0", "2", "key1", "key2")
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "BZMPOP", "0", "2", "key1", "key2", "min")
vs, err := r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
Expand All @@ -198,7 +198,7 @@ func TestCommand(t *testing.T) {
})

t.Run("COMMAND GETKEYS LMPOP", func(t *testing.T) {
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "LMPOP", "2", "key1", "key2")
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "LMPOP", "2", "key1", "key2", "left")
vs, err := r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
Expand All @@ -207,7 +207,7 @@ func TestCommand(t *testing.T) {
})

t.Run("COMMAND GETKEYS BLMPOP", func(t *testing.T) {
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "BLMPOP", "0", "2", "key1", "key2")
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "BLMPOP", "0", "2", "key1", "key2", "left")
vs, err := r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
Expand Down Expand Up @@ -250,30 +250,30 @@ func TestCommand(t *testing.T) {

t.Run("COMMAND GETKEYS GEORADIUSBYMEMBER", func(t *testing.T) {
// non-store
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m")
r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "100", "m")
vs, err := r.Slice()
require.NoError(t, err)
require.Len(t, vs, 1)
require.Equal(t, "src", vs[0])

// store
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "store", "dst")
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "100", "m", "store", "dst")
vs, err = r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
require.Equal(t, "src", vs[0])
require.Equal(t, "dst", vs[1])

// storedist
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "storedist", "dst")
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "100", "m", "storedist", "dst")
vs, err = r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
require.Equal(t, "src", vs[0])
require.Equal(t, "dst", vs[1])

// store + storedist
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "store", "dst1", "storedist", "dst2")
r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "100", "m", "store", "dst1", "storedist", "dst2")
vs, err = r.Slice()
require.NoError(t, err)
require.Len(t, vs, 2)
Expand Down
Loading