JSON: support basic legacy path #1829

mapleFU · 2023-10-16T05:20:20Z

RedisJson supports a JsonPath for legacy client [1]. This patch only support the basic JsonPath "."

Note: this patch doesn't means fully compatible with Json legacy path, it just try it best to convert to
JsonPath. There're still some behaviors we cannot compatible with.

[1] https://redis.io/docs/data-types/json/path/#legacy-path-syntax

mapleFU · 2023-10-16T05:20:59Z

src/commands/cmd_json.cc

@@ -52,7 +55,7 @@ class CommandJsonGet : public Commander {
  }
 };

-REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandJsonSet>("json.set", -3, "write", 1, 1, 1),
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandJsonSet>("json.set", 3, "write", 1, 1, 1),


We should use -3 after set with NX support.

Now I think we only support 3 arguments.

Oops, seems this should be 4 (or -4 if implement NX and other options)

PragmaTwice

Seems there are several issues that need to be addressed in this PR:

All special logic related to legacy path goes to redis_json.h/cc instead of json.h, which is not aligned with the original intention. redis_json.h/cc should only contain i/o specific code and method call to json.h. You can try to implement them in JsonValue or add a class named JsonPath.
Actually there are some difference in the query result between path $.x and .x (or x). It can be observe that query of path prefixed without $ will just get response with the first matched value instead of an array.
I think we can consider a more complete path resolution mechanism rather than just handling one case.

mapleFU · 2023-10-16T10:45:56Z

Actually there are some difference in the query result between path $.x and .x (or x). It can be observe that query of path prefixed without $ will just get response with the first matched value instead of an array.

This patch is only a basic json legacy path support, the detail rule will be handled later.

mapleFU · 2023-10-16T11:57:17Z

@PragmaTwice I've add a JsonPath type, however, currently I find the evaluation has a problem when we need support both const and mutable. Would you mind take a look?

src/types/json_path.cc

src/types/json.h

PragmaTwice · 2023-10-18T09:54:28Z

src/types/json.h

