Disallow property assignment to complex data types (#1432)

* Disallow property assignment to complex data types

* Address PR comments

* Capture errors before committing updates

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

src/errors.c

@@ -42,3 +42,7 @@ void Error_UnsupportedASTOperator(const cypher_operator_t *op) {
 	QueryCtx_SetError("RedisGraph does not currently support %s", op->str);
 }
 
+inline void Error_InvalidPropertyValue(void) {
+	QueryCtx_SetError("Property values can only be of primitive types or arrays thereof");
+}
+

src/errors.h

@@ -23,3 +23,5 @@ void Error_UnsupportedASTNodeType(const cypher_astnode_t *node);
 // Report an error on receiving an unhandled AST operator.
 void Error_UnsupportedASTOperator(const cypher_operator_t *op);
 
+// Report an error on trying to assign a complex type to a property.
+void Error_InvalidPropertyValue(void);

src/execution_plan/ops/op_update.c

@@ -5,12 +5,12 @@
 */
 
 #include "op_update.h"
+#include "../../errors.h"
 #include "../../query_ctx.h"
 #include "../../util/arr.h"
 #include "../../util/qsort.h"
 #include "../../util/rmalloc.h"
 #include "../../arithmetic/arithmetic_expression.h"
-#include "../../query_ctx.h"
 
 /* Forward declarations. */
 static OpResult UpdateInit(OpBase *opBase);
@@ -260,6 +260,12 @@ static void _EvalEntityUpdates(EntityUpdateCtx *ctx, GraphContext *gc,
 		EntityUpdateEvalCtx *update_ctx = ctx->exps + i;
 		SIValue new_value = AR_EXP_Evaluate(update_ctx->exp, r);
 
+		// Emit an error and return NULL if we're trying to add an invalid type.
+		if(!(SI_TYPE(new_value) & SI_VALID_PROPERTY_VALUE)) {
+			Error_InvalidPropertyValue();
+			break;
+		}
+
 		/* Determine whether we must update the index for this set of updates.
 		 * If at least one property being updated is indexed, each node will be reindexed. */
 		if(!update_index && label) {

src/execution_plan/ops/shared/create_functions.c

@@ -5,6 +5,7 @@
  */
 
 #include "create_functions.h"
+#include "../../../errors.h"
 #include "../../../query_ctx.h"
 
 // Add properties to the GraphEntity.
@@ -162,6 +163,11 @@ PendingProperties *ConvertPropertyMap(Record r, const PropertyMap *map) {
 	converted->values = rm_malloc(sizeof(SIValue) * map->property_count);
 	for(int i = 0; i < map->property_count; i++) {
 		converted->values[i] = AR_EXP_Evaluate(map->values[i], r);
+		// Emit an error and return NULL if we're trying to add an invalid type.
+		if(!(SI_TYPE(converted->values[i]) & SI_VALID_PROPERTY_VALUE)) {
+			Error_InvalidPropertyValue();
+			break;
+		}
 	}
 
 	return converted;

src/graph/entities/graph_entity.c

@@ -4,19 +4,31 @@
 * This file is available under the Redis Labs Source Available License Agreement
 */
 
-#include <stdio.h>
-#include <assert.h>
 #include "graph_entity.h"
-#include "../../query_ctx.h"
-#include "../../util/rmalloc.h"
-#include "../graphcontext.h"
 #include "node.h"
 #include "edge.h"
+#include "../../RG.h"
+#include "../../errors.h"
+#include "../../query_ctx.h"
+#include "../graphcontext.h"
+#include "../../util/rmalloc.h"
+#include <stdio.h>
+#include <assert.h>
 
 SIValue *PROPERTY_NOTFOUND = &(SIValue) {
 	.longval = 0, .type = T_NULL
 };
 
+/* Only primitives or arrays of primitives can be assigned as a property value.
+ * Return true if the value is valid, emit an error and return false otherwise. */
+static inline bool _GraphEntity_ValidatePropertyValue(SIValue v) {
+	if(!(SI_TYPE(v) & SI_VALID_PROPERTY_VALUE)) {
+		Error_InvalidPropertyValue();
+		return false;
+	}
+	return true;
+}
+
 /* Removes entity's property. */
 static void _GraphEntity_RemoveProperty(const GraphEntity *e, Attribute_ID attr_id) {
 	// Quick return if attribute is missing.
@@ -48,6 +60,11 @@ static void _GraphEntity_RemoveProperty(const GraphEntity *e, Attribute_ID attr_
 
 /* Add a new property to entity */
 SIValue *GraphEntity_AddProperty(GraphEntity *e, Attribute_ID attr_id, SIValue value) {
+	ASSERT(e);
+
+	// Emit an error and return NULL if we're trying to add an invalid type.
+	if(!_GraphEntity_ValidatePropertyValue(value)) return NULL;
+
 	if(e->entity->properties == NULL) {
 		e->entity->properties = rm_malloc(sizeof(EntityProperty));
 	} else {
@@ -78,7 +95,10 @@ SIValue *GraphEntity_GetProperty(const GraphEntity *e, Attribute_ID attr_id) {
 
 // Updates existing property value.
 void GraphEntity_SetProperty(const GraphEntity *e, Attribute_ID attr_id, SIValue value) {
-	assert(e);
+	ASSERT(e);
+
+	// Emit an error and return if we're trying to add an invalid type.
+	if(!_GraphEntity_ValidatePropertyValue(value)) return;
 
 	// Setting an attribute value to NULL removes that attribute.
 	if(SIValue_IsNull(value)) {

src/value.h

@@ -50,6 +50,8 @@ typedef enum {
 #define SI_NUMERIC (T_INT64 | T_DOUBLE)
 #define SI_GRAPHENTITY (T_NODE | T_EDGE)
 #define SI_ALL (T_MAP | T_NODE | T_EDGE | T_ARRAY | T_PATH | T_DATETIME | T_LOCALDATETIME | T_DATE | T_TIME | T_LOCALTIME | T_DURATION | T_STRING | T_BOOL | T_INT64 | T_DOUBLE | T_NULL | T_PTR)
+#define SI_VALID_PROPERTY_VALUE (T_ARRAY | T_DATETIME | T_LOCALDATETIME | T_DATE | T_TIME | T_LOCALTIME | T_DURATION | T_STRING | T_BOOL | T_INT64 | T_DOUBLE | T_NULL)
+
 
 /* Any values (except durations) are comparable with other values of the same type.
  * Integer and floating-point values are also comparable with each other. */

tests/flow/test_query_validation.py

@@ -447,3 +447,15 @@ def test29_invalid_filter_non_boolean_constant(self):
         # Non-existent properties are treated as NULLs, which are boolean in Cypher's 3-valued logic.
         query = """MATCH (a) WHERE a.fakeprop RETURN a"""
         redis_graph.query(query)
+
+    def test31_set_invalid_property_type(self):
+        queries = ["""MATCH (a) CREATE (:L {v: a})""",
+                   """MATCH (a), (b) WHERE b.age IS NOT NULL SET b.age = a"""]
+        for q in queries:
+            try:
+                redis_graph.query(q)
+                assert(False)
+            except redis.exceptions.ResponseError as e:
+                # Expecting an error.
+                assert("Property values can only be of primitive types" in e.message)
+                pass