Skip to content

Conversation

@Kehrlann
Copy link
Contributor

@Kehrlann Kehrlann commented Nov 19, 2025

Currently, the WebFlux SSE Server transport and WebMVC SSE Server transport do not align in behavior.
The WebMVC transport gets an ID generated an injected, instead of relying on the SessionFactory's generation mechanism.

In this PR, we put the session ID generation back into the factory. This means the "transport session" knows nothing about the session ID, so we remove the id from the SSE events and all the logging data.

@Kehrlann Kehrlann force-pushed the dgarnier/webmvc-sse-server-transport-session-id branch from 27b74a2 to 15d539f Compare November 19, 2025 13:24
@Kehrlann Kehrlann marked this pull request as ready for review November 19, 2025 13:24
@Kehrlann Kehrlann requested a review from chemicL November 19, 2025 13:30
- This aligns messages with WebFluxSseServerTransportProvider

Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
@Kehrlann Kehrlann force-pushed the dgarnier/webmvc-sse-server-transport-session-id branch from 15d539f to 2c40670 Compare November 19, 2025 14:11
this.sessions.put(sessionId, session);
return ServerResponse.sse(sseBuilder -> {
WebMvcMcpSessionTransport sessionTransport = new WebMvcMcpSessionTransport(sseBuilder);
McpServerSession session = sessionFactory.create(sessionTransport);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to have this.

Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

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

This is a useful improvement to the mess that existed there, thank you :)

@He-Pin
Copy link
Contributor

He-Pin commented Nov 20, 2025

@Kehrlann @chemicL is it possible we extract servlet and mvc support to dedicated module?

@Kehrlann Kehrlann merged commit c561675 into main Nov 20, 2025
1 check passed
@Kehrlann Kehrlann deleted the dgarnier/webmvc-sse-server-transport-session-id branch November 20, 2025 09:03
@Kehrlann
Copy link
Contributor Author

@He-Pin I'm not sure I understand the ask. MVC is already in a separate module.

@He-Pin
Copy link
Contributor

He-Pin commented Nov 20, 2025

@Kehrlann oops, wrong memory, what about servlet one

@Kehrlann
Copy link
Contributor Author

The idea is to have the basics, SDK+Jakarta base in the core. And you'll notice that jakarta-servlet is a provided dep, so we're not even bringing it in.

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.

4 participants