diff --git a/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp b/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp index 736d727d..30bbcb59 100644 --- a/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp +++ b/src/blobstore/implementations/inmemory/InMemoryBlobStore.cpp @@ -14,12 +14,15 @@ namespace inmemory { InMemoryBlobStore::InMemoryBlobStore() : _blobs() {} -BlobWithKey InMemoryBlobStore::create(const std::string &key, size_t size) { - InMemoryBlob blob(size); - _blobs.insert(make_pair(key, blob)); +unique_ptr InMemoryBlobStore::create(const std::string &key, size_t size) { + auto insert_result = _blobs.emplace(key, size); + + if (!insert_result.second) { + return nullptr; + } //Return a copy of the stored InMemoryBlob - return BlobWithKey(key, make_unique(blob)); + return make_unique(key, make_unique(insert_result.first->second)); } bool InMemoryBlobStore::exists(const std::string &key) { diff --git a/src/blobstore/implementations/inmemory/InMemoryBlobStore.h b/src/blobstore/implementations/inmemory/InMemoryBlobStore.h index e446de04..1f67a609 100644 --- a/src/blobstore/implementations/inmemory/InMemoryBlobStore.h +++ b/src/blobstore/implementations/inmemory/InMemoryBlobStore.h @@ -17,12 +17,10 @@ class InMemoryBlobStore: public BlobStoreWithRandomKeys { 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; -protected: - BlobWithKey create(const std::string &key, size_t size) override; - private: std::map _blobs; diff --git a/src/blobstore/implementations/ondisk/OnDiskBlob.cpp b/src/blobstore/implementations/ondisk/OnDiskBlob.cpp index 6d2238b9..972a89e6 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlob.cpp +++ b/src/blobstore/implementations/ondisk/OnDiskBlob.cpp @@ -51,19 +51,16 @@ unique_ptr OnDiskBlob::LoadFromDisk(const bf::path &filepath) { } unique_ptr OnDiskBlob::CreateOnDisk(const bf::path &filepath, size_t size) { - _assertFileDoesntExist(filepath); + if (bf::exists(filepath)) { + return nullptr; + } + auto blob = unique_ptr(new OnDiskBlob(filepath, size)); blob->_fillDataWithZeroes(); blob->_storeToDisk(); return blob; } -void OnDiskBlob::_assertFileDoesntExist(const bf::path &filepath) { - if (bf::exists(filepath)) { - throw FileAlreadyExistsException(filepath); - } -} - void OnDiskBlob::_fillDataWithZeroes() { _data.FillWithZeroes(); } diff --git a/src/blobstore/implementations/ondisk/OnDiskBlob.h b/src/blobstore/implementations/ondisk/OnDiskBlob.h index e318a1ef..003123b1 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlob.h +++ b/src/blobstore/implementations/ondisk/OnDiskBlob.h @@ -35,7 +35,6 @@ private: OnDiskBlob(const boost::filesystem::path &filepath, size_t size); OnDiskBlob(const boost::filesystem::path &filepath, Data &&data); - static void _assertFileDoesntExist(const boost::filesystem::path &filepath); void _fillDataWithZeroes(); void _storeToDisk() const; diff --git a/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp b/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp index 5766330b..c645e175 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp +++ b/src/blobstore/implementations/ondisk/OnDiskBlobStore.cpp @@ -4,6 +4,7 @@ #include "blobstore/utils/RandomKeyGenerator.h" using std::unique_ptr; +using std::make_unique; using std::string; using std::mutex; using std::lock_guard; @@ -16,11 +17,14 @@ namespace ondisk { OnDiskBlobStore::OnDiskBlobStore(const boost::filesystem::path &rootdir) : _rootdir(rootdir) {} -BlobWithKey OnDiskBlobStore::create(const std::string &key, size_t size) { +unique_ptr OnDiskBlobStore::create(const std::string &key, size_t size) { auto file_path = _rootdir / key; auto blob = OnDiskBlob::CreateOnDisk(file_path, size); - return BlobWithKey(key, std::move(blob)); + if (!blob) { + return nullptr; + } + return make_unique(key, std::move(blob)); } bool OnDiskBlobStore::exists(const std::string &key) { diff --git a/src/blobstore/implementations/ondisk/OnDiskBlobStore.h b/src/blobstore/implementations/ondisk/OnDiskBlobStore.h index 0db04f12..89fb7b77 100644 --- a/src/blobstore/implementations/ondisk/OnDiskBlobStore.h +++ b/src/blobstore/implementations/ondisk/OnDiskBlobStore.h @@ -18,11 +18,9 @@ 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; -protected: - BlobWithKey create(const std::string &key, size_t size) override; - private: const boost::filesystem::path _rootdir; diff --git a/src/blobstore/interface/BlobStore.h b/src/blobstore/interface/BlobStore.h index 182607c0..3bce26b0 100644 --- a/src/blobstore/interface/BlobStore.h +++ b/src/blobstore/interface/BlobStore.h @@ -17,6 +17,7 @@ public: virtual ~BlobStore() {} 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? diff --git a/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.cpp b/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.cpp index 8200ce1c..5cfb8bc9 100644 --- a/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.cpp +++ b/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.cpp @@ -5,26 +5,14 @@ using namespace blobstore; using std::string; -using std::lock_guard; -using std::mutex; - -BlobStoreWithRandomKeys::BlobStoreWithRandomKeys() - :_generate_key_mutex() { -} BlobWithKey BlobStoreWithRandomKeys::create(size_t size) { - return create(_generateKey(), size); -} - -string BlobStoreWithRandomKeys::_generateKey() { - lock_guard lock(_generate_key_mutex); - - string key; + std::unique_ptr result; do { - key = _generateRandomKey(); - } while (exists(key)); + result = create(_generateRandomKey(), size); + } while (!result); - return key; + return std::move(*result); } string BlobStoreWithRandomKeys::_generateRandomKey() { diff --git a/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.h b/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.h index 5746ba47..8936444c 100644 --- a/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.h +++ b/src/blobstore/interface/helpers/BlobStoreWithRandomKeys.h @@ -2,30 +2,23 @@ #ifndef FSPP_BLOBSTORE_BLOBSTOREWITHRANDOMKEYS_H_ #define FSPP_BLOBSTORE_BLOBSTOREWITHRANDOMKEYS_H_ -#include - #include "blobstore/interface/Blob.h" #include "blobstore/interface/BlobStore.h" namespace blobstore { +// This is an implementation helpers for BlobStores that use random blob keys. +// You should never give this static type to the client. The client should always +// work with the BlobStore interface instead. class BlobStoreWithRandomKeys: public BlobStore { public: - BlobStoreWithRandomKeys(); - virtual ~BlobStoreWithRandomKeys() {} + // Return nullptr if key already exists + virtual std::unique_ptr create(const std::string &key, size_t size) = 0; - BlobWithKey create(size_t size) override; - - virtual std::unique_ptr load(const std::string &key) = 0; - -protected: - virtual BlobWithKey create(const std::string &key, size_t size) = 0; + BlobWithKey create(size_t size) final; private: - std::string _generateKey(); std::string _generateRandomKey(); - - std::mutex _generate_key_mutex; }; } diff --git a/src/blobstore/utils/BlobWithKey.h b/src/blobstore/utils/BlobWithKey.h index 3f4f7c38..b9b64973 100644 --- a/src/blobstore/utils/BlobWithKey.h +++ b/src/blobstore/utils/BlobWithKey.h @@ -5,6 +5,7 @@ #include "blobstore/interface/Blob.h" #include +#include "fspp/utils/macros.h" namespace blobstore { diff --git a/src/test/blobstore/implementations/inmemory/InMemoryBlobStoreTest.cpp b/src/test/blobstore/implementations/inmemory/InMemoryBlobStoreTest.cpp index e39b90ca..c83f0f13 100644 --- a/src/test/blobstore/implementations/inmemory/InMemoryBlobStoreTest.cpp +++ b/src/test/blobstore/implementations/inmemory/InMemoryBlobStoreTest.cpp @@ -4,8 +4,10 @@ #include "blobstore/implementations/inmemory/InMemoryBlob.h" #include "test/blobstore/testutils/BlobStoreTest.h" +#include "test/blobstore/testutils/BlobStoreWithRandomKeysTest.h" using blobstore::BlobStore; +using blobstore::BlobStoreWithRandomKeys; using blobstore::inmemory::InMemoryBlobStore; using std::unique_ptr; @@ -19,3 +21,12 @@ public: }; INSTANTIATE_TYPED_TEST_CASE_P(InMemory, BlobStoreTest, InMemoryBlobStoreTestFixture); + +class InMemoryBlobStoreWithRandomKeysTestFixture: public BlobStoreWithRandomKeysTestFixture { +public: + unique_ptr createBlobStore() override { + return make_unique(); + } +}; + +INSTANTIATE_TYPED_TEST_CASE_P(InMemory, BlobStoreWithRandomKeysTest, InMemoryBlobStoreWithRandomKeysTestFixture); diff --git a/src/test/blobstore/implementations/ondisk/OnDiskBlobStoreTest.cpp b/src/test/blobstore/implementations/ondisk/OnDiskBlobStoreTest.cpp index 692a0791..1f524f1f 100644 --- a/src/test/blobstore/implementations/ondisk/OnDiskBlobStoreTest.cpp +++ b/src/test/blobstore/implementations/ondisk/OnDiskBlobStoreTest.cpp @@ -4,8 +4,10 @@ #include "blobstore/implementations/ondisk/OnDiskBlob.h" #include "test/blobstore/testutils/BlobStoreTest.h" +#include "test/blobstore/testutils/BlobStoreWithRandomKeysTest.h" using blobstore::BlobStore; +using blobstore::BlobStoreWithRandomKeys; using blobstore::ondisk::OnDiskBlobStore; using std::unique_ptr; @@ -21,3 +23,14 @@ private: }; INSTANTIATE_TYPED_TEST_CASE_P(OnDisk, BlobStoreTest, OnDiskBlobStoreTestFixture); + +class OnDiskBlobStoreWithRandomKeysTestFixture: public BlobStoreWithRandomKeysTestFixture { +public: + unique_ptr createBlobStore() override { + return make_unique(tempdir.path()); + } +private: + TempDir tempdir; +}; + +INSTANTIATE_TYPED_TEST_CASE_P(OnDisk, BlobStoreWithRandomKeysTest, OnDiskBlobStoreWithRandomKeysTestFixture); diff --git a/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobCreateTest.cpp b/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobCreateTest.cpp index 092fafab..d1861fe3 100644 --- a/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobCreateTest.cpp +++ b/src/test/blobstore/implementations/ondisk/OnDiskBlobTest/OnDiskBlobCreateTest.cpp @@ -34,9 +34,11 @@ TEST_F(OnDiskBlobCreateTest, CreatingBlobCreatesFile) { EXPECT_TRUE(bf::is_regular_file(file.path())); } -TEST_F(OnDiskBlobCreateTest, CreatingExistingBlobThrowsException) { +TEST_F(OnDiskBlobCreateTest, CreatingExistingBlobReturnsNull) { auto blob1 = OnDiskBlob::CreateOnDisk(file.path(), 0); - EXPECT_THROW(OnDiskBlob::CreateOnDisk(file.path(), 0), FileAlreadyExistsException); + auto blob2 = OnDiskBlob::CreateOnDisk(file.path(), 0); + EXPECT_TRUE((bool)blob1); + EXPECT_FALSE((bool)blob2); } class OnDiskBlobCreateSizeTest: public OnDiskBlobCreateTest, public WithParamInterface { diff --git a/src/test/blobstore/interface/helpers/BlobStoreWithRandomKeysTest.cpp b/src/test/blobstore/interface/helpers/BlobStoreWithRandomKeysTest.cpp new file mode 100644 index 00000000..c6c4021e --- /dev/null +++ b/src/test/blobstore/interface/helpers/BlobStoreWithRandomKeysTest.cpp @@ -0,0 +1,117 @@ +#include +#include + +#include "blobstore/interface/helpers/BlobStoreWithRandomKeys.h" +#include "blobstore/utils/RandomKeyGenerator.h" + +using ::testing::Test; +using ::testing::_; +using ::testing::Return; +using ::testing::Invoke; + +using std::string; +using std::unique_ptr; +using std::make_unique; + +using namespace blobstore; + +class BlobStoreWithRandomKeysMock: public BlobStoreWithRandomKeys { +public: + unique_ptr create(const std::string &key, size_t size) { + return unique_ptr(do_create(key, size)); + } + MOCK_METHOD2(do_create, BlobWithKey*(const std::string &, size_t)); + unique_ptr load(const string &key) { + return unique_ptr(do_load(key)); + } + MOCK_METHOD1(do_load, Blob*(const string &)); + MOCK_METHOD1(exists, bool(const string &)); +}; + +class BlobMock: public Blob { +public: + MOCK_METHOD0(data, void*()); + MOCK_CONST_METHOD0(data, const void*()); + MOCK_METHOD0(flush, void()); + MOCK_CONST_METHOD0(size, size_t()); +}; + +class BlobStoreWithRandomKeysTest: public Test { +public: + BlobStoreWithRandomKeysMock blobStoreMock; + BlobStore &blobStore = blobStoreMock; +}; + +TEST_F(BlobStoreWithRandomKeysTest, SizeIsPassedThrough0) { + EXPECT_CALL(blobStoreMock, do_create(_, 0)).WillOnce(Return(new BlobWithKey("", make_unique()))); + blobStore.create(0); +} + +TEST_F(BlobStoreWithRandomKeysTest, SizeIsPassedThrough1) { + EXPECT_CALL(blobStoreMock, do_create(_, 1)).WillOnce(Return(new BlobWithKey("", make_unique()))); + blobStore.create(1); +} + +TEST_F(BlobStoreWithRandomKeysTest, SizeIsPassedThrough1024) { + EXPECT_CALL(blobStoreMock, do_create(_, 1024)).WillOnce(Return(new BlobWithKey("", make_unique()))); + blobStore.create(1024); +} + +TEST_F(BlobStoreWithRandomKeysTest, KeyHasCorrectSize) { + EXPECT_CALL(blobStoreMock, do_create(_, _)).WillOnce(Invoke([](const string &key, size_t) { + EXPECT_EQ(RandomKeyGenerator::KEYLENGTH, key.size()); + return new BlobWithKey("", make_unique()); + })); + + blobStore.create(1024); +} + +TEST_F(BlobStoreWithRandomKeysTest, TwoBlobsGetDifferentKeys) { + string first_key; + EXPECT_CALL(blobStoreMock, do_create(_, _)) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + first_key = key; + return new BlobWithKey("", make_unique()); + })) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + EXPECT_NE(first_key, key); + return new BlobWithKey("", make_unique()); + })); + + blobStore.create(1024); + blobStore.create(1024); +} + +TEST_F(BlobStoreWithRandomKeysTest, WillTryADifferentKeyIfKeyAlreadyExists) { + string first_key; + EXPECT_CALL(blobStoreMock, do_create(_, _)) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + first_key = key; + return nullptr; + })) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + EXPECT_NE(first_key, key); + return new BlobWithKey("", make_unique()); + })); + + blobStore.create(1024); +} + +TEST_F(BlobStoreWithRandomKeysTest, WillTryADifferentKeyIfKeyAlreadyExistsTwoTimes) { + string first_key; + EXPECT_CALL(blobStoreMock, do_create(_, _)) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + first_key = key; + return nullptr; + })) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + first_key = key; + return nullptr; + })) + .WillOnce(Invoke([&first_key](const string &key, size_t) { + EXPECT_NE(first_key, key); + return new BlobWithKey("", make_unique()); + })); + + blobStore.create(1024); +} diff --git a/src/test/blobstore/testutils/BlobStoreWithRandomKeysTest.h b/src/test/blobstore/testutils/BlobStoreWithRandomKeysTest.h new file mode 100644 index 00000000..6a0ff96d --- /dev/null +++ b/src/test/blobstore/testutils/BlobStoreWithRandomKeysTest.h @@ -0,0 +1,79 @@ +#pragma once +#ifndef TEST_BLOBSTORE_IMPLEMENTATIONS_TESTUTILS_BLOBSTOREWITHRANDOMKEYSTEST_H_ +#define TEST_BLOBSTORE_IMPLEMENTATIONS_TESTUTILS_BLOBSTOREWITHRANDOMKEYSTEST_H_ + +#include "test/testutils/TempDir.h" +#include "test/testutils/VirtualTestFile.h" +#include "blobstore/interface/BlobStore.h" +#include "blobstore/utils/RandomKeyGenerator.h" + +class BlobStoreWithRandomKeysTestFixture { +public: + virtual std::unique_ptr createBlobStore() = 0; +}; + +template +class BlobStoreWithRandomKeysTest: public ::testing::Test { +public: + BOOST_STATIC_ASSERT_MSG( + (std::is_base_of::value), + "Given test fixture for instantiating the (type parameterized) BlobStoreWithRandomKeysTest must inherit from BlobStoreWithRandomKeysTestFixture" + ); + + const std::vector SIZES = {0, 1, 1024, 4096, 10*1024*1024}; + + ConcreteBlobStoreWithRandomKeysTestFixture fixture; +}; + +TYPED_TEST_CASE_P(BlobStoreWithRandomKeysTest); + +TYPED_TEST_P(BlobStoreWithRandomKeysTest, CreateTwoBlobsWithSameKeyAndSameSize) { + auto blobStore = this->fixture.createBlobStore(); + auto blob = blobStore->create("mykey", 1024); + auto blob2 = blobStore->create("mykey", 1024); + EXPECT_TRUE((bool)blob); + EXPECT_FALSE((bool)blob2); +} + +TYPED_TEST_P(BlobStoreWithRandomKeysTest, CreateTwoBlobsWithSameKeyAndDifferentSize) { + auto blobStore = this->fixture.createBlobStore(); + auto blob = blobStore->create("mykey", 1024); + auto blob2 = blobStore->create("mykey", 4096); + EXPECT_TRUE((bool)blob); + EXPECT_FALSE((bool)blob2); +} + +TYPED_TEST_P(BlobStoreWithRandomKeysTest, CreateTwoBlobsWithSameKeyAndFirstNullSize) { + auto blobStore = this->fixture.createBlobStore(); + auto blob = blobStore->create("mykey", 0); + auto blob2 = blobStore->create("mykey", 1024); + EXPECT_TRUE((bool)blob); + EXPECT_FALSE((bool)blob2); +} + +TYPED_TEST_P(BlobStoreWithRandomKeysTest, CreateTwoBlobsWithSameKeyAndSecondNullSize) { + auto blobStore = this->fixture.createBlobStore(); + auto blob = blobStore->create("mykey", 1024); + auto blob2 = blobStore->create("mykey", 0); + EXPECT_TRUE((bool)blob); + EXPECT_FALSE((bool)blob2); +} + +TYPED_TEST_P(BlobStoreWithRandomKeysTest, CreateTwoBlobsWithSameKeyAndBothNullSize) { + auto blobStore = this->fixture.createBlobStore(); + auto blob = blobStore->create("mykey", 0); + auto blob2 = blobStore->create("mykey", 0); + EXPECT_TRUE((bool)blob); + EXPECT_FALSE((bool)blob2); +} + +REGISTER_TYPED_TEST_CASE_P(BlobStoreWithRandomKeysTest, + CreateTwoBlobsWithSameKeyAndSameSize, + CreateTwoBlobsWithSameKeyAndDifferentSize, + CreateTwoBlobsWithSameKeyAndFirstNullSize, + CreateTwoBlobsWithSameKeyAndSecondNullSize, + CreateTwoBlobsWithSameKeyAndBothNullSize +); + + +#endif