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

New rule S6960 for C#: Controllers should not have too many responsibilities #9089

Closed
14 tasks done
mary-georgiou-sonarsource opened this issue Apr 15, 2024 · 3 comments · Fixed by #9111
Closed
14 tasks done
Assignees
Labels
Area: C# C# rules related issues. Sprint: ASP.NET Web API ASP.NET rules for Web API Type: New Rule Implementation for a rule that HAS been specified.
Projects
Milestone

Comments

@mary-georgiou-sonarsource
Copy link
Contributor

RSPEC PR: SonarSource/rspec#3845

Parts:

  • Disjoint Sets primitives: Disjoint Sets primitives #9145
  • Basic implementation: New rule S6960 for C#: Controllers should not have too many responsibilities #9111
    • Support for Func<service> and Lazy<service>: Service should not be well-known
    • Support injected classed with "service" as suffix
    • Support indexers (treated as methods, not as properties)
    • Method overloads are handled as a single group
    • Filter out groups of actions not using any service
    • Change message from "Belongs to responsibility #X" to "May belong to responsibility #X"
    • Fix ASP.NET 4x wrongly detected as good candidate for the rule
    • Exclude IHttpClientFactory (consider as well-known dependency)
    • Fix numbering of responsibilities in presence of unused services
    • Change behavior to include all classes and remove the "service suffix" exception
    • [ ] Introduce S6960IncludeAttribute and S6960ExcludeAttribute
    • Exclude abstract controllers (because of scenarios like this one)
    • Exclude controllers that don't inherit directly from ControllerBase
    • [ ]Exclude inherited methods (because there is no option than to implement them) this is canceled by previous task
@antonioaversa
Copy link
Contributor

antonioaversa commented Apr 30, 2024

Exclude inherited methods (because there is no option for the user than to implement them)

This may reduce the reach a lot, as many of the big projects have hierarchies of controllers. If there is an abstract base class and there is at least an method which is abstract, there is indeed no option for the user and we may want to exclude the controller altogether.

Regarding the issue on this controller: it's both an API and an MVC controller at the same time. This case seems to be a false positive because the overridden methods are abstract two levels above in the hierarchy and those abstract methods are used by public actions at that level.

@denis-troller
Copy link

An issue with static methods in file https://peach.aws-prd.sonarsource.com/issues?projects=umbraco&resolved=false&rules=csharpsquid%3AS6960&open=AY8vNwi3r4ooVA6A4A55

Should we exclude static methods?

@denis-troller
Copy link

In my opinion:

Exclude inherited methods (because there is no option for the user than to implement them)

This may reduce the reach a lot, as many of the big projects have hierarchies of controllers. If there is an abstract base class and there is at least an method which is abstract, there is indeed no option for the user and we may want to exclude the controller altogether.

Regarding the issue on this controller: it's both an API and an MVC controller at the same time. This case seems to be a false positive because the overridden methods are abstract two levels above in the hierarchy and those abstract methods are used by public actions at that level.

For the second case, we should ignore inherited controllers.
In that particular case we should not raise because the class is indeed focused. We have no way to know that in the general case (what if we inherit from an external DLL?) and the easy way out is to ignore it.

IMO we want to help people who are starting to use the framework and push them towards good habits. Someone using inheritance either

  • is a junior dev and should get on their meds again (as Greg would say), we might not be able to help here
  • is a competent dev who knows what they are doing, and we should probably back off.
    This project is a CMS framework (CMS as a lib, to integrate into your project, openiddict-like). It is not your run-off-the-mill project.

Best Kanban automation moved this from In progress to Validate Peach May 2, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from Validate Peach to Done in Best Kanban May 3, 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: ASP.NET Web API ASP.NET rules for Web API Type: New Rule Implementation for a rule that HAS been specified.
Projects
Best Kanban
  
Done
4 participants