diff --git a/.devcontainer/alpine/post-create.sh b/.devcontainer/alpine/post-create.sh index 0b6e937c3..4d5606ff2 100755 --- a/.devcontainer/alpine/post-create.sh +++ b/.devcontainer/alpine/post-create.sh @@ -1,5 +1,6 @@ #!/bin/bash containerWorkspaceFolder=$1 +git config --global --add safe.directory ${containerWorkspaceFolder} git config --global --add safe.directory ${containerWorkspaceFolder}/helio mkdir -p /root/.local/share/CMakeTools diff --git a/src/facade/reply_builder.h b/src/facade/reply_builder.h index df2c7efa9..39c1c9e72 100644 --- a/src/facade/reply_builder.h +++ b/src/facade/reply_builder.h @@ -476,6 +476,7 @@ class RedisReplyBuilder2 : public RedisReplyBuilder2Base { void SendSimpleStrArr(RedisReplyBuilder::StrSpan arr) override { SendSimpleStrArr2(arr); } + void SendStringArr(RedisReplyBuilder::StrSpan arr, CollectionType type = ARRAY) override { SendBulkStrArr(arr, type); } diff --git a/src/server/detail/wrapped_json_path.h b/src/server/detail/wrapped_json_path.h index fb2d2cf4e..fa5819e0d 100644 --- a/src/server/detail/wrapped_json_path.h +++ b/src/server/detail/wrapped_json_path.h @@ -4,15 +4,17 @@ #pragma once +#include + #include #include #include +#include "base/logging.h" #include "core/json/json_object.h" #include "core/json/path.h" #include "core/string_or_view.h" #include "facade/op_status.h" -#include "glog/logging.h" namespace dfly { @@ -25,7 +27,8 @@ template using JsonPathEvaluateCallback = absl::FunctionRef; template struct MutateCallbackResult { - MutateCallbackResult() = default; + MutateCallbackResult() { + } MutateCallbackResult(bool should_be_deleted_, T value_) : should_be_deleted(should_be_deleted_), value(std::move(value_)) { @@ -41,74 +44,83 @@ using JsonPathMutateCallback = namespace details { -template void OptionalEmplace(T value, std::optional* optional) { - optional->emplace(std::move(value)); +template +void OptionalEmplace(bool keep_defined, std::optional src, std::optional* dest) { + if (!keep_defined || !dest->has_value()) { + dest->swap(src); + } } -template -void OptionalEmplace(std::optional value, std::optional>* optional) { - if (value.has_value()) { - optional->emplace(std::move(value)); +template void OptionalEmplace(bool keep_defined, T src, T* dest) { + if (!keep_defined) { + *dest = std::move(src); } } } // namespace details template class JsonCallbackResult { + template struct is_optional : std::false_type {}; + + template struct is_optional> : std::true_type {}; + public: - /* In the case of a restricted path (legacy mode), the result consists of a single value */ - using JsonV1Result = std::optional; + JsonCallbackResult() { + } - /* In the case of an enhanced path (starts with $), the result is an array of multiple values */ - using JsonV2Result = std::vector; - - JsonCallbackResult() = default; - - explicit JsonCallbackResult(bool legacy_mode_is_enabled, bool save_first_result = false) - : save_first_result_(save_first_result) { - if (!legacy_mode_is_enabled) { - result_ = JsonV2Result{}; - } + JsonCallbackResult(bool legacy_mode_is_enabled, bool save_first_result, bool empty_is_nil) + : only_save_first_(save_first_result), + is_legacy_(legacy_mode_is_enabled), + empty_is_nil_(empty_is_nil) { } void AddValue(T value) { - if (IsV1()) { - if (!save_first_result_) { - details::OptionalEmplace(std::move(value), &AsV1()); - } else { - auto& as_v1 = AsV1(); - if (!as_v1.has_value()) { - details::OptionalEmplace(std::move(value), &as_v1); - } - } - } else { - AsV2().emplace_back(std::move(value)); + if (result_.empty() || !IsV1()) { + result_.push_back(std::move(value)); + return; } + + details::OptionalEmplace(only_save_first_, std::move(value), &result_.front()); } bool IsV1() const { - return std::holds_alternative(result_); + return is_legacy_; } - JsonV1Result& AsV1() { - return std::get(result_); + const T& AsV1() const { + return result_.front(); } - JsonV2Result& AsV2() { - return std::get(result_); + const absl::InlinedVector& AsV2() const { + return std::move(result_); } - const JsonV1Result& AsV1() const { - return std::get(result_); + bool Empty() const { + return result_.empty(); } - const JsonV2Result& AsV2() const { - return std::get(result_); + bool ShouldSendNil() const { + return is_legacy_ && empty_is_nil_ && result_.empty(); + } + + bool ShouldSendWrongType() const { + if (is_legacy_) { + if (result_.empty() && !empty_is_nil_) + return true; + + if constexpr (is_optional::value) { + return !result_.front().has_value(); + } + } + return false; } private: - std::variant result_; - bool save_first_result_ = false; + absl::InlinedVector result_; + + bool only_save_first_ = false; + bool is_legacy_ = false; + bool empty_is_nil_ = false; }; class WrappedJsonPath { @@ -137,7 +149,7 @@ class WrappedJsonPath { template JsonCallbackResult Evaluate(const JsonType* json_entry, JsonPathEvaluateCallback cb, bool save_first_result, bool legacy_mode_is_enabled) const { - JsonCallbackResult eval_result{legacy_mode_is_enabled, save_first_result}; + JsonCallbackResult eval_result{legacy_mode_is_enabled, save_first_result, true}; auto eval_callback = [&cb, &eval_result](std::string_view path, const JsonType& val) { eval_result.AddValue(cb(path, val)); @@ -159,14 +171,17 @@ class WrappedJsonPath { } template - OpResult> Mutate(JsonType* json_entry, JsonPathMutateCallback cb) const { - JsonCallbackResult mutate_result{IsLegacyModePath()}; + OpResult>> Mutate(JsonType* json_entry, + JsonPathMutateCallback cb) const { + JsonCallbackResult> mutate_result{IsLegacyModePath(), false, false}; auto mutate_callback = [&cb, &mutate_result](std::optional path, JsonType* val) -> bool { auto res = cb(path, val); if (res.value.has_value()) { mutate_result.AddValue(std::move(res.value).value()); + } else if (!mutate_result.IsV1()) { + mutate_result.AddValue(std::nullopt); } return res.should_be_deleted; }; diff --git a/src/server/json_family.cc b/src/server/json_family.cc index 57e57846d..b8881eb9a 100644 --- a/src/server/json_family.cc +++ b/src/server/json_family.cc @@ -104,9 +104,7 @@ ParseResult ParseJsonPath(std::string_view path) { namespace reply_generic { -template void Send(const std::optional& opt, RedisReplyBuilder* rb); -template void Send(const std::vector& vec, RedisReplyBuilder* rb); -template <> void Send(const std::vector& vec, RedisReplyBuilder* rb); +template void Send(I begin, I end, RedisReplyBuilder* rb); void Send(bool value, RedisReplyBuilder* rb) { rb->SendBulkString(value ? "true"sv : "false"sv); @@ -128,6 +126,10 @@ void Send(const std::string& value, RedisReplyBuilder* rb) { rb->SendBulkString(value); } +void Send(const std::vector& vec, RedisReplyBuilder* rb) { + Send(vec.begin(), vec.end(), rb); +} + void Send(const JsonType& value, RedisReplyBuilder* rb) { if (value.is_double()) { Send(value.as_double(), rb); @@ -164,26 +166,28 @@ template void Send(const std::optional& opt, RedisReplyBuilder* } } -template void Send(const std::vector& vec, RedisReplyBuilder* rb) { - if (vec.empty()) { +template void Send(I begin, I end, RedisReplyBuilder* rb) { + if (begin == end) { rb->SendEmptyArray(); } else { - rb->StartArray(vec.size()); - for (auto&& x : vec) { - Send(x, rb); + if constexpr (is_same_v) { + rb->SendStringArr(facade::OwnedArgSlice{begin, end}); + } else { + rb->StartArray(end - begin); + for (auto i = begin; i != end; ++i) { + Send(*i, rb); + } } } } -template <> void Send(const std::vector& vec, RedisReplyBuilder* rb) { - if (vec.empty()) { - rb->SendEmptyArray(); - } else { - rb->SendStringArr(vec); - } -} - template void Send(const JsonCallbackResult& result, RedisReplyBuilder* rb) { + if (result.ShouldSendNil()) + return rb->SendNull(); + + if (result.ShouldSendWrongType()) + return rb->SendError(OpStatus::WRONG_JSON_TYPE); + if (result.IsV1()) { /* The specified path was restricted (JSON legacy mode), then the result consists only of a * single value */ @@ -191,7 +195,8 @@ template void Send(const JsonCallbackResult& result, RedisReplyB } else { /* The specified path was enhanced (starts with '$'), then the result is an array of multiple * values */ - Send(result.AsV2(), rb); + const auto& arr = result.AsV2(); + Send(arr.begin(), arr.end(), rb); } } @@ -205,6 +210,8 @@ template void Send(const OpResult& result, RedisReplyBuilder* rb } // namespace reply_generic +using OptSize = optional; + facade::OpStatus SetJson(const OpArgs& op_args, string_view key, JsonType&& value) { auto& db_slice = op_args.GetDbSlice(); @@ -272,7 +279,7 @@ OpResult GetJson(const OpArgs& op_args, string_view key) { } // Returns the index of the next right bracket -optional GetNextIndex(string_view str) { +OptSize GetNextIndex(string_view str) { size_t current_idx = 0; while (current_idx + 1 < str.size()) { // ignore escaped character after the backslash (e.g. \'). @@ -361,7 +368,7 @@ string ConvertToJsonPointer(string_view json_path) { parts.emplace_back(json_path.substr(0, end_val_idx)); json_path.remove_prefix(end_val_idx + 1); } else if (is_object) { - optional end_val_idx = GetNextIndex(json_path); + OptSize end_val_idx = GetNextIndex(json_path); if (!end_val_idx) { invalid_syntax = true; break; @@ -444,26 +451,9 @@ size_t CountJsonFields(const JsonType& j) { return res; } -template struct is_optional : std::false_type {}; - -template struct is_optional> : std::true_type {}; - -template -OpResult> ReturnWrongTypeOnNullOpt(JsonCallbackResult result) { - if constexpr (is_optional::value) { - if (result.IsV1()) { - auto& as_v1 = result.AsV1(); - if (!as_v1 || !as_v1.value()) { - return OpStatus::WRONG_JSON_TYPE; - } - } - } - return result; -} - struct EvaluateOperationOptions { bool save_first_result = false; - bool return_empty_result_if_key_not_found = false; + bool return_nil_if_key_not_found = false; }; template @@ -472,19 +462,23 @@ OpResult> JsonEvaluateOperation(const OpArgs& op_args, std JsonPathEvaluateCallback cb, EvaluateOperationOptions options = {}) { OpResult result = GetJson(op_args, key); - if (options.return_empty_result_if_key_not_found && result == OpStatus::KEY_NOTFOUND) { - return JsonCallbackResult{}; + if (options.return_nil_if_key_not_found && result == OpStatus::KEY_NOTFOUND) { +// GCC 13.1 throws spurious warnings around this code. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" + return JsonCallbackResult{true, false, true}; // set legacy mode to return nil +#pragma GCC diagnostic pop } + RETURN_ON_BAD_STATUS(result); - return ReturnWrongTypeOnNullOpt( - json_path.Evaluate(result.value(), cb, options.save_first_result)); + return json_path.Evaluate(*result, cb, options.save_first_result); } template -OpResult> UpdateEntry(const OpArgs& op_args, std::string_view key, - const WrappedJsonPath& json_path, - JsonPathMutateCallback cb, - JsonReplaceVerify verify_op = {}) { +OpResult>> UpdateEntry(const OpArgs& op_args, std::string_view key, + const WrappedJsonPath& json_path, + JsonPathMutateCallback cb, + JsonReplaceVerify verify_op = {}) { auto it_res = op_args.GetDbSlice().FindMutable(op_args.db_cntx, key, OBJ_JSON); RETURN_ON_BAD_STATUS(it_res); @@ -507,7 +501,7 @@ OpResult> UpdateEntry(const OpArgs& op_args, std::string_v op_args.shard->search_indices()->AddDoc(key, op_args.db_cntx, pv); RETURN_ON_BAD_STATUS(mutate_res); - return ReturnWrongTypeOnNullOpt(*std::move(mutate_res)); + return mutate_res; } bool LegacyModeIsEnabled(const std::vector>& paths) { @@ -561,6 +555,8 @@ OpResult OpJsonGet(const OpArgs& op_args, string_view key, DCHECK(legacy_mode_is_enabled == eval_result.IsV1()); if (eval_result.IsV1()) { + if (eval_result.Empty()) + return nullopt; return eval_result.AsV1(); } @@ -598,45 +594,43 @@ auto OpType(const OpArgs& op_args, string_view key, const WrappedJsonPath& json_ return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), {false, true}); } -OpResult>> OpStrLen(const OpArgs& op_args, string_view key, - const WrappedJsonPath& json_path) { - auto cb = [](const string_view&, const JsonType& val) -> std::optional { +OpResult> OpStrLen(const OpArgs& op_args, string_view key, + const WrappedJsonPath& json_path) { + auto cb = [](const string_view&, const JsonType& val) -> OptSize { if (val.is_string()) { return val.as_string_view().size(); } else { - return std::nullopt; + return nullopt; } }; - return JsonEvaluateOperation>(op_args, key, json_path, std::move(cb), - {true, true}); + return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), {true, true}); } -OpResult>> OpObjLen(const OpArgs& op_args, string_view key, - const WrappedJsonPath& json_path) { - auto cb = [](const string_view&, const JsonType& val) -> std::optional { +OpResult> OpObjLen(const OpArgs& op_args, string_view key, + const WrappedJsonPath& json_path) { + auto cb = [](const string_view&, const JsonType& val) -> optional { if (val.is_object()) { return val.size(); } else { - return std::nullopt; + return nullopt; } }; - return JsonEvaluateOperation>(op_args, key, json_path, std::move(cb), - {true, true}); + return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), + {true, json_path.IsLegacyModePath()}); } -OpResult>> OpArrLen(const OpArgs& op_args, string_view key, - const WrappedJsonPath& json_path) { - auto cb = [](const string_view&, const JsonType& val) -> std::optional { +OpResult> OpArrLen(const OpArgs& op_args, string_view key, + const WrappedJsonPath& json_path) { + auto cb = [](const string_view&, const JsonType& val) -> OptSize { if (val.is_array()) { return val.size(); } else { return std::nullopt; } }; - return JsonEvaluateOperation>(op_args, key, json_path, std::move(cb), - {true, true}); + return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), {true, true}); } template @@ -649,7 +643,7 @@ auto OpToggle(const OpArgs& op_args, string_view key, *val = next_val; return {false, next_val}; } - return {false, std::nullopt}; + return {}; }; return UpdateEntry>(op_args, key, json_path, std::move(cb)); } @@ -825,18 +819,16 @@ auto OpObjKeys(const OpArgs& op_args, string_view key, const WrappedJsonPath& js } return vec; }; - return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), {true, true}); + + return JsonEvaluateOperation(op_args, key, json_path, std::move(cb), + {true, json_path.IsLegacyModePath()}); } -using StrAppendResult = std::optional; - -OpResult> OpStrAppend(const OpArgs& op_args, string_view key, - const WrappedJsonPath& path, - string_view value) { - auto cb = [&](std::optional, - JsonType* val) -> MutateCallbackResult { +OpResult> OpStrAppend(const OpArgs& op_args, string_view key, + const WrappedJsonPath& path, string_view value) { + auto cb = [&](optional, JsonType* val) -> MutateCallbackResult { if (!val->is_string()) - return {false, std::nullopt}; + return {}; string new_val = absl::StrCat(val->as_string_view(), value); size_t len = new_val.size(); @@ -844,7 +836,7 @@ OpResult> OpStrAppend(const OpArgs& op_args, return {false, len}; // do not delete, new value len }; - return UpdateEntry(op_args, key, path, std::move(cb)); + return UpdateEntry(op_args, key, path, std::move(cb)); } // Returns the numbers of values cleared. @@ -878,9 +870,9 @@ OpResult OpClear(const OpArgs& op_args, string_view key, const WrappedJson // Returns string vector that represents the pop out values. auto OpArrPop(const OpArgs& op_args, string_view key, WrappedJsonPath& path, int index) { auto cb = [index](std::optional, - JsonType* val) -> MutateCallbackResult> { + JsonType* val) -> MutateCallbackResult { if (!val->is_array() || val->empty()) { - return {false, std::nullopt}; + return {}; } size_t removal_index; @@ -907,16 +899,15 @@ auto OpArrPop(const OpArgs& op_args, string_view key, WrappedJsonPath& path, int val->erase(it); return {false, std::move(str)}; }; - return UpdateEntry>(op_args, key, path, std::move(cb)); + return UpdateEntry(op_args, key, path, std::move(cb)); } // Returns numeric vector that represents the new length of the array at each path. auto OpArrTrim(const OpArgs& op_args, string_view key, const WrappedJsonPath& path, int start_index, int stop_index) { - auto cb = [&](std::optional, - JsonType* val) -> MutateCallbackResult> { + auto cb = [&](optional, JsonType* val) -> MutateCallbackResult { if (!val->is_array()) { - return {false, std::nullopt}; + return {}; } if (val->empty()) { @@ -950,33 +941,32 @@ auto OpArrTrim(const OpArgs& op_args, string_view key, const WrappedJsonPath& pa *val = jsoncons::json_array(trim_start_it, trim_end_it); return {false, val->size()}; }; - return UpdateEntry>(op_args, key, path, std::move(cb)); + return UpdateEntry(op_args, key, path, std::move(cb)); } // Returns numeric vector that represents the new length of the array at each path. -OpResult>> OpArrInsert( - const OpArgs& op_args, string_view key, const WrappedJsonPath& json_path, int index, - const vector& new_values) { +OpResult> OpArrInsert(const OpArgs& op_args, string_view key, + const WrappedJsonPath& json_path, int index, + const vector& new_values) { bool out_of_boundaries_encountered = false; // Insert user-supplied value into the supplied index that should be valid. // If at least one index isn't valid within an array in the json doc, the operation is discarded. // Negative indexes start from the end of the array. - auto cb = [&](std::optional, - JsonType* val) -> MutateCallbackResult> { + auto cb = [&](std::optional, JsonType* val) -> MutateCallbackResult { if (out_of_boundaries_encountered) { return {}; } if (!val->is_array()) { - return {false, std::nullopt}; + return {}; } size_t removal_index; if (index < 0) { if (val->empty()) { out_of_boundaries_encountered = true; - return {false, std::nullopt}; + return {}; } int temp_index = index + val->size(); @@ -1003,7 +993,7 @@ OpResult>> OpArrInsert( return {false, val->size()}; }; - auto res = UpdateEntry>(op_args, key, json_path, std::move(cb)); + auto res = UpdateEntry(op_args, key, json_path, std::move(cb)); if (out_of_boundaries_encountered) { return OpStatus::OUT_OF_RANGE; } @@ -1015,7 +1005,7 @@ auto OpArrAppend(const OpArgs& op_args, string_view key, const WrappedJsonPath& auto cb = [&](std::optional, JsonType* val) -> MutateCallbackResult> { if (!val->is_array()) { - return {false, std::nullopt}; + return {}; } for (auto& new_val : append_values) { val->emplace_back(new_val); @@ -1316,8 +1306,11 @@ void JsonFamily::Set(CmdArgList args, ConnectionContext* cntx) { WrappedJsonPath json_path = GET_OR_SEND_UNEXPECTED(ParseJsonPath(path)); - bool is_nx_condition = static_cast(parser.Check("NX")); - bool is_xx_condition = static_cast(parser.Check("XX")); + int res = parser.HasNext() ? parser.Switch("NX", 1, "XX", 2) : 0; + bool is_xx_condition = (res == 2), is_nx_condition = (res == 1); + + if (parser.Error() || parser.HasNext()) // also clear the parser error dcheck + return cntx->SendError(kSyntaxErr); auto cb = [&](Transaction* t, EngineShard* shard) { return OpSet(t->GetOpArgs(shard), key, path, json_path, json_str, is_nx_condition, @@ -1472,7 +1465,7 @@ void JsonFamily::MGet(CmdArgList args, ConnectionContext* cntx) { } auto* rb = static_cast(cntx->reply_builder()); - reply_generic::Send(results, rb); + reply_generic::Send(results.begin(), results.end(), rb); } void JsonFamily::ArrIndex(CmdArgList args, ConnectionContext* cntx) { @@ -1908,7 +1901,7 @@ void JsonFamily::Register(CommandRegistry* registry) { *registry << CI{"JSON.STRLEN", CO::READONLY | CO::FAST, -2, 1, 1, acl::JSON}.HFUNC(StrLen); *registry << CI{"JSON.OBJLEN", CO::READONLY | CO::FAST, -2, 1, 1, acl::JSON}.HFUNC(ObjLen); *registry << CI{"JSON.ARRLEN", CO::READONLY | CO::FAST, -2, 1, 1, acl::JSON}.HFUNC(ArrLen); - *registry << CI{"JSON.TOGGLE", CO::WRITE | CO::FAST, -2, 1, 1, acl::JSON}.HFUNC(Toggle); + *registry << CI{"JSON.TOGGLE", CO::WRITE | CO::FAST, 3, 1, 1, acl::JSON}.HFUNC(Toggle); *registry << CI{"JSON.NUMINCRBY", CO::WRITE | CO::FAST, 4, 1, 1, acl::JSON}.HFUNC(NumIncrBy); *registry << CI{"JSON.NUMMULTBY", CO::WRITE | CO::FAST, 4, 1, 1, acl::JSON}.HFUNC(NumMultBy); *registry << CI{"JSON.DEL", CO::WRITE, -2, 1, 1, acl::JSON}.HFUNC(Del); diff --git a/src/server/json_family_test.cc b/src/server/json_family_test.cc index 0ba2849a8..e8c2c5050 100644 --- a/src/server/json_family_test.cc +++ b/src/server/json_family_test.cc @@ -326,7 +326,7 @@ TEST_F(JsonFamilyTest, Type) { "object", "array"))); resp = Run({"JSON.TYPE", "json", "$[10]"}); - EXPECT_THAT(resp.GetVec(), IsEmpty()); + EXPECT_THAT(resp, ArrLen(0)); resp = Run({"JSON.TYPE", "not_exist_key", "$[10]"}); EXPECT_THAT(resp, ArgType(RespExpr::NIL)); @@ -484,7 +484,7 @@ TEST_F(JsonFamilyTest, ObjLen) { EXPECT_THAT(resp, IntArg(3)); resp = Run({"JSON.OBJLEN", "non_existent_key", "$.a"}); - EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + EXPECT_THAT(resp, ErrArg("no such key")); /* Test response from several possible values @@ -515,7 +515,6 @@ TEST_F(JsonFamilyTest, ObjLenLegacy) { ASSERT_THAT(resp, "OK"); /* Test simple response from only one value */ - resp = Run({"JSON.STRLEN", "json"}); EXPECT_THAT(resp, ErrArg("wrong JSON type of path value")); @@ -523,7 +522,7 @@ TEST_F(JsonFamilyTest, ObjLenLegacy) { EXPECT_THAT(resp, IntArg(0)); resp = Run({"JSON.OBJLEN", "json", ".a.*"}); - EXPECT_THAT(resp, ErrArg("wrong JSON type of path value")); + EXPECT_THAT(resp, ArgType(RespExpr::NIL)); resp = Run({"JSON.OBJLEN", "json", ".b"}); EXPECT_THAT(resp, IntArg(1)); @@ -540,6 +539,9 @@ TEST_F(JsonFamilyTest, ObjLenLegacy) { resp = Run({"JSON.OBJLEN", "non_existent_key", ".a"}); EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + resp = Run({"JSON.OBJLEN", "json", ".none"}); + EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + /* Test response from several possible values In JSON legacy mode, the response contains only one value - the first object's length. @@ -580,7 +582,7 @@ TEST_F(JsonFamilyTest, ArrLen) { ArgType(RespExpr::NIL))); resp = Run({"JSON.OBJLEN", "non_existent_key", "$[*]"}); - EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + EXPECT_THAT(resp, ErrArg("no such key")); } TEST_F(JsonFamilyTest, ArrLenLegacy) { @@ -656,7 +658,7 @@ TEST_F(JsonFamilyTest, ToggleLegacy) { ASSERT_THAT(resp, "OK"); resp = Run({"JSON.TOGGLE", "json"}); - EXPECT_THAT(resp, ErrArg("wrong JSON type of path value")); + EXPECT_THAT(resp, ErrArg("wrong number of arguments")); resp = Run({"JSON.TOGGLE", "json", ".*"}); EXPECT_EQ(resp, "true"); @@ -1520,7 +1522,6 @@ TEST_F(JsonFamilyTest, StrAppendLegacyMode) { */ resp = Run({"JSON.STRAPPEND", "json", ".b.*", kVal}); - ASSERT_THAT(resp, ArgType(RespExpr::INT64)); EXPECT_THAT(resp, IntArg(2)); resp = Run({"JSON.GET", "json"}); diff --git a/tests/fakeredis/test/test_json/test_json.py b/tests/fakeredis/test/test_json/test_json.py index 9fd5d9591..150890ccc 100644 --- a/tests/fakeredis/test/test_json/test_json.py +++ b/tests/fakeredis/test/test_json/test_json.py @@ -549,10 +549,13 @@ def test_type(r: redis.Redis): } data = {k: {"a": meta_data[k]} for k in meta_data} r.json().set("doc1", "$", data) - # Test multi - assert r.json().type("doc1", "$..a") == [ - k.encode() for k in meta_data.keys() - ] # noqa: E721 + + # Dragonfly does not guarantee the traversal order for multi field traversal + # json.type api assumes a predefined order and is not designed very well. + # Test multi by comparing unordered sets + assert set(r.json().type("doc1", "$..a")) == set( + [k.encode() for k in meta_data.keys()] + ) # noqa: E721 # Test single assert r.json().type("doc1", "$.integer.a") == [b"integer"] # noqa: E721 @@ -614,7 +617,9 @@ def test_objkeys(r: redis.Redis): assert exp == keys r.json().set("obj", Path.root_path(), obj) - assert r.json().objkeys("obj") == list(obj.keys()) + + # Dragonfly does not guarantee the order (implementation detail) + assert set(r.json().objkeys("obj")) == obj.keys() assert r.json().objkeys("fakekey") is None @@ -629,10 +634,10 @@ def test_objkeys(r: redis.Redis): ) # Test single - assert r.json().objkeys("doc1", "$.nested1.a") == [[b"foo", b"bar"]] + assert set(r.json().objkeys("doc1", "$.nested1.a")[0]) == {b"foo", b"bar"} # Test legacy - assert r.json().objkeys("doc1", ".*.a") == ["foo", "bar"] + assert set(r.json().objkeys("doc1", ".*.a")) == {"foo", "bar"} # Test single assert r.json().objkeys("doc1", ".nested2.a") == ["baz"]