From 1cee7ecb8ae0698bfdab13fe3c8c1850a96e81c8 Mon Sep 17 00:00:00 2001
From: gvolfing <gabor.volfinger@memgraph.io>
Date: Fri, 4 Nov 2022 08:12:37 +0100
Subject: [PATCH 1/5] Make ShardRsm aware of trying to write the same vertex
into the skip-list
---
src/storage/v3/schema_validator.hpp | 4 +++-
src/storage/v3/shard.cpp | 6 +++++-
src/storage/v3/shard_rsm.cpp | 8 ++++++--
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/storage/v3/schema_validator.hpp b/src/storage/v3/schema_validator.hpp
index d57dfae2e..ad6f4e680 100644
--- a/src/storage/v3/schema_validator.hpp
+++ b/src/storage/v3/schema_validator.hpp
@@ -79,7 +79,9 @@ struct VertexValidator {
LabelId primary_label_;
};
+struct AlreadyInsertedElement {};
+
template <typename TValue>
-using ResultSchema = utils::BasicResult<std::variant<SchemaViolation, Error>, TValue>;
+using ResultSchema = utils::BasicResult<std::variant<SchemaViolation, AlreadyInsertedElement, Error>, TValue>;
} // namespace memgraph::storage::v3
diff --git a/src/storage/v3/shard.cpp b/src/storage/v3/shard.cpp
index d6ba56ac3..09dd91661 100644
--- a/src/storage/v3/shard.cpp
+++ b/src/storage/v3/shard.cpp
@@ -361,7 +361,11 @@ ResultSchema<VertexAccessor> Shard::Accessor::CreateVertexAndValidate(
delta->prev.Set(&it->vertex);
VertexAccessor vertex_acc{&it->vertex, transaction_, &shard_->indices_, config_, shard_->vertex_validator_};
- MG_ASSERT(inserted, "The vertex must be inserted here!");
+ // Vertex is allready inserted, return an error.
+ if (!inserted) {
+ return {AlreadyInsertedElement{}};
+ }
+ // MG_ASSERT(inserted, "The vertex must be inserted here!");
MG_ASSERT(it != acc.end(), "Invalid Vertex accessor!");
// TODO(jbajic) Improve, maybe delay index update
diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp
index 9a124b250..bdd6e7b81 100644
--- a/src/storage/v3/shard_rsm.cpp
+++ b/src/storage/v3/shard_rsm.cpp
@@ -498,6 +498,8 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateVerticesRequest &&req) {
spdlog::debug("Creating vertex failed with error: SchemaViolation");
} else if constexpr (std::is_same_v<ErrorType, Error>) {
spdlog::debug("Creating vertex failed with error: Error");
+ } else if constexpr (std::is_same_v<ErrorType, AlreadyInsertedElement>) {
+ spdlog::debug("Updating vertex failed with error: AlreadyInsertedElement");
} else {
static_assert(kAlwaysFalse<T>, "Missing type from variant visitor");
}
@@ -540,17 +542,19 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateVerticesRequest &&req) {
[&action_successful]<typename T>(T &&) {
using ErrorType = std::remove_cvref_t<T>;
if constexpr (std::is_same_v<ErrorType, SchemaViolation>) {
- action_successful = false;
spdlog::debug("Updating vertex failed with error: SchemaViolation");
} else if constexpr (std::is_same_v<ErrorType, Error>) {
- action_successful = false;
spdlog::debug("Updating vertex failed with error: Error");
+ } else if constexpr (std::is_same_v<ErrorType, AlreadyInsertedElement>) {
+ spdlog::debug("Updating vertex failed with error: AlreadyInsertedElement");
} else {
static_assert(kAlwaysFalse<T>, "Missing type from variant visitor");
}
},
error);
+ action_successful = false;
+
break;
}
}
From 3d954e7abcc998750039c1a44df7f313d3d873f8 Mon Sep 17 00:00:00 2001
From: gvolfing <gabor.volfinger@memgraph.io>
Date: Fri, 4 Nov 2022 15:04:25 +0100
Subject: [PATCH 2/5] Restructure SchemaResult type and uts usage
Rename SchemaResult to ShardOperationResult move it into a separate
header and add a new type to the underlying variant that indicates that
the vertex, the user would like to insert into the skip-list already
exist.
---
src/query/v2/common.hpp | 4 ++--
src/query/v2/db_accessor.hpp | 21 +++++++++--------
src/storage/v3/schema_validator.hpp | 5 ----
src/storage/v3/shard.cpp | 5 ++--
src/storage/v3/shard.hpp | 3 ++-
src/storage/v3/shard_operation_result.hpp | 28 +++++++++++++++++++++++
src/storage/v3/shard_rsm.cpp | 2 +-
src/storage/v3/vertex_accessor.cpp | 9 ++++----
src/storage/v3/vertex_accessor.hpp | 8 +++----
tests/unit/storage_v3_edge.cpp | 3 ++-
10 files changed, 58 insertions(+), 30 deletions(-)
create mode 100644 src/storage/v3/shard_operation_result.hpp
diff --git a/src/query/v2/common.hpp b/src/query/v2/common.hpp
index 5637a27bc..b76ce0af0 100644
--- a/src/query/v2/common.hpp
+++ b/src/query/v2/common.hpp
@@ -28,7 +28,7 @@
#include "storage/v3/id_types.hpp"
#include "storage/v3/property_value.hpp"
#include "storage/v3/result.hpp"
-#include "storage/v3/schema_validator.hpp"
+#include "storage/v3/shard_operation_result.hpp"
#include "storage/v3/view.hpp"
#include "utils/exceptions.hpp"
#include "utils/logging.hpp"
@@ -93,7 +93,7 @@ concept AccessorWithSetPropertyAndValidate = requires(T accessor, const storage:
const storage::v3::PropertyValue new_value) {
{
accessor.SetPropertyAndValidate(key, new_value)
- } -> std::same_as<storage::v3::ResultSchema<storage::v3::PropertyValue>>;
+ } -> std::same_as<storage::v3::ShardOperationResult<storage::v3::PropertyValue>>;
};
template <typename TRecordAccessor>
diff --git a/src/query/v2/db_accessor.hpp b/src/query/v2/db_accessor.hpp
index a975a70ce..b76da6131 100644
--- a/src/query/v2/db_accessor.hpp
+++ b/src/query/v2/db_accessor.hpp
@@ -23,6 +23,7 @@
#include "storage/v3/key_store.hpp"
#include "storage/v3/property_value.hpp"
#include "storage/v3/result.hpp"
+#include "storage/v3/shard_operation_result.hpp"
///////////////////////////////////////////////////////////
// Our communication layer and query engine don't mix
@@ -113,17 +114,19 @@ class VertexAccessor final {
auto PrimaryKey(storage::v3::View view) const { return impl_.PrimaryKey(view); }
- storage::v3::ResultSchema<bool> AddLabel(storage::v3::LabelId label) { return impl_.AddLabelAndValidate(label); }
-
- storage::v3::ResultSchema<bool> AddLabelAndValidate(storage::v3::LabelId label) {
+ storage::v3::ShardOperationResult<bool> AddLabel(storage::v3::LabelId label) {
return impl_.AddLabelAndValidate(label);
}
- storage::v3::ResultSchema<bool> RemoveLabel(storage::v3::LabelId label) {
+ storage::v3::ShardOperationResult<bool> AddLabelAndValidate(storage::v3::LabelId label) {
+ return impl_.AddLabelAndValidate(label);
+ }
+
+ storage::v3::ShardOperationResult<bool> RemoveLabel(storage::v3::LabelId label) {
return impl_.RemoveLabelAndValidate(label);
}
- storage::v3::ResultSchema<bool> RemoveLabelAndValidate(storage::v3::LabelId label) {
+ storage::v3::ShardOperationResult<bool> RemoveLabelAndValidate(storage::v3::LabelId label) {
return impl_.RemoveLabelAndValidate(label);
}
@@ -138,17 +141,17 @@ class VertexAccessor final {
return impl_.GetProperty(key, view);
}
- storage::v3::ResultSchema<storage::v3::PropertyValue> SetProperty(storage::v3::PropertyId key,
- const storage::v3::PropertyValue &value) {
+ storage::v3::ShardOperationResult<storage::v3::PropertyValue> SetProperty(storage::v3::PropertyId key,
+ const storage::v3::PropertyValue &value) {
return impl_.SetPropertyAndValidate(key, value);
}
- storage::v3::ResultSchema<storage::v3::PropertyValue> SetPropertyAndValidate(
+ storage::v3::ShardOperationResult<storage::v3::PropertyValue> SetPropertyAndValidate(
storage::v3::PropertyId key, const storage::v3::PropertyValue &value) {
return impl_.SetPropertyAndValidate(key, value);
}
- storage::v3::ResultSchema<storage::v3::PropertyValue> RemovePropertyAndValidate(storage::v3::PropertyId key) {
+ storage::v3::ShardOperationResult<storage::v3::PropertyValue> RemovePropertyAndValidate(storage::v3::PropertyId key) {
return SetPropertyAndValidate(key, storage::v3::PropertyValue{});
}
diff --git a/src/storage/v3/schema_validator.hpp b/src/storage/v3/schema_validator.hpp
index ad6f4e680..ce813fde2 100644
--- a/src/storage/v3/schema_validator.hpp
+++ b/src/storage/v3/schema_validator.hpp
@@ -79,9 +79,4 @@ struct VertexValidator {
LabelId primary_label_;
};
-struct AlreadyInsertedElement {};
-
-template <typename TValue>
-using ResultSchema = utils::BasicResult<std::variant<SchemaViolation, AlreadyInsertedElement, Error>, TValue>;
-
} // namespace memgraph::storage::v3
diff --git a/src/storage/v3/shard.cpp b/src/storage/v3/shard.cpp
index 09dd91661..942ce65b2 100644
--- a/src/storage/v3/shard.cpp
+++ b/src/storage/v3/shard.cpp
@@ -33,6 +33,7 @@
#include "storage/v3/mvcc.hpp"
#include "storage/v3/property_value.hpp"
#include "storage/v3/schema_validator.hpp"
+#include "storage/v3/shard_operation_result.hpp"
#include "storage/v3/transaction.hpp"
#include "storage/v3/vertex.hpp"
#include "storage/v3/vertex_accessor.hpp"
@@ -343,7 +344,7 @@ Shard::~Shard() {}
Shard::Accessor::Accessor(Shard &shard, Transaction &transaction)
: shard_(&shard), transaction_(&transaction), config_(shard_->config_.items) {}
-ResultSchema<VertexAccessor> Shard::Accessor::CreateVertexAndValidate(
+ShardOperationResult<VertexAccessor> Shard::Accessor::CreateVertexAndValidate(
const std::vector<LabelId> &labels, const std::vector<PropertyValue> &primary_properties,
const std::vector<std::pair<PropertyId, PropertyValue>> &properties) {
OOMExceptionEnabler oom_exception;
@@ -361,11 +362,9 @@ ResultSchema<VertexAccessor> Shard::Accessor::CreateVertexAndValidate(
delta->prev.Set(&it->vertex);
VertexAccessor vertex_acc{&it->vertex, transaction_, &shard_->indices_, config_, shard_->vertex_validator_};
- // Vertex is allready inserted, return an error.
if (!inserted) {
return {AlreadyInsertedElement{}};
}
- // MG_ASSERT(inserted, "The vertex must be inserted here!");
MG_ASSERT(it != acc.end(), "Invalid Vertex accessor!");
// TODO(jbajic) Improve, maybe delay index update
diff --git a/src/storage/v3/shard.hpp b/src/storage/v3/shard.hpp
index a3f1fc90d..778c92abd 100644
--- a/src/storage/v3/shard.hpp
+++ b/src/storage/v3/shard.hpp
@@ -38,6 +38,7 @@
#include "storage/v3/result.hpp"
#include "storage/v3/schema_validator.hpp"
#include "storage/v3/schemas.hpp"
+#include "storage/v3/shard_operation_result.hpp"
#include "storage/v3/transaction.hpp"
#include "storage/v3/vertex.hpp"
#include "storage/v3/vertex_accessor.hpp"
@@ -206,7 +207,7 @@ class Shard final {
public:
/// @throw std::bad_alloc
- ResultSchema<VertexAccessor> CreateVertexAndValidate(
+ ShardOperationResult<VertexAccessor> CreateVertexAndValidate(
const std::vector<LabelId> &labels, const std::vector<PropertyValue> &primary_properties,
const std::vector<std::pair<PropertyId, PropertyValue>> &properties);
diff --git a/src/storage/v3/shard_operation_result.hpp b/src/storage/v3/shard_operation_result.hpp
new file mode 100644
index 000000000..e055a0022
--- /dev/null
+++ b/src/storage/v3/shard_operation_result.hpp
@@ -0,0 +1,28 @@
+// Copyright 2022 Memgraph Ltd.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source
+// License, and you may not use this file except in compliance with the Business Source License.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+#pragma once
+
+#include <variant>
+
+#include "storage/v3/result.hpp"
+#include "storage/v3/schema_validator.hpp"
+
+namespace memgraph::storage::v3 {
+
+struct AlreadyInsertedElement {};
+
+using ResultErrorType = std::variant<SchemaViolation, AlreadyInsertedElement, Error>;
+
+template <typename TValue>
+using ShardOperationResult = utils::BasicResult<ResultErrorType, TValue>;
+
+} // namespace memgraph::storage::v3
diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp
index bdd6e7b81..faae52f95 100644
--- a/src/storage/v3/shard_rsm.cpp
+++ b/src/storage/v3/shard_rsm.cpp
@@ -539,7 +539,7 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateVerticesRequest &&req) {
auto &error = result_schema.GetError();
std::visit(
- [&action_successful]<typename T>(T &&) {
+ []<typename T>(T &&) {
using ErrorType = std::remove_cvref_t<T>;
if constexpr (std::is_same_v<ErrorType, SchemaViolation>) {
spdlog::debug("Updating vertex failed with error: SchemaViolation");
diff --git a/src/storage/v3/vertex_accessor.cpp b/src/storage/v3/vertex_accessor.cpp
index 696c4657a..543caa88a 100644
--- a/src/storage/v3/vertex_accessor.cpp
+++ b/src/storage/v3/vertex_accessor.cpp
@@ -21,8 +21,8 @@
#include "storage/v3/key_store.hpp"
#include "storage/v3/mvcc.hpp"
#include "storage/v3/property_value.hpp"
-#include "storage/v3/schema_validator.hpp"
#include "storage/v3/shard.hpp"
+#include "storage/v3/shard_operation_result.hpp"
#include "storage/v3/vertex.hpp"
#include "utils/logging.hpp"
#include "utils/memory_tracker.hpp"
@@ -98,7 +98,7 @@ Result<bool> VertexAccessor::AddLabel(LabelId label) {
return true;
}
-ResultSchema<bool> VertexAccessor::AddLabelAndValidate(LabelId label) {
+ShardOperationResult<bool> VertexAccessor::AddLabelAndValidate(LabelId label) {
if (const auto maybe_violation_error = vertex_validator_->ValidateAddLabel(label); maybe_violation_error) {
return {*maybe_violation_error};
}
@@ -134,7 +134,7 @@ Result<bool> VertexAccessor::RemoveLabel(LabelId label) {
return true;
}
-ResultSchema<bool> VertexAccessor::RemoveLabelAndValidate(LabelId label) {
+ShardOperationResult<bool> VertexAccessor::RemoveLabelAndValidate(LabelId label) {
if (const auto maybe_violation_error = vertex_validator_->ValidateRemoveLabel(label); maybe_violation_error) {
return {*maybe_violation_error};
}
@@ -331,7 +331,8 @@ Result<void> VertexAccessor::CheckVertexExistence(View view) const {
return {};
}
-ResultSchema<PropertyValue> VertexAccessor::SetPropertyAndValidate(PropertyId property, const PropertyValue &value) {
+ShardOperationResult<PropertyValue> VertexAccessor::SetPropertyAndValidate(PropertyId property,
+ const PropertyValue &value) {
if (auto maybe_violation_error = vertex_validator_->ValidatePropertyUpdate(property); maybe_violation_error) {
return {*maybe_violation_error};
}
diff --git a/src/storage/v3/vertex_accessor.hpp b/src/storage/v3/vertex_accessor.hpp
index a2bf8352f..682a04e39 100644
--- a/src/storage/v3/vertex_accessor.hpp
+++ b/src/storage/v3/vertex_accessor.hpp
@@ -17,7 +17,7 @@
#include "storage/v3/id_types.hpp"
#include "storage/v3/key_store.hpp"
#include "storage/v3/result.hpp"
-#include "storage/v3/schema_validator.hpp"
+#include "storage/v3/shard_operation_result.hpp"
#include "storage/v3/transaction.hpp"
#include "storage/v3/vertex.hpp"
#include "storage/v3/vertex_id.hpp"
@@ -55,13 +55,13 @@ class VertexAccessor final {
/// `false` is returned if the label already existed, or SchemaViolation
/// if adding the label has violated one of the schema constraints.
/// @throw std::bad_alloc
- ResultSchema<bool> AddLabelAndValidate(LabelId label);
+ ShardOperationResult<bool> AddLabelAndValidate(LabelId label);
/// Remove a label and return `true` if deletion took place.
/// `false` is returned if the vertex did not have a label already. or SchemaViolation
/// if adding the label has violated one of the schema constraints.
/// @throw std::bad_alloc
- ResultSchema<bool> RemoveLabelAndValidate(LabelId label);
+ ShardOperationResult<bool> RemoveLabelAndValidate(LabelId label);
Result<bool> HasLabel(View view, LabelId label) const;
@@ -80,7 +80,7 @@ class VertexAccessor final {
/// Set a property value and return the old value or error.
/// @throw std::bad_alloc
- ResultSchema<PropertyValue> SetPropertyAndValidate(PropertyId property, const PropertyValue &value);
+ ShardOperationResult<PropertyValue> SetPropertyAndValidate(PropertyId property, const PropertyValue &value);
/// Remove all properties and return the values of the removed properties.
/// @throw std::bad_alloc
diff --git a/tests/unit/storage_v3_edge.cpp b/tests/unit/storage_v3_edge.cpp
index 293f561df..21c4214ce 100644
--- a/tests/unit/storage_v3_edge.cpp
+++ b/tests/unit/storage_v3_edge.cpp
@@ -18,6 +18,7 @@
#include "storage/v3/name_id_mapper.hpp"
#include "storage/v3/property_value.hpp"
#include "storage/v3/shard.hpp"
+#include "storage/v3/shard_operation_result.hpp"
namespace memgraph::storage::v3::tests {
using testing::UnorderedElementsAre;
@@ -38,7 +39,7 @@ class StorageEdgeTest : public ::testing::TestWithParam<bool> {
return store.NameToEdgeType(edge_type_name);
}
- static ResultSchema<VertexAccessor> CreateVertex(Shard::Accessor &acc, const PropertyValue &key) {
+ static ShardOperationResult<VertexAccessor> CreateVertex(Shard::Accessor &acc, const PropertyValue &key) {
return acc.CreateVertexAndValidate({}, {key}, {});
}
From 39b40ecf00eb31befdbe14c44ca8af57361b57c4 Mon Sep 17 00:00:00 2001
From: gvolfing <gabor.volfinger@memgraph.io>
Date: Mon, 7 Nov 2022 10:00:34 +0100
Subject: [PATCH 3/5] Extend the Error enum instead of a separate type
The error representing that a vertex is already inserted into the
skip-list was represented by the struct AlreadyInseertedElement. Instead
of using that struct, extend the memgraph::storage::v3::Error scoped
enum and use that to represent the double-insertion error.
---
src/expr/interpret/eval.hpp | 3 ++
src/memgraph.cpp | 2 +
src/query/v2/common.hpp | 2 +
src/storage/v3/result.hpp | 1 +
src/storage/v3/shard.cpp | 2 +-
src/storage/v3/shard_operation_result.hpp | 4 +-
src/storage/v3/shard_rsm.cpp | 50 +++++++++++++++++++----
7 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/src/expr/interpret/eval.hpp b/src/expr/interpret/eval.hpp
index 1538028a0..70127c023 100644
--- a/src/expr/interpret/eval.hpp
+++ b/src/expr/interpret/eval.hpp
@@ -404,6 +404,7 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
case Error::SERIALIZATION_ERROR:
case Error::VERTEX_HAS_EDGES:
case Error::PROPERTIES_DISABLED:
+ case Error::VERTEX_ALREADY_INSERTED:
throw ExpressionRuntimeException("Unexpected error when accessing labels.");
}
}
@@ -751,6 +752,7 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
case Error::SERIALIZATION_ERROR:
case Error::VERTEX_HAS_EDGES:
case Error::PROPERTIES_DISABLED:
+ case Error::VERTEX_ALREADY_INSERTED:
throw ExpressionRuntimeException("Unexpected error when getting a property.");
}
}
@@ -779,6 +781,7 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
case Error::SERIALIZATION_ERROR:
case Error::VERTEX_HAS_EDGES:
case Error::PROPERTIES_DISABLED:
+ case Error::VERTEX_ALREADY_INSERTED:
throw ExpressionRuntimeException("Unexpected error when getting a property.");
}
}
diff --git a/src/memgraph.cpp b/src/memgraph.cpp
index 0f80266e5..deaf9e568 100644
--- a/src/memgraph.cpp
+++ b/src/memgraph.cpp
@@ -489,6 +489,7 @@ class BoltSession final : public memgraph::communication::bolt::Session<memgraph
case memgraph::storage::v3::Error::VERTEX_HAS_EDGES:
case memgraph::storage::v3::Error::PROPERTIES_DISABLED:
case memgraph::storage::v3::Error::NONEXISTENT_OBJECT:
+ case memgraph::storage::v3::Error::VERTEX_ALREADY_INSERTED:
throw memgraph::communication::bolt::ClientError("Unexpected storage error when streaming summary.");
}
}
@@ -523,6 +524,7 @@ class BoltSession final : public memgraph::communication::bolt::Session<memgraph
case memgraph::storage::v3::Error::VERTEX_HAS_EDGES:
case memgraph::storage::v3::Error::SERIALIZATION_ERROR:
case memgraph::storage::v3::Error::PROPERTIES_DISABLED:
+ case memgraph::storage::v3::Error::VERTEX_ALREADY_INSERTED:
throw memgraph::communication::bolt::ClientError("Unexpected storage error when streaming results.");
}
}
diff --git a/src/query/v2/common.hpp b/src/query/v2/common.hpp
index b76ce0af0..55c60ce10 100644
--- a/src/query/v2/common.hpp
+++ b/src/query/v2/common.hpp
@@ -110,6 +110,8 @@ inline void HandleErrorOnPropertyUpdate(const storage::v3::Error error) {
throw QueryRuntimeException("Can't set property because properties on edges are disabled.");
case storage::v3::Error::VERTEX_HAS_EDGES:
case storage::v3::Error::NONEXISTENT_OBJECT:
+ case storage::v3::Error::VERTEX_ALREADY_INSERTED:
+
throw QueryRuntimeException("Unexpected error when setting a property.");
}
}
diff --git a/src/storage/v3/result.hpp b/src/storage/v3/result.hpp
index cf12c0cff..86fdb10d0 100644
--- a/src/storage/v3/result.hpp
+++ b/src/storage/v3/result.hpp
@@ -25,6 +25,7 @@ enum class Error : uint8_t {
DELETED_OBJECT,
VERTEX_HAS_EDGES,
PROPERTIES_DISABLED,
+ VERTEX_ALREADY_INSERTED
};
template <class TValue>
diff --git a/src/storage/v3/shard.cpp b/src/storage/v3/shard.cpp
index 942ce65b2..0e596172d 100644
--- a/src/storage/v3/shard.cpp
+++ b/src/storage/v3/shard.cpp
@@ -363,7 +363,7 @@ ShardOperationResult<VertexAccessor> Shard::Accessor::CreateVertexAndValidate(
VertexAccessor vertex_acc{&it->vertex, transaction_, &shard_->indices_, config_, shard_->vertex_validator_};
if (!inserted) {
- return {AlreadyInsertedElement{}};
+ return {Error::VERTEX_ALREADY_INSERTED};
}
MG_ASSERT(it != acc.end(), "Invalid Vertex accessor!");
diff --git a/src/storage/v3/shard_operation_result.hpp b/src/storage/v3/shard_operation_result.hpp
index e055a0022..8de800b83 100644
--- a/src/storage/v3/shard_operation_result.hpp
+++ b/src/storage/v3/shard_operation_result.hpp
@@ -18,9 +18,7 @@
namespace memgraph::storage::v3 {
-struct AlreadyInsertedElement {};
-
-using ResultErrorType = std::variant<SchemaViolation, AlreadyInsertedElement, Error>;
+using ResultErrorType = std::variant<SchemaViolation, Error>;
template <typename TValue>
using ShardOperationResult = utils::BasicResult<ResultErrorType, TValue>;
diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp
index faae52f95..2911e26d6 100644
--- a/src/storage/v3/shard_rsm.cpp
+++ b/src/storage/v3/shard_rsm.cpp
@@ -492,14 +492,31 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateVerticesRequest &&req) {
auto &error = result_schema.GetError();
std::visit(
- []<typename T>(T &&) {
+ []<typename T>(T &&error) {
using ErrorType = std::remove_cvref_t<T>;
if constexpr (std::is_same_v<ErrorType, SchemaViolation>) {
spdlog::debug("Creating vertex failed with error: SchemaViolation");
} else if constexpr (std::is_same_v<ErrorType, Error>) {
- spdlog::debug("Creating vertex failed with error: Error");
- } else if constexpr (std::is_same_v<ErrorType, AlreadyInsertedElement>) {
- spdlog::debug("Updating vertex failed with error: AlreadyInsertedElement");
+ switch (error) {
+ case Error::DELETED_OBJECT:
+ spdlog::debug("Creating vertex failed with error: DELETED_OBJECT");
+ break;
+ case Error::NONEXISTENT_OBJECT:
+ spdlog::debug("Creating vertex failed with error: NONEXISTENT_OBJECT");
+ break;
+ case Error::SERIALIZATION_ERROR:
+ spdlog::debug("Creating vertex failed with error: SERIALIZATION_ERROR");
+ break;
+ case Error::PROPERTIES_DISABLED:
+ spdlog::debug("Creating vertex failed with error: PROPERTIES_DISABLED");
+ break;
+ case Error::VERTEX_HAS_EDGES:
+ spdlog::debug("Creating vertex failed with error: VERTEX_HAS_EDGES");
+ break;
+ case Error::VERTEX_ALREADY_INSERTED:
+ spdlog::debug("Creating vertex failed with error: VERTEX_ALREADY_INSERTED");
+ break;
+ }
} else {
static_assert(kAlwaysFalse<T>, "Missing type from variant visitor");
}
@@ -539,14 +556,31 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateVerticesRequest &&req) {
auto &error = result_schema.GetError();
std::visit(
- []<typename T>(T &&) {
+ []<typename T>(T &&error) {
using ErrorType = std::remove_cvref_t<T>;
if constexpr (std::is_same_v<ErrorType, SchemaViolation>) {
spdlog::debug("Updating vertex failed with error: SchemaViolation");
} else if constexpr (std::is_same_v<ErrorType, Error>) {
- spdlog::debug("Updating vertex failed with error: Error");
- } else if constexpr (std::is_same_v<ErrorType, AlreadyInsertedElement>) {
- spdlog::debug("Updating vertex failed with error: AlreadyInsertedElement");
+ switch (error) {
+ case Error::DELETED_OBJECT:
+ spdlog::debug("Updating vertex failed with error: DELETED_OBJECT");
+ break;
+ case Error::NONEXISTENT_OBJECT:
+ spdlog::debug("Updating vertex failed with error: NONEXISTENT_OBJECT");
+ break;
+ case Error::SERIALIZATION_ERROR:
+ spdlog::debug("Updating vertex failed with error: SERIALIZATION_ERROR");
+ break;
+ case Error::PROPERTIES_DISABLED:
+ spdlog::debug("Updating vertex failed with error: PROPERTIES_DISABLED");
+ break;
+ case Error::VERTEX_HAS_EDGES:
+ spdlog::debug("Updating vertex failed with error: VERTEX_HAS_EDGES");
+ break;
+ case Error::VERTEX_ALREADY_INSERTED:
+ spdlog::debug("Updating vertex failed with error: VERTEX_ALREADY_INSERTED");
+ break;
+ }
} else {
static_assert(kAlwaysFalse<T>, "Missing type from variant visitor");
}
From 91550128a59d336a8bc1a01d4ec53e28bcdcc425 Mon Sep 17 00:00:00 2001
From: gvolfing <gabor.volfinger@memgraph.io>
Date: Mon, 7 Nov 2022 11:46:24 +0100
Subject: [PATCH 4/5] Conform unit test with the new error-handling
---
tests/unit/storage_v3.cpp | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/tests/unit/storage_v3.cpp b/tests/unit/storage_v3.cpp
index b62606a8e..3098acbdb 100644
--- a/tests/unit/storage_v3.cpp
+++ b/tests/unit/storage_v3.cpp
@@ -33,6 +33,12 @@
using testing::UnorderedElementsAre;
+namespace {
+
+class AlreadyInsertedException : public std::exception {};
+
+} // namespace
+
namespace memgraph::storage::v3::tests {
class StorageV3 : public ::testing::TestWithParam<bool> {
@@ -2650,14 +2656,21 @@ TEST_P(StorageV3, TestCreateVertexAndValidate) {
(std::map<PropertyId, PropertyValue>{{prop1, PropertyValue(111)}}));
}
{
- ASSERT_DEATH(
+ EXPECT_THROW(
{
Shard store(primary_label, min_pk, std::nullopt /*max_primary_key*/, schema_property_vector);
auto acc = store.Access(GetNextHlc());
auto vertex1 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
auto vertex2 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
+
+ if (vertex2.HasError()) {
+ auto error = vertex2.GetError();
+ if (auto error_ptr = std::get_if<memgraph::storage::v3::Error>(&error)) {
+ if (*error_ptr == storage::v3::Error::VERTEX_ALREADY_INSERTED) throw AlreadyInsertedException();
+ }
+ }
},
- "");
+ AlreadyInsertedException);
}
{
auto acc = store.Access(GetNextHlc());
From 79756ae6fbf58453733f5ce8a6ec4ee54b276362 Mon Sep 17 00:00:00 2001
From: gvolfing <gabor.volfinger@memgraph.io>
Date: Tue, 8 Nov 2022 07:31:01 +0100
Subject: [PATCH 5/5] Modify unit test
Instead of Creating an exception that is would be only used in this
file, just assert the type of the error the double vertex insertion
operation should yield.
---
tests/unit/storage_v3.cpp | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/tests/unit/storage_v3.cpp b/tests/unit/storage_v3.cpp
index 3098acbdb..b107786e8 100644
--- a/tests/unit/storage_v3.cpp
+++ b/tests/unit/storage_v3.cpp
@@ -33,12 +33,6 @@
using testing::UnorderedElementsAre;
-namespace {
-
-class AlreadyInsertedException : public std::exception {};
-
-} // namespace
-
namespace memgraph::storage::v3::tests {
class StorageV3 : public ::testing::TestWithParam<bool> {
@@ -2656,21 +2650,16 @@ TEST_P(StorageV3, TestCreateVertexAndValidate) {
(std::map<PropertyId, PropertyValue>{{prop1, PropertyValue(111)}}));
}
{
- EXPECT_THROW(
- {
- Shard store(primary_label, min_pk, std::nullopt /*max_primary_key*/, schema_property_vector);
- auto acc = store.Access(GetNextHlc());
- auto vertex1 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
- auto vertex2 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
+ Shard store(primary_label, min_pk, std::nullopt /*max_primary_key*/, schema_property_vector);
+ auto acc = store.Access(GetNextHlc());
+ auto vertex1 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
+ auto vertex2 = acc.CreateVertexAndValidate({}, {PropertyValue{0}}, {});
- if (vertex2.HasError()) {
- auto error = vertex2.GetError();
- if (auto error_ptr = std::get_if<memgraph::storage::v3::Error>(&error)) {
- if (*error_ptr == storage::v3::Error::VERTEX_ALREADY_INSERTED) throw AlreadyInsertedException();
- }
- }
- },
- AlreadyInsertedException);
+ ASSERT_TRUE(vertex2.HasError());
+ auto error = vertex2.GetError();
+ auto error_ptr = std::get_if<memgraph::storage::v3::Error>(&error);
+ ASSERT_TRUE(error_ptr);
+ ASSERT_TRUE(*error_ptr == storage::v3::Error::VERTEX_ALREADY_INSERTED);
}
{
auto acc = store.Access(GetNextHlc());