fix: edge cases around mismatched path in json code (#3609)

For legacy mode:
1. For mutate commands, an empty result should throw an error
2. For read commands, it returns nil if path was not found, but if it was matched
   but only with a wrong types, it will throw an error.

For non-legacy mode, objlen should throw an error for non existing key.

Simplified JsonCallbackResult a bit and made sure more fakeredis tests are passing.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

.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

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);
   }

src/server/detail/wrapped_json_path.h

@@ -4,15 +4,17 @@
 
 #pragma once
 
+#include <absl/container/inlined_vector.h>
+
 #include <string_view>
 #include <utility>
 #include <variant>
 
+#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 <typename T>
 using JsonPathEvaluateCallback = absl::FunctionRef<T(std::string_view, const JsonType&)>;
 
 template <typename T = Nothing> 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 <typename T> void OptionalEmplace(T value, std::optional<T>* optional) {
+template <typename T>
-  optional->emplace(std::move(value));
+void OptionalEmplace(bool keep_defined, std::optional<T> src, std::optional<T>* dest) {
+  if (!keep_defined || !dest->has_value()) {
+    dest->swap(src);
+  }
 }
 
-template <typename T>
+template <typename T> void OptionalEmplace(bool keep_defined, T src, T* dest) {
-void OptionalEmplace(std::optional<T> value, std::optional<std::optional<T>>* optional) {
+  if (!keep_defined) {
-  if (value.has_value()) {
+    *dest = std::move(src);
-    optional->emplace(std::move(value));
   }
 }
 
 }  // namespace details
 
 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:
-  /* In the case of a restricted path (legacy mode), the result consists of a single value */
+  JsonCallbackResult() {
-  using JsonV1Result = std::optional<T>;
+  }
 
-  /* In the case of an enhanced path (starts with $), the result is an array of multiple values */
+  JsonCallbackResult(bool legacy_mode_is_enabled, bool save_first_result, bool empty_is_nil)
-  using JsonV2Result = std::vector<T>;
+      : only_save_first_(save_first_result),
-
+        is_legacy_(legacy_mode_is_enabled),
-  JsonCallbackResult() = default;
+        empty_is_nil_(empty_is_nil) {
-
-  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{};
-    }
   }
 
   void AddValue(T value) {
-    if (IsV1()) {
+    if (result_.empty() || !IsV1()) {
-      if (!save_first_result_) {
+      result_.push_back(std::move(value));
-        details::OptionalEmplace(std::move(value), &AsV1());
+      return;
-      } else {
-        auto& as_v1 = AsV1();
-        if (!as_v1.has_value()) {
-          details::OptionalEmplace(std::move(value), &as_v1);
-        }
-      }
-    } else {
-      AsV2().emplace_back(std::move(value));
     }
+
+    details::OptionalEmplace(only_save_first_, std::move(value), &result_.front());
   }
 
   bool IsV1() const {
-    return std::holds_alternative<JsonV1Result>(result_);
+    return is_legacy_;
   }
 
-  JsonV1Result& AsV1() {
+  const T& AsV1() const {
-    return std::get<JsonV1Result>(result_);
+    return result_.front();
   }
 
-  JsonV2Result& AsV2() {
+  const absl::InlinedVector<T, 2>& AsV2() const {
-    return std::get<JsonV2Result>(result_);
+    return std::move(result_);
   }
 
-  const JsonV1Result& AsV1() const {
+  bool Empty() const {
-    return std::get<JsonV1Result>(result_);
+    return result_.empty();
   }
 
-  const JsonV2Result& AsV2() const {
+  bool ShouldSendNil() const {
-    return std::get<JsonV2Result>(result_);
+    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<T>::value) {
+        return !result_.front().has_value();
+      }
+    }
+    return false;
   }
 
  private:
-  std::variant<JsonV1Result, JsonV2Result> result_;
+  absl::InlinedVector<T, 2> result_;
-  bool save_first_result_ = false;
+
+  bool only_save_first_ = false;
+  bool is_legacy_ = false;
+  bool empty_is_nil_ = false;
 };
 
 class WrappedJsonPath {
@@ -137,7 +149,7 @@ class WrappedJsonPath {
   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};
+    JsonCallbackResult<T> 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 <typename T>
-  OpResult<JsonCallbackResult<T>> Mutate(JsonType* json_entry, JsonPathMutateCallback<T> cb) const {
+  OpResult<JsonCallbackResult<std::optional<T>>> Mutate(JsonType* json_entry,
-    JsonCallbackResult<T> mutate_result{IsLegacyModePath()};
+                                                        JsonPathMutateCallback<T> cb) const {
+    JsonCallbackResult<std::optional<T>> mutate_result{IsLegacyModePath(), false, false};
 
     auto mutate_callback = [&cb, &mutate_result](std::optional<std::string_view> 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;
     };

src/server/json_family.cc

@@ -104,9 +104,7 @@ ParseResult<WrappedJsonPath> ParseJsonPath(std::string_view path) {
 
 namespace reply_generic {
 
-template <typename T> void Send(const std::optional<T>& opt, RedisReplyBuilder* rb);
+template <typename I> void Send(I begin, I end, RedisReplyBuilder* rb);
-template <typename T> void Send(const std::vector<T>& vec, RedisReplyBuilder* rb);
-template <> void Send(const std::vector<std::string>& vec, 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<std::string>& 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 <typename T> void Send(const std::optional<T>& opt, RedisReplyBuilder*
   }
 }
 
-template <typename T> void Send(const std::vector<T>& vec, RedisReplyBuilder* rb) {
+template <typename I> void Send(I begin, I end, RedisReplyBuilder* rb) {
-  if (vec.empty()) {
+  if (begin == end) {
     rb->SendEmptyArray();
   } else {
-    rb->StartArray(vec.size());
+    if constexpr (is_same_v<decltype(*begin), const string>) {
-    for (auto&& x : vec) {
+      rb->SendStringArr(facade::OwnedArgSlice{begin, end});
-      Send(x, rb);
+    } else {
+      rb->StartArray(end - begin);
+      for (auto i = begin; i != end; ++i) {
+        Send(*i, rb);
+      }
     }
   }
 }
 
-template <> void Send(const std::vector<std::string>& vec, RedisReplyBuilder* rb) {
-  if (vec.empty()) {
-    rb->SendEmptyArray();
-  } else {
-    rb->SendStringArr(vec);
-  }
-}
-
 template <typename T> void Send(const JsonCallbackResult<T>& 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 <typename T> void Send(const JsonCallbackResult<T>& 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 <typename T> void Send(const OpResult<T>& result, RedisReplyBuilder* rb
 
 }  // namespace reply_generic
 
+using OptSize = optional<size_t>;
+
 facade::OpStatus SetJson(const OpArgs& op_args, string_view key, JsonType&& value) {
   auto& db_slice = op_args.GetDbSlice();
 
@@ -272,7 +279,7 @@ OpResult<JsonType*> GetJson(const OpArgs& op_args, string_view key) {
 }
 
 // Returns the index of the next right bracket
-optional<size_t> 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<size_t> 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 <typename T> struct is_optional : std::false_type {};
-
-template <typename T> struct is_optional<std::optional<T>> : std::true_type {};
-
-template <typename T>
-OpResult<JsonCallbackResult<T>> ReturnWrongTypeOnNullOpt(JsonCallbackResult<T> result) {
-  if constexpr (is_optional<T>::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 <typename T>
@@ -472,19 +462,23 @@ OpResult<JsonCallbackResult<T>> JsonEvaluateOperation(const OpArgs& op_args, std
                                                       JsonPathEvaluateCallback<T> cb,
                                                       EvaluateOperationOptions options = {}) {
   OpResult<JsonType*> result = GetJson(op_args, key);
-  if (options.return_empty_result_if_key_not_found && result == OpStatus::KEY_NOTFOUND) {
+  if (options.return_nil_if_key_not_found && result == OpStatus::KEY_NOTFOUND) {
-    return JsonCallbackResult<T>{};
+// 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
+#pragma GCC diagnostic pop
   }
+
   RETURN_ON_BAD_STATUS(result);
-  return ReturnWrongTypeOnNullOpt(
+  return json_path.Evaluate<T>(*result, cb, options.save_first_result);
-      json_path.Evaluate<T>(result.value(), cb, options.save_first_result));
 }
 
 template <typename T>
-OpResult<JsonCallbackResult<T>> UpdateEntry(const OpArgs& op_args, std::string_view key,
+OpResult<JsonCallbackResult<optional<T>>> UpdateEntry(const OpArgs& op_args, std::string_view key,
-                                            const WrappedJsonPath& json_path,
+                                                      const WrappedJsonPath& json_path,
-                                            JsonPathMutateCallback<T> cb,
+                                                      JsonPathMutateCallback<T> cb,
-                                            JsonReplaceVerify verify_op = {}) {
+                                                      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<JsonCallbackResult<T>> 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<std::pair<std::string_view, WrappedJsonPath>>& paths) {
@@ -561,6 +555,8 @@ OpResult<std::string> 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<std::string>(op_args, key, json_path, std::move(cb), {false, true});
 }
 
-OpResult<JsonCallbackResult<std::optional<size_t>>> OpStrLen(const OpArgs& op_args, string_view key,
+OpResult<JsonCallbackResult<OptSize>> OpStrLen(const OpArgs& op_args, string_view key,
-                                                             const WrappedJsonPath& json_path) {
+                                               const WrappedJsonPath& json_path) {
-  auto cb = [](const string_view&, const JsonType& val) -> std::optional<std::size_t> {
+  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<std::optional<std::size_t>>(op_args, key, json_path, std::move(cb),
+  return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb), {true, true});
-                                                           {true, true});
 }
 
-OpResult<JsonCallbackResult<std::optional<size_t>>> OpObjLen(const OpArgs& op_args, string_view key,
+OpResult<JsonCallbackResult<OptSize>> OpObjLen(const OpArgs& op_args, string_view key,
-                                                             const WrappedJsonPath& json_path) {
+                                               const WrappedJsonPath& json_path) {
-  auto cb = [](const string_view&, const JsonType& val) -> std::optional<std::size_t> {
+  auto cb = [](const string_view&, const JsonType& val) -> optional<size_t> {
     if (val.is_object()) {
       return val.size();
     } else {
-      return std::nullopt;
+      return nullopt;
     }
   };
 
-  return JsonEvaluateOperation<std::optional<std::size_t>>(op_args, key, json_path, std::move(cb),
+  return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb),
-                                                           {true, true});
+                                        {true, json_path.IsLegacyModePath()});
 }
 
-OpResult<JsonCallbackResult<std::optional<size_t>>> OpArrLen(const OpArgs& op_args, string_view key,
+OpResult<JsonCallbackResult<OptSize>> OpArrLen(const OpArgs& op_args, string_view key,
-                                                             const WrappedJsonPath& json_path) {
+                                               const WrappedJsonPath& json_path) {
-  auto cb = [](const string_view&, const JsonType& val) -> std::optional<std::size_t> {
+  auto cb = [](const string_view&, const JsonType& val) -> OptSize {
     if (val.is_array()) {
       return val.size();
     } else {
       return std::nullopt;
     }
   };
-  return JsonEvaluateOperation<std::optional<std::size_t>>(op_args, key, json_path, std::move(cb),
+  return JsonEvaluateOperation<OptSize>(op_args, key, json_path, std::move(cb), {true, true});
-                                                           {true, true});
 }
 
 template <typename T>
@@ -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<std::optional<T>>(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<StringVec>(op_args, key, json_path, std::move(cb), {true, true});
+
+  return JsonEvaluateOperation<StringVec>(op_args, key, json_path, std::move(cb),
+                                          {true, json_path.IsLegacyModePath()});
 }
 
-using StrAppendResult = std::optional<std::size_t>;
+OpResult<JsonCallbackResult<OptSize>> OpStrAppend(const OpArgs& op_args, string_view key,
-
+                                                  const WrappedJsonPath& path, string_view value) {
-OpResult<JsonCallbackResult<StrAppendResult>> OpStrAppend(const OpArgs& op_args, string_view key,
+  auto cb = [&](optional<string_view>, JsonType* val) -> MutateCallbackResult<size_t> {
-                                                          const WrappedJsonPath& path,
-                                                          string_view value) {
-  auto cb = [&](std::optional<std::string_view>,
-                JsonType* val) -> MutateCallbackResult<StrAppendResult> {
     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<JsonCallbackResult<StrAppendResult>> OpStrAppend(const OpArgs& op_args,
     return {false, len};  // do not delete, new value len
   };
 
-  return UpdateEntry<StrAppendResult>(op_args, key, path, std::move(cb));
+  return UpdateEntry<size_t>(op_args, key, path, std::move(cb));
 }
 
 // Returns the numbers of values cleared.
@@ -878,9 +870,9 @@ OpResult<long> 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<std::string_view>,
-                    JsonType* val) -> MutateCallbackResult<std::optional<std::string>> {
+                    JsonType* val) -> MutateCallbackResult<std::string> {
     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<std::optional<std::string>>(op_args, key, path, std::move(cb));
+  return UpdateEntry<std::string>(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<std::string_view>,
+  auto cb = [&](optional<string_view>, JsonType* val) -> MutateCallbackResult<size_t> {
-                JsonType* val) -> MutateCallbackResult<std::optional<std::size_t>> {
     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<JsonType>(trim_start_it, trim_end_it);
     return {false, val->size()};
   };
-  return UpdateEntry<std::optional<std::size_t>>(op_args, key, path, std::move(cb));
+  return UpdateEntry<size_t>(op_args, key, path, std::move(cb));
 }
 
 // Returns numeric vector that represents the new length of the array at each path.
-OpResult<JsonCallbackResult<std::optional<std::size_t>>> OpArrInsert(
+OpResult<JsonCallbackResult<OptSize>> OpArrInsert(const OpArgs& op_args, string_view key,
-    const OpArgs& op_args, string_view key, const WrappedJsonPath& json_path, int index,
+                                                  const WrappedJsonPath& json_path, int index,
-    const vector<JsonType>& new_values) {
+                                                  const vector<JsonType>& 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<std::string_view>,
+  auto cb = [&](std::optional<std::string_view>, JsonType* val) -> MutateCallbackResult<size_t> {
-                JsonType* val) -> MutateCallbackResult<std::optional<std::size_t>> {
     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<JsonCallbackResult<std::optional<std::size_t>>> OpArrInsert(
     return {false, val->size()};
   };
 
-  auto res = UpdateEntry<std::optional<std::size_t>>(op_args, key, json_path, std::move(cb));
+  auto res = UpdateEntry<size_t>(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<std::string_view>,
                 JsonType* val) -> MutateCallbackResult<std::optional<std::size_t>> {
     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<bool>(parser.Check("NX"));
+  int res = parser.HasNext() ? parser.Switch("NX", 1, "XX", 2) : 0;
-  bool is_xx_condition = static_cast<bool>(parser.Check("XX"));
+  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<RedisReplyBuilder*>(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);

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"});

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") == [
+    # Dragonfly does not guarantee the traversal order for multi field traversal
-        k.encode() for k in meta_data.keys()
+    # json.type api assumes a predefined order and is not designed very well.
-    ]  # noqa: E721
+    # 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"]