diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 947f714c4..a091e0181 100644 --- a/src/query/plan/operator.cpp +++ b/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 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) { - for (const auto &kv : properties) record.PropsSet(kv.first, kv.second); + 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(); vertex.SwitchNew(); - for (auto label : self_.labels_) vertex.add_label(label); + 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: - lhs.Value().PropsErase(self_.lhs_->property_); + try { + lhs.Value().PropsErase(self_.lhs_->property_); + } catch (const RecordDeletedError &) { + throw QueryRuntimeException( + "Trying to remove properties from a deleted Vertex"); + } break; case TypedValue::Type::Edge: - lhs.Value().PropsErase(self_.lhs_->property_); + try { + lhs.Value().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(); vertex.SwitchNew(); - for (auto label : self_.labels_) vertex.remove_label(label); + 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; } diff --git a/src/storage/record_accessor.cpp b/src/storage/record_accessor.cpp index 0db3e4f66..0a0096e8b 100644 --- a/src/storage/record_accessor.cpp +++ b/src/storage/record_accessor.cpp @@ -162,13 +162,14 @@ TRecord &RecordAccessor::update() const { DCHECK(reconstructed) << "Unable to initialize record"; } - auto &t = db_accessor_->transaction(); - if (!new_) { - DCHECK(!old_->is_expired_by(t)) - << "Can't update a record deleted in the current transaction+commad"; - } else { - DCHECK(!new_->is_expired_by(t)) - << "Can't update a record deleted in the current transaction+command"; + const auto &t = db_accessor_->transaction(); + { + const std::string err = + "Can't update a record deleted in the current transaction+commad"; + if (!new_ && old_->is_expired_by(t)) + throw RecordDeletedError(err); + else if (new_ && new_->is_expired_by(t)) + throw RecordDeletedError(err); } if (new_) return *new_; diff --git a/src/storage/record_accessor.hpp b/src/storage/record_accessor.hpp index 05d957068..07281eb0c 100644 --- a/src/storage/record_accessor.hpp +++ b/src/storage/record_accessor.hpp @@ -156,6 +156,8 @@ class RecordAccessor : public TotalOrdering> { * * 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> { */ mutable TRecord *new_{nullptr}; }; + +/** Error when trying to update a deleted record */ +class RecordDeletedError : public utils::BasicException { + using utils::BasicException::BasicException; +}; diff --git a/tests/unit/query_plan_create_set_remove_delete.cpp b/tests/unit/query_plan_create_set_remove_delete.cpp index 79ea2e159..ff1817759 100644 --- a/tests/unit/query_plan_create_set_remove_delete.cpp +++ b/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("n"); + symbol_table[*n_get] = n.sym_; + auto delete_op = std::make_shared( + n.op_, std::vector{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(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("n"); + symbol_table[*n_get] = n.sym_; + auto delete_op = std::make_shared( + n.op_, std::vector{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, Expression *> + prop_map; + prop_map.emplace(prop, LITERAL(42)); + auto *rhs = storage.Create(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(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("n"); + symbol_table[*n_get] = n.sym_; + auto delete_op = std::make_shared( + n.op_, std::vector{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(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("n"); + symbol_table[*n_get] = n.sym_; + auto delete_op = std::make_shared( + n.op_, std::vector{n_get}, false); + std::vector labels{dba.Label("label")}; + auto rem_op = std::make_shared(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("n"); + symbol_table[*n_get] = n.sym_; + auto delete_op = std::make_shared( + n.op_, std::vector{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(delete_op, n_prop); + EXPECT_THROW(PullAll(rem_op, dba, symbol_table), QueryRuntimeException); +}