From 2c354e82f326ad57117d077756f49c2e1747ec1d Mon Sep 17 00:00:00 2001 From: Matej Ferencevic Date: Thu, 8 Nov 2018 13:27:52 +0100 Subject: [PATCH] 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 | 15 ++- src/storage/common/types/property_value.cpp | 116 +++++++++++------- src/storage/common/types/property_value.hpp | 7 +- .../common/types/property_value.slk.hpp | 109 ++++++++++++++++ tests/unit/CMakeLists.txt | 11 +- tests/unit/slk_advanced.cpp | 77 ++++++++++++ ...{custom_serialization.cpp => slk_core.cpp} | 48 ++++---- 7 files changed, 303 insertions(+), 80 deletions(-) create mode 100644 src/storage/common/types/property_value.slk.hpp create mode 100644 tests/unit/slk_advanced.cpp rename tests/unit/{custom_serialization.cpp => slk_core.cpp} (93%) diff --git a/src/communication/rpc/serialization.hpp b/src/communication/rpc/serialization.hpp index 5fb1edd1e..9704c89b7 100644 --- a/src/communication/rpc/serialization.hpp +++ b/src/communication/rpc/serialization.hpp @@ -16,6 +16,7 @@ #include #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 || "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 *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; diff --git a/src/storage/common/types/property_value.cpp b/src/storage/common/types/property_value.cpp index c1b90f419..2c28fc6b9 100644 --- a/src/storage/common/types/property_value.cpp +++ b/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() ? "true" : "false"); - case PropertyValue::Type::String: - return os << value.Value(); - case PropertyValue::Type::Int: - return os << value.Value(); - case PropertyValue::Type::Double: - return os << value.Value(); - case PropertyValue::Type::List: - os << "["; - for (const auto &x : value.Value>()) { - os << x << ","; - } - return os << "]"; - case PropertyValue::Type::Map: - os << "{"; - for (const auto &kv : - value.Value>()) { - 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() ? "true" : "false"); + case PropertyValue::Type::String: + return os << value.Value(); + case PropertyValue::Type::Int: + return os << value.Value(); + case PropertyValue::Type::Double: + return os << value.Value(); + case PropertyValue::Type::List: + os << "["; + for (const auto &x : value.Value>()) { + os << x << ","; + } + return os << "]"; + case PropertyValue::Type::Map: + os << "{"; + for (const auto &kv : + value.Value>()) { + 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() == second.Value(); + case PropertyValue::Type::Int: + return first.Value() == second.Value(); + case PropertyValue::Type::Double: + return first.Value() == second.Value(); + case PropertyValue::Type::String: + return first.Value() == second.Value(); + case PropertyValue::Type::List: + return first.Value>() == + second.Value>(); + case PropertyValue::Type::Map: + return first.Value>() == + second.Value>(); + } +} diff --git a/src/storage/common/types/property_value.hpp b/src/storage/common/types/property_value.hpp index 51e412eb5..d34114eae 100644 --- a/src/storage/common/types/property_value.hpp +++ b/src/storage/common/types/property_value.hpp @@ -91,9 +91,6 @@ class PropertyValue { template 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); diff --git a/src/storage/common/types/property_value.slk.hpp b/src/storage/common/types/property_value.slk.hpp new file mode 100644 index 000000000..11b3c3547 --- /dev/null +++ b/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(), builder); + return; + } + case PropertyValue::Type::Int: { + uint8_t type = 2; + Save(type, builder); + Save(obj.Value(), builder); + return; + } + case PropertyValue::Type::Double: { + uint8_t type = 3; + Save(type, builder); + Save(obj.Value(), builder); + return; + } + case PropertyValue::Type::String: { + uint8_t type = 4; + Save(type, builder); + Save(obj.Value(), builder); + return; + } + case PropertyValue::Type::List: { + uint8_t type = 5; + Save(type, builder); + Save(obj.Value>(), builder); + return; + } + case PropertyValue::Type::Map: { + uint8_t type = 6; + Save(type, builder); + Save(obj.Value>(), 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 value; + Load(&value, reader); + *obj = PropertyValue(std::move(value)); + return; + } + // Map + case 6: { + std::map value; + Load(&value, reader); + *obj = PropertyValue(std::move(value)); + return; + } + // Invalid type + default: { throw SlkDecodeException("Couldn't load property value!"); } + } +} +} // namespace slk diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 325ac65bf..a9d7ecbd2 100644 --- a/tests/unit/CMakeLists.txt +++ b/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) diff --git a/tests/unit/slk_advanced.cpp b/tests/unit/slk_advanced.cpp new file mode 100644 index 000000000..18daf74ed --- /dev/null +++ b/tests/unit/slk_advanced.cpp @@ -0,0 +1,77 @@ +#include + +#include "storage/common/types/property_value.slk.hpp" + +TEST(SlkAdvanced, PropertyValueList) { + std::vector 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 decoded; + slk::Reader reader(builder.data(), builder.size()); + slk::Load(&decoded, &reader); + + ASSERT_EQ(original, decoded); +} + +TEST(SlkAdvanced, PropertyValueMap) { + std::map 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 decoded; + slk::Reader reader(builder.data(), builder.size()); + slk::Load(&decoded, &reader); + + ASSERT_EQ(original, decoded); +} + +TEST(SlkAdvanced, PropertyValueComplex) { + std::vector 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 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); +} diff --git a/tests/unit/custom_serialization.cpp b/tests/unit/slk_core.cpp similarity index 93% rename from tests/unit/custom_serialization.cpp rename to tests/unit/slk_core.cpp index 0508ef615..1f765acb0 100644 --- a/tests/unit/custom_serialization.cpp +++ b/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 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 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 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 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 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 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 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 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 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 original = std::make_unique("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 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 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 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 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 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 original; std::vector 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 original = std::make_shared("nandare!"); std::vector saved; @@ -275,7 +275,7 @@ TEST(CustomSerialization, SharedPtrFull) { ASSERT_EQ(loaded.size(), 1); } -TEST(CustomSerialization, SharedPtrMultiple) { +TEST(SlkCore, SharedPtrMultiple) { std::shared_ptr ptr1 = std::make_shared("nandare!"); std::shared_ptr ptr2; std::shared_ptr ptr3 = @@ -328,7 +328,7 @@ TEST(CustomSerialization, SharedPtrMultiple) { ASSERT_EQ(dec5.get(), dec3.get()); } -TEST(CustomSerialization, Complex) { +TEST(SlkCore, Complex) { std::unique_ptr>> original = std::make_unique< std::vector>>(); @@ -378,7 +378,7 @@ void Load(Foo *obj, Reader *reader) { } } // namespace slk -TEST(CustomSerialization, VectorStruct) { +TEST(SlkCore, VectorStruct) { std::vector 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 ptr1 = std::make_shared("nandare!"); std::shared_ptr ptr2; std::shared_ptr 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> original = std::make_shared("nandare!"); std::vector saved;