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

[mary-georgiou-sonarsource]

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]

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]

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]

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.