Skip to content

Fix #280 Routing pattern not included in /message endpoint when using SSE Server #323

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

Merged
merged 5 commits into from
Apr 20, 2025

Conversation

dogdie233
Copy link
Contributor

When create SseResponseStreamTransport, parse the path of the request to extract the pattern

Motivation and Context

Solve issue #280

How Has This Been Tested?

I also modified the testing method 'Allows_Customizing_Route' to check if the message endpoint returned by the sse request is correct

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Not sure if there are other better ways to get patterns

@stephentoub stephentoub requested a review from halter73 April 17, 2025 19:48
@halter73
Copy link
Contributor

halter73 commented Apr 20, 2025

Thanks for this PR. We merged "Streamable HTTP" support with #330, so now the relevant logic for this PR is in SseHandler.cs rather than StreamableHttpHandler.cs which no longer sends an "endpoint" event as that's no longer part of the latest transport spec. However, we still want to keep the legacy "HTTP with SSE" support and getting the root-relative endpoint URL correct.

I think in order to get this logic completely right, we'll need to take into account the PathBase like ASP.NET Core takes care of Content URLs here in SharedUrlHelper.cs. It'd also be nice to have a test verifying that mapping to an endpoint like "/mcp/secondary", works with our MCP client.

If you want to fix the merge conflict and address these issues in the next few days, I'd be happy to merge your PR. Otherwise, I can also take over the fix for #280 myself. Thanks again!

# Conflicts:
#	src/ModelContextProtocol.AspNetCore/StreamableHttpHandler.cs
@dogdie233
Copy link
Contributor Author

Now it properly considering PathBase.
And I added a test case for multi-segment URL prefixes in CanConnect_WithMcpClient_AfterCustomizingRoute

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@halter73 halter73 merged commit 0c9e91f into modelcontextprotocol:main Apr 20, 2025
7 checks passed
@halter73
Copy link
Contributor

halter73 commented Apr 20, 2025

I do wonder if this change is necessary now that MapMcp only routes SSE requests to $"{groupPrefix}/sse" and not just $"{groupPrefix)" since that's now used for the "Streamable HTTP" transport". After all, "messages" should be a valid relative reference.

@dogdie233 Was there a specific client that had an issue when just "messages" was used as the relative endpoint URL? I suppose making the messages URL root-relative doesn't hurt though.

@dogdie233
Copy link
Contributor Author

You're right! Emit root-relative paths completely resolves this issue and eliminates the need for URL pattern matching.

@dogdie233 dogdie233 deleted the fix-280 branch April 21, 2025 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants