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.

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

Conversation

MikeAlhayek
Copy link
Member

No description provided.

{
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.

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?

@Piedone
Copy link
Member

Piedone commented Mar 13, 2024

What's the goal of this change? Less code? Performance?

@MikeAlhayek
Copy link
Member Author

@Piedone the idea is to reduce the amount of services needed on every single request in these controllers. Also, we can clean up the code by reducing the amount of services needed in the constructor.

@Piedone
Copy link
Member

Piedone commented Mar 14, 2024

Yeah but what's the goal with reducing the services, performance? Because perhaps the injection itself (i.e. passing the object to the ctor) has some measurable performance impact, but the resolution is most certainly happening anyway, because something will use IUpdateModelAccessor in the request.

@MikeAlhayek
Copy link
Member Author

For the IUpdateModelAccessor it's just at resolution time because that service will always be constructed in the filter. But the other services it helps reducing the need of creating the services at the controller service unless needed later on by other services.

@Piedone
Copy link
Member

Piedone commented Mar 14, 2024

IUpdateModelAccessor is scoped, so once ModelBinderAccessorFilter resolves it, there will be no further resolutions (i.e. object instantiations) within the request.

@MikeAlhayek
Copy link
Member Author

Yes there will be no construction needed because it is scoped "and constructed earlier". But there will be a resolution as the container has to get the instance of the service from the container and then inject it again in the controller. Yes it is a very cheap operation, but it is an operation that has to be done.

Anyway, if we find benefit of keeping that service in the controller, we can keep it "cheap operation". But why would someone need that in OC controller? If someone uses OC and want to provide their own implementation, they should be able to do it even after this change. They can register their own implementation and we'll resolve their custom implementation and use it in our controllers. I don't see a good reason to inject it in the controller's constructor.

On the other hand, if we were to build a test case in OC for a controller, we can Mock the filter and inject anything we want. But even if we don't want to mock the filter, we can then inject the service in the controller. But we should just inject it just because since we don't have a test case that replace that implementation.

@Piedone
Copy link
Member

Piedone commented Mar 14, 2024

We need to figure that out under #15473 (comment).

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants