From 3f5386eb77480dbe2f59339e287990d4973198bf Mon Sep 17 00:00:00 2001 From: Krunal Gandhi Date: Fri, 1 Mar 2024 05:15:03 +0000 Subject: [PATCH] Override pagination (#1588) * add limit and offset support for collection * use req->params, dont restrict limit param when records are less * add more checks and not sort with pagination * minor updations * pagination support for overrides --- include/collection.h | 7 +- src/collection.cpp | 38 ++++++++++ src/collection_manager.cpp | 6 +- src/core_api.cpp | 46 +++++++++--- test/collection_manager_test.cpp | 10 +-- test/collection_override_test.cpp | 114 ++++++++++++++++++++++++++++-- test/core_api_utils_test.cpp | 72 +++++++++++++++++++ 7 files changed, 263 insertions(+), 30 deletions(-) diff --git a/include/collection.h b/include/collection.h index 1d59e050..3a120807 100644 --- a/include/collection.h +++ b/include/collection.h @@ -620,10 +620,9 @@ public: Option remove_override(const std::string & id); - std::map get_overrides() { - std::shared_lock lock(mutex); - return overrides; - }; + Option> get_overrides(uint32_t limit=0, uint32_t offset=0); + + Option get_override(const std::string& override_id); // synonym operations diff --git a/src/collection.cpp b/src/collection.cpp index 7ff1e91c..a8f254c5 100644 --- a/src/collection.cpp +++ b/src/collection.cpp @@ -4712,6 +4712,44 @@ void Collection::synonym_reduction(const std::vector& tokens, return synonym_index->synonym_reduction(tokens, results); } +Option Collection::get_override(const std::string& override_id) { + std::shared_lock lock(mutex); + + if(overrides.count(override_id) == 0) { + return Option(404, "override " + override_id + " not found."); + } + + return Option(overrides.at(override_id)); +} + +Option> Collection::get_overrides(uint32_t limit, uint32_t offset) { + std::shared_lock lock(mutex); + std::map overrides_map; + + auto overrides_it = overrides.begin(); + + if(offset > 0) { + if(offset >= overrides.size()) { + return Option>(400, "Invalid offset param."); + } + + std::advance(overrides_it, offset); + } + + auto overrides_end = overrides.end(); + + if(limit > 0 && (offset + limit < overrides.size())) { + overrides_end = overrides_it; + std::advance(overrides_end, limit); + } + + for (overrides_it; overrides_it != overrides_end; ++overrides_it) { + overrides_map[overrides_it->first] = &overrides_it->second; + } + + return Option>(overrides_map); +} + spp::sparse_hash_map Collection::get_synonyms() { std::shared_lock lock(mutex); return synonym_index->get_synonyms(); diff --git a/src/collection_manager.cpp b/src/collection_manager.cpp index b4b29e5f..825a7295 100644 --- a/src/collection_manager.cpp +++ b/src/collection_manager.cpp @@ -2351,9 +2351,9 @@ Option CollectionManager::clone_collection(const string& existing_n } // copy overrides - auto overrides = existing_coll->get_overrides(); - for(const auto& override: overrides) { - new_coll->add_override(override.second); + auto overrides = existing_coll->get_overrides().get(); + for(const auto& kv: overrides) { + new_coll->add_override(*kv.second); } return Option(new_coll); diff --git a/src/core_api.cpp b/src/core_api.cpp index d54ccf60..f9c88b0b 100644 --- a/src/core_api.cpp +++ b/src/core_api.cpp @@ -1697,13 +1697,38 @@ bool get_overrides(const std::shared_ptr& req, const std::shared_ptrparams.count("offset") != 0) { + const auto &offset_str = req->params["offset"]; + if(!StringUtils::is_uint32_t(offset_str)) { + res->set(400, "Offset param should be unsigned integer."); + return false; + } + offset = std::stoi(offset_str); + } + + if(req->params.count("limit") != 0) { + const auto &limit_str = req->params["limit"]; + if(!StringUtils::is_uint32_t(limit_str)) { + res->set(400, "Limit param should be unsigned integer."); + return false; + } + limit = std::stoi(limit_str); + } + nlohmann::json res_json; res_json["overrides"] = nlohmann::json::array(); - const std::map& overrides = collection->get_overrides(); - for(const auto & kv: overrides) { - nlohmann::json override = kv.second.to_json(); - res_json["overrides"].push_back(override); + auto overrides_op = collection->get_overrides(limit, offset); + if(!overrides_op.ok()) { + res->set(overrides_op.code(), overrides_op.error()); + return false; + } + + const auto overrides = overrides_op.get(); + + for(const auto &kv: overrides) { + res_json["overrides"].push_back(kv.second->to_json()); } res->set_200(res_json.dump()); @@ -1721,16 +1746,15 @@ bool get_override(const std::shared_ptr& req, const std::shared_ptrparams["id"]; - const std::map& overrides = collection->get_overrides(); + auto overrides_op = collection->get_override(override_id); - if(overrides.count(override_id) != 0) { - nlohmann::json override = overrides.at(override_id).to_json(); - res->set_200(override.dump()); - return true; + if(!overrides_op.ok()) { + res->set(overrides_op.code(), overrides_op.error()); + return false; } - res->set_404(); - return false; + res->set_200(overrides_op.get().to_json()); + return true; } bool put_override(const std::shared_ptr& req, const std::shared_ptr& res) { diff --git a/test/collection_manager_test.cpp b/test/collection_manager_test.cpp index cf65003f..76d90215 100644 --- a/test/collection_manager_test.cpp +++ b/test/collection_manager_test.cpp @@ -521,9 +521,9 @@ TEST_F(CollectionManagerTest, RestoreRecordsOnRestart) { ASSERT_TRUE(collection1->get_enable_nested_fields()); - ASSERT_EQ(2, collection1->get_overrides().size()); - ASSERT_STREQ("exclude-rule", collection1->get_overrides()["exclude-rule"].id.c_str()); - ASSERT_STREQ("include-rule", collection1->get_overrides()["include-rule"].id.c_str()); + ASSERT_EQ(2, collection1->get_overrides().get().size()); + ASSERT_STREQ("exclude-rule", collection1->get_overrides().get()["exclude-rule"]->id.c_str()); + ASSERT_STREQ("include-rule", collection1->get_overrides().get()["include-rule"]->id.c_str()); const auto& synonyms = collection1->get_synonyms(); ASSERT_EQ(2, synonyms.size()); @@ -1409,7 +1409,7 @@ TEST_F(CollectionManagerTest, CloneCollection) { ASSERT_EQ("coll2", coll2->get_name()); ASSERT_EQ(1, coll2->get_fields().size()); ASSERT_EQ(1, coll2->get_synonyms().size()); - ASSERT_EQ(1, coll2->get_overrides().size()); + ASSERT_EQ(1, coll2->get_overrides().get().size()); ASSERT_EQ("", coll2->get_fallback_field_type()); ASSERT_EQ(1, coll2->get_symbols_to_index().size()); @@ -2023,4 +2023,4 @@ TEST_F(CollectionManagerTest, CollectionPagination) { collection_op = collectionManager.get_collections(limit, offset); ASSERT_FALSE(collection_op.ok()); ASSERT_EQ("Invalid offset param.", collection_op.error()); -} +} \ No newline at end of file diff --git a/test/collection_override_test.cpp b/test/collection_override_test.cpp index 0b826043..9f5d1aaf 100644 --- a/test/collection_override_test.cpp +++ b/test/collection_override_test.cpp @@ -331,9 +331,9 @@ TEST_F(CollectionOverrideTest, IncludeHitsFilterOverrides) { override_t::parse(override_json_include, "", override_include); coll_mul_fields->add_override(override_include); - std::map overrides = coll_mul_fields->get_overrides(); + std::map overrides = coll_mul_fields->get_overrides().get(); ASSERT_EQ(1, overrides.size()); - auto override_json = overrides["include-rule"].to_json(); + auto override_json = overrides.at("include-rule")->to_json(); ASSERT_TRUE(override_json.contains("filter_curated_hits")); ASSERT_TRUE(override_json["filter_curated_hits"].get()); @@ -440,9 +440,9 @@ TEST_F(CollectionOverrideTest, ExcludeIncludeFacetFilterQuery) { coll_mul_fields->add_override(override_include); - std::map overrides = coll_mul_fields->get_overrides(); + std::map overrides = coll_mul_fields->get_overrides().get(); ASSERT_EQ(1, overrides.size()); - auto override_json = overrides["include-rule"].to_json(); + auto override_json = overrides.at("include-rule")->to_json(); ASSERT_FALSE(override_json.contains("filter_by")); ASSERT_TRUE(override_json.contains("remove_matched_tokens")); ASSERT_TRUE(override_json.contains("filter_curated_hits")); @@ -520,7 +520,7 @@ TEST_F(CollectionOverrideTest, ExcludeIncludeFacetFilterQuery) { // should be able to replace existing override override_include.rule.query = "found"; coll_mul_fields->add_override(override_include); - ASSERT_STREQ("found", coll_mul_fields->get_overrides()["include-rule"].rule.query.c_str()); + ASSERT_STREQ("found", coll_mul_fields->get_overrides().get()["include-rule"]->rule.query.c_str()); coll_mul_fields->remove_override("include-rule"); } @@ -2450,9 +2450,9 @@ TEST_F(CollectionOverrideTest, DynamicFilteringWithSynonyms) { ASSERT_TRUE(op.ok()); coll1->add_override(override1); - std::map overrides = coll1->get_overrides(); + std::map overrides = coll1->get_overrides().get(); ASSERT_EQ(1, overrides.size()); - auto override_json = overrides["dynamic-filters"].to_json(); + auto override_json = overrides.at("dynamic-filters")->to_json(); ASSERT_EQ("category: {category}", override_json["filter_by"].get()); ASSERT_EQ(true, override_json["remove_matched_tokens"].get()); // must be true by default @@ -4023,3 +4023,103 @@ TEST_F(CollectionOverrideTest, WildcardSearchOverride) { collectionManager.drop_collection("coll1"); } + +TEST_F(CollectionOverrideTest, OverridesPagination) { + Collection *coll2; + + std::vector fields = {field("title", field_types::STRING, false), + field("points", field_types::INT32, false)}; + + coll2 = collectionManager.get_collection("coll2").get(); + if(coll2 == nullptr) { + coll2 = collectionManager.create_collection("coll2", 1, fields, "points").get(); + } + + for(int i = 0; i < 5; ++i) { + nlohmann::json override_json = { + {"id", "override"}, + { + "rule", { + {"query", "not-found"}, + {"match", override_t::MATCH_EXACT} + } + }, + {"metadata", { {"foo", "bar"}}}, + }; + + override_json["id"] = override_json["id"].get() + std::to_string(i + 1); + override_t override; + override_t::parse(override_json, "", override); + + coll2->add_override(override); + } + + uint32_t limit = 0, offset = 0, i = 0; + + //limit collections by 2 + limit=2; + auto override_op = coll2->get_overrides(limit); + auto override_map = override_op.get(); + ASSERT_EQ(2, override_map.size()); + i=offset; + for(const auto &kv : override_map) { + ASSERT_EQ("override" + std::to_string(i+1), kv.second->id); + ++i; + } + + //get 2 collection from offset 3 + offset=3; + override_op = coll2->get_overrides(limit, offset); + override_map = override_op.get(); + ASSERT_EQ(2, override_map.size()); + i=offset; + for(const auto &kv : override_map) { + ASSERT_EQ("override" + std::to_string(i+1), kv.second->id); + ++i; + } + + //get all collection except first + offset=1; limit=0; + override_op = coll2->get_overrides(limit, offset); + override_map = override_op.get(); + ASSERT_EQ(4, override_map.size()); + i=offset; + for(const auto &kv : override_map) { + ASSERT_EQ("override" + std::to_string(i+1), kv.second->id); + ++i; + } + + //get last collection + offset=4, limit=1; + override_op = coll2->get_overrides(limit, offset); + override_map = override_op.get(); + ASSERT_EQ(1, override_map.size()); + ASSERT_EQ("override5", override_map.begin()->second->id); + + //if limit is greater than number of collection then return all from offset + offset=0; limit=8; + override_op = coll2->get_overrides(limit, offset); + override_map = override_op.get(); + ASSERT_EQ(5, override_map.size()); + i=offset; + for(const auto &kv : override_map) { + ASSERT_EQ("override" + std::to_string(i+1), kv.second->id); + ++i; + } + + offset=3; limit=4; + override_op = coll2->get_overrides(limit, offset); + override_map = override_op.get(); + ASSERT_EQ(2, override_map.size()); + i=offset; + for(const auto &kv : override_map) { + ASSERT_EQ("override" + std::to_string(i+1), kv.second->id); + ++i; + } + + //invalid offset + offset=6; limit=0; + override_op = coll2->get_overrides(limit, offset); + ASSERT_FALSE(override_op.ok()); + ASSERT_EQ("Invalid offset param.", override_op.error()); +} \ No newline at end of file diff --git a/test/core_api_utils_test.cpp b/test/core_api_utils_test.cpp index cfb725d9..c88a1dd9 100644 --- a/test/core_api_utils_test.cpp +++ b/test/core_api_utils_test.cpp @@ -1622,6 +1622,78 @@ TEST_F(CoreAPIUtilsTest, CollectionsPagination) { ASSERT_EQ(400, resp->status_code); ASSERT_EQ("{\"message\": \"Offset param should be unsigned integer.\"}", resp->body); + //invalid limit string + req->params["offset"] = "0"; + req->params["limit"] = "-1"; + get_collections(req, resp); + ASSERT_EQ(400, resp->status_code); + ASSERT_EQ("{\"message\": \"Limit param should be unsigned integer.\"}", resp->body); +} + +TEST_F(CoreAPIUtilsTest, OverridesPagination) { + Collection *coll2; + + std::vector fields = {field("title", field_types::STRING, false), + field("points", field_types::INT32, false)}; + + coll2 = collectionManager.get_collection("coll2").get(); + if(coll2 == nullptr) { + coll2 = collectionManager.create_collection("coll2", 1, fields, "points").get(); + } + + for(int i = 0; i < 5; ++i) { + nlohmann::json override_json = { + {"id", "override"}, + { + "rule", { + {"query", "not-found"}, + {"match", override_t::MATCH_EXACT} + } + }, + {"metadata", { {"foo", "bar"}}}, + }; + + override_json["id"] = override_json["id"].get() + std::to_string(i + 1); + override_t override; + override_t::parse(override_json, "", override); + + coll2->add_override(override); + } + + auto req = std::make_shared(); + auto resp = std::make_shared(nullptr); + + req->params["collection"] = "coll2"; + req->params["offset"] = "0"; + req->params["limit"] = "1"; + + get_overrides(req, resp); + nlohmann::json expected_json = R"({ + "overrides":[ + { + "excludes":[], + "filter_curated_hits":false, + "id":"override1", + "includes":[], + "metadata":{"foo":"bar"}, + "remove_matched_tokens":false, + "rule":{ + "match":"exact", + "query":"not-found" + }, + "stop_processing":true + }] + })"_json; + + ASSERT_EQ(expected_json.dump(), resp->body); + + //invalid offset string + req->params["offset"] = "0a"; + get_collections(req, resp); + + ASSERT_EQ(400, resp->status_code); + ASSERT_EQ("{\"message\": \"Offset param should be unsigned integer.\"}", resp->body); + //invalid limit string req->params["offset"] = "0"; req->params["limit"] = "-1";