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

Constructor parameters should match property names #33796

Open
Tracked by #57797
terrajobst opened this issue Mar 19, 2020 · 32 comments · May be fixed by dotnet/roslyn-analyzers#4877
Open
Tracked by #57797

Constructor parameters should match property names #33796

terrajobst opened this issue Mar 19, 2020 · 32 comments · May be fixed by dotnet/roslyn-analyzers#4877
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Mar 19, 2020

In general, constructor arguments who initialize should match properties (casing aside). There are also cases where serializers will match properties to constructor parameters in order to construct immutable types. This would address that too.

We should limit this to types/constructs we consider being used by the JSON serializer.

Category: Design

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@jeffhandley jeffhandley added this to the 5.0 milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Medium
  • Fixer: Small

@jeffhandley
Copy link
Member

We need to detect an inclusion approach; we want to focus on the JSON deserialization scenario.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 23, 2020
@bartonjs
Copy link
Member

bartonjs commented Mar 24, 2020

This should only apply to constructors painted with the [JsonConstructor] attribute, but should be prepared to account for other enrollment scenarios.

  • For each parameter name in the parameter list of an included constructor
    • If there is not a property on the type hierarchy at that level whose name is OrdinalIgnoreCase equal to the parameter name, report a diagnostic against the parameter (ideally just the name).

If the engine already has a "close match" name detector, then a fixer could suggest correcting a typo; but otherwise a fixer seems too open of a problem (unless other bright ideas surface).

@jeffhandley
Copy link
Member

For existing violations in runtime, we likely would not apply the fix since changing parameter names would often be a breaking change.

@swtrse
Copy link

swtrse commented Jun 24, 2020

It should also considered that the JsonPropertyAttribute should be able to decorate constructor parameters.
This way the exact name in case sensitive format can be defined for the constructor parameters and the actual parameter name does not matter at all.
If done in an other way this would be fine too but there should be a way to exacly define the json names for the parameters in an case sensitive way.

@bartonjs
I strongly opose the option where the match is done with an IgnoreCase equal at least when the global options are not set to case insensitive name resolution.

@danmoseley
Copy link
Member

@jeffhandley is this 5.0 work?

@jeffhandley jeffhandley modified the milestones: 5.0.0, Future Jul 30, 2020
@jeffhandley
Copy link
Member

@danmosemsft Moved to Future. Thanks.

We will target delivering this off-cycle soon after 5.0 since it will be good to get this out before folks start producing code that will violate the rule.

@psxvoid
Copy link

psxvoid commented Feb 5, 2021

Hi,

May I "book" this issue as "up-for-grabs"?

@danmoseley
Copy link
Member

@psxvoid you are welcome, I assigned it to you. (@buyaa-n I am assuming we are not working on this since it's not assigned)

@danmoseley danmoseley removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 5, 2021
@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 5, 2021

I am assuming we are not working on this since it's not assigned

That is right

@psxvoid
Copy link

psxvoid commented Feb 6, 2021

Thank you.

I've spent some time on initial planning, and I'm wondering whether the following questions can be addressed before the implementation:

  1. Should this analyzer also check field names? See note (3).
  2. Should this analyzer be only applied to constructors marked as "JsonConstructor" from "System.Text.Json.Serialization" namespace, and not from "Newtonsoft.Json"?
  3. What target namespace should be used for this analyzer? See note (5).

Notes:
(1) I'm assuming this analyzer is also must be applied to C# 9 Records (and structs). This is a valid usage:

public record Person
{
   public string FirstName { get; init; }
   public string LastName { get; init; }

   [JsonConstructor]
   public Person(string firstName, string lastName)
     => (FirstName, LastName) = (firstName, lastName);
}

(2) I'm assuming this analyzer should also be applied to VB.

(3) This Migrate from Newtonsoft.Json to System.Text.Json - .NET | Microsoft Docs doc says:

In System.Text.Json, use the JsonSerializerOptions.IncludeFields global setting or the [JsonInclude] attribute to include public fields when serializing or deserializing. For an example, see Include fields.

(4) It seems like uninitialized public properties are not a responsibility of this analyzer. Example:

public class C
{
    public int P1 { get; }
    public int? P2 { get; }

    [JsonConstructor]
    public C(int p1)
    {
        this.P1 = p1;
        // P2 is not initialized
    }
}

(5) It seems like this analyzer belongs to the "Design" category. As far as I can see, most of them reside in:

NetAnalyzers\Core\Microsoft.CodeQuality.Analyzers\ApiDesignGuidelines

but this issue is also marked as area-System.Runtime label, and it could be also applicable to VB. Because of that, should it be placed here:

NetAnalyzers\Core\Microsoft.NetCore.Analyzers\Runtime

@tannergooding tannergooding added this to the Future milestone Jul 12, 2021
@jeffhandley jeffhandley modified the milestones: Future, 7.0.0 Aug 20, 2021
@carlossanlop carlossanlop added the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2021
@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Jul 9, 2022
@SoggyBottomBoy
Copy link

If a property is tagged with the JsonPropertyName which has a value matching the constructor param name, should this not find a match?

public class Example
{
    [JsonConstructor]
    public Example(string Name)
    {
        FirstName = Name;
    }

    [JsonPropertyName("Name")]
    public string FirstName { get; }
}

public class ExampleTest
{
    [Test]
    public void JsonRoundTrip()
    {
        var test = new Example("Joe");
        string output = JsonSerializer.Serialize(test);
        var result = JsonSerializer.Deserialize<Example>(output);

        Assert.Equals(test.FirstName, result.FirstName);
    }
}

Running the test above gives:
System.InvalidOperationException: 'Each parameter in the deserialization constructor on type 'Tests.Serialization.Example' must bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. The match can be case-insensitive.'

I'm trying to add json attributes to an existing library in which the models (combination of structs and classes), don't always have the property name matching the constructor parameter.

@ghost
Copy link

ghost commented Nov 1, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

In general, constructor arguments who initialize should match properties (casing aside). There are also cases where serializers will match properties to constructor parameters in order to construct immutable types. This would address that too.

We should limit this to types/constructs we consider being used by the JSON serializer.

Category: Design

Author: terrajobst
Assignees: psxvoid
Labels:

api-approved, area-System.Text.Json, code-analyzer, code-fixer, in-pr

Milestone: Future

@eiriktsarpalis
Copy link
Member

@carlossanlop was the in-pr label applied correctly? I couldn't a find a PR linked to this issue.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 5, 2023

The PR is in roslyn-analyzers repo: dotnet/roslyn-analyzers#4877

@eiriktsarpalis
Copy link
Member

@buyaa-n could you link the PR to this issue? I don't seem to have permissions in the roslyn-analyzers repo.

@carlossanlop
Copy link
Member

@carlossanlop
Copy link
Member

carlossanlop commented Jul 5, 2023

Someone please correct me if I'm wrong: I don't think we can link issues to PRs across repos. That's why I had to add the label manually.

Edit: I might be wrong, I can see this in the PR:

image

@eiriktsarpalis
Copy link
Member

Yeah I think it's only possible if you explicitly add a Fix <link> statement in the OP of the PR.

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 14, 2023

As there were no progress in the PR moving into future milestone, CC @psxvoid.

@buyaa-n buyaa-n modified the milestones: 8.0.0, Future Aug 14, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Aug 14, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: 9.0.0, Future Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.