From 1a16e38cbdbbd3b9db863d41dace54280a53504a Mon Sep 17 00:00:00 2001 From: poipoiPIO Date: Sat, 5 Oct 2024 11:03:46 +0300 Subject: [PATCH 1/4] fix: JSON.MSET multi-command query on a single key --- src/types/redis_json.cc | 33 +++++++++++++++++------- tests/gocase/unit/type/json/json_test.go | 8 +++++- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc index b058a2dfd6a..caf118581db 100644 --- a/src/types/redis_json.cc +++ b/src/types/redis_json.cc @@ -19,6 +19,7 @@ */ #include "redis_json.h" +#include #include "json.h" #include "lock_manager.h" @@ -573,6 +574,9 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisJson); + + std::unordered_map> dirty_keys {}; + auto s = batch->PutLogData(log_data.Encode()); if (!s.ok()) return s; @@ -583,18 +587,27 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector JsonMetadata metadata; JsonValue value; - if (auto s = read(ctx, ns_keys[i], &metadata, &value); s.IsNotFound()) { - if (paths[i] != "$") return rocksdb::Status::InvalidArgument("new objects must be created at the root"); - - value = *std::move(json_res); - } else { - if (!s.ok()) return s; - - JsonValue new_val = *std::move(json_res); - auto set_res = value.Set(paths[i], std::move(new_val)); + if (dirty_keys.count(ns_keys[i])) { + value = dirty_keys[ns_keys[i]].first; + auto set_res = value.Set(paths[i], *std::move(json_res)); if (!set_res) return rocksdb::Status::InvalidArgument(set_res.Msg()); + } else { + if (auto s = read(ctx, ns_keys[i], &metadata, &value); s.IsNotFound()) { + if (paths[i] != "$") return rocksdb::Status::InvalidArgument("new objects must be created at the root"); + value = *std::move(json_res); + } else { + if (!s.ok()) return s; + + auto set_res = value.Set(paths[i], *std::move(json_res)); + if (!set_res) return rocksdb::Status::InvalidArgument(set_res.Msg()); + } } + dirty_keys[ns_keys[i]] = std::make_pair(value, metadata); + } + + for (const auto& [ns_key, updated_object] : dirty_keys) { + auto [value, metadata] = updated_object; auto format = storage_->GetConfig()->json_storage_format; metadata.format = format; @@ -613,7 +626,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector return rocksdb::Status::InvalidArgument("Failed to encode JSON into storage: " + res.Msg()); } - s = batch->Put(metadata_cf_handle_, ns_keys[i], val); + s = batch->Put(metadata_cf_handle_, ns_key, val); if (!s.ok()) return s; } diff --git a/tests/gocase/unit/type/json/json_test.go b/tests/gocase/unit/type/json/json_test.go index b943a32a714..1b810fb73d8 100644 --- a/tests/gocase/unit/type/json/json_test.go +++ b/tests/gocase/unit/type/json/json_test.go @@ -651,6 +651,13 @@ func testJSON(t *testing.T, configs util.KvrocksServerConfigs) { EqualJSON(t, `[4]`, rdb.Do(ctx, "JSON.GET", "a1", "$.a").Val()) }) + t.Run("JSON.MSET multi-command", func(t *testing.T) { + require.NoError(t, rdb.Do(ctx, "JSON.SET", "doc", "$", `{"f1":{"a1":0},"f2":{"a2":0}}`).Err()) + require.NoError(t, rdb.Do(ctx, "JSON.MSET", "doc", "$.f1.a1", "1", "doc", "$.f2.a2", "2").Err()) + + EqualJSON(t, `{"f1":{"a1":1},"f2":{"a2":2}}`, rdb.Do(ctx, "JSON.GET", "doc").Val()) + }) + t.Run("JSON.DEBUG MEMORY basics", func(t *testing.T) { require.NoError(t, rdb.Do(ctx, "JSON.SET", "a", "$", `{"b":true,"x":1, "y":1.2, "z": {"x":[1,2,3], "y": null}, "v":{"x":"y"},"f":{"x":[]}}`).Err()) //object @@ -712,7 +719,6 @@ func testJSON(t *testing.T, configs util.KvrocksServerConfigs) { require.Equal(t, make([]interface{}, 0), rdb.Do(ctx, "JSON.RESP", "item:2", "$.a").Val()) }) - } func EqualJSON(t *testing.T, expected string, actual interface{}) { From c4115a6d9bb995d24376d6ff762cabe6dccb9d8d Mon Sep 17 00:00:00 2001 From: poipoiPIO Date: Sat, 5 Oct 2024 12:20:44 +0300 Subject: [PATCH 2/4] fix : format --- src/types/redis_json.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc index caf118581db..c56880660e3 100644 --- a/src/types/redis_json.cc +++ b/src/types/redis_json.cc @@ -19,6 +19,7 @@ */ #include "redis_json.h" + #include #include "json.h" @@ -575,7 +576,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisJson); - std::unordered_map> dirty_keys {}; + std::unordered_map> dirty_keys{}; auto s = batch->PutLogData(log_data.Encode()); if (!s.ok()) return s; @@ -606,7 +607,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector dirty_keys[ns_keys[i]] = std::make_pair(value, metadata); } - for (const auto& [ns_key, updated_object] : dirty_keys) { + for (const auto &[ns_key, updated_object] : dirty_keys) { auto [value, metadata] = updated_object; auto format = storage_->GetConfig()->json_storage_format; metadata.format = format; From 2af0d8452a9288b3d64e0ec0efa4f9089bd29896 Mon Sep 17 00:00:00 2001 From: lappely | Kirill Gnapovsky <82707867+poipoiPIO@users.noreply.github.com> Date: Sat, 5 Oct 2024 13:15:36 +0300 Subject: [PATCH 3/4] Code review requested changes Co-authored-by: hulk Co-authored-by: Twice --- src/types/redis_json.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc index c56880660e3..e243bf7cafd 100644 --- a/src/types/redis_json.cc +++ b/src/types/redis_json.cc @@ -576,6 +576,8 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisJson); + // A single JSON key may be modified multiple times in the MSET command, + // so we need to record them temporarily to avoid reading old values from DB. std::unordered_map> dirty_keys{}; auto s = batch->PutLogData(log_data.Encode()); @@ -588,6 +590,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector JsonMetadata metadata; JsonValue value; + // If a key has been modified before, just read from memory to find the modified value. if (dirty_keys.count(ns_keys[i])) { value = dirty_keys[ns_keys[i]].first; auto set_res = value.Set(paths[i], *std::move(json_res)); @@ -608,7 +611,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector } for (const auto &[ns_key, updated_object] : dirty_keys) { - auto [value, metadata] = updated_object; + auto &[value, metadata] = updated_object; auto format = storage_->GetConfig()->json_storage_format; metadata.format = format; From 00bec20a23a339fe7daaa82c8b63db35656f8f87 Mon Sep 17 00:00:00 2001 From: lappely | Kirill Gnapovsky <82707867+poipoiPIO@users.noreply.github.com> Date: Sat, 5 Oct 2024 13:53:52 +0300 Subject: [PATCH 4/4] Requested fix : Remove const modifier from loop Co-authored-by: Twice --- src/types/redis_json.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc index e243bf7cafd..5120573e7ce 100644 --- a/src/types/redis_json.cc +++ b/src/types/redis_json.cc @@ -610,7 +610,7 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector dirty_keys[ns_keys[i]] = std::make_pair(value, metadata); } - for (const auto &[ns_key, updated_object] : dirty_keys) { + for (auto &[ns_key, updated_object] : dirty_keys) { auto &[value, metadata] = updated_object; auto format = storage_->GetConfig()->json_storage_format; metadata.format = format;