From c94201621abdb996ba5b5c22533d0f180dd3669b Mon Sep 17 00:00:00 2001 From: Andi Date: Thu, 2 Nov 2023 14:07:48 +0100 Subject: [PATCH] Support deleting paths (#1383) --- src/query/plan/operator.cpp | 78 +++++++++++++------ src/storage/v2/edge_accessor.hpp | 3 + src/storage/v2/vertex_accessor.hpp | 3 + .../tests/memgraph_V1/features/delete.feature | 21 +++++ .../features/delete.feature | 22 ++++++ .../features/DeleteAcceptance.feature | 20 ----- 6 files changed, 102 insertions(+), 45 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 5b8f0981e..68d894d34 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -2588,39 +2588,68 @@ void Delete::DeleteCursor::UpdateDeleteBuffer(Frame &frame, ExecutionContext &co expression_results.emplace_back(expression->Accept(evaluator)); } + auto vertex_auth_checker = [&context](const VertexAccessor &va) -> bool { +#ifdef MG_ENTERPRISE + return !(license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker && + !context.auth_checker->Has(va, storage::View::NEW, query::AuthQuery::FineGrainedPrivilege::CREATE_DELETE)); +#else + return true; +#endif + }; + + auto edge_auth_checker = [&context](const EdgeAccessor &ea) -> bool { +#ifdef MG_ENTERPRISE + return !( + license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker && + !(context.auth_checker->Has(ea, query::AuthQuery::FineGrainedPrivilege::CREATE_DELETE) && + context.auth_checker->Has(ea.To(), storage::View::NEW, query::AuthQuery::FineGrainedPrivilege::UPDATE) && + context.auth_checker->Has(ea.From(), storage::View::NEW, query::AuthQuery::FineGrainedPrivilege::UPDATE))); +#else + return true; +#endif + }; + for (TypedValue &expression_result : expression_results) { AbortCheck(context); switch (expression_result.type()) { case TypedValue::Type::Vertex: { auto va = expression_result.ValueVertex(); -#ifdef MG_ENTERPRISE - if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker && - !context.auth_checker->Has(va, storage::View::NEW, query::AuthQuery::FineGrainedPrivilege::CREATE_DELETE)) { + if (vertex_auth_checker(va)) { + buffer_.nodes.push_back(va); + } else { throw QueryRuntimeException("Vertex not deleted due to not having enough permission!"); } -#endif - buffer_.nodes.push_back(va); break; } case TypedValue::Type::Edge: { auto ea = expression_result.ValueEdge(); -#ifdef MG_ENTERPRISE - if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker && - !(context.auth_checker->Has(ea, query::AuthQuery::FineGrainedPrivilege::CREATE_DELETE) && - context.auth_checker->Has(ea.To(), storage::View::NEW, query::AuthQuery::FineGrainedPrivilege::UPDATE) && - context.auth_checker->Has(ea.From(), storage::View::NEW, - query::AuthQuery::FineGrainedPrivilege::UPDATE))) { + if (edge_auth_checker(ea)) { + buffer_.edges.push_back(ea); + } else { throw QueryRuntimeException("Edge not deleted due to not having enough permission!"); } -#endif - buffer_.edges.push_back(ea); break; } + case TypedValue::Type::Path: { + auto path = expression_result.ValuePath(); +#ifdef MG_ENTERPRISE + auto edges_res = std::any_of(path.edges().cbegin(), path.edges().cend(), + [&edge_auth_checker](const auto &ea) { return !edge_auth_checker(ea); }); + auto vertices_res = std::any_of(path.vertices().cbegin(), path.vertices().cend(), + [&vertex_auth_checker](const auto &va) { return !vertex_auth_checker(va); }); + + if (edges_res || vertices_res) { + throw QueryRuntimeException( + "Path not deleted due to not having enough permission on all edges and vertices on the path!"); + } +#endif + buffer_.nodes.insert(buffer_.nodes.begin(), path.vertices().begin(), path.vertices().end()); + buffer_.edges.insert(buffer_.edges.begin(), path.edges().begin(), path.edges().end()); + } case TypedValue::Type::Null: break; - // check we're not trying to delete anything except vertices and edges default: - throw QueryRuntimeException("Only edges and vertices can be deleted."); + throw QueryRuntimeException("Edges, vertices and paths can be deleted."); } } } @@ -2789,15 +2818,13 @@ SetProperties::SetPropertiesCursor::SetPropertiesCursor(const SetProperties &sel namespace { template -concept AccessorWithProperties = - requires(T value, storage::PropertyId property_id, storage::PropertyValue property_value, - std::map properties) { - { - value.ClearProperties() - } -> std::same_as>>; - { value.SetProperty(property_id, property_value) }; - { value.UpdateProperties(properties) }; - }; +concept AccessorWithProperties = requires(T value, storage::PropertyId property_id, + storage::PropertyValue property_value, + std::map properties) { + { value.ClearProperties() } -> std::same_as>>; + {value.SetProperty(property_id, property_value)}; + {value.UpdateProperties(properties)}; +}; /// Helper function that sets the given values on either a Vertex or an Edge. /// @@ -5354,7 +5381,8 @@ class HashJoinCursor : public Cursor { restore_frame(self_.left_symbols_, *left_op_frame_it_); left_op_frame_it_++; - // When all left frames with the common value have been joined, move on to pulling and joining the next right frame + // When all left frames with the common value have been joined, move on to pulling and joining the next right + // frame if (common_value_found_ && left_op_frame_it_ == hashtable_[common_value].end()) { common_value_found_ = false; } diff --git a/src/storage/v2/edge_accessor.hpp b/src/storage/v2/edge_accessor.hpp index 5d5dca391..86912c2b6 100644 --- a/src/storage/v2/edge_accessor.hpp +++ b/src/storage/v2/edge_accessor.hpp @@ -110,6 +110,9 @@ class EdgeAccessor final { } // namespace memgraph::storage +static_assert(std::is_trivially_copyable::value, + "storage::EdgeAccessor must be trivially copyable!"); + namespace std { template <> struct hash { diff --git a/src/storage/v2/vertex_accessor.hpp b/src/storage/v2/vertex_accessor.hpp index 77f5cd007..8f67bc30b 100644 --- a/src/storage/v2/vertex_accessor.hpp +++ b/src/storage/v2/vertex_accessor.hpp @@ -127,6 +127,9 @@ class VertexAccessor final { bool for_deleted_{false}; }; +static_assert(std::is_trivially_copyable::value, + "storage::VertexAccessor must be trivially copyable!"); + struct EdgesVertexAccessorResult { std::vector edges; int64_t expanded_count; diff --git a/tests/gql_behave/tests/memgraph_V1/features/delete.feature b/tests/gql_behave/tests/memgraph_V1/features/delete.feature index a9f59aca4..664d85983 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/delete.feature +++ b/tests/gql_behave/tests/memgraph_V1/features/delete.feature @@ -219,3 +219,24 @@ Feature: Delete Then the result should be: | n1 | | () | + + + Scenario: Detach deleting paths + Given an empty graph + And having executed: + """ + CREATE (x:X), (n1), (n2), (n3) + CREATE (x)-[:R]->(n1) + CREATE (n1)-[:R]->(n2) + CREATE (n2)-[:R]->(n3) + """ + When executing query: + """ + MATCH p = (:X)-->()-->()-->() + DETACH DELETE p + """ + Then the result should be empty + And the side effects should be: + | -nodes | 4 | + | -relationships | 3 | + | -labels | 1 | diff --git a/tests/gql_behave/tests/memgraph_V1_on_disk/features/delete.feature b/tests/gql_behave/tests/memgraph_V1_on_disk/features/delete.feature index 027e40b94..768422966 100644 --- a/tests/gql_behave/tests/memgraph_V1_on_disk/features/delete.feature +++ b/tests/gql_behave/tests/memgraph_V1_on_disk/features/delete.feature @@ -209,3 +209,25 @@ Feature: Delete MATCH (n) DETACH DELETE n SET n.prop = 1 WITH n RETURN n """ Then an error should be raised + + + + Scenario: Detach deleting paths + Given an empty graph + And having executed: + """ + CREATE (x:X), (n1), (n2), (n3) + CREATE (x)-[:R]->(n1) + CREATE (n1)-[:R]->(n2) + CREATE (n2)-[:R]->(n3) + """ + When executing query: + """ + MATCH p = (:X)-->()-->()-->() + DETACH DELETE p + """ + Then the result should be empty + And the side effects should be: + | -nodes | 4 | + | -relationships | 3 | + | -labels | 1 | diff --git a/tests/gql_behave/tests/openCypher_M09/features/DeleteAcceptance.feature b/tests/gql_behave/tests/openCypher_M09/features/DeleteAcceptance.feature index b07f2ff49..f178d566b 100644 --- a/tests/gql_behave/tests/openCypher_M09/features/DeleteAcceptance.feature +++ b/tests/gql_behave/tests/openCypher_M09/features/DeleteAcceptance.feature @@ -99,26 +99,6 @@ Feature: DeleteAcceptance | -relationships | 3 | | -labels | 1 | - Scenario: Detach deleting paths - Given an empty graph - And having executed: - """ - CREATE (x:X), (n1), (n2), (n3) - CREATE (x)-[:R]->(n1) - CREATE (n1)-[:R]->(n2) - CREATE (n2)-[:R]->(n3) - """ - When executing query: - """ - MATCH p = (:X)-->()-->()-->() - DETACH DELETE p - """ - Then the result should be empty - And the side effects should be: - | -nodes | 4 | - | -relationships | 3 | - | -labels | 1 | - Scenario: Undirected expand followed by delete and count Given an empty graph And having executed: