From 6d0815915c72673b14a98563fa1e3b1da36d3ec4 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 7 Dec 2014 00:58:56 +0100 Subject: [PATCH] Redefined error behavior in blob loading when blob key doesn't exist --- .../inmemory/InMemoryBlobStore.cpp | 10 +++++----- .../implementations/inmemory/InMemoryBlobStore.h | 1 - .../implementations/ondisk/OnDiskBlob.cpp | 16 +++++++++++++--- .../implementations/ondisk/OnDiskBlobStore.cpp | 5 ----- .../implementations/ondisk/OnDiskBlobStore.h | 1 - src/blobstore/interface/BlobStore.h | 1 - .../ondisk/OnDiskBlobTest/OnDiskBlobLoadTest.cpp | 5 ++--- src/test/blobstore/testutils/BlobStoreTest.h | 15 ++++++--------- 8 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp b/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp index 30bbcb59..2a16ba80 100644 --- a/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp +++ b/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp @@ -25,13 +25,13 @@ unique_ptr InMemoryBlobStore::create(const std::string &key, size_t return make_unique(key, make_unique(insert_result.first->second)); } -bool InMemoryBlobStore::exists(const std::string &key) { - return _blobs.count(key) > 0; -} - unique_ptr InMemoryBlobStore::load(const string &key) { //Return a copy of the stored InMemoryBlob - return make_unique(_blobs.at(key)); + try { + return make_unique(_blobs.at(key)); + } catch (const std::out_of_range &e) { + return nullptr; + } } } /* namespace ondisk */ diff --git a/src/blobstore/implementations/inmemory/InMemoryBlobStore.h b/src/blobstore/implementations/inmemory/InMemoryBlobStore.h index 1f67a609..84a6bc6d 100644 --- a/src/blobstore/implementations/inmemory/InMemoryBlobStore.h +++ b/src/blobstore/implementations/inmemory/InMemoryBlobStore.h @@ -18,7 +18,6 @@ public: InMemoryBlobStore(); std::unique_ptr create(const std::string &key, size_t size) override; - bool exists(const std::string &key) override; std::unique_ptr load(const std::string &key) override; private: diff --git a/src/blobstore/implementations/ondisk/OnDiskBlob.cpp b/src/blobstore/implementations/ondisk/OnDiskBlob.cpp index 972a89e6..a96f6f41 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlob.cpp +++ b/src/blobstore/implementations/ondisk/OnDiskBlob.cpp @@ -2,6 +2,7 @@ #include "OnDiskBlobStore.h" #include "blobstore/implementations/ondisk/FileAlreadyExistsException.h" +#include "blobstore/utils/FileDoesntExistException.h" #include #include @@ -45,9 +46,18 @@ size_t OnDiskBlob::size() const { } unique_ptr OnDiskBlob::LoadFromDisk(const bf::path &filepath) { - Data data = Data::LoadFromFile(filepath); - - return unique_ptr(new OnDiskBlob(filepath, std::move(data))); + try { + //If it isn't a file, Data::LoadFromFile() would usually also crash. We still need this extra check + //upfront, because Data::LoadFromFile() doesn't crash if we give it the path of a directory + //instead the path of a file. + if(!bf::is_regular_file(filepath)) { + return nullptr; + } + Data data = Data::LoadFromFile(filepath); + return unique_ptr(new OnDiskBlob(filepath, std::move(data))); + } catch (const FileDoesntExistException &e) { + return nullptr; + } } unique_ptr OnDiskBlob::CreateOnDisk(const bf::path &filepath, size_t size) { diff --git a/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp b/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp index c645e175..8ad72328 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp +++ b/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp @@ -27,11 +27,6 @@ unique_ptr OnDiskBlobStore::create(const std::string &key, size_t s return make_unique(key, std::move(blob)); } -bool OnDiskBlobStore::exists(const std::string &key) { - auto file_path = _rootdir / key; - return bf::exists(file_path); -} - unique_ptr OnDiskBlobStore::load(const string &key) { auto file_path = _rootdir / key; return OnDiskBlob::LoadFromDisk(file_path); diff --git a/src/blobstore/implementations/ondisk/OnDiskBlobStore.h b/src/blobstore/implementations/ondisk/OnDiskBlobStore.h index 89fb7b77..942ca6a4 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlobStore.h +++ b/src/blobstore/implementations/ondisk/OnDiskBlobStore.h @@ -17,7 +17,6 @@ class OnDiskBlobStore: public BlobStoreWithRandomKeys { public: OnDiskBlobStore(const boost::filesystem::path &rootdir); - bool exists(const std::string &key) override; std::unique_ptr create(const std::string &key, size_t size) override; std::unique_ptr load(const std::string &key) override; diff --git a/src/blobstore/interface/BlobStore.h b/src/blobstore/interface/BlobStore.h index 3bce26b0..7c0c33ff 100644 --- a/src/blobstore/interface/BlobStore.h +++ b/src/blobstore/interface/BlobStore.h @@ -19,7 +19,6 @@ public: virtual BlobWithKey create(size_t size) = 0; // Return nullptr if blob with this key doesn't exists virtual std::unique_ptr load(const std::string &key) = 0; - virtual bool exists(const std::string &key) = 0; //TODO Needed for performance? Or is deleting loaded blobs enough? //virtual void remove(const std::string &key) = 0; }; diff --git a/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobLoadTest.cpp b/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobLoadTest.cpp index 54f27862..e9c375fe 100644 --- a/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobLoadTest.cpp +++ b/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobLoadTest.cpp @@ -68,8 +68,7 @@ TEST_P(OnDiskBlobLoadTest, LoadedDataIsCorrect) { TEST_F(OnDiskBlobLoadTest, LoadNotExistingBlob) { TempFile file2(false); // Pass false, so the file isn't created. - EXPECT_THROW( - OnDiskBlob::LoadFromDisk(file2.path()), - FileDoesntExistException + EXPECT_FALSE( + (bool)OnDiskBlob::LoadFromDisk(file2.path()) ); } diff --git a/src/test/blobstore/testutils/BlobStoreTest.h b/src/test/blobstore/testutils/BlobStoreTest.h index ff689b11..f683fce2 100644 --- a/src/test/blobstore/testutils/BlobStoreTest.h +++ b/src/test/blobstore/testutils/BlobStoreTest.h @@ -109,23 +109,20 @@ public: } void TestLoadNonExistingBlobWithDefinitelyValidKey() { - //TODO Specify loading error behavior more precise and test for concrete exception (or whatever behavior we choose) - EXPECT_ANY_THROW( - blobStore->load(blobstore::RandomKeyGenerator::singleton().create()); + EXPECT_FALSE( + (bool)blobStore->load(blobstore::RandomKeyGenerator::singleton().create()) ); } void TestLoadNonExistingBlobWithMaybeInvalidKey() { - //TODO Specify loading error behavior more precise and test for concrete exception (or whatever behavior we choose) - EXPECT_ANY_THROW( - blobStore->load("not-existing-key"); + EXPECT_FALSE( + (bool)blobStore->load("not-existing-key") ); } void TestLoadNonExistingBlobWithEmptyKey() { - //TODO Specify loading error behavior more precise and test for concrete exception (or whatever behavior we choose) - EXPECT_ANY_THROW( - blobStore->load(""); + EXPECT_FALSE( + (bool)blobStore->load("") ); }