fix: field without @ sign in the FT.AGGREGATE command error message fixed (#4955)

fixed: #4935

Author: vyavdoshenko

src/server/search/search_family.cc

@@ -390,16 +390,19 @@ ParseResult<SearchParams> ParseSearchParams(CmdArgParser* parser) {
   return params;
 }
 
-std::optional<aggregate::SortParams> ParseAggregatorSortParams(CmdArgParser* parser) {
+ParseResult<aggregate::SortParams> ParseAggregatorSortParams(CmdArgParser* parser) {
   size_t strings_num = parser->Next<size_t>();
 
   aggregate::SortParams sort_params;
   sort_params.fields.reserve(strings_num / 2);
 
   while (parser->HasNext() && strings_num > 0) {
+    std::string_view potential_field =
+        parser->Peek();  // Peek to get the field name for potential error message
     std::optional<std::string_view> parsed_field = ParseFieldWithAtSign(parser);
     if (!parsed_field) {
-      return std::nullopt;
+      return CreateSyntaxError(
+          absl::StrCat("SORTBY field name '", potential_field, "' must start with '@'"));
     }
     strings_num--;
 
@@ -416,7 +419,7 @@ std::optional<aggregate::SortParams> ParseAggregatorSortParams(CmdArgParser* par
   }
 
   if (strings_num) {
-    return std::nullopt;
+    return CreateSyntaxError("bad arguments for SORTBY: specified invalid number of strings"sv);
   }
 
   if (parser->Check("MAX")) {
@@ -487,7 +490,7 @@ ParseResult<AggregateParams> ParseAggregatorParams(CmdArgParser* parser) {
     if (parser->Check("SORTBY")) {
       auto sort_params = ParseAggregatorSortParams(parser);
       if (!sort_params) {
-        return CreateSyntaxError("bad arguments for SORTBY: specified invalid number of strings"sv);
+        return make_unexpected(sort_params.error());  // Propagate the specific error
       }
 
       params.steps.push_back(aggregate::MakeSortStep(std::move(sort_params).value()));

src/server/search/search_family_test.cc

@@ -1060,7 +1060,7 @@ TEST_F(SearchFamilyTest, AggregateGroupByReduceSort) {
                   "SORTBY", "1", "count"});
   // clang-format on
 
-  EXPECT_THAT(resp, ErrArg("bad arguments for SORTBY: specified invalid number of strings"));
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'count' must start with '@'"));
 }
 
 TEST_F(SearchFamilyTest, AggregateLoadGroupBy) {
@@ -1700,7 +1700,7 @@ TEST_F(SearchFamilyTest, AggregateResultFields) {
   EXPECT_THAT(resp, IsUnordArrayWithSize(IsMap("a", "1"), IsMap("a", "4"), IsMap("a", "7")));
   absl::SetFlag(&FLAGS_search_reject_legacy_field, true);
   resp = Run({"FT.AGGREGATE", "i1", "*", "SORTBY", "1", "a"});
-  EXPECT_THAT(resp, ErrArg("bad arguments for SORTBY: specified invalid number of strings"));
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'a' must start with '@'"));
 
   absl::SetFlag(&FLAGS_search_reject_legacy_field, false);
   resp = Run({"FT.AGGREGATE", "i1", "*", "LOAD", "1", "@b", "SORTBY", "1", "a"});
@@ -1709,7 +1709,7 @@ TEST_F(SearchFamilyTest, AggregateResultFields) {
                                    IsMap("b", "\"8\"", "a", "7")));
   absl::SetFlag(&FLAGS_search_reject_legacy_field, true);
   resp = Run({"FT.AGGREGATE", "i1", "*", "LOAD", "1", "@b", "SORTBY", "1", "a"});
-  EXPECT_THAT(resp, ErrArg("bad arguments for SORTBY: specified invalid number of strings"));
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'a' must start with '@'"));
 
   absl::SetFlag(&FLAGS_search_reject_legacy_field, false);
   resp = Run({"FT.AGGREGATE", "i1", "*", "SORTBY", "1", "a", "GROUPBY", "2", "@b", "@a", "REDUCE",
@@ -1720,7 +1720,7 @@ TEST_F(SearchFamilyTest, AggregateResultFields) {
   absl::SetFlag(&FLAGS_search_reject_legacy_field, true);
   resp = Run({"FT.AGGREGATE", "i1", "*", "SORTBY", "1", "a", "GROUPBY", "2", "@b", "@a", "REDUCE",
               "COUNT", "0", "AS", "count"});
-  EXPECT_THAT(resp, ErrArg("bad arguments for SORTBY: specified invalid number of strings"));
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'a' must start with '@'"));
 
   Run({"JSON.SET", "j4", ".", R"({"id":1, "number":4})"});
   Run({"JSON.SET", "j5", ".", R"({"id":2})"});
@@ -1879,6 +1879,29 @@ TEST_F(SearchFamilyTest, AggregateSortByParsingErrors) {
   EXPECT_THAT(resp, ErrArg(kInvalidIntErr));
 }
 
+TEST_F(SearchFamilyTest, AggregateSortByParsingErrorsWithoutAt) {
+  Run({"JSON.SET", "j1", "$", R"({"name": "first", "number": 1200, "group": "first"})"});
+
+  Run({"FT.CREATE", "index", "ON", "JSON", "SCHEMA", "$.name", "AS", "name", "TEXT", "$.number",
+       "AS", "number", "NUMERIC", "$.group", "AS", "group", "TAG"});
+
+  // Test SORTBY with field name without '@'
+  auto resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "1", "name"});
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'name' must start with '@'"));
+
+  // Test SORTBY with field name without '@' and multiple sort fields
+  resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "3", "name", "@number", "DESC"});
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'name' must start with '@'"));
+
+  // Test SORTBY with field name without '@' and MAX option
+  resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "1", "name", "MAX", "1"});
+  EXPECT_THAT(resp, ErrArg("SORTBY field name 'name' must start with '@'"));
+
+  // Check that the old error still works for wrong number of args
+  resp = Run({"FT.AGGREGATE", "index", "*", "SORTBY", "2", "@name"});
+  EXPECT_THAT(resp, ErrArg("bad arguments for SORTBY: specified invalid number of strings"));
+}
+
 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",