Skip to content

Update ExceptionHandler middleware pipeline construction to reinvoke UseRouting in error branch #34991

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions src/DefaultBuilder/src/WebApplicationBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace Microsoft.AspNetCore.Builder
public sealed class WebApplicationBuilder
{
private const string EndpointRouteBuilderKey = "__EndpointRouteBuilder";
private const string WebApplicationBuilderKey = "__WebApplicationBuilder";

private readonly HostBuilder _hostBuilder = new();
private readonly BootstrapHostBuilder _bootstrapHostBuilder;
Expand Down Expand Up @@ -170,6 +171,8 @@ public WebApplication Build()
((IConfigurationBuilder)Configuration).Sources.Clear();
Configuration.AddConfiguration(_builtApplication.Configuration);

_builtApplication.Properties[WebApplicationBuilderKey] = true;
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this property? Trying to grok where it is used...

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind! I see you are using this in the extension methods to check if they are invoked on a WebApplicationBuilder. Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added this "hack" to clearly indicate that the builder is a WebApplicationBuilder, where we make use of "creative" pipeline construction techniques 😄


// Mark the service collection as read-only to prevent future modifications
_services.IsReadOnly = true;

Expand Down Expand Up @@ -206,6 +209,10 @@ private void ConfigureApplication(WebHostBuilderContext context, IApplicationBui

// An implicitly created IEndpointRouteBuilder was addeded to app.Properties by the UseRouting() call above.
targetRouteBuilder = GetEndpointRouteBuilder(app)!;

// Copy the endpoint route builder to the built application
_builtApplication.Properties[EndpointRouteBuilderKey] = targetRouteBuilder;

implicitRouting = true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Builder
/// </summary>
public static class EndpointRoutingApplicationBuilderExtensions
{
private const string EndpointRouteBuilder = "__EndpointRouteBuilder";
internal const string EndpointRouteBuilder = "__EndpointRouteBuilder";

/// <summary>
/// Adds a <see cref="EndpointRoutingMiddleware"/> middleware to the specified <see cref="IApplicationBuilder"/>.
Expand Down Expand Up @@ -42,10 +42,48 @@ public static IApplicationBuilder UseRouting(this IApplicationBuilder builder)
throw new ArgumentNullException(nameof(builder));
}

return UseRouting(builder, true);
}

/// <summary>
/// Adds a <see cref="EndpointRoutingMiddleware"/> middleware to the specified <see cref="IApplicationBuilder"/>.
/// </summary>
/// <param name="builder">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
/// <param name="overrideEndpointRouteBuilder">Whether a new <see cref="EndpointRouteBuilder"/> should be created.</param>
/// <returns>A reference to this instance after the operation has completed.</returns>
/// <remarks>
/// <para>
/// A call to <see cref="UseRouting(IApplicationBuilder)"/> must be followed by a call to
/// <see cref="UseEndpoints(IApplicationBuilder, Action{IEndpointRouteBuilder})"/> for the same <see cref="IApplicationBuilder"/>
/// instance.
/// </para>
/// <para>
/// The <see cref="EndpointRoutingMiddleware"/> defines a point in the middleware pipeline where routing decisions are
/// made, and an <see cref="Endpoint"/> is associated with the <see cref="HttpContext"/>. The <see cref="EndpointMiddleware"/>
/// defines a point in the middleware pipeline where the current <see cref="Endpoint"/> is executed. Middleware between
/// the <see cref="EndpointRoutingMiddleware"/> and <see cref="EndpointMiddleware"/> may observe or change the
/// <see cref="Endpoint"/> associated with the <see cref="HttpContext"/>.
/// </para>
/// </remarks>
public static IApplicationBuilder UseRouting(this IApplicationBuilder builder, bool overrideEndpointRouteBuilder)
Copy link
Member

Choose a reason for hiding this comment

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

overrideEndpointRouteBuilder..... is literal.

Copy link
Member

Choose a reason for hiding this comment

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

Alternative ideas:

