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]: Add a JsonEncodedText.Value property #69716

Closed
Tracked by #63762 ...
eiriktsarpalis opened this issue May 24, 2022 · 4 comments · Fixed by #76545
Closed
Tracked by #63762 ...

[API Proposal]: Add a JsonEncodedText.Value property #69716

eiriktsarpalis opened this issue May 24, 2022 · 4 comments · Fixed by #76545
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented May 24, 2022

Background and motivation

The JsonEncodedText struct encapsulates both the UTF8 and string representations of the encoded value, however only the UTF8 value is accessible via the EncodedUtf8Bytes property. This makes the type more difficult to use in deserialization scenaria, for instance:

public class MyConverter : JsonConverter<Foo>
{
      private readonly JsonEncodedText _preEncodedPropertyName = JsonEncodedText.Encode("propertyName");

      public override void Write(Utf8JsonWriter writer, Foo value, JsonSerializerOptions options)
      {
             writer.WriteStartObject();
             writer.WriteString(_propertyName.Utf8EncodedBytes, value.Value);
             writer.WriteEndObject();
      }

      public override Foo Read(ref Utf8JsonReader reader, Type ttc, JsonSerializerOptions options)
      {
             reader.Read();
             reader.Read();
             Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);
 
             string propertyName = reader.ReadPropertyName();
             if (_preEncodedPropertyName.ToString() == propertyName) // there's no proper way to fetch the UTF-16 variant when serializing
             {
                     /* read the property value */
             }
      }
}

API Proposal

namespace System.Text.Json;

public struct JsonEncodedValue
{
       public ReadOnlySpan<byte> EncodedUtf8Bytes => _utf8Value;
+      public string Value => _value ?? string.Empty;
}

API Usage

From the previous example:

      public override Foo Read(ref Utf8JsonReader reader, Type ttc, JsonSerializerOptions options)
      {
             reader.Read();
             reader.Read();
             Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);
 
             string propertyName = reader.ReadPropertyName();
             if (_preEncodedPropertyName.Value == propertyName) // there's no proper way to fetch the UTF-16 variant when serializing
             {
                     /* read the property value */
             }
      }

Alternative Designs

We might expose the property as ReadOnlySpan<char>, but there's currently no reason why we would want to do this; the current implementation will always create a string for the UTF-16 representation.

Risks

No response

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 24, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 24, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json and removed untriaged New issue has not been triaged by the area owner labels May 24, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this May 24, 2022
@ghost
Copy link

ghost commented May 24, 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 JsonEncodedText struct encapsulates both the UTF8 and string representations of the encoded value, however only the UTF8 value is accessible via the EncodedUtf8Bytes property. This makes the type more difficult to use in deserialization scenaria, for instance:

public class MyConverter : JsonConverter<Foo>
{
      private readonly JsonEncodedText _preEncodedPropertyName = JsonEncodedText.Encode("propertyName");

      public override void Write(Utf8JsonWriter writer, Foo value, JsonSerializerOptions options)
      {
             writer.WriteStartObject();
             writer.WriteString(_propertyName.Utf8EncodedBytes, value.Value);
             writer.WriteEndObject();
      }

      public override Foo Read(ref Utf8JsonReader reader, Type ttc, JsonSerializerOptions options)
      {
             reader.Read();
             reader.Read();
             Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);
 
             string propertyName = reader.ReadPropertyName();
             if (_preEncodedPropertyName.ToString() == propertyName) // there's no proper way to fetch the UTF-16 variant when serializing
             {
                     /* read the property value */
             }
      }
}

API Proposal

namespace System.Text.Json;

public struct JsonEncodedValue
{
       public ReadOnlySpan<byte> EncodedUtf8Bytes => _utf8Value;
+      public string Value => _value ?? string.Empty;
}

API Usage

From the previous example:

      public override Foo Read(ref Utf8JsonReader reader, Type ttc, JsonSerializerOptions options)
      {
             reader.Read();
             reader.Read();
             Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);
 
             string propertyName = reader.ReadPropertyName();
             if (_preEncodedPropertyName.Value == propertyName) // there's no proper way to fetch the UTF-16 variant when serializing
             {
                     /* read the property value */
             }
      }

### Alternative Designs

_No response_

### Risks

_No response_

<table>
  <tr>
    <th align="left">Author:</th>
    <td>eiriktsarpalis</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

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

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone May 24, 2022
@eiriktsarpalis eiriktsarpalis added Cost:S Work that requires one engineer up to 1 week 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 May 24, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: 7.0.0, 8.0.0 Jul 19, 2022
@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

  • Assuming Value would either expose the string passed to Encode or produce a string when given a span, this looks fine
    • The sketched implementation seems to suggest that for the span case the return value might be the empty string, which seems problematic.
  • We should document that the string passed to Encode might be a different one from the one returned by Value
    • This happens to be the current implementation ;-)
namespace System.Text.Json;

public partial struct JsonEncodedText
{
    public string Value { get; }
}

@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 Aug 16, 2022
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Sep 29, 2022
@schoder-moreno
Copy link
Contributor

Is this one up for grabs @eiriktsarpalis? If so I'd like to take a stab at it :)

@eiriktsarpalis
Copy link
Member Author

Yep, go for it :-)

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 3, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 4, 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.Text.Json Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants