-
Notifications
You must be signed in to change notification settings - Fork 4
Support raw JSON-RPC messages from workers: Extension changes #107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,9 @@ | |
using Microsoft.AspNetCore.Http.Features; | ||
using Microsoft.Extensions.Primitives; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
using ModelContextProtocol.Protocol; | ||
using static Microsoft.Azure.Functions.Extensions.Mcp.McpConstants; | ||
|
||
namespace Microsoft.Azure.Functions.Extensions.Mcp.Http; | ||
|
@@ -39,4 +42,82 @@ internal static void SetSseContext(HttpContext context) | |
|
||
context.Features.GetRequiredFeature<IHttpResponseBodyFeature>().DisableBuffering(); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be better suited for a summary commnet above the method |
||
// wrapped payloads with the shape: { "isFunctionsMcpResult": true, "content": <JSON-RPC message> } | ||
// | ||
// When unwrapOnly is false: If the wrapper is present, deserialize the inner "content" as the JsonRpcMessage. | ||
// Otherwise, deserialize the root object directly as a JsonRpcMessage and return it. | ||
// | ||
// When unwrapOnly is true: If the wrapper is present, replace the request.Body stream with a memory stream | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ContentLength null does not necessarily mean that there is no request body. Do we want to check request.Body to be safe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also use CanRead |
||
{ | ||
return null; | ||
} | ||
|
||
// Read the request body into a JsonDocument for inspection. | ||
request.EnableBuffering(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be safer to always reset |
||
try | ||
{ | ||
using var doc = await JsonDocument.ParseAsync(request.Body, cancellationToken: cancellationToken); | ||
var root = doc.RootElement; | ||
|
||
JsonElement messageElement = root; | ||
bool isWrapped = false; | ||
|
||
if (root.ValueKind == JsonValueKind.Object && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure that |
||
root.TryGetProperty("isFunctionsMcpResult", out var marker) && | ||
marker.ValueKind == JsonValueKind.True && | ||
root.TryGetProperty("content", out var content)) | ||
{ | ||
messageElement = content; | ||
isWrapped = true; | ||
} | ||
|
||
if (unwrapOnly) | ||
{ | ||
if (isWrapped) | ||
{ | ||
var inner = messageElement.GetRawText(); | ||
var bytes = System.Text.Encoding.UTF8.GetBytes(inner); | ||
request.Body = new MemoryStream(bytes); | ||
request.ContentLength = bytes.Length; | ||
} | ||
else | ||
{ | ||
// Reset position so downstream readers can consume the original body. | ||
request.Body.Seek(0, SeekOrigin.Begin); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For extra safety, maybe check CanSeek before doing this |
||
} | ||
return null; | ||
} | ||
else | ||
{ | ||
var raw = messageElement.GetRawText(); | ||
return JsonSerializer.Deserialize<JsonRpcMessage>(raw, options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are we doing on json exceptions? And do we know that messageElement is json? |
||
} | ||
} | ||
finally | ||
{ | ||
if (!unwrapOnly) | ||
{ | ||
// Reset the request body so it can be read later by other components if necessary. | ||
request.Body.Seek(0, SeekOrigin.Begin); | ||
} | ||
} | ||
} | ||
|
||
internal static async ValueTask<JsonRpcMessage?> ExtractJsonRpcMessageSseAsync(HttpRequest request, JsonSerializerOptions options, CancellationToken cancellationToken) | ||
{ | ||
return await ProcessJsonRpcPayloadAsync(request, options, cancellationToken, unwrapOnly: false); | ||
} | ||
|
||
internal static async ValueTask ExtractJsonRpcMessageHttpStreamableAsync(HttpRequest request, CancellationToken cancellationToken) | ||
{ | ||
await ProcessJsonRpcPayloadAsync(request, JsonSerializerOptions.Default, cancellationToken, unwrapOnly: true); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,7 +105,7 @@ public async Task HandleMessageRequestAsync(HttpContext context, McpOptions mcpO | |
return; | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could implement this as a |
||
|
||
if (message is null) | ||
{ | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What "process" is happeing? Aren't we just reading? Wondering if there is a better name here