Orchardcore: IMvcCoreBuilder not accessible from modules

[jrestall]

Hi guys,

I'm creating a Swagger module for my site built on the Orchard Core Framework. This requires me to register the ApiExplorer that normally is added by app.AddMvc(), but since we use app.AddMvcCore() I have to add it manually.

That's fine but the only thing I find a bit messy is that I can't access the instance of IMvcCoreBuilder that is created in Orchard.Mvc.ModularServiceCollectionExtensions.AddMvcModules().

So my module's Startup class looks like this...

...
public override void ConfigureServices(IServiceCollection services)
{
        // We are using the AddMvcCore for a more paired-down MVC stack, so need to explicitly add the Api Explorer services
        new MvcCoreBuilder(services, null).AddVersionedApiExplorer();

        // Register the Swagger generator, defining one or more Swagger documents
        services.AddSwaggerGen(c =>
        {
           ...
}

Seems a bit messy to initialize a new internal MvcCoreBuilder to me. I'm using the new Versioned API Explorer .

FYI I believe it should be possible to call AddMvcCore again such as,
services.AddMvcCore().AddVersionedApiExplorer();

But when i try that i get issues with a duplicate 'exists' route constraint being added from the internal Mvc framework.

ArgumentException: An item with the same key has already been added. Key: exists
System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(object key)
System.Collections.Generic.Dictionary.Insert(TKey key, TValue value, bool add)
Microsoft.Extensions.Options.OptionsCache.CreateOptions()
System.Threading.LazyInitializer.EnsureInitializedCore<T>(ref T target, ref bool initialized, ref object syncLock, Func<T> valueFactory)
Microsoft.Extensions.Options.OptionsCache.get_Value()
Microsoft.AspNetCore.Routing.DefaultInlineConstraintResolver..ctor(IOptions<RouteOptions> routeOptions)
System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProvider provider)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitTransient(TransientCallSite transientCallSite, ServiceProvider provider)
Microsoft.Extensions.DependencyInjection.ServiceProvider+<>c__DisplayClass16_0.<RealizeService>b__0(ServiceProvider provider)
Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService<T>(IServiceProvider provider)
Microsoft.AspNetCore.Builder.MapRouteRouteBuilderExtensions.MapRoute(IRouteBuilder routeBuilder, string name, string template, object defaults, object constraints, object dataTokens)
Microsoft.AspNetCore.Builder.MvcAreaRouteBuilderExtensions.MapAreaRoute(IRouteBuilder routeBuilder, string name, string areaName, string template, object defaults, object constraints, object dataTokens)
Microsoft.AspNetCore.Builder.MvcAreaRouteBuilderExtensions.MapAreaRoute(IRouteBuilder routeBuilder, string name, string areaName, string template, object defaults)
Company.Admin.Startup.Configure(IApplicationBuilder builder, IRouteBuilder routes, IServiceProvider serviceProvider) in Startup.cs
+
            routes.MapAreaRoute(
Microsoft.AspNetCore.Modules.ModularTenantRouterMiddleware.BuildTenantPipeline(ShellSettings shellSettings, IServiceProvider serviceProvider)
Microsoft.AspNetCore.Modules.ModularTenantRouterMiddleware+<Invoke>d__4.MoveNext()

To the best of my ability I think this occurs because Microsoft.AspNetCore.Mvc.Internal.MvcCoreRouteOptionsSetup is doing this twice:

public static void ConfigureRouting(RouteOptions options)
{
      options.ConstraintMap.Add("exists", typeof(KnownRouteValueConstraint));
}

This I believe is because the normal checks for stopping services from being registered twice in AddMvcCore (services.TryAddEnumerable) are being bypassed, since in Orchard we create a new ServicesCollection for every module's Startup class. (See ShellContainerFactory) So if two modules call AddMvcCore the framework will add multiple route constraints since the assumption that there is a single service collection is no longer true.

The troublesome lines:

Line 97: ShellContainerFactory
startup.ConfigureServices(startupServices);
tenantServiceCollection.Add(startupServices);

Anyway thought I'd document my journey, even though I have a workaround, if anyone else sees value in exposing IMvcCoreBuilder to module's startup classes or thinks there's value in allowing multiple calls to AddMvcCore I'm happy to help resolve.

[Jetski5822]

Hey @jrestall I agree, the extension points to this subsystem could be better, and thanks for your write up.

I will take a look and see if we cant make this area more extensible.

[jrestall]

Thanks @Jetski5822 , it turns out my workaround doesn't work anyway so I'm going to need to figure out a way to allow multiple calls to AddMvcCore after all. I don't think an extension point to access IMvcCoreBuilder will be enough since 3rd party libraries such as Microsoft.AspNetCore.Mvc.Versioning call AddMvcCore again internally.

In my case adding a call to app.AddApiVersioning() caused the same issue with constraints being added twice due to the package's code:

public static IServiceCollection AddApiVersioning(this IServiceCollection services, Action<ApiVersioningOptions> setupAction)
{
services.AddMvcCore((Action<MvcOptions>) (mvcOptions =>
      {
        mvcOptions.Conventions.Add((IApplicationModelConvention) new ApiVersionConvention(options.DefaultApiVersion, options.Conventions));
      }));

If we want to support libraries such as Microsoft.AspNetCore.Mvc.Versioning and likely others, we will have to look at how ShellContainerFactory is re-creating service collections for each Startup class.

So to be clear what I'm asking is that putting a call such as the below in a Startup class would just work:

services.AddMvcCore().AddVersionedApiExplorer();

[jrestall]

I'm currently thinking something along these lines as a fix, haven't tried it though, will hopefully have time to test and submit a pull request soon. EDIT: Pull Request #843 Created.

ShellContainerFactory, Line 85.

            foreach (var startup in moduleServiceProvider.GetServices<Microsoft.AspNetCore.Modules.IStartup>())
            {
                ...

                if (!featureServiceCollections.TryGetValue(feature, out featureServiceCollection))
                {
                    featureServiceCollections.Add(feature, featureServiceCollection = new ServiceCollection());
                }

                int previousTenantServiceCount = tenantServiceCollection.Count;
                startup.ConfigureServices(tenantServiceCollection);

                // Determine and store any new services the startup class has added for this feature 
                var newTenantServices = tenantServiceCollection.Skip(previousTenantServiceCount);
                featureServiceCollection.Add(newTenantServices);
            }

Basically instead of passing a new collection into each startup class, I pass the same tenant collection of services. To determine the feature's services I just pull out the newly added items.

[Jetski5822]

Hey @jrestall heres my Swagger module for O2 btw... https://github.com/Jetski5822/ECom/blob/master/src/ECom.Modules/ECom.Swagger/Startup.cs

[jrestall]

Thanks @Jetski5822, the code for including XML comments is useful.

I'm really happy with how much easier it is to include swagger with Orchard in V2 as compared with V1. I remember having to do some serious hacks to get API Explorer to find the correct shell routes previously.

[sfmskywalker]

@jrestall did you ever get AddApiVersioning to work? When I try, it gets added successfully, but all of my controllers suddenly become inaccessible - I get a 404 no matter what URL I request.

[jrestall]

Hi @sfmskywalker, I did have it working with a previous ApiVersioning package.

I had a look for you against the latest version and did find it no longer works. After some digging it seems in June 2017 they started using the IStartupFilter interface to register their version route policy into the pipeline.

Since Orchard doesn't support IStartupFilter when building the middleware pipelines for each tenant, an important piece of the api versioning library never gets injected into the request pipeline and you get 404's.

The fix is to duplicate what Microsoft.AspNetCore.Hosting does when building their middleware pipeline within our ModularTenantRouterMiddleware.cs class (line 114):

...

var startupFilters = serviceProvider.GetService<IEnumerable<IStartupFilter>>();
Action<IApplicationBuilder> configure = _ => {}; //TODO: Ordering is important.
foreach (var filter in startupFilters.Reverse())
{
    configure = filter.Configure(configure);
}

configure(appBuilder);

var pipeline = appBuilder.Build();

return pipeline;

After making this change the latest Microsoft.AspNetCore.Mvc.Versioning works again.

I would suggest before making this change to Orchard we would want to look into the side effects of having all the startup filters now running for each tenant pipeline. There were 2 different startup filters (not including the api versioning filter) in my testing:

• AutoRequestServicesStartupFilter - Handles automatic creation and disposal of dependencies.

• DataProtectionStartupFilter - There are two of these registered in my testing. Why? Seems harmless though.

I also didn't confirm the new code is ordering the pipeline correctly.

Btw if you are still having issues with routes after this change, try set AssumeDefaultVersionWhenUnspecified to true when adding versioning.

services.AddApiVersioning(options => options.AssumeDefaultVersionWhenUnspecified = true);

[Jetski5822]

Yeah we need this

[jrestall]

Hi @Jetski5822, I have good news then! Sébastien just merged my pull request that adds IStartupFilter support https://github.com/OrchardCMS/OrchardCore/commit/fa4ac675fdeabbfcf6d92841f44588cae15065a1.

I'm happy to close this issue now as we've fixed both the ability for libraries to call AddMvcCore multiple times and added support for libraries to initialize themselves via IStartupFilter e.g. Microsoft.AspNetCore.Mvc.Versioning.

Let me know if you disagree and we can re-open or we can create a new more specific issue.

[sfmskywalker]

@jrestall Thanks mate, your solution saved the day!