mirrors/dragonflydb-dragonfly
1
0
Fork 0
mirror of https://github.com/dragonflydb/dragonfly.git synced 2024-12-14 11:58:02 +00:00
Code Activity

fix(json_family): Fix JSON.ARRPOP command in legacy mode should not return WRONGTYPE error (#3683)

Browse source 
* fix(json_family): Fix WRONGTYPE error for the JSON legacy mode in the JSON.ARRPOP command

Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>

* refactor(json_family): address comments

Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>

* refactor(json_family): address comments 2

Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>

---------

Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>
This commit is contained in:
Stepan Bagritsevich 2024-09-18 07:24:18 +02:00 committed by GitHub
parent a64fc74ce1
commit 824af02f6f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 123 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

91
src/server/detail/wrapped_json_path.h
View file

@ -59,19 +59,46 @@ template <typename T> void OptionalEmplace(bool keep_defined, T src, T* dest) {
} // namespace details
enum class JsonPathType { kV2, kLegacy /*Or V1*/ };
constexpr JsonPathType kDefaultJsonPathType = JsonPathType::kV2;
struct CallbackResultOptions {
enum class SavingOrder { kSaveFirst, kSaveLast };
enum class OnEmpty { kSendNil, kSendWrongType };
CallbackResultOptions() = default;
explicit CallbackResultOptions(JsonPathType path_type_) : path_type(path_type_) {
}
explicit CallbackResultOptions(SavingOrder saving_order_) : saving_order(saving_order_) {
}
explicit CallbackResultOptions(OnEmpty on_empty_) : on_empty(on_empty_) {
}
CallbackResultOptions(JsonPathType path_type_, SavingOrder saving_order_, OnEmpty on_empty_)
: path_type(path_type_), saving_order(saving_order_), on_empty(on_empty_) {
}
std::optional<JsonPathType> path_type = std::nullopt;
SavingOrder saving_order{SavingOrder::kSaveLast};
OnEmpty on_empty{OnEmpty::kSendWrongType};
};
template <typename T> class JsonCallbackResult {
template <typename V> struct is_optional : std::false_type {};
template <typename V> struct is_optional<std::optional<V>> : std::true_type {};
public:
using SavingOrder = CallbackResultOptions::SavingOrder;
using OnEmpty = CallbackResultOptions::OnEmpty;
JsonCallbackResult() {
}
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) {
explicit JsonCallbackResult(CallbackResultOptions options) : options_(options) {
}
void AddValue(T value) {
@ -80,11 +107,12 @@ template <typename T> class JsonCallbackResult {
return;
}
details::OptionalEmplace(only_save_first_, std::move(value), &result_.front());
details::OptionalEmplace(options_.saving_order == SavingOrder::kSaveFirst, std::move(value),
&result_.front());
}
bool IsV1() const {
return is_legacy_;
return options_.path_type == JsonPathType::kLegacy;
}
const T& AsV1() const {
@ -100,12 +128,12 @@ template <typename T> class JsonCallbackResult {
}
bool ShouldSendNil() const {
return is_legacy_ && empty_is_nil_ && result_.empty();
return IsV1() && options_.on_empty == OnEmpty::kSendNil && result_.empty();
}
bool ShouldSendWrongType() const {
if (is_legacy_) {
if (result_.empty() && !empty_is_nil_)
if (IsV1()) {
if (result_.empty() && options_.on_empty == OnEmpty::kSendWrongType)
return true;
if constexpr (is_optional<T>::value) {
@ -117,10 +145,7 @@ template <typename T> class JsonCallbackResult {
private:
absl::InlinedVector<T, 2> result_;
bool only_save_first_ = false;
bool is_legacy_ = false;
bool empty_is_nil_ = false;
CallbackResultOptions options_{kDefaultJsonPathType};
};
class WrappedJsonPath {
@ -128,28 +153,19 @@ class WrappedJsonPath {
static constexpr std::string_view kV1PathRootElement = ".";
static constexpr std::string_view kV2PathRootElement = "$";
WrappedJsonPath(json::Path json_path, StringOrView path, bool is_legacy_mode_path)
: parsed_path_(std::move(json_path)),
path_(std::move(path)),
is_legacy_mode_path_(is_legacy_mode_path) {
WrappedJsonPath(json::Path json_path, StringOrView path, JsonPathType path_type)
: parsed_path_(std::move(json_path)), path_(std::move(path)), path_type_(path_type) {
}
WrappedJsonPath(JsonExpression expression, StringOrView path, bool is_legacy_mode_path)
: parsed_path_(std::move(expression)),
path_(std::move(path)),
is_legacy_mode_path_(is_legacy_mode_path) {
WrappedJsonPath(JsonExpression expression, StringOrView path, JsonPathType path_type)
: parsed_path_(std::move(expression)), path_(std::move(path)), path_type_(path_type) {
}
template <typename T>
JsonCallbackResult<T> Evaluate(const JsonType* json_entry, JsonPathEvaluateCallback<T> cb,
bool save_first_result) const {
return Evaluate(json_entry, cb, save_first_result, IsLegacyModePath());
}
template <typename T>
JsonCallbackResult<T> Evaluate(const JsonType* json_entry, JsonPathEvaluateCallback<T> cb,
bool save_first_result, bool legacy_mode_is_enabled) const {
JsonCallbackResult<T> eval_result{legacy_mode_is_enabled, save_first_result, true};
CallbackResultOptions options = {}) const {
JsonCallbackResult<T> eval_result{{options.path_type.value_or(path_type_), options.saving_order,
CallbackResultOptions::OnEmpty::kSendNil}};
auto eval_callback = [&cb, &eval_result](std::string_view path, const JsonType& val) {
eval_result.AddValue(cb(path, val));
@ -172,8 +188,9 @@ class WrappedJsonPath {
template <typename T>
OpResult<JsonCallbackResult<std::optional<T>>> Mutate(JsonType* json_entry,
JsonPathMutateCallback<T> cb) const {
JsonCallbackResult<std::optional<T>> mutate_result{IsLegacyModePath(), false, false};
JsonPathMutateCallback<T> cb,
CallbackResultOptions options = {}) const {
JsonCallbackResult<std::optional<T>> mutate_result{InitializePathType(options)};
auto mutate_callback = [&cb, &mutate_result](std::optional<std::string_view> path,
JsonType* val) -> bool {
@ -222,7 +239,7 @@ class WrappedJsonPath {
}
bool IsLegacyModePath() const {
return is_legacy_mode_path_;
return path_type_ == JsonPathType::kLegacy;
}
bool RefersToRootElement() const {
@ -242,10 +259,18 @@ class WrappedJsonPath {
return std::get<JsonExpression>(parsed_path_);
}
private:
CallbackResultOptions InitializePathType(CallbackResultOptions options) const {
if (!options.path_type) {
options.path_type = path_type_;
}
return options;
}
private:
std::variant<json::Path, JsonExpression> parsed_path_;
StringOrView path_;
bool is_legacy_mode_path_;
JsonPathType path_type_ = kDefaultJsonPathType;
};
} // namespace dfly

98
src/server/json_family.cc
View file

@ -61,14 +61,14 @@ ParseResult<JsonExpression> ParseJsonPathAsExpression(std::string_view path) {
return res;
}
ParseResult<WrappedJsonPath> ParseJsonPath(StringOrView path, bool is_legacy_mode_path) {
ParseResult<WrappedJsonPath> ParseJsonPath(StringOrView path, JsonPathType path_type) {
if (absl::GetFlag(FLAGS_jsonpathv2)) {
auto path_result = json::ParsePath(path.view());
if (!path_result) {
VLOG(1) << "Invalid Json path: " << path << ' ' << path_result.error() << std::endl;
return nonstd::make_unexpected(kSyntaxErr);
}
return WrappedJsonPath{std::move(path_result).value(), std::move(path), is_legacy_mode_path};
return WrappedJsonPath{std::move(path_result).value(), std::move(path), path_type};
}
auto expr_result = ParseJsonPathAsExpression(path.view());
@ -76,22 +76,23 @@ ParseResult<WrappedJsonPath> ParseJsonPath(StringOrView path, bool is_legacy_mod
VLOG(1) << "Invalid Json path: " << path << ' ' << expr_result.error() << std::endl;
return nonstd::make_unexpected(kSyntaxErr);
}
return WrappedJsonPath{std::move(expr_result).value(), std::move(path), is_legacy_mode_path};
return WrappedJsonPath{std::move(expr_result).value(), std::move(path), path_type};
}
ParseResult<WrappedJsonPath> ParseJsonPathV1(std::string_view path) {
if (path.empty() || path == WrappedJsonPath::kV1PathRootElement) {
return ParseJsonPath(StringOrView::FromView(WrappedJsonPath::kV2PathRootElement), true);
return ParseJsonPath(StringOrView::FromView(WrappedJsonPath::kV2PathRootElement),
JsonPathType::kLegacy);
}
std::string v2_path = absl::StrCat(
WrappedJsonPath::kV2PathRootElement, path.front() != '.' && path.front() != '[' ? "." : "",
path); // Convert to V2 path; TODO(path.front() != all kinds of symbols)
return ParseJsonPath(StringOrView::FromString(std::move(v2_path)), true);
return ParseJsonPath(StringOrView::FromString(std::move(v2_path)), JsonPathType::kLegacy);
}
ParseResult<WrappedJsonPath> ParseJsonPathV2(std::string_view path) {
return ParseJsonPath(StringOrView::FromView(path), false);
return ParseJsonPath(StringOrView::FromView(path), JsonPathType::kV2);
}
bool IsJsonPathV2(std::string_view path) {
@ -211,6 +212,8 @@ template <typename T> void Send(const OpResult<T>& result, RedisReplyBuilder* rb
} // namespace reply_generic
using OptSize = optional<size_t>;
using SavingOrder = CallbackResultOptions::SavingOrder;
using OnEmpty = CallbackResultOptions::OnEmpty;
struct JsonGetParams {
std::optional<std::string> indent;
@ -493,8 +496,8 @@ size_t CountJsonFields(const JsonType& j) {
}
struct EvaluateOperationOptions {
bool save_first_result = false;
bool return_nil_if_key_not_found = false;
CallbackResultOptions cb_result_options;
};
template <typename T>
@ -507,19 +510,27 @@ OpResult<JsonCallbackResult<T>> JsonEvaluateOperation(const OpArgs& op_args, std
// GCC 13.1 throws spurious warnings around this code.
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
return JsonCallbackResult<T>{true, false, true}; // set legacy mode to return nil
return JsonCallbackResult<T>{
{JsonPathType::kLegacy, options.cb_result_options.saving_order,
CallbackResultOptions::OnEmpty::kSendNil}}; // set legacy mode to return nil
#pragma GCC diagnostic pop
}
RETURN_ON_BAD_STATUS(result);
return json_path.Evaluate<T>(*result, cb, options.save_first_result);
return json_path.Evaluate<T>(*result, cb, options.cb_result_options);
}
struct MutateOperationOptions {
JsonReplaceVerify verify_op;
CallbackResultOptions cb_result_options;
};
template <typename T>
OpResult<JsonCallbackResult<optional<T>>> UpdateEntry(const OpArgs& op_args, std::string_view key,
const WrappedJsonPath& json_path,
JsonPathMutateCallback<T> cb,
JsonReplaceVerify verify_op = {}) {
OpResult<JsonCallbackResult<optional<T>>> JsonMutateOperation(const OpArgs& op_args,
std::string_view key,
const WrappedJsonPath& json_path,
JsonPathMutateCallback<T> cb,
MutateOperationOptions options = {}) {
auto it_res = op_args.GetDbSlice().FindMutable(op_args.db_cntx, key, OBJ_JSON);
RETURN_ON_BAD_STATUS(it_res);
@ -531,11 +542,11 @@ OpResult<JsonCallbackResult<optional<T>>> UpdateEntry(const OpArgs& op_args, std
op_args.shard->search_indices()->RemoveDoc(key, op_args.db_cntx, pv);
auto mutate_res = json_path.Mutate(json_val, cb);
auto mutate_res = json_path.Mutate(json_val, cb, options.cb_result_options);
// Make sure that we don't have other internal issue with the operation
if (mutate_res && verify_op) {
verify_op(*json_val);
if (mutate_res && options.verify_op) {
options.verify_op(*json_val);
}
it_res->post_updater.Run();
@ -584,13 +595,14 @@ OpResult<std::string> OpJsonGet(const OpArgs& op_args, string_view key,
options.after_key_chars(params.space.value());
}
const bool legacy_mode_is_enabled = LegacyModeIsEnabled(paths);
auto cb = [](std::string_view, const JsonType& val) { return val; };
auto eval_wrapped = [&json_entry, &cb, legacy_mode_is_enabled](
const WrappedJsonPath& json_path) -> std::optional<JsonType> {
auto eval_result = json_path.Evaluate<JsonType>(&json_entry, cb, false, legacy_mode_is_enabled);
const bool legacy_mode_is_enabled = LegacyModeIsEnabled(paths);
CallbackResultOptions cb_options{legacy_mode_is_enabled ? JsonPathType::kLegacy
: JsonPathType::kV2};
auto eval_wrapped = [&](const WrappedJsonPath& json_path) -> std::optional<JsonType> {
auto eval_result = json_path.Evaluate<JsonType>(&json_entry, cb, cb_options);
DCHECK(legacy_mode_is_enabled == eval_result.IsV1());
@ -631,7 +643,7 @@ auto OpType(const OpArgs& op_args, string_view key, const WrappedJsonPath& json_
auto cb = [](const string_view&, const JsonType& val) -> std::string {
return JsonTypeToName(val);
};
return JsonEvaluateOperation<std::string>(op_args, key, json_path, std::move(cb), {false, true});
return JsonEvaluateOperation<std::string>(op_args, key, json_path, std::move(cb), {true, {}});
}
OpResult<JsonCallbackResult<OptSize>> OpStrLen(const OpArgs& op_args, string_view key,
@ -644,7 +656,8 @@ OpResult<JsonCallbackResult<OptSize>> OpStrLen(const OpArgs& op_args, string_vie
}
};
return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb), {true, true});
return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb),
{true, CallbackResultOptions{SavingOrder::kSaveFirst}});
}
OpResult<JsonCallbackResult<OptSize>> OpObjLen(const OpArgs& op_args, string_view key,
@ -656,9 +669,9 @@ OpResult<JsonCallbackResult<OptSize>> OpObjLen(const OpArgs& op_args, string_vie
return nullopt;
}
};
return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb),
{true, json_path.IsLegacyModePath()});
return JsonEvaluateOperation<OptSize>(
op_args, key, json_path, std::move(cb),
{json_path.IsLegacyModePath(), CallbackResultOptions{SavingOrder::kSaveFirst}});
}
OpResult<JsonCallbackResult<OptSize>> OpArrLen(const OpArgs& op_args, string_view key,
@ -670,7 +683,8 @@ OpResult<JsonCallbackResult<OptSize>> OpArrLen(const OpArgs& op_args, string_vie
return std::nullopt;
}
};
return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb), {true, true});
return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb),
{true, CallbackResultOptions{SavingOrder::kSaveFirst}});
}
template <typename T>
@ -685,7 +699,7 @@ auto OpToggle(const OpArgs& op_args, string_view key,
}
return {};
};
return UpdateEntry<std::optional<T>>(op_args, key, json_path, std::move(cb));
return JsonMutateOperation<std::optional<T>>(op_args, key, json_path, std::move(cb));
}
template <typename T>
@ -777,7 +791,7 @@ OpResult<string> OpDoubleArithmetic(const OpArgs& op_args, string_view key,
return {};
};
auto res = UpdateEntry<Nothing>(op_args, key, json_path, std::move(cb));
auto res = JsonMutateOperation<Nothing>(op_args, key, json_path, std::move(cb));
if (is_result_overflow)
return OpStatus::INVALID_NUMERIC_RESULT;
@ -860,8 +874,9 @@ auto OpObjKeys(const OpArgs& op_args, string_view key, const WrappedJsonPath& js
return vec;
};
return JsonEvaluateOperation<StringVec>(op_args, key, json_path, std::move(cb),
{true, json_path.IsLegacyModePath()});
return JsonEvaluateOperation<StringVec>(
op_args, key, json_path, std::move(cb),
{json_path.IsLegacyModePath(), CallbackResultOptions{SavingOrder::kSaveFirst}});
}
OpResult<JsonCallbackResult<OptSize>> OpStrAppend(const OpArgs& op_args, string_view key,
@ -876,7 +891,7 @@ OpResult<JsonCallbackResult<OptSize>> OpStrAppend(const OpArgs& op_args, string_
return {false, len}; // do not delete, new value len
};
return UpdateEntry<size_t>(op_args, key, path, std::move(cb));
return JsonMutateOperation<size_t>(op_args, key, path, std::move(cb));
}
// Returns the numbers of values cleared.
@ -902,7 +917,7 @@ OpResult<long> OpClear(const OpArgs& op_args, string_view key, const WrappedJson
return {};
};
auto res = UpdateEntry<Nothing>(op_args, key, path, std::move(cb));
auto res = JsonMutateOperation<Nothing>(op_args, key, path, std::move(cb));
RETURN_ON_BAD_STATUS(res);
return clear_items;
}
@ -939,7 +954,9 @@ auto OpArrPop(const OpArgs& op_args, string_view key, WrappedJsonPath& path, int
val->erase(it);
return {false, std::move(str)};
};
return UpdateEntry<std::string>(op_args, key, path, std::move(cb));
return JsonMutateOperation<std::string>(
op_args, key, path, std::move(cb),
MutateOperationOptions{{}, CallbackResultOptions{OnEmpty::kSendNil}});
}
// Returns numeric vector that represents the new length of the array at each path.
@ -981,7 +998,7 @@ auto OpArrTrim(const OpArgs& op_args, string_view key, const WrappedJsonPath& pa
*val = jsoncons::json_array<JsonType>(trim_start_it, trim_end_it);
return {false, val->size()};
};
return UpdateEntry<size_t>(op_args, key, path, std::move(cb));
return JsonMutateOperation<size_t>(op_args, key, path, std::move(cb));
}
// Returns numeric vector that represents the new length of the array at each path.
@ -1033,7 +1050,7 @@ OpResult<JsonCallbackResult<OptSize>> OpArrInsert(const OpArgs& op_args, string_
return {false, val->size()};
};
auto res = UpdateEntry<size_t>(op_args, key, json_path, std::move(cb));
auto res = JsonMutateOperation<size_t>(op_args, key, json_path, std::move(cb));
if (out_of_boundaries_encountered) {
return OpStatus::OUT_OF_RANGE;
}
@ -1052,7 +1069,7 @@ auto OpArrAppend(const OpArgs& op_args, string_view key, const WrappedJsonPath&
}
return {false, val->size()};
};
return UpdateEntry<std::optional<std::size_t>>(op_args, key, path, std::move(cb));
return JsonMutateOperation<std::optional<std::size_t>>(op_args, key, path, std::move(cb));
}
// Returns a numeric vector representing each JSON value first index of the JSON scalar.
@ -1131,7 +1148,7 @@ std::vector<std::optional<std::string>> OpJsonMGet(const WrappedJsonPath& json_p
auto eval_wrapped = [&json_val,
&cb](const WrappedJsonPath& json_path) -> std::optional<JsonType> {
auto eval_result = json_path.Evaluate<JsonType>(json_val, std::move(cb), false);
auto eval_result = json_path.Evaluate<JsonType>(json_val, std::move(cb));
if (eval_result.IsV1()) {
return eval_result.AsV1();
@ -1249,7 +1266,8 @@ OpResult<bool> OpSet(const OpArgs& op_args, string_view key, string_view path,
return OpStatus::OK;
};
auto res = UpdateEntry<Nothing>(op_args, key, json_path, std::move(cb), inserter);
auto res = JsonMutateOperation<Nothing>(op_args, key, json_path, std::move(cb),
MutateOperationOptions{std::move(inserter), {}});
RETURN_ON_BAD_STATUS(res);
return operation_result;
}
@ -1325,7 +1343,7 @@ OpStatus OpMerge(const OpArgs& op_args, string_view key, string_view path,
return {};
};
auto res = UpdateEntry<Nothing>(op_args, key, json_path, std::move(cb));
auto res = JsonMutateOperation<Nothing>(op_args, key, json_path, std::move(cb));
if (res.status() != OpStatus::KEY_NOTFOUND)
return res.status();

8
src/server/json_family_test.cc
View file

@ -1813,7 +1813,13 @@ TEST_F(JsonFamilyTest, ArrPopLegacy) {
ASSERT_THAT(resp, "OK");
resp = Run({"JSON.ARRPOP", "json", "."});
EXPECT_THAT(resp, ErrArg("wrong JSON type of path value"));
EXPECT_THAT(resp, ArgType(RespExpr::NIL));
resp = Run({"JSON.SET", "json", ".", "[]"});
ASSERT_THAT(resp, "OK");
resp = Run({"JSON.ARRPOP", "json", "."});
EXPECT_THAT(resp, ArgType(RespExpr::NIL));
}
TEST_F(JsonFamilyTest, ArrTrim) {