diff --git a/src/database/graph_db_accessor.hpp b/src/database/graph_db_accessor.hpp index 1eac2e457..858d496c9 100644 --- a/src/database/graph_db_accessor.hpp +++ b/src/database/graph_db_accessor.hpp @@ -251,12 +251,17 @@ class GraphDbAccessor { * The old_ and new_ pointers need to be initialized * with appropriate values, and current_ set to old_ * if it exists and to new_ otherwise. + * + * @return True if accessor is valid after reconstruction. + * This means that at least one record pointer was found + * (either new_ or old_), possibly both. */ template - void Reconstruct(RecordAccessor &accessor) { + bool Reconstruct(RecordAccessor &accessor) { accessor.vlist_->find_set_new_old(*transaction_, accessor.old_, accessor.new_); accessor.current_ = accessor.old_ ? accessor.old_ : accessor.new_; + return accessor.old_ != nullptr || accessor.new_ != nullptr; // TODO review: we should never use a record accessor that // does not have either old_ or new_ (both are null), but // we can't assert that here because we construct such an accessor diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 8cdc5f3e7..62399738e 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -899,10 +899,16 @@ namespace { void ReconstructTypedValue(TypedValue &value) { switch (value.type()) { case TypedValue::Type::Vertex: - value.Value().Reconstruct(); + if (!value.Value().Reconstruct()) + throw QueryRuntimeException( + "Vertex invalid after WITH clause, (most likely deleted by a " + "preceeding DELETE clause)"); break; case TypedValue::Type::Edge: - value.Value().Reconstruct(); + if (!value.Value().Reconstruct()) + throw QueryRuntimeException( + "Edge invalid after WITH clause, (most likely deleted by a " + "preceeding DELETE clause)"); break; case TypedValue::Type::List: for (TypedValue &inner_value : value.Value>()) diff --git a/src/storage/record_accessor.cpp b/src/storage/record_accessor.cpp index 37e7a322b..791edefa2 100644 --- a/src/storage/record_accessor.cpp +++ b/src/storage/record_accessor.cpp @@ -1,6 +1,6 @@ +#include "storage/record_accessor.hpp" #include "database/graph_db_accessor.hpp" #include "storage/edge.hpp" -#include "storage/record_accessor.hpp" #include "storage/vertex.hpp" #include "utils/assert.hpp" @@ -59,13 +59,11 @@ RecordAccessor &RecordAccessor::SwitchNew() { // we can just Reconstruct the pointers, old_ will get initialized // to the same value as it has now, and the amount of work is the // same as just looking for a new_ record - Reconstruct(); + if (!Reconstruct()) + debug_fail( + "RecordAccessor::SwitchNew - accessor invalid after Reconstruct"); } - // set new if we have it - // if we don't then current_ is old_ and remains there - if (new_) current_ = new_; - debug_assert(current_ != nullptr, - "RecordAccessor::SwitchNew - current_ is nullptr"); + current_ = new_ ? new_ : old_; return *this; } @@ -81,8 +79,8 @@ RecordAccessor &RecordAccessor::SwitchOld() { } template -void RecordAccessor::Reconstruct() { - db_accessor().Reconstruct(*this); +bool RecordAccessor::Reconstruct() { + return db_accessor().Reconstruct(*this); } template diff --git a/src/storage/record_accessor.hpp b/src/storage/record_accessor.hpp index 7c2bfe4a5..9b02b478d 100644 --- a/src/storage/record_accessor.hpp +++ b/src/storage/record_accessor.hpp @@ -159,9 +159,11 @@ class RecordAccessor { * accessor so it uses the versions appropriate * to this transaction+command. * - * TODO consider what it does after delete+advance_command + * @return True if this accessor is valid after reconstruction. + * This means that at least one record pointer was found + * (either new_ or old_), possibly both. */ - void Reconstruct(); + bool Reconstruct(); protected: /** diff --git a/src/utils/assert.hpp b/src/utils/assert.hpp index 5bbb500b9..8d3b35891 100644 --- a/src/utils/assert.hpp +++ b/src/utils/assert.hpp @@ -56,6 +56,8 @@ */ #ifdef DEBUG_ASSERT_ON #define debug_assert(condition, message) permanent_assert(condition, message) +#define debug_fail(message) permanent_fail(message) #else -#define debug_assert(condition, message) +#define debug_assert(condition, message) {} +#define debug_fail(message) {} #endif diff --git a/tests/unit/query_plan_create_set_remove_delete.cpp b/tests/unit/query_plan_create_set_remove_delete.cpp index 5580e1a81..0b5289200 100644 --- a/tests/unit/query_plan_create_set_remove_delete.cpp +++ b/tests/unit/query_plan_create_set_remove_delete.cpp @@ -412,6 +412,34 @@ TEST(QueryPlan, DeleteReturn) { EXPECT_EQ(0, CountIterable(dba->vertices())); } +TEST(QueryPlan, DeleteAdvance) { + // test queries on empty DB: + // CREATE (n) + // MATCH (n) DELETE n WITH n ... + // this fails due to us advancing the command + // when processing the WITH clause + // + // note that Neo does not fail when the deleted + // record is not used in subsequent clauses, but + // we are not yet compatible with that + Dbms dbms; + auto dba = dbms.active(); + dba->insert_vertex(); + dba->advance_command(); + + AstTreeStorage storage; + SymbolTable symbol_table; + + 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 advance = std::make_shared( + delete_op, std::vector{n.sym_}, true); + EXPECT_THROW(PullAll(advance, *dba, symbol_table), QueryRuntimeException); +} + TEST(QueryPlan, SetProperty) { Dbms dbms; auto dba = dbms.active();