aspnet / Hosting

[Archived] Code for hosting and starting up an ASP.NET Core application. Project moved to https://github.com/aspnet/Extensions and https://github.com/aspnet/AspNetCore
Apache License 2.0
552 stars 312 forks source link

Search in more locations for Startup class #254

Closed PonchoPowers closed 9 years ago

PonchoPowers commented 9 years ago

We need the ability to specify the Startup type we want to use as in our case the Startup type we want to use resides in a referenced dependency project.

Our preferred solution is to extend the FindStartupType method so that just before the type is returned, a search of all assemblies is performed to look for a startup type, we think that this solution will be of the most benefit to a wider community.

A search of Startup types could be cached during initial compilation in a similar way to how the ControllerTypeCache performs a search for controllers and caches the result in the MVC stack.

Use case scenario A sample use case scenario is a solution which takes on the following structure: Project 1: Library of classes

Project 2: Scaffold of website Depends on Project 1 (This will contain a Startup class)

Project 3: Website of models, views and controllers Depends on Project 1 and Project 2

Project 4: Scaffold of administration website Depends on Project 1 and Project 2 (This will contain a Startup class)

Project 5: Administration website for Project 3 Depends on Project 1, Project 2 and Project 3

This structure allows for the scaffold of a website or administration website to be reused by multiple websites and administration websites over and over again allowing for a rapid development while allowing for changes to be made to the scaffold which will be applied to the websites using the scaffolds.

In the scenario of Project 4 depending on Project 2 which already contains a Startup class, because Project 4 contains a Startup class the Startup class in Project 2 is not loaded.

Code example

Project 2 ~/Startup.cs

using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Hosting;
using Microsoft.AspNet.Mvc;
using Microsoft.Framework.DependencyInjection;
using Microsoft.Framework.Logging;

namespace Sapphire.Cms
{
    public class Startup
    {
        public virtual void ConfigureServices(IServiceCollection services)
        {
            services.AddMvc();
        }

        public virtual void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerfactory)
        {
            app.UseMvc(routes =>
            {
                routes.MapRoute(
                    name: "default",
                    template: "{controller=Home}/{action=Index}/{id?}"
                );
            });
        }
    }
}

Project 3 ~/Startup.cs

namespace New606
{
    public class Startup : Sapphire.Cms.Startup
    {
    }
}

As you can see since Project 2 does everything required, we are currently left having to create a class that serves no purpose.

Backwards compatibility In ASP.NET 4.5 we had the option of specifying the owin:appStartup appSetting key which somewhat mitigated this issue, but this is no longer an option. Since ASP.NET 5 is supposed to be a progression of ASP.NET 4.5 it would be nice to see the search in all assemblies implemented as mentioned at the start.

Alternative solutions Specify the Startup class in a config file As in previous versions where we could specify the Startup class in a web.config file, we could allow for a similar implementation.

We dislike this solution as it requires extra configuring which is unnecessary when you consider the initial solution proposed.

Specify the Startup class using an attribute As in previous versions where we could specify the Startup class using an attribute, we could allow for a similar implementation.

We dislike this solution in the same way we dislike the config file solution as it requires extra configuring which is unnecessary when you consider the initial solution proposed.

davidfowl commented 9 years ago

Our preferred solution is to extend the FindStartupType method so that just before the type is returned, a search of all assemblies is performed to look for a startup type, we think that this solution will be of the most benefit to a wider community.

I can tell you now, if it's one thing we have no plans on doing, its scanning all assemblies. That was one of the things we profiled extensively on the old stack and promised that we would avoid in this new one (it's just so terrible). An option to scan all referenced assemblies (which is a huge set of dependencies for CoreCLR) would have to be something completely opt in via some level of configuration.

PonchoPowers commented 9 years ago

You already scan all assemblies for controllers via an internal class, so can't the scanning of assemblies be moved from the MVC stack into a standalone operation with events so that there is still only 1 scan but we have the option to hook into it as I'm sure there are other uses for scanning all assemblies, such as fixing the inconsistency of scanning all assemblies for controllers, but only the currently executing assembly for views.

davidfowl commented 9 years ago

No we don't

davidfowl commented 9 years ago

