Skip to content

Expand a few DI-related tests #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2025
Merged

Expand a few DI-related tests #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,20 +508,33 @@ public void WithTools_Parameters_Satisfiable_From_DI(bool parameterInServices)
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void WithToolsFromAssembly_Parameters_Satisfiable_From_DI(bool parameterInServices)
[InlineData(ServiceLifetime.Singleton)]
[InlineData(ServiceLifetime.Scoped)]
[InlineData(ServiceLifetime.Transient)]
[InlineData(null)]
public void WithToolsFromAssembly_Parameters_Satisfiable_From_DI(ServiceLifetime? lifetime)
{
ServiceCollection sc = new();
if (parameterInServices)
switch (lifetime)
{
sc.AddSingleton(new ComplexObject());
case ServiceLifetime.Singleton:
sc.AddSingleton(new ComplexObject());
break;

case ServiceLifetime.Scoped:
sc.AddScoped(_ => new ComplexObject());
break;

case ServiceLifetime.Transient:
sc.AddTransient(_ => new ComplexObject());
break;
}

sc.AddMcpServer().WithToolsFromAssembly();
IServiceProvider services = sc.BuildServiceProvider();

McpServerTool tool = services.GetServices<McpServerTool>().First(t => t.ProtocolTool.Name == "EchoComplex");
if (parameterInServices)
if (lifetime is not null)
{
Assert.DoesNotContain("\"complex\"", JsonSerializer.Serialize(tool.ProtocolTool.InputSchema));
}
Expand Down
45 changes: 36 additions & 9 deletions tests/ModelContextProtocol.Tests/Server/McpServerToolTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,47 @@ public async Task SupportsIMcpServer()
Assert.Equal("42", result.Content[0].Text);
}

[Fact]
public async Task SupportsServiceFromDI()
[Theory]
[InlineData(ServiceLifetime.Singleton)]
[InlineData(ServiceLifetime.Scoped)]
[InlineData(ServiceLifetime.Transient)]
public async Task SupportsServiceFromDI(ServiceLifetime injectedArgumentLifetime)
{
MyService expectedMyService = new();
MyService singletonService = new();

ServiceCollection sc = new();
sc.AddSingleton(expectedMyService);
IServiceProvider services = sc.BuildServiceProvider();
switch (injectedArgumentLifetime)
{
case ServiceLifetime.Singleton:
sc.AddSingleton(singletonService);
break;

case ServiceLifetime.Scoped:
sc.AddScoped(_ => new MyService());
break;

case ServiceLifetime.Transient:
sc.AddTransient(_ => new MyService());
break;
}

McpServerTool tool = McpServerTool.Create((MyService actualMyService) =>
sc.AddSingleton(services =>
{
Assert.Same(expectedMyService, actualMyService);
return "42";
}, new() { Services = services });
return McpServerTool.Create((MyService actualMyService) =>
{
Assert.NotNull(actualMyService);
if (injectedArgumentLifetime == ServiceLifetime.Singleton)
{
Assert.Same(singletonService, actualMyService);
}

return "42";
}, new() { Services = services });
});

IServiceProvider services = sc.BuildServiceProvider();

McpServerTool tool = services.GetRequiredService<McpServerTool>();
Comment on lines +86 to +88
Copy link
Contributor

@halter73 halter73 Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
IServiceProvider services = sc.BuildServiceProvider();
McpServerTool tool = services.GetRequiredService<McpServerTool>();
IServiceProvider services = sc.BuildServiceProvider(validateScopes: true);
await using var scope = services.CreateAsyncScope();
McpServerTool tool = scope.ServiceProvider.GetRequiredService<McpServerTool>();

Nit: Not a big deal since this is just a test, but resolving a scoped service without first creating a scope is bad practice, and our hosts now guard against it in dev environments. https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/9.0/hostbuilder-validation

Again, it doesn't really matter, but it's good to always show resolving scoped services from a scope. We might need to update SetupGet(x => x.Services).Returns(scope.ServiceProvider); below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see you had already merged. Nevermind. It's definitely no big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolving a scoped service without first creating a scope is bad practice

Is there a product change we need here, to ensure the IServiceProvider passed to the tool's invocation is appropriately scoped? I'm looking at #198 wondering how to repro that, but maybe we need to be creating a scope as part of a stdio server's handling?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to make any product changes related to scopes. At least not without deeper discussion.

Using scoped services in MCP tools is not currently supported for stdio scenarios (unless you disable validation) because there are no scopes. Without creating scopes, it's hard to see the point of scoped services anyway. You should be able to resolve request scoped services in tools invoked via MapMcp though. I'll add tests for this in my next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless you disable validation

Where is that validation coming from / why is my test above not hitting it?

Copy link
Contributor

@halter73 halter73 Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The validation is coming from the host in "development" environments from here. This used to always default to false until .NET 9.

I also left a comment on #198 after making my last comment here. After further thought, I do think we should make product changes. We should create a scope per handler invocation. I assigned the issue to myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation is coming from the host in "development" environments from here. This used to always default to false until .NET 9.

Ok, so this test isn't hitting that because it's not using a host.


Assert.DoesNotContain("actualMyService", JsonSerializer.Serialize(tool.ProtocolTool.InputSchema));

Expand Down