chore: improve compatibility of EXPIRE functions with Redis (#2696)

* chore: improve compatibility of EXPIRE functions with Redis Also, provide a module name if stumbled upon module data that can not be loaded by dragonfly. --------- Signed-off-by: Roman Gershman <[email protected]>
dragonflydb · Mar 12, 2024 · d709f2a · d709f2a
1 parent 4a9f816
commit d709f2a
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 20 deletions.
diff --git a/src/server/common.h b/src/server/common.h
@@ -23,7 +23,8 @@ namespace dfly {
 enum class ListDir : uint8_t { LEFT, RIGHT };
 
 // Dependent on ExpirePeriod representation of the value.
-constexpr int64_t kMaxExpireDeadlineSec = (1u << 28) - 1;
+constexpr int64_t kMaxExpireDeadlineSec = (1u << 28) - 1;  // 8.5 years
+constexpr int64_t kMaxExpireDeadlineMs = kMaxExpireDeadlineSec * 1000;
 
 using DbIndex = uint16_t;
 using ShardId = uint16_t;

diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc
@@ -178,7 +178,7 @@ class RestoreArgs {
 [[nodiscard]] bool RestoreArgs::UpdateExpiration(int64_t now_msec) {
   if (HasExpiration()) {
     int64_t ttl = abs_time_ ? expiration_ - now_msec : expiration_;
-    if (ttl > kMaxExpireDeadlineSec * 1000)
+    if (ttl > kMaxExpireDeadlineMs)
       return false;
 
     expiration_ = ttl < 0 ? -1 : ttl + now_msec;
@@ -743,11 +743,13 @@ void GenericFamily::Expire(CmdArgList args, ConnectionContext* cntx) {
     return cntx->SendError(kInvalidIntErr);
   }
 
-  if (int_arg > kMaxExpireDeadlineSec || int_arg < -kMaxExpireDeadlineSec) {
-    return cntx->SendError(InvalidExpireTime(cntx->cid->name()));
+  int_arg = std::max<int64_t>(int_arg, -1);
+
+  // silently cap the expire time to kMaxExpireDeadlineSec which is more than 8 years.
+  if (int_arg > kMaxExpireDeadlineSec) {
+    int_arg = kMaxExpireDeadlineSec;
   }
 
-  int_arg = std::max<int64_t>(int_arg, -1);
   auto expire_options = ParseExpireOptionsOrReply(args.subspan(2), cntx);
   if (!expire_options) {
     return;
@@ -851,7 +853,13 @@ void GenericFamily::Pexpire(CmdArgList args, ConnectionContext* cntx) {
   if (!absl::SimpleAtoi(msec, &int_arg)) {
     return cntx->SendError(kInvalidIntErr);
   }
-  int_arg = std::max<int64_t>(int_arg, 0L);
+  int_arg = std::max<int64_t>(int_arg, -1);
+
+  // to be more compatible with redis, we silently cap the expire time to kMaxExpireDeadlineSec
+  if (int_arg > kMaxExpireDeadlineMs) {
+    int_arg = kMaxExpireDeadlineMs;
+  }
+
   auto expire_options = ParseExpireOptionsOrReply(args.subspan(2), cntx);
   if (!expire_options) {
     return;

diff --git a/src/server/rdb_load.cc b/src/server/rdb_load.cc
@@ -203,6 +203,25 @@ int ziplistPairsConvertAndValidateIntegrity(const uint8_t* zl, size_t size, unsi
   return ret;
 }
 
+string ModuleTypeName(uint64_t module_id) {
+  static const char ModuleNameSet[] =
+      "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+      "abcdefghijklmnopqrstuvwxyz"
+      "0123456789-_";
+
+  char name[10];
+
+  name[9] = '\0';
+  char* p = name + 8;
+  module_id >>= 10;
+  for (int j = 0; j < 9; j++) {
+    *p-- = ModuleNameSet[module_id & 63];
+    module_id >>= 6;
+  }
+
+  return string{name};
+}
+
 }  // namespace
 
 class DecompressImpl {
@@ -1978,7 +1997,11 @@ error_code RdbLoader::Load(io::Source* src) {
     }
 
     if (type == RDB_OPCODE_MODULE_AUX) {
-      LOG(ERROR) << "Modules are not supported";
+      uint64_t module_id;
+      SET_OR_RETURN(LoadLen(nullptr), module_id);
+      string module_name = ModuleTypeName(module_id);
+
+      LOG(ERROR) << "Modules are not supported, error loading module " << module_name;
       return RdbError(errc::feature_not_supported);
     }
 

diff --git a/src/server/string_family.cc b/src/server/string_family.cc
@@ -739,16 +739,16 @@ void StringFamily::Set(CmdArgList args, ConnectionContext* cntx) {
           return builder->SendStored();
         }
       }
-      if (!is_ms && int_arg >= kMaxExpireDeadlineSec) {
-        return builder->SendError(InvalidExpireTime("set"));
-      }
-
-      if (!is_ms) {
+      if (is_ms) {
+        if (int_arg > kMaxExpireDeadlineMs) {
+          int_arg = kMaxExpireDeadlineMs;
+        }
+      } else {
+        if (int_arg > kMaxExpireDeadlineSec) {
+          int_arg = kMaxExpireDeadlineSec;
+        }
         int_arg *= 1000;
       }
-      if (int_arg >= kMaxExpireDeadlineSec * 1000) {
-        return builder->SendError(InvalidExpireTime("set"));
-      }
       sparams.expire_after_ms = int_arg;
     } else if (cur_arg == "NX" && !(sparams.flags & SetCmd::SET_IF_EXISTS)) {
       sparams.flags |= SetCmd::SET_IF_NOTEXIST;
@@ -1147,22 +1147,29 @@ void StringFamily::SetExGeneric(bool seconds, CmdArgList args, ConnectionContext
   string_view key = ArgS(args, 0);
   string_view ex = ArgS(args, 1);
   string_view value = ArgS(args, 2);
-  int32_t unit_vals;
+  int64_t unit_vals;
 
   if (!absl::SimpleAtoi(ex, &unit_vals)) {
     return cntx->SendError(kInvalidIntErr);
   }
 
-  if (unit_vals < 1 || unit_vals >= kMaxExpireDeadlineSec) {
+  if (unit_vals < 1) {
     return cntx->SendError(InvalidExpireTime(cntx->cid->name()));
   }
 
   SetCmd::SetParams sparams;
   sparams.flags |= SetCmd::SET_EXPIRE_AFTER_MS;
-  if (seconds)
+  if (seconds) {
+    if (unit_vals > kMaxExpireDeadlineSec) {
+      unit_vals = kMaxExpireDeadlineSec;
+    }
     sparams.expire_after_ms = uint64_t(unit_vals) * 1000;
-  else
+  } else {
+    if (unit_vals > kMaxExpireDeadlineMs) {
+      unit_vals = kMaxExpireDeadlineMs;
+    }
     sparams.expire_after_ms = unit_vals;
+  }
 
   auto cb = [&](Transaction* t, EngineShard* shard) {
     SetCmd sg(t->GetOpArgs(shard), true);

diff --git a/src/server/string_family_test.cc b/src/server/string_family_test.cc
@@ -451,7 +451,8 @@ TEST_F(StringFamilyTest, SetEx) {
   ASSERT_THAT(Run({"ttl", "key"}), IntArg(10));
   ASSERT_THAT(Run({"setex", "key", "0", "val"}), ErrArg("invalid expire time"));
   ASSERT_EQ(Run({"setex", "key", StrCat(5 * 365 * 24 * 3600), "val"}), "OK");
-  ASSERT_THAT(Run({"setex", "key", StrCat(1 << 30), "val"}), ErrArg("invalid expire time"));
+  ASSERT_THAT(Run({"setex", "key", StrCat(1 << 30), "val"}), "OK");
+  ASSERT_THAT(Run({"ttl", "key"}), IntArg(kMaxExpireDeadlineSec));
 }
 
 TEST_F(StringFamilyTest, Range) {