Skip to content
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

[API Proposal]: Add IHostApplicationBuilder interface #85486

Closed
Tracked by #78518
eerhardt opened this issue Apr 27, 2023 · 27 comments · Fixed by #86974
Closed
Tracked by #78518

[API Proposal]: Add IHostApplicationBuilder interface #85486

eerhardt opened this issue Apr 27, 2023 · 27 comments · Fixed by #86974
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Hosting
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Apr 27, 2023

Background and motivation

In .NET 6, ASP.NET added a WebApplicationBuilder "minimal hosting" API. This was a new, non-callback approach to the HostBuilder Generic Host API. In .NET 7, Extensions.Hosting added a similar "generic host" API that followed the same non-callback API design.

The problem is these two hosting APIs have the same members (mostly), but don't have a common base abstraction that libraries can add extension methods to. In the previous, callback API IHostBuilder was the interface libraries could extend and add their library-specific DI services, etc. to.

To enable this, we should add a general abstraction to these hosting builder APIs.

See the conversations at:

API Proposal

namespace Microsoft.Extensions.Hosting;

public interface IHostApplicationBuilder
{
    IDictionary<object, object> Properties { get; }

    IConfigurationManager Configuration { get; }
    IHostEnvironment Environment { get; }
    ILoggingBuilder Logging { get; }
    IServiceCollection Services { get; }

    IHost Build();  // See Note below on `Build()`
    void ConfigureContainer<TContainerBuilder>(IServiceProviderFactory<TContainerBuilder>factory, Action<TContainerBuilder>? configure = null) where TContainerBuilder: notnull;
}

-public sealed class HostApplicationBuilder {}
+public sealed class HostApplicationBuilder : IHostApplicationBuilder {}

namespace Microsoft.Extensions.Configuration;

public interface IConfigurationManager : IConfiguration, IConfigurationBuilder
{
}

-public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable {}
+public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable, IConfigurationManager {}

Note on Build()

If we wanted to make this API work for all ApplicationBuilders like MauiAppBuilder and WebAssemblyHostBuilder, we would need to think about IHost Build(). Those 2 app models follow this same ApplicationBuilder pattern, but the things they build don't implement IHost. Options here are:

  • Don't put Build() in the interface. But the name of the interface is "Builder", so it doesn't feel right to not have a Build method.
  • For the app models that don't support IHost, they would throw NotSupported for this method. This is my recommendation.

API Usage

Using an existing extension method on IHostBuilder as an example, a new extension method could be written like:

public static class HostBuilderExtensions
{
    public static IHostBuilder AddSteeltoe(this IHostApplicationBuilder hostBuilder, IEnumerable<string> exclusions = null, ILoggerFactory loggerFactory = null)
    {
...
        hostBuilder.Properties[LoggerName] = logger;

        hostBuilder.Configuration.AddSteeltoeConfigServer();
        hostBuilder.Services.AddMongoClient()
    }
}

Alternative Designs

We could make WebApplicationBuilder and HostApplicationBuilder explicitly implement IHostBuilder, but this breaks the API design of making the hosting code minimal, and running inline. It would cause a lot of confusion on when services, config, etc gets added in which order. For this reason, we don't want to keep using the IHostBuilder interface in new code.

Risks

N/A

@eerhardt eerhardt added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Hosting labels Apr 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 27, 2023
@ghost
Copy link

ghost commented Apr 27, 2023

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

In .NET 6, ASP.NET added a WebApplicationBuilder "minimal hosting" API. This was a new, non-callback approach to the HostBuilder Generic Host API. In .NET 7, Extensions.Hosting added a similar "generic host" API that followed the same non-callback API design.

The problem is these two hosting APIs have the same members (mostly), but don't have a common base abstraction that libraries can add extension methods to. In the previous, callback API IHostBuilder was the interface libraries could extend and add their library-specific DI services, etc. to.

To enable this, we should add a general abstraction to these hosting builder APIs.

See the conversations at:

API Proposal

namespace Microsoft.Extensions.Hosting;

public interface IHostApplicationBuilder
{
    IDictionary<object, object> Properties { get; }

    IConfigurationBuilder Configuration { get; }  // can't be ConfigurationManager since that is in MS.Ext.Configuration and Hosting.Abstractions references Configuration.Abstractions. So need to pick between IConfiguration and IConfigurationBuilder
    IHostEnvironment Environment { get; }
    ILoggingBuilder Logging { get; }
    IServiceCollection Services { get; }

    IHost Build();  // See Note below on `Build()`
    void ConfigureContainer<TContainerBuilder>(IServiceProviderFactory<TContainerBuilder>factory, Action<TContainerBuilder>? configure = null) where TContainerBuilder: notnull;
}

Note on Build()

If we wanted to make this API work for all ApplicationBuilders like MauiAppBuilder and WebAssemblyHostBuilder, we would need to think about IHost Build(). Those 2 app models follow this same ApplicationBuilder pattern, but the things they build don't implement IHost. Options here are:

  • Don't put Build() in the interface. But the name of the interface is "Builder", so it doesn't feel right to not have a Build method.
  • For the app models that don't support IHost, they would throw NotSupported for this method. This is my recommendation.

API Usage

Using an existing extension method on IHostBuilder as an example, a new extension method could be written like:

public static class HostBuilderExtensions
{
    public static IHostBuilder AddSteeltoe(this IHostBuilder hostBuilder, IEnumerable<string> exclusions = null, ILoggerFactory loggerFactory = null)
    {
...
        hostBuilder.Properties[LoggerName] = logger;

        hostBuilder.Configuration.AddSteeltoeConfigServer();
        hostBuilder.Services.AddMongoClient()
    }
}

Alternative Designs

We could make WebApplicationBuilder and HostApplicationBuilder explicitly implement IHostBuilder, but this breaks the API design of making the hosting code minimal, and running inline. It would cause a lot of confusion on when services, config, etc gets added in which order. For this reason, we don't want to keep using the IHostBuilder interface in new code.

Risks

N/A

Author: eerhardt
Assignees: -
Labels:

api-suggestion, area-Extensions-Hosting

Milestone: -

@eerhardt
Copy link
Member Author

cc @davidfowl @halter73

@davidfowl
Copy link
Member

We have to fix the Configuration API to allow reading it. Maybe we need an interface for the ConfigurationManager (this is why interfaces are great 😄).

@eerhardt
Copy link
Member Author

We have to fix the Configuration API to allow reading it.

How do you feel about 2 properties?

