Throw RecordDeletedError if updating a deleted record

Summary:
Previously, we would have a `DCHECK` which crashes the application. This
was evident when testing a queries, such as:

    MATCH (n) DELETE n SET n.prop = 42

Since the argument to update clauses is evaluated during execution, it
makes it very difficult to prevent such errors during semantic analysis.
For example:

    MATCH (n)--(m) WITH collect(n) as ns, m
    DETACH DELETE ns[m.prop] SET head(ns).prop = 42

Test query updates on deleted graph elements

Reviewers: florijan, dgleich

Reviewed By: florijan

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1114

src/query/plan/operator.cpp

@@ -37,6 +37,9 @@ void PropsSetChecked(TRecordAccessor &record, storage::Property key,
   } catch (const TypedValueException &) {
     throw QueryRuntimeException("'{}' cannot be used as a property value.",
                                 value.type());
+  } catch (const RecordDeletedError &) {
+    throw QueryRuntimeException(
+        "Trying to set properties on a deleted graph element.");
   }
 }
 
@@ -1355,10 +1358,22 @@ template <typename TRecordAccessor>
 void SetProperties::SetPropertiesCursor::Set(TRecordAccessor &record,
                                              const TypedValue &rhs) const {
   record.SwitchNew();
-  if (self_.op_ == Op::REPLACE) record.PropsClear();
+  if (self_.op_ == Op::REPLACE) {
+    try {
+      record.PropsClear();
+    } catch (const RecordDeletedError &) {
+      throw QueryRuntimeException(
+          "Trying to set properties on a deleted graph element.");
+    }
+  }
 
   auto set_props = [&record](const auto &properties) {
+    try {
       for (const auto &kv : properties) record.PropsSet(kv.first, kv.second);
+    } catch (const RecordDeletedError &) {
+      throw QueryRuntimeException(
+          "Trying to set properties on a deleted graph element.");
+    }
   };
 
   switch (rhs.type()) {
@@ -1411,7 +1426,11 @@ bool SetLabels::SetLabelsCursor::Pull(Frame &frame, Context &context) {
   ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex);
   auto &vertex = vertex_value.Value<VertexAccessor>();
   vertex.SwitchNew();
+  try {
     for (auto label : self_.labels_) vertex.add_label(label);
+  } catch (const RecordDeletedError &) {
+    throw QueryRuntimeException("Trying to set labels on a deleted Vertex");
+  }
 
   return true;
 }
@@ -1444,10 +1463,20 @@ bool RemoveProperty::RemovePropertyCursor::Pull(Frame &frame,
 
   switch (lhs.type()) {
     case TypedValue::Type::Vertex:
+      try {
         lhs.Value<VertexAccessor>().PropsErase(self_.lhs_->property_);
+      } catch (const RecordDeletedError &) {
+        throw QueryRuntimeException(
+            "Trying to remove properties from a deleted Vertex");
+      }
       break;
     case TypedValue::Type::Edge:
+      try {
         lhs.Value<EdgeAccessor>().PropsErase(self_.lhs_->property_);
+      } catch (const RecordDeletedError &) {
+        throw QueryRuntimeException(
+            "Trying to remove properties from a deleted Edge");
+      }
       break;
     case TypedValue::Type::Null:
       // Skip removing properties on Null (can occur in optional match).
@@ -1486,7 +1515,12 @@ bool RemoveLabels::RemoveLabelsCursor::Pull(Frame &frame, Context &context) {
   ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex);
   auto &vertex = vertex_value.Value<VertexAccessor>();
   vertex.SwitchNew();
+  try {
     for (auto label : self_.labels_) vertex.remove_label(label);
+  } catch (const RecordDeletedError &) {
+    throw QueryRuntimeException(
+        "Trying to remove labels from a deleted Vertex");
+  }
 
   return true;
 }

src/storage/record_accessor.cpp

@@ -162,13 +162,14 @@ TRecord &RecordAccessor<TRecord>::update() const {
     DCHECK(reconstructed) << "Unable to initialize record";
   }
 
-  auto &t = db_accessor_->transaction();
+  const auto &t = db_accessor_->transaction();
-  if (!new_) {
+  {
-    DCHECK(!old_->is_expired_by(t))
+    const std::string err =
-        << "Can't update a record deleted in the current transaction+commad";
+        "Can't update a record deleted in the current transaction+commad";
-  } else {
+    if (!new_ && old_->is_expired_by(t))
-    DCHECK(!new_->is_expired_by(t))
+      throw RecordDeletedError(err);
-        << "Can't update a record deleted in the current transaction+command";
+    else if (new_ && new_->is_expired_by(t))
+      throw RecordDeletedError(err);
   }
 
   if (new_) return *new_;

src/storage/record_accessor.hpp

@@ -156,6 +156,8 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    *
    * It is not legal to call this function on a Vertex/Edge that has been
    * deleted in the current transaction+command.
+   *
+   * @throws RecordDeletedError
    */
   TRecord &update() const;
 
@@ -207,3 +209,8 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    */
   mutable TRecord *new_{nullptr};
 };
+
+/** Error when trying to update a deleted record */
+class RecordDeletedError : public utils::BasicException {
+  using utils::BasicException::BasicException;
+};

tests/unit/query_plan_create_set_remove_delete.cpp

@@ -949,3 +949,130 @@ TEST(QueryPlan, CreateIndex) {
   EXPECT_EQ(PullAll(create_index, dba, symbol_table), 1);
   EXPECT_TRUE(dba.LabelPropertyIndexExists(label, property));
 }
+
+TEST(QueryPlan, DeleteSetProperty) {
+  database::SingleNode db;
+  database::GraphDbAccessor dba(db);
+  // Add a single vertex.
+  dba.InsertVertex();
+  dba.AdvanceCommand();
+  EXPECT_EQ(1, CountIterable(dba.Vertices(false)));
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  // MATCH (n) DELETE n SET n.property = 42
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  auto prop = PROPERTY_PAIR("property");
+  auto n_prop = PROPERTY_LOOKUP("n", prop);
+  symbol_table[*n_prop->expression_] = n.sym_;
+  auto set_op =
+      std::make_shared<plan::SetProperty>(delete_op, n_prop, LITERAL(42));
+  EXPECT_THROW(PullAll(set_op, dba, symbol_table), QueryRuntimeException);
+}
+
+TEST(QueryPlan, DeleteSetPropertiesFromMap) {
+  database::SingleNode db;
+  database::GraphDbAccessor dba(db);
+  // Add a single vertex.
+  dba.InsertVertex();
+  dba.AdvanceCommand();
+  EXPECT_EQ(1, CountIterable(dba.Vertices(false)));
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  // MATCH (n) DELETE n SET n = {property: 42}
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  auto prop = PROPERTY_PAIR("property");
+  auto n_prop = PROPERTY_LOOKUP("n", prop);
+  symbol_table[*n_prop->expression_] = n.sym_;
+  std::unordered_map<std::pair<std::string, storage::Property>, Expression *>
+      prop_map;
+  prop_map.emplace(prop, LITERAL(42));
+  auto *rhs = storage.Create<MapLiteral>(prop_map);
+  symbol_table[*rhs] = n.sym_;
+  for (auto op_type :
+       {plan::SetProperties::Op::REPLACE, plan::SetProperties::Op::UPDATE}) {
+    auto set_op =
+        std::make_shared<plan::SetProperties>(delete_op, n.sym_, rhs, op_type);
+    EXPECT_THROW(PullAll(set_op, dba, symbol_table), QueryRuntimeException);
+  }
+}
+
+TEST(QueryPlan, DeleteSetPropertiesFromVertex) {
+  database::SingleNode db;
+  database::GraphDbAccessor dba(db);
+  // Add a single vertex.
+  {
+    auto v = dba.InsertVertex();
+    v.PropsSet(dba.Property("property"), 1);
+  }
+  dba.AdvanceCommand();
+  EXPECT_EQ(1, CountIterable(dba.Vertices(false)));
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  // MATCH (n) DELETE n SET n = n
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  auto prop = PROPERTY_PAIR("property");
+  auto n_prop = PROPERTY_LOOKUP("n", prop);
+  symbol_table[*n_prop->expression_] = n.sym_;
+  auto *rhs = IDENT("n");
+  symbol_table[*rhs] = n.sym_;
+  for (auto op_type :
+       {plan::SetProperties::Op::REPLACE, plan::SetProperties::Op::UPDATE}) {
+    auto set_op =
+        std::make_shared<plan::SetProperties>(delete_op, n.sym_, rhs, op_type);
+    EXPECT_THROW(PullAll(set_op, dba, symbol_table), QueryRuntimeException);
+  }
+}
+
+TEST(QueryPlan, DeleteRemoveLabels) {
+  database::SingleNode db;
+  database::GraphDbAccessor dba(db);
+  // Add a single vertex.
+  dba.InsertVertex();
+  dba.AdvanceCommand();
+  EXPECT_EQ(1, CountIterable(dba.Vertices(false)));
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  // MATCH (n) DELETE n REMOVE n :label
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  std::vector<storage::Label> labels{dba.Label("label")};
+  auto rem_op = std::make_shared<plan::RemoveLabels>(delete_op, n.sym_, labels);
+  EXPECT_THROW(PullAll(rem_op, dba, symbol_table), QueryRuntimeException);
+}
+
+TEST(QueryPlan, DeleteRemoveProperty) {
+  database::SingleNode db;
+  database::GraphDbAccessor dba(db);
+  // Add a single vertex.
+  dba.InsertVertex();
+  dba.AdvanceCommand();
+  EXPECT_EQ(1, CountIterable(dba.Vertices(false)));
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  // MATCH (n) DELETE n REMOVE n.property
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  auto prop = PROPERTY_PAIR("property");
+  auto n_prop = PROPERTY_LOOKUP("n", prop);
+  symbol_table[*n_prop->expression_] = n.sym_;
+  auto rem_op = std::make_shared<plan::RemoveProperty>(delete_op, n_prop);
+  EXPECT_THROW(PullAll(rem_op, dba, symbol_table), QueryRuntimeException);
+}