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

Feature request: treat Memory/ReadOnlyMemory<T> as T[] in System.Text.Json #86442

Closed
sandersaares opened this issue May 18, 2023 · 4 comments · Fixed by #88713
Closed

Feature request: treat Memory/ReadOnlyMemory<T> as T[] in System.Text.Json #86442

sandersaares opened this issue May 18, 2023 · 4 comments · Fixed by #88713
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@sandersaares
Copy link

sandersaares commented May 18, 2023

EDIT: see #86442 (comment) for API proposal.

Original Proposal

I would like System.Text.Json to serialize/deserialize Memory/ReadOnlyMemory<T> using the same logic as it uses for T[].

This implies:

  • Memory<byte> should be base64-encoded just like byte[] is
  • Memory<FooBar> should be serialized as appropriate for the type FooBar, same as FooBar[] would.

The main motivation is that I have an array from ArrayPool<T>.Shared, which can have space for more items than I actually need. To serialize only a segment of the array, I would like to use Memory<T> to identify only use "used" part of the array.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 18, 2023
@ghost
Copy link

ghost commented May 18, 2023

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

I would like System.Text.Json to serialize/deserialize Memory/ReadOnlyMemory<T> using the same logic as it uses for T[].

This implies:

  • Memory<byte> should be base64-encoded just like byte[] is
  • Memory<FooBar> should be serialized as appropriate for the type FooBar, same as FooBar[] would.

The main motivation is that I have an array from ArrayPool<T>.Shared, which can have space for more items than I actually need. To serialize only a segment of the array, I would like to use Memory<T> to identify only use "used" part of the array.

Author: sandersaares
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels May 18, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone May 18, 2023
@eiriktsarpalis
Copy link
Member

Also related to #73500, #81970 and #60780. One concern that has prevented us from adding built-in support for more types is trimmed application size, although this has become less of an issue for apps that only source generators.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 24, 2023

Provisional API proposal:

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    // Similar to byte[] serializes as Base64 strings
    public static JsonConverter<Memory<byte>> MemoryByteConverter { get; }
    public static JsonConverter<ReadOnlyMemory<byte>> ReadOnlyMemoryByteConverter { get; }

    // Other element types serialize as JSON arrays
    public static JsonTypeInfo<Memory<TElement>> CreateMemoryInfo<TElement>(JsonSerializerOptions options, JsonCollectionInfoValues<Memory<TElement>> collectionInfo);
    public static JsonTypeInfo<ReadOnlyMemory<TElement>> CreateReadOnlyMemoryInfo<TElement>(JsonSerializerOptions options, JsonCollectionInfoValues<ReadOnlyMemory<TElement>> collectionInfo);
}

@eiriktsarpalis eiriktsarpalis self-assigned this Jun 24, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Jun 24, 2023
@eiriktsarpalis eiriktsarpalis added partner-impact This issue impacts a partner who needs to be kept updated api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 24, 2023
@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2023

Video

Looks good as proposed

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    // Similar to byte[] serializes as Base64 strings
    public static JsonConverter<Memory<byte>> MemoryByteConverter { get; }
    public static JsonConverter<ReadOnlyMemory<byte>> ReadOnlyMemoryByteConverter { get; }

    // Other element types serialize as JSON arrays
    public static JsonTypeInfo<Memory<TElement>> CreateMemoryInfo<TElement>(JsonSerializerOptions options, JsonCollectionInfoValues<Memory<TElement>> collectionInfo);
    public static JsonTypeInfo<ReadOnlyMemory<TElement>> CreateReadOnlyMemoryInfo<TElement>(JsonSerializerOptions options, JsonCollectionInfoValues<ReadOnlyMemory<TElement>> collectionInfo);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 6, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
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.Text.Json partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants