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 FN: rule should raise in case of value type property annotated with RequiredAttribute #9263

Open
hugoqribeiro opened this issue May 8, 2024 · 3 comments · May be fixed by #9313
Open
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: Improvement Making existing code better.
Projects
Milestone

Comments

@hugoqribeiro
Copy link

Description

S6964 is reporting an error on value type model properties.

Repro steps

Here is a value type model property that is triggering the rule:

public class ChangeEmailCodeViewModel : VerificationCodeViewModelBase
{
  public int CanRetry
  {
      get;
      set;
  }

  // (...)
}

Expected behavior

The rule should not be trigger for value type properties.

Actual behavior

See above.

Known workarounds

None.

Related information

  • SonarAnalyzer.CSharp version 9.25.0.90414
  • Visual Studio 17.9.6
  • .NET 8.0.204
  • Windows 10
@hugoqribeiro hugoqribeiro changed the title Fix S6964 FP: rule trigger for value type properties Fix S6964 FP: rule triggered for value type properties May 10, 2024
@mary-georgiou-sonarsource
Copy link
Contributor

Hello @hugoqribeiro,
This rule raises on value type properties of classes if they are used in a controller action, as input, and are not required or nullable.
If, for example, the value type properties are not set while posting, this leads to under posting and might lead to issues due to the value types ending up getting their default values.

From my point of view, this is not a false positive, but I might be missing some context regarding your code base.

@hugoqribeiro
Copy link
Author

I see your point if you think of [Required] as metadata.

In my opinion, [Required] is foremost for validation and there is no way to implement [Required] on value type properties (as there will always be a value)... That's why people don't annotate these properties with [Required].

@mary-georgiou-sonarsource
Copy link
Contributor

@hugoqribeiro I checked the rule implementation and you are right. The rule should ask for nullable and required at the same time (with required not being mandatory - but as metadata) - or [JsonRequired] in the case of a web API controller action that accepts a model object as input in JSON form(if this can be detected).

I will add this ticket to our backlog, and we'll handle it soon as we are working now on hardening the new ASP.NET rules.

Thanks a lot for taking the time to report this and the other issues!

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: rule triggered for value type properties Fix S6964 FP: Rule should ask only for nullable or JsonRequired May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added Sprint: Hardening Fix FPs/FNs/improvements Area: C# C# rules related issues. Type: Improvement Making existing code better. labels May 21, 2024
@github-actions github-actions bot added this to To do in Best Kanban May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Rule should ask only for nullable or JsonRequired Rule S6964 should recommend to turn a value type property to nullable or annotate it with Json required 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
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Rule S6964 should recommend to turn a value type property to nullable or annotate it with Json required Fix S6964 FN: rule should raise in case of value type property annotated with RequiredAttribute May 22, 2024
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 22, 2024
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban May 24, 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: Improvement Making existing code better.
Projects
Best Kanban
  
Review approved
3 participants