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

Fix S6934 FP: Attributes implementing IRouteTemplateProvider or inheriting from RouteAttribute #8985

Closed
CaringDev opened this issue Mar 26, 2024 · 7 comments · Fixed by #9316
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@CaringDev
Copy link

CaringDev commented Mar 26, 2024

Description

Using attributes inheriting from RouteAttribute / implementing IRouteTemplateProvider are not considered for S6934

Repro steps

public sealed class BarRouteAttribute() : RouteAttribute("/barrr/[controller]/[action]");

[BarRoute]
public class SomeController : ControllerBase // FP: S6934
{
    [HttpGet("foo")]
    public string Foo() => "Hi";
}

Expected behavior

No S6934

Actual behavior

False positive S6934 for controllers decorated with an attribute inheriting from RouteAttribute / implementing IRouteTemplateProvider

Known workarounds

None

Related information

  • SonarAnalyzer.CSharp 9.23.0.88079
@sebastien-marichal sebastien-marichal changed the title Fix S6934 FP/FN: attributes inheriting from RouteAttribute Fix S6934 FP: attributes inheriting from RouteAttribute Mar 27, 2024
@mary-georgiou-sonarsource
Copy link
Contributor

Hello @CaringDev,

Thank you for reporting this. I confirm it's a false positive.

@mary-georgiou-sonarsource mary-georgiou-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: C# C# rules related issues. labels Mar 27, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6934 FP: attributes inheriting from RouteAttribute Fix S6934 FP: Attributes inheriting from RouteAttribute Mar 27, 2024
@mary-georgiou-sonarsource
Copy link
Contributor

@CaringDev Seeing this example, I'd like to ask what's the use case here so that we can understand better and improve our analyzer.
Why did you create this base class for the attribute?
Thanks!

@CaringDev
Copy link
Author

CaringDev commented Mar 27, 2024

We use two attributes for easy, consistent setup of controllers:

public sealed class ControllerRouteAttribute() : RouteAttribute("api/v{version:apiVersion}/[controller]");
public sealed class ActionRouteAttribute() : RouteAttribute("api/v{version:apiVersion}/[controller]/[action]");

Then, with "find usages", we can immediately see which controllers use "standard" behavior and which ones action method named routing. It also helps to devs (first two are not convention aware):

    [HttpGet("ActionMethod")]
    [HttpGet(nameof(ActionMethod))]
    [HttpGet("[action]")]
    public void ActionMethod() { /* ... */ }

We could also use e.g.

public static class Routes {
    public const string Controller = "api/v{version:apiVersion}/[controller]";
    public const string Action = "api/v{version:apiVersion}/[controller]/[action]";
}

[Route(Routes.Controller)]
public class FooController : ControllerBase { /* ... */ }

The FP should be fixed in any case though... I think there are as many legit use-cases as there are users out there :-)

@costin-zaharia-sonarsource
Copy link
Member

If I understand correctly, we should actually look for all the attributes that implement IRouteTemplateProvider. ASP.NET Core uses the attributes that implement IRouteTemplateProvider to build the initial set of routes.

@mary-georgiou-sonarsource
Copy link
Contributor

mary-georgiou-sonarsource commented Mar 27, 2024

@CaringDev thanks a lot for the detailed explanation.
One last question: Did you consider the usage of Areas for this case?

@CaringDev CaringDev changed the title Fix S6934 FP: Attributes inheriting from RouteAttribute Fix S6934 FP: Attributes inheriting from IRouteTemplateProvider Mar 27, 2024
@CaringDev CaringDev changed the title Fix S6934 FP: Attributes inheriting from IRouteTemplateProvider Fix S6934 FP: Attributes implementing IRouteTemplateProvider Mar 27, 2024
@CaringDev
Copy link
Author

CaringDev commented Mar 27, 2024

Did you consider the usage of Areas for this case?

No, we have several ASP.Net Core WebAPI (non-MVC) projects... would you think Areas would be applicable there as well?
Given our current approach:

namespace Container;

[ControllerRoute]
public class FooController : ControllerBase
{
    [HttpGet]
    public string GetIndex() => "hi";
}

[ActionRoute]
public class BarController : ControllerBase
{
   [HttpPost]
   public void ServeDrinks() { }
}

this would involve changing ControllerRoute and ActionRoute to Area("Controller") and Area("Action") and adding corresponding conventions, no? I doubt there's a benefit here... or are you thinking of something else?

@mary-georgiou-sonarsource
Copy link
Contributor

mary-georgiou-sonarsource commented Mar 27, 2024

Thanks a lot again for the detailed reply, @CaringDev!
I did not have something particular in mind; I was just curious if you evaluated the Areas feature for this use case and, if yes, why you did not go with it.

I guess for this:

Then, with "find usages", we can immediately see which controllers use "standard" behavior and which ones action method named routing.

Areas might not work (but not sure).

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6934 FP: Attributes implementing IRouteTemplateProvider Fix S6934 FP: Attributes implementing IRouteTemplateProvider or inheriting from RouteAttribute May 17, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.26 milestone May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants