# HG changeset patch # User Sebastien Jodogne # Date 1617700620 -7200 # Node ID 675f8322eb7ceefbd52fbf0704115c556eb64dfb # Parent 5744f09b0b1b2d64c0936f220606672f1d32dcfe refactored StorageBackend by introducing an accessor class diff -r 5744f09b0b1b -r 675f8322eb7c Framework/Plugins/StorageBackend.cpp --- a/Framework/Plugins/StorageBackend.cpp Sat Apr 03 18:56:14 2021 +0200 +++ b/Framework/Plugins/StorageBackend.cpp Tue Apr 06 11:17:00 2021 +0200 @@ -84,92 +84,112 @@ } - void StorageBackend::Create(DatabaseManager::Transaction& transaction, - const std::string& uuid, - const void* content, - size_t size, - OrthancPluginContentType type) + void StorageBackend::Accessor::Create(const std::string& uuid, + const void* content, + size_t size, + OrthancPluginContentType type) { - DatabaseManager::CachedStatement statement( - STATEMENT_FROM_HERE, GetManager(), - "INSERT INTO StorageArea VALUES (${uuid}, ${content}, ${type})"); + DatabaseManager::Transaction transaction(manager_, TransactionType_ReadWrite); + + { + DatabaseManager::CachedStatement statement( + STATEMENT_FROM_HERE, manager_, + "INSERT INTO StorageArea VALUES (${uuid}, ${content}, ${type})"); - statement.SetParameterType("uuid", ValueType_Utf8String); - statement.SetParameterType("content", ValueType_File); - statement.SetParameterType("type", ValueType_Integer64); + statement.SetParameterType("uuid", ValueType_Utf8String); + statement.SetParameterType("content", ValueType_File); + statement.SetParameterType("type", ValueType_Integer64); - Dictionary args; - args.SetUtf8Value("uuid", uuid); - args.SetFileValue("content", content, size); - args.SetIntegerValue("type", type); + Dictionary args; + args.SetUtf8Value("uuid", uuid); + args.SetFileValue("content", content, size); + args.SetIntegerValue("type", type); - statement.Execute(args); + statement.Execute(args); + } + + transaction.Commit(); } - void StorageBackend::Read(StorageBackend::IFileContentVisitor& target, - DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type) + void StorageBackend::Accessor::Read(StorageBackend::IFileContentVisitor& visitor, + const std::string& uuid, + OrthancPluginContentType type) { - DatabaseManager::CachedStatement statement( - STATEMENT_FROM_HERE, GetManager(), - "SELECT content FROM StorageArea WHERE uuid=${uuid} AND type=${type}"); + DatabaseManager::Transaction transaction(manager_, TransactionType_ReadOnly); + + { + DatabaseManager::CachedStatement statement( + STATEMENT_FROM_HERE, manager_, + "SELECT content FROM StorageArea WHERE uuid=${uuid} AND type=${type}"); + + statement.SetParameterType("uuid", ValueType_Utf8String); + statement.SetParameterType("type", ValueType_Integer64); + + Dictionary args; + args.SetUtf8Value("uuid", uuid); + args.SetIntegerValue("type", type); - statement.SetParameterType("uuid", ValueType_Utf8String); - statement.SetParameterType("type", ValueType_Integer64); + statement.Execute(args); - Dictionary args; - args.SetUtf8Value("uuid", uuid); - args.SetIntegerValue("type", type); - - statement.Execute(args); + if (statement.IsDone()) + { + throw Orthanc::OrthancException(Orthanc::ErrorCode_UnknownResource); + } + else if (statement.GetResultFieldsCount() != 1) + { + throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); + } + else + { + const IValue& value = statement.GetResultField(0); + + switch (value.GetType()) + { + case ValueType_File: + visitor.Assign(dynamic_cast(value).GetContent()); + break; - if (statement.IsDone()) - { - throw Orthanc::OrthancException(Orthanc::ErrorCode_UnknownResource); + case ValueType_BinaryString: + visitor.Assign(dynamic_cast(value).GetContent()); + break; + + default: + throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); + } + } } - else if (statement.GetResultFieldsCount() != 1) - { - throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); - } - else + + transaction.Commit(); + + if (!visitor.IsSuccess()) { - const IValue& value = statement.GetResultField(0); - - switch (value.GetType()) - { - case ValueType_File: - target.Assign(dynamic_cast(value).GetContent()); - break; - - case ValueType_BinaryString: - target.Assign(dynamic_cast(value).GetContent()); - break; - - default: - throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); - } + throw Orthanc::OrthancException(Orthanc::ErrorCode_Database, "Could not read attachment from the storage area"); } } - void StorageBackend::Remove(DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type) + void StorageBackend::Accessor::Remove(const std::string& uuid, + OrthancPluginContentType type) { - DatabaseManager::CachedStatement statement( - STATEMENT_FROM_HERE, GetManager(), - "DELETE FROM StorageArea WHERE uuid=${uuid} AND type=${type}"); + DatabaseManager::Transaction transaction(manager_, TransactionType_ReadWrite); + + { + DatabaseManager::CachedStatement statement( + STATEMENT_FROM_HERE, manager_, + "DELETE FROM StorageArea WHERE uuid=${uuid} AND type=${type}"); - statement.SetParameterType("uuid", ValueType_Utf8String); - statement.SetParameterType("type", ValueType_Integer64); + statement.SetParameterType("uuid", ValueType_Utf8String); + statement.SetParameterType("type", ValueType_Integer64); - Dictionary args; - args.SetUtf8Value("uuid", uuid); - args.SetIntegerValue("type", type); + Dictionary args; + args.SetUtf8Value("uuid", uuid); + args.SetIntegerValue("type", type); - statement.Execute(args); + statement.Execute(args); + } + + transaction.Commit(); } @@ -189,11 +209,12 @@ { throw Orthanc::OrthancException(Orthanc::ErrorCode_BadSequenceOfCalls); } - - DatabaseManager::Transaction transaction(backend_->GetManager(), TransactionType_ReadWrite); - backend_->Create(transaction, uuid, content, static_cast(size), type); - transaction.Commit(); - return OrthancPluginErrorCode_Success; + else + { + StorageBackend::Accessor accessor(*backend_); + accessor.Create(uuid, content, static_cast(size), type); + return OrthancPluginErrorCode_Success; + } } ORTHANC_PLUGINS_DATABASE_CATCH; } @@ -217,7 +238,7 @@ { } - bool IsSuccess() const + virtual bool IsSuccess() const ORTHANC_OVERRIDE { return success_; } @@ -254,27 +275,21 @@ { throw Orthanc::OrthancException(Orthanc::ErrorCode_BadSequenceOfCalls); } - - if (target == NULL) + else if (target == NULL) { throw Orthanc::OrthancException(Orthanc::ErrorCode_NullPointer); } - - Visitor visitor(target); - + else { - DatabaseManager::Transaction transaction(backend_->GetManager(), TransactionType_ReadOnly); - backend_->Read(visitor, transaction, uuid, type); + Visitor visitor(target); - if (!visitor.IsSuccess()) { - throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); + StorageBackend::Accessor accessor(*backend_); + accessor.Read(visitor, uuid, type); } - - transaction.Commit(); + + return OrthancPluginErrorCode_Success; } - - return OrthancPluginErrorCode_Success; } ORTHANC_PLUGINS_DATABASE_CATCH; } @@ -302,7 +317,7 @@ { } - bool IsSuccess() const + virtual bool IsSuccess() const ORTHANC_OVERRIDE { return success_; } @@ -351,28 +366,22 @@ { throw Orthanc::OrthancException(Orthanc::ErrorCode_BadSequenceOfCalls); } - - if (data == NULL || - size == NULL) + else if (data == NULL || + size == NULL) { throw Orthanc::OrthancException(Orthanc::ErrorCode_NullPointer); } - - Visitor visitor(data, size); - + else { - DatabaseManager::Transaction transaction(backend_->GetManager(), TransactionType_ReadOnly); - backend_->Read(visitor, transaction, uuid, type); + Visitor visitor(data, size); - if (!visitor.IsSuccess()) { - throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); - } - - transaction.Commit(); + StorageBackend::Accessor accessor(*backend_); + accessor.Read(visitor, uuid, type); + } + + return OrthancPluginErrorCode_Success; } - - return OrthancPluginErrorCode_Success; } ORTHANC_PLUGINS_DATABASE_CATCH; } @@ -383,10 +392,16 @@ { try { - DatabaseManager::Transaction transaction(backend_->GetManager(), TransactionType_ReadWrite); - backend_->Remove(transaction, uuid, type); - transaction.Commit(); - return OrthancPluginErrorCode_Success; + if (backend_.get() == NULL) + { + throw Orthanc::OrthancException(Orthanc::ErrorCode_BadSequenceOfCalls); + } + else + { + StorageBackend::Accessor accessor(*backend_); + accessor.Remove(uuid, type); + return OrthancPluginErrorCode_Success; + } } ORTHANC_PLUGINS_DATABASE_CATCH; } @@ -440,10 +455,9 @@ } - void StorageBackend::ReadToString(std::string& target, - DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type) + void StorageBackend::Accessor::ReadToString(std::string& target, + const std::string& uuid, + OrthancPluginContentType type) { class Visitor : public StorageBackend::IFileContentVisitor { @@ -458,7 +472,7 @@ { } - bool IsSuccess() const + virtual bool IsSuccess() const ORTHANC_OVERRIDE { return success_; } @@ -479,12 +493,6 @@ Visitor visitor(target); - - Read(visitor, transaction, uuid, type); - - if (!visitor.IsSuccess()) - { - throw Orthanc::OrthancException(Orthanc::ErrorCode_Database); - } + Read(visitor, uuid, type); } } diff -r 5744f09b0b1b -r 675f8322eb7c Framework/Plugins/StorageBackend.h --- a/Framework/Plugins/StorageBackend.h Sat Apr 03 18:56:14 2021 +0200 +++ b/Framework/Plugins/StorageBackend.h Tue Apr 06 11:17:00 2021 +0200 @@ -33,6 +33,11 @@ private: std::unique_ptr manager_; + DatabaseManager& GetManager(); + + protected: + void SetDatabase(IDatabase* database); // Takes ownership + public: class IFileContentVisitor : public boost::noncopyable { @@ -42,43 +47,46 @@ } virtual void Assign(const std::string& content) = 0; + + virtual bool IsSuccess() const = 0; + }; + + class Accessor : public boost::noncopyable + { + private: + DatabaseManager& manager_; + + public: + Accessor(StorageBackend& backend) : + manager_(backend.GetManager()) + { + } + + void Create(const std::string& uuid, + const void* content, + size_t size, + OrthancPluginContentType type); + + void Read(IFileContentVisitor& visitor, + const std::string& uuid, + OrthancPluginContentType type); + + void Remove(const std::string& uuid, + OrthancPluginContentType type); + + // For unit tests + void ReadToString(std::string& target, + const std::string& uuid, + OrthancPluginContentType type); }; virtual ~StorageBackend() { } - void SetDatabase(IDatabase* database); // Takes ownership - - DatabaseManager& GetManager(); - - // NB: These methods will always be invoked in mutual exclusion, - // as having access to some "DatabaseManager::Transaction" implies - // that the parent "DatabaseManager" is locked - virtual void Create(DatabaseManager::Transaction& transaction, - const std::string& uuid, - const void* content, - size_t size, - OrthancPluginContentType type) ORTHANC_FINAL; - - virtual void Read(IFileContentVisitor& target, - DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type) ORTHANC_FINAL; - - virtual void Remove(DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type) ORTHANC_FINAL; - static void Register(OrthancPluginContext* context, StorageBackend* backend); // Takes ownership static void Finalize(); - - // For unit tests - void ReadToString(std::string& target, - DatabaseManager::Transaction& transaction, - const std::string& uuid, - OrthancPluginContentType type); }; } diff -r 5744f09b0b1b -r 675f8322eb7c MySQL/UnitTests/UnitTestsMain.cpp --- a/MySQL/UnitTests/UnitTestsMain.cpp Sat Apr 03 18:56:14 2021 +0200 +++ b/MySQL/UnitTests/UnitTestsMain.cpp Tue Apr 06 11:17:00 2021 +0200 @@ -137,41 +137,50 @@ static int64_t CountFiles(OrthancDatabases::MySQLDatabase& db) { - OrthancDatabases::Query query("SELECT COUNT(*) FROM StorageArea", true); - OrthancDatabases::MySQLStatement s(db, query); - OrthancDatabases::MySQLTransaction t(db, OrthancDatabases::TransactionType_ReadOnly); - OrthancDatabases::Dictionary d; - std::unique_ptr result(s.Execute(t, d)); - return dynamic_cast(result->GetField(0)).GetValue(); + OrthancDatabases::MySQLTransaction transaction(db, OrthancDatabases::TransactionType_ReadOnly); + + int64_t count; + { + OrthancDatabases::Query query("SELECT COUNT(*) FROM StorageArea", true); + OrthancDatabases::MySQLStatement s(db, query); + OrthancDatabases::MySQLTransaction t(db, OrthancDatabases::TransactionType_ReadOnly); + OrthancDatabases::Dictionary d; + std::unique_ptr result(s.Execute(t, d)); + count = dynamic_cast(result->GetField(0)).GetValue(); + } + + transaction.Commit(); + return count; } TEST(MySQL, StorageArea) { - OrthancDatabases::MySQLStorageArea storageArea(globalParameters_, true); + std::unique_ptr database( + OrthancDatabases::MySQLDatabase::OpenDatabaseConnection(globalParameters_)); + + OrthancDatabases::MySQLStorageArea storageArea(globalParameters_, true /* clear database */); { - OrthancDatabases::DatabaseManager::Transaction transaction(storageArea.GetManager(), OrthancDatabases::TransactionType_ReadWrite); - OrthancDatabases::MySQLDatabase& db = - dynamic_cast(transaction.GetDatabase()); - - ASSERT_EQ(0, CountFiles(db)); + OrthancDatabases::MySQLStorageArea::Accessor accessor(storageArea); + + ASSERT_EQ(0, CountFiles(*database)); for (int i = 0; i < 10; i++) { std::string uuid = boost::lexical_cast(i); std::string value = "Value " + boost::lexical_cast(i * 2); - storageArea.Create(transaction, uuid, value.c_str(), value.size(), OrthancPluginContentType_Unknown); + accessor.Create(uuid, value.c_str(), value.size(), OrthancPluginContentType_Unknown); } std::string buffer; - ASSERT_THROW(storageArea.ReadToString(buffer, transaction, "nope", OrthancPluginContentType_Unknown), + ASSERT_THROW(accessor.ReadToString(buffer, "nope", OrthancPluginContentType_Unknown), Orthanc::OrthancException); - ASSERT_EQ(10, CountFiles(db)); - storageArea.Remove(transaction, "5", OrthancPluginContentType_Unknown); + ASSERT_EQ(10, CountFiles(*database)); + accessor.Remove("5", OrthancPluginContentType_Unknown); - ASSERT_EQ(9, CountFiles(db)); + ASSERT_EQ(9, CountFiles(*database)); for (int i = 0; i < 10; i++) { @@ -181,25 +190,22 @@ if (i == 5) { - ASSERT_THROW(storageArea.ReadToString(buffer, transaction, uuid, OrthancPluginContentType_Unknown), + ASSERT_THROW(accessor.ReadToString(buffer, uuid, OrthancPluginContentType_Unknown), Orthanc::OrthancException); } else { - storageArea.ReadToString(buffer, transaction, uuid, OrthancPluginContentType_Unknown); + accessor.ReadToString(buffer, uuid, OrthancPluginContentType_Unknown); ASSERT_EQ(expected, buffer); } } for (int i = 0; i < 10; i++) { - storageArea.Remove(transaction, boost::lexical_cast(i), - OrthancPluginContentType_Unknown); + accessor.Remove(boost::lexical_cast(i), OrthancPluginContentType_Unknown); } - ASSERT_EQ(0, CountFiles(db)); - - transaction.Commit(); + ASSERT_EQ(0, CountFiles(*database)); } } diff -r 5744f09b0b1b -r 675f8322eb7c PostgreSQL/UnitTests/PostgreSQLTests.cpp --- a/PostgreSQL/UnitTests/PostgreSQLTests.cpp Sat Apr 03 18:56:14 2021 +0200 +++ b/PostgreSQL/UnitTests/PostgreSQLTests.cpp Tue Apr 06 11:17:00 2021 +0200 @@ -65,10 +65,19 @@ static int64_t CountLargeObjects(PostgreSQLDatabase& db) { - // Count the number of large objects in the DB - PostgreSQLStatement s(db, "SELECT COUNT(*) FROM pg_catalog.pg_largeobject"); - PostgreSQLResult r(s); - return r.GetInteger64(0); + PostgreSQLTransaction transaction(db, TransactionType_ReadOnly); + + int64_t count; + + { + // Count the number of large objects in the DB + PostgreSQLStatement s(db, "SELECT COUNT(*) FROM pg_catalog.pg_largeobject"); + PostgreSQLResult r(s); + count = r.GetInteger64(0); + } + + transaction.Commit(); + return count; } @@ -349,30 +358,30 @@ TEST(PostgreSQL, StorageArea) { + std::unique_ptr database(PostgreSQLDatabase::OpenDatabaseConnection(globalParameters_)); + PostgreSQLStorageArea storageArea(globalParameters_, true /* clear database */); { - DatabaseManager::Transaction transaction(storageArea.GetManager(), TransactionType_ReadWrite); - PostgreSQLDatabase& db = - dynamic_cast(transaction.GetDatabase()); - - ASSERT_EQ(0, CountLargeObjects(db)); + PostgreSQLStorageArea::Accessor accessor(storageArea); + + ASSERT_EQ(0, CountLargeObjects(*database)); for (int i = 0; i < 10; i++) { std::string uuid = boost::lexical_cast(i); std::string value = "Value " + boost::lexical_cast(i * 2); - storageArea.Create(transaction, uuid, value.c_str(), value.size(), OrthancPluginContentType_Unknown); + accessor.Create(uuid, value.c_str(), value.size(), OrthancPluginContentType_Unknown); } std::string buffer; - ASSERT_THROW(storageArea.ReadToString(buffer, transaction, "nope", OrthancPluginContentType_Unknown), + ASSERT_THROW(accessor.ReadToString(buffer, "nope", OrthancPluginContentType_Unknown), Orthanc::OrthancException); - ASSERT_EQ(10, CountLargeObjects(db)); - storageArea.Remove(transaction, "5", OrthancPluginContentType_Unknown); + ASSERT_EQ(10, CountLargeObjects(*database)); + accessor.Remove("5", OrthancPluginContentType_Unknown); - ASSERT_EQ(9, CountLargeObjects(db)); + ASSERT_EQ(9, CountLargeObjects(*database)); for (int i = 0; i < 10; i++) { @@ -381,25 +390,22 @@ if (i == 5) { - ASSERT_THROW(storageArea.ReadToString(buffer, transaction, uuid, OrthancPluginContentType_Unknown), + ASSERT_THROW(accessor.ReadToString(buffer, uuid, OrthancPluginContentType_Unknown), Orthanc::OrthancException); } else { - storageArea.ReadToString(buffer, transaction, uuid, OrthancPluginContentType_Unknown); + accessor.ReadToString(buffer, uuid, OrthancPluginContentType_Unknown); ASSERT_EQ(expected, buffer); } } for (int i = 0; i < 10; i++) { - storageArea.Remove(transaction, boost::lexical_cast(i), - OrthancPluginContentType_Unknown); + accessor.Remove(boost::lexical_cast(i), OrthancPluginContentType_Unknown); } - ASSERT_EQ(0, CountLargeObjects(db)); - - transaction.Commit(); + ASSERT_EQ(0, CountLargeObjects(*database)); } }