We scan assemblies referencing mvc

PonchoPowers commented 9 years ago

My apologies, a lot has changed, the MVC stack used to scan all assemblies.

Surely then we could use an attribute of [Startup] to mark the class as a Startup class and then search assemblies referencing the namespace the attribute class resides in for a Startup class?

PonchoPowers commented 9 years ago

I'm worried that this is being pushed back at a time when our Startup class just keeps growing in size. I've already gave a compelling reason as to why this should be implemented along with discussing the implementation however unfortauntely I'm not allowed to sigh the SLA due to my employment so it is pointless me sending a pull request, therefore can someone who can sign the SLA please submit a pull request for this feature request.

Tragetaschen commented 9 years ago

I'm a bit confused how much implementation and documentation work would be required to remove one empty class that's trivial to implement and doesn't need any maintenance. It sounds like you want to move five lines of code (where one line contains the important stuff) into one line of configuration code.

It's likely that I'm missing something, but what I understand now doesn't sound like enough bang for the bucks.

PonchoPowers commented 9 years ago

You are both right and wrong, you are right in that this relates to 5 lines of code but wrong in that it isn't about moving these lines, it is about removing these lines completely as you shouldn't have to extend a class in order for the code to be executed.

I'm trying to achieve the following:

The existing functionality remains the same, it is just being extended so that if the Startup class is not found within the web application itself the other assemblies are searched for a Startup class.

The required documentation to specify this addition in minimal and the code that requires implementing could be similar to the MVC code for searching for controllers so again the time invested should be fairly minimal, which is in contrast a hugh benefit to not having to implement a Startup class in every web application project.

Tragetaschen commented 9 years ago

Ok, everything you require is trivially solved by deriving from or calling into an existing Startup class using the main project's Startup class without any need for extending Hosting. The only reason you gave for not doing this seems to be because you don't want to.

I'm not writing this to dismiss your request. I'm just someone that could write the PR, but I don't see enough value in your request.

PonchoPowers commented 9 years ago

The problem isn't that I don't want to, the problem is that no matter which way we do this it is incorrect.

I've already explained why extending a class is wrong, but also consider the following:

class Startup {
    public void ConfigureServices(IServiceCollection services)
    {
        BaseStartup.ConfigureServices(services);
    }
    public void ConfigureDevelopment(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        BaseStartup.ConfigureDevelopment(app, loggerFactory);
    }
    public void ConfigureStaging(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        BaseStartup.ConfigureStaging(app, loggerFactory);
    }
    public void ConfigureProduction(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        BaseStartup.ConfigureProduction(app, loggerFactory);
    }
    public void Configure(IApplicationBuilder app)
    {
        BaseStartup.Configure(app);
    }
}

This is just as stupid as extending the class, yet at present this is what we are forced to do.

davidfowl commented 9 years ago

I'm sorry I can't get over the scan. We just don't scan things by default anymore and it's not just a decision we made on a whim, it's just bad scanning all loaded assemblies this early in the pipeline. I absolutely think the majority of users will have the startup class defined in the main assembly (similar to how they have their Program.Main in the assembly they are starting).

Allowing you specify the startup class seems like a fair compromise to me.

PonchoPowers commented 9 years ago

I've already said we don't need to scan all assemblies, we could introduce an attribute and then we search for the namespace in which that attribute exists to see if there is a Startup class in that assembly.

davidfowl commented 9 years ago

I'm thinking we just let you specify the startup class either in web.config (for helios) or Microsoft.AspNet.Hosting.json/ini or the command line arguments.

PonchoPowers commented 9 years ago

Please, that would be nice as we currently use the web.config file to specify the startup class in our ASP.NET 4.5 projects.

Tratcher commented 9 years ago

@Matthew-Bonner You can already specify the assembly containing your Startup class via config. The key is Hosting:Application. For sites running in IIS/Express you can specify this as an environment variable or in a Microsoft.AspNet.Hosting.ini file in the root of your project directory.

https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNet.Hosting/WebHostBuilder.cs#L24 https://github.com/aspnet/Configuration/blob/dev/src/Microsoft.Framework.Configuration.Ini/IniFileConfigurationSource.cs#L15