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.

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

#1100: Fix LPOP to support multiple arguments #1231

Merged
merged 8 commits into from
Nov 8, 2024
58 changes: 55 additions & 3 deletions integration_tests/commands/async/deque_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,17 @@ func TestRPush(t *testing.T) {
}{
{
name: "RPUSH",
cmds: []string{"LPUSH k v", "LPUSH k v1 1 v2 2", "LPUSH k 3 3 3 v3 v3 v3"},
cmds: []string{"RPUSH k v", "RPUSH k v1 1 v2 2", "RPUSH k 3 3 3 v3 v3 v3"},
expect: []any{int64(1), int64(5), int64(11)},
},
{
name: "RPUSH normal values",
cmds: []string{"LPUSH k " + strings.Join(deqNormalValues, " ")},
cmds: []string{"RPUSH k " + strings.Join(deqNormalValues, " ")},
expect: []any{int64(25)},
},
{
name: "RPUSH edge values",
cmds: []string{"LPUSH k " + strings.Join(deqEdgeValues, " ")},
cmds: []string{"RPUSH k " + strings.Join(deqEdgeValues, " ")},
JyotinderSingh marked this conversation as resolved.
Show resolved Hide resolved
expect: []any{int64(42)},
},
}
Expand Down Expand Up @@ -442,6 +442,58 @@ func TestLLEN(t *testing.T) {
deqCleanUp(conn, "k")
}

func TestLPOPCount(t *testing.T) {
deqTestInit()
conn := getLocalConnection()
defer conn.Close()

testCases := []struct {
name string
cmds []string
expect []interface{}
}{
{
name: "LPOP with count argument - valid, invalid, and edge cases",
cmds: []string{
"RPUSH k v1 v2 v3 v4",
"LPOP k 2",
"LLEN k",
"LPOP k 0",
"LLEN k",
"LPOP k 5",
"LLEN k",
"LPOP k -1",
"LPOP k abc",
"LLEN k",
},
expect: []any{
int64(4),
[]interface{}{"v1", "v2"},
int64(2),
[]interface{}{},
int64(2),
[]interface{}{"v3", "v4"},
int64(0),
"ERR value is out of range",
"ERR value is not an integer or out of range",
int64(0),
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
for i, cmd := range tc.cmds {
result := FireCommand(conn, cmd)
assert.Equal(t, tc.expect[i], result)
}
})
}

deqCleanUp(conn, "k")

}

