Fix status mis-return in JSON.CLEAR and polish code style (apache#1853)

Yangsx-1 · Oct 25, 2023 · eaced67 · eaced67
1 parent 7598e8d
commit eaced67
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 42 deletions.
diff --git a/src/commands/cmd_json.cc b/src/commands/cmd_json.cc
@@ -153,7 +153,7 @@ class CommandJsonClear : public Commander {
   Status Execute(Server *svr, Connection *conn, std::string *output) override {
     redis::Json json(svr->storage, conn->GetNamespace());
 
-    int result = 0;
+    size_t result = 0;
 
     // If path not specified set it to $
     std::string path = (args_.size() > 2) ? args_[2] : "$";

diff --git a/src/types/json.h b/src/types/json.h
@@ -134,72 +134,68 @@ struct JsonValue {
     return result_count;
   }
 
-  Status Type(std::string_view path, std::vector<std::string> *types) const {
-    types->clear();
+  StatusOr<std::vector<std::string>> Type(std::string_view path) const {
+    std::vector<std::string> types;
     try {
       jsoncons::jsonpath::json_query(value, path, [&types](const std::string & /*path*/, const jsoncons::json &val) {
         switch (val.type()) {
           case jsoncons::json_type::null_value:
-            types->emplace_back("null");
+            types.emplace_back("null");
             break;
           case jsoncons::json_type::bool_value:
-            types->emplace_back("boolean");
+            types.emplace_back("boolean");
             break;
           case jsoncons::json_type::int64_value:
           case jsoncons::json_type::uint64_value:
-            types->emplace_back("integer");
+            types.emplace_back("integer");
             break;
           case jsoncons::json_type::double_value:
-            types->emplace_back("number");
+            types.emplace_back("number");
             break;
           case jsoncons::json_type::string_value:
-            types->emplace_back("string");
+            types.emplace_back("string");
             break;
           case jsoncons::json_type::array_value:
-            types->emplace_back("array");
+            types.emplace_back("array");
             break;
           case jsoncons::json_type::object_value:
-            types->emplace_back("object");
+            types.emplace_back("object");
             break;
           default:
-            types->emplace_back("unknown");
+            types.emplace_back("unknown");
             break;
         }
       });
     } catch (const jsoncons::jsonpath::jsonpath_error &e) {
       return {Status::NotOK, e.what()};
     }
-    return Status::OK();
+
+    return types;
   }
 
-  Status Clear(std::string_view path, int *result) {
+  StatusOr<size_t> Clear(std::string_view path) {
+    size_t count = 0;
     try {
-      int cleared_count = 0;
-      jsoncons::jsonpath::json_replace(value, path,
-                                       [&cleared_count](const std::string &
-                                                        /*path*/,
-                                                        jsoncons::json &val) {
-                                         bool is_array = val.is_array() && !val.empty();
-                                         bool is_object = val.is_object() && !val.empty();
-                                         bool is_number = val.is_number() && val.as<double>() != 0;
-
-                                         if (is_array)
-                                           val = jsoncons::json::array();
-                                         else if (is_object)
-                                           val = jsoncons::json::object();
-                                         else if (is_number)
-                                           val = 0;
-                                         else
-                                           return;
-
-                                         cleared_count++;
-                                       });
-
-      *result = cleared_count;
+      jsoncons::jsonpath::json_replace(value, path, [&count](const std::string & /*path*/, jsoncons::json &val) {
+        bool is_array = val.is_array() && !val.empty();
+        bool is_object = val.is_object() && !val.empty();
+        bool is_number = val.is_number() && val.as<double>() != 0;
+
+        if (is_array)
+          val = jsoncons::json::array();
+        else if (is_object)
+          val = jsoncons::json::object();
+        else if (is_number)
+          val = 0;
+        else
+          return;
+
+        count++;
+      });
     } catch (const jsoncons::jsonpath::jsonpath_error &e) {
       return {Status::NotOK, e.what()};
     }
-    return Status::OK();
+    return count;
   }
 
   JsonValue(const JsonValue &) = default;

diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc
@@ -159,11 +159,14 @@ rocksdb::Status Json::Type(const std::string &user_key, const std::string &path,
   auto s = read(ns_key, &metadata, &json_val);
   if (!s.ok()) return s;
 
-  auto res = json_val.Type(path, results);
+  auto res = json_val.Type(path);
+  if (!res) return rocksdb::Status::InvalidArgument(res.Msg());
+
+  *results = *res;
   return rocksdb::Status::OK();
 }
 
-rocksdb::Status Json::Clear(const std::string &user_key, const std::string &path, int *result) {
+rocksdb::Status Json::Clear(const std::string &user_key, const std::string &path, size_t *result) {
   auto ns_key = AppendNamespacePrefix(user_key);
 
   LockGuard guard(storage_->GetLockManager(), ns_key);
@@ -174,9 +177,10 @@ rocksdb::Status Json::Clear(const std::string &user_key, const std::string &path
 
   if (!s.ok()) return s;
 
-  auto res = json_val.Clear(path, result);
-  if (!res.OK()) return s;
+  auto res = json_val.Clear(path);
+  if (!res) return rocksdb::Status::InvalidArgument(res.Msg());
 
+  *result = *res;
   if (*result == 0) {
     return rocksdb::Status::OK();
   }

diff --git a/src/types/redis_json.h b/src/types/redis_json.h
@@ -38,7 +38,7 @@ class Json : public Database {
   rocksdb::Status Type(const std::string &user_key, const std::string &path, std::vector<std::string> *results);
   rocksdb::Status ArrAppend(const std::string &user_key, const std::string &path,
                             const std::vector<std::string> &values, std::vector<size_t> *result_count);
-  rocksdb::Status Clear(const std::string &user_key, const std::string &path, int *result);
+  rocksdb::Status Clear(const std::string &user_key, const std::string &path, size_t *result);
 
  private:
   rocksdb::Status write(Slice ns_key, JsonMetadata *metadata, const JsonValue &json_val);

diff --git a/tests/cppunit/types/json_test.cc b/tests/cppunit/types/json_test.cc
@@ -183,7 +183,7 @@ TEST_F(RedisJsonTest, ArrAppend) {
 }
 
 TEST_F(RedisJsonTest, Clear) {
-  int result = 0;
+  size_t result = 0;
 
   ASSERT_TRUE(
       json_