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

Reduce constructor injection in controllers when possible #15473

Closed
wants to merge 3 commits into from
Closed

Reduce constructor injection in controllers when possible #15473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f0b4b5
Reduce constructor injection in controllers when possible
MikeAlhayek Mar 7, 2024
d2bb3a9
fix buil
MikeAlhayek Mar 7, 2024
c4da2c4
Merge branch 'main' into ma/cleanup-controllers
MikeAlhayek Apr 1, 2024
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
9 changes: 3 additions & 6 deletions src/OrchardCore.Modules/OrchardCore.AdminDashboard/Controllers/DashboardController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@
namespace OrchardCore.AdminDashboard.Controllers
{
[Admin]
public class DashboardController : Controller
public class DashboardController : Controller, IUpdateModel
{
private readonly IAuthorizationService _authorizationService;
private readonly IAdminDashboardService _adminDashboardService;
private readonly IContentManager _contentManager;
private readonly IContentItemDisplayManager _contentItemDisplayManager;
private readonly IContentDefinitionManager _contentDefinitionManager;
private readonly IUpdateModelAccessor _updateModelAccessor;
Copy link
Member

Choose a reason for hiding this comment

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

lolz, this was originally done to make it replacable (from memory), so suspect you are breaking something for someone here.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this was the case, it would be replaced in a test case. I don't think we are replacing it from a test so it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

haha. you're dreaming if you think the tests cover everything :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not what I meant. We have a filter that changes the modelBinderAccessor.ModelUpdater used on the controller which reads it from the services collection. So if someone want to provider their own implementation of IUpdateModelAccessor the updated code will read their custom implementation and use. So it should not break anyone.

OrchardCore/src/OrchardCore/OrchardCore.DisplayManagement/ModelBinding/ModelBinderAccessorFilter.cs

Lines 19 to 20 in d99deaa

var modelBinderAccessor = context.HttpContext.RequestServices.GetRequiredService<IUpdateModelAccessor>();
modelBinderAccessor.ModelUpdater = new ControllerModelUpdater(controller);

Copy link
Member Author

Choose a reason for hiding this comment

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

@deanmarcussen do you still think there is an issue with this?

Copy link
Member

Choose a reason for hiding this comment

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

Where resolved, do you mean? In the example of this controller, IContentItemDisplayManager and the drivers down the line will use the instance of the controller as IUpdateModel. You could previously override that via IUpdateModelAccessor.ModelUpdater but not anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Piedone look at the comment above #15473 (comment)

The same instance is already set at every controller from the filter. So if you resolve that updater directly from the controller or you resolver it from the container, you'll get the same instance. At the end of the day, they all resolve the instance that you registered in the container.

Copy link
Member

Choose a reason for hiding this comment

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

On line 75 the controller instance is passed to _contentItemDisplayManager.BuildDisplayAsync(). Due to this, it doesn't matter what's in IUpdateModelAccessor.ModelUpdater, display management will use the controller.

Before this change, the (current) controller being used as IUpdateModel was one option that was the default, set by the filter you mention. Now it's the only option, and even if you set ``IUpdateModelAccessor.ModelUpdater` to something else, still, the controller will be used. That's why I'm saying it's a capability lost (whether it's a useful capability is a different question, and I'm not sure).

Since almost all of the cases where the current IUpdateModel instance is accessed are in drivers, and thus coming similarly from controllers (there are a handful of cases of IUpdateModelAccessor being used in services and views), this PR effectively removes the ability to use anything else than the current controller for IUpdateModel.

Again, I'm not sure if this is a bad thing, but it's an impactful one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at this more, there is no way to override the update logic that asp.net core. So the idea of changing the implementation is not ideal because even if you do, you can't change the updater behavior that is in the BasecController. I am wondering if we really need to care about allowing others to change the behavior of IUpdateModel since that will only change the logic partially "not for the methods found in the controller".

Copy link
Member Author

Choose a reason for hiding this comment

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

@sebastienros thought here? Do we really need or should care about injecting IUpdateModelAccessor into controllers where there is no pure way to change the updater behavior in the app entirely even when we want to?

private readonly YesSql.ISession _session;

public DashboardController(
Expand All @@ -35,15 +34,13 @@ public DashboardController(
IContentManager contentManager,
IContentItemDisplayManager contentItemDisplayManager,
IContentDefinitionManager contentDefinitionManager,
IUpdateModelAccessor updateModelAccessor,
YesSql.ISession session)
{
_authorizationService = authorizationService;
_adminDashboardService = adminDashboardService;
_contentManager = contentManager;
_contentItemDisplayManager = contentItemDisplayManager;
_contentDefinitionManager = contentDefinitionManager;
_updateModelAccessor = updateModelAccessor;
_session = session;
}

Expand Down Expand Up @@ -75,7 +72,7 @@ public async Task<IActionResult> Index()
wrappers.Add(new DashboardWrapper
{
Dashboard = widget,
Content = await _contentItemDisplayManager.BuildDisplayAsync(widget, _updateModelAccessor.ModelUpdater, "DetailAdmin")
Content = await _contentItemDisplayManager.BuildDisplayAsync(widget, this, "DetailAdmin")
});
}

Expand Down Expand Up @@ -127,7 +124,7 @@ public async Task<IActionResult> Manage()
var wrapper = new DashboardWrapper
{
Dashboard = widget,
Content = await _contentItemDisplayManager.BuildDisplayAsync(widget, _updateModelAccessor.ModelUpdater, "DetailAdmin")
Content = await _contentItemDisplayManager.BuildDisplayAsync(widget, this, "DetailAdmin")
};

wrappers.Add(wrapper);
Expand Down
24 changes: 10 additions & 14 deletions src/OrchardCore.Modules/OrchardCore.AdminMenu/Controllers/MenuController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,42 +27,38 @@ public class MenuController : Controller

private readonly IAuthorizationService _authorizationService;
private readonly IAdminMenuService _adminMenuService;
private readonly PagerOptions _pagerOptions;
private readonly IShapeFactory _shapeFactory;
private readonly INotifier _notifier;
private readonly ILogger _logger;

protected readonly IStringLocalizer S;
protected readonly IHtmlLocalizer H;

public MenuController(
IAuthorizationService authorizationService,
IAdminMenuService adminMenuService,
IOptions<PagerOptions> pagerOptions,
IShapeFactory shapeFactory,
INotifier notifier,
IStringLocalizer<MenuController> stringLocalizer,
IHtmlLocalizer<MenuController> htmlLocalizer,
ILogger<MenuController> logger)
IHtmlLocalizer<MenuController> htmlLocalizer)
{
_authorizationService = authorizationService;
_adminMenuService = adminMenuService;
_pagerOptions = pagerOptions.Value;
_shapeFactory = shapeFactory;
_notifier = notifier;
S = stringLocalizer;
H = htmlLocalizer;
_logger = logger;
}

public async Task<IActionResult> List(ContentOptions options, PagerParameters pagerParameters)
public async Task<IActionResult> List(
[FromServices] IOptions<PagerOptions> pagerOptions,
[FromServices] IShapeFactory shapeFactory,
[FromServices] ILogger<MenuController> logger,
ContentOptions options,
PagerParameters pagerParameters)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageAdminMenu))
{
return Forbid();
}

