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

Fix S6964 FP: Should not raise for reference properties in nullable context #9284

Closed
cremor opened this issue May 16, 2024 · 3 comments · Fixed by #9308
Closed

Fix S6964 FP: Should not raise for reference properties in nullable context #9284

cremor opened this issue May 16, 2024 · 3 comments · Fixed by #9308
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@cremor
Copy link

cremor commented May 16, 2024

Description

The rule S6964 should not be active for model properties in a nullable aware context (where nullable reference types are enabled). This is because of:

By default, MVC will treat a non-nullable reference type parameters and properties as-if [Required] has been applied, resulting in validation errors when no value was bound.

Source: https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.mvcoptions.suppressimplicitrequiredattributefornonnullablereferencetypes

Even if a project sets this property to true, the rule should not be reported in my opinion. Because then it was an explicit decision made by the developer (e.g. because the project uses a different validation framework like FluentValidation).

Repro steps

  1. Create a new ASP.NET Core Web API project (nullable reference types are enabled by default in current .NET versions)
  2. Add a model with not-nullable reference type properties (e.g. string) and use it in a POST controller method.
  3. Call the POST method without providing the property. See that ASP.NET Core returns an validation error, even though you didn't apply the Required attribute manually.

Expected behavior

Rule should not be reported.

Actual behavior

Rule is reported.

Known workarounds

I had to create a new quality profile in SonarCloud to disable the rule completely.

Related information

  • C#/VB.NET Plugins version: 9.25.0.90414 (used by the SonarCloudPrepare@1 task)
  • Visual Studio version: not relevant
  • MSBuild / dotnet version: 8.0.300
  • SonarScanner for .NET version: SonarScanner for MSBuild 5.15 (used by the SonarCloudAnalyze@1 task)
  • Operating System: windows-latest build agent on Azure DevOps
@mary-georgiou-sonarsource
Copy link
Contributor

Hello @cremor and thanks for reporting this.

The rule should be raised only in value-type properties, not on reference-type properties.
Could you please provide me with a code snippet where you mark where the issue is raised?

Thanks a lot!

@cremor
Copy link
Author

cremor commented May 21, 2024

Here is an example:

public sealed record class TodoItemCreateRequest
{
    public required string Title { get; init; }

    public string? Description { get; init; }
}

grafik

Note that this is a double false positive, the other one is #9285

@mary-georgiou-sonarsource
Copy link
Contributor

mary-georgiou-sonarsource commented May 21, 2024

Hey, @cremor - it should not raise on a string, indeed - regardless of the nullable context.
EDIT: It should not raise for reference types in nullable context.
Thanks!

I've added this issue to our backlog for this sprint.

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Rule shouldn't be active in a nullable aware context Fix S6964 FP: Should not raise for string properties May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements labels May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.26 milestone May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Should not raise for string properties Fix S6964 FP: Should not raise for reference properties in nullable context May 22, 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: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants