From 71aa4c51440ef5ecb5404999a80350fade1fbd43 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 1 May 2025 18:40:56 +0300 Subject: [PATCH 1/2] fix: json.set recursive processing crash --- src/core/json/detail/jsoncons_dfs.cc | 29 +++++++++++++++++++++++++++- src/core/json/detail/jsoncons_dfs.h | 19 ++++++++++++++++-- src/core/json/jsonpath_test.cc | 24 +++++++++++++++++++++++ src/core/json/path.cc | 13 +++++++++---- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/core/json/detail/jsoncons_dfs.cc b/src/core/json/detail/jsoncons_dfs.cc index e398b8fd18a2..0355759ef75e 100644 --- a/src/core/json/detail/jsoncons_dfs.cc +++ b/src/core/json/detail/jsoncons_dfs.cc @@ -8,6 +8,8 @@ #include "core/json/detail/jsoncons_dfs.h" +#include + namespace dfly::json::detail { using namespace std; @@ -82,6 +84,10 @@ Dfs Dfs::Mutate(absl::Span path, const MutateCallback& callba return dfs; } + // Use vector to maintain order and set to check uniqueness + std::vector nodes_to_mutate; + absl::flat_hash_set seen_nodes; + using Item = detail::JsonconsDfsItem; vector stack; stack.emplace_back(json); @@ -103,14 +109,35 @@ Dfs Dfs::Mutate(absl::Span path, const MutateCallback& callba if (next_seg_id + 1 < path.size()) { stack.emplace_back(next, next_seg_id); } else { - dfs.MutateStep(path[next_seg_id], callback, next); + // Terminal step: collect node for mutation if not seen before + if (seen_nodes.find(next) == seen_nodes.end()) { + nodes_to_mutate.push_back(next); + seen_nodes.insert(next); + } } } } else { + // If Advance failed (e.g., MISMATCH or OUT_OF_BOUNDS), check if the current node + // itself is a terminal target for mutation due to a previous DESCENT segment. + // This handles cases like $.a..b where 'a' itself might match the 'b' after descent. + if (!res && segment_index > 0 && path[segment_index - 1].type() == SegmentType::DESCENT && + stack.back().get_segment_step() == 0) { + if (segment_index + 1 == path.size()) { + // Attempt to apply the final mutation step directly to the current node. + // We apply it directly here because this node won't be added to the stack again. + dfs.MutateStep(path_segment, callback, stack.back().obj_ptr()); + } + } stack.pop_back(); } } while (!stack.empty()); + // Apply mutations after DFS traversal is complete + const PathSegment& terminal_segment = path.back(); + for (JsonType* node : nodes_to_mutate) { + dfs.MutateStep(terminal_segment, callback, node); + } + return dfs; } diff --git a/src/core/json/detail/jsoncons_dfs.h b/src/core/json/detail/jsoncons_dfs.h index 551cd949c4fb..b67d3e44943b 100644 --- a/src/core/json/detail/jsoncons_dfs.h +++ b/src/core/json/detail/jsoncons_dfs.h @@ -43,6 +43,14 @@ template class JsonconsDfsItem { return depth_state_.second; } + Ptr obj_ptr() const { + return depth_state_.first; + } + + unsigned get_segment_step() const { + return segment_step_; + } + private: static bool ShouldIterateAll(SegmentType type) { return type == SegmentType::WILDCARD || type == SegmentType::DESCENT; @@ -128,8 +136,15 @@ class Dfs { } bool Mutate(const MutateCallback& callback, std::optional key, JsonType* node) { - ++matches_; - return callback(key, node); + // matches_ is incremented in MutateStep after successful deletion/mutation by callback. + bool deleted = callback(key, node); + // We only increment matches_ here if the callback indicates deletion, + // as MutateStep handles incrementing based on its own logic. + // This might need adjustment if non-deleting mutations should also count universally. + if (deleted) { + matches_++; + } + return deleted; // Return true if node should be deleted } unsigned matches_ = 0; diff --git a/src/core/json/jsonpath_test.cc b/src/core/json/jsonpath_test.cc index 268e161d29b1..3f1f1eed6436 100644 --- a/src/core/json/jsonpath_test.cc +++ b/src/core/json/jsonpath_test.cc @@ -521,6 +521,30 @@ TYPED_TEST(JsonPathTest, Mutate) { } } +TYPED_TEST(JsonPathTest, MutateRecursiveDescentKey) { + ASSERT_EQ(0, this->Parse("$..value")); + Path path = this->driver_.TakePath(); + + JsonType json = ValidJson(R"({"data":{"value":10,"subdata":{"value":20}}})"); + JsonType replacement = ValidJson(R"({"value": 30})"); + + auto cb = [&](optional key, JsonType* val) { + if (key && key.value() == "value" && (val->is_int64() || val->is_double())) { + *val = replacement; + return false; + } + return false; + }; + + unsigned reported_matches = MutatePath(path, cb, &json); + + JsonType expected = + ValidJson(R"({"data":{"subdata":{"value":{"value":30}},"value":{"value":30}}})"); + + EXPECT_EQ(expected, json); + EXPECT_EQ(0, reported_matches); +} + TYPED_TEST(JsonPathTest, SubRange) { TypeParam json = ValidJson(R"({"arr": [1, 2, 3, 4, 5]})"); ASSERT_EQ(0, this->Parse("$.arr[1:2]")); diff --git a/src/core/json/path.cc b/src/core/json/path.cc index dc1612243457..e88e70a0fd56 100644 --- a/src/core/json/path.cc +++ b/src/core/json/path.cc @@ -288,11 +288,16 @@ unsigned MutatePath(const Path& path, MutateCallback callback, FlatJson json, flexbuffers::Builder* fbb) { JsonType mut_json = FromFlat(json); unsigned res = MutatePath(path, std::move(callback), &mut_json); - if (res) { - FromJsonType(mut_json, fbb); - fbb->Finish(); - } + // Populate the output builder 'fbb' with the resulting JSON state + // (mutated or original if res == 0) and finalize it. + // The builder MUST be finished before returning so that the caller + // can safely access the resulting flatbuffer data (e.g., via GetBuffer()). + // Skipping Finish() would leave the builder in an invalid, unusable state. + FromJsonType(mut_json, fbb); // Always convert (changed or not) JSON + fbb->Finish(); // Always finish the builder + + // Return the number of actual mutations that occurred. return res; } From da38d1e8916368a693b8f8018873878d509d957b Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Sun, 4 May 2025 17:44:46 +0300 Subject: [PATCH 2/2] fix: removed useless set --- src/core/json/detail/jsoncons_dfs.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/core/json/detail/jsoncons_dfs.cc b/src/core/json/detail/jsoncons_dfs.cc index 0355759ef75e..4c077fa9ef83 100644 --- a/src/core/json/detail/jsoncons_dfs.cc +++ b/src/core/json/detail/jsoncons_dfs.cc @@ -8,8 +8,6 @@ #include "core/json/detail/jsoncons_dfs.h" -#include - namespace dfly::json::detail { using namespace std; @@ -84,9 +82,8 @@ Dfs Dfs::Mutate(absl::Span path, const MutateCallback& callba return dfs; } - // Use vector to maintain order and set to check uniqueness + // Use vector to maintain order std::vector nodes_to_mutate; - absl::flat_hash_set seen_nodes; using Item = detail::JsonconsDfsItem; vector stack; @@ -110,10 +107,7 @@ Dfs Dfs::Mutate(absl::Span path, const MutateCallback& callba stack.emplace_back(next, next_seg_id); } else { // Terminal step: collect node for mutation if not seen before - if (seen_nodes.find(next) == seen_nodes.end()) { - nodes_to_mutate.push_back(next); - seen_nodes.insert(next); - } + nodes_to_mutate.push_back(next); } } } else {