From 74ead6f615fb9a6882899549cb284ec6cfc71f3e Mon Sep 17 00:00:00 2001 From: YuqiGuo105 Date: Mon, 18 May 2026 21:56:34 -0600 Subject: [PATCH] fix(mcp): honor custom endpoint in StreamableHttpServerParameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user passed a URL with a custom sub-path to StreamableHttpServerParameters (e.g. http://localhost:8080/mcp/stream), the MCP library's Utils.resolveUri() would call Java's URI.resolve() with the default endpoint '/mcp', which—per RFC 3986—replaces the entire path, producing http://localhost:8080/mcp instead of the intended URL. This caused every request to return 404 if the server was not exposed on /mcp. Fix: - Add an optional endpoint field to StreamableHttpServerParameters (and its Builder) that mirrors the pattern of SseServerParameters.sseEndpoint. - In DefaultMcpTransportBuilder, pass the base URL (host only) to the library builder and, when the user has set an endpoint, call HttpClientStreamableHttpTransport.Builder.endpoint() to set the path separately. This way resolveUri(baseUri, endpoint) works correctly. - When no endpoint is provided the library default ('/mcp') is preserved, keeping full backward compatibility. Fixes #1196 --- .../tools/mcp/DefaultMcpTransportBuilder.java | 22 +- .../mcp/StreamableHttpServerParameters.java | 19 +- .../mcp/DefaultMcpTransportBuilderTest.java | 200 ++++++++++++++++++ .../StreamableHttpServerParametersTest.java | 173 +++++++++++++++ 4 files changed, 404 insertions(+), 10 deletions(-) create mode 100644 core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java create mode 100644 core/src/test/java/com/google/adk/tools/mcp/StreamableHttpServerParametersTest.java diff --git a/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java b/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java index 84c882c4e..e64854a84 100644 --- a/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java +++ b/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java @@ -44,15 +44,19 @@ public McpClientTransport build(Object connectionParams) { .orElse("")))) .build(); } else if (connectionParams instanceof StreamableHttpServerParameters streamableParams) { - return HttpClientStreamableHttpTransport.builder(streamableParams.url()) - .connectTimeout(streamableParams.timeout()) - .jsonMapper(jsonMapper) - .asyncHttpRequestCustomizer( - (builder, method, uri, body, context) -> { - streamableParams.headers().forEach((key, value) -> builder.header(key, value)); - return Mono.just(builder); - }) - .build(); + HttpClientStreamableHttpTransport.Builder transportBuilder = + HttpClientStreamableHttpTransport.builder(streamableParams.url()) + .connectTimeout(streamableParams.timeout()) + .jsonMapper(jsonMapper) + .asyncHttpRequestCustomizer( + (builder, method, uri, body, context) -> { + streamableParams.headers().forEach((key, value) -> builder.header(key, value)); + return Mono.just(builder); + }); + if (streamableParams.endpoint() != null) { + transportBuilder.endpoint(streamableParams.endpoint()); + } + return transportBuilder.build(); } else { throw new IllegalArgumentException( "DefaultMcpTransportBuilder supports only ServerParameters, SseServerParameters, or" diff --git a/core/src/main/java/com/google/adk/tools/mcp/StreamableHttpServerParameters.java b/core/src/main/java/com/google/adk/tools/mcp/StreamableHttpServerParameters.java index e9f8a3ac8..7cedc819a 100644 --- a/core/src/main/java/com/google/adk/tools/mcp/StreamableHttpServerParameters.java +++ b/core/src/main/java/com/google/adk/tools/mcp/StreamableHttpServerParameters.java @@ -26,6 +26,7 @@ /** Server parameters for Streamable HTTP client transport. */ public class StreamableHttpServerParameters { private final String url; + private final String endpoint; private final Map headers; private final Duration timeout; private final Duration readTimeout; @@ -35,6 +36,8 @@ public class StreamableHttpServerParameters { * Server parameters for Streamable HTTP client transport. * * @param url The base URL for the MCP Streamable HTTP server. + * @param endpoint The endpoint path on the server (e.g. {@code /mcp/stream}). When {@code null}, + * the MCP library default ({@code /mcp}) is used. * @param headers Optional headers to include in requests. * @param timeout Timeout for HTTP operations (default: 30 seconds). * @param readTimeout Timeout for reading data from the streamed http events(default: 5 minutes). @@ -42,12 +45,14 @@ public class StreamableHttpServerParameters { */ public StreamableHttpServerParameters( String url, + @Nullable String endpoint, Map headers, @Nullable Duration timeout, @Nullable Duration readTimeout, @Nullable Boolean terminateOnClose) { Assert.hasText(url, "url must not be empty"); this.url = url; + this.endpoint = endpoint; this.headers = headers == null ? Collections.emptyMap() : headers; this.timeout = timeout == null ? Duration.ofSeconds(30) : timeout; this.readTimeout = readTimeout == null ? Duration.ofMinutes(5) : readTimeout; @@ -58,6 +63,11 @@ public String url() { return url; } + @Nullable + public String endpoint() { + return endpoint; + } + public Map headers() { return headers; } @@ -81,6 +91,7 @@ public static Builder builder() { /** Builder for {@link StreamableHttpServerParameters}. */ public static class Builder { private String url; + private String endpoint; private Map headers = Collections.emptyMap(); private Duration timeout = Duration.ofSeconds(30); private Duration readTimeout = Duration.ofMinutes(5); @@ -95,6 +106,12 @@ public Builder url(String url) { return this; } + @CanIgnoreReturnValue + public Builder endpoint(String endpoint) { + this.endpoint = endpoint; + return this; + } + @CanIgnoreReturnValue public Builder headers(Map headers) { this.headers = headers; @@ -121,7 +138,7 @@ public Builder terminateOnClose(boolean terminateOnClose) { public StreamableHttpServerParameters build() { return new StreamableHttpServerParameters( - url, headers, timeout, readTimeout, terminateOnClose); + url, endpoint, headers, timeout, readTimeout, terminateOnClose); } } } diff --git a/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java b/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java new file mode 100644 index 000000000..2eb30755b --- /dev/null +++ b/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java @@ -0,0 +1,200 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.tools.mcp; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import io.modelcontextprotocol.client.transport.HttpClientSseClientTransport; +import io.modelcontextprotocol.client.transport.HttpClientStreamableHttpTransport; +import io.modelcontextprotocol.client.transport.ServerParameters; +import io.modelcontextprotocol.client.transport.StdioClientTransport; +import io.modelcontextprotocol.spec.McpClientTransport; +import java.lang.reflect.Field; +import java.net.URI; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link DefaultMcpTransportBuilder}. */ +@RunWith(JUnit4.class) +public final class DefaultMcpTransportBuilderTest { + + private final DefaultMcpTransportBuilder builder = new DefaultMcpTransportBuilder(); + + // ------------------------------------------------------------------------- + // Helper: read private fields via reflection + // ------------------------------------------------------------------------- + + private static String getEndpointField(HttpClientStreamableHttpTransport transport) + throws Exception { + Field field = HttpClientStreamableHttpTransport.class.getDeclaredField("endpoint"); + field.setAccessible(true); + return (String) field.get(transport); + } + + private static URI getBaseUriField(HttpClientStreamableHttpTransport transport) throws Exception { + Field field = HttpClientStreamableHttpTransport.class.getDeclaredField("baseUri"); + field.setAccessible(true); + return (URI) field.get(transport); + } + + // ------------------------------------------------------------------------- + // StreamableHttp transport tests + // ------------------------------------------------------------------------- + + @Test + public void build_withStreamableHttpParamsWithoutEndpoint_usesLibraryDefaultEndpoint() + throws Exception { + // When the user does NOT set .endpoint(), the library default "/mcp" must be preserved. + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + // No .endpoint() call → endpoint() returns null + .build(); + + McpClientTransport transport = builder.build(params); + + assertThat(transport).isInstanceOf(HttpClientStreamableHttpTransport.class); + HttpClientStreamableHttpTransport streamableTransport = + (HttpClientStreamableHttpTransport) transport; + + // baseUri stores exactly the url passed by the user + assertThat(getBaseUriField(streamableTransport)) + .isEqualTo(URI.create("http://localhost:8080")); + // endpoint is the library's hard-coded default because the user did not override it + assertThat(getEndpointField(streamableTransport)).isEqualTo("/mcp"); + } + + @Test + public void build_withStreamableHttpParamsWithCustomEndpoint_setsEndpointOnTransport() + throws Exception { + // When the user sets .endpoint("/mcp/stream"), the transport's endpoint field must reflect it. + // This is the core of the bug fix: the library's Utils.resolveUri(baseUri, endpoint) will now + // compute URI.create("http://localhost:8080").resolve("/mcp/stream") + // = "http://localhost:8080/mcp/stream" ✅ + // instead of the broken pre-fix behaviour: + // URI.create("http://localhost:8080/mcp/stream").resolve("/mcp") + // = "http://localhost:8080/mcp" ❌ + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .endpoint("/mcp/stream") + .build(); + + McpClientTransport transport = builder.build(params); + + assertThat(transport).isInstanceOf(HttpClientStreamableHttpTransport.class); + HttpClientStreamableHttpTransport streamableTransport = + (HttpClientStreamableHttpTransport) transport; + + assertThat(getBaseUriField(streamableTransport)) + .isEqualTo(URI.create("http://localhost:8080")); + // endpoint was explicitly overridden — must NOT be the default "/mcp" + assertThat(getEndpointField(streamableTransport)).isEqualTo("/mcp/stream"); + } + + @Test + public void build_withStreamableHttpParams_defaultEndpointProducesCorrectFinalUri() + throws Exception { + // Confirm that the final resolved URI for default case is http://host/mcp (not broken). + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder().url("http://localhost:8080").build(); + + McpClientTransport transport = builder.build(params); + HttpClientStreamableHttpTransport streamableTransport = + (HttpClientStreamableHttpTransport) transport; + + URI base = getBaseUriField(streamableTransport); + String endpoint = getEndpointField(streamableTransport); + URI resolved = base.resolve(endpoint); + + assertThat(resolved).isEqualTo(URI.create("http://localhost:8080/mcp")); + } + + @Test + public void build_withStreamableHttpParams_customEndpointProducesCorrectFinalUri() + throws Exception { + // Confirm that the final resolved URI for the custom endpoint case is correct. + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .endpoint("/mcp/stream") + .build(); + + McpClientTransport transport = builder.build(params); + HttpClientStreamableHttpTransport streamableTransport = + (HttpClientStreamableHttpTransport) transport; + + URI base = getBaseUriField(streamableTransport); + String endpoint = getEndpointField(streamableTransport); + URI resolved = base.resolve(endpoint); + + assertThat(resolved).isEqualTo(URI.create("http://localhost:8080/mcp/stream")); + } + + // ------------------------------------------------------------------------- + // SSE transport tests — ensure existing behaviour is unchanged + // ------------------------------------------------------------------------- + + @Test + public void build_withSseParams_returnsSseTransport() { + SseServerParameters params = + SseServerParameters.builder().url("http://localhost:8080").build(); + + McpClientTransport transport = builder.build(params); + + assertThat(transport).isInstanceOf(HttpClientSseClientTransport.class); + } + + @Test + public void build_withSseParamsWithCustomSseEndpoint_returnsSseTransport() { + SseServerParameters params = + SseServerParameters.builder() + .url("http://localhost:8080") + .sseEndpoint("events") + .build(); + + McpClientTransport transport = builder.build(params); + + assertThat(transport).isInstanceOf(HttpClientSseClientTransport.class); + } + + // ------------------------------------------------------------------------- + // Stdio transport tests — ensure existing behaviour is unchanged + // ------------------------------------------------------------------------- + + @Test + public void build_withStdioParams_returnsStdioTransport() { + ServerParameters params = ServerParameters.builder("echo").args("hello").build(); + + McpClientTransport transport = builder.build(params); + + assertThat(transport).isInstanceOf(StdioClientTransport.class); + } + + // ------------------------------------------------------------------------- + // Unknown param type test + // ------------------------------------------------------------------------- + + @Test + public void build_withUnknownParamType_throwsIllegalArgumentException() { + Object unknownParams = new Object(); + + assertThrows(IllegalArgumentException.class, () -> builder.build(unknownParams)); + } +} diff --git a/core/src/test/java/com/google/adk/tools/mcp/StreamableHttpServerParametersTest.java b/core/src/test/java/com/google/adk/tools/mcp/StreamableHttpServerParametersTest.java new file mode 100644 index 000000000..74c41e9d1 --- /dev/null +++ b/core/src/test/java/com/google/adk/tools/mcp/StreamableHttpServerParametersTest.java @@ -0,0 +1,173 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.tools.mcp; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import java.time.Duration; +import java.util.Map; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link StreamableHttpServerParameters}. */ +@RunWith(JUnit4.class) +public final class StreamableHttpServerParametersTest { + + // ------------------------------------------------------------------------- + // Constructor tests + // ------------------------------------------------------------------------- + + @Test + public void constructor_withUrlOnly_createsParameters() { + StreamableHttpServerParameters params = + new StreamableHttpServerParameters( + "http://localhost:8080", null, null, null, null, null); + + assertThat(params.url()).isEqualTo("http://localhost:8080"); + assertThat(params.endpoint()).isNull(); + assertThat(params.headers()).isEmpty(); + assertThat(params.timeout()).isEqualTo(Duration.ofSeconds(30)); + assertThat(params.readTimeout()).isEqualTo(Duration.ofMinutes(5)); + assertThat(params.terminateOnClose()).isTrue(); + } + + @Test + public void constructor_withUrlAndEndpoint_createsParameters() { + StreamableHttpServerParameters params = + new StreamableHttpServerParameters( + "http://localhost:8080", "/mcp/stream", null, null, null, null); + + assertThat(params.url()).isEqualTo("http://localhost:8080"); + // endpoint() returns exactly what was passed — the transport builder will use it + assertThat(params.endpoint()).isEqualTo("/mcp/stream"); + } + + @Test + public void constructor_withEmptyUrl_throwsIllegalArgumentException() { + // Assert.hasText() in the constructor rejects blank/empty strings + assertThrows( + IllegalArgumentException.class, + () -> + new StreamableHttpServerParameters("", null, null, null, null, null)); + } + + @Test + public void constructor_withNullUrl_throwsIllegalArgumentException() { + assertThrows( + IllegalArgumentException.class, + () -> + new StreamableHttpServerParameters(null, null, null, null, null, null)); + } + + @Test + public void constructor_withNullTimeouts_usesDefaults() { + StreamableHttpServerParameters params = + new StreamableHttpServerParameters( + "http://localhost:8080", null, null, /*timeout=*/ null, /*readTimeout=*/ null, null); + + assertThat(params.timeout()).isEqualTo(Duration.ofSeconds(30)); + assertThat(params.readTimeout()).isEqualTo(Duration.ofMinutes(5)); + } + + @Test + public void constructor_withNullTerminateOnClose_defaultsToTrue() { + StreamableHttpServerParameters params = + new StreamableHttpServerParameters( + "http://localhost:8080", null, null, null, null, /*terminateOnClose=*/ null); + + assertThat(params.terminateOnClose()).isTrue(); + } + + // ------------------------------------------------------------------------- + // Builder tests + // ------------------------------------------------------------------------- + + @Test + public void builder_withUrlOnly_buildsParameters() { + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .build(); + + assertThat(params.url()).isEqualTo("http://localhost:8080"); + // No endpoint set → null, so DefaultMcpTransportBuilder will use library default "/mcp" + assertThat(params.endpoint()).isNull(); + } + + @Test + public void builder_withUrlAndEndpoint_buildsParameters() { + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .endpoint("/mcp/stream") + .build(); + + assertThat(params.url()).isEqualTo("http://localhost:8080"); + assertThat(params.endpoint()).isEqualTo("/mcp/stream"); + } + + @Test + public void builder_withDefaults_usesDefaultTimeouts() { + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .build(); + + assertThat(params.timeout()).isEqualTo(Duration.ofSeconds(30)); + assertThat(params.readTimeout()).isEqualTo(Duration.ofMinutes(5)); + assertThat(params.terminateOnClose()).isTrue(); + } + + @Test + public void builder_withCustomTimeouts_buildsParameters() { + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .timeout(Duration.ofSeconds(10)) + .readTimeout(Duration.ofMinutes(2)) + .build(); + + assertThat(params.timeout()).isEqualTo(Duration.ofSeconds(10)); + assertThat(params.readTimeout()).isEqualTo(Duration.ofMinutes(2)); + } + + @Test + public void builder_withHeaders_buildsParameters() { + Map headers = Map.of("Authorization", "Bearer token123"); + + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .headers(headers) + .build(); + + assertThat(params.headers()).containsEntry("Authorization", "Bearer token123"); + } + + @Test + public void builder_withTerminateOnCloseFalse_buildsParameters() { + StreamableHttpServerParameters params = + StreamableHttpServerParameters.builder() + .url("http://localhost:8080") + .terminateOnClose(false) + .build(); + + assertThat(params.terminateOnClose()).isFalse(); + } +}