Reduce scope of HELLO implementation

src/facade/facade_types.h

@@ -18,6 +18,11 @@ using MutableSlice = absl::Span<char>;
 using CmdArgList = absl::Span<MutableSlice>;
 using CmdArgVec = std::vector<MutableSlice>;
 
+struct CmdArgListFormatter {
+  void operator()(std::string* out, MutableSlice arg) const {
+    out->append(absl::StrCat("`", std::string_view(arg.data(), arg.size()), "`"));
+  }
+};
 
 struct ConnectionStats {
   absl::flat_hash_map<std::string, uint64_t> err_count_map;

src/server/dragonfly_test.cc

@@ -295,17 +295,22 @@ TEST_F(DflyEngineTest, EvalResp) {
 }
 
 TEST_F(DflyEngineTest, Hello) {
-  auto resp_no_param = Run({"hello"});
-  ASSERT_THAT(resp_no_param, ArrLen(12));
-
-  auto resp = Run({"hello", "2"});
+  auto resp = Run({"hello"});
+  ASSERT_THAT(resp, ArrLen(12));
+  resp = Run({"hello", "2"});
   ASSERT_THAT(resp, ArrLen(12));
-  EXPECT_THAT(resp.GetVec(),
-              ElementsAre("server", "redis", "version", ArgType(RespExpr::STRING), "proto",
-                          IntArg(2), "id", ArgType(RespExpr::INT64), "mode",
-                          "standalone", "role", "master"));
 
-  EXPECT_THAT(Run({"hello", "3"}), ErrArg("ERR NOPROTO unsupported protocol"));
+  EXPECT_THAT(resp.GetVec(), ElementsAre("server", "redis", "version", ArgType(RespExpr::STRING),
+                                         "proto", IntArg(2), "id", ArgType(RespExpr::INT64), "mode",
+                                         "standalone", "role", "master"));
+
+  // These are valid arguments to HELLO, however as they are not yet supported the implementation
+  // is degraded to 'unknown command'.
+  EXPECT_THAT(Run({"hello", "3"}),
+              ErrArg("ERR unknown command 'HELLO' with args beginning with: `3`"));
+  EXPECT_THAT(
+      Run({"hello", "2", "AUTH", "uname", "pwd"}),
+      ErrArg("ERR unknown command 'HELLO' with args beginning with: `2`, `AUTH`, `uname`, `pwd`"));
 }
 
 TEST_F(DflyEngineTest, EvalSha) {

src/server/server_family.cc

@@ -92,6 +92,11 @@ string UnknownSubCmd(string_view subcmd, string cmd) {
                       cmd, " HELP.");
 }
 
+string UnknownCmd(string cmd, CmdArgList args) {
+  return absl::StrCat("Unknown command '", cmd, "' with arguments beginning with: ",
+                      StrJoin(args.begin(), args.end(), ",", CmdArgListFormatter()));
+}
+
 string InferLoadFile(fs::path data_dir) {
   const auto& dbname = GetFlag(FLAGS_dbfilename);
 
@@ -953,11 +958,16 @@ void ServerFamily::Info(CmdArgList args, ConnectionContext* cntx) {
 }
 
 void ServerFamily::Hello(CmdArgList args, ConnectionContext* cntx) {
+  // Allow calling this commands with no arguments or protover=2
+  // technically that is all that is supported at the moment.
+  // For all other cases degrade to 'unknown command' so that clients
+  // checking for the existence of the command to detect if RESP3 is
+  // supported or whether authentication can be performed using HELLO
+  // will gracefully fallback to RESP2 and using the AUTH command explicitly.
   if (args.size() > 1) {
     string_view proto_version = ArgS(args, 1);
-
-    if (proto_version != "2") {
-      (*cntx)->SendError("NOPROTO unsupported protocol version");
+    if (proto_version != "2" || args.size() > 2) {
+      (*cntx)->SendError(UnknownCmd("HELLO", args.subspan(1)));
       return;
     }
   }

src/server/stream_family_test.cc

@@ -24,7 +24,7 @@ TEST_F(StreamFamilyTest, Add) {
   auto resp = Run({"xadd", "key", "*", "field", "value"});
   ASSERT_THAT(resp, ArgType(RespExpr::STRING));
   string id = string(ToSV(resp.GetBuf()));
-  EXPECT_TRUE(id.ends_with("-0")) << id;
+  EXPECT_THAT(id, EndsWith("-0"));
 
   resp = Run({"xrange", "null", "-", "+"});
   EXPECT_THAT(resp, ArrLen(0));