#447 LMDB Exceptions Normalization (#1285)

Implementation of lmdb storage specific exceptions. This PR is a refactor of #584 which was not merged because it was too complex. This PR also implements a framework for testing storage exceptions. `GenericStorageTest` class allows testing of common exceptions which are triggered among all the storages. Some examples of these exceptions are `KeyNotFoundException` if you read an in-existent key, `DuplicateKeyException` if you attempt to overwrite a key that already exists. `lmdb::map_full_error` is an example of exception specific to LMDB which is triggered when the remaining map size is not enough to write new keys/values.
man-group · Feb 2, 2024 · 6f741d2 · 6f741d2
1 parent 7bab716
commit 6f741d2
Show file tree

Hide file tree

Showing 4 changed files with 181 additions and 3 deletions.
diff --git a/cpp/arcticdb/CMakeLists.txt b/cpp/arcticdb/CMakeLists.txt
@@ -792,6 +792,7 @@ if(${TEST})
             version/test/test_version_store.cpp
             version/test/test_sorting_info_state_machine.cpp
             version/test/version_map_model.hpp
+            storage/test/test_storage_exceptions.cpp
             )
 
     set(EXECUTABLE_PERMS OWNER_WRITE OWNER_READ OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) # 755

diff --git a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
@@ -92,7 +92,8 @@ void LmdbStorage::do_update(Composite<KeySegmentPair>&& kvs, UpdateOpts opts) {
     if(!failed_deletes.empty()) {
         ARCTICDB_SUBSAMPLE(LmdbStorageCommit, 0)
         txn.commit();
-        throw KeyNotFoundException(Composite<VariantKey>(std::move(failed_deletes)));
+        std::string err_message = fmt::format("do_update called with upsert=false on non-existent key(s): {}", failed_deletes);
+        throw KeyNotFoundException(Composite<VariantKey>(std::move(failed_deletes)), err_message);
     }
     do_write_internal(std::move(kvs), txn);
     ARCTICDB_SUBSAMPLE(LmdbStorageCommit, 0)

diff --git a/cpp/arcticdb/storage/storage.hpp b/cpp/arcticdb/storage/storage.hpp
@@ -61,13 +61,18 @@ class NoDataFoundException : public ArcticCategorizedException<ErrorCategory::MI
     std::optional<VariantId> key_;
 };
 
-class KeyNotFoundException : public ArcticSpecificException<ErrorCode::E_DUPLICATE_KEY> {
+class KeyNotFoundException : public ArcticSpecificException<ErrorCode::E_KEY_NOT_FOUND> {
 public:
     explicit KeyNotFoundException(Composite<VariantKey>&& keys) :
-        ArcticSpecificException<ErrorCode::E_DUPLICATE_KEY>(fmt::format("{}", keys)),
+        ArcticSpecificException<ErrorCode::E_KEY_NOT_FOUND>(fmt::format("Not found: {}", keys)),
         keys_(std::make_shared<Composite<VariantKey>>(std::move(keys))) {
     }
 
+    explicit KeyNotFoundException(Composite<VariantKey>&& keys, std::string err_output) :
+            ArcticSpecificException<ErrorCode::E_KEY_NOT_FOUND>(err_output),
+            keys_(std::make_shared<Composite<VariantKey>>(std::move(keys))) {
+    }
+
     Composite<VariantKey>& keys() {
         return *keys_;
     }

diff --git a/cpp/arcticdb/storage/test/test_storage_exceptions.cpp b/cpp/arcticdb/storage/test/test_storage_exceptions.cpp
@@ -0,0 +1,171 @@
+/* Copyright 2023 Man Group Operations Limited
+ *
+ * Use of this software is governed by the Business Source License 1.1 included in the file licenses/BSL.txt.
+ *
+ * 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.
+ */
+
+#include <gtest/gtest.h>
+
+#include <arcticdb/codec/codec.hpp>
+#include <arcticdb/storage/storage.hpp>
+#include <arcticdb/storage/lmdb/lmdb_storage.hpp>
+#include <arcticdb/util/buffer.hpp>
+
+#include <filesystem>
+#include <memory>
+
+inline const fs::path TEST_DATABASES_PATH = "./test_databases";
+
+class StorageFactory {
+public:
+    virtual ~StorageFactory() = default;
+    virtual std::unique_ptr<arcticdb::storage::Storage> create() = 0;
+
+    virtual void setup() { }
+    virtual void clear_setup() { }
+};
+
+class LMDBStorageFactory : public StorageFactory {
+private:
+    uint64_t map_size;
+
+public:
+    LMDBStorageFactory() : map_size(128ULL * (1ULL << 20) /* 128MB */) { }
+
+    explicit LMDBStorageFactory(uint64_t map_size) : map_size(map_size) { }
+
+    std::unique_ptr<arcticdb::storage::Storage> create() override {
+        arcticdb::proto::lmdb_storage::Config cfg;
+
+        fs::path db_name = "test_lmdb";
+        cfg.set_path((TEST_DATABASES_PATH / db_name).generic_string());
+        cfg.set_map_size(map_size);
+        cfg.set_recreate_if_exists(true);
+
+        arcticdb::storage::LibraryPath library_path{"a", "b"};
+
+        return std::make_unique<arcticdb::storage::lmdb::LmdbStorage>(library_path, arcticdb::storage::OpenMode::WRITE, cfg);
+    }
+
+    void setup() override {
+        if (!fs::exists(TEST_DATABASES_PATH)) {
+            fs::create_directories(TEST_DATABASES_PATH);
+        }
+    }
+
+    void clear_setup() override {
+        if (fs::exists(TEST_DATABASES_PATH)) {
+            fs::remove_all(TEST_DATABASES_PATH);
+        }
+    }
+};
+
+// Generic tests that run with all types of storages
+
+class GenericStorageTest : public ::testing::TestWithParam<std::shared_ptr<StorageFactory>> {
+protected:
+    std::unique_ptr<arcticdb::storage::Storage> storage;
+
+    void SetUp() override {
+        GetParam()->setup();
+        storage = GetParam()->create();
+    }
+
+    void TearDown() override {
+        storage.reset();
+        GetParam()->clear_setup();
+    }
+};
+
+TEST_P(GenericStorageTest, WriteDuplicateKeyException) {
+    arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build<arcticdb::entity::KeyType::VERSION>("sym");
+
+    arcticdb::storage::KeySegmentPair kv(k);
+    kv.segment().set_buffer(std::make_shared<arcticdb::Buffer>());
+
+    storage->write(std::move(kv));
+
+    ASSERT_TRUE(storage->key_exists(k));
+
+    arcticdb::storage::KeySegmentPair kv1(k);
+    kv1.segment().set_buffer(std::make_shared<arcticdb::Buffer>());
+
+    ASSERT_THROW({
+        storage->write(std::move(kv1));
+    },  arcticdb::storage::DuplicateKeyException);
+
+}
+
+TEST_P(GenericStorageTest, ReadKeyNotFoundException) {
+    arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build<arcticdb::entity::KeyType::VERSION>("sym");
+
+    ASSERT_TRUE(!storage->key_exists(k));
+    ASSERT_THROW({
+        storage->read(k, arcticdb::storage::ReadKeyOpts{});
+    },  arcticdb::storage::KeyNotFoundException);
+
+}
+
+TEST_P(GenericStorageTest, UpdateKeyNotFoundException) {
+    arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build<arcticdb::entity::KeyType::VERSION>("sym");
+
+    arcticdb::storage::KeySegmentPair kv(k);
+    kv.segment().header().set_start_ts(1234);
+    kv.segment().set_buffer(std::make_shared<arcticdb::Buffer>());
+
+    ASSERT_TRUE(!storage->key_exists(k));
+    ASSERT_THROW({
+        storage->update(std::move(kv), arcticdb::storage::UpdateOpts{});
+    },  arcticdb::storage::KeyNotFoundException);
+
+}
+
+TEST_P(GenericStorageTest, RemoveKeyNotFoundException) {
+    arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build<arcticdb::entity::KeyType::VERSION>("sym");
+
+    ASSERT_TRUE(!storage->key_exists(k));
+    ASSERT_THROW({
+        storage->remove(k, arcticdb::storage::RemoveOpts{});
+    },  arcticdb::storage::KeyNotFoundException);
+
+}
+
+INSTANTIATE_TEST_SUITE_P(
+        AllStoragesCommonTests,
+        GenericStorageTest,
+        ::testing::Values(std::make_shared<LMDBStorageFactory>())
+);
+
+// LMDB Storage specific tests
+
+class LMDBStorageTestBase : public ::testing::Test {
+protected:
+    void SetUp() override {
+        if (!fs::exists(TEST_DATABASES_PATH)) {
+            fs::create_directories(TEST_DATABASES_PATH);
+        }
+    }
+
+    void TearDown() override {
+        if (fs::exists(TEST_DATABASES_PATH)) {
+            fs::remove_all(TEST_DATABASES_PATH);
+        }
+    }
+};
+
+TEST_F(LMDBStorageTestBase, WriteMapFullError) {
+    // Create a Storage with 32KB map size
+    LMDBStorageFactory factory(32ULL * (1ULL << 10));
+    auto storage = factory.create();
+
+    arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build<arcticdb::entity::KeyType::VERSION>("sym");
+    arcticdb::storage::KeySegmentPair kv(k);
+    kv.segment().header().set_start_ts(1234);
+    kv.segment().set_buffer(std::make_shared<arcticdb::Buffer>(40000));
+
+    ASSERT_THROW({
+        storage->write(std::move(kv));
+    },  ::lmdb::map_full_error);
+
+}