Error properly on graph accesses of non-graph keys (RedisGraph#1069)

* Error properly on graph accesses of non-graph keys

* Explicitly free QueryCtx on failed delete

* Update cmd_delete.c

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

src/commands/cmd_delete.c

@@ -20,13 +20,15 @@ extern RedisModuleType *GraphContextRedisModuleType;
 int MGraph_Delete(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
 	if(argc != 2) return RedisModule_WrongArity(ctx);
 
+	int res = REDISMODULE_OK;
 	char *strElapsed = NULL;
 	QueryCtx_BeginTimer(); // Start deletion timing.
 
 	RedisModuleString *graph_name = argv[1];
 	GraphContext *gc = GraphContext_Retrieve(ctx, graph_name, false, false);    // Increase ref count.
+	// If the GraphContext is null, key access failed and an error has been emitted.
 	if(!gc) {
-		RedisModule_ReplyWithError(ctx, "Graph is either missing or referred key is of a different type.");
+		res = REDISMODULE_ERR;
 		goto cleanup;
 	}
 
@@ -39,12 +41,12 @@ int MGraph_Delete(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
 	double t = QueryCtx_GetExecutionTime();
 	asprintf(&strElapsed, "Graph removed, internal execution time: %.6f milliseconds", t);
 	RedisModule_ReplyWithStringBuffer(ctx, strElapsed, strlen(strElapsed));
+
+	// Delete commands should always modify slaves.
+	RedisModule_ReplicateVerbatim(ctx);
 
 cleanup:
 	QueryCtx_Free(); // Reset the QueryCtx and free its allocations.
 	if(strElapsed) free(strElapsed);
-	// Delete commands should always modify slaves.
-	RedisModule_ReplicateVerbatim(ctx);
-	return REDISMODULE_OK;
+	return res;
 }
-

src/commands/cmd_dispatcher.c

@@ -66,6 +66,8 @@ int CommandDispatch(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
 	if(_validate_command_arity(cmd, argc) == false) return RedisModule_WrongArity(ctx);
 	Command_Handler handler = get_command_handler(cmd);
 	GraphContext *gc = GraphContext_Retrieve(ctx, graph_name, true, true);
+	// If the GraphContext is null, key access failed and an error has been emitted.
+	if(!gc) return REDISMODULE_ERR;
 
 	/* Determin query execution context
 	 * queries issued within a LUA script or multi exec block must

src/graph/graphcontext.c

@@ -86,14 +86,21 @@ GraphContext *GraphContext_Retrieve(RedisModuleCtx *ctx, RedisModuleString *grap
 
 	RedisModuleKey *key = RedisModule_OpenKey(ctx, graphID, rwFlag);
 	if(RedisModule_KeyType(key) == REDISMODULE_KEYTYPE_EMPTY) {
-		// Key doesn't exists, create it.
 		if(shouldCreate) {
+			// Key doesn't exist, create it.
 			const char *graphName = RedisModule_StringPtrLen(graphID, NULL);
 			gc = _GraphContext_Create(ctx, graphName, GRAPH_DEFAULT_NODE_CAP, GRAPH_DEFAULT_EDGE_CAP);
+		} else {
+			// Key does not exist and won't be created, emit an error.
+			RedisModule_ReplyWithError(ctx, "ERR Invalid graph operation on empty key");
 		}
 	} else if(RedisModule_ModuleTypeGetType(key) == GraphContextRedisModuleType) {
 		gc = RedisModule_ModuleTypeGetValue(key);
+	} else {
+		// Key exists but is not a graph, emit an error.
+		RedisModule_ReplyWithError(ctx, REDISMODULE_ERRORMSG_WRONGTYPE);
 	}
+
 	RedisModule_CloseKey(key);
 
 	if(gc) _GraphContext_IncreaseRefCount(gc);
@@ -379,3 +386,4 @@ static void _GraphContext_Free(void *arg) {
 
 	rm_free(gc);
 }
+

tests/flow/test_rename.py → tests/flow/test_keyspace_accesses.py

@@ -1,5 +1,6 @@
 import os
 import sys
+import redis
 from RLTest import Env
 from redisgraph import Graph, Node, Edge
 
@@ -15,7 +16,7 @@
 GRAPH_ID = "G"
 NEW_GRAPH_ID = "G2"
 
-class testGraphRename(FlowTestsBase):
+class testKeyspaceAccesses(FlowTestsBase):
     def __init__(self):
         self.env = Env()
         global graph
@@ -40,3 +41,27 @@ def test00_test_data_valid_after_rename(self):
         expected_results = [[node0], [node1]]
         query_info = QueryInfo(query = query, description="Tests data is valid after renaming", expected_result = expected_results)
         self._assert_resultset_and_expected_mutually_included(graph.query(query), query_info)
+
+    # Graph queries should fail gracefully on accessing non-graph keys.
+    def test01_graph_access_on_invalid_key(self):
+        redis_con.set("integer_key", 5)
+        graph = Graph("integer_key", redis_con)
+        try:
+            query = """MATCH (n) RETURN noneExistingFunc(n.age) AS cast"""
+            graph.query(query)
+            assert(False)
+        except redis.exceptions.ResponseError as e:
+            # Expecting an error.
+            assert("WRONGTYPE" in e.message)
+            pass
+
+    # Fail gracefully on attempting a graph deletion of an empty key.
+    def test02_graph_delete_on_empty_key(self):
+        graph = Graph("nonexistent_key", redis_con)
+        try:
+            graph.delete()
+            assert(False)
+        except redis.exceptions.ResponseError as e:
+            # Expecting an error.
+            assert("empty key" in e.message)
+            pass