diff --git a/include/silo/common/data_version.h b/include/silo/common/data_version.h index e9d0dd362..f9c04709f 100644 --- a/include/silo/common/data_version.h +++ b/include/silo/common/data_version.h @@ -20,7 +20,7 @@ class DataVersion { static std::optional fromString(const std::string& string); private: - DataVersion(std::string data_version); + explicit DataVersion(std::string data_version); std::string data_version; }; diff --git a/include/silo/database.h b/include/silo/database.h index 10dc88aeb..7ff814958 100644 --- a/include/silo/database.h +++ b/include/silo/database.h @@ -10,6 +10,7 @@ #include "silo/common/data_version.h" #include "silo/config/database_config.h" +#include "silo/query_engine/query_result.h" #include "silo/storage/aa_store.h" #include "silo/storage/column/date_column.h" #include "silo/storage/column/float_column.h" @@ -63,9 +64,11 @@ class Database { void setDataVersion(const DataVersion& data_version); virtual DataVersion getDataVersion() const; + virtual query_engine::QueryResult executeQuery(const std::string& query) const; + private: PangoLineageAliasLookup alias_key; - DataVersion data_version_ = {""}; + DataVersion data_version_ = DataVersion{""}; void build( const preprocessing::PreprocessingConfig& preprocessing_config, diff --git a/include/silo_api/database_mutex.h b/include/silo_api/database_mutex.h index 779ca87f3..bf0dd3f23 100644 --- a/include/silo_api/database_mutex.h +++ b/include/silo_api/database_mutex.h @@ -12,9 +12,9 @@ class FixedDatabase { std::shared_lock lock; public: - FixedDatabase(silo::Database& database, std::shared_mutex& mutex); + FixedDatabase(const silo::Database& database, std::shared_lock&& mutex); - silo::Database& database; + const silo::Database& database; }; class DatabaseMutex { @@ -26,9 +26,9 @@ class DatabaseMutex { explicit DatabaseMutex(silo::Database&& database); - void setDatabase(silo::Database&& db); + void setDatabase(silo::Database&& new_database); - const FixedDatabase getDatabase(); + virtual FixedDatabase getDatabase(); }; } // namespace silo_api diff --git a/src/silo/common/data_version.cpp b/src/silo/common/data_version.cpp index 0dcd7e39f..1abccb33e 100644 --- a/src/silo/common/data_version.cpp +++ b/src/silo/common/data_version.cpp @@ -1,5 +1,6 @@ #include "silo/common/data_version.h" +#include #include #include @@ -8,7 +9,7 @@ namespace silo { DataVersion DataVersion::mineDataVersion() { const auto now = std::chrono::system_clock::now(); const auto now_as_time_t = std::chrono::system_clock::to_time_t(now); - return DataVersion(std::to_string(now_as_time_t)); + return DataVersion{std::to_string(now_as_time_t)}; } DataVersion::DataVersion(std::string data_version) @@ -24,7 +25,9 @@ bool DataVersion::operator<(const DataVersion& other) const { } std::optional DataVersion::fromString(const std::string& string) { - if (std::all_of(string.begin(), string.end(), [](char c) { return c >= '0' && c <= '9'; })) { + if (std::all_of(string.begin(), string.end(), [](char character) { + return character >= '0' && character <= '9'; + })) { return DataVersion{string}; } return std::nullopt; diff --git a/src/silo/common/data_version.test.cpp b/src/silo/common/data_version.test.cpp index 536ea117c..fe86ebb0a 100644 --- a/src/silo/common/data_version.test.cpp +++ b/src/silo/common/data_version.test.cpp @@ -11,11 +11,22 @@ TEST(DataVersion, shouldMineDataVersionFromUnixTime) { } TEST(DataVersion, shouldConstructFromVersionString) { - const auto version = DataVersion("1234567890"); - EXPECT_EQ(version.toString(), "1234567890"); + const auto version = DataVersion::fromString("1234567890"); + EXPECT_TRUE(version.has_value()); + if (version.has_value()) { + EXPECT_EQ(version->toString(), "1234567890"); + } +} + +TEST(DataVersion, shouldRejectFalseVersionFromString) { + const auto version = DataVersion::fromString("3X123"); + EXPECT_FALSE(version.has_value()); } TEST(DataVersion, shouldConstructWithDefaultVersion) { - const auto version = DataVersion(); - EXPECT_EQ(version.toString(), ""); + const auto version = DataVersion::fromString(""); + EXPECT_TRUE(version.has_value()); + if (version.has_value()) { + EXPECT_EQ(version->toString(), ""); + } } \ No newline at end of file diff --git a/src/silo/database.cpp b/src/silo/database.cpp index cbc99cfec..07c858106 100644 --- a/src/silo/database.cpp +++ b/src/silo/database.cpp @@ -40,6 +40,7 @@ #include "silo/preprocessing/pango_lineage_count.h" #include "silo/preprocessing/partition.h" #include "silo/preprocessing/preprocessing_config.h" +#include "silo/query_engine/query_engine.h" #include "silo/storage/aa_store.h" #include "silo/storage/column/date_column.h" #include "silo/storage/column/float_column.h" @@ -758,4 +759,10 @@ DataVersion Database::getDataVersion() const { return data_version_; } +query_engine::QueryResult Database::executeQuery(const std::string& query) const { + const silo::query_engine::QueryEngine query_engine(*this); + + return query_engine.executeQuery(query); +} + } // namespace silo diff --git a/src/silo/database.test.cpp b/src/silo/database.test.cpp index 8c6af77f7..887d66e8d 100644 --- a/src/silo/database.test.cpp +++ b/src/silo/database.test.cpp @@ -102,18 +102,21 @@ TEST(DatabaseTest, shouldReturnCorrectDatabaseInfo) { TEST(DatabaseTest, shouldSaveAndReloadDatabaseWithoutErrors) { auto first_database = buildTestDatabase(); - const std::string directory = "output/test_serialized_state/"; + const std::filesystem::path directory = "output/test_serialized_state/"; if (std::filesystem::exists(directory)) { std::filesystem::remove_all(directory); } std::filesystem::create_directories(directory); + silo::DataVersion data_version = first_database.getDataVersion(); + first_database.saveDatabaseState(directory); - auto database = silo::Database::loadDatabaseState(directory); + auto database = silo::Database::loadDatabaseState(directory / data_version.toString()); const auto simple_database_info = database.getDatabaseInfo(); EXPECT_GT(simple_database_info.total_size, 0); EXPECT_EQ(simple_database_info.sequence_count, 100); + EXPECT_GT(simple_database_info.n_bitmaps_size, 0); } diff --git a/src/silo/storage/database_partition.cpp b/src/silo/storage/database_partition.cpp index 0bb3ca26f..1c7a59549 100644 --- a/src/silo/storage/database_partition.cpp +++ b/src/silo/storage/database_partition.cpp @@ -21,6 +21,7 @@ class StringColumnPartition; DatabasePartition::DatabasePartition(std::vector chunks) : chunks(std::move(chunks)) {} +// NOLINTNEXTLINE(readability-function-cognitive-complexity) void DatabasePartition::flipBitmaps() { for (auto& [_, seq_store] : nuc_sequences) { tbb::enumerable_thread_specific @@ -37,7 +38,7 @@ void DatabasePartition::flipBitmaps() { } }); for (const auto& local : flipped_bitmaps) { - for (auto& element : local) { + for (const auto& element : local) { seq_store.indexing_differences_to_reference_genome.emplace_back(element); } } @@ -57,7 +58,7 @@ void DatabasePartition::flipBitmaps() { } }); for (const auto& local : flipped_bitmaps) { - for (auto& element : local) { + for (const auto& element : local) { aa_store.indexing_differences_to_reference_sequence.emplace_back(element); } } diff --git a/src/silo_api/api.cpp b/src/silo_api/api.cpp index 4d564a783..73f2aa2f1 100644 --- a/src/silo_api/api.cpp +++ b/src/silo_api/api.cpp @@ -183,7 +183,7 @@ class SiloServer : public Poco::Util::ServerApplication { const Poco::Net::ServerSocket server_socket(port); - silo_api::DatabaseWatcher watcher(data_directory, database_mutex); + const silo_api::DatabaseWatcher watcher(data_directory, database_mutex); Poco::Net::HTTPServer server( new silo_api::SiloRequestHandlerFactory(database_mutex), diff --git a/src/silo_api/database_mutex.cpp b/src/silo_api/database_mutex.cpp index 058861461..fe104855c 100644 --- a/src/silo_api/database_mutex.cpp +++ b/src/silo_api/database_mutex.cpp @@ -1,17 +1,21 @@ #include "silo_api/database_mutex.h" -silo_api::FixedDatabase::FixedDatabase(silo::Database& database, std::shared_mutex& mutex) - : lock(mutex), +silo_api::FixedDatabase::FixedDatabase( + const silo::Database& database, + std::shared_lock&& mutex +) + : lock(std::move(mutex)), database(database) {} silo_api::DatabaseMutex::DatabaseMutex(silo::Database&& database) : database(std::move(database)) {} -void silo_api::DatabaseMutex::setDatabase(silo::Database&& db) { +void silo_api::DatabaseMutex::setDatabase(silo::Database&& new_database) { const std::unique_lock lock(mutex); - database = std::move(db); + database = std::move(new_database); } -const silo_api::FixedDatabase silo_api::DatabaseMutex::getDatabase() { - return FixedDatabase(database, mutex); +silo_api::FixedDatabase silo_api::DatabaseMutex::getDatabase() { + std::shared_lock lock(mutex); + return {database, std::move(lock)}; } diff --git a/src/silo_api/database_watcher.cpp b/src/silo_api/database_watcher.cpp index 6ff83f951..9a66c6f2c 100644 --- a/src/silo_api/database_watcher.cpp +++ b/src/silo_api/database_watcher.cpp @@ -1,11 +1,15 @@ #include "silo_api/database_watcher.h" +#include + +#include + namespace { std::optional findInitialDatabase(const std::filesystem::path& path) { SPDLOG_INFO("Scanning path {} for valid data", path.string()); std::vector> all_found_data; - for (auto& directory_entry : std::filesystem::directory_iterator{path}) { + for (const auto& directory_entry : std::filesystem::directory_iterator{path}) { SPDLOG_INFO("Checking directory entry {}", directory_entry.path().string()); if (!directory_entry.is_directory()) { SPDLOG_INFO("Skipping {} because it is not a directory", directory_entry.path().string()); @@ -14,7 +18,7 @@ std::optional findInitialDatabase(const std::filesystem::path& p auto data_version = silo::DataVersion::fromString(directory_entry.path().filename().string()); if (data_version == std::nullopt) { SPDLOG_INFO( - "Skipping {}. Its name is not a valid data version.", path.filename().string() + "Skipping {}. Its name is not a valid data version.", directory_entry.path().string() ); continue; } @@ -23,21 +27,21 @@ std::optional findInitialDatabase(const std::filesystem::path& p directory_entry.path().string(), data_version->toString() ); - all_found_data.emplace_back(directory_entry, *data_version); + all_found_data.emplace_back(directory_entry.path(), std::move(*data_version)); } if (all_found_data.empty()) { SPDLOG_INFO("No previous data found, place data in {} for ingestion", path.string()); return std::nullopt; } if (all_found_data.size() == 1) { - std::filesystem::path data_path = all_found_data.at(0).first; + const std::filesystem::path data_path = all_found_data.at(0).first; SPDLOG_INFO("Using previous data: {}", data_path.string()); return silo::Database::loadDatabaseState(data_path); } - auto max_element = std::max( + auto max_element = std::max_element( all_found_data.begin(), all_found_data.end(), - [](const auto& element1, const auto& element2) { return element1->second < element2->second; } + [](const auto& element1, const auto& element2) { return element1.second < element2.second; } ); SPDLOG_INFO( "Selected database with highest data-version {} in path {} for ingestion", @@ -59,37 +63,39 @@ silo_api::DatabaseWatcher::DatabaseWatcher(const std::string& path, DatabaseMute } } -void silo_api::DatabaseWatcher::onItemAdded(const Poco::DirectoryWatcher::DirectoryEvent& ev) { - SPDLOG_TRACE("Item {} was added to the folder {}", watcher.directory().path(), ev.item.path()); - if (!ev.item.isDirectory()) { - SPDLOG_INFO("Ignoring item added event: {}. It is not a folder", ev.item.path()); +void silo_api::DatabaseWatcher::onItemAdded(const Poco::DirectoryWatcher::DirectoryEvent& event) { + SPDLOG_TRACE( + "Item {} was added to the folder {}", watcher.directory().path(), event.item.path() + ); + if (!event.item.isDirectory()) { + SPDLOG_INFO("Ignoring item added event: {}. It is not a folder", event.item.path()); return; } - std::filesystem::path path = ev.item.path(); + const std::filesystem::path path = event.item.path(); auto new_data_version = silo::DataVersion::fromString(path.filename().string()); if (new_data_version == std::nullopt) { SPDLOG_INFO( "Ignoring item added event: {}. Its name {} is not a valid data version.", - ev.item.path(), + event.item.path(), path.filename().string() ); return; } { - auto fixed_database = database_mutex.getDatabase(); + const auto fixed_database = database_mutex.getDatabase(); if (!(fixed_database.database.getDataVersion() < new_data_version.value())) { SPDLOG_INFO( "Ignoring item added event: {}. Its version is not newer than the current version", - ev.item.path() + event.item.path() ); return; } } - SPDLOG_INFO("New data version detected: {}", ev.item.path()); - const std::string newVersionPath = ev.item.path(); - auto database = silo::Database::loadDatabaseState(newVersionPath); + SPDLOG_INFO("New data version detected: {}", event.item.path()); + const std::string new_version_path = event.item.path(); + auto database = silo::Database::loadDatabaseState(new_version_path); - database_mutex.setDatabase(silo::Database::loadDatabaseState(newVersionPath)); + database_mutex.setDatabase(silo::Database::loadDatabaseState(new_version_path)); } \ No newline at end of file diff --git a/src/silo_api/info_handler.cpp b/src/silo_api/info_handler.cpp index 25cbd0c3e..617a22f31 100644 --- a/src/silo_api/info_handler.cpp +++ b/src/silo_api/info_handler.cpp @@ -89,7 +89,7 @@ void InfoHandler::get( ) { const auto request_parameter = getQueryParameter(request); - auto& fixed_database = database.getDatabase(); + const auto fixed_database = database.getDatabase(); response.set("data-version", fixed_database.database.getDataVersion().toString()); diff --git a/src/silo_api/query_handler.cpp b/src/silo_api/query_handler.cpp index 1e35df903..68310c247 100644 --- a/src/silo_api/query_handler.cpp +++ b/src/silo_api/query_handler.cpp @@ -34,9 +34,7 @@ void QueryHandler::post( try { auto fixed_database = database.getDatabase(); - silo::query_engine::QueryEngine query_engine(fixed_database.database); - - const auto query_result = query_engine.executeQuery(query); + const auto query_result = fixed_database.database.executeQuery(query); response.set("data-version", fixed_database.database.getDataVersion().toString()); diff --git a/src/silo_api/request_handler_factory.test.cpp b/src/silo_api/request_handler_factory.test.cpp index 7ed63483b..20c789da4 100644 --- a/src/silo_api/request_handler_factory.test.cpp +++ b/src/silo_api/request_handler_factory.test.cpp @@ -6,7 +6,6 @@ #include "silo/common/nucleotide_symbols.h" #include "silo/database.h" #include "silo/database_info.h" -#include "silo/query_engine/query_engine.h" #include "silo/query_engine/query_result.h" #include "silo/storage/column/date_column.h" #include "silo/storage/column/indexed_string_column.h" @@ -14,6 +13,7 @@ #include "silo/storage/column/pango_lineage_column.h" #include "silo/storage/column/string_column.h" #include "silo/storage/database_partition.h" +#include "silo_api/database_mutex.h" #include "silo_api/manual_poco_mocks.test.h" #include "silo_api/request_handler_factory.h" @@ -22,28 +22,32 @@ class MockDatabase : public silo::Database { MOCK_METHOD(silo::DatabaseInfo, getDatabaseInfo, (), (const)); MOCK_METHOD(silo::DetailedDatabaseInfo, detailedDatabaseInfo, (), (const)); MOCK_METHOD(silo::DataVersion, getDataVersion, (), (const)); + + MOCK_METHOD(silo::query_engine::QueryResult, executeQuery, (const std::string&), (const)); }; -class MockQueryEngine : public silo::query_engine::QueryEngine { +class MockDatabaseMutex : public silo_api::DatabaseMutex { public: - explicit MockQueryEngine(const silo::Database& database) - : QueryEngine(database) {} + std::shared_mutex mutex; + MockDatabase mock_database; - MOCK_METHOD(silo::query_engine::QueryResult, executeQuery, (const std::string&), (const)); + silo_api::FixedDatabase getDatabase() override { + std::shared_lock lock(mutex); + return silo_api::FixedDatabase(mock_database, std::move(lock)); + } }; class RequestHandlerTestFixture : public ::testing::Test { protected: - MockDatabase mock_database; - MockQueryEngine mock_query_engine; + MockDatabaseMutex database_mutex; silo_api::test::MockResponse response; silo_api::test::MockRequest request; silo_api::SiloRequestHandlerFactory under_test; RequestHandlerTestFixture() - : mock_query_engine(MockQueryEngine(mock_database)), + : database_mutex(), request(silo_api::test::MockRequest(response)), - under_test(silo_api::SiloRequestHandlerFactory(mock_database, mock_query_engine)) {} + under_test(database_mutex) {} void processRequest() { std::unique_ptr request_handler( @@ -54,10 +58,10 @@ class RequestHandlerTestFixture : public ::testing::Test { }; TEST_F(RequestHandlerTestFixture, handlesGetInfoRequest) { - EXPECT_CALL(mock_database, getDatabaseInfo) + EXPECT_CALL(database_mutex.mock_database, getDatabaseInfo) .WillRepeatedly(testing::Return(silo::DatabaseInfo{1, 2, 3})); - EXPECT_CALL(mock_database, getDataVersion) - .WillRepeatedly(testing::Return(silo::DataVersion("1234"))); + EXPECT_CALL(database_mutex.mock_database, getDataVersion) + .WillRepeatedly(testing::Return(silo::DataVersion::fromString("1234").value())); request.setURI("/info"); @@ -81,10 +85,10 @@ TEST_F(RequestHandlerTestFixture, handlesGetInfoRequestDetails) { const silo::DetailedDatabaseInfo detailed_database_info = {{{"main", stats}}}; - EXPECT_CALL(mock_database, detailedDatabaseInfo) + EXPECT_CALL(database_mutex.mock_database, detailedDatabaseInfo) .WillRepeatedly(testing::Return(detailed_database_info)); - EXPECT_CALL(mock_database, getDataVersion) - .WillRepeatedly(testing::Return(silo::DataVersion("1234"))); + EXPECT_CALL(database_mutex.mock_database, getDataVersion) + .WillRepeatedly(testing::Return(silo::DataVersion::fromString("1234").value())); request.setURI("/info?details=true"); @@ -117,9 +121,10 @@ TEST_F(RequestHandlerTestFixture, handlesPostQueryRequest) { {"count", 5}}; const std::vector tmp{{fields}}; const silo::query_engine::QueryResult query_result{tmp}; - EXPECT_CALL(mock_query_engine, executeQuery).WillRepeatedly(testing::Return(query_result)); - EXPECT_CALL(mock_database, getDataVersion) - .WillRepeatedly(testing::Return(silo::DataVersion("1234"))); + EXPECT_CALL(database_mutex.mock_database, executeQuery) + .WillRepeatedly(testing::Return(query_result)); + EXPECT_CALL(database_mutex.mock_database, getDataVersion) + .WillRepeatedly(testing::Return(silo::DataVersion::fromString("1234").value())); request.setMethod("POST"); request.setURI("/query"); @@ -145,7 +150,7 @@ TEST_F(RequestHandlerTestFixture, returnsMethodNotAllowedOnGetQuery) { } TEST_F(RequestHandlerTestFixture, givenRequestToUnknownUrl_thenReturnsNotFound) { - auto under_test = silo_api::SiloRequestHandlerFactory(mock_database, mock_query_engine); + auto under_test = silo_api::SiloRequestHandlerFactory(database_mutex); request.setURI("/doesNotExist");