fix: json.merge exception crash #3409

kostasrim · 2024-07-30T09:23:07Z

json.merge would throw an exception when the json object did not contain the element to replace because RecursiveMerge functions used &dest->at(k_v.key()) which threw the exception. Remove RecursiveMerge completely and use the one implemented in jsoncons lib.

add test
replace RecursiveMerge with mergepatch::apply_merge_patch
add exception handling for that flow

kostasrim · 2024-07-30T09:23:57Z

src/server/json_family.cc

+      RecursiveMerge(*parsed_json, obj);
+    } catch (const std::exception& e) {
+      LOG(WARNING) << "Exception in OpMerge: " << e.what();
+      return OpStatus::KEY_NOTFOUND;


@romange not particularly happy with this. Any recommendations for actual OpStatus error ? Imo KEY_NOTFOUND is not good enough

INVALID_VALUE?

and please make it LOG(ERROR) and also provide more information like json values, so we could reproduce if this log ever happens.

LOG_EVERY_T(ERROR,1)

romange · 2024-07-30T09:47:40Z

src/server/json_family.cc

@@ -1466,7 +1466,7 @@ void RecursiveMerge(const JsonType& patch, JsonType* dest) {
  for (const auto& k_v : patch.object_range()) {
    if (k_v.value().is_null()) {
      dest->erase(k_v.key());
-    } else {
+    } else if (dest->find(k_v.key()) != dest->object_range().end()) {


i suspect it's not a correct fix. Above this function there is a spec with a pseudo code. Have you looked at it?

yes but the pseudocode is not very clear. I will need to think about it but I will take care of this.

romange · 2024-07-30T09:49:58Z

src/server/json_family_test.cc

+  resp = Run({"JSON.SET", "foo", "$", "{}"});
+  EXPECT_EQ(resp, "OK");
+  resp = Run({"JSON.MERGE", "foo", "$", R"({"a":2} {"b":2})"});
+  EXPECT_EQ(resp, "OK");


please also verify the result value

yep I also verified it with Valkey to make sure the results consistent

why "a" has been merged but "b" have not?

because there is no comma, it's ignored. Same behaviour in Valkey but I wanted to leave it as is as reminder that there is no error or something

i find it confusing because it's out of scope of this PR (json parsing).
Lets have 2 fields f1, common in foo, 2 fields that are merged: f2, common and show how f1, f2, common appear in the final result.

@romange done. I also tested the same command/query with Valkey and they give the same result (the only difference is the order, common is second but that doesn't change the correctness since json is unordered)

kostasrim · 2024-07-30T15:12:09Z

src/server/json_family.cc

-    RecursiveMerge(*parsed_json, obj);
+    try {
+      // https://datatracker.ietf.org/doc/html/rfc7386#section-2
+      mergepatch::apply_merge_patch(*obj, *parsed_json);


@romange I am 99% sure this doesn't throw but I will keep the try/block just in case. + I don't think we now need to worry about correctness.

nice! Did not know that they had this API 💯

romange

👏🏼

fix: json.merge crash

bafb7bf

kostasrim requested a review from romange July 30, 2024 09:23

kostasrim commented Jul 30, 2024

View reviewed changes

kostasrim self-assigned this Jul 30, 2024

romange reviewed Jul 30, 2024

View reviewed changes

fixes

c421ec5

kostasrim requested a review from romange July 30, 2024 15:11

kostasrim commented Jul 30, 2024

View reviewed changes

dranikpg linked an issue Jul 30, 2024 that may be closed by this pull request

check failed on Json::Merge #3408

Closed

comments

90511a3

romange approved these changes Jul 31, 2024

View reviewed changes

kostasrim merged commit 7e911c1 into main Jul 31, 2024
10 checks passed

kostasrim deleted the fix_json_merge branch July 31, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: json.merge exception crash #3409

fix: json.merge exception crash #3409

kostasrim commented Jul 30, 2024 •

edited

Loading

kostasrim Jul 30, 2024

romange Jul 30, 2024

romange Jul 30, 2024

romange Jul 30, 2024

kostasrim Jul 30, 2024

romange Jul 30, 2024

kostasrim Jul 30, 2024

romange Jul 30, 2024

kostasrim Jul 31, 2024

romange Jul 31, 2024

kostasrim Jul 31, 2024

kostasrim Jul 30, 2024

romange Jul 31, 2024 •

edited

Loading

romange left a comment

fix: json.merge exception crash #3409

fix: json.merge exception crash #3409

Conversation

kostasrim commented Jul 30, 2024 • edited Loading

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

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

romange Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

romange left a comment

Choose a reason for hiding this comment

kostasrim commented Jul 30, 2024 •

edited

Loading

romange Jul 31, 2024 •

edited

Loading