fix: BITFIELD: overflow handling, type validation, and return values

src/server/bitops_family.cc

@@ -635,7 +635,7 @@ bool Overflow::UIntOverflow(int64_t incr, size_t total_bits, int64_t* value) con
     switch (type) {
       case Overflow::WRAP:
         // safe to do, won't overflow, both incr and value are <= than 2^63 - 1
-        *value = (incr_value + *value) % max;
+        *value = (incr_value + *value) & max;
         break;
       case Overflow::SAT:
         *value = max;
@@ -790,17 +790,19 @@ ResultType Set::ApplyTo(Overflow ov, string* bitfield) {
     return {};
   }
 
+  // First, extract the old value properly using the Get method
+  Get get(attr_);
+  auto old_value_result = get.ApplyTo(ov, &bytes);
+  int64_t old_value = old_value_result ? *old_value_result : 0;
+
   uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1;
-  int64_t old_value = 0;
 
   for (size_t i = 0; i < attr_.encoding_bit_size; ++i) {
     bool bit_value = (set_value_ >> i) & 0x01;
     uint8_t byte{GetByteValue(bytes, lsb)};
     int32_t index = GetNormalizedBitIndex(lsb);
-    int64_t old_bit = CheckBitStatus(byte, index);
     byte = bit_value ? TurnBitOn(byte, index) : TurnBitOff(byte, index);
     bytes[GetByteIndex(lsb)] = byte;
-    old_value |= old_bit << i;
     --lsb;
   }
 
@@ -964,12 +966,14 @@ nonstd::expected<CommonAttributes, string> ParseCommonAttr(CmdArgParser* parser)
   if (encoding.empty()) {
     return make_unexpected(kSyntaxErr);
   }
-  if (encoding[0] == 'U' || encoding[0] == 'u') {
+  if (encoding[0] == 'u') {
     parsed.type = EncodingType::UINT;
-  } else if (encoding[0] == 'I' || encoding[0] == 'i') {
+  } else if (encoding[0] == 'i') {
     parsed.type = EncodingType::INT;
   } else {
-    return make_unexpected(kSyntaxErr);
+    return make_unexpected(
+        "invalid bitfield type. use something like i16 u8. note that u64 is not supported but i64 "
+        "is.");
   }
 
   string_view bits = encoding.substr(1);

src/server/bitops_family_test.cc

@@ -822,4 +822,37 @@ TEST_F(BitOpsFamilyTest, BitFieldLargeOffset) {
   EXPECT_THAT(resp, 0);
 }
 
+TEST_F(BitOpsFamilyTest, BitFieldIssue5237_SetOverflowSat) {
+  Run({"set", "key:bitfield_set", "\xff\xf0\x00"});
+  auto resp = Run({"bitfield", "key:bitfield_set", "overflow", "sat", "set", "i4", "0", "8", "set",
+                   "i4", "4", "7"});
+
+  EXPECT_THAT(resp, RespArray(ElementsAre(IntArg(-1), IntArg(-1))));
+}
+
+TEST_F(BitOpsFamilyTest, BitFieldIssue5237_IncrbyCorrectness) {
+  Run({"set", "key:bitfield_incr", "\xff\xf0\x00"});
+  auto resp = Run(
+      {"bitfield", "key:bitfield_incr", "incrby", "u8", "0", "85", "incrby", "u8", "16", "170"});
+
+  EXPECT_THAT(resp, RespArray(ElementsAre(IntArg(84), IntArg(170))));
+}
+
+TEST_F(BitOpsFamilyTest, BitFieldIssue5237_InvalidTypeUppercase_Set) {
+  auto expected_error = ErrArg(
+      "ERR invalid bitfield type. use something like i16 u8. note that u64 is not supported but "
+      "i64 is.");
+
+  ASSERT_THAT(Run({"bitfield", "key:bitfield_set:wrong:args", "set", "I8", "0", "0"}),
+              expected_error);
+}
+
+TEST_F(BitOpsFamilyTest, BitFieldIssue5237_InvalidTypeUppercase_Get) {
+  auto expected_error = ErrArg(
+      "ERR invalid bitfield type. use something like i16 u8. note that u64 is not supported but "
+      "i64 is.");
+
+  ASSERT_THAT(Run({"bitfield", "key:bitfield_get:wrong:args", "get", "I8", "0"}), expected_error);
+}
+
 }  // end of namespace dfly