Don't enable validation when UseDefaultServiceProvider is called

[ericstj]

Fix #105134

Not entirely sure this is the right thing, or if we should just revert the regressing change.

Please have a look @wcsanders1

[wcsanders1]

Fix #105134

Not entirely sure this is the right thing, or if we should just revert the regressing change.

Please have a look @wcsanders1

@ericstj It seems to me that this would correct the problem of _defaultProviderFactoryUsed being true where it should be false. Maybe this is not an ideal solution because future methods making assignments to _serviceProviderFactory would need to be aware that _defaultProviderFactoryUsed needs to be set. However, I don't see a better solution without more substantial refactoring.

Reverting my change would mean scope and dependency-chain validation would not happen by default in development where Host.CreateDefaultBuilder is not called.

[ericstj]

@wcsanders1 was this what you had in mind when you added _defaultProviderFactoryUsed. Why wasn't that ever set to false in the original PR?

[wcsanders1]

@wcsanders1 was this what you had in mind when you added _defaultProviderFactoryUsed. Why wasn't that ever set to false in the original PR?

This is what I had in mind. I neglected to set it to false because I failed to see that the factory method also uses that constructor.

[ericstj]

@steveharter could you review this change?

[halter73]

Am I correct in assuming that without this fix UseServiceProviderFactory is completely broken in development? If so, I wonder if we should backport to preview7.

Not entirely sure this is the right thing, or if we should just revert the regressing change.

This is better than what we had but reading the following from #97091 makes me tempted to revert this.

It looks like this failure to validate the scope is a result of calling the HostBuilder constructor rather than the CreateDefaultBuilder method, which enables scope validation when the environment is Development:

I think this analysis is correct, but I don't think we should make the HostBuilder constructor behave like CreateDefaultBuilder just because we like the CreateDefaultBuilder behavior better. CreateDefaultBuilder is meant to be more opinionated.

The only reason GrandNode happened to be calling the following

    .UseDefaultServiceProvider((context, options) =>
    {
        options.ValidateScopes = false;
        options.ValidateOnBuild = false;
    });

was because it was using CreateDefaultBuilder instead of the HostBuilder constructor. I think there will be far more code that was previously using HostBuilder that won't explicitly disable validation because it wasn't necessary. While resolving a scoped service from the root container is generally a bad idea and can probably always be avoided with proper care, there's plenty of scenarios where it won't cause real problems.

[ericstj]

If so, I wonder if we should backport to preview7.

Yes, my plan is to take this as ask for P7.

I think there will be far more code that was previously using HostBuilder that won't explicitly disable validation because it wasn't necessary. While resolving a scoped service from the root container is generally a bad idea and can probably always be avoided with proper care, there's plenty of scenarios where it won't cause real problems.

Yes, that's why I put the breaking-change label on the original PR. I thought the goal of that PR was to intentionally enable this in and document that as a breaking change.

Do folks think it's too breaking for the value that it provides? We could let this go out this way in P7 (bringing this fix in) and see what folks say and revert completely it if it's too breaking.

[ericstj]

/ba-g failure here is #105332 which is resolved as fixed with #104516 but that was merged after this built.

[steveharter]

Am I correct in assuming that without this fix UseServiceProviderFactory is completely broken in development? If so, I wonder if we should backport to preview7.

Yes we need this fix since _serviceProviderFactory will always be overwritten by the one assigned in UseServiceProviderFactory.

[ericstj]

/backport to release/9.0-preview7

[github-actions]

Started backporting to release/9.0-preview7: https://github.com/dotnet/runtime/actions/runs/10066731186