From ba57145c53b40b92b59d6548950fa7bf8c74161c Mon Sep 17 00:00:00 2001
From: Diskein <kalantaevskii@gmail.com>
Date: Sat, 12 Oct 2024 09:55:01 +0200
Subject: [PATCH] fix!: fix BITPOS command responses (#3893) (#3910)

BITPOS returns 0 for non-existent keys according to Redis's
implmentation.

BITPOS allows only 0 and 1 as the bit mode argument.

Signed-off-by: Denis K <kalantaevskii@gmail.com>
---
 src/server/bitops_family.cc      | 12 +++++++++---
 src/server/bitops_family_test.cc | 11 +++++++++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc
index 043b7a49e..48449efc7 100644
--- a/src/server/bitops_family.cc
+++ b/src/server/bitops_family.cc
@@ -548,6 +548,8 @@ void BitPos(CmdArgList args, ConnectionContext* cntx) {
 
   if (!absl::SimpleAtoi(ArgS(args, 1), &value)) {
     return cntx->SendError(kInvalidIntErr);
+  } else if (value != 0 && value != 1) {
+    return cntx->SendError("The bit argument must be 1 or 0");
   }
 
   if (args.size() >= 3) {
@@ -1340,11 +1342,15 @@ OpResult<int64_t> FindFirstBitWithValue(const OpArgs& op_args, std::string_view
                                         int64_t start, int64_t end, bool as_bit) {
   OpResult<std::string> value = ReadValue(op_args.db_cntx, key, op_args.shard);
 
-  std::string_view value_str;
-  if (value) {  // non-existent keys are treated as empty strings, per Redis
-    value_str = value.value();
+  // non-existent keys are handled exactly as in Redis's implementation,
+  // even though it contradicts its docs:
+  //     If a clear bit isn't found in the specified range, the function returns -1
+  //     as the user specified a clear range and there are no 0 bits in that range
+  if (!value) {
+    return bit_value ? -1 : 0;
   }
 
+  std::string_view value_str = value.value();
   int64_t size = value_str.size();
   if (as_bit) {
     size *= OFFSET_FACTOR;
diff --git a/src/server/bitops_family_test.cc b/src/server/bitops_family_test.cc
index 8eaf15dc9..17bcca6a0 100644
--- a/src/server/bitops_family_test.cc
+++ b/src/server/bitops_family_test.cc
@@ -555,8 +555,15 @@ TEST_F(BitOpsFamilyTest, BitPos) {
   EXPECT_EQ(-1, CheckedInt({"bitpos", "empty", "0"}));
   EXPECT_EQ(-1, CheckedInt({"bitpos", "empty", "0", "1"}));
 
-  // Non-existent key should be treated like an empty string.
-  EXPECT_EQ(-1, CheckedInt({"bitpos", "d", "0"}));
+  // Non-existent key should be treated like padded with zeros string.
+  EXPECT_EQ(-1, CheckedInt({"bitpos", "d", "1"}));
+  EXPECT_EQ(0, CheckedInt({"bitpos", "d", "0"}));
+
+  // Make sure we accept only 0 and 1 for the bit mode arguement.
+  const auto argument_must_be_0_or_1_error = ErrArg("ERR The bit argument must be 1 or 0");
+  ASSERT_THAT(Run({"bitpos", "d", "2"}), argument_must_be_0_or_1_error);
+  ASSERT_THAT(Run({"bitpos", "d", "42"}), argument_must_be_0_or_1_error);
+  ASSERT_THAT(Run({"bitpos", "d", "-1"}), argument_must_be_0_or_1_error);
 }
 
 TEST_F(BitOpsFamilyTest, BitFieldParsing) {