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

[API Proposal]: Provide APIs to create JsonPropertyInfo based on PropertyInfo #78711

Open
angelaki opened this issue Nov 22, 2022 · 11 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Milestone

Comments

@angelaki
Copy link

Background and motivation

The latests System.Text.Json APIs finally allow me to use Interfaces for deserialization since I'm able to control class to be used etc.. Creating my own TypeInfoResolver based on DefaultJsonTypeInfoResolver makes it now pretty hard to popule my own PropertyCollection since the default resolver uses pretty much internal APIs to generate it's property collection.

If I'm not mistaken, most of the magic happens in ReflectionJsonTypeInfo (internal sealed class).

API Proposal

Create a public API that offers these functionalities (e.g. by making this class public available?).

API Usage

public class MyTypeInfoResolver : DefaultJsonTypeInfoResolver
{
    public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        if (type.IsInterface)
        {
            var result = JsonTypeInfo.CreateJsonTypeInfo(type, options);
            result.CreateObject = () => new CAA();

            foreach (var p in type.GetProperties())
            {
                //Allow me to add properties here based on `System.Reflection.PropertyInfo`
            }

            return result;
        }

        return base.GetTypeInfo(type, options);
    }
}

Alternative Designs

No response

Risks

No response

@angelaki angelaki added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 22, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@ghost
Copy link

ghost commented Nov 22, 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

Background and motivation

The latests System.Text.Json APIs finally allow me to use Interfaces for deserialization since I'm able to control class to be used etc.. Creating my own TypeInfoResolver based on DefaultJsonTypeInfoResolver makes it now pretty hard to popule my own PropertyCollection since the default resolver uses pretty much internal APIs to generate it's property collection.

If I'm not mistaken, most of the magic happens in ReflectionJsonTypeInfo (internal sealed class).

API Proposal

Create a public API that offers these functionalities (e.g. by making this class public available?).

API Usage

public class MyTypeInfoResolver : DefaultJsonTypeInfoResolver
{
    public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        if (type.IsInterface)
        {
            var result = JsonTypeInfo.CreateJsonTypeInfo(type, options);
            result.CreateObject = () => new CAA();

            foreach (var p in type.GetProperties())
            {
                //Allow me to add properties here based on `System.Reflection.PropertyInfo`
            }

            return result;
        }

        return base.GetTypeInfo(type, options);
    }
}

Alternative Designs

No response

Risks

No response

Author: angelaki
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

I might not be understanding the request well enough but doesn't adding entries to the JsonTypeInfo.Properties property work for you? See this docs section for an example.

@layomia
Copy link
Contributor

layomia commented Nov 22, 2022

Also, per the issue title and to raise the visibility of the method in the doc Eirik linked: are you aware of JsonTypeInfo.CreateJsonPropertyInfo?

@layomia layomia added this to the 8.0.0 milestone Nov 22, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@layomia layomia added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 22, 2022
@ghost
Copy link

ghost commented Nov 22, 2022

This issue has been marked needs-author-action and may be missing some important information.

@angelaki
Copy link
Author

Yes, I know JsonTypeInfo.CreateJsonPropertyInfo, but thank you for mentioning it! Is there a reason it is not a static method? I was asking myself when I found it.

Never the less, I'd like to have an easy API allowing me to generate a System.Text.Json.Serialization.Metadata.JsonPropertyInfo based on a System.Reflection.PropertyInfo. Imho this is a common use-case and right now I'd need to write an extension method for something the DefaultJsonTypeInfoResolver is doing internal anyway.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Nov 22, 2022
@eiriktsarpalis
Copy link
Member

We might consider offering that as an accelerator method, but generally speaking it should pretty straightforward to write one yourself:

public static JsonPropertyInfo CreateJsonPropertyInfo(this JsonTypeInfo jsonTypeInfo, PropertyInfo propertyInfo)
{
    // NB does not map any attribute data like JsonConverterAttribute or JsonPropertyName
    JsonPropertyInfo jsonPropertyInfo = jsonTypeInfo.CreateJsonPropertyInfo(propertyInfo.PropertyType, propertyInfo.Name);
    jsonPropertyInfo.AttributeProvider = propertyInfo;

    if (propertyInfo.CanRead)
        jsonPropertyInfo.Get = propertyInfo.GetValue;

    if (propertyInfo.CanWrite)
        jsonPropertyInfo.Set = propertyInfo.SetValue;

    return jsonPropertyInfo;
}

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Nov 23, 2022
@eiriktsarpalis eiriktsarpalis added wishlist Issue we would like to prioritize, but we can't commit we will get to it yet and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Nov 23, 2022
@angelaki
Copy link
Author

angelaki commented Nov 23, 2022

@eiriktsarpalis Yeah, sure. We're speaking about micro optimization here. But imho this extension method could / should be used by the reflector JsonPropertyInfo internally, too.

Corresponding it should be an implementation supporting attributes etc. Optionally configurable.

@krwq
Copy link
Member

krwq commented Nov 23, 2022

The reason CreateJsonPropertyInfo is not static is that we want it to be possible to create JsonPropertyInfo<DeclaringType, PropertyType> in the future. JsonPropertyInfo is currently forced to be tied with specific JsonTypeInfo implementation so that no one is tempted to create it with different options and we're not locked with the design forever.

Please update your proposal with specific API addition and we can consider that depending on the demand. Generally speaking creating properties from scratch most of the time should not be necessary because of the modifiers and different knobs in the options and that's why we didn't particularly focus on adding utilities.

Can you describe your scenario more why it needs to add properties from scratch and it can't re-use what was added by default?

@angelaki
Copy link
Author

angelaki commented Nov 23, 2022

I've implemented an API where for in- and outgoing objects only interfaces are used. For a long time I was forced to use Newtonsoft.JSON, with S.T.J 7.0 I'm finally able to use interfaces (btw, thank you for this new, awesome API!).

But since Interfaces do not inherit properties the classical way, I need to loop through the interfaces and add their properties to serialization manually. That is where I'm forced to add the properties myself. (1)

Furthermore I'm adding my own OutputFormatters (based on SystemTextJsonOutputFormatter) for similar proposes. Right now I need to add an OutputFormatter for every deviating serializer-settings, since my Controllers are able to set the used interface for serialization via attribute. Since the (sealed) WriteResponseBodyAsync method uses the JsonSerializerOptions provided with the constructor, I am not able to deal with varying settings in a single OutputFormatter. (2)

Is one of those use-cases interesting for you to get respected?

  1. Commonly used method to create reflection based JsonPropertyInfos
  2. SystemTextJsonOutputFormatter being able to use varying JsonSerializerOptions

please let me know, so I would create a new API Proposal and close this discussion.

@eiriktsarpalis
Copy link
Member

But since Interfaces do not inherit properties the classical way

I'm guessing you mean this issue? #41749

@angelaki
Copy link
Author

angelaki commented Nov 24, 2022

@eiriktsarpalis at least this issue is pretty related, yes. But imho STJ isn't the library that needs to handle this. Guess we'll end up with the discussion, why .Net doesn't think inherited interface properties belong to the interface (just like inherited class properties does). Even though I never found a good reason for it, that is the way .Net handles this and changing it would be quite more than just a breaking change.

I think the use-case is pretty advanced / rare and interface serialization should be handled manually anyway. Otherwise people could complain that the serializer writes properties, typeof(IObject).GetProperties() doesn't provide. Staying with .Net's behavior is just right I guess. Never the less, a common code base to generate properties (JsonPropertyInfo) based on PropertyInfos would be great.

And what do you think of the SystemTextJsonFormatters being able to change the SerializerSettings context based? I could write a pretty simple PR containing this possibility. Wouldn't be a breaking change and allow even more customizations I think. I'd create an abstract DynamicSystemTextJsonFormatter providing and abstract function to get the SerializerSettings used. The default SystemTextJsonsFormatters would just provide the only they received in their constructor.

If you think this use-case is too rare I'd stay with my solution implementing my very own Formatter that copies just the default ones code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Projects
None yet
Development

No branches or pull requests

4 participants