fix: scoped KV, save KV to DB, boolean vals (#1661)

A previous PR, optimizing saves, ended up canceling out saves here. Also: • Fixed nested scopes (and added a unit test) • Added boolean as a possible type for the KV values • Simplified the logic inside KV by removing a lot of templates and made scopes lighter weight with a simpler hierarchy.
opentibiabr · Oct 3, 2023 · 3ef5307 · 3ef5307
1 parent 8179d21
commit 3ef5307
Show file tree

Hide file tree

Showing 19 changed files with 261 additions and 126 deletions.
diff --git a/CMakePresets.json b/CMakePresets.json
@@ -28,7 +28,7 @@
       },
       "vendor": {
         "microsoft.com/VisualStudioSettings/CMake/1.0": {
-          "hostOS": [ "Windows" ]
+          "hostOS": ["Windows"]
         }
       },
       "condition": {

diff --git a/src/creatures/players/player.hpp b/src/creatures/players/player.hpp
@@ -471,8 +471,8 @@ class Player final : public Creature, public Cylinder, public Bankable {
 	int32_t getStorageValueByName(const std::string &storageName) const;
 	void addStorageValueByName(const std::string &storageName, const int32_t value, const bool isLogin = false);
 
-	std::shared_ptr<KVStore> kv() const {
-		return g_kv().scoped("player")->scoped(getID());
+	std::shared_ptr<KV> kv() const {
+		return g_kv().scoped("player")->scoped(fmt::format("{}", getID()));
 	}
 
 	void genReservedStorageRange();

diff --git a/src/items/item.cpp b/src/items/item.cpp
@@ -281,7 +281,6 @@ void Item::onRemoved() {
 }
 
 void Item::setID(uint16_t newid) {
-	g_logger().debug("[Item::setID] - Setting item id from {} to {}", id, newid);
 	const ItemType &prevIt = Item::items[id];
 	id = newid;
 

diff --git a/src/kv/README.md b/src/kv/README.md
@@ -21,10 +21,10 @@ The Canary KV Library is designed to offer a simple, efficient, persistent, and
 #include <kv/kv.h>
 
 // In your class constructor
-MyClass(KVStore &kv) : kv(kv) {}
+MyClass(KV &kv) : kv(kv) {}
 
 // Or use the global singleton
-KVStore &kv = g_kv();
+KV &kv = g_kv();
 ```
 
 ### Basic Usage

diff --git a/src/kv/kv.cpp b/src/kv/kv.cpp
@@ -73,6 +73,11 @@ std::optional<ValueWrapper> KVStore::get(const std::string &key, bool forceLoad
 	return value;
 }
 
-void KVStore::remove(const std::string &key) {
+void KV::remove(const std::string &key) {
 	set(key, ValueWrapper::deleted());
 }
+
+std::shared_ptr<KV> KVStore::scoped(const std::string &scope) {
+	logger.debug("KVStore::scoped({})", scope);
+	return std::make_shared<ScopedKV>(logger, *this, scope);
+}
diff --git a/src/kv/kv.hpp b/src/kv/kv.hpp
@@ -18,43 +18,49 @@
 #include "lib/logging/logger.hpp"
 #include "kv/value_wrapper.hpp"
 
-class KVStore {
+class KV : public std::enable_shared_from_this<KV> {
 public:
-	static constexpr size_t MAX_SIZE = 10000;
-
-	static KVStore &getInstance();
+	virtual void set(const std::string &key, const std::initializer_list<ValueWrapper> &init_list) = 0;
+	virtual void set(const std::string &key, const std::initializer_list<std::pair<const std::string, ValueWrapper>> &init_list) = 0;
+	virtual void set(const std::string &key, const ValueWrapper &value) = 0;
 
-	explicit KVStore(Logger &logger) :
-		logger(logger) { }
-	virtual ~KVStore() = default;
+	virtual std::optional<ValueWrapper> get(const std::string &key, bool forceLoad = false) = 0;
 
-	template <typename T>
-	void set(const std::string &key, const std::vector<T> &vec);
-	virtual void set(const std::string &key, const std::initializer_list<ValueWrapper> &init_list);
-	virtual void set(const std::string &key, const std::initializer_list<std::pair<const std::string, ValueWrapper>> &init_list);
-	virtual void set(const std::string &key, const ValueWrapper &value);
+	virtual bool saveAll() {
+		return true;
+	}
 
-	virtual std::optional<ValueWrapper> get(const std::string &key, bool forceLoad = false);
+	virtual std::shared_ptr<KV> scoped(const std::string &scope) = 0;
 
 	void remove(const std::string &key);
 
-	template <typename T>
-	T get(const std::string &key, bool forceLoad = false);
-
-	virtual bool saveAll() {
-		return true;
+	virtual void flush() {
+		saveAll();
 	}
+};
 
-	template <typename T>
-	std::shared_ptr<KVStore> scoped(const T &scope);
+class KVStore : public KV {
+public:
+	static constexpr size_t MAX_SIZE = 10000;
+	static KVStore &getInstance();
+
+	explicit KVStore(Logger &logger) :
+		logger(logger) { }
 
-	friend class ScopedKV;
+	void set(const std::string &key, const std::initializer_list<ValueWrapper> &init_list) override;
+	void set(const std::string &key, const std::initializer_list<std::pair<const std::string, ValueWrapper>> &init_list) override;
+	void set(const std::string &key, const ValueWrapper &value) override;
 
-	void flush() {
-		saveAll();
+	std::optional<ValueWrapper> get(const std::string &key, bool forceLoad = false) override;
+
+	void flush() override {
+		std::scoped_lock lock(mutex_);
+		KV::flush();
 		store_.clear();
 	}
 
+	std::shared_ptr<KV> scoped(const std::string &scope) override final;
+
 protected:
 	phmap::parallel_flat_hash_map<std::string, std::pair<ValueWrapper, std::list<std::string>::iterator>> getStore() {
 		std::scoped_lock lock(mutex_);
@@ -64,11 +70,13 @@ class KVStore {
 		}
 		return copy;
 	}
-	virtual std::optional<ValueWrapper> load(const std::string &key) = 0;
-	virtual bool save(const std::string &key, const ValueWrapper &value) = 0;
 
+protected:
 	Logger &logger;
 
+	virtual std::optional<ValueWrapper> load(const std::string &key) = 0;
+	virtual bool save(const std::string &key, const ValueWrapper &value) = 0;
+
 private:
 	void setLocked(const std::string &key, const ValueWrapper &value);
 
@@ -77,42 +85,23 @@ class KVStore {
 	std::mutex mutex_;
 };
 
-template <typename T>
-void KVStore::set(const std::string &key, const std::vector<T> &vec) {
-	ValueWrapper wrapped(vec);
-	set(key, wrapped);
-}
-
-template <typename T>
-T KVStore::get(const std::string &key, bool forceLoad /*= false */) {
-	auto optValue = get(key, forceLoad);
-	if (optValue.has_value()) {
-		return optValue->get<T>();
-	}
-	return T {};
-}
-
-class ScopedKV final : public KVStore {
+class ScopedKV final : public KV {
 public:
-	ScopedKV(KVStore &parentKV, const std::string &prefix) :
-		KVStore(parentKV.logger), parentKV_(parentKV), prefix_(prefix) { }
+	ScopedKV(Logger &logger, KVStore &rootKV, const std::string &prefix) :
+		logger(logger), rootKV_(rootKV), prefix_(prefix) { }
 
-	template <typename T>
-	void set(const std::string &key, const std::vector<T> &vec) {
-		parentKV_.set(buildKey(key), vec);
-	}
 	void set(const std::string &key, const std::initializer_list<ValueWrapper> &init_list) override {
-		parentKV_.set(buildKey(key), init_list);
+		rootKV_.set(buildKey(key), init_list);
 	}
 	void set(const std::string &key, const std::initializer_list<std::pair<const std::string, ValueWrapper>> &init_list) override {
-		parentKV_.set(buildKey(key), init_list);
+		rootKV_.set(buildKey(key), init_list);
 	}
 	void set(const std::string &key, const ValueWrapper &value) override {
-		parentKV_.set(buildKey(key), value);
+		rootKV_.set(buildKey(key), value);
 	}
 
 	std::optional<ValueWrapper> get(const std::string &key, bool forceLoad = false) override {
-		return parentKV_.get(buildKey(key), forceLoad);
+		return rootKV_.get(buildKey(key), forceLoad);
 	}
 
 	template <typename T>
@@ -125,29 +114,22 @@ class ScopedKV final : public KVStore {
 	}
 
 	bool saveAll() override {
-		return parentKV_.saveAll();
+		return rootKV_.saveAll();
 	}
 
-protected:
-	std::optional<ValueWrapper> load(const std::string &key) override {
-		return parentKV_.load(buildKey(key));
-	}
-	bool save(const std::string &key, const ValueWrapper &value) override {
-		return parentKV_.save(buildKey(key), value);
+	std::shared_ptr<KV> scoped(const std::string &scope) override final {
+		logger.debug("ScopedKV::scoped({})", buildKey(scope));
+		return std::make_shared<ScopedKV>(logger, rootKV_, buildKey(scope));
 	}
 
 private:
 	std::string buildKey(const std::string &key) const {
-		return prefix_ + "." + key;
+		return fmt::format("{}.{}", prefix_, key);
 	}
 
-	KVStore &parentKV_;
+	Logger &logger;
+	KVStore &rootKV_;
 	std::string prefix_;
 };
 
-template <typename T>
-std::shared_ptr<KVStore> KVStore::scoped(const T &scope) {
-	return std::make_shared<ScopedKV>(*this, fmt::format("{}", scope));
-}
-
 constexpr auto g_kv = KVStore::getInstance;
diff --git a/src/kv/kv_sql.cpp b/src/kv/kv_sql.cpp
@@ -58,19 +58,20 @@ bool KVSQL::prepareSave(const std::string &key, const ValueWrapper &value, DBIns
 		return db.executeQuery(query);
 	}
 
-	update.upsert({ "timestamp", "value" });
-	update.addRow(fmt::format("({}, {}, {})", db.escapeString(key), getTimeMsNow(), db.escapeString(data)));
+	update.addRow(fmt::format("{}, {}, {}", db.escapeString(key), getTimeMsNow(), db.escapeString(data)));
 	return true;
 }
 
 bool KVSQL::saveAll() {
 	auto store = getStore();
 	bool success = DBTransaction::executeWithinTransaction([this, &store]() {
 		auto update = dbUpdate();
-		return std::ranges::all_of(store, [this, &update](const auto &kv) {
-			const auto &[key, value] = kv;
-			return prepareSave(key, value.first, update);
-		});
+		if (!std::ranges::all_of(store, [this, &update](const auto &kv) {
+				const auto &[key, value] = kv;
+				return prepareSave(key, value.first, update);
+			})) {
+			return false;
+		}
 		return update.execute();
 	});
 

diff --git a/src/kv/kv_sql.hpp b/src/kv/kv_sql.hpp
@@ -29,8 +29,10 @@ class KVSQL final : public KVStore {
 	bool save(const std::string &key, const ValueWrapper &value) override;
 	bool prepareSave(const std::string &key, const ValueWrapper &value, DBInsert &update);
 
-	DBInsert dbUpdate() const {
-		return DBInsert("INSERT INTO `kv_store` (`key_name`, `timestamp`, `value`) VALUES");
+	DBInsert dbUpdate() {
+		auto insert = DBInsert("INSERT INTO `kv_store` (`key_name`, `timestamp`, `value`) VALUES");
+		insert.upsert({ "key_name", "timestamp", "value" });
+		return insert;
 	}
 
 	Database &db;

diff --git a/src/kv/value_wrapper.cpp b/src/kv/value_wrapper.cpp
@@ -9,6 +9,9 @@ ValueWrapper::ValueWrapper(const ValueVariant &value, uint64_t timestamp) :
 ValueWrapper::ValueWrapper(const std::string &value, uint64_t timestamp) :
 	data_(value), timestamp_(timestamp) { }
 
+ValueWrapper::ValueWrapper(bool value, uint64_t timestamp) :
+	data_(value), timestamp_(timestamp) { }
+
 ValueWrapper::ValueWrapper(int value, uint64_t timestamp) :
 	data_(value), timestamp_(timestamp) { }
 

diff --git a/src/kv/value_wrapper.hpp b/src/kv/value_wrapper.hpp
@@ -22,18 +22,20 @@
 class ValueWrapper;
 
 using StringType = std::string;
+using BooleanType = bool;
 using IntType = int;
 using DoubleType = double;
 using ArrayType = std::vector<ValueWrapper>;
 using MapType = phmap::flat_hash_map<std::string, std::shared_ptr<ValueWrapper>>;
 
-using ValueVariant = std::variant<StringType, IntType, DoubleType, ArrayType, MapType>;
+using ValueVariant = std::variant<StringType, BooleanType, IntType, DoubleType, ArrayType, MapType>;
 
 class ValueWrapper {
 public:
 	explicit ValueWrapper(uint64_t timestamp = 0);
 	explicit(false) ValueWrapper(const ValueVariant &value, uint64_t timestamp = 0);
 	explicit(false) ValueWrapper(const std::string &value, uint64_t timestamp = 0);
+	explicit(false) ValueWrapper(bool value, uint64_t timestamp = 0);
 	explicit(false) ValueWrapper(int value, uint64_t timestamp = 0);
 	explicit(false) ValueWrapper(double value, uint64_t timestamp = 0);
 	explicit(false) ValueWrapper(const phmap::flat_hash_map<std::string, ValueWrapper> &value, uint64_t timestamp = 0);
@@ -85,6 +87,10 @@ class ValueWrapper {
 		return get<StringType>();
 	}
 
+	explicit(false) operator bool() const {
+		return get<BooleanType>();
+	}
+
 	explicit(false) operator int() const {
 		return get<IntType>();
 	}

diff --git a/src/kv/value_wrapper_proto.hpp b/src/kv/value_wrapper_proto.hpp
@@ -31,6 +31,10 @@ namespace ProtoHelpers {
 		protoValue.set_str_value(arg);
 	}
 
+	void setProtoBooleanValue(Canary::protobuf::kv::ValueWrapper &protoValue, const BooleanType &arg) {
+		protoValue.set_bool_value(arg);
+	}
+
 	void setProtoIntValue(Canary::protobuf::kv::ValueWrapper &protoValue, const IntType &arg) {
 		protoValue.set_int_value(arg);
 	}
@@ -64,6 +68,8 @@ inline Canary::protobuf::kv::ValueWrapper ProtoSerializable<ValueWrapper>::toPro
 			using T = std::decay_t<decltype(arg)>;
 			if constexpr (std::is_same_v<T, StringType>) {
 				ProtoHelpers::setProtoStringValue(protoValue, arg);
+			} else if constexpr (std::is_same_v<T, BooleanType>) {
+				ProtoHelpers::setProtoBooleanValue(protoValue, arg);
 			} else if constexpr (std::is_same_v<T, IntType>) {
 				ProtoHelpers::setProtoIntValue(protoValue, arg);
 			} else if constexpr (std::is_same_v<T, DoubleType>) {
@@ -86,6 +92,9 @@ inline ValueWrapper ProtoSerializable<ValueWrapper>::fromProto(const Canary::pro
 		case Canary::protobuf::kv::ValueWrapper::kStrValue:
 			data = protoValue.str_value();
 			break;
+		case Canary::protobuf::kv::ValueWrapper::kBoolValue:
+			data = protoValue.bool_value();
+			break;
 		case Canary::protobuf::kv::ValueWrapper::kIntValue:
 			data = protoValue.int_value();
 			break;