public interface IHostApplicationBuilder
{
    IConfiguration Configuration { get; }
    IConfigurationBuilder ConfigurationBuilder { get; }

?

@davidfowl
Copy link
Member

Doesn't feel good, but it's an option.

@eerhardt
Copy link
Member Author

Updated the proposed API to include a new interface IConfigurationManager : IConfiguration, IConfigurationBuilder

@eerhardt
Copy link
Member Author

cc @dotnet/area-extensions-configuration as well, since this proposal spans both Hosting and Configuration.

@steveharter
Copy link
Member

Who is planning on driving and implementing this?

@ericstj
Copy link
Member

ericstj commented May 4, 2023

@eerhardt @halter73 -- does this proposal imply libraries need to extend both IHostBuilder and IHostApplicationBuilder if they'd want to work in both minimal APIs and not?

@halter73
Copy link
Member

halter73 commented May 4, 2023

The majority of libraries can get by just extending IServiceCollection. But if libraries need to also add config source and add services in a single method, or read the config/environment before adding services, they'll need to extend both IHostBuilder and IHostApplicationBuilder as you say.

I was able to update UseWindowsService and UseSystemd which previously targeted IHostBuilder to target IServiceCollection instead by reading the IHostEnvironment when constructing services rather than when adding them collection. I was hoping that most IHostBuilder extension methods could be updated to do something similar to avoid adding yet another builder interface.

If you extend just IHostBuilder, it will often still work with WebApplicationBuilder via the WebApplicationBuilder.Host property unless the extension method tries changing the host environment (e.g. by changing the content root) or calling Build() which is simply unsupported.

We could consider adding a similar IHostBuilder property to HostApplicationBuilder. We've already done most of the work to implement it because that's what's exposed via the "Microsoft.Extensions.Hosting" DiagnosticListener, but we decided against this in the original HostApplicationBuilder API review because of the inability to change certain host configuration keys like HostDefaults.ContentRootKey in ConfigureHostConfiguration or call Build() was clunky.

internal static DiagnosticListener LogHostBuilding(HostApplicationBuilder hostApplicationBuilder)
{
var diagnosticListener = new DiagnosticListener(HostBuildingDiagnosticListenerName);
if (diagnosticListener.IsEnabled() && diagnosticListener.IsEnabled(HostBuildingEventName))
{
Write(diagnosticListener, HostBuildingEventName, hostApplicationBuilder.AsHostBuilder());
}
return diagnosticListener;
}

// It's okay if the ConfigureHostConfiguration callbacks either left the config unchanged or set it back to the real ContentRootPath.
// Setting it to anything else indicates code intends to change the content root via HostFactoryResolver which is unsupported.
string? currentContentRootConfig = config[HostDefaults.ContentRootKey];
if (!string.Equals(previousContentRootConfig, currentContentRootConfig, StringComparison.OrdinalIgnoreCase) &&
!string.Equals(previousContentRootPath, HostBuilder.ResolveContentRootPath(currentContentRootConfig, AppContext.BaseDirectory), StringComparison.OrdinalIgnoreCase))
{
throw new NotSupportedException(SR.Format(SR.ContentRootChangeNotSupported, previousContentRootConfig, currentContentRootConfig));
}

@eerhardt
Copy link
Member Author

eerhardt commented May 4, 2023

The majority of libraries can get by just extending IServiceCollection

Agreed. That is what I found most libraries doing. Only a few, bigger libraries actually needed to extend IHostBuilder.

https://grep.app/search?q=this%20IHostBuilder

There are quite a few of cases I found that are just using .ConfigureServices.

We could consider adding a similar IHostBuilder property to HostApplicationBuilder

I am not in favor of doing this. I think we need to break away from the old, callback approach. My reasoning is in the Alternative Designs section above.

@davidfowl
Copy link
Member

Let's push forward with @eerhardt 's proposal.

@eerhardt
Copy link
Member Author

eerhardt commented May 4, 2023

Who is planning on driving and implementing this?

I can drive it in .NET 8.

@eerhardt eerhardt self-assigned this May 4, 2023
@eerhardt eerhardt added this to the 8.0.0 milestone May 4, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 4, 2023
@ericstj
Copy link
Member

ericstj commented May 4, 2023

So the problem statement here is that existing libraries which extend IHostBuilder don't work with minimal APIs. We'd like to create a way for them to work with minimal APIs that doesn't involve implementing an extension method for every concrete ApplicationBuilder that exists.

Looks like we'd need to ask all those folks who have already implemented extensions to WebApplicationBuilder and HostApplicationBuilder to replace those with this new interface. Would we also want an analyzer to remind folks when extending IHostBuilder that they should also extend IHostApplicationBuilder? I understand that we wouldn't want minimal API apps to use IHostBuilder - but folks building extensions would need to. Minimal APIs remain an equal alternative to the old way right?

@eerhardt
Copy link
Member Author

eerhardt commented May 4, 2023

Looks like we'd need to ask all those folks who have already implemented extensions to WebApplicationBuilder and HostApplicationBuilder to replace those with this new interface.

I have logged a few issues on code that I found extending IHostBuilder to make extension methods on IServiceCollection.

https://grep.app/search?q=this%20HostApplicationBuilder didn't find anyone trying to add an extension for HostApplicationBuilder.

https://grep.app/search?q=this%20WebApplicationBuilder just finds regular apps defining their own extension methods. Outside of SteelToeOSS, I didn't find any reusable libraries extending WebApplicationBuilder. Which makes sense - reusable libraries don't want to depend directly on ASP.NET.

Would we also want an analyzer to remind folks when extending IHostBuilder that they should also extend IHostApplicationBuilder?

I'm not sure that analyzer would be worth the effort. It would also cause false positives in the case where I am just creating an extension method in my own app that uses IHostBuilder.

Minimal APIs remain an equal alternative to the old way right?

There is a bit of conflation of terms here. "Minimal APIs" mostly means being able to use app.MapGet("route", () => ...) vs. using MVC Controllers. The new, inline Hosting APIs came with "Minimal APIs", but in my mind is separate. For example you can use the new, inline Hosting APIs with MVC Controllers.

After dotnet/aspnetcore#47720, we no longer have any templates in .NET 8 that use the "old Hosting" APIs (i.e. IHostBuilder). Which is what prompted this issue.

@ericstj
Copy link
Member

ericstj commented May 6, 2023

The new, inline Hosting APIs came with "Minimal APIs", but in my mind is separate. For example you can use the new, inline Hosting APIs with MVC Controllers.

So how are we thinking about these in relation to the old callback APIs - both equal - or are the callback APIs now legacy?

@eerhardt
Copy link
Member Author

eerhardt commented May 8, 2023

In my mind, the callback APIs are not going to get anymore improvements. Any new scenarios will only be using the inline Hosting APIs. So the callback APIs are effectively legacy.

@davidfowl
Copy link
Member

So how are we thinking about these in relation to the old callback APIs - both equal - or are the callback APIs now legacy?

This is a tough question to answer, but we're attempting to move to this new model as it solves a couple of problems we have with the callback model and is a simpler API. That said, there are a different set of tradeoffs this model introduces over the callback model we had before (ones that we've discovered while having this). That said, it's the model we're trying to move forward with.

