Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable dependency injection for IServiceRouteMapper #4155

Merged
merged 1 commit into from
Oct 13, 2020
Merged

Enable dependency injection for IServiceRouteMapper #4155

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions DNN Platform/DotNetNuke.Web/Api/Internal/ServicesRoutingManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information
namespace DotNetNuke.Web.Api.Internal
{
using System;
Expand All @@ -23,6 +22,8 @@ namespace DotNetNuke.Web.Api.Internal
using DotNetNuke.Web.Api.Internal.Auth;
using DotNetNuke.Web.ConfigSection;

using Microsoft.Extensions.DependencyInjection;

public sealed class ServicesRoutingManager : IMapRoute
{
private static readonly ILog Logger = LoggerSource.Instance.GetLogger(typeof(ServicesRoutingManager));
Expand Down Expand Up @@ -225,16 +226,18 @@ private void LocateServicesAndMapRoutes()
this.ClearCachedRouteData();

this._moduleUsage.Clear();
foreach (IServiceRouteMapper routeMapper in this.GetServiceRouteMappers())
using (var serviceScope = Globals.DependencyProvider.CreateScope())
{
try
{
routeMapper.RegisterRoutes(this);
}
catch (Exception e)
foreach (IServiceRouteMapper routeMapper in this.GetServiceRouteMappers(serviceScope.ServiceProvider))
{
Logger.ErrorFormat("{0}.RegisterRoutes threw an exception. {1}\r\n{2}", routeMapper.GetType().FullName,
e.Message, e.StackTrace);
try
{
routeMapper.RegisterRoutes(this);
}
catch (Exception e)
{
Logger.ErrorFormat("{0}.RegisterRoutes threw an exception. {1}\r\n{2}", routeMapper.GetType().FullName, e.Message, e.StackTrace);
}
}
}
}
Expand All @@ -249,7 +252,7 @@ private void RegisterSystemRoutes()
// _routes.IgnoreRoute("{resource}.axd/{*pathInfo}");
}

private IEnumerable<IServiceRouteMapper> GetServiceRouteMappers()
private IEnumerable<IServiceRouteMapper> GetServiceRouteMappers(IServiceProvider serviceProvider)
{
IEnumerable<Type> types = this.GetAllServiceRouteMapperTypes();

Expand All @@ -258,12 +261,11 @@ private IEnumerable<IServiceRouteMapper> GetServiceRouteMappers()
IServiceRouteMapper routeMapper;
try
{
routeMapper = Activator.CreateInstance(routeMapperType) as IServiceRouteMapper;
routeMapper = ActivatorUtilities.CreateInstance(serviceProvider, routeMapperType) as IServiceRouteMapper;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like adding the ActivatorUtlities here it enables us to use either reflection or the container. I was wondering if it made sense to register all implementations of IServiceRouteMapper as a transient service. That way we can just depend on the container and it would never use reflection here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had been thinking about that (especially with some of the trickier scenarios I looked at in #3933). In this case, we're only instantiating these instances one time on startup, so it doesn't seem to me like it'd be worth it. But definitely a route we can look at for other extension points that are used multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, the only scenario I can think of which would make sense would be route localization. If you want to inject a localization service to the IServiceRouteMapper which would handle generating localized routes.

The developer could still just register the mapper in their own implementation of IDnnStartup.

}
catch (Exception e)
{
Logger.ErrorFormat("Unable to create {0} while registering service routes. {1}", routeMapperType.FullName,
e.Message);
Logger.ErrorFormat("Unable to create {0} while registering service routes. {1}", routeMapperType.FullName, e.Message);
routeMapper = null;
}

Expand Down