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.

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

Conversation

huan086
Copy link
Contributor

@huan086 huan086 commented May 23, 2019

Summary of the changes

  • Change SkipStatusCodePagesAttribute to inherit from IAlwaysRunResultFilter instead of IResourceFilter
  • Add IOrderedFilter to SkipStatusCodePagesAttribute so that callers can ensure it executes first

Addresses #10317

@dnfclas
Copy link

dnfclas commented May 23, 2019

CLA assistant check
All CLA requirements met.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 23, 2019
@huan086 huan086 force-pushed the feature/always-run-SkipStatusCodePagesAttribute branch from 336e1d3 to f94334c Compare May 24, 2019 02:39
@huan086 huan086 force-pushed the feature/always-run-SkipStatusCodePagesAttribute branch from f94334c to 6f08099 Compare June 2, 2019 14:49
@@ -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

@huan086
Copy link
Contributor Author

huan086 commented Jun 13, 2019

Cool, let me revert the changes and follow the way CorsMiddleware does it

@mkArtakMSFT
Copy link
Contributor

Thanks for your effort, @huan086 .
Do you plan to resolve @pranavkm 's comments here, or should this be closed?

@huan086
Copy link
Contributor Author

huan086 commented Jul 24, 2019

I haven't have the time to make the necessary changes yet

@pranavkm
Copy link
Contributor

Feel free to re-send this PR once you've had a chance to update it per PR comments.

@pranavkm pranavkm closed this Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants