fix: Fix unsupported object type rejson-rl in RedisInsight (#3384)

* fix: Fix unsupported object type rejson-rl in RedisInsight

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

* fix(generic_family): fix case for the TYPE option in SCAN command

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

* feat(generic_family_test): Add test for the Redis GUI

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

* refactor: address comments

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

* refactor: address comments 2

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

* refactor: change variable name from obj_type_as_string to obj_type

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

---------

Signed-off-by: Stepan Bagritsevich <bagr.stepan@gmail.com>

src/core/compact_object.cc

@@ -621,7 +621,7 @@ uint64_t CompactObj::HashCode(string_view str) {
   return XXH3_64bits_withSeed(str.data(), str.size(), kHashSeed);
 }
 
-unsigned CompactObj::ObjType() const {
+CompactObjType CompactObj::ObjType() const {
   if (IsInline() || taglen_ == INT_TAG || taglen_ == SMALL_TAG || taglen_ == EXTERNAL_TAG)
     return OBJ_STRING;
 
@@ -637,30 +637,7 @@ unsigned CompactObj::ObjType() const {
   }
 
   LOG(FATAL) << "TBD " << int(taglen_);
-  return 0;
-}
-
-string_view CompactObj::ObjTypeToString(unsigned type) {
-#define OBJECT_TYPE_CASE(type) \
-  case type:                   \
-    return absl::StripPrefix(#type, "OBJ_")
-
-  switch (type) {
-    OBJECT_TYPE_CASE(OBJ_STRING);
-    OBJECT_TYPE_CASE(OBJ_LIST);
-    OBJECT_TYPE_CASE(OBJ_SET);
-    OBJECT_TYPE_CASE(OBJ_ZSET);
-    OBJECT_TYPE_CASE(OBJ_HASH);
-    OBJECT_TYPE_CASE(OBJ_MODULE);
-    OBJECT_TYPE_CASE(OBJ_STREAM);
-    OBJECT_TYPE_CASE(OBJ_JSON);
-    OBJECT_TYPE_CASE(OBJ_SBF);
-    default:
-      DCHECK(false) << "Unknown object type " << type;
-      return "OTHER";
-  }
-
-#undef OBJECT_TYPE_CASE
+  return kInvalidCompactObjType;
 }
 
 unsigned CompactObj::Encoding() const {
@@ -674,7 +651,7 @@ unsigned CompactObj::Encoding() const {
   }
 }
 
-void CompactObj::InitRobj(unsigned type, unsigned encoding, void* obj) {
+void CompactObj::InitRobj(CompactObjType type, unsigned encoding, void* obj) {
   DCHECK_NE(type, OBJ_STRING);
   SetMeta(ROBJ_TAG, mask_);
   u_.r_obj.Init(type, encoding, obj);
@@ -1255,4 +1232,29 @@ MemoryResource* CompactObj::memory_resource() {
   return tl.local_mr;
 }
 
+constexpr std::pair<CompactObjType, std::string_view> kObjTypeToString[8] = {
+    {OBJ_STRING, "string"sv},  {OBJ_LIST, "list"sv},    {OBJ_SET, "set"sv},
+    {OBJ_ZSET, "zset"sv},      {OBJ_HASH, "hash"sv},    {OBJ_STREAM, "stream"sv},
+    {OBJ_JSON, "ReJSON-RL"sv}, {OBJ_SBF, "MBbloom--"sv}};
+
+std::string_view ObjTypeToString(CompactObjType type) {
+  for (auto& p : kObjTypeToString) {
+    if (type == p.first) {
+      return p.second;
+    }
+  }
+
+  LOG(DFATAL) << "Unsupported type " << type;
+  return "Invalid type"sv;
+}
+
+std::optional<CompactObjType> ObjTypeFromString(std::string_view sv) {
+  for (auto& p : kObjTypeToString) {
+    if (absl::EqualsIgnoreCase(sv, p.second)) {
+      return p.first;
+    }
+  }
+  return std::nullopt;
+}
+
 }  // namespace dfly

src/core/compact_object.h

@@ -97,6 +97,10 @@ struct TieredColdRecord;
 
 }  // namespace detail
 
+using CompactObjType = unsigned;
+
+constexpr CompactObjType kInvalidCompactObjType = std::numeric_limits<CompactObjType>::max();
+
 class CompactObj {
   static constexpr unsigned kInlineLen = 16;
 
@@ -268,9 +272,7 @@ class CompactObj {
   }
 
   unsigned Encoding() const;
-  unsigned ObjType() const;
-
-  static std::string_view ObjTypeToString(unsigned type);
+  CompactObjType ObjType() const;
 
   void* RObjPtr() const {
     return u_.r_obj.inner_obj();
@@ -282,7 +284,7 @@ class CompactObj {
 
   // takes ownership over obj_inner.
   // type should not be OBJ_STRING.
-  void InitRobj(unsigned type, unsigned encoding, void* obj_inner);
+  void InitRobj(CompactObjType type, unsigned encoding, void* obj_inner);
 
   // For STR object.
   void SetInt(int64_t val);
@@ -536,6 +538,10 @@ class CompactObjectView {
   CompactObj obj_;
 };
 
+std::string_view ObjTypeToString(CompactObjType type);
+
+std::optional<CompactObjType> ObjTypeFromString(std::string_view sv);
+
 namespace detail {
 
 struct TieredColdRecord : public ::boost::intrusive::list_base_hook<

src/server/common.cc

@@ -132,31 +132,6 @@ const char* GlobalStateName(GlobalState s) {
   ABSL_UNREACHABLE();
 }
 
-const char* ObjTypeName(int type) {
-  switch (type) {
-    case OBJ_STRING:
-      return "string";
-    case OBJ_LIST:
-      return "list";
-    case OBJ_SET:
-      return "set";
-    case OBJ_ZSET:
-      return "zset";
-    case OBJ_HASH:
-      return "hash";
-    case OBJ_STREAM:
-      return "stream";
-    case OBJ_JSON:
-      return "rejson-rl";
-    case OBJ_SBF:
-      return "MBbloom--";
-
-    default:
-      LOG(ERROR) << "Unsupported type " << type;
-  }
-  return "invalid";
-};
-
 const char* RdbTypeName(unsigned type) {
   switch (type) {
     case RDB_TYPE_STRING:
@@ -318,8 +293,11 @@ OpResult<ScanOpts> ScanOpts::TryFrom(CmdArgList args) {
       if (scan_opts.pattern == "*")
         scan_opts.pattern = string_view{};
     } else if (opt == "TYPE") {
-      ToLower(&args[i + 1]);
-      scan_opts.type_filter = ArgS(args, i + 1);
+      auto obj_type = ObjTypeFromString(ArgS(args, i + 1));
+      if (!obj_type) {
+        return facade::OpStatus::SYNTAX_ERR;
+      }
+      scan_opts.type_filter = obj_type;
     } else if (opt == "BUCKET") {
       if (!absl::SimpleAtoi(ArgS(args, i + 1), &scan_opts.bucket_id)) {
         return facade::OpStatus::INVALID_INT;

src/server/common.h

@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "core/compact_object.h"
 #include "facade/facade_types.h"
 #include "facade/op_status.h"
 #include "util/fibers/fibers.h"
@@ -121,7 +122,6 @@ inline void ToLower(const MutableSlice* val) {
 
 bool ParseHumanReadableBytes(std::string_view str, int64_t* num_bytes);
 bool ParseDouble(std::string_view src, double* value);
-const char* ObjTypeName(int type);
 
 const char* RdbTypeName(unsigned type);
 
@@ -295,7 +295,7 @@ class Context : protected Cancellation {
 struct ScanOpts {
   std::string_view pattern;
   size_t limit = 10;
-  std::string_view type_filter;
+  std::optional<CompactObjType> type_filter;
   unsigned bucket_id = UINT_MAX;
 
   bool Matches(std::string_view val_name) const;

src/server/debugcmd.cc

@@ -16,6 +16,7 @@ extern "C" {
 
 #include "base/flags.h"
 #include "base/logging.h"
+#include "core/compact_object.h"
 #include "core/string_map.h"
 #include "server/blocking_controller.h"
 #include "server/container_utils.h"
@@ -908,7 +909,7 @@ void DebugCmd::ObjHist() {
   absl::StrAppend(&result, "___begin object histogram___\n\n");
 
   for (auto& [obj_type, hist_ptr] : obj_hist_map_arr[0]) {
-    StrAppend(&result, "OBJECT:", ObjTypeName(obj_type), "\n");
+    StrAppend(&result, "OBJECT:", ObjTypeToString(obj_type), "\n");
     StrAppend(&result, "________________________________________________________________\n");
     StrAppend(&result, "Key length histogram:\n", hist_ptr->key_len.ToString(), "\n");
     StrAppend(&result, "Value length histogram:\n", hist_ptr->val_len.ToString(), "\n");

src/server/generic_family.cc

@@ -551,7 +551,7 @@ bool ScanCb(const OpArgs& op_args, PrimeIterator prime_it, const ScanOpts& opts,
   if (!IsValid(it))
     return false;
 
-  bool matches = opts.type_filter.empty() || ObjTypeName(it->second.ObjType()) == opts.type_filter;
+  bool matches = !opts.type_filter || it->second.ObjType() == opts.type_filter;
 
   if (!matches)
     return false;
@@ -1386,7 +1386,7 @@ void GenericFamily::Dump(CmdArgList args, ConnectionContext* cntx) {
 void GenericFamily::Type(CmdArgList args, ConnectionContext* cntx) {
   std::string_view key = ArgS(args, 0);
 
-  auto cb = [&](Transaction* t, EngineShard* shard) -> OpResult<int> {
+  auto cb = [&](Transaction* t, EngineShard* shard) -> OpResult<CompactObjType> {
     auto& db_slice = cntx->ns->GetDbSlice(shard->shard_id());
     auto it = db_slice.FindReadOnly(t->GetDbContext(), key).it;
     if (!it.is_done()) {
@@ -1395,11 +1395,11 @@ void GenericFamily::Type(CmdArgList args, ConnectionContext* cntx) {
       return OpStatus::KEY_NOTFOUND;
     }
   };
-  OpResult<int> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
+  OpResult<CompactObjType> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
   if (!result) {
     cntx->SendSimpleString("none");
   } else {
-    cntx->SendSimpleString(ObjTypeName(result.value()));
+    cntx->SendSimpleString(ObjTypeToString(result.value()));
   }
 }
 

src/server/generic_family_test.cc

@@ -747,4 +747,19 @@ TEST_F(GenericFamilyTest, RandomKey) {
   EXPECT_EQ(Run({"randomkey"}), "k1");
 }
 
+TEST_F(GenericFamilyTest, JsonType) {
+  auto resp = Run({"json.set", "json", "$", R"({"example":"value"})"});
+  EXPECT_EQ(resp, "OK");
+
+  resp = Run({"type", "json"});
+  EXPECT_EQ(resp, "ReJSON-RL") << "For the Redis GUI the register of the JSON type is important. "
+                                  "See https://github.com/dragonflydb/dragonfly/issues/3386";
+
+  // Test json type lowercase works for the SCAN commmand
+  resp = Run({"scan", "0", "type", "rejson-rl"});
+  EXPECT_THAT(resp, ArrLen(2));
+  auto vec = StrArray(resp.GetVec()[1]);
+  ASSERT_THAT(vec, ElementsAre("json"));
+}
+
 }  // namespace dfly

src/server/server_family.cc

@@ -1196,7 +1196,7 @@ void PrintPrometheusMetrics(const Metrics& m, DflyCmd* dfly_cmd, StringResponse*
     for (unsigned type = 0; type < total.memory_usage_by_type.size(); type++) {
       size_t mem = total.memory_usage_by_type[type];
       if (mem > 0) {
-        AppendMetricValue("type_used_memory", mem, {"type"}, {CompactObj::ObjTypeToString(type)},
+        AppendMetricValue("type_used_memory", mem, {"type"}, {ObjTypeToString(type)},
                           &type_used_memory_metric);
         added = true;
       }
@@ -2078,7 +2078,7 @@ void ServerFamily::Info(CmdArgList args, ConnectionContext* cntx) {
     for (unsigned type = 0; type < total.memory_usage_by_type.size(); type++) {
       size_t mem = total.memory_usage_by_type[type];
       if (mem > 0) {
-        append(absl::StrCat("type_used_memory_", CompactObj::ObjTypeToString(type)), mem);
+        append(absl::StrCat("type_used_memory_", ObjTypeToString(type)), mem);
       }
     }
     append("table_used_memory", total.table_mem_usage);

tests/dragonfly/json_test.py

@@ -22,7 +22,7 @@ async def test_basic_json_get_set(async_client: aioredis.Redis):
     result = await get_set_json(connection=async_client, key=key_name, value=jane)
     assert result, "failed to set JSON value"
     the_type = await async_client.type(key_name)
-    assert the_type == "rejson-rl"
+    assert the_type == "ReJSON-RL"
     assert len(result) == 1
     assert result[0]["name"] == "Jane"
     assert result[0]["Age"] == 33
@@ -34,7 +34,7 @@ async def test_access_json_value_as_string(async_client: aioredis.Redis):
     assert result is not None, "failed to set JSON value"
     # make sure that we have valid JSON here
     the_type = await async_client.type(key_name)
-    assert the_type == "rejson-rl"
+    assert the_type == "ReJSON-RL"
     # you cannot access this key as string
     try:
         result = await async_client.get(key_name)
@@ -49,7 +49,7 @@ async def test_reset_key_to_string(async_client: aioredis.Redis):
     assert result is not None, "failed to set JSON value"
     # make sure that we have valid JSON here
     the_type = await async_client.type(key_name)
-    assert the_type == "rejson-rl"
+    assert the_type == "ReJSON-RL"
 
     # set the key to be string - this is legal
     await async_client.set(key_name, "some random value")
@@ -60,7 +60,7 @@ async def test_reset_key_to_string(async_client: aioredis.Redis):
     # to change the type to JSON and override it
     result = await get_set_json(async_client, key=key_name, value=jane)
     the_type = await async_client.type(key_name)
-    assert the_type == "rejson-rl"
+    assert the_type == "ReJSON-RL"
 
 
 async def test_update_value(async_client: aioredis.Redis):
@@ -69,7 +69,7 @@ async def test_update_value(async_client: aioredis.Redis):
     assert result is not None, "failed to set JSON value"
     # make sure that we have valid JSON here
     the_type = await async_client.type(key_name)
-    assert the_type == "rejson-rl"
+    assert the_type == "ReJSON-RL"
     result = await get_set_json(async_client, value="0", key=key_name, path="$.a.*")
     assert len(result) == 3
     # make sure that all the values under 'a' where set to 0