dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

STJ should allow setting naming policies on the member level. #108232

Open eiriktsarpalis opened 1 month ago

eiriktsarpalis commented 1 month ago

Motivation

System.Text.Json supports user-defined naming policies for properties/fields which can be specified via the JsonSerializerOptions.PropertyNamingPolicy. Today such policies can only be specified globally which removes the ability to apply granular policies on the individual type/property/field level.

API Proposal

namespace System.Text.Json.Serialization;

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
public class JsonNamingPolicyAttribute : JsonAttribute
{
    public JsonNamingPolicy(JsonKnownNamingPolicy namingPolicy);
    protected JsonNamingPolicy(JsonNamingPolicy namingPolicy); // protected ctor for user-defined extensibility

    public JsonNamingPolicy NamingPolicy { get; }
}

API Usage

JsonSerializerOptions options = new() { PropertyNamingPolicy = JsonNamingPolicy.SnakeCaseLower };
JsonSerializer.Serialize(new MyPoco(), options); // { "myFirstProperty" : "first", "my-second-property": "second" }

[JsonNamingPolicy(JsonKnownNamingPolicy.CamelCase)]
public class MyPoco
{
    public string MyFirstProperty { get; set; } = "first";

    [JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
    public string MySecondProperty { get; set; } = "second";
}

cc @stephentoub

dotnet-policy-service[bot] commented 1 month ago

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

terrajobst commented 1 month ago

Video

namespace System.Text.Json.Serialization;

[AttributeUsage(AttributeTargets.Class |
                AttributeTargets.Struct |
                AttributeTargets.Interface |
                AttributeTargets.Property |
                AttributeTargets.Field, AllowMultiple = false)]
public class JsonNamingPolicyAttribute : JsonAttribute
{
    public JsonNamingPolicy(JsonKnownNamingPolicy namingPolicy);
    protected JsonNamingPolicy(JsonNamingPolicy namingPolicy);
    public JsonNamingPolicy NamingPolicy { get; }
}
prezaei commented 5 days ago

FWIW, we really need this :)

eiriktsarpalis commented 5 days ago

FYI @jeffhandley @PranavSenthilnathan something to consider for .NET 10.

gregsdennis commented 5 days ago

What is the advantage of

[JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
public string MySecondProperty { get; set; }

over

[JsonPropertyName("my-second-property")]
public string MySecondProperty { get; set; }

?

The second seems more explicit, more intentional.

eiriktsarpalis commented 5 days ago

I personally prefer the explicit approach, ultimately it's a trade-off between writing less boilerplate and having an explicit contract.

gregsdennis commented 5 days ago

@prezaei I'm interested in why you think we really need this in light of the current capability I listed above. What's your use case where listing the policy is better (or more preferred)?

(I find catering to edge cases tends to lead to bloat, and the framework is really not a place to have bloat, IMO.)

stephentoub commented 5 days ago

If I have a class with 20 members, I'd rather put one attribute on the class then one on each of the 20 properties.

As Eric says, it's a tradeoff between verbosity and explicitness.

gregsdennis commented 5 days ago

@stephentoub I think you make be missing my point about explicitness. I think using [JsonPropertyName()] is more explicit and intentional than applying a policy to an individual property. My point wasn't about global vs local.

stephentoub commented 5 days ago

@stephentoub I think you make be missing my point about explicitness. I think using [JsonPropertyName()] is more explicit and intentional than applying a policy to an individual property. My point wasn't about global vs local.

In https://github.com/dotnet/runtime/issues/108232#issuecomment-2474258742, you asked about the advantage of:

[JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
public string MySecondProperty { get; set; }

vs

[JsonPropertyName("my-second-property")]
public string MySecondProperty { get; set; }

That's not the comparison that's interesting. The comparison that's interesting is:

[JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
class MyClass
{
    public string MyPropA { get; set; }
    public string MyPropB { get; set; }
    public string MyPropC { get; set; }
    public string MyPropD { get; set; }
    public string MyPropE { get; set; }
    public string MyPropF { get; set; }
    public string MyPropG { get; set; }
    public string MyPropH { get; set; }
}

vs

class MyClass
{
    [JsonPropertyName("my-prop-a")]
    public string MyPropA { get; set; }

    [JsonPropertyName("my-prop-b")]
    public string MyPropB { get; set; }

    [JsonPropertyName("my-prop-c")]
    public string MyPropC { get; set; }

    [JsonPropertyName("my-prop-d")]
    public string MyPropD { get; set; }

    [JsonPropertyName("my-prop-e")]
    public string MyPropE { get; set; }

    [JsonPropertyName("my-prop-f")]
    public string MyPropF { get; set; }

    [JsonPropertyName("my-prop-g")]
    public string MyPropG { get; set; }

    [JsonPropertyName("my-prop-h")]
    public string MyPropH { get; set; }
}

And there there's an obvious advantage of the former, that of brevity, maintenance, etc.

That advantage is then weighed against the slightly more explicit nature of JsonPropertyName. I say slightly because both are explicit; it's just a question of whether a constant is provided or whether a constant is provided with a formula over it, e.g. "6" vs "x == 3 and the formula is 2 * x". JsonPropertyName is arguably a bit more explicit, which you see as an advantage.

Hence, "it's a tradeoff between verbosity and explicitness".

I don't believe I'm missing the point.

prezaei commented 5 days ago

To give you an example, we just had a service outage because someone incorrectly translated the property names when combining a set of classes with different property naming schemes into a single serialization context. (They were before separate contexts). They did not realize that they had to manually add JsonPropertyName on hundreds of property names that were affected by this. Happy to share the PR.

eiriktsarpalis commented 5 days ago

Happy to share the PR.

Feel free to create an implementation PR following the approved API shape.

gregsdennis commented 5 days ago

@stephentoub I still don't think you've got it.

The example you posted of comparing a single policy attribute at the class level with multiple property name attributes can be done today, without this proposal.

This proposal is being able to apply the policy at the property level. The appropriate comparison is having a global policy applied to the class and then overriding a single property with a different policy.

[JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
class MyClass
{
    public string MyPropA { get; set; }
    public string MyPropB { get; set; }
    [JsonNamingPolicy(JsonKnownNamingPolicy.CamelCase)]  // this is the proposal
    public string MyPropC { get; set; }
    public string MyPropD { get; set; }
    public string MyPropE { get; set; }
    public string MyPropF { get; set; }
    public string MyPropG { get; set; }
    public string MyPropH { get; set; }
}

vs

[JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)]
class MyClass
{
    public string MyPropA { get; set; }
    public string MyPropB { get; set; }
    [JsonPropertyName("myPropC")]  // this can be done already
    public string MyPropC { get; set; }
    public string MyPropD { get; set; }
    public string MyPropE { get; set; }
    public string MyPropF { get; set; }
    public string MyPropG { get; set; }
    public string MyPropH { get; set; }
}

These would produce the same outcome. Specifically, I'm questioning the value of this from the original post:

removes the ability to apply granular policies on the individual type/property/field level

Having typed out the example above, I do see that this is a coder preference scenario; that it gives the coder a choice in how they want to represent the override. But my question still stands: what is the advantage of specifying a policy at the property level over specifying the json property name with a constant? If the only advantage is that it gives the coder a choice, that's fine, I'd have my answer.

This is why I asked about @prezaei's use case. (Yes, I would like to see the PR if it's publicly available.) I don't understand how having this functionality would have solved or prevented a problem when a solution already exists, just in a (very slightly) different form.

eiriktsarpalis commented 5 days ago

The example you posted of comparing a single policy attribute at the class level with multiple property name attributes can be done today, without this proposal.

How? JsonNamingPolicyAttribute isn't a type that exists today.

gregsdennis commented 5 days ago

And thus my misunderstanding has been revealed. I thought the attribute existed but was only available at the type level, and that the proposal was to also allow it at the property level.

My bad. Please disregard my comments.