Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(search_family): Add options test for the FT.SEARCH command #4478

Merged
merged 1 commit into from
Jan 30, 2025
Merged

fix(search_family): Add options test for the FT.SEARCH command #4478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/server/search/doc_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ struct SearchParams {
Only one of load_fields and return_fields should be set.
*/
std::optional<SearchFieldsList> load_fields;
bool no_content = false;

std::optional<search::SortOption> sort_option;
search::QueryParams query_params;
Expand All @@ -157,7 +158,7 @@ struct SearchParams {
}

bool IdsOnly() const {
return return_fields && return_fields->empty();
return no_content || (return_fields && return_fields->empty());
}

bool ShouldReturnField(std::string_view alias) const;
Expand Down
9 changes: 4 additions & 5 deletions src/server/search/search_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ void ParseLoadFields(CmdArgParser* parser, std::optional<SearchFieldsList>* load
load_fields->emplace();
}

while (num_fields--) {
while (parser->HasNext() && num_fields--) {
string_view str = parser->Next();

if (absl::StartsWith(str, "@"sv)) {
Expand Down Expand Up @@ -274,7 +274,7 @@ optional<SearchParams> ParseSearchParamsOrReply(CmdArgParser* parser, SinkReplyB
* AS a */
size_t num_fields = parser->Next<size_t>();
params.return_fields.emplace();
while (params.return_fields->size() < num_fields) {
while (parser->HasNext() && params.return_fields->size() < num_fields) {
StringOrView name = StringOrView::FromString(parser->Next<std::string>());

if (parser->Check("AS")) {
Expand All @@ -285,8 +285,7 @@ optional<SearchParams> ParseSearchParamsOrReply(CmdArgParser* parser, SinkReplyB
}
}
} else if (parser->Check("NOCONTENT")) { // NOCONTENT
params.load_fields.emplace();
params.return_fields.emplace();
params.no_content = true;
} else if (parser->Check("PARAMS")) { // [PARAMS num(ignored) name(ignored) knn_vector]
params.query_params = ParseQueryParams(parser);
} else if (parser->Check("SORTBY")) {
Expand Down Expand Up @@ -360,7 +359,7 @@ optional<AggregateParams> ParseAggregatorParamsOrReply(CmdArgParser parser,

std::vector<std::string> fields;
fields.reserve(num_fields);
while (num_fields > 0 && parser.HasNext()) {
while (parser.HasNext() && num_fields > 0) {
auto parsed_field = ParseFieldWithAtSign(&parser);

/*
Expand Down
75 changes: 68 additions & 7 deletions src/server/search/search_family_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@

#include "base/gtest.h"
#include "base/logging.h"
#include "facade/error.h"
#include "facade/facade_test.h"
#include "server/command_registry.h"
#include "server/test_utils.h"

using namespace testing;
using namespace std;
using namespace util;
using namespace facade;

namespace dfly {

Expand Down Expand Up @@ -628,10 +630,10 @@ TEST_F(SearchFamilyTest, TestReturn) {

// Check no fields are returned
resp = Run({"ft.search", "i1", "@justA:0", "return", "0"});
EXPECT_THAT(resp, RespArray(ElementsAre(IntArg(1), "k0")));
EXPECT_THAT(resp, IsArray(IntArg(1), "k0"));

resp = Run({"ft.search", "i1", "@justA:0", "nocontent"});
EXPECT_THAT(resp, RespArray(ElementsAre(IntArg(1), "k0")));
EXPECT_THAT(resp, IsArray(IntArg(1), "k0"));

// Check only one field is returned (and with original identifier)
resp = Run({"ft.search", "i1", "@justA:0", "return", "1", "longA"});
Expand Down Expand Up @@ -856,7 +858,7 @@ TEST_F(SearchFamilyTest, FtProfileErrorReply) {
EXPECT_THAT(resp, ErrArg("no `SEARCH` or `AGGREGATE` provided"));

resp = Run({"ft.profile", "i1", "search", "not_query", "(a | b) c d"});
EXPECT_THAT(resp, ErrArg("syntax error"));
EXPECT_THAT(resp, ErrArg(kSyntaxErr));

resp = Run({"ft.profile", "non_existent_key", "search", "query", "(a | b) c d"});
EXPECT_THAT(resp, ErrArg("non_existent_key: no such index"));
Expand Down Expand Up @@ -1803,15 +1805,15 @@ TEST_F(SearchFamilyTest, AggregateSortByParsingErrors) {

// Test SORTBY with negative argument count
resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "-3", "@name", "@number", "DESC"});
EXPECT_THAT(resp, ErrArg("value is not an integer or out of range"));
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test MAX with invalid value
resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "1", "@name", "MAX", "-10"});
EXPECT_THAT(resp, ErrArg("value is not an integer or out of range"));
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test MAX without a value
resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "1", "@name", "MAX"});
EXPECT_THAT(resp, ErrArg("syntax error"));
EXPECT_THAT(resp, ErrArg(kSyntaxErr));

// Test SORTBY with a non-existing field
/* Temporary unsupported
Expand All @@ -1820,7 +1822,66 @@ TEST_F(SearchFamilyTest, AggregateSortByParsingErrors) {

// Test SORTBY with an invalid value
resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "notvalue", "@name"});
EXPECT_THAT(resp, ErrArg("value is not an integer or out of range"));
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));
}

TEST_F(SearchFamilyTest, InvalidSearchOptions) {
Run({"JSON.SET", "j1", ".", R"({"field1":"first","field2":"second"})"});
Run({"FT.CREATE", "idx", "ON", "JSON", "SCHEMA", "$.field1", "AS", "field1", "TEXT", "$.field2",
"AS", "field2", "TEXT"});

/* Test with an empty query and LOAD. TODO: Add separate test for query syntax
auto resp = Run({"FT.SEARCH", "idx", "", "LOAD", "1", "@field1"});
EXPECT_THAT(resp, IsMapWithSize()); */

// Test with LIMIT missing arguments
auto resp = Run({"FT.SEARCH", "idx", "*", "LIMIT", "0"});
EXPECT_THAT(resp, ErrArg(kSyntaxErr));

// Test with LIMIT exceeding the maximum allowed value
resp = Run({"FT.SEARCH", "idx", "*", "LIMIT", "0", "100000000000000000000"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with LIMIT and negative arguments
resp = Run({"FT.SEARCH", "idx", "*", "LIMIT", "-1", "10"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with LIMIT and invalid argument types
resp = Run({"FT.SEARCH", "idx", "*", "LIMIT", "start", "count"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with invalid LOAD arguments
resp = Run({"FT.SEARCH", "idx", "*", "LOAD", "@field1", "@field2"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with duplicate fields in LOAD
resp = Run({"FT.SEARCH", "idx", "*", "LOAD", "4", "@field1", "@field1", "@field2", "@field2"});
EXPECT_THAT(resp, IsMapWithSize("j1", IsMap("field1", "\"first\"", "field2", "\"second\"", "$",
R"({"field1":"first","field2":"second"})")));

// Test with LOAD exceeding maximum allowed count
resp = Run({"FT.SEARCH", "idx", "*", "LOAD", "100000000000000000000", "@field1", "@field2"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with invalid RETURN syntax (missing count)
resp = Run({"FT.SEARCH", "idx", "*", "RETURN", "@field1", "@field2"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with RETURN having duplicate fields
resp = Run({"FT.SEARCH", "idx", "*", "RETURN", "4", "field1", "field1", "field2", "field2"});
EXPECT_THAT(resp, IsMapWithSize("j1", IsMap("field1", "\"first\"", "field2", "\"second\"")));

// Test with RETURN exceeding maximum allowed count
resp = Run({"FT.SEARCH", "idx", "*", "RETURN", "100000000000000000000", "@field1", "@field2"});
EXPECT_THAT(resp, ErrArg(kInvalidIntErr));

// Test with NOCONTENT and LOAD
resp = Run({"FT.SEARCH", "idx", "*", "NOCONTENT", "LOAD", "2", "@field1", "@field2"});
EXPECT_THAT(resp, IsArray(IntArg(1), "j1"));

// Test with NOCONTENT and RETURN
resp = Run({"FT.SEARCH", "idx", "*", "NOCONTENT", "RETURN", "2", "@field1", "@field2"});
EXPECT_THAT(resp, IsArray(IntArg(1), "j1"));
}

} // namespace dfly
Loading