-      jsoncons::jsonpath::json_replace(value, path, [&new_value](const std::string & /*path*/, jsoncons::json &origin) {
-        origin = new_value.value;
-      });
+      path.EvalReplaceExpression(


Actually I do not think this function name is clear enough. Maybe we can figure out another name.

src/types/json_path.cc

PragmaTwice · 2023-10-18T10:00:16Z

src/types/json_path.h

+ public:
+  using JsonType = jsoncons::basic_json<char>;
+  using JsonPathExpression = jsoncons::jsonpath::jsonpath_expression<JsonType, const JsonType&>;
+  using JsonQueryCallback = std::function<void(const std::string_view&, const JsonType&)>;


Cannot find where it is used 🤔

PragmaTwice · 2023-10-18T10:00:54Z

src/types/json_path.h

+  JsonType EvalQueryExpression(const JsonType& json_value) const {
+    return expression_.evaluate(const_cast<JsonType&>(json_value));
+  }
+
+  void EvalReplaceExpression(JsonType& json_value, const JsonReplaceCallback& callback) const {
+    auto wrapped_cb = [&callback](const std::string_view& path, const JsonType& json) {
+      // Though JsonPath supports mutable reference, `jsoncons::make_expression`
+      // only supports const reference, so const_cast is used as a workaround.
+      callback(path, const_cast<JsonType&>(json));
+    };
+    expression_.evaluate(json_value, wrapped_cb);
+  }


Is it necessary to provide two function?

🤔the first one is used to get a JsonValue, the second value is used to replace. Any better idea for this?

PragmaTwice · 2023-10-18T10:01:15Z

src/types/json_path.h

+  JsonPath(std::string path, std::string fixed_path, JsonPathExpression path_expression)
+      : origin_(std::move(path)), fixed_path_(std::move(fixed_path)), expression_(std::move(path_expression)) {}
+
+  std::string origin_;


Is it necessary to store the original path string? Or just store the original one rather than the modified one?

Is it necessary to store the original path string

We need the OriginPath in someplace like MSET. It's also ok to only has the origin_path and expression. But I think it would not heavy to having one more string?

src/types/json_path.h

tests/cppunit/types/json_test.cc

mapleFU · 2023-10-18T16:28:40Z

Note: this patch doesn't means fully compatible with Json legacy path, it just try it best to convert to
JsonPath. There're still some behaviors we cannot compatible with.

I found that:

Field name like $a is not support
['a'] or ["a"] is not so well support now

src/types/json.h

src/types/redis_json.cc

PragmaTwice · 2023-10-20T10:00:48Z

src/types/json.h

+
+  static constexpr std::string_view ROOT_PATH = "$";
+
+  static StatusOr<JsonPath> BuildJsonPath(std::string_view path);


Suggested change

static StatusOr<JsonPath> BuildJsonPath(std::string_view path);

static StatusOr<JsonPath> FromString(std::string_view path);

PragmaTwice · 2023-10-20T10:02:44Z

src/types/json.h

+  void EvalJsonReplace(JsonType &json_value, const BinaryJsonFunction &callback) const {
+    auto wrapped_cb = [&callback](const std::string_view &path, const JsonType &json) {
+      // Though JsonPath supports mutable reference, `jsoncons::make_expression`
+      // only supports const reference, so const_cast is used as a workaround.
+      callback(path, const_cast<JsonType &>(json));
+    };
+    expression_.evaluate(json_value, wrapped_cb);
+  }


Same as #1829 (comment), maybe json_replace has no const_cast?

json_replace has use a different expression 😅 I don't know how jsoncons consider it. I'll dive into it

danielaparker/jsoncons#466

@PragmaTwice The author says he will implement a mutable in next release. Let me make this patch small and waiting for next release

PragmaTwice · 2023-10-20T10:05:12Z

src/types/json.cc

+  if (json_parse_error) {
+    return {Status::NotOK, json_parse_error.message()};
+  }
+  return JsonPath(std::string(json_string), !fixed_path.empty(), std::move(path_expression));


I think maybe we can avoid re-construct json_string while it is fixed_path.

PragmaTwice · 2023-10-20T16:21:52Z

src/types/json.h

+  JsonType EvalJsonQuery(const JsonType &json_value) const {
+    return expression_.evaluate(const_cast<JsonType &>(json_value));
+  }


Could it return multiple query result? e.g. query $..a from {"a":{"a":1}}, we'll get {"a":1} and 1.

mapleFU added 2 commits October 16, 2023 13:09

JSON: support basic legacy path

6433101

add basic impl

0d06d9e

mapleFU commented Oct 16, 2023

View reviewed changes

mapleFU requested a review from PragmaTwice October 16, 2023 05:21

mapleFU added 3 commits October 16, 2023 13:53

fix lint

26be5fe

fix arity to 4

366c316

Merge branch 'unstable' into json/support-basic-legacy-path

6d7f215

PragmaTwice reviewed Oct 16, 2023

View reviewed changes

Update: add JsonPath class

68c3677

fix lint

69ddf73

mapleFU force-pushed the json/support-basic-legacy-path branch from 592d348 to 9d76a51 Compare October 16, 2023 13:22

support more json-legacy-path

50fa211

mapleFU force-pushed the json/support-basic-legacy-path branch from 9d76a51 to 50fa211 Compare October 16, 2023 13:23

mapleFU requested a review from PragmaTwice October 17, 2023 05:36

PragmaTwice reviewed Oct 18, 2023

View reviewed changes

mapleFU added 3 commits October 18, 2023 23:01

resolve some comments

236bfce

add test

40c1851

Merge branch 'unstable' into json/support-basic-legacy-path

b79c0a3

PragmaTwice reviewed Oct 19, 2023

View reviewed changes

src/types/json.h Outdated Show resolved Hide resolved

PragmaTwice reviewed Oct 19, 2023

View reviewed changes

src/types/redis_json.cc Outdated Show resolved Hide resolved

mapleFU added 2 commits October 19, 2023 13:42

resolve comment

9e110c2

Update: resolve comment

6f66850

mapleFU requested a review from PragmaTwice October 19, 2023 16:43

PragmaTwice reviewed Oct 20, 2023

View reviewed changes

PragmaTwice mentioned this pull request Oct 21, 2023

Tracking issue for the Redis JSON support #1805

Open

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON: support basic legacy path #1829

JSON: support basic legacy path #1829

mapleFU commented Oct 16, 2023 •

edited

mapleFU Oct 16, 2023

mapleFU Oct 16, 2023

PragmaTwice left a comment •

edited

mapleFU commented Oct 16, 2023

mapleFU commented Oct 16, 2023

PragmaTwice Oct 18, 2023

PragmaTwice Oct 18, 2023

mapleFU Oct 18, 2023

PragmaTwice Oct 18, 2023

mapleFU Oct 18, 2023

PragmaTwice Oct 18, 2023

mapleFU Oct 18, 2023

mapleFU commented Oct 18, 2023

PragmaTwice Oct 20, 2023

PragmaTwice Oct 20, 2023

mapleFU Oct 20, 2023

mapleFU Oct 25, 2023

PragmaTwice Oct 20, 2023

PragmaTwice Oct 20, 2023


		static constexpr std::string_view ROOT_PATH = "$";

		static StatusOr<JsonPath> BuildJsonPath(std::string_view path);

	static StatusOr<JsonPath> BuildJsonPath(std::string_view path);
	static StatusOr<JsonPath> FromString(std::string_view path);

JSON: support basic legacy path #1829

Are you sure you want to change the base?

JSON: support basic legacy path #1829

Conversation

mapleFU commented Oct 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice left a comment • edited

Choose a reason for hiding this comment

mapleFU commented Oct 16, 2023

mapleFU commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Oct 16, 2023 •

edited

PragmaTwice left a comment •

edited