System.Text.Json: Path Information in JsonException.Message

[weifenluo]

When multiple (de)serializers exist and one provides converter to another, in order to have correct path information in JsonException.Message, we can remove JsonException.SetMessage method and JsonException.AppendPathInformation property, and replace Message property implementation as following:

public override string Message
{
    get
    {
        string message = _message ?? base.Message;
        if (!string.IsNullOrEmpty(Path))
        {
            return LineNumber.HasValue
                ? $"{message} | Path: {Path} | LineNumber: {LineNumber} | BytePositionInLine: {BytePositionInLine}"
                : $"{message} | Path: {Path}";
        }
        return message;
    }
}

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When multiple (de)serializers exist and one provides converter to another, in order to have correct path information in JsonException.Message, we can remove JsonException.SetMessage method and JsonException.AppendPathInformation property, and replace Message property implementation as following:

public override string Message
{
    get
    {
        string message = _message ?? base.Message;
        if (!string.IsNullOrEmpty(Path))
        {
            return LineNumber.HasValue
                ? $"{message} | Path: {Path} | LineNumber: {LineNumber} | BytePositionInLine: {BytePositionInLine}"
                : $"{message} | Path: {Path}";
        }
        return message;
    }
}

Author:	weifenluo
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

When multiple (de)serializers exist and one provides converter to another, in order to have correct path information in JsonException.Message, we can remove JsonException.SetMessage method and JsonException.AppendPathInformation property, and replace Message property implementation as following

Hi @weifenluo, I'm not sure I understand the problem. Would it be possible to share a minimal reproduction highlighting the issue and explaining why changing your exception message is necessary?

[weifenluo]

I'm having a custom type EntitySet<T>, similar to System.Data.DataSet, except its strongly typed.

I've implemented a custom serializer/deserializer for this type, using low level Utf8JsonWriter/Utf8JsonReader, say EntitySetJsonSerializer/EntitySetJsonDeserializer, with exactly same functionalities provided by System.Text.Json.JsonSerializer. Converter EntitySetJsonConverter<T> /EntitySetJsonConverterFactory is also provided for type EntitySet<T>:

[JsonConverter(typeof(EntitySetJsonConverterFactory))]
public class EntitySet<T>
{
    ...
}

The implementation of EntitySetJsonConverter<T> simply calls overloaded method of EntitySetJsonSerializer/EntitySetJsonDeserializer, which takes a Utf8JsonReader /Utf8JsonWriter parameter. EntitySetJsonSerializer/EntitySetJsonDeserializer also provide async read from/write to stream, which is not supported by converters.

To (de)serialize EntitySet<T> object, user can:

1. Use EntitySetJsonSerializer /EntitySetJsonDeserializer, if this object is top level;
2. Use built-in System.Text.Json.JsonSerializer if it's a member of other class, for example:

public class MyClass
{
    public EntitySet<T> Data { get; set; }
}

Please keep in mind EntitySet<T> is a complex object, it contains a list of Entity<T> objects, which is a tree of fields, sub entity, child entity sets, etc. The Path information is important for diagnostic when exception thrown. The field is the leaf node of the tree, each field will retrieve JsonConverter based on its data type to perform the value (de)serialization.

The exception handling can be a little bit tricky here. Since JsonException is publicly immutable, in EntitySetJsonSerializer/EntitySetJsonDeserializer implementation, all exceptions are caught and rethrown as JsonException, with Path, and hopefully LineNumber and BytePositionInLine (see #50629).

However for use case No.2, the exception will be caught again by System.Text.Json.JsonSerializer, it will modify the exception, overwrite the Path property (see #51537), and append path information ('| Path ... | LineNumber ... | BytePositionInLine ...') into JsonException.Message. It not possible to get the consistent Exception.Message for both use cases: either no path information for use case 1, or duplicated path info for use case 2.

What I proposed here together with #51537 can solve this problem, and IMO manipulating JsonException.Message outside of JsonException class, is kind of ugly - it breaks encapsulation of OOP.

[eiriktsarpalis]

So if I understand the problem correctly, you are using custom serialization logic that throws its own JsonExceptions which are overwritten by the STJ serializer that can call into that custom logic via a converter?

A duplicate of #51537 then?

[weifenluo]

Yes, your understand is correct.

It's not a duplicate of #51537, which is about JsonException.Path property. This issue is about inconsistence of JsonException.Message property between STJ serializer and custom serializer(s).

[eiriktsarpalis]

Could you provide a minimal reproduction demonstrating the inconsistency? A console app showing the actual message versus the expected message?

[weifenluo]

I've made a test project here:
https://github.com/weifenluo/JsonExceptionMessage

I managed to get it work as expected. The trick is when dealing with non top level object, I should rethrow a JsonException with a null or empty message:
https://github.com/weifenluo/JsonExceptionMessage/blob/03f35f42c795d4165513d6b8236a16390ab23254/JsonExceptionMessage/EntitySerializer.cs#L80-L85

This, however, relies on internal implementation of STJ and is a bit hacky. IMO, JsonException should not be internally mutable in the first place.