Add ConfigureHostApplicationBuilder virtual method on WebApplicationFactory to allow early configuration of the host application builder

[Youssef1313]

Fixes #37680
Fixes #66574

Runtime PR for HostFactoryResolver: dotnet/runtime#127743

Docs update: dotnet/AspNetCore.Docs#37291

[halter73]

I took a quick glance to see if I left any comments on #37680 (comment), and I see that I said we should just document the current behavior.

That means, based on my previous analysis, I think we should just teach people the following pattern for doing this:

​builder.Services.AddSingleton(new Options(builder.Configuration["AppSettings:Options"]));

https://github.com/tl-maisie-sadler/net6-config-error/blob/main/src/Demo.Api/Program.cs#L3

Is really this:

​builder.Services.AddSingleton(() => new Options(builder.Configuration["AppSettings:Options"]));

#37680 (comment)

The easiest thing to do on our end would to be to call this out in the docs for WebApplicationFactory at https://learn.microsoft.com/aspnet/core/test/integration-tests at https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/test/integration-tests.md rather than make any potentially risky changes to WebApplicationBuilder or HostFactoryResolver.

This makes sense to me, because test configuration code naturally happens in the middle of normal configuration (so it can replace services added by normal configuration rather than be replaced by them. Same with configuration.

One thing that makes WebApplicationBuilder trickier than the older HostBuilder-based APIs, is that we let you read the builder.Configuration while you're still adding configuration sources, like test configuration sources, so you can end up accidentally reading config too early pretty easily. This is not as common of a problem with builder.Services, since most people know to not try to resolve services prior to Build() the whole host (although you do see people call BuildServiceProvider() themselves and pay the consequences.

There might be some trick we can do in HostFactoryResolver that both adds test configuration sources before the WebApplication.CreateBuilder() runs so that early reads of config pull in test config, and then moves the configuration to the end later (so as to not break people who expected test config to come after normal config and override it), I would consider it. I think we'd have to be careful with the implementation, because deleting a configuration source from the beginning and adding it to the end will get worst-case behavior from Microsoft.Extensions.Configuration.ConfigurationManager (O(n) where n is the total number of config sources), but this worst-case behavior might be okay for just testing and other design-time scenarios that use HostFactoryResolver (like EF migrations).

[halter73]

I also left this comment on a related issue:

The fundamental issue here is that CustomWebApplicationFactory.ConfigureWebHost() is called during var app = builder.Build(); after var useTests = builder.Configuration.GetValue<bool>("UseTests"); already ran. It's important that ConfigureWebHost() runs this late so it can override services and configuration set up before the call to builder.Build().

If you want to configure the app.Configuration you can do so with the WebApplicationFactory, but you cannot influence builder.Configuration. As a workaround, if you need to influence app logic before calling builder.Build(), maybe you can set an environment variable from the test and read that.

Long term, we could consider a new WebApplicationFactory method that gets called when the WebApplicationBuilder is constructed that could influence the initial builder.Configuration and builder.Services, but this would require some design.

#40681

You can also see one doc improvement I made to this area with dotnet/AspNetCore.Docs#28697

I stand by my position there too. If we cannot do the HostFactoryResolver double-test-configuration-load-magic, we should just create a new method to add config even earlier, but will get overridden by custom application configuration sources.

[Youssef1313]

There might be some trick we can do in HostFactoryResolver that both adds test configuration sources before the WebApplication.CreateBuilder() runs.

I think it's unlikely to be before, but during. So after the call returns, the test configuration can be accessed.

One important thing to note is whether the users need the actual configuration source to be fully available, or if it's okay to have a snapshot of configuration that is then passed to the entrypoint. Having a snapshot is already possible today and the approach was suggested in one of the comments in the original issue, but it seems another user needed the actual configuration source to still be available. Why or what's the scenario, I don't know. Maybe we need to collect more info from users running into this issue to understand:

1. Why having a snapshot (which is effectively just overriding CreateHost instead of ConfigureWebHost, and calling ConfigureHostConfiguration instead of ConfigureAppConfiguration) isn't sufficient.
2. Why it is really needed for the configuration source to be available before Build is called. A service can still be registered before Build is called with a lambda so that the configuration is accessed at the right point. Are there any user scenarios where this won't work and the configuration need to really be read early before Build is called
3. Given that 1) and 2) are different alternatives, each with its own limitations, I would be especially interested in knowing a scenario where neither of the two approaches can work for the user (i.e, the user needs both the actual configuration source instead of a snapshot and also need to really read a configuration value early)

