-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix 307 Temporary Redirect when use streamable_http #781
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
base: main
Are you sure you want to change the base?
Changes from 29 commits
0bf3521
dd00227
d89e737
7d6e14f
a84399b
900c2dd
1a638ad
b3bf4c9
645e1ab
3e9e33f
7ef5f8d
303cac8
0d05075
898fc88
45a5cd8
04f637c
2377b70
09e403c
a03c62e
10ccbbf
e102e7d
a72ffe8
6cf74f1
2748a75
b726c86
3b03b83
d2ee4df
355c882
86da08d
3d86e61
cc9f3d9
cce6d2f
04c4bcf
1d9632b
2761e8f
2aa0428
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 |
---|---|---|
|
@@ -618,7 +618,7 @@ async def run_streamable_http_async(self) -> None: | |
import uvicorn | ||
|
||
starlette_app = self.streamable_http_app() | ||
|
||
starlette_app.router.redirect_slashes = False | ||
config = uvicorn.Config( | ||
starlette_app, | ||
host=self.settings.host, | ||
|
@@ -770,7 +770,7 @@ async def sse_endpoint(request: Request) -> Response: | |
def streamable_http_app(self) -> Starlette: | ||
"""Return an instance of the StreamableHTTP server app.""" | ||
from starlette.middleware import Middleware | ||
from starlette.routing import Mount | ||
from starlette.routing import Route | ||
|
||
# Create session manager on first call (lazy initialization) | ||
if self._session_manager is None: | ||
|
@@ -782,16 +782,21 @@ def streamable_http_app(self) -> Starlette: | |
) | ||
|
||
# Create the ASGI handler | ||
async def handle_streamable_http( | ||
scope: Scope, receive: Receive, send: Send | ||
) -> None: | ||
await self.session_manager.handle_request(scope, receive, send) | ||
async def handle_streamable_http(request: Request) -> Response: | ||
await self.session_manager.handle_request( | ||
request.scope, request.receive, request._send | ||
) | ||
return Response() | ||
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 is not fine. Does 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. |
||
|
||
# Create routes | ||
routes: list[Route | Mount] = [] | ||
routes: list[Route] = [] | ||
middleware: list[Middleware] = [] | ||
required_scopes = [] | ||
|
||
# Always register both /mcp and /mcp/ for full compatibility | ||
_main_path = self.settings.streamable_http_path.removesuffix("/") | ||
_alt_path = _main_path + "/" | ||
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. I think the problem was the redirect, but if you have a 404 then it's not an issue anymore, and we shouldn't be needing this. |
||
|
||
# Add auth endpoints if auth provider is configured | ||
if self._auth_server_provider: | ||
assert self.settings.auth | ||
|
@@ -817,19 +822,39 @@ async def handle_streamable_http( | |
revocation_options=self.settings.auth.revocation_options, | ||
) | ||
) | ||
routes.append( | ||
Mount( | ||
self.settings.streamable_http_path, | ||
app=RequireAuthMiddleware(handle_streamable_http, required_scopes), | ||
) | ||
routes.extend( | ||
[ | ||
Route( | ||
_main_path, | ||
endpoint=RequireAuthMiddleware( | ||
handle_streamable_http, required_scopes | ||
), | ||
methods=["GET", "POST", "OPTIONS"], | ||
), | ||
Route( | ||
_alt_path, | ||
endpoint=RequireAuthMiddleware( | ||
handle_streamable_http, required_scopes | ||
), | ||
methods=["GET", "POST", "OPTIONS"], | ||
), | ||
] | ||
) | ||
else: | ||
# Auth is disabled, no wrapper needed | ||
routes.append( | ||
Mount( | ||
self.settings.streamable_http_path, | ||
app=handle_streamable_http, | ||
) | ||
routes.extend( | ||
[ | ||
Route( | ||
_main_path, | ||
endpoint=handle_streamable_http, | ||
methods=["GET", "POST", "OPTIONS"], | ||
), | ||
Route( | ||
_alt_path, | ||
endpoint=handle_streamable_http, | ||
methods=["GET", "POST", "OPTIONS"], | ||
), | ||
] | ||
) | ||
|
||
routes.extend(self._custom_starlette_routes) | ||
|
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.
Hmmm... I think we can add
redirect_slashes
in theStarlette
constructor on starlette's side.But this part is fine.