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
770 stars 226 forks source link

Fix S6964 FP: Issue is raised for properties in record constructor even if they are annotated as required #9363

Open elnigno opened 3 months ago

elnigno commented 3 months ago

Description

Rule ID: S6964

Repro steps

using System.ComponentModel.DataAnnotations;
public record MyRecord([Required] string MyString);

Expected behavior

The rule should not activate.

Actual behavior

The rule activates.

Known workarounds

-

Related information

mary-georgiou-sonarsource commented 3 months ago

Hello @elnigno and thank you for reporting this. My understanding is that the record is a model property. Could you please provide a reproducer with more context? Thanks a lot!

elnigno commented 3 months ago

Hi @mary-georgiou-sonarsource, Yes, that record is used in a controller action, binding to the model with the FromBody attribute.

[HttpPut]
[Route("{productId}")]
public async Task<IActionResult> DoSomething(
    int productId,
    [FromBody] MyRecord requestBody)
{
    ...
}

Notice this is not exact code that's triggering the rule, because I can't disclose it, but I what I am providing captures the essence of it. Thanks for looking into this!

mary-georgiou-sonarsource commented 3 months ago

Thanks @elnigno.

Your model is the record itself?

elnigno commented 3 months ago

Yes.

mary-georgiou-sonarsource commented 3 months ago

Hello @elnigno! I did some tests with the following example :

[ApiController]
[Route("[controller]")]
public class AController : ControllerBase
{
    [HttpPost("new")]
    public string PostValue([FromBody] RecordModel model) =>$"MyString:{model.MyString}, MyInt: {model.MyInt}";
}

public record RecordModel(string MyString, int MyInt);

The rule correctly raises an issue for MyInt and not for MyString since the first is a value type. However, I see that for records, our rule does not take into consideration the attributes, and this needs to be fixed.

If you agree I'll add this issue to my backlog but with a different title "Raises on record properties even if they are annotated as required".

PS: This rule has been updated in the latest analyzer version and it does not take into account the RequiredAttribute as it has no effect on value types. See here for more info.

elnigno commented 3 months ago

@mary-georgiou-sonarsource sounds good, thanks for looking into it!