From 9e92bc9ce0d630071f7e882337428500fd8e62dd Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 13 Mar 2025 12:45:30 -0400 Subject: [PATCH] Remove InputOutputStreams and clean up StdioServerTransport ctors (#71) - There doesn't appear to be a meaningful scenario for injecting different readers/writers than stdin/stdout, and having InputOutputStreams complicates the API. - Logically folks talk about "input and output", "readers and writers", etc., and "stdio" is "standard input and output", so having input specified first is less likely to be a source of bugs. - Logger should be optional, not required, to simplify the common and getting-started cases. --- .../McpServerServiceCollectionExtension.cs | 1 - .../Protocol/Transport/InputOutputStreams.cs | 26 ----- .../Transport/StdioServerTransport.cs | 110 ++++++++++++++---- .../Transport/StdioServerTransportTests.cs | 38 +++--- 4 files changed, 109 insertions(+), 66 deletions(-) delete mode 100644 src/mcpdotnet/Protocol/Transport/InputOutputStreams.cs diff --git a/src/mcpdotnet/Configuration/McpServerServiceCollectionExtension.cs b/src/mcpdotnet/Configuration/McpServerServiceCollectionExtension.cs index c0aa8fc..7f9e8a5 100644 --- a/src/mcpdotnet/Configuration/McpServerServiceCollectionExtension.cs +++ b/src/mcpdotnet/Configuration/McpServerServiceCollectionExtension.cs @@ -39,7 +39,6 @@ public static class McpServerServiceCollectionExtension services.AddSingleton(); services.AddHostedService(); services.AddOptions(); - services.AddSingleton(InputOutputStreams.Default); services.AddSingleton(sp => sp.GetRequiredService().CreateServer()); diff --git a/src/mcpdotnet/Protocol/Transport/InputOutputStreams.cs b/src/mcpdotnet/Protocol/Transport/InputOutputStreams.cs deleted file mode 100644 index deccafe..0000000 --- a/src/mcpdotnet/Protocol/Transport/InputOutputStreams.cs +++ /dev/null @@ -1,26 +0,0 @@ -namespace McpDotNet.Protocol.Transport; - -/// -/// Container for the input and output streams. -/// -public class InputOutputStreams -{ - /// - /// Gets the default input and output streams. - /// - public static readonly InputOutputStreams Default = new() - { - Output = Console.Out, - Input = Console.In - }; - - /// - /// Gets or sets the output stream. - /// - public required TextWriter Output { get; init; } - - /// - /// Gets or sets the input stream. - /// - public required TextReader Input { get; init; } -} diff --git a/src/mcpdotnet/Protocol/Transport/StdioServerTransport.cs b/src/mcpdotnet/Protocol/Transport/StdioServerTransport.cs index d202eae..cc3a09f 100644 --- a/src/mcpdotnet/Protocol/Transport/StdioServerTransport.cs +++ b/src/mcpdotnet/Protocol/Transport/StdioServerTransport.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using McpDotNet.Configuration; using McpDotNet.Logging; using McpDotNet.Protocol.Messages; using McpDotNet.Server; @@ -6,10 +7,12 @@ using McpDotNet.Utils.Json; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +#pragma warning disable CA2208 // Instantiate argument exceptions correctly + namespace McpDotNet.Protocol.Transport; /// -/// Implements the MCP transport protocol over standard input/output streams. +/// Provides an implementation of the MCP transport protocol over standard input/output streams. /// public sealed class StdioServerTransport : TransportBase, IServerTransport { @@ -24,47 +27,90 @@ public sealed class StdioServerTransport : TransportBase, IServerTransport private string EndpointName => $"Server (stdio) ({_serverName})"; /// - /// Initializes a new instance of the StdioServerTransport class. + /// Initializes a new instance of the class, using + /// and for input and output streams. /// /// The server options. - /// A logger factory for creating loggers. - /// Container for the input and output streams - public StdioServerTransport(McpServerOptions serverOptions, ILoggerFactory? loggerFactory, InputOutputStreams inputOutputStreams) - : this( - serverOptions is not null ? serverOptions.ServerInfo.Name : throw new ArgumentNullException(nameof(serverOptions)), - loggerFactory, - inputOutputStreams is not null ? inputOutputStreams.Output : throw new ArgumentNullException(nameof(inputOutputStreams)), - inputOutputStreams.Input - ) + /// Optional logger factory used for logging employed by the transport. + /// is or contains a null name. + /// + /// + /// By default, no logging is performed. If a is supplied, it must not log + /// to , as that will interfere with the transport's output. + /// + /// + public StdioServerTransport(McpServerOptions serverOptions, ILoggerFactory? loggerFactory = null) + : this(GetServerName(serverOptions), loggerFactory) { } /// - /// Initializes a new instance of the StdioServerTransport class. + /// Initializes a new instance of the class, using + /// and for input and output streams. /// /// The name of the server. - /// A logger factory for creating loggers. - public StdioServerTransport(string serverName, ILoggerFactory? loggerFactory) - : this(serverName, loggerFactory, Console.Out, Console.In) + /// Optional logger factory used for logging employed by the transport. + /// is . + /// + /// + /// By default, no logging is performed. If a is supplied, it must not log + /// to , as that will interfere with the transport's output. + /// + /// + public StdioServerTransport(string serverName, ILoggerFactory? loggerFactory = null) + : this(serverName ?? throw new ArgumentNullException(nameof(serverName)), Console.In, Console.Out, loggerFactory) { - } /// - /// Initializes a new instance of the StdioServerTransport class. + /// Initializes a new instance of the class using the specified + /// and streams. + /// + /// The server options. + /// The reader, from which all input is read. + /// The writer, to which all output is written. + /// Optional logger factory used for logging employed by the transport. + /// is or contains a null name. + /// is . + /// is . + /// + /// + /// By default, no logging is performed. If a is supplied, it must not log + /// to , as that will interfere with the transport's output. + /// + /// + public StdioServerTransport(McpServerOptions serverOptions, TextReader input, TextWriter output, ILoggerFactory? loggerFactory = null) + : this(GetServerName(serverOptions), input, output, loggerFactory) + { + } + + /// + /// Initializes a new instance of the class using the specified + /// and streams. /// /// The name of the server. - /// A logger factory for creating loggers. - /// The output stream writer - /// The input stream reader - public StdioServerTransport(string serverName, ILoggerFactory? loggerFactory, TextWriter output, TextReader input) + /// The reader, from which all input is read. + /// The writer, to which all output is written. + /// Optional logger factory used for logging employed by the transport. + /// is . + /// is . + /// is . + /// + /// + /// By default, no logging is performed. If a is supplied, it must not log + /// to , as that will interfere with the transport's output. + /// + /// + public StdioServerTransport(string serverName, TextReader input, TextWriter output, ILoggerFactory? loggerFactory = null) : base(loggerFactory) { - _serverName = serverName; - _logger = loggerFactory is not null ? loggerFactory.CreateLogger() : NullLogger.Instance; - _jsonOptions = JsonSerializerOptionsExtensions.DefaultOptions; + _serverName = serverName ?? throw new ArgumentNullException(nameof(serverName)); _input = input ?? throw new ArgumentNullException(nameof(input)); _output = output ?? throw new ArgumentNullException(nameof(output)); + + _logger = loggerFactory is not null ? loggerFactory.CreateLogger() : NullLogger.Instance; + + _jsonOptions = JsonSerializerOptionsExtensions.DefaultOptions; } /// @@ -224,4 +270,20 @@ public sealed class StdioServerTransport : TransportBase, IServerTransport SetConnected(false); _logger.TransportCleanedUp(EndpointName); } + + /// Validates the and extracts from it the server name to use. + private static string GetServerName(McpServerOptions serverOptions) + { + if (serverOptions is null) + { + throw new ArgumentNullException(nameof(serverOptions)); + } + + if (serverOptions.ServerInfo is null) + { + throw new ArgumentNullException($"{nameof(serverOptions)}.{nameof(serverOptions.ServerInfo)}"); + } + + return serverOptions.ServerInfo.Name; + } } \ No newline at end of file diff --git a/tests/mcpdotnet.Tests/Transport/StdioServerTransportTests.cs b/tests/mcpdotnet.Tests/Transport/StdioServerTransportTests.cs index 25a05ea..3ce7705 100644 --- a/tests/mcpdotnet.Tests/Transport/StdioServerTransportTests.cs +++ b/tests/mcpdotnet.Tests/Transport/StdioServerTransportTests.cs @@ -31,7 +31,7 @@ public class StdioServerTransportTests public async Task Constructor_Should_Initialize_With_Valid_Parameters() { // Act - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, InputOutputStreams.Default); + await using var transport = new StdioServerTransport(_serverOptions); // Assert Assert.NotNull(transport); @@ -40,21 +40,27 @@ public class StdioServerTransportTests [Fact] public void Constructor_Throws_For_Null_Options() { - var exception = Assert.Throws(() => new StdioServerTransport(serverOptions: null!, NullLoggerFactory.Instance, InputOutputStreams.Default)); - Assert.Equal("serverOptions", exception.ParamName); + Assert.Throws("serverName", () => new StdioServerTransport((string)null!)); + + Assert.Throws("serverOptions", () => new StdioServerTransport((McpServerOptions)null!)); + Assert.Throws("serverOptions.ServerInfo", () => new StdioServerTransport(new McpServerOptions() { ServerInfo = null! })); + Assert.Throws("serverName", () => new StdioServerTransport(new McpServerOptions() { ServerInfo = new() { Name = null!, Version = "" } })); } [Fact] public void Constructor_Throws_For_Null_Streams() { - var exception = Assert.Throws(() => new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, null!)); - Assert.Equal("inputOutputStreams", exception.ParamName); + Assert.Throws("input", () => new StdioServerTransport(_serverOptions, null!, Console.Out)); + Assert.Throws("input", () => new StdioServerTransport("name", null!, Console.Out)); + + Assert.Throws("output", () => new StdioServerTransport("name", Console.In, null!)); + Assert.Throws("output", () => new StdioServerTransport(_serverOptions, Console.In, null!)); } [Fact] public async Task StartListeningAsync_Should_Set_Connected_State() { - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, InputOutputStreams.Default); + await using var transport = new StdioServerTransport(_serverOptions); await transport.StartListeningAsync(); @@ -64,8 +70,10 @@ public class StdioServerTransportTests [Fact] public async Task SendMessageAsync_Should_Send_Message() { - var streams = new InputOutputStreams { Output = new StringWriter(), Input = new StringReader("") }; - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, streams); + var input = new StringReader(""); + var output = new StringWriter(); + + await using var transport = new StdioServerTransport(_serverOptions, input, output, NullLoggerFactory.Instance); await transport.StartListeningAsync(); var message = new JsonRpcRequest { Method = "test", Id = RequestId.FromNumber(44) }; @@ -73,7 +81,7 @@ public class StdioServerTransportTests await transport.SendMessageAsync(message); - var result = streams.Output.ToString()?.Trim(); + var result = output.ToString()?.Trim(); var expected = JsonSerializer.Serialize(message, JsonSerializerOptionsExtensions.DefaultOptions); Assert.Equal(expected, result); @@ -82,7 +90,7 @@ public class StdioServerTransportTests [Fact] public async Task SendMessageAsync_Throws_Exception_If_Not_Connected() { - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, InputOutputStreams.Default); + await using var transport = new StdioServerTransport(_serverOptions); var message = new JsonRpcRequest { Method = "test" }; @@ -92,7 +100,7 @@ public class StdioServerTransportTests [Fact] public async Task DisposeAsync_Should_Dispose_Resources() { - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, InputOutputStreams.Default); + await using var transport = new StdioServerTransport(_serverOptions); await transport.DisposeAsync(); @@ -105,10 +113,10 @@ public class StdioServerTransportTests var message = new JsonRpcRequest { Method = "test", Id = RequestId.FromNumber(44) }; var json = JsonSerializer.Serialize(message, JsonSerializerOptionsExtensions.DefaultOptions); - using var sr = new StringReader(json); + var input = new StringReader(json); + var output = new StringWriter(); - var streams = new InputOutputStreams { Output = new StringWriter(), Input = new StringReader(json) }; - await using var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, streams); + await using var transport = new StdioServerTransport(_serverOptions, input, output); await transport.StartListeningAsync(); var canRead = await transport.MessageReader.WaitToReadAsync(); @@ -123,7 +131,7 @@ public class StdioServerTransportTests [Fact] public async Task CleanupAsync_Should_Cleanup_Resources() { - var transport = new StdioServerTransport(_serverOptions, NullLoggerFactory.Instance, InputOutputStreams.Default); + var transport = new StdioServerTransport(_serverOptions); await transport.StartListeningAsync(); await transport.DisposeAsync();