chore: introduce ParsedArguments class

Currently we use ArgSlice, CmdArgList everywhere where we want a span on string_views.
As a result, it's very hard to understand the requirements from the passed classes.

This PR introduces a new ParsedArguments class and reduces the API surface for the passed object.
Make sure to unwind ParsedArguments to ArgSlice only when we start executing the command.

We will clean-up redundant aliases in the futures.

Signed-off-by: Roman Gershman <[email protected]>

Author: romange

src/facade/cmd_arg_parser.h

@@ -56,7 +56,7 @@ struct CmdArgParser {
   };
 
  public:
-  CmdArgParser(CmdArgList args) : args_{args} {
+  CmdArgParser(ArgSlice args) : args_{args} {
   }
 
   // Debug asserts sure error was consumed
@@ -159,7 +159,7 @@ struct CmdArgParser {
   }
 
   // Return remaining arguments
-  CmdArgList Tail() const {
+  ArgSlice Tail() const {
     return args_.subspan(cur_i_);
   }
 
@@ -168,7 +168,7 @@ struct CmdArgParser {
     return cur_i_ < args_.size() && !error_;
   }
 
-  bool HasError() {
+  bool HasError() const {
     return error_.type != ErrorType::NO_ERROR;
   }
 
@@ -275,7 +275,7 @@ struct CmdArgParser {
 
  private:
   size_t cur_i_ = 0;
-  CmdArgList args_;
+  ArgSlice args_;
 
   ErrorInfo error_;
 };

src/facade/command_id.h

@@ -4,16 +4,14 @@
 
 #pragma once
 
+#include <cstdint>
+#include <string>
 #include <string_view>
 
-#include "facade/facade_types.h"
-
 namespace facade {
 
 class CommandId {
  public:
-  using CmdArgList = facade::CmdArgList;
-
   /**
    * @brief Construct a new Command Id object
    *

src/facade/dragonfly_connection.cc

@@ -558,8 +558,7 @@ void Connection::AsyncOperations::operator()(Connection::PipelineMessage& msg) {
   DVLOG(2) << "Dispatching pipeline: " << ToSV(msg.args.front());
 
   ++self->local_stats_.cmds;
-  self->service_->DispatchCommand(CmdArgList{msg.args.data(), msg.args.size()},
-                                  self->reply_builder_.get(), self->cc_.get());
+  self->service_->DispatchCommand({msg.args}, self->reply_builder_.get(), self->cc_.get());
 
   self->last_interaction_ = time(nullptr);
   self->skip_next_squashing_ = false;
@@ -1219,7 +1218,7 @@ Connection::ParserStatus Connection::ParseRedis(unsigned max_busy_cycles) {
 
   auto dispatch_sync = [this] {
     RespExpr::VecToArgList(tmp_parse_args_, &tmp_cmd_vec_);
-    service_->DispatchCommand(absl::MakeSpan(tmp_cmd_vec_), reply_builder_.get(), cc_.get());
+    service_->DispatchCommand({tmp_cmd_vec_}, reply_builder_.get(), cc_.get());
   };
 
   auto dispatch_async = [this]() -> MessageHandle { return {FromArgs(tmp_parse_args_)}; };
@@ -1536,7 +1535,7 @@ void Connection::SquashPipeline() {
   DCHECK_EQ(dispatch_q_.size(), pending_pipeline_cmd_cnt_);
   DCHECK_EQ(reply_builder_->GetProtocol(), Protocol::REDIS);  // Only Redis is supported.
 
-  vector<ArgSlice> squash_cmds;
+  vector<ParsedArgs> squash_cmds;
   squash_cmds.reserve(dispatch_q_.size());
 
   uint64_t start = CycleClock::Now();
@@ -1546,7 +1545,7 @@ void Connection::SquashPipeline() {
         << msg.handle.index() << " on " << DebugInfo();
 
     auto& pmsg = get<PipelineMessagePtr>(msg.handle);
-    squash_cmds.emplace_back(absl::MakeSpan(pmsg->args));
+    squash_cmds.emplace_back(ParsedArgs(pmsg->args));
     if (squash_cmds.size() >= pipeline_squash_limit_cached) {
       // We reached the limit of commands to squash, so we dispatch them.
       break;

src/facade/facade_types.h

@@ -41,6 +41,37 @@ using CmdArgVec = std::vector<std::string_view>;
 using ArgSlice = absl::Span<const std::string_view>;
 using OwnedArgSlice = absl::Span<const std::string>;
 
+class ParsedArgs {
+ public:
+  ParsedArgs() = default;
+
+  ParsedArgs(ArgSlice args) : args_(args) {  // NOLINT google-explicit-constructor
+  }
+
+  size_t size() const {
+    return args_.size();
+  }
+
+  bool empty() const {
+    return args_.empty();
+  }
+
+  ParsedArgs Tail() const {
+    return ParsedArgs{args_.subspan(1)};
+  }
+
+  std::string_view Front() const {
+    return args_.front();
+  }
+
+  ArgSlice ToSlice() const {
+    return args_;
+  }
+
+ private:
+  absl::Span<const std::string_view> args_;
+};
+
 inline std::string_view ToSV(std::string_view slice) {
   return slice;
 }

src/facade/ok_main.cc

@@ -22,13 +22,13 @@ namespace {
 
 class OkService : public ServiceInterface {
  public:
-  DispatchResult DispatchCommand(ArgSlice args, SinkReplyBuilder* builder,
+  DispatchResult DispatchCommand(ParsedArgs args, SinkReplyBuilder* builder,
                                  ConnectionContext* cntx) final {
     builder->SendOk();
     return DispatchResult::OK;
   }
 
-  DispatchManyResult DispatchManyCommands(absl::Span<ArgSlice> args_lists,
+  DispatchManyResult DispatchManyCommands(absl::Span<ParsedArgs> args_lists,
                                           SinkReplyBuilder* builder,
                                           ConnectionContext* cntx) final {
     for (auto args : args_lists)

src/facade/service_interface.h

@@ -36,10 +36,10 @@ class ServiceInterface {
   virtual ~ServiceInterface() {
   }
 
-  virtual DispatchResult DispatchCommand(ArgSlice args, SinkReplyBuilder* builder,
+  virtual DispatchResult DispatchCommand(ParsedArgs args, SinkReplyBuilder* builder,
                                          ConnectionContext* cntx) = 0;
 
-  virtual DispatchManyResult DispatchManyCommands(absl::Span<ArgSlice> commands,
+  virtual DispatchManyResult DispatchManyCommands(absl::Span<ParsedArgs> commands,
                                                   SinkReplyBuilder* builder,
                                                   ConnectionContext* cntx) = 0;
 

src/server/command_registry.cc

@@ -325,17 +325,17 @@ CommandRegistry::FamiliesVec CommandRegistry::GetFamilies() {
   return std::move(family_of_commands_);
 }
 
-std::pair<const CommandId*, ArgSlice> CommandRegistry::FindExtended(string_view cmd,
-                                                                    ArgSlice tail_args) const {
+std::pair<const CommandId*, ParsedArgs> CommandRegistry::FindExtended(string_view cmd,
+                                                                      ParsedArgs tail_args) const {
   if (cmd == RenamedOrOriginal("ACL"sv)) {
     if (tail_args.empty()) {
       return {Find(cmd), {}};
     }
 
-    auto second_cmd = absl::AsciiStrToUpper(ArgS(tail_args, 0));
+    auto second_cmd = absl::AsciiStrToUpper(tail_args.Front());
     string full_cmd = absl::StrCat(cmd, " ", second_cmd);
 
-    return {Find(full_cmd), tail_args.subspan(1)};
+    return {Find(full_cmd), tail_args.Tail()};
   }
 
   const CommandId* res = Find(cmd);
@@ -344,7 +344,7 @@ std::pair<const CommandId*, ArgSlice> CommandRegistry::FindExtended(string_view
 
   // A workaround for XGROUP HELP that does not fit our static taxonomy of commands.
   if (tail_args.size() == 1 && res->name() == "XGROUP") {
-    if (absl::EqualsIgnoreCase(ArgS(tail_args, 0), "HELP")) {
+    if (absl::EqualsIgnoreCase(tail_args.Front(), "HELP")) {
       res = Find("_XGROUP_HELP");
     }
   }

src/server/command_registry.h

@@ -14,6 +14,7 @@
 
 #include "base/function2.hpp"
 #include "facade/command_id.h"
+#include "facade/facade_types.h"
 
 namespace facade {
 class SinkReplyBuilder;
@@ -107,6 +108,8 @@ template <typename T> class MoveOnly {
 
 class CommandId : public facade::CommandId {
  public:
+  using CmdArgList = facade::CmdArgList;
+
   // NOTICE: name must be a literal string, otherwise metrics break! (see cmd_stats_map in
   // server_state.h)
   CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key, int8_t last_key,
@@ -251,8 +254,8 @@ class CommandRegistry {
   using FamiliesVec = std::vector<std::vector<std::string>>;
   FamiliesVec GetFamilies();
 
-  std::pair<const CommandId*, facade::ArgSlice> FindExtended(std::string_view cmd,
-                                                             facade::ArgSlice tail_args) const;
+  std::pair<const CommandId*, facade::ParsedArgs> FindExtended(std::string_view cmd,
+                                                               facade::ParsedArgs tail_args) const;
 
   absl::flat_hash_map<std::string, hdr_histogram*> LatencyMap() const;
 

src/server/http_api.cc

@@ -198,7 +198,7 @@ void HttpAPI(const http::QueryArgs& args, HttpRequest&& req, Service* service,
   facade::CapturingReplyBuilder reply_builder;
 
   // TODO: to finish this.
-  service->DispatchCommand(absl::MakeSpan(cmd_slices), &reply_builder, context);
+  service->DispatchCommand({cmd_slices}, &reply_builder, context);
   facade::CapturingReplyBuilder::Payload payload = reply_builder.Take();
 
   auto response = http::MakeStringResponse();

src/server/main_service.cc

@@ -1423,16 +1423,16 @@ std::optional<ErrorReply> Service::VerifyCommandState(const CommandId* cid, CmdA
   return VerifyConnectionAclStatus(cid, &dfly_cntx, "has no ACL permissions", tail_args);
 }
 
-DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder,
+DispatchResult Service::DispatchCommand(facade::ParsedArgs args, SinkReplyBuilder* builder,
                                         facade::ConnectionContext* cntx) {
   DCHECK(!args.empty());
   DCHECK_NE(0u, shard_set->size()) << "Init was not called";
 
   absl::Cleanup clear_last_error([builder]() { builder->ConsumeLastError(); });
   ServerState& etl = *ServerState::tlocal();
 
-  string cmd = absl::AsciiStrToUpper(args[0]);
-  const auto [cid, args_no_cmd] = registry_.FindExtended(cmd, args.subspan(1));
+  string cmd = absl::AsciiStrToUpper(args.Front());
+  const auto [cid, args_no_cmd] = registry_.FindExtended(cmd, args.Tail());
 
   if (cid == nullptr) {
     builder->SendError(ReportUnknownCmd(cmd));
@@ -1446,7 +1446,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
 
   if (VLOG_IS_ON(2) && cntx->conn() /* no owner in replica context */) {
     LOG(INFO) << "Got (" << cntx->conn()->GetClientId() << "): " << (under_script ? "LUA " : "")
-              << args << " in dbid=" << dfly_cntx->conn_state.db_index;
+              << args.ToSlice() << " in dbid=" << dfly_cntx->conn_state.db_index;
   }
 
   // Don't interrupt running multi commands or admin connections.
@@ -1460,7 +1460,8 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
     cntx->paused = false;
   }
 
-  if (auto err = VerifyCommandState(cid, args_no_cmd, *dfly_cntx); err) {
+  auto tail_args = args_no_cmd.ToSlice();
+  if (auto err = VerifyCommandState(cid, tail_args, *dfly_cntx); err) {
     LOG_IF(WARNING, cntx->replica_conn || !cntx->conn() /* no owner in replica context */)
         << "VerifyCommandState error: " << err->ToSv();
     if (auto& exec_info = dfly_cntx->conn_state.exec_info; exec_info.IsCollecting())
@@ -1469,7 +1470,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
     // We need to skip this because ACK's should not be replied to
     // Bonus points because this allows to continue replication with ACL users who got
     // their access revoked and reinstated
-    if (cid->name() == "REPLCONF" && absl::EqualsIgnoreCase(ArgS(args_no_cmd, 0), "ACK")) {
+    if (cid->name() == "REPLCONF" && absl::EqualsIgnoreCase(args_no_cmd.Front(), "ACK")) {
       server_family_.GetDflyCmd()->OnClose(dfly_cntx->conn_state.replication_info.repl_session_id);
       return DispatchResult::ERROR;
     }
@@ -1486,7 +1487,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
     // TODO: protect against aggregating huge transactions.
     auto& exec_info = dfly_cntx->conn_state.exec_info;
     const size_t old_size = exec_info.GetStoredCmdBytes();
-    exec_info.AddStoredCmd(cid, true, args_no_cmd);
+    exec_info.AddStoredCmd(cid, true, tail_args);
     etl.stats.stored_cmd_bytes += exec_info.GetStoredCmdBytes() - old_size;
     if (cid->IsWriteOnly()) {
       exec_info.is_write = true;
@@ -1503,7 +1504,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
     if (cid->IsTransactional()) {
       dfly_cntx->transaction->MultiSwitchCmd(cid);
       OpStatus status = dfly_cntx->transaction->InitByArgs(
-          dfly_cntx->ns, dfly_cntx->conn_state.db_index, args_no_cmd);
+          dfly_cntx->ns, dfly_cntx->conn_state.db_index, tail_args);
 
       if (status != OpStatus::OK) {
         builder->SendError(status);
@@ -1519,7 +1520,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
       if (!dist_trans->IsMulti()) {  // Multi command initialize themself based on their mode.
         CHECK(dfly_cntx->ns != nullptr);
         if (auto st =
-                dist_trans->InitByArgs(dfly_cntx->ns, dfly_cntx->conn_state.db_index, args_no_cmd);
+                dist_trans->InitByArgs(dfly_cntx->ns, dfly_cntx->conn_state.db_index, tail_args);
             st != OpStatus::OK) {
           builder->SendError(st);
           return DispatchResult::ERROR;
@@ -1535,8 +1536,7 @@ DispatchResult Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder
 
   dfly_cntx->cid = cid;
 
-  auto res =
-      InvokeCmd(cid, args_no_cmd, CommandContext{dfly_cntx->transaction, builder, dfly_cntx});
+  auto res = InvokeCmd(cid, tail_args, CommandContext{dfly_cntx->transaction, builder, dfly_cntx});
   if ((res != DispatchResult::OK) && (res != DispatchResult::OOM)) {
     builder->SendError("Internal Error");
     builder->CloseConnection();
@@ -1722,7 +1722,7 @@ DispatchResult Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args,
   return res;
 }
 
-DispatchManyResult Service::DispatchManyCommands(absl::Span<CmdArgList> args_list,
+DispatchManyResult Service::DispatchManyCommands(absl::Span<facade::ParsedArgs> args_list,
                                                  SinkReplyBuilder* builder,
                                                  facade::ConnectionContext* cntx) {
   ConnectionContext* dfly_cntx = static_cast<ConnectionContext*>(cntx);
@@ -1768,9 +1768,9 @@ DispatchManyResult Service::DispatchManyCommands(absl::Span<CmdArgList> args_lis
     stored_cmds.clear();
   };
 
-  for (auto args : args_list) {
-    string cmd = absl::AsciiStrToUpper(ArgS(args, 0));
-    const auto [cid, tail_args] = registry_.FindExtended(cmd, args.subspan(1));
+  for (const auto& args : args_list) {
+    string cmd = absl::AsciiStrToUpper(args.Front());
+    const auto [cid, tail_args] = registry_.FindExtended(cmd, args.Tail());
 
     // MULTI...EXEC commands need to be collected into a single context, so squashing is not
     // possible
@@ -1787,7 +1787,7 @@ DispatchManyResult Service::DispatchManyCommands(absl::Span<CmdArgList> args_lis
 
     if (!is_multi && !is_eval && !is_blocking && cid != nullptr) {
       stored_cmds.reserve(args_list.size());
-      stored_cmds.emplace_back(cid, false /* do not deep-copy commands*/, tail_args);
+      stored_cmds.emplace_back(cid, false /* do not deep-copy commands*/, tail_args.ToSlice());
       continue;
     }
 
@@ -1945,7 +1945,7 @@ void Service::DispatchMC(const MemcacheParser::Command& cmd, std::string_view va
     }
   }
 
-  DispatchCommand(CmdArgList{args}, mc_builder, cntx);
+  DispatchCommand(ParsedArgs{args}, mc_builder, cntx);
 
   // Reset back.
   dfly_cntx->conn_state.memcache_flag = 0;