Skip to content

Change SkipStatusCodePagesAttribute to inherit from IAlwaysRunResultFilter #10490

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.

Sign up for GitHub

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 2 commits into from
Closed

Change SkipStatusCodePagesAttribute to inherit from IAlwaysRunResultFilter #10490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d77c3b8
Change SkipStatusCodePagesAttribute to inherit from IAlwaysRunResultF…
huan086 May 23, 2019
6f08099
Add IOrderedFilter to SkipStatusCodePagesAttribute so that callers ca…
huan086 Jun 2, 2019
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: 4 additions & 3 deletions src/Mvc/Mvc.ViewFeatures/ref/Microsoft.AspNetCore.Mvc.ViewFeatures.netcoreapp3.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,12 @@ public virtual void AddValidation(Microsoft.AspNetCore.Mvc.ModelBinding.Validati
public override bool IsValid(object value) { throw null; }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Method, AllowMultiple=false, Inherited=true)]
public partial class SkipStatusCodePagesAttribute : System.Attribute, Microsoft.AspNetCore.Mvc.Filters.IFilterMetadata, Microsoft.AspNetCore.Mvc.Filters.IResourceFilter
public partial class SkipStatusCodePagesAttribute : System.Attribute, Microsoft.AspNetCore.Mvc.Filters.IAlwaysRunResultFilter, Microsoft.AspNetCore.Mvc.Filters.IFilterMetadata, Microsoft.AspNetCore.Mvc.Filters.IOrderedFilter, Microsoft.AspNetCore.Mvc.Filters.IResultFilter
{
public SkipStatusCodePagesAttribute() { }
public void OnResourceExecuted(Microsoft.AspNetCore.Mvc.Filters.ResourceExecutedContext context) { }
public void OnResourceExecuting(Microsoft.AspNetCore.Mvc.Filters.ResourceExecutingContext context) { }
public int Order { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public void OnResultExecuted(Microsoft.AspNetCore.Mvc.Filters.ResultExecutedContext context) { }
public void OnResultExecuting(Microsoft.AspNetCore.Mvc.Filters.ResultExecutingContext context) { }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Property, Inherited=true, AllowMultiple=false)]
public sealed partial class TempDataAttribute : System.Attribute
Expand Down
16 changes: 12 additions & 4 deletions src/Mvc/Mvc.ViewFeatures/src/SkipStatusCodePagesAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,23 @@ namespace Microsoft.AspNetCore.Mvc
/// A filter that prevents execution of the StatusCodePages middleware.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter
public class SkipStatusCodePagesAttribute : Attribute, IAlwaysRunResultFilter, IOrderedFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this change makes sense, this does

a) is a breaking change
b) does not work with endpoint routing

We should leave the IResourceFilter on there since it does not hurt to invoke the filter in multiple stages and allows it to be non-breaking. In addition, as of 3.0, Auth happens as part of the middleware pipeline, so this change really doesn't help there. If you'd like to solve it for the latter, one solution would be to:

a) Introduce an ISkipStatusCodePagesMetadata
b) Use the presence of the metadata to skip the status code page if you're doing endpoint routing. Here's a similar pattern: https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs#L140-L159

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to consider skipping solving this for endpoint metadata, let me know and I can file a separate issue to track it. cc @JamesNK \ @rynowak \ @Tratcher

{
/// <inheritdoc />
public void OnResourceExecuted(ResourceExecutedContext context)
/// <summary>
/// Gets or sets the order value for determining the order of execution of filters. Filters execute in
/// ascending numeric value of the <see cref="Order" /> property.
/// </summary>
public int Order { get; set; } = -1000;

/// <inheritdoc />
public void OnResultExecuted(ResultExecutedContext context)
{
// Intentionally empty.
}

/// <inheritdoc />
public void OnResourceExecuting(ResourceExecutingContext context)
public void OnResultExecuting(ResultExecutingContext context)
{
if (context == null)
{
Expand All @@ -34,4 +42,4 @@ public void OnResourceExecuting(ResourceExecutingContext context)
}
}
}
}
}
21 changes: 10 additions & 11 deletions src/Mvc/Mvc.ViewFeatures/test/SkipStatusCodePagesAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
// 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.Collections.Generic;
using Microsoft.AspNetCore.Diagnostics;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Routing;
using Xunit;

Expand All @@ -19,12 +17,12 @@ public void SkipStatusCodePagesAttribute_TurnsOfStatusCodePages()
{
// Arrange
var skipStatusCodeAttribute = new SkipStatusCodePagesAttribute();
var resourceExecutingContext = CreateResourceExecutingContext(new IFilterMetadata[] { skipStatusCodeAttribute });
var resultExecutingContext = CreateResultExecutingContext(new IFilterMetadata[] { skipStatusCodeAttribute });
var statusCodePagesFeature = new TestStatusCodeFeature();
resourceExecutingContext.HttpContext.Features.Set<IStatusCodePagesFeature>(statusCodePagesFeature);
resultExecutingContext.HttpContext.Features.Set<IStatusCodePagesFeature>(statusCodePagesFeature);

// Act
skipStatusCodeAttribute.OnResourceExecuting(resourceExecutingContext);
skipStatusCodeAttribute.OnResultExecuting(resultExecutingContext);

// Assert
Assert.False(statusCodePagesFeature.Enabled);
Expand All @@ -35,18 +33,19 @@ public void SkipStatusCodePagesAttribute_Does_Not_Throw_If_Feature_Missing()
{
// Arrange
var skipStatusCodeAttribute = new SkipStatusCodePagesAttribute();
var resourceExecutingContext = CreateResourceExecutingContext(new IFilterMetadata[] { skipStatusCodeAttribute });
var resultExecutingContext = CreateResultExecutingContext(new IFilterMetadata[] { skipStatusCodeAttribute });

// Act
skipStatusCodeAttribute.OnResourceExecuting(resourceExecutingContext);
skipStatusCodeAttribute.OnResultExecuting(resultExecutingContext);
}

private static ResourceExecutingContext CreateResourceExecutingContext(IFilterMetadata[] filters)
private static ResultExecutingContext CreateResultExecutingContext(IFilterMetadata[] filters)
{
return new ResourceExecutingContext(
return new ResultExecutingContext(
CreateActionContext(),
filters,
new List<IValueProviderFactory>());
null,
new object());
}

private static ActionContext CreateActionContext()
Expand All @@ -59,4 +58,4 @@ private class TestStatusCodeFeature : IStatusCodePagesFeature
public bool Enabled { get; set; } = true;
}
}
}
}