diff --git a/integration_tests/commands/http/json_arrpop_test.go b/integration_tests/commands/http/json_arrpop_test.go index 8a80b5e08..9741598ac 100644 --- a/integration_tests/commands/http/json_arrpop_test.go +++ b/integration_tests/commands/http/json_arrpop_test.go @@ -14,7 +14,7 @@ func TestJSONARRPOP(t *testing.T) { arrayAtRoot := []interface{}{0, 1, 2, 3} nestedArray := map[string]interface{}{"a": 2, "b": []interface{}{0, 1, 2, 3}} arrayWithinArray := map[string]interface{}{"a": 2, "b": []interface{}{0, 1, 2, []interface{}{3, 4, 5}}} - + simpleJson := map[string]interface{}{"a": 2} // Deleting the used keys exec.FireCommand(HTTPCommand{ Command: "DEL", @@ -122,6 +122,44 @@ func TestJSONARRPOP(t *testing.T) { }, expected: []interface{}{"OK", "ERR invalid JSONPath"}, }, + { + name: "key doesn't exist error", + commands: []HTTPCommand{ + { + Command: "JSON.ARRPOP", + Body: map[string]interface{}{"key": "doc_new"}, + }, + }, + expected: []interface{}{"ERR could not perform this operation on a key that doesn't exist"}, + }, + { + name: "arr pop on wrong key type", + commands: []HTTPCommand{ + { + Command: "SET", + Body: map[string]interface{}{"key": "doc_new", "value": "v1"}, + }, + { + Command: "JSON.ARRPOP", + Body: map[string]interface{}{"key": "doc_new"}, + }, + }, + expected: []interface{}{"OK", "ERR Existing key has wrong Dice type"}, + }, + { + name: "nil response for arr pop", + commands: []HTTPCommand{ + { + Command: "JSON.SET", + Body: map[string]interface{}{"key": "doc", "path": "$", "json": simpleJson}, + }, + { + Command: "JSON.ARRPOP", + Body: map[string]interface{}{"key": "doc", "path": "$.a", "index": "1"}, + }, + }, + expected: []interface{}{"OK", []interface{}{nil}}, + }, } for _, tc := range testCases { diff --git a/internal/server/httpServer.go b/internal/server/httpServer.go index 8862fed62..c15e29716 100644 --- a/internal/server/httpServer.go +++ b/internal/server/httpServer.go @@ -12,6 +12,8 @@ import ( "sync" "time" + "github.com/dicedb/dice/internal/eval" + "github.com/dicedb/dice/config" "github.com/dicedb/dice/internal/clientio" "github.com/dicedb/dice/internal/cmd" @@ -22,14 +24,15 @@ import ( "github.com/dicedb/dice/internal/shard" ) -const Abort = "ABORT" +const ( + Abort = "ABORT" + stringNil = "(nil)" +) var unimplementedCommands = map[string]bool{ "Q.UNWATCH": true, } -const stringNil = "(nil)" - type HTTPServer struct { shardManager *shard.ShardManager ioChan chan *ops.StoreResponse @@ -326,33 +329,23 @@ func (s *HTTPServer) writeQWatchResponse(writer http.ResponseWriter, response in } func (s *HTTPServer) writeResponse(writer http.ResponseWriter, result *ops.StoreResponse, diceDBCmd *cmd.DiceDBCmd) { + var ( + responseValue interface{} + err error + httpResponse utils.HTTPResponse + isDiceErr bool + ) + + // Check if the command is migrated, if it is we use EvalResponse values + // else we use RESPParser to decode the response _, ok := WorkerCmdsMeta[diceDBCmd.Cmd] - var rp *clientio.RESPParser - - var responseValue interface{} - var isDiceErr bool = false - var httpResponse utils.HTTPResponse // TODO: Remove this conditional check and if (true) condition when all commands are migrated if !ok { - var err error - if result.EvalResponse.Error != nil { - rp = clientio.NewRESPParser(bytes.NewBuffer([]byte(result.EvalResponse.Error.Error()))) - } else { - rp = clientio.NewRESPParser(bytes.NewBuffer(result.EvalResponse.Result.([]byte))) - } - - res, err := rp.DecodeOne() - responseValue = replaceNilInInterface(res) + responseValue, err = decodeEvalResponse(result.EvalResponse) if err != nil { s.logger.Error("Error decoding response", "error", err) - httpResponse := utils.HTTPResponse{Status: utils.HTTPStatusError, Data: "Internal Server Error"} - responseJSON, _ := json.Marshal(httpResponse) - writer.Header().Set("Content-Type", "application/json") - writer.WriteHeader(http.StatusInternalServerError) // Set HTTP status code to 500 - _, err = writer.Write(responseJSON) - if err != nil { - s.logger.Error("Error writing response", "error", err) - } + httpResponse = utils.HTTPResponse{Status: utils.HTTPStatusError, Data: "Internal Server Error"} + writeJSONResponse(writer, httpResponse, http.StatusInternalServerError) return } } else { @@ -364,7 +357,55 @@ func (s *HTTPServer) writeResponse(writer http.ResponseWriter, result *ops.Store } } - // func HandlePredefinedResponse(response interface{}) []byte { + // Create the HTTP response + httpResponse = createHTTPResponse(responseValue) + if isDiceErr { + httpResponse.Status = utils.HTTPStatusError + } else { + httpResponse.Status = utils.HTTPStatusSuccess + } + + // Write the response back to the client + writeJSONResponse(writer, httpResponse, http.StatusOK) +} + +// Helper function to decode EvalResponse based on the error or result +func decodeEvalResponse(evalResp *eval.EvalResponse) (interface{}, error) { + var rp *clientio.RESPParser + + if evalResp.Error != nil { + rp = clientio.NewRESPParser(bytes.NewBuffer([]byte(evalResp.Error.Error()))) + } else { + rp = clientio.NewRESPParser(bytes.NewBuffer(evalResp.Result.([]byte))) + } + + res, err := rp.DecodeOne() + if err != nil { + return nil, err + } + + return replaceNilInInterface(res), nil +} + +// Helper function to write the JSON response +func writeJSONResponse(writer http.ResponseWriter, response utils.HTTPResponse, statusCode int) { + writer.Header().Set("Content-Type", "application/json") + writer.WriteHeader(statusCode) + + responseJSON, err := json.Marshal(response) + if err != nil { + slog.Error("Error marshaling response", "error", err) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return + } + + _, err = writer.Write(responseJSON) + if err != nil { + slog.Error("Error writing response", "error", err) + } +} + +func createHTTPResponse(responseValue interface{}) utils.HTTPResponse { respArr := []string{ "(nil)", // Represents a RESP Nil Bulk String, which indicates a null value. "OK", // Represents a RESP Simple String with value "OK". @@ -376,36 +417,43 @@ func (s *HTTPServer) writeResponse(writer http.ResponseWriter, result *ops.Store "*0", // Represents an empty RESP Array. } - if val, ok := responseValue.(clientio.RespType); ok { - responseValue = respArr[val] - } + switch v := responseValue.(type) { + case []interface{}: + // Parses []interface{} as part of EvalResponse e.g. JSON.ARRPOP + // and adds to httpResponse. Also replaces "(nil)" with null JSON value + // in response array. + r := make([]interface{}, 0, len(v)) + for _, resp := range v { + if val, ok := resp.(clientio.RespType); ok { + if stringNil == respArr[val] { + r = append(r, nil) + } else { + r = append(r, respArr[val]) + } + } else { + r = append(r, resp) + } + } + return utils.HTTPResponse{Data: r} - if responseValue == stringNil { - responseValue = nil // in order to convert it in json null - } + case []byte: + return utils.HTTPResponse{Data: string(v)} - if bt, ok := responseValue.([]byte); ok { - responseValue = string(bt) - } - if isDiceErr { - httpResponse = utils.HTTPResponse{Status: utils.HTTPStatusError, Data: responseValue} - } else { - httpResponse = utils.HTTPResponse{Status: utils.HTTPStatusSuccess, Data: responseValue} - } + case clientio.RespType: + responseValue = respArr[v] + if responseValue == stringNil { + responseValue = nil // in order to convert it in json null + } - responseJSON, err := json.Marshal(httpResponse) - if err != nil { - s.logger.Error("Error marshaling response", "error", err) - http.Error(writer, "Internal Server Error", http.StatusInternalServerError) - return - } + return utils.HTTPResponse{Data: responseValue} - writer.Header().Set("Content-Type", "application/json") - writer.WriteHeader(http.StatusOK) - _, err = writer.Write(responseJSON) - if err != nil { - s.logger.Error("Error writing response", "error", err) + case interface{}: + if val, ok := v.(clientio.RespType); ok { + return utils.HTTPResponse{Data: respArr[val]} + } } + + return utils.HTTPResponse{Data: responseValue} } func generateUniqueInt32(r *http.Request) uint32 {