var pager = new Pager(pagerParameters, _pagerOptions.GetPageSize());
var pager = new Pager(pagerParameters, pagerOptions.Value.GetPageSize());

var adminMenuList = (await _adminMenuService.GetAdminMenuListAsync()).AdminMenu;

Expand All @@ -86,7 +82,7 @@ public async Task<IActionResult> List(ContentOptions options, PagerParameters pa
}
catch (Exception ex)
{
_logger.LogError(ex, "Error when retrieving the list of admin menus.");
logger.LogError(ex, "Error when retrieving the list of admin menus.");
await _notifier.ErrorAsync(H["Error when retrieving the list of admin menus."]);
}

Expand All @@ -98,7 +94,7 @@ public async Task<IActionResult> List(ContentOptions options, PagerParameters pa
routeData.Values.TryAdd(_optionsSearch, options.Search);
}

var pagerShape = await _shapeFactory.PagerAsync(pager, adminMenuList.Count, routeData);
var pagerShape = await shapeFactory.PagerAsync(pager, adminMenuList.Count, routeData);

var model = new AdminMenuListViewModel
{
Expand Down
15 changes: 6 additions & 9 deletions src/OrchardCore.Modules/OrchardCore.AdminMenu/Controllers/NodeController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,29 @@
namespace OrchardCore.AdminMenu.Controllers
{
[Admin("AdminMenu/Node/{action}", "AdminMenuNode{action}")]
public class NodeController : Controller
public class NodeController : Controller, IUpdateModel
{
private readonly IAuthorizationService _authorizationService;
private readonly IDisplayManager<MenuItem> _displayManager;
private readonly IEnumerable<IAdminNodeProviderFactory> _factories;
private readonly IAdminMenuService _adminMenuService;
private readonly INotifier _notifier;
protected readonly IHtmlLocalizer H;
private readonly IUpdateModelAccessor _updateModelAccessor;

protected readonly IHtmlLocalizer H;

public NodeController(
IAuthorizationService authorizationService,
IDisplayManager<MenuItem> displayManager,
IEnumerable<IAdminNodeProviderFactory> factories,
IAdminMenuService adminMenuService,
IHtmlLocalizer<NodeController> htmlLocalizer,
INotifier notifier,
IUpdateModelAccessor updateModelAccessor)
INotifier notifier)
{
_displayManager = displayManager;
_factories = factories;
_adminMenuService = adminMenuService;
_authorizationService = authorizationService;
_notifier = notifier;
_updateModelAccessor = updateModelAccessor;
H = htmlLocalizer;
}

Expand Down Expand Up @@ -109,7 +106,7 @@ public async Task<IActionResult> Create(string id, string type)
AdminNode = treeNode,
AdminNodeId = treeNode.UniqueId,
AdminNodeType = type,
Editor = await _displayManager.BuildEditorAsync(treeNode, updater: _updateModelAccessor.ModelUpdater, isNew: true, "", "")
Editor = await _displayManager.BuildEditorAsync(treeNode, updater: this, isNew: true, string.Empty, string.Empty)
};

return View(model);
Expand Down Expand Up @@ -187,7 +184,7 @@ public async Task<IActionResult> Edit(string id, string treeNodeId)
AdminNodeType = treeNode.GetType().Name,
Priority = treeNode.Priority,
Position = treeNode.Position,
Editor = await _displayManager.BuildEditorAsync(treeNode, updater: _updateModelAccessor.ModelUpdater, isNew: false, "", "")
Editor = await _displayManager.BuildEditorAsync(treeNode, updater: this, isNew: false, string.Empty, string.Empty)
};

