diff --git a/src/facade/facade_types.h b/src/facade/facade_types.h index 9555be9e3..4549720a8 100644 --- a/src/facade/facade_types.h +++ b/src/facade/facade_types.h @@ -18,6 +18,11 @@ using MutableSlice = absl::Span; using CmdArgList = absl::Span; using CmdArgVec = std::vector; +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 err_count_map; diff --git a/src/server/dragonfly_test.cc b/src/server/dragonfly_test.cc index 512c2728e..298686002 100644 --- a/src/server/dragonfly_test.cc +++ b/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) { diff --git a/src/server/server_family.cc b/src/server/server_family.cc index 58ff3d53a..b9b967ea2 100644 --- a/src/server/server_family.cc +++ b/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 args 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; } } diff --git a/src/server/stream_family_test.cc b/src/server/stream_family_test.cc index 21faa1d9c..73485c020 100644 --- a/src/server/stream_family_test.cc +++ b/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));