dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

[API Proposal]: Add SkipStatusCodePages property to ApiBehaviorOptions #45369

Open alienwareone opened 1 year ago

alienwareone commented 1 year ago

Background and Motivation

When mixing MVC Controllers with Api Controllers in an application, there should be a global setting in ApiBehaviorOptions to turn off the IStatusCodePagesFeature for Api Controllers to avoid overriding the raw api response with an MVC view/html based status code page implementation.

Proposed API

namespace Microsoft.AspNetCore.Mvc;

public class ApiBehaviorOptions : IEnumerable<ICompatibilitySwitch>
{
+    public bool SkipStatusCodePages { get; set; }
}

Usage Examples

// Program.cs Minimal API

var builder = WebApplication.CreateBuilder(args);

// Register services

app.AddMvc().ConfigureApiBehaviorOptions(options =>
{
    options.SkipStatusCodePages = true;
});

var app = builder.Build();

// Register middlewares

app.MapControllers();

await app.RunAsync();

Alternative Designs

public static class SkipStatusCodePagesMetadataExtensions
{
    public static IEndpointConventionBuilder SkipStatusCodePagesForApiControllers(this IEndpointConventionBuilder builder)
    {
        builder.Add(endpointBuilder =>
        {
            var apiControllerAttribute = endpointBuilder.Metadata.FirstOrDefault(m => m.GetType() == typeof(ApiControllerAttribute)) as ApiControllerAttribute;

            if (apiControllerAttribute == null)
            {
                return;
            }

            endpointBuilder.Metadata.Add(new SkipStatusCodePagesMetadata());

            endpointBuilder.FilterFactories.Add((context, next) =>
            {
                return async context =>
                {
                    var statusCodeFeature = context.HttpContext.Features.Get<IStatusCodePagesFeature>();

                    if (statusCodeFeature != null)
                    {
                        // Turn off the StatusCodePages feature.
                        statusCodeFeature.Enabled = false;
                    }

                    return await next(context);
                };
            });
        });

        return builder;
    }
}

 // Marker metadata class
file class SkipStatusCodePagesMetadata : ISkipStatusCodePagesMetadata
{
}
// Program.cs Minimal API

var builder = WebApplication.CreateBuilder(args);

// Register services

app.AddMvc();

var app = builder.Build();

// Register middlewares

app.MapControllers().SkipStatusCodePagesForApiControllers();

await app.RunAsync();

Risks

There are no risks because the default value for ApiBehaviorOptions.SkipStatusCodePages will be false which is the current behavior.

ghost commented 1 year ago

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

halter73 commented 1 year ago

@alienwareone I don't think you need to define a complicated SkipStatusCodePagesForApiControllers method that adds filters or anything like that. Doesn't the following already work?

app.MapControllers().WithMetadata(new SkipStatusCodePagesAttribute());

It looks like the middleware is already checking for the ISkipStatusCodePagesMetadata interface implemented by the attribute.

https://github.com/dotnet/aspnetcore/blob/600eb9aa53c052ec7327e2399744215dbe493a89/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesMiddleware.cs#L44-L49

You just have to be careful to call UseStatusCodePages after UseRouting. This is probably a good reason to make writing an analyzer to flag this misordering a higher priority. We already have #38591 tracking this.

What do we think about adding a new IEndpointConventionBuilder extension method that did exactly this called .SkipStatusCodePages() instead? It's similar to your alternate proposal, but it should just work for everything including minimal route handlers and route groups.

alienwareone commented 1 year ago

You just have to be careful to call UseStatusCodePages after UseRouting. This is probably a good reason to make writing an analyzer to flag this misordering a higher priority. We already have #38591 tracking this.

@halter73 Thanks for pointing that out! With the correct ordering the need to add a filter is not required.

app.MapControllers().WithMetadata(new SkipStatusCodePagesAttribute());

But this will also skip the StatusCodePagesMiddleware for MVC Controllers which is not desired.

Here is a minimal repo about this issue.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

halter73 commented 1 year ago

API Review Notes:

namespace Microsoft.AspNetCore.Mvc;

public class ApiBehaviorOptions : IEnumerable<ICompatibilitySwitch>
{
+    public bool SkipStatusCodePages { get; set; }
}

API Approved as proposed!

onurkanbakirci commented 4 weeks ago

I'm working on it.