  • branchBuilder or branchRouteBuilder
  • replaceBuilder or replaceRouteBuilder
  • overrideBuilder or overrideRouteBuilder
  • ?

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 think if we agree that this approach makes sense, we can discuss the naming in the API review. I think I like the overrideRouteBuilder option the best though.

Copy link
Member

@Tratcher Tratcher Aug 4, 2021

Choose a reason for hiding this comment

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

The bool doesn't seem like a meaningful public parameter, anyone that wants the 'true' behavior will already be calling UseRouting().

This should be a separate method name and the bool would be an implementation detail.
UseRoutingAgain() (🚲 🐑)

Copy link
Member

Choose a reason for hiding this comment

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

Can we not make this an extension method and leave it as a plain old static method? I don't expect many people to want to call this overload.

Copy link
Contributor Author

@JunTaoLuo JunTaoLuo Aug 4, 2021

Choose a reason for hiding this comment

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

This was actually one of my earlier ideas! I wanted to call it app.ReRoute() or app.RerunRouting() or app.UseRerouting().

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a different name. ResumeRouting?

{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

VerifyRoutingServicesAreRegistered(builder);

var endpointRouteBuilder = new DefaultEndpointRouteBuilder(builder);
builder.Properties[EndpointRouteBuilder] = endpointRouteBuilder;
IEndpointRouteBuilder endpointRouteBuilder;
if (overrideEndpointRouteBuilder || !builder.Properties.TryGetValue(EndpointRouteBuilder, out var routeBuilder))
{
endpointRouteBuilder = new DefaultEndpointRouteBuilder(builder);
builder.Properties[EndpointRouteBuilder] = endpointRouteBuilder;
}
else
{
endpointRouteBuilder = (IEndpointRouteBuilder)routeBuilder!;
}

return builder.UseMiddleware<EndpointRoutingMiddleware>(endpointRouteBuilder);
}
Expand Down
1 change: 1 addition & 0 deletions src/Http/Routing/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Microsoft.AspNetCore.Routing.IDataTokensMetadata.DataTokens.get -> System.Collec
Microsoft.AspNetCore.Routing.IRouteNameMetadata.RouteName.get -> string?
Microsoft.AspNetCore.Routing.RouteNameMetadata.RouteName.get -> string?
Microsoft.AspNetCore.Routing.RouteNameMetadata.RouteNameMetadata(string? routeName) -> void
static Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions.UseRouting(this Microsoft.AspNetCore.Builder.IApplicationBuilder! builder, bool overrideEndpointRouteBuilder) -> Microsoft.AspNetCore.Builder.IApplicationBuilder!
static Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.Map(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, Microsoft.AspNetCore.Routing.Patterns.RoutePattern! pattern, System.Delegate! action) -> Microsoft.AspNetCore.Builder.MinimalActionEndpointConventionBuilder!
static Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.Map(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! action) -> Microsoft.AspNetCore.Builder.MinimalActionEndpointConventionBuilder!
static Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.MapDelete(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! action) -> Microsoft.AspNetCore.Builder.MinimalActionEndpointConventionBuilder!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,72 @@ public async Task UseRouting_ServicesRegistered_Match_DoesNotSetsFeature()
Assert.Same(endpoint, httpContext.GetEndpoint());
}

[Fact]
public void UseRouting_Default_CreatesEndpointRouteBuilder()
{
// Arrange
var services = CreateServices();
var app = new ApplicationBuilder(services);

// Assert
Assert.False(app.Properties.ContainsKey(EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder));

// Act
app.UseRouting();

// Assert
Assert.NotNull(app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder]);
}

[Fact]
public void UseRouting_Default_OverridesEndpointRouteBuilder()
{
// Arrange
var services = CreateServices();
var app = new ApplicationBuilder(services);
var endpointRouteBuilder = new DefaultEndpointRouteBuilder(app);
app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder] = endpointRouteBuilder;

// Act
app.UseRouting();

// Assert
Assert.NotSame(endpointRouteBuilder, app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder]);
}

[Fact]
public void UseRouting_OverrideEndpointRouteBuilderFalse_CreatesEndpointRouteBuilderIfNotFound()
{
// Arrange
var services = CreateServices();
var app = new ApplicationBuilder(services);

// Assert
Assert.False(app.Properties.ContainsKey(EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder));

// Act
app.UseRouting(overrideEndpointRouteBuilder: false);

// Assert
Assert.NotNull(app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder]);
}

[Fact]
public void UseRouting_OverrideEndpointRouteBuilderFalse_UsesExistingEndpointRouteBuilderIfFound()
{
// Arrange
var services = CreateServices();
var app = new ApplicationBuilder(services);
var endpointRouteBuilder = new DefaultEndpointRouteBuilder(app);
app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder] = endpointRouteBuilder;

// Act
app.UseRouting(overrideEndpointRouteBuilder: false);

// Assert
Assert.Same(endpointRouteBuilder, app.Properties[EndpointRoutingApplicationBuilderExtensions.EndpointRouteBuilder]);
}

[Fact]
public void UseEndpoint_WithoutEndpointRoutingMiddleware_Throws()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Diagnostics;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Builder
Expand All @@ -17,6 +21,9 @@ public static class ExceptionHandlerExtensions
/// Adds a middleware to the pipeline that will catch exceptions, log them, and re-execute the request in an alternate pipeline.
/// The request will not be re-executed if the response has already started.
/// </summary>
/// <remarks>
/// This overload requires you to configure options with <see cref="ExceptionHandlerServiceCollectionExtensions.AddExceptionHandler"/>.
/// </remarks>
/// <param name="app"></param>
/// <returns></returns>
public static IApplicationBuilder UseExceptionHandler(this IApplicationBuilder app)
Expand Down Expand Up @@ -95,7 +102,33 @@ public static IApplicationBuilder UseExceptionHandler(this IApplicationBuilder a
throw new ArgumentNullException(nameof(options));
}

