chore: introduce ParsedArguments class (#6169)

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,8 @@ 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(ParsedArgs{msg.args}, self->reply_builder_.get(),
+                                  self->cc_.get());
 
   self->last_interaction_ = time(nullptr);
   self->skip_next_squashing_ = false;
@@ -1219,7 +1219,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(ParsedArgs{tmp_cmd_vec_}, reply_builder_.get(), cc_.get());
   };
 
   auto dispatch_async = [this]() -> MessageHandle { return {FromArgs(tmp_parse_args_)}; };
@@ -1536,7 +1536,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 +1546,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;
+
+  explicit 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(facade::ParsedArgs{cmd_slices}, &reply_builder, context);
   facade::CapturingReplyBuilder::Payload payload = reply_builder.Take();
 
   auto response = http::MakeStringResponse();

src/server/journal/executor.cc

@@ -65,8 +65,8 @@ void JournalExecutor::FlushSlots(const cluster::SlotRange& slot_range) {
 }
 
 facade::DispatchResult JournalExecutor::Execute(journal::ParsedEntry::CmdData& cmd) {
-  auto span = CmdArgList{cmd.cmd_args.data(), cmd.cmd_args.size()};
-  return service_->DispatchCommand(span, &reply_builder_, &conn_context_);
+  return service_->DispatchCommand(facade::ParsedArgs{cmd.cmd_args}, &reply_builder_,
+                                   &conn_context_);
 }
 
 void JournalExecutor::SelectDb(DbIndex dbid) {