Redefine error behavior in BlobStoreWithRandomKeys when key already exists and write test cases for it

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<BlobWithKey> 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<InMemoryBlob>(blob));
+  return make_unique<BlobWithKey>(key, make_unique<InMemoryBlob>(insert_result.first->second));
 }
 
 bool InMemoryBlobStore::exists(const std::string &key) {

src/blobstore/implementations/inmemory/InMemoryBlobStore.h

@@ -17,12 +17,10 @@ class InMemoryBlobStore: public BlobStoreWithRandomKeys {
 public:
   InMemoryBlobStore();
 
+  std::unique_ptr<BlobWithKey> create(const std::string &key, size_t size) override;
   bool exists(const std::string &key) override;
   std::unique_ptr<Blob> load(const std::string &key) override;
 
-protected:
-  BlobWithKey create(const std::string &key, size_t size) override;
-
 private:
   std::map<std::string, InMemoryBlob> _blobs;
 

src/blobstore/implementations/ondisk/OnDiskBlob.cpp

@@ -51,19 +51,16 @@ unique_ptr<OnDiskBlob> OnDiskBlob::LoadFromDisk(const bf::path &filepath) {
 }
 
 unique_ptr<OnDiskBlob> OnDiskBlob::CreateOnDisk(const bf::path &filepath, size_t size) {
-  _assertFileDoesntExist(filepath);
+  if (bf::exists(filepath)) {
+    return nullptr;
+  }
+
   auto blob = unique_ptr<OnDiskBlob>(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();
 }

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;
 

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<BlobWithKey> 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<BlobWithKey>(key, std::move(blob));
 }
 
 bool OnDiskBlobStore::exists(const std::string &key) {

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<BlobWithKey> create(const std::string &key, size_t size) override;
   std::unique_ptr<Blob> load(const std::string &key) override;
 
-protected:
-  BlobWithKey create(const std::string &key, size_t size) override;
-
 private:
   const boost::filesystem::path _rootdir;
 

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<Blob> load(const std::string &key) = 0;
   virtual bool exists(const std::string &key) = 0;
   //TODO Needed for performance? Or is deleting loaded blobs enough?

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<mutex> lock(_generate_key_mutex);
-
-  string key;
+  std::unique_ptr<BlobWithKey> result;
   do {
-    key = _generateRandomKey();
-  } while (exists(key));
+    result = create(_generateRandomKey(), size);
+  } while (!result);
 
-  return key;
+  return std::move(*result);
 }
 
 string BlobStoreWithRandomKeys::_generateRandomKey() {

src/blobstore/interface/helpers/BlobStoreWithRandomKeys.h

@@ -2,30 +2,23 @@
 #ifndef FSPP_BLOBSTORE_BLOBSTOREWITHRANDOMKEYS_H_
 #define FSPP_BLOBSTORE_BLOBSTOREWITHRANDOMKEYS_H_
 
-#include <mutex>
-
 #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<BlobWithKey> create(const std::string &key, size_t size) = 0;
 
-  BlobWithKey create(size_t size) override;
-
-  virtual std::unique_ptr<Blob> 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;
 };
 
 }

src/blobstore/utils/BlobWithKey.h

@@ -5,6 +5,7 @@
 #include "blobstore/interface/Blob.h"
 
 #include <memory>
+#include "fspp/utils/macros.h"
 
 namespace blobstore {
 

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<BlobStoreWithRandomKeys> createBlobStore() override {
+    return make_unique<InMemoryBlobStore>();
+  }
+};
+
+INSTANTIATE_TYPED_TEST_CASE_P(InMemory, BlobStoreWithRandomKeysTest, InMemoryBlobStoreWithRandomKeysTestFixture);

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<BlobStoreWithRandomKeys> createBlobStore() override {
+    return make_unique<OnDiskBlobStore>(tempdir.path());
+  }
+private:
+  TempDir tempdir;
+};
+
+INSTANTIATE_TYPED_TEST_CASE_P(OnDisk, BlobStoreWithRandomKeysTest, OnDiskBlobStoreWithRandomKeysTestFixture);

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<size_t> {

src/test/blobstore/interface/helpers/BlobStoreWithRandomKeysTest.cpp

@@ -0,0 +1,117 @@
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#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<BlobWithKey> create(const std::string &key, size_t size) {
+    return unique_ptr<BlobWithKey>(do_create(key, size));
+  }
+  MOCK_METHOD2(do_create, BlobWithKey*(const std::string &, size_t));
+  unique_ptr<Blob> load(const string &key) {
+    return unique_ptr<Blob>(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<BlobMock>())));
+  blobStore.create(0);
+}
+
+TEST_F(BlobStoreWithRandomKeysTest, SizeIsPassedThrough1) {
+  EXPECT_CALL(blobStoreMock, do_create(_, 1)).WillOnce(Return(new BlobWithKey("", make_unique<BlobMock>())));
+  blobStore.create(1);
+}
+
+TEST_F(BlobStoreWithRandomKeysTest, SizeIsPassedThrough1024) {
+  EXPECT_CALL(blobStoreMock, do_create(_, 1024)).WillOnce(Return(new BlobWithKey("", make_unique<BlobMock>())));
+  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<BlobMock>());
+  }));
+
+  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<BlobMock>());
+      }))
+      .WillOnce(Invoke([&first_key](const string &key, size_t) {
+        EXPECT_NE(first_key, key);
+        return new BlobWithKey("", make_unique<BlobMock>());
+      }));
+
+  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<BlobMock>());
+      }));
+
+  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<BlobMock>());
+      }));
+
+  blobStore.create(1024);
+}

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<blobstore::BlobStoreWithRandomKeys> createBlobStore() = 0;
+};
+
+template<class ConcreteBlobStoreWithRandomKeysTestFixture>
+class BlobStoreWithRandomKeysTest: public ::testing::Test {
+public:
+  BOOST_STATIC_ASSERT_MSG(
+    (std::is_base_of<BlobStoreWithRandomKeysTestFixture, ConcreteBlobStoreWithRandomKeysTestFixture>::value),
+    "Given test fixture for instantiating the (type parameterized) BlobStoreWithRandomKeysTest must inherit from BlobStoreWithRandomKeysTestFixture"
+  );
+
+  const std::vector<size_t> 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