Support raw JSON-RPC messages from workers: Extension changes #107
Conversation
| } | ||
|
|
||
| var message = await context.Request.ReadFromJsonAsync<JsonRpcMessage>(McpJsonSerializerOptions.DefaultOptions, context.RequestAborted); | ||
| var message = await McpHttpUtility.ExtractJsonRpcMessageSseAsync(context.Request, McpJsonSerializerOptions.DefaultOptions, context.RequestAborted); |
There was a problem hiding this comment.
You could implement this as a HttpRequest extension
| context.Features.GetRequiredFeature<IHttpResponseBodyFeature>().DisableBuffering(); | ||
| } | ||
|
|
||
| internal static async ValueTask<JsonRpcMessage?> ProcessJsonRpcPayloadAsync(HttpRequest request, JsonSerializerOptions options, CancellationToken cancellationToken, bool unwrapOnly = false) |
There was a problem hiding this comment.
What "process" is happeing? Aren't we just reading? Wondering if there is a better name here
|
|
||
| internal static async ValueTask<JsonRpcMessage?> ProcessJsonRpcPayloadAsync(HttpRequest request, JsonSerializerOptions options, CancellationToken cancellationToken, bool unwrapOnly = false) | ||
| { | ||
| // Process the incoming request body as JSON. Support both raw JSON-RPC messages and |
There was a problem hiding this comment.
This might be better suited for a summary commnet above the method
| // containing only the inner content. If no wrapper is present, leave the original body intact. | ||
|
|
||
| // If the body is empty, return null. | ||
| if (request.ContentLength == null || request.ContentLength == 0) |
There was a problem hiding this comment.
ContentLength null does not necessarily mean that there is no request body. Do we want to check request.Body to be safe?
| } | ||
|
|
||
| // Read the request body into a JsonDocument for inspection. | ||
| request.EnableBuffering(); |
There was a problem hiding this comment.
Is this the first point of entry, could this have been set before we got to this method (middleware perhaps?) I don't think there is an issue setting this twice, but I wonder where is the right place
| } | ||
|
|
||
| // Read the request body into a JsonDocument for inspection. | ||
| request.EnableBuffering(); |
There was a problem hiding this comment.
Could another component have read the body before we get to this part of the code? If so, do we need to reset the position?
There was a problem hiding this comment.
It might be safer to always reset
| JsonElement messageElement = root; | ||
| bool isWrapped = false; | ||
|
|
||
| if (root.ValueKind == JsonValueKind.Object && |
There was a problem hiding this comment.
bool isWrapped =
root.ValueKind == JsonValueKind.Object &&
root.TryGetProperty("isFunctionsMcpResult", out var marker) &&
marker.ValueKind == JsonValueKind.True &&
root.TryGetProperty("content", out var messageElement);TryGetProperty is already setting a conent var so we don't really need to assign it again
There was a problem hiding this comment.
Not sure that root.TryGetProperty("content", out var messageElement); needs to be connected to the result of isFunctionsMcpResult?
| else | ||
| { | ||
| // Reset position so downstream readers can consume the original body. | ||
| request.Body.Seek(0, SeekOrigin.Begin); |
There was a problem hiding this comment.
For extra safety, maybe check CanSeek before doing this
| else | ||
| { | ||
| var raw = messageElement.GetRawText(); | ||
| return JsonSerializer.Deserialize<JsonRpcMessage>(raw, options); |
There was a problem hiding this comment.
What are we doing on json exceptions? And do we know that messageElement is json?
Issue describing the changes in this PR
resolves #26
Pull request checklist
release_notes.mdAdditional information
Adding support to the extension to support the following format: