Update the ExceptionHandlerMiddleware to support getting the path to redirect to via LinkGenerator

[DamianEdwards]

Today, the web app project templates have a block like this:

if (env.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}
else
{
    app.UseExceptionHandler("/Error");
    app.UseHsts();
}

Note the string literal parameter in the call to app.UseExceptionHandler. That string (in this case) is the path to a Razor Page (~/Pages/Error.cshtml), but as it's a string literal, if the page's route changes (e.g. because its name changes or another route for it is configured) this string path will now be wrong.

It would be great if the middleware allowed the user to specify the path using a LinkGenerator (expression?) such that strongly-typed paths to MVC endpoints in 2.2 (and in 3.0 any endpoint) could be done.

@rynowak @JamesNK

[rynowak]

A design for this might look like:

if (env.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}
else
{
    app.UseExceptionHandler(link => link.GetPathByAction("Error", "Home"));
    app.UseHsts();
}

It's a lambda because doing otherwise would require baking knowledge of an MVC extension method into diagnostics. This will require diagnostics to take a dependency on Routing (probably). This will add a using Microsoft.AspNetCore.Routing to the startup class.

@davidfowl @DamianEdwards thoughts?

[Tratcher]

@rynowak in 3.0 routing will already move down to a level that diagnostics can use, right?

[rynowak]

Well that's a question we need to ask ourselves. Currently LinkGenerator and related features live in the routing package and require a AddRouting call by someone. I believe this discussion is about 2.2 though

[mkArtakMSFT]

@DamianEdwards, can you please confirm whether you're ok with the design @rynowak proposed:

if (env.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}
else
{
    app.UseExceptionHandler(link => link.GetPathByAction("Error", "Home"));
    app.UseHsts();
}

[mkArtakMSFT]

I was thinking about not even define it at all. Just have app.UseExceptionHandler() and have a convention which would look for a page with a [DefaultExceptionHandler] attribute applied to it.

[JamesNK]

As part of 3.0 work we're planning to review all middleware to see what would benefit from integrating with endpoints.

I think looking at generating URLs could happen in the same review, and it would be benefical to have a single strategy rather than introducing link generation to one middleware at a time.

[DamianEdwards]

To make sure I understand, MVC would add extension methods to LinkGenerator to enable methods like:

link.GetPathForAction(string actionName, string controllerName);
link.GetPathForAction(string actionName, string controllerName, params object[] routeArgs);
link.GetPathForPage(string pageName);
link.GetPathForPage(string pageName, string handlerName);
link.GetPathForPage(string pageName, params object[] routeArgs);
link.GetPathForPage(string pageName, string handlerName, params object[] routeArgs);

Then we'd either need Diagnostics to take a dependency on Routing, or create a new package that depends on both, to add the middleware extension methods like:

app.UseExceptionHandler(Func<LinkGenerator, string> pathSelector);

Is that right?

I don't mind @mkArtakMSFT's idea about adding a meta-data convention to the default overload too, falling back to the default we already have today (throwing if both the handler and handler path are null).

[rynowak]

To make sure I understand, MVC would add extension methods to LinkGenerator to enable method

This is already done 😋

I like the Func<> option because it grows up nicely. It's also really clear what's happening if we put that code in the template.

I think the no-args thing is pretty interesting, and I feel good about it if it's for 3.0. In a 2.2 context it would be pretty hard to explain to a user because it will introduce a bunch of new concepts that will only be used for this feature (in 2.2). I think if we do this one we should do both.

--

I also don't want to design this feature in a vacuum. As @JamesNK pointed out, one of his next tasks is to review all of the other candidates for using link generation in middleware.

[javiercn]

@rynowak @JamesNK Sounds good to me. Should we put this in the back-burner then?

[Tratcher]

Careful with the func, they can make inline lambda's really confusing to write when there's multiple overloads.
https://github.com/aspnet/Diagnostics/blob/204ff0a78504b3c688d906bab4e03e87d8ea1b76/src/Microsoft.AspNetCore.Diagnostics/ExceptionHandler/ExceptionHandlerExtensions.cs#L56

[rynowak]

This would be a different arity though, so it should be alright

[Tratcher]

Only in that it has a return value, which is really ambiguous when you're not using the brackets link => link.GetPathByAction("Error", "Home") vs builder => builder.UseWelcomePage().