JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.73k stars 3.25k forks source link

JsonPropertyAttribute: CA1507 appears when using .NET 7 / MSBuild 17.5 #2826

Open adiessl opened 1 year ago

adiessl commented 1 year ago

We have a fairly large solution with multiple projects using Newtonsoft.Json and after upgrading Visual Studio to version 17.5 recently encountered a problem that I would like to bring to your attention and also document it here for others to find.

How to reproduce

The easiest way to reproduce the problem is to have a project targeting .NET 7 that has all .NET Analyzers enabled:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net7.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>

        <EnableNETAnalyzers>true</EnableNETAnalyzers>
        <AnalysisLevel>latest</AnalysisLevel>
        <AnalysisMode>All</AnalysisMode>

        <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    </PropertyGroup>

    <ItemGroup>
      <PackageReference Include="Newtonsoft.Json" Version="13.0.2" />
    </ItemGroup>

</Project>

If this project contains a class like the following,

using Newtonsoft.Json;

namespace ConsoleApp;

internal sealed class DataTransferObject
{
    [JsonProperty("Property")]
    public string? Property { get; set; }
}

the warning CA1507 appears when building:

Warning CA1507 : Use nameof in place of string literal 'Property'

If TreatWarningsAsErrors is enabled, the following error occurs:

Error CA1507 : Use nameof in place of string literal 'Property'

Excerpt from the Microsoft documentation explaining under which circumstances the rule CA1507 fires:

The declared name of the parameter is propertyName and the constant value of the string literal matches the name of a property of the type within which the method or constructor is being invoked.

This is precisely the case for JsonPropertyAttribute used here:

https://github.com/JamesNK/Newtonsoft.Json/blob/57025815e564d36821acf778e2c00d02225aab35/Src/Newtonsoft.Json/JsonPropertyAttribute.cs#L218


It seems that some change within the 17.5 release of MSBuild now makes this rule fire for attributes, which was not the case previously.

Related information:

Possible solutions

  1. Suppress the warning via #pragma: --> Works, but if there are lots of occurrences, a lot of #pragmas have to be used, which makes the source code ugly.
  2. Suppress the warning on project level, e.g. via .editorconfig. --> Works as well, but silences this warning completely, so all valid use-cases (e.g. ArgumentNullException) also disappear.
  3. Do not explicitly set propertyName if it matches the underlying property's name:

    using Newtonsoft.Json;
    
    namespace ConsoleApp;
    
    internal sealed class DataTransferObject
    {
        [JsonProperty]
        public string? Property { get; set; }
    }

    --> The problem with this approach is that a future code refactoring might lead to bugs, e.g. if the property Property is renamed, but the name used in the JSON has to remain "Property" for compatibility with an external API. The person doing the refactoring has to pay attention when doing so, which is prone to errors.

  4. Rename the JsonPropertyAttribute's constructor parameter propertyName to something else, therefore making sure CA1507 does not fire anymore. --> The easiest solution for everybody using Newtonsoft.Json, but propertyName is the correct name for the parameter, finding another, fitting one might not be easy. System.Text.Json uses name in their JsonPropertyNameAttribute constructor: https://github.com/dotnet/runtime/blob/2b5f34218de7ff46dc9c0c37976c7a4abc64fcd2/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonPropertyNameAttribute.cs#L17
sungam3r commented 1 year ago

I't strange that warning is shown only for NSJ.

adiessl commented 1 year ago

Newtonsoft.Json is affected because the circumstances necessary apply to its JsonPropertyAttribute. It can also affect other code, for example:

The attribute

[AttributeUsage(AttributeTargets.Property)]
public sealed class DemoAttribute : Attribute
{
    public DemoAttribute(string propertyName)
    {
        PropertyName = propertyName;
    }

    public string PropertyName { get; }
}

applied to

    [Demo("Property")]
    public string? Property { get; init; }

leads to the same result:

Warning CA1507 : Use nameof in place of string literal 'Property'
elgonzo commented 1 year ago

finding another, fitting one might not be easy. System.Text.Json uses name [...]

name is quite good, in my opinion. If that is not desired (but i could not imagine why), jsonPropertyName should also do the trick...