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

Add property JsonNamingPolicy.ConvertName() overload with Type parameter #48396

Closed
MaceWindu opened this issue Feb 17, 2021 · 6 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner

Comments

@MaceWindu
Copy link

Background and Motivation

During migration from Newtonsoft.Json we discovered following unsupported scenario.

We had special naming policy for properties of specific types. In our case we were adding type-specific prefix to property names of some types (they are complex types with custom converters) which were used by consumer (javascript app in our case) to apply deserialization logic based on property prefix.

Problem with System.Text.Json is that JsonNamingPolicy.ConvertName method accepts only property name.
I suggest to add additional overload with Type parameter (all call sites already have type information for it).

Only workaround I found is manually annotate all affected properties with JsonPropertyName attribute, which is quite unpleasant way to fix it 😞

Proposed API

namespace System.Text.Json
{
    public abstract class JsonNamingPolicy {
+        public virtual string ConvertName(string name, Type type);
    }
}

Proposed method implementation (should be called instead of current ConvertName by consumers):

public virtual string ConvertName(string name, Type type) => ConvertName(name);

Usage Examples

public class MyCustomNamingPolicy  : JsonNamingPolicy
{
    public override string ConvertName(string name, Type type)
    {
        var name = ConvertName(name);
        // or
        //var name = JsonNamingPolicy.CamelCase.ConvertName(name);
        return type == typeof(PerfixedType) : "prefix_" + name : name;
    }

    public override string ConvertName(string name) => JsonNamingPolicy.CamelCase.ConvertName(name);
}

Alternative Designs

I think it is most straightforward one. And much-much cleaner than implementation we used with Newtonsoft.Json.

Risks

None? Minor performance hit on extra call with extra parameter shouldn't be a problem as results of property name generation are cached and reused.

@MaceWindu MaceWindu added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Feb 17, 2021
@ghost
Copy link

ghost commented Feb 17, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

During migration from Newtonsoft.Json we discovered following unsupported scenario.

We had special naming policy for properties of specific types. In our case we were adding type-specific prefix to property names of some types (they are complex types with custom converters) which were used by consumer (javascript app in our case) to apply deserialization logic based on property prefix.

Problem with System.Text.Json is that JsonNamingPolicy.ConvertName method accepts only property name.
I suggest to add additional overload with Type parameter (all call sites already have type information for it).

Only workaround I found is manually annotate all affected properties with JsonPropertyName attribute, which is quite unpleasant way to fix it 😞

Proposed API

namespace System.Text.Json
{
    public abstract class JsonNamingPolicy {
+        public virtual string ConvertName(string name, Type type);
    }
}

Proposed method implementation (should be called instead of current ConvertName by consumers):

public virtual string ConvertName(string name, Type type) => ConvertName(name);

Usage Examples

public class MyCustomNamingPolicy  : JsonNamingPolicy
{
    public override string ConvertName(string name, Type type)
    {
        var name = ConvertName(name);
        // or
        //var name = JsonNamingPolicy.CamelCase.ConvertName(name);
        return type == typeof(PerfixedType) : "prefix_" + name : name;
    }

    public override string ConvertName(string name) => JsonNamingPolicy.CamelCase.ConvertName(name);
}

Alternative Designs

I think it is most straightforward one. And much-much cleaner than implementation we used with Newtonsoft.Json.

Risks

None? Minor performance hit on extra call with extra parameter shouldn't be a problem as results of property name generation are cached and reused.

Author: MaceWindu
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

I'm not very familiar with the Json.NET feature, but wouldn't there also need to be a corresponding construct for deserialization?

@MaceWindu
Copy link
Author

This proposal is about richer context for json property name generation logic. Don't think it has anything with de/serialization.

@eiriktsarpalis
Copy link
Member

Out of curiosity, how were you able to achieve the same result in Json.NET? At quick glance the equivalent construct seems to be the NamingStrategy class, which doesn't seem to accept a Type parameter either.

@MaceWindu
Copy link
Author

  1. we used class, derived from CamelCasePropertyNamesContractResolver and override CreateProperties method
  2. also for model binding from request (asp.net mvc app) we were customizing System.Web.Mvc.DefaultModelBinder using BindProperty method override...

@layomia
Copy link
Contributor

layomia commented Feb 22, 2021

In .NET 6 we are providing an IContractResolver-like mechanism to configure type and property metadata, as part of the JSON source gen work (#31257, #45448). You'll be able to configure property metadata as you described above:

we used class, derived from CamelCasePropertyNamesContractResolver and override CreateProperties method

@layomia layomia closed this as completed Feb 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants