From d71f726c42963d55809605b4dc4144970ca0f230 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Tue, 16 May 2023 14:11:38 +1000 Subject: [PATCH] Add pagination to database queries --- src/database/database.cpp | 219 ++++++++++++++++++++++------ src/database/include/database.hpp | 81 +++++----- src/database/include/song.hpp | 11 +- src/database/song.cpp | 6 + src/database/test/test_database.cpp | 129 ++++++++++------ src/main/app_console.cpp | 29 ++-- 6 files changed, 325 insertions(+), 150 deletions(-) diff --git a/src/database/database.cpp b/src/database/database.cpp index df10ebb9..f5fe5240 100644 --- a/src/database/database.cpp +++ b/src/database/database.cpp @@ -2,9 +2,11 @@ #include +#include #include #include #include +#include #include #include "esp_log.h" @@ -216,6 +218,43 @@ auto Database::Update() -> std::future { }); } +auto Database::GetSongs(std::size_t page_size) -> std::future*> { + return RunOnDbTask*>([=, this]() -> Result* { + Continuation c{.iterator = nullptr, + .prefix = CreateDataPrefix().data, + .start_key = CreateDataPrefix().data, + .forward = true, + .was_prev_forward = true, + .page_size = page_size}; + return dbGetPage(c); + }); +} + +auto Database::GetDump(std::size_t page_size) + -> std::future*> { + return RunOnDbTask*>([=, this]() -> Result* { + Continuation c{.iterator = nullptr, + .prefix = "", + .start_key = "", + .forward = true, + .was_prev_forward = true, + .page_size = page_size}; + return dbGetPage(c); + }); +} + +template +auto Database::GetPage(Continuation* c) -> std::future*> { + Continuation copy = *c; + return RunOnDbTask*>( + [=, this]() -> Result* { return dbGetPage(copy); }); +} + +template auto Database::GetPage(Continuation* c) + -> std::future*>; +template auto Database::GetPage(Continuation* c) + -> std::future*>; + auto Database::dbMintNewSongId() -> SongId { SongId next_id = 1; std::string val; @@ -287,37 +326,148 @@ auto Database::dbPutSong(SongId id, dbPutHash(hash, id); } -auto parse_song(ITagParser* parser, - const leveldb::Slice& key, - const leveldb::Slice& value) -> std::optional { - std::optional data = ParseDataValue(value); - if (!data) { +template +auto Database::dbGetPage(const Continuation& c) -> Result* { + // Work out our starting point. Sometimes this will already done. + leveldb::Iterator* it = nullptr; + if (c.iterator != nullptr) { + it = c.iterator->release(); + } + if (it == nullptr) { + it = db_->NewIterator(leveldb::ReadOptions()); + it->Seek(c.start_key); + } + + // Fix off-by-one if we just changed direction. + if (c.forward != c.was_prev_forward) { + if (c.forward) { + it->Next(); + } else { + it->Prev(); + } + } + + // Grab results. + std::optional first_key; + std::vector records; + while (records.size() < c.page_size && it->Valid()) { + if (!it->key().starts_with(c.prefix)) { + break; + } + if (!first_key) { + first_key = it->key().ToString(); + } + std::optional parsed = ParseRecord(it->key(), it->value()); + if (parsed) { + records.push_back(*parsed); + } + if (c.forward) { + it->Next(); + } else { + it->Prev(); + } + } + + std::unique_ptr iterator(it); + if (iterator != nullptr) { + if (!iterator->Valid() || !it->key().starts_with(c.prefix)) { + iterator.reset(); + } + } + + // Put results into canonical order if we were iterating backwards. + if (!c.forward) { + std::reverse(records.begin(), records.end()); + } + + // Work out the new continuations. + std::optional> next_page; + if (c.forward) { + if (iterator != nullptr) { + // We were going forward, and now we want the next page. Re-use the + // existing iterator, and point the start key at it. + std::string key = iterator->key().ToString(); + next_page = Continuation{ + .iterator = std::make_shared>( + std::move(iterator)), + .prefix = c.prefix, + .start_key = key, + .forward = true, + .was_prev_forward = true, + .page_size = c.page_size, + }; + } + // No iterator means we ran out of results in this direction. + } else { + // We were going backwards, and now we want the next page. This is a + // reversal, to set the start key to the first record we saw and mark that + // it's off by one. + next_page = Continuation{ + .iterator = nullptr, + .prefix = c.prefix, + .start_key = *first_key, + .forward = true, + .was_prev_forward = false, + .page_size = c.page_size, + }; + } + + std::optional> prev_page; + if (c.forward) { + // We were going forwards, and now we want the previous page. Set the search + // key to the first result we saw, and mark that it's off by one. + prev_page = Continuation{ + .iterator = nullptr, + .prefix = c.prefix, + .start_key = *first_key, + .forward = false, + .was_prev_forward = true, + .page_size = c.page_size, + }; + } else { + if (iterator != nullptr) { + // We were going backwards, and we still want to go backwards. The + // iterator is still valid. + std::string key = iterator->key().ToString(); + prev_page = Continuation{ + .iterator = std::make_shared>( + std::move(iterator)), + .prefix = c.prefix, + .start_key = key, + .forward = false, + .was_prev_forward = false, + .page_size = c.page_size, + }; + } + // No iterator means we ran out of results in this direction. + } + + return new Result(std::move(records), next_page, prev_page); +} + +template auto Database::dbGetPage(const Continuation& c) + -> Result*; +template auto Database::dbGetPage( + const Continuation& c) -> Result*; + +template <> +auto Database::ParseRecord(const leveldb::Slice& key, + const leveldb::Slice& val) + -> std::optional { + std::optional data = ParseDataValue(val); + if (!data || data->is_tombstoned()) { return {}; } SongTags tags; - if (!parser->ReadAndParseTags(data->filepath(), &tags)) { + if (!tag_parser_->ReadAndParseTags(data->filepath(), &tags)) { return {}; } return Song(*data, tags); } -auto Database::GetSongs(std::size_t page_size) -> std::future> { - return RunOnDbTask>([=, this]() -> Result { - return Query(CreateDataPrefix().slice, page_size, - std::bind_front(&parse_song, tag_parser_)); - }); -} - -auto Database::GetMoreSongs(std::size_t page_size, Continuation c) - -> std::future> { - leveldb::Iterator* it = c.release(); - return RunOnDbTask>([=, this]() -> Result { - return Query(it, page_size, - std::bind_front(&parse_song, tag_parser_)); - }); -} - -auto parse_dump(const leveldb::Slice& key, const leveldb::Slice& value) +template <> +auto Database::ParseRecord(const leveldb::Slice& key, + const leveldb::Slice& val) -> std::optional { std::ostringstream stream; stream << "key: "; @@ -335,33 +485,14 @@ auto parse_dump(const leveldb::Slice& key, const leveldb::Slice& value) << static_cast(str[i]); } } - for (std::size_t i = 2; i < str.size(); i++) { - } } stream << "\tval: 0x"; - std::string str = value.ToString(); - for (int i = 0; i < value.size(); i++) { + std::string str = val.ToString(); + for (int i = 0; i < val.size(); i++) { stream << std::hex << std::setfill('0') << std::setw(2) << static_cast(str[i]); } return stream.str(); } -auto Database::GetDump(std::size_t page_size) - -> std::future> { - leveldb::Iterator* it = db_->NewIterator(leveldb::ReadOptions()); - it->SeekToFirst(); - return RunOnDbTask>([=, this]() -> Result { - return Query(it, page_size, &parse_dump); - }); -} - -auto Database::GetMoreDump(std::size_t page_size, Continuation c) - -> std::future> { - leveldb::Iterator* it = c.release(); - return RunOnDbTask>([=, this]() -> Result { - return Query(it, page_size, &parse_dump); - }); -} - } // namespace database diff --git a/src/database/include/database.hpp b/src/database/include/database.hpp index 29872e8d..da0ed083 100644 --- a/src/database/include/database.hpp +++ b/src/database/include/database.hpp @@ -22,7 +22,15 @@ namespace database { -typedef std::unique_ptr Continuation; +template +struct Continuation { + std::shared_ptr> iterator; + std::string prefix; + std::string start_key; + bool forward; + bool was_prev_forward; + size_t page_size; +}; /* * Wrapper for a set of results from the database. Owns the list of results, as @@ -32,29 +40,23 @@ typedef std::unique_ptr Continuation; template class Result { public: - auto values() -> std::vector* { return values_.release(); } - auto continuation() -> Continuation { return std::move(c_); } - auto HasMore() -> bool { return c_->Valid(); } - - Result(std::vector* values, Continuation c) - : values_(values), c_(std::move(c)) {} - - Result(std::unique_ptr> values, Continuation c) - : values_(std::move(values)), c_(std::move(c)) {} + auto values() const -> const std::vector& { return values_; } - Result(Result&& other) - : values_(move(other.values_)), c_(std::move(other.c_)) {} + auto next_page() -> std::optional>& { return next_page_; } + auto prev_page() -> std::optional>& { return prev_page_; } - Result operator=(Result&& other) { - return Result(other.values(), std::move(other.continuation())); - } + Result(const std::vector&& values, + std::optional> next, + std::optional> prev) + : values_(values), next_page_(next), prev_page_(prev) {} Result(const Result&) = delete; Result& operator=(const Result&) = delete; private: - std::unique_ptr> values_; - Continuation c_; + std::vector values_; + std::optional> next_page_; + std::optional> prev_page_; }; class Database { @@ -73,13 +75,11 @@ class Database { auto Update() -> std::future; - auto GetSongs(std::size_t page_size) -> std::future>; - auto GetMoreSongs(std::size_t page_size, Continuation c) - -> std::future>; + auto GetSongs(std::size_t page_size) -> std::future*>; + auto GetDump(std::size_t page_size) -> std::future*>; - auto GetDump(std::size_t page_size) -> std::future>; - auto GetMoreDump(std::size_t page_size, Continuation c) - -> std::future>; + template + auto GetPage(Continuation* c) -> std::future*>; Database(const Database&) = delete; Database& operator=(const Database&) = delete; @@ -110,31 +110,20 @@ class Database { -> void; template - using Parser = std::function(const leveldb::Slice& key, - const leveldb::Slice& value)>; - - template - auto Query(const leveldb::Slice& prefix, - std::size_t max_results, - Parser parser) -> Result { - leveldb::Iterator* it = db_->NewIterator(leveldb::ReadOptions()); - it->Seek(prefix); - return Query(it, max_results, parser); - } + auto dbGetPage(const Continuation& c) -> Result*; template - auto Query(leveldb::Iterator* it, std::size_t max_results, Parser parser) - -> Result { - auto results = std::make_unique>(); - for (std::size_t i = 0; i < max_results && it->Valid(); i++) { - std::optional r = std::invoke(parser, it->key(), it->value()); - if (r) { - results->push_back(*r); - } - it->Next(); - } - return {std::move(results), std::unique_ptr(it)}; - } + auto ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) + -> std::optional; }; +template <> +auto Database::ParseRecord(const leveldb::Slice& key, + const leveldb::Slice& val) + -> std::optional; +template <> +auto Database::ParseRecord(const leveldb::Slice& key, + const leveldb::Slice& val) + -> std::optional; + } // namespace database diff --git a/src/database/include/song.hpp b/src/database/include/song.hpp index e51e5587..9a84e124 100644 --- a/src/database/include/song.hpp +++ b/src/database/include/song.hpp @@ -4,6 +4,7 @@ #include #include +#include #include "leveldb/db.h" #include "span.hpp" @@ -133,16 +134,20 @@ class SongData { */ class Song { public: - Song(SongData data, SongTags tags) : data_(data), tags_(tags) {} + Song(const SongData& data, const SongTags& tags) : data_(data), tags_(tags) {} + Song(const Song& other) = default; - auto data() -> const SongData& { return data_; } - auto tags() -> const SongTags& { return tags_; } + auto data() const -> const SongData& { return data_; } + auto tags() const -> const SongTags& { return tags_; } bool operator==(const Song&) const = default; + Song operator=(const Song& other) const { return Song(other); } private: const SongData data_; const SongTags tags_; }; +void swap(Song& first, Song& second); + } // namespace database diff --git a/src/database/song.cpp b/src/database/song.cpp index a0e6ce4b..b6a1baac 100644 --- a/src/database/song.cpp +++ b/src/database/song.cpp @@ -36,4 +36,10 @@ auto SongData::Exhume(const std::string& new_path) const -> SongData { return SongData(id_, new_path, tags_hash_, play_count_, false); } +void swap(Song& first, Song& second) { + Song temp = first; + first = second; + second = temp; +} + } // namespace database diff --git a/src/database/test/test_database.cpp b/src/database/test/test_database.cpp index d61421ee..a88c4bb3 100644 --- a/src/database/test/test_database.cpp +++ b/src/database/test/test_database.cpp @@ -60,8 +60,8 @@ TEST_CASE("song database", "[integration]") { std::unique_ptr db(open_res.value()); SECTION("empty database") { - std::unique_ptr> res(db->GetSongs(10).get().values()); - REQUIRE(res->size() == 0); + std::unique_ptr> res(db->GetSongs(10).get()); + REQUIRE(res->values().size() == 0); } SECTION("add new songs") { @@ -71,24 +71,23 @@ TEST_CASE("song database", "[integration]") { db->Update(); - std::unique_ptr> res(db->GetSongs(10).get().values()); - REQUIRE(res->size() == 3); - CHECK(*res->at(0).tags().title == "Song 1"); - CHECK(res->at(0).data().id() == 1); - CHECK(*res->at(1).tags().title == "Song 2"); - CHECK(res->at(1).data().id() == 2); - CHECK(*res->at(2).tags().title == "Song 3"); - CHECK(res->at(2).data().id() == 3); + std::unique_ptr> res(db->GetSongs(10).get()); + REQUIRE(res->values().size() == 3); + CHECK(*res->values().at(0).tags().title == "Song 1"); + CHECK(res->values().at(0).data().id() == 1); + CHECK(*res->values().at(1).tags().title == "Song 2"); + CHECK(res->values().at(1).data().id() == 2); + CHECK(*res->values().at(2).tags().title == "Song 3"); + CHECK(res->values().at(2).data().id() == 3); SECTION("update with no filesystem changes") { db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - REQUIRE(new_res->size() == 3); - CHECK(res->at(0) == new_res->at(0)); - CHECK(res->at(1) == new_res->at(1)); - CHECK(res->at(2) == new_res->at(2)); + std::unique_ptr> new_res(db->GetSongs(10).get()); + REQUIRE(new_res->values().size() == 3); + CHECK(res->values().at(0) == new_res->values().at(0)); + CHECK(res->values().at(1) == new_res->values().at(1)); + CHECK(res->values().at(2) == new_res->values().at(2)); } SECTION("update with all songs gone") { @@ -96,19 +95,17 @@ TEST_CASE("song database", "[integration]") { db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - CHECK(new_res->size() == 0); + std::unique_ptr> new_res(db->GetSongs(10).get()); + CHECK(new_res->values().size() == 0); SECTION("update with one song returned") { songs.MakeSong("song2.wav", "Song 2"); db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - REQUIRE(new_res->size() == 1); - CHECK(res->at(1) == new_res->at(0)); + std::unique_ptr> new_res(db->GetSongs(10).get()); + REQUIRE(new_res->values().size() == 1); + CHECK(res->values().at(1) == new_res->values().at(0)); } } @@ -117,11 +114,10 @@ TEST_CASE("song database", "[integration]") { db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - REQUIRE(new_res->size() == 2); - CHECK(res->at(0) == new_res->at(0)); - CHECK(res->at(2) == new_res->at(1)); + std::unique_ptr> new_res(db->GetSongs(10).get()); + REQUIRE(new_res->values().size() == 2); + CHECK(res->values().at(0) == new_res->values().at(0)); + CHECK(res->values().at(2) == new_res->values().at(1)); } SECTION("update with tags changed") { @@ -129,14 +125,14 @@ TEST_CASE("song database", "[integration]") { db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - REQUIRE(new_res->size() == 3); - CHECK(res->at(0) == new_res->at(0)); - CHECK(res->at(1) == new_res->at(1)); - CHECK(*new_res->at(2).tags().title == "The Song 3"); + std::unique_ptr> new_res(db->GetSongs(10).get()); + REQUIRE(new_res->values().size() == 3); + CHECK(res->values().at(0) == new_res->values().at(0)); + CHECK(res->values().at(1) == new_res->values().at(1)); + CHECK(*new_res->values().at(2).tags().title == "The Song 3"); // The id should not have changed, since this was just a tag update. - CHECK(res->at(2).data().id() == new_res->at(2).data().id()); + CHECK(res->values().at(2).data().id() == + new_res->values().at(2).data().id()); } SECTION("update with one new song") { @@ -144,14 +140,61 @@ TEST_CASE("song database", "[integration]") { db->Update(); - std::unique_ptr> new_res( - db->GetSongs(10).get().values()); - REQUIRE(new_res->size() == 4); - CHECK(res->at(0) == new_res->at(0)); - CHECK(res->at(1) == new_res->at(1)); - CHECK(res->at(2) == new_res->at(2)); - CHECK(*new_res->at(3).tags().title == "Song 1 (nightcore remix)"); - CHECK(new_res->at(3).data().id() == 4); + std::unique_ptr> new_res(db->GetSongs(10).get()); + REQUIRE(new_res->values().size() == 4); + CHECK(res->values().at(0) == new_res->values().at(0)); + CHECK(res->values().at(1) == new_res->values().at(1)); + CHECK(res->values().at(2) == new_res->values().at(2)); + CHECK(*new_res->values().at(3).tags().title == + "Song 1 (nightcore remix)"); + CHECK(new_res->values().at(3).data().id() == 4); + } + + SECTION("get songs with pagination") { + std::unique_ptr> res(db->GetSongs(1).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 1); + REQUIRE(res->next_page()); + + res.reset(db->GetPage(&res->next_page().value()).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 2); + REQUIRE(res->next_page()); + + res.reset(db->GetPage(&res->next_page().value()).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 3); + REQUIRE(!res->next_page()); + + SECTION("page backwards") { + REQUIRE(res->prev_page()); + + res.reset(db->GetPage(&res->prev_page().value()).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 2); + REQUIRE(res->prev_page()); + + res.reset(db->GetPage(&res->prev_page().value()).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 1); + REQUIRE(!res->prev_page()); + + SECTION("page forwards again") { + REQUIRE(res->next_page()); + + res.reset(db->GetPage(&res->next_page().value()).get()); + + REQUIRE(res->values().size() == 1); + CHECK(res->values().at(0).data().id() == 2); + CHECK(res->next_page()); + CHECK(res->prev_page()); + } + } } } } diff --git a/src/main/app_console.cpp b/src/main/app_console.cpp index 759afa91..a85faaf0 100644 --- a/src/main/app_console.cpp +++ b/src/main/app_console.cpp @@ -176,15 +176,15 @@ int CmdDbSongs(int argc, char** argv) { return 1; } - database::Result res = - sInstance->database_->GetSongs(20).get(); + std::unique_ptr> res( + sInstance->database_->GetSongs(5).get()); while (true) { - std::unique_ptr> r(res.values()); - for (database::Song s : *r) { + for (database::Song s : res->values()) { std::cout << s.tags().title.value_or("[BLANK]") << std::endl; } - if (res.HasMore()) { - res = sInstance->database_->GetMoreSongs(10, res.continuation()).get(); + if (res->next_page()) { + auto continuation = res->next_page().value(); + res.reset(sInstance->database_->GetPage(&continuation).get()); } else { break; } @@ -211,17 +211,18 @@ int CmdDbDump(int argc, char** argv) { std::cout << "=== BEGIN DUMP ===" << std::endl; - database::Result res = sInstance->database_->GetDump(20).get(); + std::unique_ptr> res( + sInstance->database_->GetDump(5).get()); while (true) { - std::unique_ptr> r(res.values()); - if (r == nullptr) { - break; - } - for (std::string s : *r) { + for (std::string s : res->values()) { std::cout << s << std::endl; } - if (res.HasMore()) { - res = sInstance->database_->GetMoreDump(20, res.continuation()).get(); + if (res->next_page()) { + auto continuation = res->next_page().value(); + res.reset( + sInstance->database_->GetPage(&continuation).get()); + } else { + break; } }