Fix blob serialization in MCP output adaptation#1275
Conversation
| case BLOB -> Document.of( | ||
| Base64.getEncoder().encodeToString(ByteBufferUtils.getBytes(doc.asBlob()))); | ||
| default -> badType(doc.type(), toType); | ||
| }; |
There was a problem hiding this comment.
This is a incorrect fix and in the wrong layer. The premise of this fix is also incorrect
to a downstream Smithy service using RPC v2 CBOR, Blob fields in the response can be deserialized as string Documents (already base64-encoded) rather than typed blob Documents.
That's the representation over the wire not after materialization.
There was a problem hiding this comment.
The actual issue is Document.of(output) at McpService.java:704 round-trips JsonDocuments.StringDocument (has asBlob()) into core Documents.StringDocument (throws on asBlob()) before adaptOutputDocument resolves @OneOf document members containing blob fields. Should I fix it by skipping the round-trip (cast StructDocument directly since it implements Document), or add asBlob() to core Documents.StringDocument?
2 participants
What behavior changes?
adaptOutputDocumentno longer throwsSerializationException: Expected a blob document, but found stringwhen a downstream RPC v2 response contains a Blob field that arrives as an already-base64-encoded string Document. Instead, it passes the string through unchanged. Binary blob Documents continue to be base64-encoded as before.Why is this change needed?
When MCP proxies a
tools/callto a downstream Smithy service using RPC v2 CBOR, Blob fields in the response can be deserialized as string Documents (already base64-encoded) rather than typed blob Documents. The previous code unconditionally calleddoc.asBlob(), which throws when the document type is STRING.How was this validated?
McpServiceTestcovering all three branches: STRING passthrough, BLOB encode, and unsupported type errorMcpServerTestsuite passesWhat should reviewers focus on?
McpService.javalines 1319–1324: the new nested switch ondoc.type()inside theBLOBcaseMcpServiceTest.java: verify the test coverage is sufficient and the package-private visibility is acceptableAdditional Links
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.