dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.11k stars 9.91k forks source link

Consider IStartup alternative #11050

Open Tratcher opened 5 years ago

Tratcher commented 5 years ago

IStartup isn't going to work with generic host which is the default in 3.0. It also conflicts with a few other patterns like ConfigureTestServices.

See https://github.com/aspnet/AspNetCore/issues/11001

@davidfowl

davidfowl commented 4 years ago

Is the codefix to warn about the use of IStartup and turn it into IStartupConvention? That's not already a type, is it?

The code fix is the warn about malformed or missing Configure (or malformed ConfigureServices).

arialdomartini commented 4 years ago

@SetTrend

The Configure() method currently accepts additional parameters, fed by DI. It would be hard to put that into an interface, I believe.

Ideally, the additional parameters would be injected through the constructor, which luckily is not part of the interface.

SetTrend commented 4 years ago

@arialdomartini:

Sounds a good idea to me. And that's absolutely what I would suggest striving for, too.

However, there is a subtlety that needs to be dealt with: some of the objects initialized in Configure() may require arguments provided by services, themselves being provided by dependency injection.

For the sake of this discussion let's assume there are two kinds of services: (a) system provided, and (b) custom provided.

System provided services may be injected with the constructor. Custom provided services currently are being initialized in ConfigureServices().

That's the current approach as it is today.

Supposed you switch to using the interface as suggested by @davidfowl (and I sincerely hope you do), this would cause a conceptual change:

  1. System provided services would be fed to the constructor and need to be stored in local fields.
  2. Custom provided services would be added to the IServiceCollection and be stored in local fields, too.

Then all required services would be available in Configure()*) through local fields.

A sample implementation then would look like this:

internal class WebConfiguration : IStartupConvention
{
  private readonly IConfiguration _configuration;
  private readonly ILogger _logger;
  private WebHostBuilderContext _context;
  private Options _options;

  internal WebConfiguration(IConfiguration configuration, ILogger logger)
  {
    _configuration = configuration;
    _logger = logger;
  }

  public void ConfigureServices(IServiceCollection services, WebHostBuilderContext context)
  {
    _context = context;

    services
      .AddSingleton(_options = _configuration.GetSection("AppSettings").Get<Options>())
      .AddControllers()
      ;
  }

  public void Configure(IApplicationBuilder app)
  {
    if (_context.HostingEnvironment.IsDevelopment()) app.UseDeveloperExceptionPage();

    DbContext.ConnectionString = Regex.Replace(_options.DbConStr, @"\|DataDirectory\|\\?", Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) + @"\");

    app
      .UseHttpsRedirection()
      .UseRouting()
      .UseAuthorization()
      .UseEndpoints(endpoints => endpoints.MapControllers())
      ;
  }
}


*) I'd like to stress again that Configure, just like Startup, is an unspecific name, not giving meaning to the purpose of the method and the class itself.

It's a no-go, according to Microsoft naming guidelines.

I strongly advice to overhaul the naming.


Instead of shoving private fields back and forth in such a class — wouldn't it make sense, be much easier and more streamlined to just put all of it into one single function (defined by interface), i.e. omit the constructor and omit a second function? IServiceCollection would require a generic T Get<T>() method then, that's all.

If required, the programmer then may split that function in whatever sub functions (s)he believes is appropriate.

Such interface and corresponding sample class may look like this:

interface IWebAppConfiguration
{
  void ConfigureApp(IServiceCollection services, IApplicationBuilder app, WebHostBuilderContext context);
}
internal class WebAppConfiguration : IWebAppConfiguration
{
  public void ConfigureApp(IServiceCollection services, IApplicationBuilder app, WebHostBuilderContext context)
  {
    Options options;

    services
      .AddSingleton(options = services.Get<IConfiguration>().GetSection("AppSettings").Get<Options>())
      .AddControllers()
      ;

    DbContext.ConnectionString = Regex.Replace(options.DbConStr, @"\|DataDirectory\|\\?", Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) + @"\");

    if (context.HostingEnvironment.IsDevelopment()) app.UseDeveloperExceptionPage();

    app
      .UseHttpsRedirection()
      .UseRouting()
      .UseAuthorization()
      .UseEndpoints(endpoints => endpoints.MapControllers())
      ;
  }
}
davidfowl commented 4 years ago

Ideally, the additional parameters would be injected through the constructor, which luckily is not part of the interface.

What parameters would those be?

I have a feeling that based on the suggestions made, there's a fundamental disconnect and understanding on how Startup works. One that I have expressed in this thread several times but seems to have gotten lost in the details. Startup cannot support injected arguments that are not already materialized outside of host's dependency injection context. It can't because the container hasn't been built yet. The DI container has 2 phases:

  1. Add services - during this phase services cannot be resolved. (this is why you see people calling BuildServiceProvider and why we now warn by default when you do this in Startup)
  2. Build container - during this phase the container is built and services can be resolved

This is why we have 2 methods, and this is why the phases exist. Hopefully that explains why things are split.

