SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
797 stars 229 forks source link

Fix S6960 FP: Falsely detects MVC controller as API controller #9684

Closed rjgotten closed 1 month ago

rjgotten commented 1 month ago

Description

S6960 is described as only applying to API controllers, but the rule seems to not care for the (non-)presence of an [ApiController] attribute, which is formally how a controller states that it is an API controller. Instead, it seems to rely on the fact that the controller inherits from ControllerBase and this causes it to also apply to regular MVC controllers inheriting from ControllerBase because they have no need of the support to render views. (E.g. because they only implement post-redirect-get patterns to other controllers.)

Repro steps

Create any controller inheriting from ControllerBase without an [ApiController] attribute on it, and trip the other conditions for S6960 to apply.

Expected behavior

S6960 is not raised, because the controller is not an API controller.

Actual behavior

S6960 is raised, even though the controller is not an API controller.

Known workarounds

Suppress the rule.

Related information

gregory-paidis-sonarsource commented 1 month ago

Hey there, This is by design.

It is quite hard to make sure that a controller is an API controller and not an MVC controller, so this is a heuristic. The problem is that some people might not like what comes with ApiController, for example the automatic payload validation. So we opted for this:

The reason for the second case is that usually, if you implement from ControllerBase, it means that you don't need the View functionality of MVC, so you are probably implementing an API controller. Also, some people forget the ApiController attribute.

...this causes it to also apply to regular MVC controllers inheriting from ControllerBase because they have no need of the support to render views. (E.g. because they only implement post-redirect-get patterns to other controllers.

You can use filters or custom middleware to achieve the same redirection logic, a controller is not needed for this. Apart from that, I don't think you would use a ControllerBase as an MVC controller if you don't need View functionality, as MVC means that you are serving (at least partially) views.

This is open to discussions, of course. Can you think of another case where you would not derive from Controller, but instead from ControllerBase in MVC context?

rjgotten commented 1 month ago

It is quite hard to make sure that a controller is an API controller and not an MVC controller

No it's not. If it has the [ApiController] attribute, it's an API controller. Otherwise it's not. Period. The fact that users think they're creating API controllers by just inheriting from ControllerBase doesn't mean they are. They just get away with it, because topically the most important things kind-of-sort-of work -- thanks to (Or perhaps 'due to'? Matter of perspective?) Microsoft having consciously blurred the line between normal MVC controllers and API controllers. But if you try to use things such as assembly-level default API conventions you'll find out differently.

You can use filters or custom middleware to achieve the same redirection logic

Middleware is not the proper composable solution for endpoints any longer. Endpoint routing is. Controllers are the de-facto easiest way to integrate with those, as they wire up automatically.

If you perform MapControllers() on an endpoint builder it will automatically discover all implementations of IController and wire them up. Using endpoint handlers directly is possible; but requires manually maintaining mappings. On top of that; if you are building an application along a certain line of architecture, it's best for discoverability to other team members as well as - and probably in particular - those freshly on-boarding to not break from that line unless you have to.

Can you think of another case where you would not derive from Controller, but instead from ControllerBase in MVC context?

Basic YAGNI. If I have a controller that doesn't need to do anything special but take in some data and perform a redirect based on that data, then I'd rather use ControllerBase than Controller because:

  1. The intent is to never have it support views; and this makes that separation of concerns clear.
  2. Controller actually does more than just add support for views. It implements also IActionFilter, IAsyncActionFilter and IDisposable. The latter is a small cost, but the former can spend real effort doing nothing, since it involves aspect-oriented programming to run filters before/after execution of the main action method body.
gregory-paidis-sonarsource commented 1 month ago

Microsoft having consciously blurred the line between normal MVC controllers and API controllers

I think that's the main point. Right now, there are three "categories", broadly, for controllers:

This case belongs to the third case.

The fact that users think they're creating API controllers by just inheriting from ControllerBase doesn't mean they are.

Actually, for all senses and purposes, it does. There is no reason to necessarily use the ApiController attribute. This is the convention according to Microsoft for view controllers. Notice that they mention deriving from Controller by convention.

And this is how they hint to the same heuristic we are using in the code:

A controller-based web API consists of one or more controller classes that derive from ControllerBase.

Also here:

Web API controllers should typically derive from ControllerBase rather from Controller. Controller derives from ControllerBase and adds support for views, so it's for handling web pages, not web API requests. If the same controller must support views and web APIs, derive from Controller.

The ApiController attribute is mentioned as optional here:

The [ApiController] attribute can be applied to a controller class to enable the following opinionated, API-specific behaviors.

As Microsoft suggests, it's an optional, opinionated feature that you may or may not want to use.

In any case, I agree that this is a blurry line. This means that we have to go about it in a best-effort way, and statistically speaking, if you inherit from ControllerBase, you are most probably intending to make an API Controller.