return app.UseMiddleware<ExceptionHandlerMiddleware>(Options.Create(options));
// UseRouting called before this middleware or Minimal
if (app.Properties.ContainsKey("__EndpointRouteBuilder") || app.Properties.ContainsKey("__WebApplicationBuilder"))
{
return app.Use(next =>
{
var loggerFactory = app.ApplicationServices.GetRequiredService<ILoggerFactory>();
var diagnosticListener = app.ApplicationServices.GetRequiredService<DiagnosticListener>();

if (!string.IsNullOrEmpty(options.ExceptionHandlingPath) && options.ExceptionHandler is null)
{
// start a new middleware pipeline
var builder = app.New();
// use the old routing pipeline if it exists so we preserve all the routes and matching logic
builder.UseRouting(overrideEndpointRouteBuilder: false);
// apply the next middleware
builder.Run(next);
// store the pipeline for the error case
options.ExceptionHandler = builder.Build();
}

return new ExceptionHandlerMiddleware(next, loggerFactory, Options.Create(options), diagnosticListener).Invoke;
});
}
else
{
return app.UseMiddleware<ExceptionHandlerMiddleware>(Options.Create(options));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -657,5 +657,45 @@ public async Task ExceptionHandler_CanReturn404Responses_WhenAllowed()
&& w.EventId == 4
&& w.Message == "No exception handler was found, rethrowing original exception.");
}

[Fact]
public async Task ExceptionHandler_RerunsRoutingOnError_WhenConfiguredWithExceptionHandlingPath()
{
using var host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.ConfigureServices(services =>
{
services.AddRouting();
})
.Configure(app =>
{
app.UseRouting();

app.UseExceptionHandler("/error");

app.UseEndpoints(endpoints =>
{
endpoints.MapGet("/error", context =>
{
return context.Response.WriteAsync("Exception handled");
});
endpoints.MapGet("/throw", context => throw new InvalidOperationException("Something bad happened."));
});
});
}).Build();

await host.StartAsync();

using (var server = host.GetTestServer())
{
var client = server.CreateClient();
var response = await client.GetAsync("throw");
Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
Assert.Equal("Exception handled", await response.Content.ReadAsStringAsync());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<Reference Include="Microsoft.Extensions.FileProviders.Abstractions" />
<Reference Include="Microsoft.Extensions.Logging.Abstractions" />
<Reference Include="Microsoft.Extensions.Options" />
<Reference Include="Microsoft.AspNetCore.Routing" />
</ItemGroup>

</Project>
19 changes: 19 additions & 0 deletions src/Middleware/Rewrite/src/RewriteBuilderExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,25 @@ public static IApplicationBuilder UseRewriter(this IApplicationBuilder app, Rewr
throw new ArgumentNullException(nameof(options));
}

// UseRouting called before this middleware or Minimal
if (app.Properties.ContainsKey("__EndpointRouteBuilder") || app.Properties.ContainsKey("__WebApplicationBuilder"))
{
return app.Use(next =>
{
// start a new middleware pipeline
var sub = app.New();
// insert the rewrite middleware before routing so any path changes will be matched correctly
sub.UseMiddleware<RewriteMiddleware>(Options.Create(options));
// use the old routing pipeline if it exists so we preserve all the routes and matching logic
sub.UseRouting(overrideEndpointRouteBuilder: false);
// apply the next middleware
sub.Run(next);
// return the modified middleware
var nextWithRewriteAndRouting = sub.Build();
return nextWithRewriteAndRouting.Invoke;
});
}

// put middleware in pipeline
return app.UseMiddleware<RewriteMiddleware>(Options.Create(options));
}
Expand Down
6 changes: 6 additions & 0 deletions src/Middleware/Rewrite/src/RewriteMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ public Task Invoke(HttpContext context)
Result = RuleResult.ContinueRules
};

// TODO: only do this if a rule applies
if (context.GetEndpoint() is not null)
{
context.SetEndpoint(null);
}

foreach (var rule in _options.Rules)
{
rule.ApplyRule(rewriteContext);
Expand Down
41 changes: 41 additions & 0 deletions src/Middleware/Rewrite/test/MiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -680,5 +680,46 @@ public async Task CheckRedirectToWwwWithStatusCodeInWhitelistedDomains(int statu
Assert.Equal(statusCode, (int)response.StatusCode);
}

[Fact]
public async Task Rewrite_RerunsRouting_WhenConfiguredAfterRouting()
{
var options = new RewriteOptions().AddRewrite("(.*)", "http://example.com/g", skipRemainingRules: false);
using var host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.ConfigureServices(services =>
{
services.AddRouting();
})
.Configure(app =>
{
app.UseRouting();
app.UseRewriter(options);

app.UseEndpoints(endpoints =>
{
endpoints.MapGet("/foo", context => context.Response.WriteAsync(
"bad"));

endpoints.MapGet("/g", context => context.Response.WriteAsync(
context.Request.Scheme +
"://" +
context.Request.Host +
context.Request.Path +
context.Request.QueryString));
});
});
}).Build();

await host.StartAsync();

var server = host.GetTestServer();

var response = await server.CreateClient().GetStringAsync("foo");

Assert.Equal("http://example.com/g", response);
}
}
}