@SetTrend I'd like to make it clear that there's low to no appetite to change anything as drastically as you have specified it here (in fact what you suggest doesn't even work practically but I rather not get into it):

Let me try to explain the reasoning behind this:

The next big change to the startup experience will solve a bunch of these problems all at once and not introduce new changes without huge benefits.

SetTrend commented 4 years ago

You don't consider a number of blog posts written immutable and more important that robustness, testability and program design, do you? This is about the next generation of ASP.NET. SemVer: "MAJOR version when you make incompatible API changes".

  • It suddenly increases the number of ways to do things.

What in particular does it increase?

  • It increases the concept count

Yes, from bad to good. The current concept is based on runtime-typing and otherwise typeless reflection. That's not a concept, that's a design flaw. If runtime typing was such a great feature, then why would they have invented TypeScript over JavaScript?

  • It clashes with our existing concept of configuration

So what? Just take a deep breath and improve.

It doesn't improve the authoring experience

What makes you believe that compile time type checking wouldn't improve programming experience? Did you ask a DevOp manager about this?

  • It doesn't improve any of the harder problems that exist around the Startup class like:

    • Abstracting pieces of startup away (Composing bigger startup classes from smaller pieces)

lt doesn't keep you from doing this, either.

- Not being async (in fact this makes it worse)

Gee ... So go on and make it async. The current version isn't async, either.

jzabroski commented 4 years ago
  • It doesn't improve any of the harder problems that exist around the Startup class like:
    • Abstracting pieces of startup away (Composing bigger startup classes from smaller pieces) -- @davidfowl

lt doesn't keep you from doing this, either. @SetTrend

Actually, when I think of composing bigger startup classes from smaller pieces, I think of F# Giraffe web framework, which is built on top of ASP.NET but uses F# pipelines to hide much of the ASP.NET Startup glue. It's actually not even about composing bigger startup classes from smaller pieces, it's about describing the pieces as pure computations with no side effects, so that there is a top-down explanation of the API and behavior that can be stepped through without reflection. Instead, Giraffe uses an HttpHandler that is a composition of functions similar to what is provided by IApplicationBuilder (and, in fact, to bootstrap itself, Giraffe uses IApplicationBuilder). Below is an example, originally taken from Scott Hanselman but his blog post example is also featured on the Giraffe github page :

open Giraffe.HttpHandlers
open Giraffe.Middleware

let webApp =
    choose [
        route "/ping"   >=> text "pong"
        route "/"       >=> htmlFile "/pages/index.html" ]

type Startup() =
    member __.Configure (app : IApplicationBuilder)
                        (env : IHostingEnvironment)
                        (loggerFactory : ILoggerFactory) =

        app.UseGiraffe webApp // THIS IS IT.  This is basically the whole configuration.

And, if you scroll down further on their page, they describe a F# Giraffe program that doesn't even use a Startup class.

open System
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Microsoft.Extensions.DependencyInjection
open Giraffe

let webApp =
    choose [
        route "/ping"   >=> text "pong"
        route "/"       >=> htmlFile "/pages/index.html" ]

let configureApp (app : IApplicationBuilder) =
    // Add Giraffe to the ASP.NET Core pipeline
    app.UseGiraffe webApp

let configureServices (services : IServiceCollection) =
    // Add Giraffe dependencies
    services.AddGiraffe() |> ignore

[<EntryPoint>]
let main _ =
    Host.CreateDefaultBuilder()
        .ConfigureWebHostDefaults(
            fun webHostBuilder ->
                webHostBuilder
                    .Configure(configureApp)
                    .ConfigureServices(configureServices)
                    |> ignore)
        .Build()
        .Run()
    0
davidfowl commented 4 years ago

@jzabroski I don't see how this fixes the problems I'm talking about but F# syntax is succinct 😄

davidfowl commented 4 years ago

@SetTrend maybe it wasn't clear but I was explaining that the next turn of the crank for startup will tackle all of those problems.

jzabroski commented 4 years ago

I think I would need specific examples of the problem, then, because the F# approach does solve several problems with structural composition (the composition is also not free; the trade-off is someone has to stitch together manually the whole pipeline). At the same time, you wrote the framework, so where I see problems you may see "RTFM" :-). For example, one I did not mention above, because I wanted to be brief, is that through using a Hindley-Milner type system, you can write code that manipulates an existential type through a universally quantified function. One particular use case for this technical concept is the question of how you can write open generic controllers that can then be "interpreted" by other parts of the framework, like a Swagger endpoint. In ASP.NET Core Swagger, if you try to UseSwagger() on a WebApi project with open generic controllers (like an open generic SearchController<T>), then you cannot just "plug-in Swagger". In this sense, many "web parts" in ASP.NET Core are monolithic since they do not allow you to create abstract interpretations of the execution and use those abstract interpretations in other places of the program. But all Swagger is doing is "Create a new HttpHandler that returns html from an HttpHandler that accepts/returns json."

Pilchie commented 3 years ago

Moving to Backlog to consider for 6.0 at this point.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.