model.Editor.TreeNode = treeNode;
Expand Down Expand Up @@ -218,7 +215,7 @@ public async Task<IActionResult> Edit(AdminNodeEditViewModel model)
return NotFound();
}

var editor = await _displayManager.UpdateEditorAsync(treeNode, updater: _updateModelAccessor.ModelUpdater, isNew: false, "", "");
var editor = await _displayManager.UpdateEditorAsync(treeNode, updater: this, isNew: false, string.Empty, string.Empty);

if (ModelState.IsValid)
{
Expand Down
33 changes: 14 additions & 19 deletions src/OrchardCore.Modules/OrchardCore.AuditTrail/Controllers/AdminController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,33 +18,25 @@

namespace OrchardCore.AuditTrail.Controllers
{
public class AdminController : Controller
public class AdminController : Controller, IUpdateModel
{
private readonly PagerOptions _pagerOptions;
private readonly IShapeFactory _shapeFactory;
private readonly IAuditTrailManager _auditTrailManager;
private readonly IUpdateModelAccessor _updateModelAccessor;
private readonly IAuthorizationService _authorizationService;
private readonly IAuditTrailAdminListQueryService _auditTrailAdminListQueryService;
private readonly IDisplayManager<AuditTrailEvent> _displayManager;
private readonly IDisplayManager<AuditTrailIndexOptions> _auditTrailOptionsDisplayManager;

protected readonly IStringLocalizer S;

public AdminController(
IOptions<PagerOptions> pagerOptions,
IShapeFactory shapeFactory,
IAuditTrailManager auditTrailManager,
IUpdateModelAccessor updateModelAccessor,
IAuthorizationService authorizationService,
IAuditTrailAdminListQueryService auditTrailAdminListQueryService,
IDisplayManager<AuditTrailEvent> displayManager,
IDisplayManager<AuditTrailIndexOptions> auditTrailOptionsDisplayManager,
IStringLocalizer<AdminController> stringLocalizer)
{
_pagerOptions = pagerOptions.Value;
_shapeFactory = shapeFactory;
_auditTrailManager = auditTrailManager;
_updateModelAccessor = updateModelAccessor;
_authorizationService = authorizationService;
_auditTrailAdminListQueryService = auditTrailAdminListQueryService;
_displayManager = displayManager;
Expand All @@ -53,7 +45,10 @@ public AdminController(
}

[Admin("AuditTrail/{correlationId?}", "AuditTrailIndex")]
public async Task<ActionResult> Index([ModelBinder(BinderType = typeof(AuditTrailFilterEngineModelBinder), Name = "q")] QueryFilterResult<AuditTrailEvent> queryFilterResult, PagerParameters pagerParameters, string correlationId = "")
public async Task<ActionResult> Index(
[FromServices] IOptions<PagerOptions> pagerOptions,
[FromServices] IShapeFactory shapeFactory,
[ModelBinder(BinderType = typeof(AuditTrailFilterEngineModelBinder), Name = "q")] QueryFilterResult<AuditTrailEvent> queryFilterResult, PagerParameters pagerParameters, string correlationId = "")
{
if (!await _authorizationService.AuthorizeAsync(User, AuditTrailPermissions.ViewAuditTrail))
{
Expand All @@ -78,7 +73,7 @@ public async Task<ActionResult> Index([ModelBinder(BinderType = typeof(AuditTrai
options.FilterResult.TryAddOrReplace(new CorrelationIdFilterNode(options.CorrelationId));
}

var pager = new Pager(pagerParameters, _pagerOptions.GetPageSize());
var pager = new Pager(pagerParameters, pagerOptions.Value.GetPageSize());

// With the options populated we filter the query, allowing the filters to alter the options.
var result = await _auditTrailAdminListQueryService.QueryAsync(pager.Page, pager.PageSize, options);
Expand All @@ -90,13 +85,13 @@ public async Task<ActionResult> Index([ModelBinder(BinderType = typeof(AuditTrai
// Populate route values to maintain previous route data when generating page links.
options.RouteValues.TryAdd("q", options.FilterResult.ToString());

var pagerShape = await _shapeFactory.PagerAsync(pager, result.TotalCount, options.RouteValues);
var pagerShape = await shapeFactory.PagerAsync(pager, result.TotalCount, options.RouteValues);
var items = new List<IShape>();

foreach (var auditTrailEvent in result.Events)
{
items.Add(
await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: _updateModelAccessor.ModelUpdater, displayType: "SummaryAdmin")
await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: this, displayType: "SummaryAdmin")
);
}

Expand All @@ -106,9 +101,9 @@ await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: _updateModelAc
options.EventsCount = items.Count;
options.TotalItemCount = result.TotalCount;

var header = await _auditTrailOptionsDisplayManager.BuildEditorAsync(options, _updateModelAccessor.ModelUpdater, false, string.Empty, string.Empty);
var header = await _auditTrailOptionsDisplayManager.BuildEditorAsync(options, this, false, string.Empty, string.Empty);

var shapeViewModel = await _shapeFactory.CreateAsync<AuditTrailListViewModel>("AuditTrailAdminList", viewModel =>
var shapeViewModel = await shapeFactory.CreateAsync<AuditTrailListViewModel>("AuditTrailAdminList", viewModel =>
{
viewModel.Events = items;
viewModel.Pager = pagerShape;
Expand All @@ -123,15 +118,15 @@ await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: _updateModelAc
[FormValueRequired("submit.Filter")]
public async Task<ActionResult> IndexFilterPOST(AuditTrailIndexOptions options)
{
await _auditTrailOptionsDisplayManager.UpdateEditorAsync(options, _updateModelAccessor.ModelUpdater, false, string.Empty, string.Empty);
await _auditTrailOptionsDisplayManager.UpdateEditorAsync(options, this, false, string.Empty, string.Empty);
// When the user has typed something into the search input no further evaluation of the form post is required.
if (!string.Equals(options.SearchText, options.OriginalSearchText, StringComparison.OrdinalIgnoreCase))
{
return RedirectToAction(nameof(Index), new RouteValueDictionary { { "q", options.SearchText } });
}

// Evaluate the values provided in the form post and map them to the filter result and route values.
await _auditTrailOptionsDisplayManager.UpdateEditorAsync(options, _updateModelAccessor.ModelUpdater, false, string.Empty, string.Empty);
await _auditTrailOptionsDisplayManager.UpdateEditorAsync(options, this, false, string.Empty, string.Empty);

// The route value must always be added after the editors have updated the models.
options.RouteValues.TryAdd("q", options.FilterResult.ToString());
Expand All @@ -154,7 +149,7 @@ public async Task<ActionResult> Display(string auditTrailEventId)
}


var shape = await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: _updateModelAccessor.ModelUpdater, displayType: "DetailAdmin");
var shape = await _displayManager.BuildDisplayAsync(auditTrailEvent, updater: this, displayType: "DetailAdmin");

return View(new AuditTrailItemViewModel { Shape = shape });
}
Expand Down
16 changes: 7 additions & 9 deletions src/OrchardCore.Modules/OrchardCore.BackgroundTasks/Controllers/BackgroundTaskController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@ public class BackgroundTaskController : Controller
private readonly IAuthorizationService _authorizationService;
private readonly IEnumerable<IBackgroundTask> _backgroundTasks;
private readonly BackgroundTaskManager _backgroundTaskManager;
private readonly PagerOptions _pagerOptions;
private readonly INotifier _notifier;
private readonly IShapeFactory _shapeFactory;

protected readonly IStringLocalizer S;
protected readonly IHtmlLocalizer H;
Expand All @@ -40,24 +38,24 @@ public BackgroundTaskController(
IAuthorizationService authorizationService,
IEnumerable<IBackgroundTask> backgroundTasks,
BackgroundTaskManager backgroundTaskManager,
IOptions<PagerOptions> pagerOptions,
IShapeFactory shapeFactory,
IHtmlLocalizer<BackgroundTaskController> htmlLocalizer,
IStringLocalizer<BackgroundTaskController> stringLocalizer,
INotifier notifier)
{
_authorizationService = authorizationService;
_backgroundTasks = backgroundTasks;
_backgroundTaskManager = backgroundTaskManager;
_pagerOptions = pagerOptions.Value;
_notifier = notifier;
_shapeFactory = shapeFactory;
S = stringLocalizer;
H = htmlLocalizer;
}

[Admin("BackgroundTasks", "BackgroundTasks")]
public async Task<IActionResult> Index(AdminIndexOptions options, PagerParameters pagerParameters)
public async Task<IActionResult> Index(
[FromServices] IOptions<PagerOptions> pagerOptions,
[FromServices] IShapeFactory shapeFactory,
AdminIndexOptions options,
PagerParameters pagerParameters)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageBackgroundTasks))
{
Expand Down Expand Up @@ -125,8 +123,8 @@ public async Task<IActionResult> Index(AdminIndexOptions options, PagerParameter
routeData.Values.TryAdd(_optionsStatus, options.Status);
}

var pager = new Pager(pagerParameters, _pagerOptions.GetPageSize());
var pagerShape = await _shapeFactory.PagerAsync(pager, taskItems.Count, routeData);
var pager = new Pager(pagerParameters, pagerOptions.Value.GetPageSize());
var pagerShape = await shapeFactory.PagerAsync(pager, taskItems.Count, routeData);

var model = new BackgroundTaskIndexViewModel
{
Expand Down
8 changes: 2 additions & 6 deletions ...ules/OrchardCore.ContentFields/Controllers/LocalizationSetContentPickerAdminController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
using OrchardCore.Modules;
using YesSql;
using YesSql.Services;
using IHttpContextAccessor = Microsoft.AspNetCore.Http.IHttpContextAccessor;

namespace OrchardCore.ContentFields.Controllers
{
Expand All @@ -27,22 +26,19 @@ public class LocalizationSetContentPickerAdminController : Controller
private readonly IContentManager _contentManager;
private readonly ISession _session;
private readonly IAuthorizationService _authorizationService;
private readonly IHttpContextAccessor _httpContextAccessor;

public LocalizationSetContentPickerAdminController(
IContentDefinitionManager contentDefinitionManager,
IContentLocalizationManager contentLocalizationManager,
IContentManager contentManager,
ISession session,
IAuthorizationService authorizationService,
IHttpContextAccessor httpContextAccessor)
IAuthorizationService authorizationService)
{
_contentDefinitionManager = contentDefinitionManager;
_contentLocalizationManager = contentLocalizationManager;
_contentManager = contentManager;
_session = session;
_authorizationService = authorizationService;
_httpContextAccessor = httpContextAccessor;
}

[HttpGet]
Expand Down Expand Up @@ -80,7 +76,7 @@ public async Task<IActionResult> SearchLocalizationSets(string part, string fiel

foreach (var contentItem in cleanedContentItems)
{
if (await _authorizationService.AuthorizeAsync(_httpContextAccessor.HttpContext.User, CommonPermissions.ViewContent, contentItem))
if (await _authorizationService.AuthorizeAsync(User, CommonPermissions.ViewContent, contentItem))
{
results.Add(new VueMultiselectItemViewModel
{
Expand Down
Loading
Loading