Initial design for exception page filters #8958

davidfowl · 2019-04-01T02:05:25Z

This change introduces the concept of an IDeveloperPageException filter that runs whenever the developer exception page has encountered an error. It follows the middleware pattern (chain of resposibility) which allows short circuiting or decorating the default logic.
Added tests

Contributes to #8536

- This change introduces the concept of an IDeveloperPageException filter that runs whenever the developer exception page has encountered an error. It follows the middleware pattern (chain of resposibility) which allows short circuiting or decorating the default logic. - Added tests

src/Middleware/Diagnostics/src/DeveloperExceptionPage/IDeveloperPageExceptionFilter.cs

rynowak · 2019-04-01T02:32:51Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs

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


breaking change yo

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 😄

rynowak · 2019-04-01T02:32:59Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs

@@ -40,12 +41,14 @@ public class DeveloperExceptionPageMiddleware
        /// <param name="loggerFactory"></param>
        /// <param name="hostingEnvironment"></param>
        /// <param name="diagnosticSource"></param>
+        /// <param name="filters"></param>


this are some hq docs

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs

rynowak · 2019-04-01T02:34:19Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs

+            {
+                var nextFilter = _exceptionHandler;
+                _exceptionHandler = (context, exception) => filter.HandleExceptionAsync(context, exception, nextFilter);
+            }


the CoR is so elegant and cool

src/Middleware/Diagnostics/test/UnitTests/DeveloperExceptionPageMiddlewareTest.cs

davidfowl · 2019-04-01T02:59:25Z

src/Middleware/Diagnostics/ref/Microsoft.AspNetCore.Diagnostics.netcoreapp3.0.cs

@@ -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) { }


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)

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.

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.

Added the breaking change label 👼

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

src/Middleware/Diagnostics/src/DeveloperExceptionPage/IDeveloperPageExceptionFilter.cs

Tratcher · 2019-04-01T16:50:12Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs

        {
-            var compilationException = ex as ICompilationException;
+            var compilationException = errorContext.Exception as ICompilationException;


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

Why? It's just a function.

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

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.

Eilon

I dig the design.

src/Middleware/Diagnostics.Abstractions/src/IDeveloperPageExceptionFilter.cs

src/Middleware/Diagnostics.Abstractions/src/ErrorContext.cs

src/Middleware/Diagnostics.Abstractions/src/IDeveloperPageExceptionFilter.cs

davidfowl requested review from DamianEdwards and Tratcher April 1, 2019 02:05

davidfowl commented Apr 1, 2019

View reviewed changes

src/Middleware/Diagnostics/src/DeveloperExceptionPage/IDeveloperPageExceptionFilter.cs Outdated Show resolved Hide resolved

davidfowl requested a review from rynowak April 1, 2019 02:13

rynowak reviewed Apr 1, 2019

View reviewed changes

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs Show resolved Hide resolved

rynowak reviewed Apr 1, 2019

View reviewed changes

src/Middleware/Diagnostics/test/UnitTests/DeveloperExceptionPageMiddlewareTest.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits March 31, 2019 19:57

PR feedback and ref asm

786a8fa

Sort usings

b14d33c

davidfowl commented Apr 1, 2019

View reviewed changes

Tratcher reviewed Apr 1, 2019

View reviewed changes

src/Middleware/Diagnostics/src/DeveloperExceptionPage/IDeveloperPageExceptionFilter.cs Outdated Show resolved Hide resolved

src/Middleware/Diagnostics/src/DeveloperExceptionPage/IDeveloperPageExceptionFilter.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits April 1, 2019 09:37

Added an ErrorContext holder type

4a68f3d

Ref asm and doc comments

0328ea0

Tratcher reviewed Apr 1, 2019

View reviewed changes

Moved interface and context to abstractions

2cdf95b

Eilon approved these changes Apr 1, 2019

View reviewed changes

src/Middleware/Diagnostics.Abstractions/src/IDeveloperPageExceptionFilter.cs Outdated Show resolved Hide resolved

src/Middleware/Diagnostics.Abstractions/src/IDeveloperPageExceptionFilter.cs Outdated Show resolved Hide resolved

Updated docs a bit

8a1a164

davidfowl marked this pull request as ready for review April 2, 2019 04:00

davidfowl requested a review from analogrelay as a code owner April 2, 2019 04:00

analogrelay added the area-middleware label Apr 2, 2019

analogrelay added this to the 3.0.0-preview4 milestone Apr 2, 2019

JamesNK reviewed Apr 2, 2019

View reviewed changes

More doc comments and PR feedback

ffca724

davidfowl merged commit f4c61de into master Apr 2, 2019

JamesNK added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Apr 5, 2019

davidfowl deleted the davidfowl/error-page-filters branch April 15, 2019 07:48

amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial design for exception page filters #8958

Initial design for exception page filters #8958

davidfowl commented Apr 1, 2019

rynowak Apr 1, 2019

davidfowl Apr 1, 2019

rynowak Apr 1, 2019

rynowak Apr 1, 2019

davidfowl Apr 1, 2019 •

edited

Loading

DamianEdwards Apr 5, 2019

davidfowl Apr 5, 2019

JamesNK Apr 5, 2019

Tratcher Apr 5, 2019

Tratcher Apr 1, 2019

davidfowl Apr 1, 2019 •

edited

Loading

Tratcher Apr 1, 2019

davidfowl Apr 1, 2019 •

edited

Loading

Eilon left a comment

Initial design for exception page filters #8958

Initial design for exception page filters #8958

Conversation

davidfowl commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Eilon left a comment

Choose a reason for hiding this comment

davidfowl Apr 1, 2019 •

edited

Loading

davidfowl Apr 1, 2019 •

edited

Loading

davidfowl Apr 1, 2019 •

edited

Loading