fix(coordinator): return error when INFIELDS exceeds max instead of silently dropping

When InfieldsFromGRPC received more than kMaxTextFieldsCount fields, it
logged a warning and returned std::nullopt. This caused the filter parser
to interpret the result as "search all text fields", silently broadening
the search scope instead of restricting it as the caller requested.

In a version-skewed cluster where a newer coordinator encodes more than
64 fields, each shard would run an unrestricted full-text scan and return
incorrect results with no error surfaced to the client.

Change InfieldsFromGRPC to return absl::StatusOr and propagate an
InvalidArgumentError when the limit is exceeded, consistent with the
local FT.SEARCH parser path (ft_search_parser.cc).

Signed-off-by: Riley Des <riley.desserre@improving.com>

Author: rileydes-improving

src/coordinator/search_converter.cc

@@ -25,7 +25,6 @@
 #include "src/query/predicate.h"
 #include "src/query/search.h"
 #include "src/schema_manager.h"
-#include "vmsdk/src/log.h"
 #include "vmsdk/src/managed_pointers.h"
 #include "vmsdk/src/status/status_macros.h"
 #include "vmsdk/src/type_conversions.h"
@@ -44,17 +43,15 @@ void SortByToGRPC(const std::optional<query::SortByParameter>& sortby,
                        : coordinator::SORT_ORDER_DESCENDING);
 }
 
-std::optional<absl::flat_hash_set<std::string>> InfieldsFromGRPC(
-    const SearchIndexPartitionRequest& request) {
+absl::StatusOr<std::optional<absl::flat_hash_set<std::string>>>
+InfieldsFromGRPC(const SearchIndexPartitionRequest& request) {
   if (request.infields().empty()) {
     return std::nullopt;
   }
   if (static_cast<size_t>(request.infields().size()) > kMaxTextFieldsCount) {
-    VMSDK_LOG(WARNING, nullptr)
-        << "INFIELDS count (" << request.infields().size()
-        << ") exceeds maximum supported (" << kMaxTextFieldsCount
-        << "), ignoring INFIELDS";
-    return std::nullopt;
+    return absl::InvalidArgumentError(absl::StrCat(
+        "INFIELDS count (", request.infields().size(),
+        ") exceeds maximum supported (", kMaxTextFieldsCount, ")"));
   }
   absl::flat_hash_set<std::string> infields;
   infields.reserve(request.infields().size());
@@ -274,7 +271,7 @@ absl::Status GRPCSearchRequestToParameters(
   parameters->filter_parse_results.query_operations =
       static_cast<QueryOperations>(request.query_operations());
   parameters->sortby_parameter = SortByFromGRPC(request);
-  parameters->infields = InfieldsFromGRPC(request);
+  VMSDK_ASSIGN_OR_RETURN(parameters->infields, InfieldsFromGRPC(request));
   return absl::OkStatus();
 }
 

src/coordinator/search_converter.h

@@ -34,9 +34,10 @@ void SortByToGRPC(const std::optional<query::SortByParameter>& sortby,
 
 // Decodes the infields repeated field from a proto request into an optional
 // set. Returns nullopt when no infields are present on the wire (same semantics
-// as an unset optional in SearchParameters).
-std::optional<absl::flat_hash_set<std::string>> InfieldsFromGRPC(
-    const SearchIndexPartitionRequest& request);
+// as an unset optional in SearchParameters). Returns an error if the infields
+// count exceeds kMaxTextFieldsCount.
+absl::StatusOr<std::optional<absl::flat_hash_set<std::string>>>
+InfieldsFromGRPC(const SearchIndexPartitionRequest& request);
 
 }  // namespace valkey_search::coordinator
 

testing/search_converter_test.cc

@@ -12,9 +12,11 @@
 #include <string>
 
 #include "absl/container/flat_hash_set.h"
+#include "absl/status/status.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "src/coordinator/coordinator.pb.h"
+#include "src/index_schema.h"
 #include "src/query/search.h"
 #include "testing/common.h"
 #include "vmsdk/src/testing_infra/utils.h"
@@ -35,7 +37,8 @@ absl::flat_hash_set<std::string> SerializeAndReadBackInfields(
 
   auto request = ParametersToGRPCSearchRequest(params);
   auto decoded = InfieldsFromGRPC(*request);
-  return decoded.value_or(absl::flat_hash_set<std::string>{});
+  EXPECT_TRUE(decoded.ok());
+  return decoded->value_or(absl::flat_hash_set<std::string>{});
 }
 
 TEST_F(SearchConverterInfieldsTest, SerializeSingleField) {
@@ -50,7 +53,9 @@ TEST_F(SearchConverterInfieldsTest, UnsetIsNotSerialized) {
 
   auto request = ParametersToGRPCSearchRequest(params);
   EXPECT_EQ(request->infields_size(), 0);
-  EXPECT_FALSE(InfieldsFromGRPC(*request).has_value());
+  auto result = InfieldsFromGRPC(*request);
+  ASSERT_TRUE(result.ok());
+  EXPECT_FALSE(result->has_value());
 }
 
 // An engaged-but-empty set collapses to the same wire bytes as unset. This is
@@ -62,7 +67,9 @@ TEST_F(SearchConverterInfieldsTest, EngagedEmptyCollapsesToUnset) {
 
   auto request = ParametersToGRPCSearchRequest(params);
   EXPECT_EQ(request->infields_size(), 0);
-  EXPECT_FALSE(InfieldsFromGRPC(*request).has_value());
+  auto result = InfieldsFromGRPC(*request);
+  ASSERT_TRUE(result.ok());
+  EXPECT_FALSE(result->has_value());
 }
 
 TEST_F(SearchConverterInfieldsTest, WireRoundTrip) {
@@ -81,5 +88,15 @@ TEST_F(SearchConverterInfieldsTest, WireRoundTrip) {
   EXPECT_EQ(deserialized, *params.infields);
 }
 
+TEST_F(SearchConverterInfieldsTest, ExceedsMaxFieldsReturnsError) {
+  coordinator::SearchIndexPartitionRequest request;
+  for (size_t i = 0; i <= kMaxTextFieldsCount; ++i) {
+    request.add_infields("field_" + std::to_string(i));
+  }
+  auto result = InfieldsFromGRPC(request);
+  EXPECT_FALSE(result.ok());
+  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
+}
+
 }  // namespace
 }  // namespace valkey_search