Skip to content

Commit 014efd8

Browse files
KehrlannKehrlann
committed
subscription/listen without params or notifications returns error
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
1 parent 97edf22 commit 014efd8

4 files changed

Lines changed: 131 additions & 3 deletions

File tree

‎mcp-core/src/main/java/io/modelcontextprotocol/modern/server/SubscriptionsFeature.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,22 @@ public Set<String> methods() {
7171

7272
@Override
7373
public Mono<McpAsyncResponse<Result>> handle(McpRequestContext ctx, Object params) {
74+
// The spec requires `notifications` to be a SubscriptionFilter object. Checked on
75+
// the raw params because SubscriptionsListenRequest deserializes a missing or
76+
// null value to an empty filter, which would be acknowledged as a valid
77+
// subscription. Requiring an object (not just non-null) also stops a lenient
78+
// mapper from coercing "" or [] to null, and therefore to that same empty filter.
79+
if (!(params instanceof Map<?, ?> map) || !(map.get("notifications") instanceof Map)) {
80+
return Mono.error(McpException.invalidParams("params.notifications is required"));
81+
}
7482
SubscriptionsListenRequest request;
7583
try {
76-
request = params == null ? null : this.jsonMapper.convertValue(params, SubscriptionsListenRequest.class);
84+
request = this.jsonMapper.convertValue(params, SubscriptionsListenRequest.class);
7785
}
7886
catch (RuntimeException ex) {
7987
return Mono.error(McpException.invalidParams("Malformed SubscriptionsListenRequest"));
8088
}
81-
SubscriptionFilter requested = request == null || request.notifications() == null ? SubscriptionFilter.EMPTY
82-
: request.notifications();
89+
SubscriptionFilter requested = request.notifications();
8390
return Mono.just(McpAsyncResponse.streaming(notifier -> listen(ctx, intersect(requested), notifier)));
8491
}
8592

‎mcp-test/src/test/java/io/modelcontextprotocol/modern/server/SubscriptionsFeatureTests.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,16 @@
77
import java.util.HashMap;
88
import java.util.List;
99
import java.util.Map;
10+
import java.util.stream.Stream;
1011

1112
import io.modelcontextprotocol.common.McpTransportContext;
1213
import io.modelcontextprotocol.json.McpJsonDefaults;
1314
import io.modelcontextprotocol.modern.JsonRpc.JSONRPCNotification;
1415
import io.modelcontextprotocol.modern.JsonRpc.JSONRPCRequest;
1516
import io.modelcontextprotocol.modern.JsonRpc.JSONRPCResponse;
17+
import io.modelcontextprotocol.modern.JsonRpc.JSONRPCResponse.JSONRPCError;
1618
import io.modelcontextprotocol.modern.McpSchema.CacheScope;
19+
import io.modelcontextprotocol.modern.McpSchema.ErrorCodes;
1720
import io.modelcontextprotocol.modern.McpSchema.MetaKeys;
1821
import io.modelcontextprotocol.modern.McpSchema.SubscriptionFilter;
1922
import io.modelcontextprotocol.modern.McpSchema.SubscriptionsAcknowledgedParams;
@@ -27,6 +30,8 @@
2730
import io.modelcontextprotocol.modern.server.feature.ServerChange;
2831
import io.modelcontextprotocol.modern.server.feature.ToolsFeature;
2932
import org.junit.jupiter.api.Test;
33+
import org.junit.jupiter.params.ParameterizedTest;
34+
import org.junit.jupiter.params.provider.MethodSource;
3035
import reactor.core.publisher.Mono;
3136
import reactor.test.StepVerifier;
3237

@@ -35,7 +40,10 @@
3540
import static io.modelcontextprotocol.modern.server.ModernTestFixtures.emptyTools;
3641
import static io.modelcontextprotocol.modern.server.ModernTestFixtures.meta;
3742
import static io.modelcontextprotocol.modern.server.ModernTestFixtures.respond;
43+
import static org.assertj.core.api.Assertions.as;
3844
import static org.assertj.core.api.Assertions.assertThat;
45+
import static org.assertj.core.api.InstanceOfAssertFactories.STRING;
46+
import static org.assertj.core.api.InstanceOfAssertFactories.type;
3947

4048
class SubscriptionsFeatureTests {
4149

@@ -171,6 +179,77 @@ void listenAfterEarlierListenEndedReceivesChanges() {
171179
.verifyComplete();
172180
}
173181

182+
@Test
183+
void listenWithoutNotificationsIsRejected() {
184+
McpServer server = toolsServer(new McpChangeFeed());
185+
186+
Map<String, Object> params = new HashMap<>();
187+
params.put("_meta", meta());
188+
McpTransportResponse response = server
189+
.handle(McpTransportContext.EMPTY, new JSONRPCRequest("subscriptions/listen", 1, params))
190+
.block();
191+
192+
assertNotificationsRequired(response);
193+
}
194+
195+
@Test
196+
void listenWithNullNotificationsIsRejected() {
197+
McpServer server = toolsServer(new McpChangeFeed());
198+
199+
Map<String, Object> params = new HashMap<>();
200+
params.put("_meta", meta());
201+
params.put("notifications", null);
202+
McpTransportResponse response = server
203+
.handle(McpTransportContext.EMPTY, new JSONRPCRequest("subscriptions/listen", 1, params))
204+
.block();
205+
206+
assertNotificationsRequired(response);
207+
}
208+
209+
@ParameterizedTest
210+
@MethodSource("nonObjectNotifications")
211+
void listenWithNonObjectNotificationsIsRejected(Object notifications) {
212+
McpServer server = toolsServer(new McpChangeFeed());
213+
214+
Map<String, Object> params = new HashMap<>();
215+
params.put("_meta", meta());
216+
params.put("notifications", notifications);
217+
McpTransportResponse response = server
218+
.handle(McpTransportContext.EMPTY, new JSONRPCRequest("subscriptions/listen", 1, params))
219+
.block();
220+
221+
assertNotificationsRequired(response);
222+
}
223+
224+
static Stream<Object> nonObjectNotifications() {
225+
// "" and [] matter in particular: a mapper configured with
226+
// ACCEPT_EMPTY_STRING_AS_NULL_OBJECT / ACCEPT_EMPTY_ARRAY_AS_NULL_OBJECT would
227+
// coerce them to null, which SubscriptionsListenRequest turns into an empty
228+
// filter.
229+
return Stream.of("", List.of(), true, 1);
230+
}
231+
232+
@Test
233+
void emptyNotificationsIsAcknowledgedAndStaysOpen() {
234+
McpChangeFeed feed = new McpChangeFeed();
235+
McpServer server = toolsServer(feed);
236+
237+
Map<String, Object> params = new HashMap<>();
238+
params.put("_meta", meta());
239+
params.put("notifications", Map.of());
240+
var invocation = (McpTransportResponse.Streaming) server
241+
.handle(McpTransportContext.EMPTY, new JSONRPCRequest("subscriptions/listen", 1, params))
242+
.block();
243+
244+
StepVerifier.create(invocation.messages())
245+
.assertNext(msg -> assertThat(acknowledgedNotifications((JSONRPCNotification) msg))
246+
.isEqualTo(SubscriptionFilter.EMPTY))
247+
.then(() -> feed.broadcast(new ServerChange.ToolsListChanged()))
248+
.then(server::closeGracefully)
249+
.expectNextMatches(msg -> msg instanceof JSONRPCResponse)
250+
.verifyComplete();
251+
}
252+
174253
@Test
175254
void resourceSubscribeIsAdvertisedAndHonouredWhenRepositorySupportsIt() {
176255
McpServer server = resourcesServer(true, new McpChangeFeed());
@@ -268,6 +347,15 @@ private static McpTransportResponse.Streaming listen(McpServer server, int id) {
268347
return (McpTransportResponse.Streaming) server.handle(McpTransportContext.EMPTY, request).block();
269348
}
270349

350+
private static void assertNotificationsRequired(McpTransportResponse response) {
351+
// An error response, not a stream: no acknowledgement is ever sent.
352+
assertThat(response).asInstanceOf(type(McpTransportResponse.Error.class))
353+
.extracting(error -> error.response().error())
354+
.returns(ErrorCodes.INVALID_PARAMS, JSONRPCError::code)
355+
.extracting(JSONRPCError::message, as(STRING))
356+
.contains("notifications", "required");
357+
}
358+
271359
private static Object subscriptionIdOf(JSONRPCNotification notification) {
272360
return ((SubscriptionsAcknowledgedParams) notification.params()).meta().get(MetaKeys.SUBSCRIPTION_ID);
273361
}

‎mcp-test/src/test/java/io/modelcontextprotocol/modern/server/transport/HttpServletMcpTransportIntegrationTests.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,20 @@ void errorIsAnsweredAsJsonWithTheStatusForItsCode() throws Exception {
296296
assertThat(errorCode(response)).isEqualTo(ErrorCodes.INVALID_PARAMS);
297297
}
298298

299+
@Test
300+
void listenWithoutNotificationsIsAnsweredAsJsonError() throws Exception {
301+
HttpRequest request = post("subscriptions/listen", Map.of("_meta", meta())).build();
302+
// Bounded: an accepted listen would open an event stream that never ends.
303+
HttpResponse<String> response = HttpClient.newHttpClient()
304+
.sendAsync(request, HttpResponse.BodyHandlers.ofString())
305+
.get(5, TimeUnit.SECONDS);
306+
307+
assertThat(response.statusCode()).isEqualTo(400);
308+
assertThat(response.headers().firstValue("Content-Type"))
309+
.hasValueSatisfying(v -> assertThat(v).contains("application/json"));
310+
assertThat(errorCode(response)).isEqualTo(ErrorCodes.INVALID_PARAMS);
311+
}
312+
299313
@Test
300314
void customErrorCodeUsesConfiguredStatus() throws Exception {
301315
HttpResponse<String> response = callTool("quota");

‎mcp-test/src/test/java/io/modelcontextprotocol/modern/server/transport/StdioMcpTransportTests.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import io.modelcontextprotocol.modern.server.McpSyncResponse;
5151
import io.modelcontextprotocol.modern.server.McpTransportResponse;
5252
import io.modelcontextprotocol.modern.server.feature.McpAsyncTool;
53+
import io.modelcontextprotocol.modern.server.feature.McpChangeFeed;
5354
import io.modelcontextprotocol.modern.server.feature.McpChangePublisher;
5455
import io.modelcontextprotocol.modern.server.feature.McpSyncTool;
5556
import io.modelcontextprotocol.modern.server.feature.McpSyncToolRepository;
@@ -623,6 +624,24 @@ void closeGracefullyCancelsListenStreams() throws Exception {
623624
await().atMost(Duration.ofSeconds(1)).untilTrue(publisherCancelled);
624625
}
625626

627+
@Test
628+
void listenWithoutNotificationsIsAnsweredWithAnError() throws Exception {
629+
McpServer server = McpServer.builder()
630+
.serverInfo(SERVER_INFO)
631+
.jsonMapper(this.jsonMapper)
632+
.feature(ToolsFeature.ofAsync(emptyTools(), this.jsonMapper, PERMISSIVE_VALIDATOR, 0L, CacheScope.PRIVATE))
633+
.subscriptions(new McpChangeFeed())
634+
.build();
635+
start(server);
636+
637+
send("subscriptions/listen", 3, Map.of("_meta", meta()));
638+
639+
// The error is the first line: no acknowledgement precedes it.
640+
String line = readLineWithTimeout();
641+
assertThatJson(line).inPath("$.id").isEqualTo(3);
642+
assertThatJson(line).inPath("$.error.code").isEqualTo(ErrorCodes.INVALID_PARAMS);
643+
}
644+
626645
@Test
627646
void closeGracefullyCompletesTheStartMono() throws Exception {
628647
McpRequestManager manager = managerOf((transportContext, request) -> Mono

0 commit comments

Comments
 (0)