Skip to content

Initial design for exception page filters #8958

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

Merged
merged 8 commits into from
Apr 2, 2019
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.0'">
<Compile Include="Microsoft.AspNetCore.Diagnostics.Abstractions.netcoreapp3.0.cs" />

<Reference Include="Microsoft.AspNetCore.Http.Abstractions" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,20 @@ public DiagnosticMessage(string message, string formattedMessage, string filePat
public int StartColumn { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public int StartLine { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
}
public partial class ErrorContext
{
public ErrorContext(Microsoft.AspNetCore.Http.HttpContext httpContext, System.Exception exception) { }
public System.Exception Exception { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Http.HttpContext HttpContext { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
}
public partial interface ICompilationException
{
System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Diagnostics.CompilationFailure> CompilationFailures { get; }
}
public partial interface IDeveloperPageExceptionFilter
{
System.Threading.Tasks.Task HandleExceptionAsync(Microsoft.AspNetCore.Diagnostics.ErrorContext errorContext, System.Func<Microsoft.AspNetCore.Diagnostics.ErrorContext, System.Threading.Tasks.Task> next);
}
public partial interface IExceptionHandlerFeature
{
System.Exception Error { get; }
Expand Down
35 changes: 35 additions & 0 deletions src/Middleware/Diagnostics.Abstractions/src/ErrorContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Diagnostics
{
/// <summary>
/// Provides context about the error currently being handled bt the DeveloperExceptionPageMiddleware.
/// </summary>
public class ErrorContext
{
/// <summary>
/// Initializes the ErrorContext with the specified <see cref="HttpContext"/> and <see cref="Exception"/>.
/// </summary>
/// <param name="httpContext"></param>
/// <param name="exception"></param>
public ErrorContext(HttpContext httpContext, Exception exception)
{
HttpContext = httpContext ?? throw new ArgumentNullException(nameof(httpContext));
Exception = exception ?? throw new ArgumentNullException(nameof(exception));
}

/// <summary>
/// The <see cref="HttpContext"/>.
/// </summary>
public HttpContext HttpContext { get; }

/// <summary>
/// The <see cref="Exception"/> thrown during request processing.
/// </summary>
public Exception Exception { get; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Diagnostics
{
/// <summary>
/// Provides an extensiblity point for changing the behavior of the DeveloperExceptionPageMiddleware.
/// </summary>
public interface IDeveloperPageExceptionFilter
{
/// <summary>
/// An exception handling method that is used to either format the exception or delegate to the next handler in the chain.
/// </summary>
/// <param name="errorContext">The error context.</param>
/// <param name="next">The next filter in the pipeline.</param>
/// <returns>A task the completes when the handler is done executing.</returns>
Task HandleExceptionAsync(ErrorContext errorContext, Func<ErrorContext, Task> next);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core diagnostics middleware abstractions and feature interface definitions.</Description>
Expand All @@ -9,4 +9,8 @@
<PackageTags>aspnetcore;diagnostics</PackageTags>
</PropertyGroup>

<ItemGroup>
<Reference Include="Microsoft.AspNetCore.Http.Abstractions" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ namespace Microsoft.AspNetCore.Diagnostics
{
public partial class DeveloperExceptionPageMiddleware
{
public DeveloperExceptionPageMiddleware(Microsoft.AspNetCore.Http.RequestDelegate next, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Builder.DeveloperExceptionPageOptions> options, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.Hosting.IWebHostEnvironment hostingEnvironment, System.Diagnostics.DiagnosticSource diagnosticSource) { }
public DeveloperExceptionPageMiddleware(Microsoft.AspNetCore.Http.RequestDelegate next, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Builder.DeveloperExceptionPageOptions> options, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.Hosting.IWebHostEnvironment hostingEnvironment, System.Diagnostics.DiagnosticSource diagnosticSource, System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Diagnostics.IDeveloperPageExceptionFilter> filters) { }
Copy link
Member Author

@davidfowl davidfowl Apr 1, 2019

Choose a reason for hiding this comment

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

This is a breaking change. If you want to revolt, tell me and I'll add an overload (https://github.com/aspnet/AspNetCore/pull/8958/files#r270702887)

Copy link
Member

Choose a reason for hiding this comment

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

Do we really think there are many (any?) folks newing up this middleware class themselves? That said, generally speaking we should lean to remaining compatible for all changes unless we have good reason not to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we really think there are many (any?) folks newing up this middleware class themselves?

No.

That said, generally speaking we should lean to remaining compatible for all changes unless we have good reason not to.

We may have to live with this one. Adding another constructor results in the system picking the "wrong" one. I think the risk is small enough to warrant it. I'm investigating anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Added the breaking change label 👼

Copy link
Member

Choose a reason for hiding this comment

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

It picks the first constructor it can satisfy, put the new one on top.

[System.Diagnostics.DebuggerStepThroughAttribute]
public System.Threading.Tasks.Task Invoke(Microsoft.AspNetCore.Http.HttpContext context) { throw null; }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public class DeveloperExceptionPageMiddleware
private readonly IFileProvider _fileProvider;
private readonly DiagnosticSource _diagnosticSource;
private readonly ExceptionDetailsProvider _exceptionDetailsProvider;
private readonly Func<ErrorContext, Task> _exceptionHandler;

/// <summary>
/// Initializes a new instance of the <see cref="DeveloperExceptionPageMiddleware"/> class
Expand All @@ -40,12 +41,14 @@ public class DeveloperExceptionPageMiddleware
/// <param name="loggerFactory"></param>
/// <param name="hostingEnvironment"></param>
/// <param name="diagnosticSource"></param>
/// <param name="filters"></param>
Copy link
Member

Choose a reason for hiding this comment

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

this are some hq docs

public DeveloperExceptionPageMiddleware(
RequestDelegate next,
IOptions<DeveloperExceptionPageOptions> options,
ILoggerFactory loggerFactory,
IWebHostEnvironment hostingEnvironment,
DiagnosticSource diagnosticSource)
DiagnosticSource diagnosticSource,
IEnumerable<IDeveloperPageExceptionFilter> filters)
Copy link
Member

Choose a reason for hiding this comment

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

breaking change yo

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know but it's 3.0 and I'm expecting this to be acceptable. I didn't update the refs tho because who ever remembers that the first time 😄

{
if (next == null)
{
Expand All @@ -57,12 +60,24 @@ public DeveloperExceptionPageMiddleware(
throw new ArgumentNullException(nameof(options));
}

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

_next = next;
_options = options.Value;
_logger = loggerFactory.CreateLogger<DeveloperExceptionPageMiddleware>();
_fileProvider = _options.FileProvider ?? hostingEnvironment.ContentRootFileProvider;
_diagnosticSource = diagnosticSource;
_exceptionDetailsProvider = new ExceptionDetailsProvider(_fileProvider, _options.SourceCodeLineCount);
_exceptionHandler = DisplayException;

foreach (var filter in filters.Reverse())
{
var nextFilter = _exceptionHandler;
_exceptionHandler = errorContext => filter.HandleExceptionAsync(errorContext, nextFilter);
}
Copy link
Member

Choose a reason for hiding this comment

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

the CoR is so elegant and cool

}

/// <summary>
Expand Down Expand Up @@ -91,7 +106,7 @@ public async Task Invoke(HttpContext context)
context.Response.Clear();
context.Response.StatusCode = 500;

await DisplayException(context, ex);
await _exceptionHandler(new ErrorContext(context, ex));

if (_diagnosticSource.IsEnabled("Microsoft.AspNetCore.Diagnostics.UnhandledException"))
{
Expand All @@ -110,15 +125,15 @@ public async Task Invoke(HttpContext context)
}

// Assumes the response headers have not been sent. If they have, still attempt to write to the body.
private Task DisplayException(HttpContext context, Exception ex)
private Task DisplayException(ErrorContext errorContext)
{
var compilationException = ex as ICompilationException;
var compilationException = errorContext.Exception as ICompilationException;
Copy link
Member

Choose a reason for hiding this comment

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

Factor out DisplayCompilationException as a filter? Even if you don't register it via DI.

Copy link
Member Author

@davidfowl davidfowl Apr 1, 2019

Choose a reason for hiding this comment

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

Why? It's just a function.

Copy link
Member

Choose a reason for hiding this comment

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

Proof of concept. This is doing exactly what filters are designed to do.

Copy link
Member Author

@davidfowl davidfowl Apr 1, 2019

Choose a reason for hiding this comment

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

I don't think it makes any sense. The thing left to resolve is the ordering concern that @rynowak brought up. The slight code will make the diff bigger and don't add any value.

if (compilationException != null)
{
return DisplayCompilationException(context, compilationException);
return DisplayCompilationException(errorContext.HttpContext, compilationException);
}

return DisplayRuntimeException(context, ex);
return DisplayRuntimeException(errorContext.HttpContext, errorContext.Exception);
}

private Task DisplayCompilationException(
Expand Down Expand Up @@ -215,7 +230,7 @@ private Task DisplayRuntimeException(HttpContext context, Exception ex)
}
}
}

var request = context.Request;

var model = new ErrorPageModel
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using Xunit;
Expand Down Expand Up @@ -46,6 +45,88 @@ public async Task UnhandledErrorsWriteToDiagnosticWhenUsingExceptionPage()
Assert.Null(listener.DiagnosticHandledException?.Exception);
}

[Fact]
public async Task ExceptionPageFiltersAreApplied()
{
// Arrange
var builder = new WebHostBuilder()
.ConfigureServices(services =>
{
services.AddSingleton<IDeveloperPageExceptionFilter, ExceptionMessageFilter>();
})
.Configure(app =>
{
app.UseDeveloperExceptionPage();
app.Run(context =>
{
throw new Exception("Test exception");
});
});
var server = new TestServer(builder);

// Act
var response = await server.CreateClient().GetAsync("/path");

// Assert
Assert.Equal("Test exception", await response.Content.ReadAsStringAsync());
}

[Fact]
public async Task ExceptionFilterCallingNextWorks()
{
// Arrange
var builder = new WebHostBuilder()
.ConfigureServices(services =>
{
services.AddSingleton<IDeveloperPageExceptionFilter, PassThroughExceptionFilter>();
services.AddSingleton<IDeveloperPageExceptionFilter, AlwaysBadFormatExceptionFilter>();
services.AddSingleton<IDeveloperPageExceptionFilter, ExceptionMessageFilter>();
})
.Configure(app =>
{
app.UseDeveloperExceptionPage();
app.Run(context =>
{
throw new Exception("Test exception");
});
});
var server = new TestServer(builder);

// Act
var response = await server.CreateClient().GetAsync("/path");

// Assert
Assert.Equal("Bad format exception!", await response.Content.ReadAsStringAsync());
}

[Fact]
public async Task ExceptionPageFiltersAreAppliedInOrder()
{
// Arrange
var builder = new WebHostBuilder()
.ConfigureServices(services =>
{
services.AddSingleton<IDeveloperPageExceptionFilter, AlwaysThrowSameMessageFilter>();
services.AddSingleton<IDeveloperPageExceptionFilter, ExceptionMessageFilter>();
services.AddSingleton<IDeveloperPageExceptionFilter, ExceptionToStringFilter>();
})
.Configure(app =>
{
app.UseDeveloperExceptionPage();
app.Run(context =>
{
throw new Exception("Test exception");
});
});
var server = new TestServer(builder);

// Act
var response = await server.CreateClient().GetAsync("/path");

// Assert
Assert.Equal("An error occurred", await response.Content.ReadAsStringAsync());
}

public static TheoryData CompilationExceptionData
{
get
Expand Down Expand Up @@ -140,5 +221,45 @@ public CustomCompilationException(IEnumerable<CompilationFailure> compilationFai

public IEnumerable<CompilationFailure> CompilationFailures { get; }
}

public class ExceptionMessageFilter : IDeveloperPageExceptionFilter
{
public Task HandleExceptionAsync(ErrorContext context, Func<ErrorContext, Task> next)
{
return context.HttpContext.Response.WriteAsync(context.Exception.Message);
}
}

public class ExceptionToStringFilter : IDeveloperPageExceptionFilter
{
public Task HandleExceptionAsync(ErrorContext context, Func<ErrorContext, Task> next)
{
return context.HttpContext.Response.WriteAsync(context.Exception.ToString());
}
}

public class AlwaysThrowSameMessageFilter : IDeveloperPageExceptionFilter
{
public Task HandleExceptionAsync(ErrorContext context, Func<ErrorContext, Task> next)
{
return context.HttpContext.Response.WriteAsync("An error occurred");
}
}

public class AlwaysBadFormatExceptionFilter : IDeveloperPageExceptionFilter
{
public Task HandleExceptionAsync(ErrorContext context, Func<ErrorContext, Task> next)
{
return next(new ErrorContext(context.HttpContext, new FormatException("Bad format exception!")));
}
}

public class PassThroughExceptionFilter : IDeveloperPageExceptionFilter
{
public Task HandleExceptionAsync(ErrorContext context, Func<ErrorContext, Task> next)
{
return next(context);
}
}
}
}