In addition, the approach I'm taking here, while it might work:

• it might be unnecessary complexity if the existing alternatives can work.
• So far it cannot pass HostBuilderContext (and so will have to be some new API with just Action<IConfigurationBuilder>). The new API is likely to be in Mvc.Testing only, but it's still additional maintenance burden, so if it can be avoided, that's definitely better.

Moreover, the suggested approach here touches WebApplicationBuilder a little bit, to notify Mvc.Testing about the correct timing and the HostApplicationBuilder it should deal with. So there is additional risk of what can happen when new Mvc.Testing is used along with old Microsoft.AspNetCore assembly (and whether or not that is a supported scenario).

I'm just thinking out loud here, trying to also spot any disadvantages with the suggested approach here.

---

I think we'd have to be careful with the implementation, because deleting a configuration source from the beginning and adding it to the end will get worst-case behavior from Microsoft.Extensions.Configuration.ConfigurationManager (O(n) where n is the total number of config sources), but this worst-case behavior might be okay for just testing and other design-time scenarios that use HostFactoryResolver (like EF migrations).

While you already said that this might be okay, I'm somewhat interested in knowing more about this .

---

I stand by my position there too. If we cannot do the HostFactoryResolver double-test-configuration-load-magic, we should just create a new method to add config even earlier, but will get overridden by custom application configuration sources.

The PR on its current shape is definitely not merge-able, it's more of a PoC, but if we ever wanted to do anything, I think it will have to be a new API (virtual method most likely) on Mvc.Testing. So, consider as if the changes suggested here are refactored into a new API. The question now is more of, do we want to make any change at all, or just update the documentation, and whether or not we really fully understand the user scenario here.

[halter73]

It sounds like we're in agreement then. Having a new API on MVC testing for config to be added during WebApplication.CreateBuilder(), so overrides can be read pre-build. If you also want to override custom config sources during Build, you can continue to do that the way you do today. And if you want to have the test config early, and then also override app config that gets added later, you'll probably have to add your test config twice.

[Youssef1313]

It sounds like we're in agreement then. Having a new API on MVC testing for config to be added during WebApplication.CreateBuilder(), so overrides can be read pre-build. If you also want to override custom config sources during Build, you can continue to do that the way you do today. And if you want to have the test config early, and then also override app config that gets added later, you'll probably have to add your test config twice.

@halter73 Great. I'll try to adjust my prototype here to be a new API and try to ensure it works, and then open an API proposal issue for review. Once the API is approved, I'll then mark this PR as ready for review. Does that sound like a good plan to you?

[Youssef1313]

@halter73 @javiercn Aside from currently copying runtime's HostFactoryResolver, this is ready for review. I'll cleanup and use the source-only NuGet package back once my PR to dotnet/runtime merges and digested in this repo.

[DeagleGross]

is it HostApplicationBuilderConstructed not WebApplicationBuilderConstructed for matching other events?

[Youssef1313]

We could use WebApplicationBuilderConstructed for sure. It's not user-facing but I like the suggestion. Going to make this change.

[halter73]

Are we still planning to make this change? I like it. Even though the signature is IHostApplicationBuilder again, the object will always be a WebApplicationBuilder, not a HostApplicationBuilder (at least not directly).

I also wonder if ConfigureWebApplicationBuilder might not be a better name for the public API since that's the only host type supported by that method. I wouldn't hold this PR on that though. I definitely want to get the already-approved API merged first. But if there's consensus that ConfigureWebApplicationBuilder is better (despite still taking an IHostApplcationBuilder argument), we can do that as a fast follow.

[Youssef1313]

Tracking in #67393

[halter73]

Are we still planning to make this change? I like it. Even though the signature is IHostApplicationBuilder again, the object will always be a WebApplicationBuilder, not a HostApplicationBuilder (at least not directly).

I also wonder if ConfigureWebApplicationBuilder might not be a better name for the public API since that's the only host type supported by that method. I wouldn't hold this PR on that though. I definitely want to get the already-approved API merged first. But if there's consensus that ConfigureWebApplicationBuilder is better (despite still taking an IHostApplcationBuilder argument), we can do that as a fast follow.