Skip to content

EndpointBuilder.ServiceProvider should be non-nullabe and renamed to ApplicationServices #42137

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.

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

Closed
halter73 opened this issue Jun 10, 2022 · 2 comments · Fixed by #42195
Closed
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Jun 10, 2022

Background and Motivation

We missed this when renaming Services to ApplicationServices on a few other types in #41900 (comment).

Proposed API

namespace Microsoft.AspNetCore.Builder;

public abstract class EndpointBuilder
{
-    public IServiceProvider? ServiceProvider { get; set; }
+    public IServiceProvider ApplicationServices { get; set; }
}

Usage Examples

var serviceProvider = routeEndpointBuilder.ApplicationServices;
var hostEnvironment = serviceProvider.GetService<IHostEnvironment>();
var serviceProviderIsService = serviceProvider.GetService<IServiceProviderIsService>();

Alternative Designs

Keep ApplicationServices nullable. OpenApiRouteHandlerBuilderExtensions currently has different behavior if the IServiceProvider is null or not. If it's null it doesn't add any metadata to the endpoint.

var serviceProvider = routeEndpointBuilder.ServiceProvider;
if (methodInfo == null || serviceProvider == null)
{
return null;
}
var hostEnvironment = serviceProvider.GetService<IHostEnvironment>();
var serviceProviderIsService = serviceProvider.GetService<IServiceProviderIsService>();

Is this what we want for WithOpenApi? Could it be valid to change behavior given a "null" ServiceProvider for other things adding endpoint metadata? What about route handler filters with RouteHandlerContext.ApplicationServices? Technically, we're only hiding information by making these non-nullable. Maybe this is best to hide this information so people don't short circuit for a null service provider. I'm not sure. What do we think @captainsafia @DamianEdwards @davidfowl?

We could also keep the ServiceProvider name even though we've avoided it in other APIs. It does align with IEndpointRouteBuilder.ServiceProvider that first shipped in 3.0. I do like the increased clarity of ApplicationServices though, and we seem to use that in more places.

Risks

This hasn't shipped in a major release, so the risk is not doing this before we are unable to make breaking changes.

@halter73 halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 10, 2022
@adityamandaleeka adityamandaleeka added this to the 7.0-preview6 milestone Jun 13, 2022
@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 13, 2022
@ghost
Copy link

ghost commented Jun 13, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 17, 2022
@halter73
Copy link
Member Author

halter73 commented Jun 17, 2022

API review notes from @captainsafia:

  • [this proposal] is a sensible change that reflects our efforts to make use of IServiceProvider​ in contexts more consistent.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants