New rule S6934: You should specify the RouteAttribute when an HttpMethodAttribute is specified at an action level

[martin-strecker-sonarsource]

Fixes #8872

Remaining:

• RSPEC rework (@martin-strecker-sonarsource)
• add more test cases (including partial classes) (@costin-zaharia-sonarsource
• implement the exception (@costin-zaharia-sonarsource)

[martin-strecker-sonarsource]

Add a non-compliant action here. I'm not sure if secondary locations are filtered or not. I'm also not sure if reporting a secondary location in a generated file is a problem or not. But it would be interesting to learn.

[costin-zaharia-sonarsource]

Good point. These are not filtered.

[costin-zaharia-sonarsource]

I will not change the behavior, I will just document it.

[antonioaversa]

I am almost done with the review. I am releasing a bunch of comments, so that they can be addressed while I finish the review. The rule looks very good.

[antonioaversa]

@costin-zaharia-sonarsource Coverage is almost 100%. I think we should be able to cover if (declaration.GetIdentifier() is { } identifier), maybe with incomplete code having a method missing its name.

[costin-zaharia-sonarsource]

Coverage is almost 100%. I think we should be able to cover if (declaration.GetIdentifier() is { } identifier), maybe with incomplete code having a method missing its name.

I have mixed feelings about this. Do you see a specific risk that we take if we don't cover this case?

[antonioaversa]

LGTM!
I have left a few comments, but those are mostly on tests, and there is no need for another round of review. Once addressed or replied to, the PR can be merged.

[antonioaversa]

Very minor concern. I have tried the following out:

public class SomeController : Controller
{
    [Route(" ")]
    public IActionResult WithRouteAttribute() => View();
}

It registers the route https://localhost:7010/%20. The same happens with [Route("/t")]
So technically, the rule would apply for any non-null template, including space, tabs, and probably other whitespace.

[costin-zaharia-sonarsource]

Ok, I've changed the behavior.

[antonioaversa]

We don't seem to have tests for:

• routes with whitespace chars: space and tab in particular
• multiple attributes on the same action
  • one attribute with a non-empty route template and one without (non-compliant)
  • both attributes without non-empty route templates (compliant)
  • both attributes with non-empty route templates (non-compliant)
  • mix of HttpMethodAttribute and RouteAttribute

[costin-zaharia-sonarsource]

We have already some test cases with a mix:

I'll check and add the others

[antonioaversa]

Personal opinion: I would use the name of the method to convey the meaning for the test. For example:

• Index -> WithoutHttpAttribute
• Index2 -> WithHttpAttributeNoRoute
• Index3 -> WithHttpAttributeAndArgumentListNoRoute
• etc.

This way the intent for the test becomes clearer, and the list is also easy to rearrange, since the increasing numbering doesn't have to be honored.

[antonioaversa]

I would add the following, to document that the "null or whitespace" behavior also applies to Route:

    [Route(" ")]
    public IActionResult WithSpaceRouteInRouteAttribute() => View();  // Compliant

    [Route("\t")]
    public IActionResult WithTabRouteInRouteAttribute() => View();  // Compliant

[costin-zaharia-sonarsource]

I've made them noncompliant, according to: #8954 (comment)

[antonioaversa]

The test should have the _CSharp12 suffix, instead of _CS.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
98.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[antonioaversa]

Coverage is almost 100%. I think we should be able to cover if (declaration.GetIdentifier() is { } identifier), maybe with incomplete code having a method missing its name.

I have mixed feelings about this. Do you see a specific risk that we take if we don't cover this case?

I missed this question. I think it's fine. I tried to do something like the following:

class X {
	virtual void (string x) { }
}

But I got an IncompleteMemberSyntax, not a MethodDeclarationSyntax:

You can have a missing identifier in a DestructorDeclarationSyntax:

class X {
	~() { }
}

However, DestructorDeclarationSyntax derives from BaseMethodDeclarationSyntax, not from MethodDeclarationSyntax.
Similar reasoning applies to all other production rules of base_method_declaration.

Moreover, the identifier is not optional in the production rule of method_declaration.

So I don't see an easy way of generating a case where the Identifier is null.
That means that, in principle, we could use declaration.GetIdentifier() is var identifier, but being defensive is also OK in this case, I think.