func deqCleanUp(conn net.Conn, key string) {
for {
result := FireCommand(conn, "LPOP "+key)
Expand Down
48 changes: 48 additions & 0 deletions integration_tests/commands/http/deque_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,3 +556,51 @@ func TestLLEN(t *testing.T) {

exec.FireCommand(HTTPCommand{Command: "DEL", Body: map[string]interface{}{"keys": [...]string{"k"}}})
}

func TestLPOPCount(t *testing.T) {
deqTestInit()
exec := NewHTTPCommandExecutor()

testCases := []struct {
name string
cmds []HTTPCommand
expect []any
}{
{
name: "LPOP with count argument - valid, invalid, and edge cases",
cmds: []HTTPCommand{
{Command: "RPUSH", Body: map[string]interface{}{"key": "k", "value": "v1"}},
{Command: "RPUSH", Body: map[string]interface{}{"key": "k", "value": "v2"}},
{Command: "RPUSH", Body: map[string]interface{}{"key": "k", "value": "v3"}},
{Command: "RPUSH", Body: map[string]interface{}{"key": "k", "value": "v4"}},
{Command: "LPOP", Body: map[string]interface{}{"key": "k", "value": 2}},
{Command: "LPOP", Body: map[string]interface{}{"key": "k", "value": 2}},
{Command: "LPOP", Body: map[string]interface{}{"key": "k", "value": -1}},
{Command: "LPOP", Body: map[string]interface{}{"key": "k", "value": "abc"}},
{Command: "LLEN", Body: map[string]interface{}{"key": "k"}},
},
expect: []any{
float64(1),
float64(2),
float64(3),
float64(4),
[]interface{}{"v1", "v2"},
[]interface{}{"v3", "v4"},
"ERR value is out of range",
"ERR value is not an integer or out of range",
float64(0),
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
exec.FireCommand(HTTPCommand{Command: "DEL", Body: map[string]interface{}{"keys": []interface{}{"k"}}})
for i, cmd := range tc.cmds {
result, _ := exec.FireCommand(cmd)
assert.Equal(t, tc.expect[i], result, "Value mismatch for cmd %v", cmd)
}
})
}
}

58 changes: 58 additions & 0 deletions integration_tests/commands/websocket/deque_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package websocket

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestLPOPCount(t *testing.T) {
exec := NewWebsocketCommandExecutor()

testCases := []struct {
name string
commands []string
expected []interface{}
cleanupKey string
}{
{
name: "LPOP with count argument - valid, invalid, and edge cases",
commands: []string{
"RPUSH k v1",
"RPUSH k v2",
"RPUSH k v3",
"RPUSH k v4",
"LPOP k 2",
"LPOP k 2",
"LPOP k -1",
"LPOP k abc",
"LLEN k",
},
expected: []any{
float64(1),
float64(2),
float64(3),
float64(4),
[]interface{}{"v1", "v2"},
[]interface{}{"v3", "v4"},
"ERR value is out of range",
"ERR value is not an integer or out of range",
float64(0),
},
cleanupKey: "k",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
conn := exec.ConnectToServer()

for i, cmd := range tc.commands {
result, err := exec.FireCommandAndReadResponse(conn, cmd)
assert.Nil(t, err)
assert.Equal(t, tc.expected[i], result)
}
DeleteKey(t, conn, exec, tc.cleanupKey)
})
}
}

49 changes: 42 additions & 7 deletions internal/eval/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -2157,10 +2157,31 @@ func evalRPOP(args []string, store *dstore.Store) []byte {
}

func evalLPOP(args []string, store *dstore.Store) []byte {
if len(args) != 1 {
// By default we pop only 1
popNumber := 1

// LPOP accepts 1 or 2 arguments only - LPOP key [count]
if len(args) < 1 || len(args) > 2 {
return diceerrors.NewErrArity("LPOP")
}

// to updated the number of pops
if len(args) == 2 {
nos, err := strconv.Atoi(args[1])
if err != nil {
return diceerrors.NewErrWithFormattedMessage(diceerrors.IntOrOutOfRangeErr)
}
if nos == 0 {
// returns empty string if count given is 0
return clientio.Encode([]string{}, false)
}
if nos < 0 {
// returns an out of range err if count is negetive
return diceerrors.NewErrWithFormattedMessage(diceerrors.ValOutOfRangeErr)
}
popNumber = nos
}

obj := store.Get(args[0])
if obj == nil {
return clientio.RespNIL
Expand All @@ -2180,15 +2201,29 @@ func evalLPOP(args []string, store *dstore.Store) []byte {
}

deq := obj.Value.(*Deque)
x, err := deq.LPop()
if err != nil {
if errors.Is(err, ErrDequeEmpty) {
return clientio.RespNIL

// holds the elements popped
var str []string
JyotinderSingh marked this conversation as resolved.
Show resolved Hide resolved
for iter := 0; iter < popNumber; iter++ {
x, err := deq.LPop()
if err != nil {
if errors.Is(err, ErrDequeEmpty) {
break
}
panic(fmt.Sprintf("unknown error: %v", err))
}
panic(fmt.Sprintf("unknown error: %v", err))
str = append(str, x)
}

return clientio.Encode(x, false)
if len(str) == 0 {
return clientio.RespNIL
}

if len(str) == 1 {
return clientio.Encode(str[0], false)
}

return clientio.Encode(str, false)
}

func evalLLEN(args []string, store *dstore.Store) []byte {
Expand Down
56 changes: 56 additions & 0 deletions internal/eval/eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,14 @@ func TestEval(t *testing.T) {
testEvalHEXISTS(t, store)
testEvalHDEL(t, store)
testEvalHSCAN(t, store)
testEvalPFMERGE(t, store)
testEvalJSONSTRLEN(t, store)
testEvalJSONOBJLEN(t, store)
testEvalHLEN(t, store)
testEvalSELECT(t, store)
testEvalLLEN(t, store)
testEvalGETDEL(t, store)
testEvalGETEX(t, store)
testEvalJSONNUMINCRBY(t, store)
testEvalDUMP(t, store)
testEvalTYPE(t, store)
Expand All @@ -118,6 +120,8 @@ func TestEval(t *testing.T) {
testEvalZRANK(t, store)
testEvalZCARD(t, store)
testEvalZREM(t, store)
testEvalZADD(t, store)
testEvalZRANGE(t, store)
testEvalHVALS(t, store)
testEvalBitField(t, store)
testEvalHINCRBYFLOAT(t, store)
Expand All @@ -137,6 +141,7 @@ func TestEval(t *testing.T) {
testEvalBFADD(t, store)
testEvalLINSERT(t, store)
testEvalLRANGE(t, store)
testEvalLPOP(t, store)
}

func testEvalPING(t *testing.T, store *dstore.Store) {
Expand Down Expand Up @@ -3779,6 +3784,57 @@ func testEvalLLEN(t *testing.T, store *dstore.Store) {
runEvalTests(t, tests, evalLLEN, store)
}

func testEvalLPOP(t *testing.T, store *dstore.Store) {
tests := map[string]evalTestCase{
"empty args": {
input: nil,
output: []byte("-ERR wrong number of arguments for 'lpop' command\r\n"),
},
"more than 2 args": {
input: []string{"k", "2", "3"},
output: []byte("-ERR wrong number of arguments for 'lpop' command\r\n"),
},
"pop one element": {
setup: func() {
evalRPUSH([]string{"k", "v1", "v2", "v3", "v4"}, store)
},
input: []string{"k"},
output: []byte("$2\r\nv1\r\n"),
},
"pop two elements": {
setup: func() {
evalRPUSH([]string{"k", "v1", "v2", "v3", "v4"}, store)
},
input: []string{"k", "2"},
output: []byte("*2\r\n$2\r\nv1\r\n$2\r\nv2\r\n")},
"pop more elements than available": {
setup: func() {
evalRPUSH([]string{"k", "v1", "v2"}, store)
},
input: []string{"k", "5"},
output: []byte("*2\r\n$2\r\nv1\r\n$2\r\nv2\r\n")},
"pop 0 elements": {
setup: func() {
evalRPUSH([]string{"k", "v1", "v2"}, store)
},
input: []string{"k", "0"},
output: []byte("*0\r\n")},
"negative count": {
input: []string{"k", "-1"},
output: []byte("-ERR value is out of range\r\n"),
},
"non-integer count": {
input: []string{"k", "abc"},
output: []byte("-ERR value is not an integer or out of range\r\n"),
},
"key does not exist": {
input: []string{"nonexistent_key"},
output: []byte("$-1\r\n"),
},
}
runEvalTests(t, tests, evalLPOP, store)
}

func testEvalJSONNUMINCRBY(t *testing.T, store *dstore.Store) {
tests := map[string]evalTestCase{
"incr on numeric field": {
Expand Down