chore: JSON.GET now works with our own jsonpath

dragonflydb · Feb 23, 2024 · 15d9849 · 15d9849
1 parent 47171c4
commit 15d9849
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 34 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -148,7 +148,7 @@ jobs:
           ./dragonfly_test
           ./multi_test --multi_exec_mode=1
           ./multi_test --multi_exec_mode=3
-          # GLOG_logtostderr=1 GLOG_vmodule=transaction=1,engine_shard_set=1 CTEST_OUTPUT_ON_FAILURE=1 ninja server/test
+          ./json_family_test --jsonpathv2
 
       - name: Upload unit logs on failure
         if: failure()

diff --git a/src/core/json/jsonpath_test.cc b/src/core/json/jsonpath_test.cc
@@ -30,6 +30,12 @@ class TestDriver : public Driver {
   }
 };
 
+inline JsonType ValidJson(string_view str) {
+  auto res = ::dfly::JsonFromString(str, pmr::get_default_resource());
+  CHECK(res) << "Failed to parse json: " << str;
+  return *res;
+}
+
 class JsonPathTest : public ::testing::Test {
  protected:
   JsonPathTest() {
@@ -123,6 +129,19 @@ TEST_F(JsonPathTest, Parser) {
   EXPECT_EQ(0, Parse("$.plays[*].game"));
 }
 
+TEST_F(JsonPathTest, Root) {
+  JsonType json = ValidJson(R"({"foo" : 1, "bar": "str" })");
+  ASSERT_EQ(0, Parse("$"));
+  Path path = driver_.TakePath();
+  int called = 0;
+  EvaluatePath(path, json, [&](optional<string_view>, const JsonType& val) {
+    ++called;
+    ASSERT_TRUE(val.is_object());
+    ASSERT_EQ(json, val);
+  });
+  ASSERT_EQ(1, called);
+}
+
 TEST_F(JsonPathTest, Functions) {
   ASSERT_EQ(0, Parse("max($.plays[*].score)"));
   Path path = driver_.TakePath();
@@ -131,9 +150,7 @@ TEST_F(JsonPathTest, Functions) {
   EXPECT_THAT(path[1], SegType(SegmentType::IDENTIFIER));
   EXPECT_THAT(path[2], SegType(SegmentType::WILDCARD));
   EXPECT_THAT(path[3], SegType(SegmentType::IDENTIFIER));
-  JsonType json =
-      JsonFromString(R"({"plays": [{"score": 1}, {"score": 2}]})", pmr::get_default_resource())
-          .value();
+  JsonType json = ValidJson(R"({"plays": [{"score": 1}, {"score": 2}]})");
   int called = 0;
   EvaluatePath(path, json, [&](auto, const JsonType& val) {
     ASSERT_TRUE(val.is<int>());
@@ -163,16 +180,15 @@ TEST_F(JsonPathTest, Descent) {
 
 TEST_F(JsonPathTest, Path) {
   Path path;
-  JsonType json = JsonFromString(R"({"v11":{ "f" : 1, "a2": [0]}, "v12": {"f": 2, "a2": [1]},
+  JsonType json = ValidJson(R"({"v11":{ "f" : 1, "a2": [0]}, "v12": {"f": 2, "a2": [1]},
       "v13": 3
-      })",
-                                 pmr::get_default_resource())
-                      .value();
+      })");
   int called = 0;
 
   // Empty path
   EvaluatePath(path, json, [&](optional<string_view>, const JsonType& val) { ++called; });
-  ASSERT_EQ(0, called);
+  ASSERT_EQ(1, called);
+  called = 0;
 
   path.emplace_back(SegmentType::IDENTIFIER, "v13");
   EvaluatePath(path, json, [&](optional<string_view> key, const JsonType& val) {
@@ -206,11 +222,10 @@ TEST_F(JsonPathTest, Path) {
 }
 
 TEST_F(JsonPathTest, EvalDescent) {
-  JsonType json = JsonFromString(R"(
+  JsonType json = ValidJson(R"(
     {"v11":{ "f" : 1, "a2": [0]}, "v12": {"f": 2, "v21": {"f": 3, "a2": [1]}},
       "v13": { "a2" : { "b" : {"f" : 4}}}
-      })",
-                                 pmr::get_default_resource());
+      })");
 
   Path path;
 
@@ -241,10 +256,9 @@ TEST_F(JsonPathTest, EvalDescent) {
   });
   ASSERT_EQ(4, called);
 
-  json = JsonFromString(R"(
+  json = ValidJson(R"(
     {"a":[7], "inner": {"a": {"b": 2, "c": 1337}}}
-  )",
-                        pmr::get_default_resource());
+  )");
   path.pop_back();
   path.emplace_back(SegmentType::IDENTIFIER, "a");
 
@@ -263,7 +277,7 @@ TEST_F(JsonPathTest, Wildcard) {
   ASSERT_EQ(1, path.size());
   EXPECT_THAT(path[0], SegType(SegmentType::WILDCARD));
 
-  JsonType json = JsonFromString(R"([1, 2, 3])", pmr::get_default_resource()).value();
+  JsonType json = ValidJson(R"([1, 2, 3])");
   vector<int> arr;
   EvaluatePath(path, json, [&](optional<string_view> key, const JsonType& val) {
     ASSERT_FALSE(key);
@@ -273,7 +287,7 @@ TEST_F(JsonPathTest, Wildcard) {
 }
 
 TEST_F(JsonPathTest, Mutate) {
-  JsonType json = JsonFromString(R"([1, 2, 3, 5, 6])", pmr::get_default_resource()).value();
+  JsonType json = ValidJson(R"([1, 2, 3, 5, 6])");
   ASSERT_EQ(0, Parse("$[*]"));
   Path path = driver_.TakePath();
   MutateCallback cb = [&](optional<string_view>, JsonType* val) {
@@ -289,10 +303,9 @@ TEST_F(JsonPathTest, Mutate) {
   }
   ASSERT_THAT(arr, ElementsAre(2, 3, 4, 6, 7));
 
-  json = JsonFromString(R"(
+  json = ValidJson(R"(
     {"a":[7], "inner": {"a": {"bool": true, "c": 42}}}
-  )",
-                        pmr::get_default_resource());
+  )");
   ASSERT_EQ(0, Parse("$..a.*"));
   path = driver_.TakePath();
   MutatePath(

diff --git a/src/core/json/path.cc b/src/core/json/path.cc
@@ -432,8 +432,10 @@ JsonType PathSegment::GetResult() const {
 }
 
 void EvaluatePath(const Path& path, const JsonType& json, PathCallback callback) {
-  if (path.empty())
+  if (path.empty()) {  // root node
+    callback(nullopt, json);
     return;
+  }
 
   if (path.front().type() != SegmentType::FUNCTION) {
     Dfs().Traverse(path, json, std::move(callback));

diff --git a/src/server/json_family.cc b/src/server/json_family.cc
@@ -65,6 +65,17 @@ inline void Evaluate(const json::Path& expr, const JsonType& obj, ExprCallback c
   });
 }
 
+inline JsonType Evaluate(const JsonExpression& expr, const JsonType& obj) {
+  return expr.evaluate(obj);
+}
+
+inline JsonType Evaluate(const json::Path& expr, const JsonType& obj) {
+  JsonType res(json_array_arg);
+  json::EvaluatePath(expr, obj,
+                     [&res](optional<string_view>, const JsonType& val) { res.push_back(val); });
+  return res;
+}
+
 inline OpStatus JsonReplaceVerifyNoOp(JsonType&) {
   return OpStatus::OK;
 }
@@ -418,7 +429,7 @@ void SendJsonValue(RedisReplyBuilder* rb, const JsonType& j) {
 }
 
 OpResult<string> OpJsonGet(const OpArgs& op_args, string_view key,
-                           const vector<pair<string_view, optional<JsonExpression>>>& expressions,
+                           const vector<pair<string_view, optional<JsonPathV2>>>& expressions,
                            bool should_format, const OptString& indent, const OptString& new_line,
                            const OptString& space) {
   OpResult<JsonType*> result = GetJson(op_args, key);
@@ -456,16 +467,17 @@ OpResult<string> OpJsonGet(const OpArgs& op_args, string_view key,
     }
   }
 
-  auto eval_wrapped = [&json_entry](const optional<JsonExpression>& expr) {
-    return expr ? expr->evaluate(json_entry) : json_entry;
+  auto eval_wrapped = [&json_entry](const optional<JsonPathV2>& expr) -> JsonType {
+    return expr ? visit([&](auto& arg) { return Evaluate(arg, json_entry); }, *expr) : json_entry;
   };
 
   JsonType out{json_object_arg};  // see https://github.com/danielaparker/jsoncons/issues/482
   if (expressions.size() == 1) {
     out = eval_wrapped(expressions[0].second);
   } else {
-    for (auto& [expr_str, expr] : expressions)
+    for (const auto& [expr_str, expr] : expressions) {
       out[expr_str] = eval_wrapped(expr);
+    }
   }
 
   if (should_format) {
@@ -1826,7 +1838,9 @@ void JsonFamily::Get(CmdArgList args, ConnectionContext* cntx) {
   OptString indent;
   OptString new_line;
   OptString space;
-  vector<pair<string_view, optional<JsonExpression>>> expressions;
+
+  // '.' corresponds to the legacy, non-array format and is passed as nullopt.
+  vector<pair<string_view, optional<JsonPathV2>>> expressions;
 
   while (parser.HasNext()) {
     if (parser.Check("SPACE").IgnoreCase().ExpectTail(1)) {
@@ -1842,17 +1856,12 @@ void JsonFamily::Get(CmdArgList args, ConnectionContext* cntx) {
       continue;
     }
 
-    optional<JsonExpression> expr;
+    optional<JsonPathV2> expr;
     string_view expr_str = parser.Next();
 
     if (expr_str != ".") {
-      io::Result<JsonExpression> res = ParseJsonPath(expr_str);
-      if (!res) {
-        LOG(WARNING) << "path '" << expr_str
-                     << "': Invalid JSONPath syntax: " << res.error().message();
-        return cntx->SendError(kSyntaxErr);
-      }
-      expr.emplace(std::move(*res));
+      JsonPathV2 expression = PARSE_PATHV2(expr_str);
+      expr.emplace(std::move(expression));
     }
 
     expressions.emplace_back(expr_str, std::move(expr));

diff --git a/src/server/json_family_test.cc b/src/server/json_family_test.cc
@@ -276,6 +276,9 @@ TEST_F(JsonFamilyTest, Toggle) {
   auto resp = Run({"JSON.SET", "json", ".", json});
   ASSERT_THAT(resp, "OK");
 
+  resp = Run({"JSON.GET", "json", "$.*"});
+  EXPECT_EQ(R"([true,false,1,null,"foo",[],{}])", resp);
+
   resp = Run({"JSON.TOGGLE", "json", "$.*"});
   ASSERT_THAT(resp, ArgType(RespExpr::ARRAY));
   EXPECT_THAT(resp.GetVec(),