yyqdbngt opened a new pull request, #5355:
URL: https://github.com/apache/rocketmq-dashboard/pull/5355
### Which Issue(s) This PR Fixes
- Fixes #5272
### Brief Description
All frames the client sees on the stdio channel are classified by
`decodeMCPMessage` in `rmqctl/internal/studio/mcp_message.go`. It recognized
a request
(`method` + `id`) and a notification (`method`), and everything else fell
through to a
single generic error:
```go
return decodedMCPMessage{}, fmt.Errorf("invalid MCP JSON-RPC message:
missing method")
```
A JSON-RPC **response** frame (`id`, no `method`) is such a frame, so it was
reported with
exactly the same text as a frame carrying neither field. The operator saw a
decode failure
that named no frame kind, and "missing method" reads like a malformed
request - it gives no
hint that the rejected frame was a response, i.e. not something this
transport originates.
The fix adds the frame kind that was missing (`mcp_message.go:92`, before
the generic
return), so each classification now has its own message:
```go
if hasID {
// An id without a method is a JSON-RPC response frame. This transport
// only carries client-originated requests and notifications, so name the
// frame kind instead of reporting the generic "missing method", which
// reads like a malformed request or notification.
return decodedMCPMessage{}, fmt.Errorf(
"invalid MCP JSON-RPC message: response frame (id without method); "
+
"expected a client-originated request or notification")
}
return decodedMCPMessage{}, fmt.Errorf("invalid MCP JSON-RPC message:
missing method")
```
The request, notification and unnamed-frame paths are byte-for-byte
unchanged, and the
generic message stays for the frame that really has no `id` and no `method`.
Note for the reviewer: an earlier open PR by another contributor (#5273)
targets this same
issue and the same branch of this function; this PR is my own independent
change and wording,
offered because the campaign working the current issue list asked for it.
Whichever wording
the maintainers prefer should win - there is nothing in this change worth a
conflict.
### How Did You Test This Change?
Red run on the unmodified tree, green run after the fix - same command, same
tests
(`rmqctl/internal/studio/mcp_frame_kind_test.go`, a new file, so it does not
touch the
existing `mcp_message_test.go`):
```powershell
$ cd rmqctl
$ go test ./internal/studio/ -run TestDecodeMCPMessage -count=1 -v
=== RUN
TestDecodeMCPMessageNamesTheFrameKind/response_frame_carrying_a_result
mcp_frame_kind_test.go:73:
decodeMCPMessage({"jsonrpc":"2.0","id":7,"result":{"ok":true}}) error =
"invalid MCP JSON-RPC message: missing method", want it to mention "response
frame"
mcp_frame_kind_test.go:73:
decodeMCPMessage({"jsonrpc":"2.0","id":8,"error":{...}}) error = "invalid MCP
JSON-RPC message: missing method", want it to mention "response frame"
mcp_frame_kind_test.go:73:
decodeMCPMessage({"jsonrpc":"2.0","id":"a3","result":null}) error = "invalid
MCP JSON-RPC message: missing method", want it to mention "response frame"
--- FAIL: TestDecodeMCPMessageNamesTheFrameKind (0.00s)
FAIL github.com/apache/rocketmq-dashboard/rmqctl/internal/studio 0.088s
```
```powershell
$ go test ./internal/studio/ -run TestDecodeMCPMessage -count=1 -v
--- PASS: TestDecodeMCPMessageNamesTheFrameKind (0.00s)
--- PASS:
TestDecodeMCPMessageNamesTheFrameKind/response_frame_carrying_a_result (0.00s)
--- PASS:
TestDecodeMCPMessageNamesTheFrameKind/response_frame_carrying_an_error (0.00s)
--- PASS:
TestDecodeMCPMessageNamesTheFrameKind/response_frame_with_a_null_result (0.00s)
--- PASS:
TestDecodeMCPMessageNamesTheFrameKind/frame_without_id_and_without_method
(0.00s)
--- PASS: TestDecodeMCPMessageKeepsDecodingClientFrames (0.00s)
PASS
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/studio 0.087s
```
What the test pins: for the three response-frame shapes (`result`, `error`,
`result: null`)
the error must mention the frame kind and must no longer contain the
misleading
`missing method` text; a frame with neither `id` nor `method` must keep the
`missing method` error and must *not* be called a response frame, so the two
cases stay
distinguishable; and request and notification frames must still decode. The
second test
covers that last part separately.
Rest of the module after the fix:
```powershell
$ go test ./internal/... ./cmd/... -count=1
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/catalog 0.068s
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/catalog/generate
0.139s
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/config 0.087s
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/output 0.072s
ok github.com/apache/rocketmq-dashboard/rmqctl/internal/studio 0.093s
ok github.com/apache/rocketmq-dashboard/rmqctl/cmd 2.105s
$ go vet ./internal/... ./cmd/... # clean; gofmt -l clean
```
(`go test ./...` also runs `rmqctl/scripts`, whose packaging test shells out
to `bash` and
fails on this Windows box for that unrelated reason; it fails identically
before the change.
The Go module needs go >= 1.27.1 and this machine's default GOPROXY is
unreachable here, so
the runs above used `GOPROXY=https://goproxy.cn,direct`.)
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits
- [x] Tests added or updated for non-trivial changes
- [ ] New UI text has both Chinese and English entries under `web/src/i18n/`
- not applicable, no UI text
- [ ] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks) - not applicable, Go module only
- [x] New source files carry the ASF license header
- [ ] Documentation touched where behaviour changed (README / `docs/` /
in-app help) - not applicable, the message is not documented anywhere; the
comment in `decodeMCPMessage` states the frame kinds
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
