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

Improve error message for dictionary key converters not implementing ReadAsPropertyName/WriteAsPropertyName. #93406

Closed
bitbonk opened this issue Oct 12, 2023 · 7 comments · Fixed by #93553
Assignees
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@bitbonk
Copy link
Contributor

bitbonk commented Oct 12, 2023

Description

When I serialize an object that contains a property of type Dictionary<TKey, TValue> and TKey is a custom class, I get a NotSupportedException, even if there is a JSON converter for this.

JsonSerializer.Serialize(
    new Custom
    {
        Dict = new Dictionary<CustomKey, string>
        {
            [new CustomKey {Value1 = "one", Value2 = "two"}] = "three",
            [new CustomKey {Value1 = "four", Value2 = "five"}] = "six"
        }});
class Custom
{
    public Dictionary<CustomKey, string> Dict { get; set; }
}

[JsonConverter(typeof(CustomKeyJsonConverter))]
class CustomKey
{
    public string Value1 { get; set; }
    public string Value2 { get; set; }

    public static CustomKey Parse(string text)
    {
        var values = text.Split("-");
        return new CustomKey {Value1 = values[0], Value2 = values[1]};
    }

    public override string ToString()
    {
        return $"{this.Value1}-{this.Value2}";
    }
}

class CustomKeyJsonConverter : JsonConverter<CustomKey>
{
    public override CustomKey? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        return CustomKey.Parse(reader.GetString());
    }

    public override void Write(Utf8JsonWriter writer, CustomKey value, JsonSerializerOptions options)
    {
        writer.WriteStringValue(value.ToString());
    }
}

Reproduction Steps

see above

Expected behavior

The serialization (and deserialization) should succeed if there is a JSON converter for the key.

Actual behavior

The following NotSupportedException exception is thrown

System.NotSupportedException
The type 'JsonTest.CustomKey' is not a supported dictionary key using converter of type 'JsonTtest.CustomKeyJsonConverter'. The unsupported member type is located on type 'System.String'. Path: $.Dict.
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException(WriteStack& state, NotSupportedException ex)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Serialize(Utf8JsonWriter writer, T& rootValue, Object rootValueBoxed)
   at System.Text.Json.JsonSerializer.WriteString[TValue](TValue& value, JsonTypeInfo`1 jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)

Regression?

The same behavior is in .NET 6, 7 and 8, so it probably not a regression.

Known Workarounds

Write a custom JSON for the whole Dictionary<CustomKey, string> type.

Configuration

Windows 11, ,NET SDK 8.0.100-rc.2.23502.2

Other information

No response

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

ghost commented Oct 12, 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

Description

When I serialize an object that contains a property of type Dictionary<TKey, TValue> and TKey is a custom class, I get a NotSupportedException, even if there is JSON converter for this.

JsonSerializer.Serialize(
    new Custom
    {
        Dict = new Dictionary<CustomKey, string>
        {
            [new CustomKey {Value1 = "one", Value2 = "two"}] = "three",
            [new CustomKey {Value1 = "four", Value2 = "five"}] = "six"
        }});
class Custom
{
    public Dictionary<CustomKey, string> Dict { get; set; }
}

[JsonConverter(typeof(CustomKeyJsonConverter))]
class CustomKey
{
    public string Value1 { get; set; }
    public string Value2 { get; set; }

    public static CustomKey Parse(string text)
    {
        var values = text.Split("-");
        return new CustomKey {Value1 = values[0], Value2 = values[1]};
    }

    public override string ToString()
    {
        return $"{this.Value1}-{this.Value2}";
    }
}

class CustomKeyJsonConverter : JsonConverter<CustomKey>
{
    public override CustomKey? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        return CustomKey.Parse(reader.GetString());
    }

    public override void Write(Utf8JsonWriter writer, CustomKey value, JsonSerializerOptions options)
    {
        writer.WriteStringValue(value.ToString());
    }
}

Reproduction Steps

see above

Expected behavior

The serialization (and deserialization) should succeed if there is a JSON converter for the key.

Actual behavior

The following NotSupportedException exception is thrown

```text
System.NotSupportedException
The type 'JsonTtest.CustomKey' is not a supported dictionary key using converter of type 'JsonTtest.CustomKeyJsonConverter'. The unsupported member type is located on type 'System.String'. Path: $.Dict.
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException(WriteStack& state, NotSupportedException ex)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Serialize(Utf8JsonWriter writer, T& rootValue, Object rootValueBoxed)
   at System.Text.Json.JsonSerializer.WriteString[TValue](TValue& value, JsonTypeInfo`1 jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)

### Regression?

The same behavior is in .NET 6, 7 and 8, so it probably not a regression.

### Known Workarounds

Write a custom JSON for the whole `Dictionary<CustomKey, string>` type.

### Configuration

Windows 11, ,NET SDK 8.0.100-rc.2.23502.2

### Other information

_No response_

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

`area-System.Text.Json`

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

@eiriktsarpalis
Copy link
Member

Using converters for dictionary key serialization requires overriding the ReadAsPropertyName/WriteAsPropertyName methods:

class CustomKeyJsonConverter : JsonConverter<CustomKey>
{
    public override CustomKey? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => CustomKey.Parse(reader.GetString());

    public override void Write(Utf8JsonWriter writer, CustomKey value, JsonSerializerOptions options)
        => writer.WriteStringValue(value.ToString());

    public override CustomKey ReadAsPropertyName(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => CustomKey.Parse(reader.GetString());

    public override void WriteAsPropertyName(Utf8JsonWriter writer, CustomKey value, JsonSerializerOptions options)
        => writer.WritePropertyName(value.ToString());
}

Keeping the issue open since we could improve the error message to include this information.

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Oct 13, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Oct 13, 2023
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Oct 13, 2023
@eiriktsarpalis eiriktsarpalis changed the title Cannot serialize a dictionary where the key is a custom class with a JsonConverter Improve error message for dictionary key converters not implementing ReadAsPropertyName/WriteAsPropertyName. Oct 13, 2023
@naeemaei
Copy link
Contributor

I am interested in helping to contribute in this issue

@eiriktsarpalis
Copy link
Member

Thanks @naeemaei, I've assigned it to you.

@naeemaei
Copy link
Contributor

Considering that this error occurs when the methods are not implemented in the converter, is it better to create a method with NotImplementedException in ThrowHelper.cs or append dictionary key converter not implemented ReadAsPropertyName/WriteAsPropertyName methods message to the existing message of the DictionaryKeyTypeNotSupported key?

[DoesNotReturn]
public static void ThrowNotImplementedException_DictionaryKeyTypeConvertorNotImplemented(Type keyType, JsonConverter converter)
{
    throw new NotImplementedException(SR.Format(SR.DictionaryKeyTypeConvertorNotImplemented, keyType, converter.GetType()));
}

@eiriktsarpalis

@eiriktsarpalis
Copy link
Member

Changing the exception type can break users that depend on catching the current exception. I think it would be preferable to just update the error message.

@bitbonk
Copy link
Contributor Author

bitbonk commented Oct 16, 2023

Just for completeness and future readers: Changing the converter as @eiriktsarpalis suggested in #93406 (comment) does fix the problem, the exceptions goes away and the type is correctly (de)serialized (testet with .NET 6).

naeemaei added a commit to naeemaei/runtime that referenced this issue Oct 16, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 16, 2023
eiriktsarpalis added a commit that referenced this issue Oct 16, 2023
* Improve DictionaryKeyTypeNotSupported message

Fix #93406

* Improve error message

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

---------

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions 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