Skip to content
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

Make RouteAttribute non-inherited #10236

Merged
merged 2 commits into from
May 14, 2019
Merged

Make RouteAttribute non-inherited #10236

merged 2 commits into from
May 14, 2019

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented May 14, 2019

Fixes: #5529

Inheriting and looking for inherited route attributes will cause nothing
but trouble. We had a bug tracking what to do about this and we decided
to make it really clear that routes are not inherited.

Previously the attribute was marked as inherited, but we woulnd't look
for inherited routes.

Fixes: #5529

Inheriting and looking for inherited route attributes will cause nothing
but trouble. We had a bug tracking what to do about this and we decided
to make it really clear that routes are not inherited.

Previously the attribute was marked as inherited, but we woulnd't look
for inherited routes.
@rynowak rynowak requested a review from SteveSandersonMS May 14, 2019 16:35
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Would be great to have an extra unit test in RouteTableTests to represent this.

Besides that, looks great!

@rynowak
Copy link
Member Author

rynowak commented May 14, 2019

Added some tests

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 14, 2019
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Components
/// <summary>
/// Indicates that the associated component should match the specified route template pattern.
/// </summary>
[AttributeUsage(AttributeTargets.Class, AllowMultiple = true, Inherited = true)]
[AttributeUsage(AttributeTargets.Class, AllowMultiple = true, Inherited = false)]
public class RouteAttribute : Attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it sealed? I think you're allowed to redefine AttributeUsage on derived types so you could get in to the same problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm that's wierd that we didn't do this already. Since github didn't show your comment until after I merged, I'm going to add this to the API review item.

@rynowak rynowak merged commit d794c52 into master May 14, 2019
@ghost ghost deleted the rynowak/fix-5529 branch May 14, 2019 22:37
@rynowak rynowak mentioned this pull request May 14, 2019
20 tasks
@gsnoff
Copy link

gsnoff commented Mar 23, 2020

@rynowak @SteveSandersonMS One possible legitimate scenario with inherited route templates I could see, is when the API scheme is using API version in the URL path, and one would like to avoid duplicating the same [Route("api/v{version:apiVersion}/[controller]")] attribute in every versioned API controller in order to uphold the DRY principle.

Right now I’m having to resort to a IEndpointRouteBuilder.MapControllerRoute(...) call in my Startup class to configure something like that, which just isn’t that flexible when my controllers would have vastly different sets of action endpoints.

@rynowak
Copy link
Member Author

rynowak commented Mar 23, 2020

This issue was related to Blazor. What you're suggesting with work fine with MVC.

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.

Router does not find inherited RouteAttribute
5 participants