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: Add more attributes to the exclusions #9337

Closed
denis-troller opened this issue May 27, 2024 · 3 comments · Fixed by #9355
Closed

Fix S6964 FP: Add more attributes to the exclusions #9337

denis-troller opened this issue May 27, 2024 · 3 comments · Fixed by #9355
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

@denis-troller
Copy link

Description

Rule S6964 raises on members that are defined with the [JSonProperty(Require = Always)] attribute.
Since a missing value will cause a deserialization error, the rule should not raise.
Please also consider [JSonProperty(Require = DisallowNull)]

Repro steps

Please provide a minimal code snippet that compiles which reproduces the problem. Alternatively, you can share a link to the SonarCloud public project where the issue appears. If the issue requires some specific project configuration to reproduce, you can create a minimal reproducible project and share its git repository or attach it as a ZIP file.

Example:

/// <summary>This class defines a GetPartnerSubscriptionRequest model.</summary>
[JsonObject(NamingStrategyType = typeof(CamelCaseNamingStrategy))]
public class GetPartnerSubscriptionRequest
{
    /// <summary>Gets or sets the ClientSiteId.</summary>
    [ValidateNever] // Prevents sonar error/warning S6964
    [JsonProperty(Required = Required.Always)]
    public Guid ClientSiteId { get; set; }

    /// <summary>Gets or sets the PartnerId.</summary>
    [ValidateNever] // Prevents sonar error/warning S6964
    [JsonProperty(Required = Required.Always)]
    public Guid PartnerId { get; set; }
}
@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Area: C# C# rules related issues. label May 27, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.26 milestone May 27, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be. labels May 27, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP/FN: use of the JSonProperty(Require = Always) attribute Fix S6964 FP: Don't raise when property is annotated with JSonProperty(Require = Always) attribute May 29, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Don't raise when property is annotated with JSonProperty(Require = Always) attribute Fix S6964 FP: Don't raise when property is annotated with JSonProperty(Require = Always) or Range attribute May 29, 2024
@mary-georgiou-sonarsource
Copy link
Contributor

mary-georgiou-sonarsource commented May 29, 2024

I've added an exception for JSonProperty(Require = Always) and SonProperty(Require = AllowNull) and also for Range (since it's a minor change).

With this PR I'm excluding properties annotated with:

  • JsonProperty(Required = Required.Always)
  • JsonProperty(Required = Required.AllowNull)
  • Newtonsoft.Json.JsonIgnore
  • Newtonsoft.Json.JsonRequired
  • System.Text.Json.Serialization.JsonRequired
  • System.Text.Json.Serialization.JsonIgnore
  • Range

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Don't raise when property is annotated with JSonProperty(Require = Always) or Range attribute Fix S6964 FP: Don't raise when property is annotated with JsonProperty(Require = Always) or Range attribute May 29, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Don't raise when property is annotated with JsonProperty(Require = Always) or Range attribute Fix S6964 FP: Don't raise when property is annotated with Newtonsofr JsonPropertyAttribute or Range attribute May 29, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Don't raise when property is annotated with Newtonsofr JsonPropertyAttribute or Range attribute Fix S6964 FP: Don't raise when property is annotated with Newtonsoft JsonPropertyAttribute or RangeAttribute May 29, 2024
@knopa
Copy link

knopa commented May 30, 2024

It Also has an issue when you have multiple next to required
image

@mary-georgiou-sonarsource
Copy link
Contributor

mary-georgiou-sonarsource commented May 30, 2024

Hello @knopa, thanks for reporting this. This is a different issue. However, this behavior will be fixed with the new release coming in the next few days.
One more, the rule will raise if your value type property is annotated with Required. Required has no effect on value type properties as they always have a value (default one).

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Don't raise when property is annotated with Newtonsoft JsonPropertyAttribute or RangeAttribute Fix S6964 FP: Exclude properties for certain Json.NET attributes and more System.Text.Json.Serialization attributes May 30, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Exclude properties for certain Json.NET attributes and more System.Text.Json.Serialization attributes Fix S6964 FP: Exclude properties for certain Json.NET attributes, more System.Text.Json.Serialization attributes and RangeAttribute May 30, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Fix S6964 FP: Exclude properties for certain Json.NET attributes, more System.Text.Json.Serialization attributes and RangeAttribute Fix S6964 FP: Add more attributes to the exclusions May 30, 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.

4 participants