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 JsonTypeInfo overloads to System.Memory.Data #54979

Closed
eerhardt opened this issue Jun 30, 2021 · 7 comments · Fixed by #69012
Closed

Add JsonTypeInfo overloads to System.Memory.Data #54979

eerhardt opened this issue Jun 30, 2021 · 7 comments · Fixed by #69012
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@eerhardt
Copy link
Member

Background and Motivation

In 6.0, we have added APIs to System.Text.Json and System.Net.Http.Json that take a JsonTypeInfo or JsonSerializerContext. See #1568 for the reasons and advantages of these overloads.

We have very thin wrapper APIs in System.Memory.Data around JsonSerializer, however these APIs weren't updated for JsonTypeInfo overloads. We should add them to get the same advantages as listed above.

Proposed API

namespace System
{
    public partial class BinaryData
    {
        // exisitng APIs
        public BinaryData(object? jsonSerializable, System.Text.Json.JsonSerializerOptions? options = null, System.Type? type = null) { }
        public static System.BinaryData FromObjectAsJson<T>(T jsonSerializable, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
        public T? ToObjectFromJson<T>(System.Text.Json.JsonSerializerOptions? options = null) { throw null; }

        // new proposed APIs
+       public BinaryData(object? jsonSerializable, System.Text.Json.Serialization.JsonSerializerContext context, System.Type? type = null) { }
+       public static System.BinaryData FromObjectAsJson<T>(T jsonSerializable, System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) { throw null; }
+       public T? ToObjectFromJson<T>(System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) { throw null; }
    }
}

Usage Examples

var myObject = new MyType();

var binaryData = new BinaryData(myObject, JsonContext.Default);
// --- or ---
var binaryData = BinaryData.FromObjectAsJson(myObject, JsonContext.Default.MyType);


var myObject2 = binaryData.ToObjectFromJson(JsonContext.Default.MyType);

Alternative Designs

N/A

Risks

None

cc @layomia @JoshLove-msft @KrzysztofCwalina

@eerhardt eerhardt added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory labels Jun 30, 2021
@ghost
Copy link

ghost commented Jun 30, 2021

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

Issue Details

Background and Motivation

In 6.0, we have added APIs to System.Text.Json and System.Net.Http.Json that take a JsonTypeInfo or JsonSerializerContext. See #1568 for the reasons and advantages of these overloads.

We have very thin wrapper APIs in System.Memory.Data around JsonSerializer, however these APIs weren't updated for JsonTypeInfo overloads. We should add them to get the same advantages as listed above.

Proposed API

namespace System
{
    public partial class BinaryData
    {
        // exisitng APIs
        public BinaryData(object? jsonSerializable, System.Text.Json.JsonSerializerOptions? options = null, System.Type? type = null) { }
        public static System.BinaryData FromObjectAsJson<T>(T jsonSerializable, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
        public T? ToObjectFromJson<T>(System.Text.Json.JsonSerializerOptions? options = null) { throw null; }

        // new proposed APIs
+       public BinaryData(object? jsonSerializable, System.Text.Json.Serialization.JsonSerializerContext context, System.Type? type = null) { }
+       public static System.BinaryData FromObjectAsJson<T>(T jsonSerializable, System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) { throw null; }
+       public T? ToObjectFromJson<T>(System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) { throw null; }
    }
}

Usage Examples

var myObject = new MyType();

var binaryData = new BinaryData(myObject, JsonContext.Default);
// --- or ---
var binaryData = BinaryData.FromObjectAsJson(myObject, JsonContext.Default.MyType);


var myObject2 = binaryData.ToObjectFromJson(JsonContext.Default.MyType);

Alternative Designs

N/A

Risks

None

cc @layomia @JoshLove-msft @KrzysztofCwalina

Author: eerhardt
Assignees: -
Labels:

api-suggestion, area-System.Memory

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 30, 2021
@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Jul 1, 2021
eerhardt added a commit to eerhardt/runtime that referenced this issue Jul 1, 2021
The current APIs require unreferenced code, since they call into JsonSerializer without providing JsonTypeInfo.

dotnet#54979 is logged to add "trim compatible" APIs here.
eerhardt added a commit that referenced this issue Jul 6, 2021
The current APIs require unreferenced code, since they call into JsonSerializer without providing JsonTypeInfo.

#54979 is logged to add "trim compatible" APIs here.
@eerhardt
Copy link
Member Author

eerhardt commented Apr 6, 2022

Given there hasn't been any feedback on this. I'm going to move it to ready for review.

@eerhardt eerhardt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 6, 2022
@eerhardt eerhardt self-assigned this Apr 6, 2022
@terrajobst
Copy link
Member

terrajobst commented Apr 12, 2022

Video

  • Looks good as proposed
namespace System;

public partial class BinaryData
{
    // Existing APIs
    // public BinaryData(object? jsonSerializable, JsonSerializerOptions? options = null, Type? type = null);
    // public static BinaryData FromObjectAsJson<T>(T jsonSerializable, JsonSerializerOptions? options = null);
    // public T? ToObjectFromJson<T>(JsonSerializerOptions? options = null);

    // New APIs
    public BinaryData(object? jsonSerializable, JsonSerializerContext context, Type? type = null);
    public static BinaryData FromObjectAsJson<T>(T jsonSerializable, JsonTypeInfo<T> jsonTypeInfo);
    public T? ToObjectFromJson<T>(JsonTypeInfo<T> jsonTypeInfo);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 12, 2022
@danmoseley
Copy link
Member

@eerhardt should this be labeled 'up for grabs'

@eerhardt eerhardt added the help wanted [up-for-grabs] Good issue for external contributors label Apr 13, 2022
@eerhardt eerhardt removed their assignment Apr 13, 2022
@Tarun047
Copy link
Contributor

Tarun047 commented May 7, 2022

Hi guys I'd like to take this one up.
@eerhardt, @danmoseley please review my for Initial implementation and suggest if any changes are needed

@KrzysztofCwalina
Copy link
Member

Will System.Memory.Data remain a NetStandard 2.0 package after this change?

@eerhardt
Copy link
Member Author

Yes, System.Memory.Data still supports netstandard2.0 after this change:

<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>

The JsonTypeInfo type comes from System.Text.Json, which also supports netstandard2.0.

@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants