Add support for PropertyValue to SLK

Summary:
Improve tests
Add exceptions

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

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

src/communication/rpc/serialization.hpp

@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "communication/rpc/streams.hpp"
+#include "utils/exceptions.hpp"
 
 // The namespace name stands for SaveLoadKit. It should be not mistaken for the
 // Mercedes car model line.
@@ -27,6 +28,13 @@ static_assert(std::experimental::is_same_v<std::uint8_t, char> ||
               "The slk library requires uint8_t to be implemented as char or "
               "unsigned char.");
 
+/// Exception that will be thrown if an object can't be decoded from the byte
+/// stream.
+class SlkDecodeException : public utils::BasicException {
+ public:
+  using utils::BasicException::BasicException;
+};
+
 // Forward declarations for all recursive `Save` and `Load` functions must be
 // here because C++ doesn't know how to resolve the function call if it isn't in
 // the global namespace.
@@ -320,8 +328,11 @@ inline void Load(std::shared_ptr<T> *obj, Reader *reader,
     } else {
       uint64_t index = 0;
       Load(&index, reader);
-      // TODO: handle if index doesn't exist!
-      *obj = (*loaded)[index];
+      if (index < loaded->size()) {
+        *obj = (*loaded)[index];
+      } else {
+        throw SlkDecodeException("Couldn't load shared pointer!");
+      }
     }
   } else {
     *obj = nullptr;

src/storage/common/types/property_value.cpp

@@ -152,53 +152,6 @@ PropertyValue::PropertyValue(PropertyValue &&other) : type_(other.type_) {
   }
 }
 
-std::ostream &operator<<(std::ostream &os, const PropertyValue::Type type) {
-  switch (type) {
-    case PropertyValue::Type::Null:
-      return os << "null";
-    case PropertyValue::Type::Bool:
-      return os << "bool";
-    case PropertyValue::Type::String:
-      return os << "string";
-    case PropertyValue::Type::Int:
-      return os << "int";
-    case PropertyValue::Type::Double:
-      return os << "double";
-    case PropertyValue::Type::List:
-      return os << "list";
-    case PropertyValue::Type::Map:
-      return os << "map";
-  }
-}
-
-std::ostream &operator<<(std::ostream &os, const PropertyValue &value) {
-  switch (value.type_) {
-    case PropertyValue::Type::Null:
-      return os << "Null";
-    case PropertyValue::Type::Bool:
-      return os << (value.Value<bool>() ? "true" : "false");
-    case PropertyValue::Type::String:
-      return os << value.Value<std::string>();
-    case PropertyValue::Type::Int:
-      return os << value.Value<int64_t>();
-    case PropertyValue::Type::Double:
-      return os << value.Value<double>();
-    case PropertyValue::Type::List:
-      os << "[";
-      for (const auto &x : value.Value<std::vector<PropertyValue>>()) {
-        os << x << ",";
-      }
-      return os << "]";
-    case PropertyValue::Type::Map:
-      os << "{";
-      for (const auto &kv :
-           value.Value<std::map<std::string, PropertyValue>>()) {
-        os << kv.first << ": " << kv.second << ",";
-      }
-      return os << "}";
-  }
-}
-
 PropertyValue &PropertyValue::operator=(const PropertyValue &other) {
   if (this != &other) {
     DestroyValue();
@@ -294,3 +247,72 @@ void PropertyValue::DestroyValue() {
 }
 
 PropertyValue::~PropertyValue() { DestroyValue(); }
+
+std::ostream &operator<<(std::ostream &os, const PropertyValue::Type type) {
+  switch (type) {
+    case PropertyValue::Type::Null:
+      return os << "null";
+    case PropertyValue::Type::Bool:
+      return os << "bool";
+    case PropertyValue::Type::String:
+      return os << "string";
+    case PropertyValue::Type::Int:
+      return os << "int";
+    case PropertyValue::Type::Double:
+      return os << "double";
+    case PropertyValue::Type::List:
+      return os << "list";
+    case PropertyValue::Type::Map:
+      return os << "map";
+  }
+}
+
+std::ostream &operator<<(std::ostream &os, const PropertyValue &value) {
+  switch (value.type()) {
+    case PropertyValue::Type::Null:
+      return os << "Null";
+    case PropertyValue::Type::Bool:
+      return os << (value.Value<bool>() ? "true" : "false");
+    case PropertyValue::Type::String:
+      return os << value.Value<std::string>();
+    case PropertyValue::Type::Int:
+      return os << value.Value<int64_t>();
+    case PropertyValue::Type::Double:
+      return os << value.Value<double>();
+    case PropertyValue::Type::List:
+      os << "[";
+      for (const auto &x : value.Value<std::vector<PropertyValue>>()) {
+        os << x << ",";
+      }
+      return os << "]";
+    case PropertyValue::Type::Map:
+      os << "{";
+      for (const auto &kv :
+           value.Value<std::map<std::string, PropertyValue>>()) {
+        os << kv.first << ": " << kv.second << ",";
+      }
+      return os << "}";
+  }
+}
+
+bool operator==(const PropertyValue &first, const PropertyValue &second) {
+  if (first.type() != second.type()) return false;
+  switch (first.type()) {
+    case PropertyValue::Type::Null:
+      return true;
+    case PropertyValue::Type::Bool:
+      return first.Value<bool>() == second.Value<bool>();
+    case PropertyValue::Type::Int:
+      return first.Value<int64_t>() == second.Value<int64_t>();
+    case PropertyValue::Type::Double:
+      return first.Value<double>() == second.Value<double>();
+    case PropertyValue::Type::String:
+      return first.Value<std::string>() == second.Value<std::string>();
+    case PropertyValue::Type::List:
+      return first.Value<std::vector<PropertyValue>>() ==
+             second.Value<std::vector<PropertyValue>>();
+    case PropertyValue::Type::Map:
+      return first.Value<std::map<std::string, PropertyValue>>() ==
+             second.Value<std::map<std::string, PropertyValue>>();
+  }
+}

src/storage/common/types/property_value.hpp

@@ -91,9 +91,6 @@ class PropertyValue {
   template <typename T>
   T &Value();
 
-  friend std::ostream &operator<<(std::ostream &stream,
-                                  const PropertyValue &prop);
-
  private:
   void DestroyValue();
 
@@ -127,3 +124,7 @@ class PropertyValueException : public utils::StacktraceException {
 
 // stream output
 std::ostream &operator<<(std::ostream &os, const PropertyValue::Type type);
+std::ostream &operator<<(std::ostream &os, const PropertyValue &value);
+
+// comparison
+bool operator==(const PropertyValue &first, const PropertyValue &second);

src/storage/common/types/property_value.slk.hpp

@@ -0,0 +1,109 @@
+#pragma once
+
+#include "communication/rpc/serialization.hpp"
+#include "communication/rpc/streams.hpp"
+#include "storage/common/types/property_value.hpp"
+
+namespace slk {
+inline void Save(const PropertyValue &obj, Builder *builder) {
+  switch (obj.type()) {
+    case PropertyValue::Type::Null: {
+      uint8_t type = 0;
+      Save(type, builder);
+      return;
+    }
+    case PropertyValue::Type::Bool: {
+      uint8_t type = 1;
+      Save(type, builder);
+      Save(obj.Value<bool>(), builder);
+      return;
+    }
+    case PropertyValue::Type::Int: {
+      uint8_t type = 2;
+      Save(type, builder);
+      Save(obj.Value<int64_t>(), builder);
+      return;
+    }
+    case PropertyValue::Type::Double: {
+      uint8_t type = 3;
+      Save(type, builder);
+      Save(obj.Value<double>(), builder);
+      return;
+    }
+    case PropertyValue::Type::String: {
+      uint8_t type = 4;
+      Save(type, builder);
+      Save(obj.Value<std::string>(), builder);
+      return;
+    }
+    case PropertyValue::Type::List: {
+      uint8_t type = 5;
+      Save(type, builder);
+      Save(obj.Value<std::vector<PropertyValue>>(), builder);
+      return;
+    }
+    case PropertyValue::Type::Map: {
+      uint8_t type = 6;
+      Save(type, builder);
+      Save(obj.Value<std::map<std::string, PropertyValue>>(), builder);
+      return;
+    }
+  }
+}
+
+inline void Load(PropertyValue *obj, Reader *reader) {
+  uint8_t type = 0;
+  Load(&type, reader);
+  switch (type) {
+    // Null
+    case 0: {
+      *obj = PropertyValue();
+      return;
+    }
+    // Bool
+    case 1: {
+      bool value = false;
+      Load(&value, reader);
+      *obj = PropertyValue(value);
+      return;
+    }
+    // Int
+    case 2: {
+      int64_t value = 0;
+      Load(&value, reader);
+      *obj = PropertyValue(value);
+      return;
+    }
+    // Double
+    case 3: {
+      double value = 0.0;
+      Load(&value, reader);
+      *obj = PropertyValue(value);
+      return;
+    }
+    // String
+    case 4: {
+      std::string value;
+      Load(&value, reader);
+      *obj = PropertyValue(std::move(value));
+      return;
+    }
+    // List
+    case 5: {
+      std::vector<PropertyValue> value;
+      Load(&value, reader);
+      *obj = PropertyValue(std::move(value));
+      return;
+    }
+    // Map
+    case 6: {
+      std::map<std::string, PropertyValue> value;
+      Load(&value, reader);
+      *obj = PropertyValue(std::move(value));
+      return;
+    }
+    // Invalid type
+    default: { throw SlkDecodeException("Couldn't load property value!"); }
+  }
+}
+}  // namespace slk

tests/unit/CMakeLists.txt

@@ -37,10 +37,6 @@ target_link_libraries(${test_prefix}concurrent_map mg-single-node kvstore_dummy_
 add_unit_test(counters.cpp)
 target_link_libraries(${test_prefix}counters mg-distributed kvstore_dummy_lib)
 
-# TODO (mferencevic): remove glog and gflags
-add_unit_test(custom_serialization.cpp)
-target_link_libraries(${test_prefix}custom_serialization glog gflags)
-
 add_unit_test(cypher_main_visitor.cpp)
 target_link_libraries(${test_prefix}cypher_main_visitor mg-single-node kvstore_dummy_lib)
 
@@ -219,6 +215,13 @@ target_link_libraries(${test_prefix}skiplist_reverse_iteration mg-single-node kv
 add_unit_test(skiplist_suffix.cpp)
 target_link_libraries(${test_prefix}skiplist_suffix mg-single-node kvstore_dummy_lib)
 
+add_unit_test(slk_advanced.cpp)
+target_link_libraries(${test_prefix}slk_advanced mg-single-node kvstore_dummy_lib)
+
+# TODO (mferencevic): remove glog, gflags and mg-single-node
+add_unit_test(slk_core.cpp)
+target_link_libraries(${test_prefix}slk_core glog gflags)
+
 add_unit_test(state_delta.cpp)
 target_link_libraries(${test_prefix}state_delta mg-single-node kvstore_dummy_lib)
 

tests/unit/slk_advanced.cpp

@@ -0,0 +1,77 @@
+#include <gtest/gtest.h>
+
+#include "storage/common/types/property_value.slk.hpp"
+
+TEST(SlkAdvanced, PropertyValueList) {
+  std::vector<PropertyValue> original{"hello world!", 5, 1.123423, true,
+                                      PropertyValue()};
+  ASSERT_EQ(original[0].type(), PropertyValue::Type::String);
+  ASSERT_EQ(original[1].type(), PropertyValue::Type::Int);
+  ASSERT_EQ(original[2].type(), PropertyValue::Type::Double);
+  ASSERT_EQ(original[3].type(), PropertyValue::Type::Bool);
+  ASSERT_EQ(original[4].type(), PropertyValue::Type::Null);
+
+  slk::Builder builder;
+  slk::Save(original, &builder);
+
+  std::vector<PropertyValue> decoded;
+  slk::Reader reader(builder.data(), builder.size());
+  slk::Load(&decoded, &reader);
+
+  ASSERT_EQ(original, decoded);
+}
+
+TEST(SlkAdvanced, PropertyValueMap) {
+  std::map<std::string, PropertyValue> original{{"hello", "world"},
+                                                {"number", 5},
+                                                {"real", 1.123423},
+                                                {"truth", true},
+                                                {"nothing", PropertyValue()}};
+  ASSERT_EQ(original["hello"].type(), PropertyValue::Type::String);
+  ASSERT_EQ(original["number"].type(), PropertyValue::Type::Int);
+  ASSERT_EQ(original["real"].type(), PropertyValue::Type::Double);
+  ASSERT_EQ(original["truth"].type(), PropertyValue::Type::Bool);
+  ASSERT_EQ(original["nothing"].type(), PropertyValue::Type::Null);
+
+  slk::Builder builder;
+  slk::Save(original, &builder);
+
+  std::map<std::string, PropertyValue> decoded;
+  slk::Reader reader(builder.data(), builder.size());
+  slk::Load(&decoded, &reader);
+
+  ASSERT_EQ(original, decoded);
+}
+
+TEST(SlkAdvanced, PropertyValueComplex) {
+  std::vector<PropertyValue> vec_v{"hello world!", 5, 1.123423, true,
+                                   PropertyValue()};
+  ASSERT_EQ(vec_v[0].type(), PropertyValue::Type::String);
+  ASSERT_EQ(vec_v[1].type(), PropertyValue::Type::Int);
+  ASSERT_EQ(vec_v[2].type(), PropertyValue::Type::Double);
+  ASSERT_EQ(vec_v[3].type(), PropertyValue::Type::Bool);
+  ASSERT_EQ(vec_v[4].type(), PropertyValue::Type::Null);
+
+  std::map<std::string, PropertyValue> map_v{{"hello", "world"},
+                                             {"number", 5},
+                                             {"real", 1.123423},
+                                             {"truth", true},
+                                             {"nothing", PropertyValue()}};
+  ASSERT_EQ(map_v["hello"].type(), PropertyValue::Type::String);
+  ASSERT_EQ(map_v["number"].type(), PropertyValue::Type::Int);
+  ASSERT_EQ(map_v["real"].type(), PropertyValue::Type::Double);
+  ASSERT_EQ(map_v["truth"].type(), PropertyValue::Type::Bool);
+  ASSERT_EQ(map_v["nothing"].type(), PropertyValue::Type::Null);
+
+  PropertyValue original({vec_v, map_v});
+  ASSERT_EQ(original.type(), PropertyValue::Type::List);
+
+  slk::Builder builder;
+  slk::Save(original, &builder);
+
+  PropertyValue decoded;
+  slk::Reader reader(builder.data(), builder.size());
+  slk::Load(&decoded, &reader);
+
+  ASSERT_EQ(original, decoded);
+}

tests/unit/slk_core.cpp

@@ -15,7 +15,7 @@
     ASSERT_EQ(original, decoded);                                            \
   }
 
-TEST(CustomSerialization, Primitive) {
+TEST(SlkCore, Primitive) {
   CREATE_PRIMITIVE_TEST(bool, true, false);
   CREATE_PRIMITIVE_TEST(int8_t, 0x12, 0);
   CREATE_PRIMITIVE_TEST(uint8_t, 0x12, 0);
@@ -29,7 +29,7 @@ TEST(CustomSerialization, Primitive) {
   CREATE_PRIMITIVE_TEST(double, 1234567890.1234567890, 0);
 }
 
-TEST(CustomSerialization, String) {
+TEST(SlkCore, String) {
   std::string original = "hello world";
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -40,7 +40,7 @@ TEST(CustomSerialization, String) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, VectorPrimitive) {
+TEST(SlkCore, VectorPrimitive) {
   std::vector<int> original{1, 2, 3, 4, 5};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -51,7 +51,7 @@ TEST(CustomSerialization, VectorPrimitive) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, VectorString) {
+TEST(SlkCore, VectorString) {
   std::vector<std::string> original{"hai hai hai", "nandare!"};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -66,7 +66,7 @@ TEST(CustomSerialization, VectorString) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, SetPrimitive) {
+TEST(SlkCore, SetPrimitive) {
   std::set<int> original{1, 2, 3, 4, 5};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -77,7 +77,7 @@ TEST(CustomSerialization, SetPrimitive) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, SetString) {
+TEST(SlkCore, SetString) {
   std::set<std::string> original{"hai hai hai", "nandare!"};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -92,7 +92,7 @@ TEST(CustomSerialization, SetString) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, MapPrimitive) {
+TEST(SlkCore, MapPrimitive) {
   std::map<int, int> original{{1, 2}, {3, 4}, {5, 6}};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -104,7 +104,7 @@ TEST(CustomSerialization, MapPrimitive) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, MapString) {
+TEST(SlkCore, MapString) {
   std::map<std::string, std::string> original{{"hai hai hai", "nandare!"}};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -120,7 +120,7 @@ TEST(CustomSerialization, MapString) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, UnorderedMapPrimitive) {
+TEST(SlkCore, UnorderedMapPrimitive) {
   std::unordered_map<int, int> original{{1, 2}, {3, 4}, {5, 6}};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -132,7 +132,7 @@ TEST(CustomSerialization, UnorderedMapPrimitive) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, UnorderedMapString) {
+TEST(SlkCore, UnorderedMapString) {
   std::unordered_map<std::string, std::string> original{
       {"hai hai hai", "nandare!"}};
   slk::Builder builder;
@@ -149,7 +149,7 @@ TEST(CustomSerialization, UnorderedMapString) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, UniquePtrEmpty) {
+TEST(SlkCore, UniquePtrEmpty) {
   std::unique_ptr<std::string> original;
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -162,7 +162,7 @@ TEST(CustomSerialization, UniquePtrEmpty) {
   ASSERT_EQ(decoded.get(), nullptr);
 }
 
-TEST(CustomSerialization, UniquePtrFull) {
+TEST(SlkCore, UniquePtrFull) {
   std::unique_ptr<std::string> original =
       std::make_unique<std::string>("nandare!");
   slk::Builder builder;
@@ -177,7 +177,7 @@ TEST(CustomSerialization, UniquePtrFull) {
   ASSERT_EQ(*original.get(), *decoded.get());
 }
 
-TEST(CustomSerialization, OptionalPrimitiveEmpty) {
+TEST(SlkCore, OptionalPrimitiveEmpty) {
   std::experimental::optional<int> original;
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -189,7 +189,7 @@ TEST(CustomSerialization, OptionalPrimitiveEmpty) {
   ASSERT_EQ(decoded, std::experimental::nullopt);
 }
 
-TEST(CustomSerialization, OptionalPrimitiveFull) {
+TEST(SlkCore, OptionalPrimitiveFull) {
   std::experimental::optional<int> original = 5;
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -202,7 +202,7 @@ TEST(CustomSerialization, OptionalPrimitiveFull) {
   ASSERT_EQ(*original, *decoded);
 }
 
-TEST(CustomSerialization, OptionalStringEmpty) {
+TEST(SlkCore, OptionalStringEmpty) {
   std::experimental::optional<std::string> original;
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -214,7 +214,7 @@ TEST(CustomSerialization, OptionalStringEmpty) {
   ASSERT_EQ(decoded, std::experimental::nullopt);
 }
 
-TEST(CustomSerialization, OptionalStringFull) {
+TEST(SlkCore, OptionalStringFull) {
   std::experimental::optional<std::string> original = "nandare!";
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -227,7 +227,7 @@ TEST(CustomSerialization, OptionalStringFull) {
   ASSERT_EQ(*original, *decoded);
 }
 
-TEST(CustomSerialization, Pair) {
+TEST(SlkCore, Pair) {
   std::pair<std::string, int> original{"nandare!", 5};
   slk::Builder builder;
   slk::Save(original, &builder);
@@ -239,7 +239,7 @@ TEST(CustomSerialization, Pair) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, SharedPtrEmpty) {
+TEST(SlkCore, SharedPtrEmpty) {
   std::shared_ptr<std::string> original;
   std::vector<std::string *> saved;
   slk::Builder builder;
@@ -256,7 +256,7 @@ TEST(CustomSerialization, SharedPtrEmpty) {
   ASSERT_EQ(loaded.size(), 0);
 }
 
-TEST(CustomSerialization, SharedPtrFull) {
+TEST(SlkCore, SharedPtrFull) {
   std::shared_ptr<std::string> original =
       std::make_shared<std::string>("nandare!");
   std::vector<std::string *> saved;
@@ -275,7 +275,7 @@ TEST(CustomSerialization, SharedPtrFull) {
   ASSERT_EQ(loaded.size(), 1);
 }
 
-TEST(CustomSerialization, SharedPtrMultiple) {
+TEST(SlkCore, SharedPtrMultiple) {
   std::shared_ptr<std::string> ptr1 = std::make_shared<std::string>("nandare!");
   std::shared_ptr<std::string> ptr2;
   std::shared_ptr<std::string> ptr3 =
@@ -328,7 +328,7 @@ TEST(CustomSerialization, SharedPtrMultiple) {
   ASSERT_EQ(dec5.get(), dec3.get());
 }
 
-TEST(CustomSerialization, Complex) {
+TEST(SlkCore, Complex) {
   std::unique_ptr<std::vector<std::experimental::optional<std::string>>>
       original = std::make_unique<
           std::vector<std::experimental::optional<std::string>>>();
@@ -378,7 +378,7 @@ void Load(Foo *obj, Reader *reader) {
 }
 }  // namespace slk
 
-TEST(CustomSerialization, VectorStruct) {
+TEST(SlkCore, VectorStruct) {
   std::vector<Foo> original;
   original.push_back({"hai hai hai", 5});
   original.push_back({"nandare!", std::experimental::nullopt});
@@ -399,7 +399,7 @@ TEST(CustomSerialization, VectorStruct) {
   ASSERT_EQ(original, decoded);
 }
 
-TEST(CustomSerialization, VectorSharedPtr) {
+TEST(SlkCore, VectorSharedPtr) {
   std::shared_ptr<std::string> ptr1 = std::make_shared<std::string>("nandare!");
   std::shared_ptr<std::string> ptr2;
   std::shared_ptr<std::string> ptr3 =
@@ -445,7 +445,7 @@ TEST(CustomSerialization, VectorSharedPtr) {
   ASSERT_EQ(decoded[4].get(), decoded[2].get());
 }
 
-TEST(CustomSerialization, OptionalSharedPtr) {
+TEST(SlkCore, OptionalSharedPtr) {
   std::experimental::optional<std::shared_ptr<std::string>> original =
       std::make_shared<std::string>("nandare!");
   std::vector<std::string *> saved;