Fix RWTypeChecker and some small improvements (#247)

* Fix doc of mgp_graph_vertices

* Make write_proc example meaningful write procedure example

* Improve wrap_exceptions

* Add check for write procedures for ReadWriteTypeChecker

* Change error code in case of invalid default value for optional arguments

include/mg_procedure.h

@@ -757,6 +757,7 @@ struct mgp_vertices_iterator;
 /// Free the memory used by a mgp_vertices_iterator.
 void mgp_vertices_iterator_destroy(struct mgp_vertices_iterator *it);
 
+/// Start iterating over vertices of the given graph.
 /// Resulting mgp_vertices_iterator needs to be deallocated with mgp_vertices_iterator_destroy.
 /// Return MGP_ERROR_UNABLE_TO_ALLOCATE if unable to allocate a mgp_vertices_iterator.
 enum mgp_error mgp_graph_iter_vertices(struct mgp_graph *g, struct mgp_memory *memory,
@@ -902,7 +903,7 @@ typedef void (*mgp_proc_cb)(struct mgp_list *, struct mgp_graph *, struct mgp_re
 enum mgp_error mgp_module_add_read_procedure(struct mgp_module *module, const char *name, mgp_proc_cb cb,
                                              struct mgp_proc **result);
 
-/// Register a read-only procedure to a module.
+/// Register a writeable procedure to a module.
 ///
 /// The `name` must be a valid identifier, following the same rules as the
 /// procedure`name` in mgp_module_add_read_procedure.
@@ -949,7 +950,7 @@ enum mgp_error mgp_proc_add_arg(struct mgp_proc *proc, const char *name, struct
 ///
 /// Return MGP_ERROR_UNABLE_TO_ALLOCATE if unable to allocate memory for an argument.
 /// Return MGP_ERROR_INVALID_ARGUMENT if `name` is not a valid argument name.
-/// RETURN MGP_ERROR_OUT_OF_RANGE if `default_value` is a graph element (vertex, edge or path).
+/// RETURN MGP_ERROR_VALUE_CONVERSION if `default_value` is a graph element (vertex, edge or path).
 /// RETURN MGP_ERROR_LOGIC_ERROR if `default_value` does not satisfy `type`.
 enum mgp_error mgp_proc_add_opt_arg(struct mgp_proc *proc, const char *name, struct mgp_type *type,
                                     struct mgp_value *default_value);

include/mgp.py

@@ -1202,23 +1202,30 @@ def write_proc(func: typing.Callable[..., Record]):
 
     @mgp.write_proc
     def procedure(context: mgp.ProcCtx,
-                  required_arg: mgp.Nullable[mgp.Any],
-                  optional_arg: mgp.Nullable[mgp.Any] = None
-                  ) -> mgp.Record(result=str, args=list):
-        args = [required_arg, optional_arg]
-        # Multiple rows can be produced by returning an iterable of mgp.Record
-        return mgp.Record(args=args, result='Hello World!')
+                required_arg: str,
+                optional_arg: mgp.Nullable[str] = None
+                ) -> mgp.Record(result=mgp.Vertex):
+        vertex = context.graph.create_vertex()
+        vertex_properties = vertex.properties
+        vertex_properties["required_arg"] = required_arg
+        if optional_arg is not None:
+            vertex_properties["optional_arg"] = optional_arg
+
+        return mgp.Record(result=vertex)
     ```
 
-    The example procedure above returns 2 fields: `args` and `result`.
-      * `args` is a copy of arguments passed to the procedure.
-      * `result` is the result of this procedure, a "Hello World!" string.
+    The example procedure above returns  a newly created vertex which has
+    at most 2 properties:
+      * `required_arg` is always present and its value is the first
+        argument of the procedure.
+      * `optional_arg` is present if the second argument of the procedure
+        is not `null`.
     Any errors can be reported by raising an Exception.
 
     The procedure can be invoked in openCypher using the following calls:
-      CALL example.procedure(1, 2) YIELD args, result;
-      CALL example.procedure(1) YIELD args, result;
-    Naturally, you may pass in different arguments or yield less fields.
+      CALL example.procedure("property value", "another one") YIELD result;
+      CALL example.procedure("single argument") YIELD result;
+    Naturally, you may pass in different arguments.
     """
     return _register_proc(func, True)
 
@@ -1355,7 +1362,7 @@ def transformation(func: typing.Callable[..., Record]):
     return func
 
 
-def wrap_exceptions():
+def _wrap_exceptions():
     def wrap_function(func):
         @wraps(func)
         def wrapped_func(*args, **kwargs):
@@ -1408,9 +1415,8 @@ def wrap_exceptions():
             continue
         if inspect.isclass(obj):
             wrap_member_functions(obj)
-        if inspect.isfunction(obj) and obj != wrap_exceptions \
-                and not name.startswith("_"):
+        elif inspect.isfunction(obj) and not name.startswith("_"):
             setattr(module, name, wrap_function(obj))
 
 
-wrap_exceptions()
+_wrap_exceptions()

src/query/plan/read_write_type_checker.cpp

@@ -58,7 +58,15 @@ bool ReadWriteTypeChecker::PreVisit(Union &op) {
 }
 
 PRE_VISIT(Unwind, RWType::NONE, true)
-PRE_VISIT(CallProcedure, RWType::R, true)
+
+bool ReadWriteTypeChecker::PreVisit(CallProcedure &op) {
+  if (op.is_write_) {
+    UpdateType(RWType::RW);
+    return false;
+  }
+  UpdateType(RWType::R);
+  return true;
+}
 
 #undef PRE_VISIT
 

src/query/procedure/mg_procedure_impl.cpp

@@ -1834,8 +1834,8 @@ mgp_error mgp_proc_add_opt_arg(mgp_proc *proc, const char *name, mgp_type *type,
       case MGP_VALUE_TYPE_EDGE:
       case MGP_VALUE_TYPE_PATH:
         // default_value must not be a graph element.
-        throw std::out_of_range{fmt::format(
-            "Default value of argument '{}' of procedure '{}' name must not be a graph element!", name, proc->name)};
+        throw ValueConversionException{
+            "Default value of argument '{}' of procedure '{}' name must not be a graph element!", name, proc->name};
       case MGP_VALUE_TYPE_NULL:
       case MGP_VALUE_TYPE_BOOL:
       case MGP_VALUE_TYPE_INT:

tests/unit/query_plan_read_write_typecheck.cpp

@@ -179,17 +179,45 @@ TEST_F(ReadWriteTypeCheckTest, Union) {
   CheckPlanType(union_op.get(), RWType::R);
 }
 
-TEST_F(ReadWriteTypeCheckTest, CallProcedure) {
+TEST_F(ReadWriteTypeCheckTest, CallReadProcedure) {
   plan::CallProcedure call_op;
   call_op.input_ = std::make_shared<Once>();
   call_op.procedure_name_ = "mg.reload";
   call_op.arguments_ = {LITERAL("example")};
   call_op.result_fields_ = {"name", "signature"};
+  call_op.is_write_ = false;
   call_op.result_symbols_ = {GetSymbol("name_alias"), GetSymbol("signature_alias")};
 
   CheckPlanType(&call_op, RWType::R);
 }
 
+TEST_F(ReadWriteTypeCheckTest, CallWriteProcedure) {
+  plan::CallProcedure call_op;
+  call_op.input_ = std::make_shared<Once>();
+  call_op.procedure_name_ = "mg.reload";
+  call_op.arguments_ = {LITERAL("example")};
+  call_op.result_fields_ = {"name", "signature"};
+  call_op.is_write_ = true;
+  call_op.result_symbols_ = {GetSymbol("name_alias"), GetSymbol("signature_alias")};
+
+  CheckPlanType(&call_op, RWType::RW);
+}
+
+TEST_F(ReadWriteTypeCheckTest, CallReadProcedureBeforeUpdate) {
+  std::shared_ptr<LogicalOperator> last_op = std::make_shared<Once>();
+  last_op = std::make_shared<CreateNode>(last_op, NodeCreationInfo());
+
+  std::string procedure_name{"mg.reload"};
+  std::vector<Expression *> arguments{LITERAL("example")};
+  std::vector<std::string> result_fields{"name", "signature"};
+  std::vector<Symbol> result_symbols{GetSymbol("name_alias"), GetSymbol("signature_alias")};
+
+  last_op = std::make_shared<plan::CallProcedure>(last_op, procedure_name, arguments, result_fields, result_symbols,
+                                                  nullptr, 0, false);
+
+  CheckPlanType(last_op.get(), RWType::RW);
+}
+
 TEST_F(ReadWriteTypeCheckTest, ConstructNamedPath) {
   auto node1_sym = GetSymbol("node1");
   auto edge1_sym = GetSymbol("edge1");