Support expressions instead of magic strings where possible

[ivaylokenov]

Do you plan to support expressions instead of magic strings where possible?

For example:

• RedirectToAction<HomeController>(c => c.Index(5))
• Url.Action(c => c.Index(5))

Where values in the methods are resolved to their corresponding route values?

[danroth27]

We don't plan to do this for the initial release of MVC 6, simply because of time and resource constraints. But it's certainly something we will consider in the future. Adding this to our backlog.

[akamud]

This would be a great addition. Magic strings are too unreliable.

[ivaylokenov]

@danroth27 @Eilon I want to contribute to MVC 6 by implementing this feature. It should be fairly easy task for me since last year I made a NuGet package for MVC 5, containing some extension methods adding the functionality: https://github.com/ivaylokenov/ASP.NET-MVC-Lambda-Expression-Helpers If you want to see how I write/design code, you can take a look at this repository: https://github.com/ivaylokenov/MyTested.WebApi. Of course, I will follow your conventions.

I have already read the guidelines and signed the needed document.

I can add Controller.Redirect, Url.Action, ModelState.AddModelError and all applicable HtmlHelper methods. If it is OK with you, I have couple of questions:

• What do you prefer - adding the new methods directly into the classes/interfaces or creating extensions methods for them?
• Which one works better for you - opening new issues and pull requests for every class/interface I add methods to or just use this issue as a reference (again with couple of pull requests for easier code review)?

[ivaylokenov]

After running several performance tests, I came to the consclusion that this feature will bring little to no value in MVC 6 at all. Comparing lambda expression parameter resolving to PropertyHelper.ObjectToDictionary gave the following results:

• When using expressions with constants, for example c => c.SomeAction(5), the results were faster than PropertyHelper.ObjectToDictionary(new { id = 5 }) (no matter the caching), but using constants is not the common use case. Most of the time the route values depend on variables.
• When using expressions with variables, for example c => c.SomeAction(id), where id is not known compile time, the results were quite slower than PropertyHelper.ObjectToDictionary(new { id }). Using caching techniques is not efficient, since the bottleneck is resolving the variable run-time value, which will be needed for the cache key.

The developer value these methods gave in MVC 5 was not relying on magic strings, when building URLs and redirects, but with the C# 6 nameof compile-time feature, it is a lot better to use RedirectToAction(nameof(Controller.Action), nameof(Controller), new { id }), instead of RedirectToAction<Controller>(c => c.Action(id)) (although the code looks prettier with lambda expressions in my opition).

If you have another opinion or suggestion, let me know. I will take a look at the other issues to find something I can help with. :)

[Eilon]

@ivaylokenov we're not currently looking at adding this feature to MVC 6, but if you have a prototype you'd like to share, we'd be happy to have a look at it. No pressure :smile:

[ivaylokenov]

@Eilon Well, since MVC is one of the finest technologies out there, I've always thought your team as big bad wolfs (some of the profile pictures confirm it). :wolf: Anyway, I will write again when there is something interesting to share. :+1:

[Eilon]

Naah we're mostly kittens and puppies if you look at our profile pics :smile:

[rynowak]

I've always thought your team as big bad wolfs (some of the profile pictures confirm it

I hope you don't mean me!

I wouldn't let performance dissuade you from building something like this. If you have an example of a site where link generation is a significant contributor to overall performance, I'd be interested to see how we can improve that.

A few thoughts on the topic that I'm not sure I've written up before...

The high performance option (that still uses the routing system) will always be Attribute Routing + generating links with Named Routes. Url.Route("CreateUser"). Named routes are 'direct' and we just do a simple dictionary lookup to find the right route.

We're going to do more work to optimize RouteValueDictionary for cases like Url.Action("Buy", "Products", new { id = 5 }). Today the reflection code used to map id into a dictionary is cached, going forward we're going to elide the dictionary allocation and just create a slim wrapper object that implements IDictionary.

Your example RedirectToAction(nameof(Controller.Action), nameof(Controller), new { id }) doesn't really work in a lot of cases. For instance, the nameof() doesn't know how to infer the name of the controller from the name of the case. Likewise the developer could use [ActionName(...)] to override an action name.

The issue that you quickly run into with an expression-based system is that expressions are created anew every time the page runs, which is very expensive. This is how the compiler works, and we have implemented some caching for trivial cases, but that likely will not include anything that would be used in link generation.

---

Now all of this is benchmark-level perf info. For most users an expression based might still be useful even if the perf is strictly worse.

[ivaylokenov]

@rynowak Thanks for the answer, I got your point. :+1: To continue your thought, I have never actually used the string-based link generation - always written expression based extensions (knowing they are slower) and never had performance issues. I just prefer writing high quality code and then optimize, if needed, but this is me. However, adding the feature to the core MVC framework may mislead some developers to use it without knowing fully what is going on behind the scenes and this made me think twice whether it is appropriate. Especially when the benchmark tests showed ~80 times slower execution time compared to PropertyHelper.ObjectToDictionary method. I have seen the expression caching in your code but it is intended for other usages. Frankly, I am not quite sure how the extracted argument values from such method call expresion c => c.Action(id) can be cached so that the second call to the same expression is not compiled again. Anyway, I am already implementing some of the features with performance in mind. Here are the methods with all applicable overloads:

• [ ] RedirectToAction<HomeController>(c => c.Index(id))
• [ ] RedirectToActionPermanent<HomeController>(c => c.Index(id))
• [ ] Html.ActionLink<HomeController>(c => c.Index(id))
• [ ] Html.BeginForm<HomeController>(c => c.Index(id))
• [ ] Url.Action<HomeController>(c => c.Index(id))

These should respect ActionName, IRouteConstraint (like AreaAttribute), IActionModelConvention and IControllerModelConvention. Any other ideas? Maybe ViewComponents? Thanks!

[rynowak]

We're actually already taking a look at doing this for ViewComponents.

And you're right about c => c.Action(id) - in general that's not cacheable. We don't cache things like that in Razor.

[ivaylokenov]

@rynowak @Eilon I have implemented most of the core expression route values resolver and the IUrlHelper additional methods. They are located in this branch - LINK. Some minor things are still not added (like proper exception messages) but they will be soon enough. I would be happy, if the code gets reviewed, since all other helper methods (redirects, action links and forms) will depend on the ExpressionRouteHelper class, which should be fully functional and unit tested before I move on to them. Should I open a new Work In Progress pull request, where you can take a look and put comments and suggestions when you have the time to do so?

Additionally, I have two questions:

• Is there a better way to get ControllerActionDescriptors by MethodInfo? Currently I am searching through the list provided by the IActionDescriptorsCollectionProvider and then cache the result.
• On a side note, I was thinking whether it will be good idea to add something which Moq is doing in their expressions with It.Is<> but something like c => c.Action(NoValue.For<RequestModel>()). Honestly speaking c => c.Action(null, null) looks ugly and some developers will be tempted to implement their own dummy class but it will have huge drawback, since the expression resolver will compile it (while null can be extracted right away). If the framework provides such class, it will be easy to extract it from the expression without the compilation - big performance difference. What do you think?

Thank you for your time! :hand:

[Eilon]

@ivaylokenov I would recommend trying to build this as a separate project so that it could potentially be published as an add-on to MVC, as opposed to making the changes in MVC itself. In theory you should be able to write everything as extension methods to IUrlHelper.

[ivaylokenov]

@Eilon Sorry, I could not understand you. You mean to extract the code from the fork, create a separate package and upload it as my own add-on in NuGet? Thank you for the reply. :)

[Eilon]

@ivaylokenov yes exactly, that's where I would start out.

[ivaylokenov]

Sure, will do! Thanks! :+1:

[ivaylokenov]

@Eilon @rynowak Implemented, tested and documented :smile: However, I have doubts developers will find this package and its features easily without heavy advertising on the Internet. Anyway, thank you guys for the marvelous open source code of MVC 6. I learnt a few tricks, while reading it and playing with it. :+1:

[Eilon]

@ivaylokenov very nice! We'd be happy to link to your project from the MVC repo's readme. Do you have a 1-line description we could use in a link to your repo?

[ivaylokenov]

@Eilon Thank you very much! I would be very grateful, if you provide a link to the repo! As for a 1-line description, I believe something like this: "Collection of extension methods providing strongly typed routing and link generation for ASP.NET MVC 6 projects."

[Eilon]

@ivaylokenov I added a link: https://github.com/aspnet/Mvc/blob/dev/README.md

Thanks!

[ivaylokenov]

No, @Eilon, thank you! You guys are truly awesome! :+1:

[GraemeSMiller]

Came to the issues looking to see if this feature existed in backlog. Great work @ivaylokenov. Death to magic strings - they have no place in the core framework.

[ivaylokenov]

@GraemeSMiller Thank you!

[achmstein]

Good job @ivaylokenov

[danroth27]

See also http://www.strathweb.com/2016/06/introducing-strathweb-typedrouting-for-asp-net-mvc-core/ for an alternative implementation

[ghost]

I had a similar thing come up for me, where I'd like to use the new nameof() operator. So, I just created #5535.

[Mardoxx]

Any developments on this?

In the mean time, I am wondering how would you go about testing that:

return RedirectToAction("BarAction", "FooController");

is going to redirect to the controller and method?

Having a quick look at how RedirectToActionResult is handled, a URL is created regardless of if this controller/action pair is valid. Is there a service available that I can use to resolve a controller/action name to the method which will get invoked? I'd imagine this is the responsibility of the router, but I'm really having difficulty figuring this out.

[pranavkm]

@Mardoxx the recommendation here is to use @ivaylokenov's library - https://github.com/ivaylokenov/AspNet.Mvc.TypedRouting. Could you start a different thread for your question? I'd recommend starting Stackoverflow first, you generally get better traction there from the community.