Move GetRootServiceProvider to abstractions#9067
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9067 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors the GetRootServiceProvider extension methods by moving them from concrete implementation files to the abstractions layer, centralizing them in a single location for better maintainability and reusability.
Changes:
- Moved
GetRootServiceProviderextension methods from execution and fusion implementation files to a newExecutionServiceProviderExtensionsclass in the abstractions layer - Changed parameter type from concrete
Schemaclass toISchemaDefinitioninterface for better abstraction - Added
Microsoft.Extensions.DependencyInjection.Abstractionspackage reference to support the new extension methods - Added
BuildRequestExecutorAsynchelper method forIFusionGatewayBuilder
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/HotChocolate/Fusion-vnext/src/Fusion.Execution/DependencyInjection/HotChocolateFusionServiceCollectionExtensions.cs | Removed duplicate GetRootServiceProvider extension method |
| src/HotChocolate/Core/src/Execution/DependencyInjection/InternalSchemaServiceCollectionExtensions.cs | Removed duplicate GetRootServiceProvider extension methods (both Schema and IServiceProvider overloads) |
| src/HotChocolate/Core/src/Execution.Abstractions/HotChocolate.Execution.Abstractions.csproj | Added Microsoft.Extensions.DependencyInjection.Abstractions package reference |
| src/HotChocolate/Core/src/Execution.Abstractions/Execution/Extensions/ExecutionServiceProviderExtensions.cs | New centralized location for GetRootServiceProvider extension methods with ISchemaDefinition parameter |
| src/HotChocolate/Fusion-vnext/src/Fusion.Execution/DependencyInjection/CoreFusionGatewayBuilderExtensions.Services.cs | Added BuildRequestExecutorAsync helper method and HotChocolate.Execution using statement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .GetRequiredService<IRequestExecutorProvider>() | ||
| .GetExecutorAsync(schemaName, cancellationToken); |
There was a problem hiding this comment.
This method is a duplicate of the existing BuildRequestExecutorAsync extension method found in src/HotChocolate/Core/src/Execution/DependencyInjection/RequestExecutorServiceProviderExtensions.cs (lines 109-117). Both IFusionGatewayBuilder and IRequestExecutorBuilder have identical structure (Name and Services properties), so this duplication could be avoided by either: 1) extracting a common interface, 2) creating a shared helper method, or 3) using a generic extension method that works with any type that has a Services property of type IServiceCollection.
| .GetRequiredService<IRequestExecutorProvider>() | |
| .GetExecutorAsync(schemaName, cancellationToken); | |
| .BuildRequestExecutorAsync(schemaName, cancellationToken); |
| public static ValueTask<IRequestExecutor> BuildRequestExecutorAsync( | ||
| this IFusionGatewayBuilder builder, | ||
| string? schemaName = null, | ||
| CancellationToken cancellationToken = default) => |
There was a problem hiding this comment.
This method is missing XML documentation comments. Based on the existing pattern in the codebase (see RequestExecutorServiceProviderExtensions.cs lines 93-109), this method should include XML documentation describing its purpose, parameters, and return value.
| public static ValueTask<IRequestExecutor> BuildRequestExecutorAsync( | ||
| this IFusionGatewayBuilder builder, | ||
| string? schemaName = null, | ||
| CancellationToken cancellationToken = default) => | ||
| builder | ||
| .Services | ||
| .BuildServiceProvider() | ||
| .GetRequiredService<IRequestExecutorProvider>() | ||
| .GetExecutorAsync(schemaName, cancellationToken); |
There was a problem hiding this comment.
Calling BuildServiceProvider() creates a new service provider instance that is not disposed. This can lead to resource leaks, especially if the service provider contains disposable services. The existing implementation in RequestExecutorServiceProviderExtensions.cs has the same issue, but adding another instance of this pattern compounds the problem. Consider documenting this limitation or using a using statement to ensure proper disposal.
| public static ValueTask<IRequestExecutor> BuildRequestExecutorAsync( | |
| this IFusionGatewayBuilder builder, | |
| string? schemaName = null, | |
| CancellationToken cancellationToken = default) => | |
| builder | |
| .Services | |
| .BuildServiceProvider() | |
| .GetRequiredService<IRequestExecutorProvider>() | |
| .GetExecutorAsync(schemaName, cancellationToken); | |
| public static async ValueTask<IRequestExecutor> BuildRequestExecutorAsync( | |
| this IFusionGatewayBuilder builder, | |
| string? schemaName = null, | |
| CancellationToken cancellationToken = default) | |
| { | |
| using var serviceProvider = builder.Services.BuildServiceProvider(); | |
| var executorProvider = serviceProvider.GetRequiredService<IRequestExecutorProvider>(); | |
| return await executorProvider | |
| .GetExecutorAsync(schemaName, cancellationToken) | |
| .ConfigureAwait(false); | |
| } |
🚀 Fusion Gateway Performance ResultsSimple Composite QueryConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query fragment User on User {
id
username
name
}
fragment Review on Review {
id
body
}
fragment Product on Product {
inStock
name
price
shippingEstimate
upc
weight
}
query TestQuery {
topProducts(first: 5) {
...Product
reviews {
...Review
author {
...User
}
}
}
}Deep Recursion QueryConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query fragment User on User {
id
username
name
}
fragment Review on Review {
id
body
}
fragment Product on Product {
inStock
name
price
shippingEstimate
upc
weight
}
query TestQuery {
users {
...User
reviews {
...Review
product {
...Product
reviews {
...Review
author {
...User
reviews {
...Review
product {
...Product
}
}
}
}
}
}
}
topProducts(first: 5) {
...Product
reviews {
...Review
author {
...User
reviews {
...Review
product {
...Product
}
}
}
}
}
}Variable Batching ThroughputConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query query TestQuery_8f7a46ce_2(
$__fusion_1_upc: ID!
$__fusion_2_price: Long!
$__fusion_2_weight: Long!
) {
productByUpc(upc: $__fusion_1_upc) {
inStock
shippingEstimate(weight: $__fusion_2_weight, price: $__fusion_2_price)
}
}Variables (5 sets batched in single request) [
{ "__fusion_1_upc": "1", "__fusion_2_price": 899, "__fusion_2_weight": 100 },
{ "__fusion_1_upc": "2", "__fusion_2_price": 1299, "__fusion_2_weight": 1000 },
{ "__fusion_1_upc": "3", "__fusion_2_price": 15, "__fusion_2_weight": 20 },
{ "__fusion_1_upc": "4", "__fusion_2_price": 499, "__fusion_2_weight": 100 },
{ "__fusion_1_upc": "5", "__fusion_2_price": 1299, "__fusion_2_weight": 1000 }
]No baseline data available for comparison. Run 21392629778 • Commit bf56797 • Tue, 27 Jan 2026 10:14:53 GMT |
No description provided.