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: Do not raise in properties with required modifier #9285

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

Fix S6964 FP: Do not raise in properties with required modifier #9285

cremor opened this issue May 16, 2024 · 3 comments · Fixed by #9307
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.
Projects
Milestone

Comments

@cremor
Copy link

cremor commented May 16, 2024

Description

The rule S6964 should not be active for model properties that use the required keyword. Those properties can never be under-posted because the JSON deserialization for them fails.

Repro steps

  1. Create a new ASP.NET Core Web API project.
  2. Add a model with required properties (e.g. required int) and use it in a POST controller method.
  3. Call the POST method without providing the property. See that ASP.NET Core returns an 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.

The rule does take into consideration the RequiredAttribute.
Could you please provide me with a reproducer so I can investigate further?

Thanks!

@cremor
Copy link
Author

cremor commented May 17, 2024

My issue is about the required C# keyword. Not about the RequiredAttribute class.

@mary-georgiou-sonarsource
Copy link
Contributor

@cremor I misread - my bad. Thanks a lot!

I confirm this is a false positive. We'll tackle it soon, as we plan to harden the ASP.NET rules in the following weeks.

@mary-georgiou-sonarsource mary-georgiou-sonarsource added Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be. labels May 17, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Properties with the required keyword should be ignored Fix S6964 FP: Do not raise in properties with required modifier May 17, 2024
antonioaversa pushed a commit that referenced this issue May 17, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label 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 moved this from To do to In progress in Best Kanban May 21, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 21, 2024
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban May 21, 2024
Best Kanban automation moved this from Review approved to Validate Peach May 22, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from Validate Peach to Done in Best Kanban May 28, 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
Best Kanban
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants