Update ExceptionHandler middleware pipeline construction to reinvoke UseRouting in error branch by JunTaoLuo · Pull Request #34991 · dotnet/aspnetcore

JunTaoLuo · 2021-08-03T16:52:55Z

JunTaoLuo · 2021-08-03T17:10:08Z

-                    app.UseRouting();
+                    app.UseRouting(overrideEndpointRouteBuilder: false);
+
+                    // Copy the endpoint route builder to the built application


This is a bit hacky but required to ensure the EndpointRouteBuilder is shared between _builtApplication and app. We should probably invest in a cleaner approach for sharing the Properties bag between the two builders.

… UseRouting in error branch

JunTaoLuo · 2021-08-03T17:16:48Z

            }

-            return app.UseMiddleware<ExceptionHandlerMiddleware>(Options.Create(options));
+            return app.Use(next =>


All middlewares that modifies paths will now need to follow this pattern to work for Minimal APIs. I don't think this is very intuitive though.

Also, a similar fix is likely needed for RewriteMiddleware

This also needs to be more specific, we probably need to distinguish between minimal API vs regular apps to figure out if we need to handle the middleware differently.

davidfowl · 2021-08-03T17:45:22Z

+        /// <see cref="Endpoint"/> associated with the <see cref="HttpContext"/>.
+        /// </para>
+        /// </remarks>
+        public static IApplicationBuilder UseRouting(this IApplicationBuilder builder, bool overrideEndpointRouteBuilder)


overrideEndpointRouteBuilder..... is literal.

Alternative ideas:

branchBuilder or branchRouteBuilder

replaceBuilder or replaceRouteBuilder

overrideBuilder or overrideRouteBuilder

?

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.

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() (🚲 🐑)

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.

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

I like the idea of a different name. ResumeRouting?

davidfowl · 2021-08-03T18:47:26Z

+                    options.ExceptionHandler = errorBuilder.Build();
+                }
+
+                return new ExceptionHandlerMiddleware(next, loggerFactory, Options.Create(options), diagnosticListener).Invoke;


Why do you need to change this? Can't you use the normal UseMiddleware<ExceptionHandlerMiddleware>?

JunTaoLuo · 2021-08-04T18:45:58Z

I'll flip this to a non-draft PR but I expect there to be a few rounds of discussions regarding the design.

halter73 · 2021-08-04T22:48:08Z

+                    foreach (var item in app.Properties)
+                    {
+                        _builtApplication.Properties[item.Key] = item.Value;
+                    }

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


This feels a little less hacky. This works, right?

Suggested change

foreach (var item in app.Properties)

{

_builtApplication.Properties[item.Key] = item.Value;

}

// An implicitly created IEndpointRouteBuilder was addeded to app.Properties by the UseRouting() call above.

targetRouteBuilder = GetEndpointRouteBuilder(app)!;

// 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;

I think it'd work but I'm curious why that's better? Adding specific item to the Property bag still seems "hacky". I'm curious if we should keep the property bags on _builtApplication and app in sync? Would that be crossing too many wires?

I'd rather do only what's necessary rather than do extra stuff. If there was a scenario this fixed, then fine.

halter73 · 2021-08-04T22:53:14Z

                {
                    // The app defined endpoints without calling UseRouting() explicitly, so call UseRouting() implicitly.
-                    app.UseRouting();
+                    app.UseRouting(overrideEndpointRouteBuilder: false);


Why do we need to call this overload? We know there's no IEndpointRouteBuilder in this case, right?

Not quite. UseExceptionHandler is added by the user in Program.cs which occurs before app.Run() which is where this configuration runs. So UseRouting() here is configuring an EndpointRoutingMiddleware before the ExceptionHandlerMiddleware in the pipeline but doing so after UseExceptionHandler is called.

Let me know if I need to be clearer since it's all a bit magical.

I'm not sure why it matters when UseExceptionHandler is called since it's looking for "__WebApplicationBuilder" anyway and that's always defined. The ExceptionHandlerMiddleware will always be added later in the pipeline with the call _builtApplication.BuildRequestDelegate().

Can you provide a test case where there's already an IEndpointRouteBuilder in app.Properties here? It's not easy for code in Program.cs to get access to this app instance. The only thing I could think of is an IStartupFilter or something like that. If we really care about a startup filter calling app.UseRouting() before calling next(app), we could skip calling UseRouting() altogether. I'm not convinced that's realistic though, so I'm not sure it's worth complicating the code for for this. If we do, we should add a regression test.

Hmm now I think about it, I don't recall why I set the argument to false here. However, I do clearly recall without it, things didn't work. I could debug it to come up with a more clear explanation.

That being said, after chatting with @javiercn today, I'm not sure if this is the approach we are going to take so I think it's a moot point. I think there's more investigation/design to be done before addressing this specific feedback.

captainsafia · 2021-08-05T20:49:13Z

            ((IConfigurationBuilder)Configuration).Sources.Clear();
            Configuration.AddConfiguration(_builtApplication.Configuration);

+            _builtApplication.Properties[WebApplicationBuilderKey] = true;


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

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

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

Kahbazi · 2021-08-06T05:12:34Z

This method also needs to be updated.

aspnetcore/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerExtensions.cs

Lines 26 to 34 in 1f2d58b

    
           public static IApplicationBuilder UseExceptionHandler(this IApplicationBuilder app) 
        
           { 
        
               if (app == null) 
        
               { 
        
                   throw new ArgumentNullException(nameof(app)); 
        
               } 
        
               return app.UseMiddleware<ExceptionHandlerMiddleware>(); 
        
           }

for the case when ExceptionHandlerOptions is configured via service collection.

aspnetcore/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerServiceCollectionExtensions.cs

Lines 20 to 32 in 1f2d58b

    
           public static IServiceCollection AddExceptionHandler(this IServiceCollection services, Action<ExceptionHandlerOptions> configureOptions) 
        
           { 
        
               if (services == null) 
        
               { 
        
                   throw new ArgumentNullException(nameof(services)); 
        
               } 
        
               if (configureOptions == null) 
        
               { 
        
                   throw new ArgumentNullException(nameof(configureOptions)); 
        
               } 
        
               return services.Configure(configureOptions); 
        
           }

BrennanConroy · 2021-08-17T23:08:38Z

Closing in favor of #35426

ghost added the area-runtime label Aug 3, 2021

JunTaoLuo changed the title ~~Johluo/diagnostic option3~~ Update ExceptionHandler middleware pipenline construction to reinvoke UseRouting in error branch Aug 3, 2021

JunTaoLuo commented Aug 3, 2021

View reviewed changes

Update ExceptionHandler middleware pipenline construction to reinvoke…

575aa50

… UseRouting in error branch

JunTaoLuo force-pushed the johluo/diagnostic-option3 branch from 7482973 to 575aa50 Compare August 3, 2021 17:13

JunTaoLuo commented Aug 3, 2021

View reviewed changes

Update baselines

d469012

davidfowl reviewed Aug 3, 2021

View reviewed changes

BrennanConroy changed the title ~~Update ExceptionHandler middleware pipenline construction to reinvoke UseRouting in error branch~~ Update ExceptionHandler middleware pipeline construction to reinvoke UseRouting in error branch Aug 3, 2021

davidfowl requested review from captainsafia, halter73 and javiercn August 4, 2021 00:16

Kahbazi reviewed Aug 4, 2021

View reviewed changes

Comment thread src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs Outdated

Add routing tests

596f8b7

JunTaoLuo force-pushed the johluo/diagnostic-option3 branch from 90450b1 to 596f8b7 Compare August 4, 2021 18:17

John Luo added 2 commits August 4, 2021 11:39

Add a property for MinimalAPI scenarios

fbd0d40

Merge branch 'main' into johluo/diagnostic-option3

1f2d58b

JunTaoLuo marked this pull request as ready for review August 4, 2021 18:45

JunTaoLuo requested review from BrennanConroy and Tratcher as code owners August 4, 2021 18:45

halter73 reviewed Aug 4, 2021

View reviewed changes

captainsafia reviewed Aug 5, 2021

View reviewed changes

fb and rewrite wip

1a0eea1

BrennanConroy mentioned this pull request Aug 17, 2021

Update middleware that assumes UseRouting is called after them, for minimal hosting #35426

Merged

BrennanConroy closed this Aug 17, 2021

BrennanConroy deleted the johluo/diagnostic-option3 branch August 17, 2021 23:08

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023

Conversation

JunTaoLuo commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JunTaoLuo commented Aug 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kahbazi commented Aug 6, 2021

Uh oh!

BrennanConroy commented Aug 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

JunTaoLuo commented Aug 3, 2021 •

edited

Loading

Tratcher Aug 4, 2021 •

edited

Loading

JunTaoLuo Aug 4, 2021 •

edited

Loading

JunTaoLuo Aug 4, 2021 •

edited

Loading

halter73 Aug 5, 2021 •

edited

Loading