@eerhardt eerhardt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 18, 2023
@eerhardt
Copy link
Member Author

Marking as ready for review in order to get this into .NET 8.

@davidfowl
Copy link
Member

@eerhardt This will need to support the new IMetricsBuilder in the future as well cc @noahfalk

@eerhardt
Copy link
Member Author

This will need to support the new IMetricsBuilder in the future as well

Do you have a link for that proposal? This issue is the only one that comes up when I search this repo for IMetricsBuilder. Did you mean IMeterFactory? #77514?

@tarekgh
Copy link
Member

tarekgh commented May 19, 2023

@eerhardt

@noahfalk is currently working on creating the proposal. So, it is not out yet.

@davidfowl
Copy link
Member

Part of that was that we know of new things that will be on these builders. Not saying it will be an abstract base class (you know how much I like those!) but we do need to evolve this API.

@eerhardt
Copy link
Member Author

We've established using DIMs in Microsoft.Extensions.* for net6.0+ targets. For example, see #50406.

@davidfowl
Copy link
Member

I love it!

@noahfalk
Copy link
Member

@noahfalk is currently working on creating the proposal. So, it is not out yet.

I have my working draft here: https://gist.github.com/noahfalk/32bb919907c972ac06ac128bc767d48d#integration-with-hosting-apis

I've never worked in the hosting APIs before so the current design follows the philosophy "duplicate whatever ILoggingBuilder did". Feedback is welcome if you think there is something we should be doing differently.

@terrajobst
Copy link
Member

terrajobst commented May 30, 2023

Video

  • We should remove the Build method
    • The people consuming the abstractions don't need it and the final result will have different types to be useful
 namespace Microsoft.Extensions.Hosting;
 
 public interface IHostApplicationBuilder
 {
     IDictionary<object, object> Properties { get; }
 
     IConfigurationManager Configuration { get; }
     IHostEnvironment Environment { get; }
     ILoggingBuilder Logging { get; }
     IServiceCollection Services { get; }
 
     void ConfigureContainer<TContainerBuilder>(IServiceProviderFactory<TContainerBuilder>factory, Action<TContainerBuilder>? configure = null) where TContainerBuilder: notnull;
 }

-public sealed class HostApplicationBuilder {}
+public sealed class HostApplicationBuilder : IHostApplicationBuilder {}
 namespace Microsoft.Extensions.Configuration;
 
 public interface IConfigurationManager : IConfiguration, IConfigurationBuilder
 {
 }

-public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable {}
+public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable, IConfigurationManager {}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 30, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 31, 2023
eerhardt added a commit to eerhardt/runtime that referenced this issue Jun 3, 2023
- Move ILoggingBuilder from MS.Ext.Logging to MS.Ext.Logging.Abstractions so IHostApplicationBuilder can reference it.
- Add IConfigurationManager and implement it in ConfigurationManager.
- Add CompatiibilitySuppressions. These errors are caused by ILoggingBuilder being moved to Logging.Abstractions, and ApiCompat not respecting TypeForwardedTo attributes.

Fix dotnet#85486
eerhardt added a commit that referenced this issue Jun 8, 2023
* Add IHostApplicationBuilder and implement it in HostApplicationBuilder.

- Move ILoggingBuilder from MS.Ext.Logging to MS.Ext.Logging.Abstractions so IHostApplicationBuilder can reference it.
- Add IConfigurationManager and implement it in ConfigurationManager.
- Add CompatiibilitySuppressions. These errors are caused by ILoggingBuilder being moved to Logging.Abstractions, and ApiCompat not respecting TypeForwardedTo attributes.

Fix #85486
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 8, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 9, 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-Extensions-Hosting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants