Preserve query in endpoint URL sent by SSE transport #156
Closed
jalingo wants to merge 5 commits intomodelcontextprotocol:mattt/sse-transportfrom
Closed
Preserve query in endpoint URL sent by SSE transport #156jalingo wants to merge 5 commits intomodelcontextprotocol:mattt/sse-transportfrom
jalingo wants to merge 5 commits intomodelcontextprotocol:mattt/sse-transportfrom
Conversation
mattt
reviewed
Aug 29, 2025
| endpoint: URL(string: "http://localhost:8080/sse")! // Ensure endpoint is SSE-specific if needed | ||
| ) | ||
| try await client.connect(transport: sseTransport) | ||
| try await client.initialize() |
Contributor
There was a problem hiding this comment.
This is no longer required, as of #100
Suggested change
| try await client.initialize() |
mattt
reviewed
Aug 29, 2025
mattt
reviewed
Aug 29, 2025
Comment on lines
+421
to
+430
| if let pathComponents = URLComponents(string: pathToUse) { | ||
| components.path = pathComponents.path | ||
| // Preserve any query parameters from the endpoint path | ||
| if let queryItems = pathComponents.queryItems { | ||
| components.queryItems = queryItems | ||
| } | ||
| } else { | ||
| components.path = pathToUse | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Slightly more comprehensive version:
Suggested change
| if let pathComponents = URLComponents(string: pathToUse) { | |
| components.path = pathComponents.path | |
| // Preserve any query parameters from the endpoint path | |
| if let queryItems = pathComponents.queryItems { | |
| components.queryItems = queryItems | |
| } | |
| } else { | |
| components.path = pathToUse | |
| } | |
| // For relative paths, preserve the scheme, host, and port. | |
| let pathToUse = path.starts(with: "/") ? path : "/\(path)" | |
| if let relativeComponents = URLComponents(string: pathToUse) { | |
| // Set path (prefer percentEncodedPath if present to avoid double-encoding) | |
| if let percentEncodedPath = relativeComponents.percentEncodedPath.isEmpty | |
| ? nil : relativeComponents.percentEncodedPath | |
| { | |
| components.percentEncodedPath = percentEncodedPath | |
| } else { | |
| components.path = relativeComponents.path | |
| } | |
| // Set query via queryItems to ensure correct percent-encoding | |
| if let queryItems = relativeComponents.queryItems, !queryItems.isEmpty { | |
| components.queryItems = queryItems | |
| } else { | |
| components.queryItems = nil | |
| } | |
| // Preserve fragment if provided | |
| components.fragment = relativeComponents.fragment | |
| } else { | |
| // Fallback: set path directly if components parsing fails | |
| components.path = pathToUse | |
| } | |
Contributor
|
Hi @jalingo. Thanks for your contribution. Making sure I correctly understand the motivation behind this PR: You're using query parameters (e.g. |
Contributor
|
We should give this new behavior some test coverage. Here are some tests that pass with the code I suggested above: @Test("Query parameter encoding in relative endpoint URL", .sseTransportSetup)
func testQueryEncodingInRelativeEndpoint() async throws {
let configuration = URLSessionConfiguration.ephemeral
configuration.protocolClasses = [MockSSEURLProtocol.self]
let transport = SSEClientTransport(
endpoint: testEndpoint,
configuration: configuration
)
let rawEndpoint =
"/messages/with space?q=hello world&sym=%&plus=+&slash=a/b&emoji=😀#frag"
let capturedRequest = CapturedRequest()
await MockSSEURLProtocol.setHandler { request in
if request.httpMethod == "GET" {
let response = HTTPURLResponse(
url: testEndpoint,
statusCode: 200,
httpVersion: "HTTP/1.1",
headerFields: ["Content-Type": "text/event-stream"]
)!
return (
response,
Data(
"""
event: endpoint
data: \(rawEndpoint)
""".utf8)
)
} else if request.httpMethod == "POST" {
await capturedRequest.setValue(request)
let response = HTTPURLResponse(
url: request.url!,
statusCode: 200,
httpVersion: "HTTP/1.1",
headerFields: ["Content-Type": "application/json"]
)!
return (response, Data())
} else {
throw MCPError.internalError("Unexpected request method")
}
}
try await transport.connect()
try await transport.send(Data())
let postReq = await capturedRequest.getValue()
#expect(postReq != nil)
guard let url = postReq?.url,
let comps = URLComponents(url: url, resolvingAgainstBaseURL: false)
else {
Issue.record("Invalid URL in captured POST request")
return
}
#expect(comps.scheme == testEndpoint.scheme)
#expect(comps.host == testEndpoint.host)
#expect(comps.path == "/messages/with space")
let expectedItems = [
URLQueryItem(name: "q", value: "hello world"),
URLQueryItem(name: "sym", value: "%"),
URLQueryItem(name: "plus", value: "+"),
URLQueryItem(name: "slash", value: "a/b"),
URLQueryItem(name: "emoji", value: "😀"),
]
#expect(comps.queryItems?.count == expectedItems.count)
if let items = comps.queryItems {
for (idx, item) in items.enumerated() {
#expect(item.name == expectedItems[idx].name)
#expect(item.value == expectedItems[idx].value)
}
}
#expect(comps.fragment == "frag")
await transport.disconnect()
}
@Test("No double-encoding for already percent-encoded inputs", .sseTransportSetup)
func testNoDoubleEncoding() async throws {
let configuration = URLSessionConfiguration.ephemeral
configuration.protocolClasses = [MockSSEURLProtocol.self]
let transport = SSEClientTransport(
endpoint: testEndpoint,
configuration: configuration
)
let rawEndpoint = "/messages/%E2%9C%93?q=a%2Bb"
let capturedRequest = CapturedRequest()
await MockSSEURLProtocol.setHandler { request in
if request.httpMethod == "GET" {
let response = HTTPURLResponse(
url: testEndpoint,
statusCode: 200,
httpVersion: "HTTP/1.1",
headerFields: ["Content-Type": "text/event-stream"]
)!
return (
response,
Data(
"""
event: endpoint
data: \(rawEndpoint)
""".utf8)
)
} else if request.httpMethod == "POST" {
await capturedRequest.setValue(request)
let response = HTTPURLResponse(
url: request.url!,
statusCode: 200,
httpVersion: "HTTP/1.1",
headerFields: ["Content-Type": "application/json"]
)!
return (response, Data())
} else {
throw MCPError.internalError("Unexpected request method")
}
}
try await transport.connect()
try await transport.send(Data())
guard let url = await capturedRequest.getValue()?.url,
let comps = URLComponents(url: url, resolvingAgainstBaseURL: false)
else {
Issue.record("Invalid URL in captured POST request")
return
}
#expect(comps.path == "/messages/✓")
#expect(comps.queryItems?.first(where: { $0.name == "q" })?.value == "a+b")
await transport.disconnect()
}
} |
Author
|
Actually, just found out I'm not allowed to contribute to open source anymore; but when the endpoint was delivered for all future posts it was getting malformed. Also not my experience with the init, but maybe that was added to the branch recently. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
URL was formatting incorrectly, preventing endpoint from being recognized after sse connection.
Motivation and Context
Wanted SSE connections to mcp services to work.
How Has This Been Tested?
It works with multiple sse mcp services now.
Breaking Changes
Don't think so.
Types of changes
Checklist
Additional context
Pretty straightforward