Incorrect JsonException.Path when using non-leaf custom JsonConverters

[bart-degreed]

Description

When using System.Text.Json with a custom non-leaf JsonConverter and the input JSON is invalid, the JsonException.Path property is invalid.

Reproduction Steps

The next repro shows how the path becomes wrong when using a custom converter. Remove the brokenJson lines to observe the source JSON is identical.

The reason why we need a custom converter is because we are implementing the JSON:API specification, which dictates that the data element can contain a single item, null, or an array. Depending on context, the data element is reused at several places in the spec. It would be impractical for us to define separate data structures for all of the possible combinations that can occur.

using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;
using JsonExceptionPathBugRepro.Converters;
using JsonExceptionPathBugRepro.Objects;

namespace JsonExceptionPathBugRepro
{
    internal static class Program
    {
        public static void Main()
        {
            DeserializeWithoutCustomConverterForItem();
            DeserializeWithoutCustomConverterForArray();

            DeserializeWithCustomConverterForItem();
            DeserializeWithCustomConverterForArray();
        }

        private static void DeserializeWithoutCustomConverterForItem()
        {
            var root = new SimpleRootDocumentWithItem
            {
                Data = new ResourceObject
                {
                    Id = "1",
                    Attributes = new Dictionary<string, object?>
                    {
                        ["name"] = "John Doe"
                    }
                }
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<SimpleRootDocumentWithItem>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints correct path: "$.data.attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithoutCustomConverterForArray()
        {
            var root = new SimpleRootDocumentWithArray
            {
                Data = new List<ResourceObject>
                {
                    new()
                    {
                        Id = "1",
                        Attributes = new Dictionary<string, object?>
                        {
                            ["name"] = "John Doe"
                        }
                    }
                }
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<SimpleRootDocumentWithArray>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints correct path: "$.data[0].attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithCustomConverterForItem()
        {
            var root = new RootDocument
            {
                Data = new SingleOrManyData<ResourceObject>(new ResourceObject
                {
                    Id = "1",
                    Attributes = new Dictionary<string, object?>
                    {
                        ["name"] = "John Doe"
                    }
                })
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true,
                Converters =
                {
                    new SingleOrManyDataConverterFactory()
                }
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<RootDocument>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints "$.data" instead of "$.data.attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithCustomConverterForArray()
        {
            var root = new RootDocument
            {
                Data = new SingleOrManyData<ResourceObject>(new List<ResourceObject>
                {
                    new()
                    {
                        Id = "1",
                        Attributes = new Dictionary<string, object?>
                        {
                            ["name"] = "John Doe"
                        }
                    }
                })
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true,
                Converters =
                {
                    new SingleOrManyDataConverterFactory()
                }
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<RootDocument>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints "$.data" instead of "$.data[0].attributes.name"
                Console.WriteLine(exception.Path);
            }
        }
    }

    namespace Objects
    {
        public sealed class ResourceObject
        {
            [JsonPropertyName("id")]
            [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
            public string? Id { get; set; }

            [JsonPropertyName("attributes")]
            [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
            public IDictionary<string, object?>? Attributes { get; set; }
        }

        public sealed class SimpleRootDocumentWithItem
        {
            [JsonPropertyName("data")]
            public ResourceObject? Data { get; set; }
        }

        public sealed class SimpleRootDocumentWithArray
        {
            [JsonPropertyName("data")]
            public List<ResourceObject>? Data { get; set; }
        }

        public readonly struct SingleOrManyData<T>
            where T : class, new()
        {
            public object? Value => ManyValue != null ? ManyValue : SingleValue;

            [JsonIgnore]
            public bool IsAssigned { get; }

            [JsonIgnore]
            public T? SingleValue { get; }

            [JsonIgnore]
            public IList<T>? ManyValue { get; }

            public SingleOrManyData(object? value)
            {
                IsAssigned = true;

                if (value is IEnumerable<T> manyData)
                {
                    ManyValue = manyData.ToList();
                    SingleValue = null;
                }
                else
                {
                    ManyValue = null;
                    SingleValue = (T?)value;
                }
            }
        }

        public sealed class RootDocument
        {
            [JsonPropertyName("data")]
            public SingleOrManyData<ResourceObject> Data { get; set; }
        }
    }

    namespace Converters
    {
        /// <summary>
        /// Converts <see cref="SingleOrManyData{T}" /> to/from JSON.
        /// </summary>
        public sealed class SingleOrManyDataConverterFactory : JsonConverterFactory
        {
            public override bool CanConvert(Type typeToConvert)
            {
                return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(SingleOrManyData<>);
            }

            public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
            {
                Type objectType = typeToConvert.GetGenericArguments()[0];
                Type converterType = typeof(SingleOrManyDataConverter<>).MakeGenericType(objectType);

                return (JsonConverter)Activator.CreateInstance(converterType, BindingFlags.Instance | BindingFlags.Public, null, null, null)!;
            }

            private sealed class SingleOrManyDataConverter<T> : JsonObjectConverter<SingleOrManyData<T>>
                where T : class, new()
            {
                public override SingleOrManyData<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions serializerOptions)
                {
                    var objects = new List<T?>();
                    bool isManyData = false;
                    bool hasCompletedToMany = false;

                    do
                    {
                        switch (reader.TokenType)
                        {
                            case JsonTokenType.EndArray:
                            {
                                hasCompletedToMany = true;
                                break;
                            }
                            case JsonTokenType.Null:
                            {
                                if (isManyData)
                                {
                                    objects.Add(new T());
                                }

                                break;
                            }
                            case JsonTokenType.StartObject:
                            {
                                var resourceObject = ReadSubTree<T>(ref reader, serializerOptions);
                                objects.Add(resourceObject);
                                break;
                            }
                            case JsonTokenType.StartArray:
                            {
                                isManyData = true;
                                break;
                            }
                        }
                    }
                    while (isManyData && !hasCompletedToMany && reader.Read());

                    object? data = isManyData ? objects : objects.FirstOrDefault();
                    return new SingleOrManyData<T>(data);
                }

                public override void Write(Utf8JsonWriter writer, SingleOrManyData<T> value, JsonSerializerOptions options)
                {
                    WriteSubTree(writer, value.Value, options);
                }
            }
        }

        public abstract class JsonObjectConverter<TObject> : JsonConverter<TObject>
        {
            protected static TValue? ReadSubTree<TValue>(ref Utf8JsonReader reader, JsonSerializerOptions options)
            {
                if (typeof(TValue) != typeof(object) && options.GetConverter(typeof(TValue)) is JsonConverter<TValue> converter)
                {
                    return converter.Read(ref reader, typeof(TValue), options);
                }

                return JsonSerializer.Deserialize<TValue>(ref reader, options);
            }

            protected static void WriteSubTree<TValue>(Utf8JsonWriter writer, TValue value, JsonSerializerOptions options)
            {
                if (typeof(TValue) != typeof(object) && options.GetConverter(typeof(TValue)) is JsonConverter<TValue> converter)
                {
                    converter.Write(writer, value, options);
                }
                else
                {
                    JsonSerializer.Serialize(writer, value, options);
                }
            }
        }
    }
}

Expected behavior

The JsonException.Path value is the same for the same error in the source JSON, irrespective of whether a custom converter is used.

Actual behavior

Because the current position is tracked inside the built-in JsonConverters, there's no way for a custom converter to interact with them. And therefore there is no way to report the correct path from inside JsonException. The built-in converters just "forget" the inner scope when the path is built.

Regression?

We didn't have this problem when we were using Newtonsoft.Json.

Known Workarounds

We currently place sentinel error values in the deserialized object tree to reduce the chance of causing a JsonException. After deserialization has completed, we traverse the returned object tree, scanning for the sentinel values and throwing an exception with the proper path ourselves. We've basically re-implemented position tracking. However, this does not work in all cases (as shown in the example).

Configuration

The latest version of .NET 6. The same issue occurs in .NET 5.

Other information

I believe this is a design flaw. JsonConverter was initially only intended for leaf values. But using System.Text.Json in practice turns out to be so limiting, compared to Newtonsoft.Json, that the MSDN documentation is full of "you'll need to write a custom converter to make that work".

To reduce the pain a bit, we keep separate JsonSerializerOptions for reading and writing. That way, our write-only custom converters do not need to be registered when reading, and vice versa. It would be nice if converters could be read-only or write-only.

[ghost]

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 using System.Text.Json with a custom non-leaf JsonConverter and the input JSON is invalid, the JsonException.Path property is invalid.

Reproduction Steps

The next repro shows how the path becomes wrong when using a custom converter. Remove the brokenJson lines to observe the source JSON is identical.

The reason why we need a custom converter is because we are implementing the JSON:API specification, which dictates that the data element can contain a single item, null, or an array. Depending on context, the data element is reused at several places in the spec. It would be impractical for us to define separate data structures for all of the possible combinations that can occur.

using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;
using JsonExceptionPathBugRepro.Converters;
using JsonExceptionPathBugRepro.Objects;

namespace JsonExceptionPathBugRepro
{
    internal static class Program
    {
        public static void Main()
        {
            DeserializeWithoutCustomConverterForItem();
            DeserializeWithCustomConverterForItem();

            DeserializeWithoutCustomConverterForArray();
            DeserializeWithCustomConverterForArray();
        }

        private static void DeserializeWithoutCustomConverterForItem()
        {
            var root = new SimpleRootDocumentWithItem
            {
                Data = new ResourceObject
                {
                    Id = "1",
                    Attributes = new Dictionary<string, object?>
                    {
                        ["name"] = "John Doe"
                    }
                }
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<SimpleRootDocumentWithItem>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints correct path: "$.data.attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithoutCustomConverterForArray()
        {
            var root = new SimpleRootDocumentWithArray
            {
                Data = new List<ResourceObject>
                {
                    new()
                    {
                        Id = "1",
                        Attributes = new Dictionary<string, object?>
                        {
                            ["name"] = "John Doe"
                        }
                    }
                }
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<SimpleRootDocumentWithArray>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints correct path: "$.data[0].attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithCustomConverterForItem()
        {
            var root = new RootDocument
            {
                Data = new SingleOrManyData<ResourceObject>(new ResourceObject
                {
                    Id = "1",
                    Attributes = new Dictionary<string, object?>
                    {
                        ["name"] = "John Doe"
                    }
                })
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true,
                Converters =
                {
                    new SingleOrManyDataConverterFactory()
                }
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<RootDocument>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints "$.data" instead of "$.data.attributes.name"
                Console.WriteLine(exception.Path);
            }
        }

        private static void DeserializeWithCustomConverterForArray()
        {
            var root = new RootDocument
            {
                Data = new SingleOrManyData<ResourceObject>(new List<ResourceObject>
                {
                    new()
                    {
                        Id = "1",
                        Attributes = new Dictionary<string, object?>
                        {
                            ["name"] = "John Doe"
                        }
                    }
                })
            };

            var options = new JsonSerializerOptions
            {
                WriteIndented = true,
                Converters =
                {
                    new SingleOrManyDataConverterFactory()
                }
            };

            string json = JsonSerializer.Serialize(root, options);
            string brokenJson = json.Replace("John Doe", "John \"Doe");

            try
            {
                JsonSerializer.Deserialize<RootDocument>(brokenJson, options);
            }
            catch (JsonException exception)
            {
                // prints "$.data" instead of "$.data[0].attributes.name"
                Console.WriteLine(exception.Path);
            }
        }
    }

    namespace Objects
    {
        public sealed class ResourceObject
        {
            [JsonPropertyName("id")]
            [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
            public string? Id { get; set; }

            [JsonPropertyName("attributes")]
            [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
            public IDictionary<string, object?>? Attributes { get; set; }
        }

        public sealed class SimpleRootDocumentWithItem
        {
            [JsonPropertyName("data")]
            public ResourceObject? Data { get; set; }
        }

        public sealed class SimpleRootDocumentWithArray
        {
            [JsonPropertyName("data")]
            public List<ResourceObject>? Data { get; set; }
        }

        public readonly struct SingleOrManyData<T>
            where T : class, new()
        {
            public object? Value => ManyValue != null ? ManyValue : SingleValue;

            [JsonIgnore]
            public bool IsAssigned { get; }

            [JsonIgnore]
            public T? SingleValue { get; }

            [JsonIgnore]
            public IList<T>? ManyValue { get; }

            public SingleOrManyData(object? value)
            {
                IsAssigned = true;

                if (value is IEnumerable<T> manyData)
                {
                    ManyValue = manyData.ToList();
                    SingleValue = null;
                }
                else
                {
                    ManyValue = null;
                    SingleValue = (T?)value;
                }
            }
        }

        public sealed class RootDocument
        {
            [JsonPropertyName("data")]
            public SingleOrManyData<ResourceObject> Data { get; set; }
        }
    }

    namespace Converters
    {
        /// <summary>
        /// Converts <see cref="SingleOrManyData{T}" /> to/from JSON.
        /// </summary>
        public sealed class SingleOrManyDataConverterFactory : JsonConverterFactory
        {
            public override bool CanConvert(Type typeToConvert)
            {
                return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(SingleOrManyData<>);
            }

            public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
            {
                Type objectType = typeToConvert.GetGenericArguments()[0];
                Type converterType = typeof(SingleOrManyDataConverter<>).MakeGenericType(objectType);

                return (JsonConverter)Activator.CreateInstance(converterType, BindingFlags.Instance | BindingFlags.Public, null, null, null)!;
            }

            private sealed class SingleOrManyDataConverter<T> : JsonObjectConverter<SingleOrManyData<T>>
                where T : class, new()
            {
                public override SingleOrManyData<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions serializerOptions)
                {
                    var objects = new List<T?>();
                    bool isManyData = false;
                    bool hasCompletedToMany = false;

                    do
                    {
                        switch (reader.TokenType)
                        {
                            case JsonTokenType.EndArray:
                            {
                                hasCompletedToMany = true;
                                break;
                            }
                            case JsonTokenType.Null:
                            {
                                if (isManyData)
                                {
                                    objects.Add(new T());
                                }

                                break;
                            }
                            case JsonTokenType.StartObject:
                            {
                                var resourceObject = ReadSubTree<T>(ref reader, serializerOptions);
                                objects.Add(resourceObject);
                                break;
                            }
                            case JsonTokenType.StartArray:
                            {
                                isManyData = true;
                                break;
                            }
                        }
                    }
                    while (isManyData && !hasCompletedToMany && reader.Read());

                    object? data = isManyData ? objects : objects.FirstOrDefault();
                    return new SingleOrManyData<T>(data);
                }

                public override void Write(Utf8JsonWriter writer, SingleOrManyData<T> value, JsonSerializerOptions options)
                {
                    WriteSubTree(writer, value.Value, options);
                }
            }
        }

        public abstract class JsonObjectConverter<TObject> : JsonConverter<TObject>
        {
            protected static TValue? ReadSubTree<TValue>(ref Utf8JsonReader reader, JsonSerializerOptions options)
            {
                if (typeof(TValue) != typeof(object) && options.GetConverter(typeof(TValue)) is JsonConverter<TValue> converter)
                {
                    return converter.Read(ref reader, typeof(TValue), options);
                }

                return JsonSerializer.Deserialize<TValue>(ref reader, options);
            }

            protected static void WriteSubTree<TValue>(Utf8JsonWriter writer, TValue value, JsonSerializerOptions options)
            {
                if (typeof(TValue) != typeof(object) && options.GetConverter(typeof(TValue)) is JsonConverter<TValue> converter)
                {
                    converter.Write(writer, value, options);
                }
                else
                {
                    JsonSerializer.Serialize(writer, value, options);
                }
            }
        }
    }
}

Expected behavior

The JsonException.Path value is the same for the same error in the source JSON, irrespective of whether a custom converter is used.

Actual behavior

Because the current position is tracked inside the built-in JsonConverters, there's no way for a custom converter to interact with them. And therefore there is no way to report the correct path from inside JsonException. The built-in converters just "forget" the outer scope when the path is built.

Regression?

We didn't have this problem when we were using Newtonsoft.Json.

Known Workarounds

We currently place sentinel error values in the deserialized object tree to reduce the chance of causing a JsonException. After deserialization has completed, we traverse the returned object tree, scanning for the sentinel values and throwing an exception with the proper path ourselves. We've basically re-implemented position tracking. However, this does not work in all cases (as shown in the example).

Configuration

The latest version of .NET 6. The same issue occurs in .NET 5.

Other information

I believe this is a design flaw. JsonConverter was initially only intended for leaf values. But using System.Text.Json in practice turns out to be so limiting, compared to Newtonsoft.Json, that the MSDN documentation is full of "you'll need to write a custom converter to make that work".

To reduce the pain a bit, we keep separate JsonSerializerOptions for reading and writing. That way, our write-only custom converters do not need to be registered, and vice versa. It would be nice if converters could be read-only or write-only.

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

[bart-degreed]

More details are described at json-api-dotnet/JsonApiDotNetCore#1075, along with other pain points we faced while migrating from Newtonsoft.Json.

[eiriktsarpalis]

This is a known issue with custom converters: the public Read/Write methods do not pass the serialization state (which includes the JSON path) resulting in loss of information when calling into custom converters. We are planning on mitigating this via #63795.

[bart-degreed]

I was asked by @krwq to open an issue for this. We both searched through existing issues first. So I find it odd this is now closed as a duplicate, without even mentioning the problem in the linked issue.

I get that the team takes customer feedback, analyzes it, and then creates design issues to resolve them. But handling it like this makes it hard for anyone running into the same problem to find where this is tracked, resulting in more duplicates.

[eiriktsarpalis]

@bart-degreed just so I understand your feedback correctly, do you take issue with #63795 not being shared with you earlier, with this issue being closed or that the linked user story doesn't explicitly list the particular manifestation of the bug as part of its acceptance criteria?

From an area owner's perspective it doesn't make a lot of sense to keep open issues that are effectively different manifestations of the same underlying root cause (which is why #51715 was also closed in favor of #63795). Having a consolidated backlog means that we can more easily prioritize feature work. As far as creating duplicates is concerned, that's also not an issue -- we will close as appropriate. If anything a user story linking to multiple user reported issues (closed or otherwise) is a healthy indication of a feature's impact.

[bart-degreed]

@bart-degreed just so I understand your feedback correctly, do you take issue with #63795 not being shared with you earlier, with this issue being closed or that the linked user story doesn't explicitly list the particular manifestation of the bug as part of its acceptance criteria?

The last one. What happened is that I brought up my problem here. Then a Microsoft employee performs a search, even mentions #63795, but still concludes my problem isn't tracked anywhere yet, and asks me to create an issue for it. So I did. Then after I did all that work, another Microsoft employee closes it. This makes me feel I did all the work for nothing.

I can imagine that the description of #63795 is clear in itself to you. But this is open-source, not an internal work tracking system. For outsiders like me, it's not at all obvious that fixing #63795 will address the incorrect JSON path. So yes, I'd like my case added to the user story. To ensure fixing this is considered part of its scope, but also to make it easier for future users hitting the same problem to be able to find it.

[eiriktsarpalis]

What happened is that I brought up my problem #63686 (comment). Then a Microsoft employee performs a search, even mentions #63795, but still concludes my problem isn't tracked anywhere yet, and #63686 (comment) me to create an issue for it. So I did. Then after I did all that work, another Microsoft employee closes it. This makes me feel I did all the work for nothing.

It cannot be expected that every Microsoft employee has full knowledge or understanding of the entire issue backlog, and that includes myself. Sometimes multiple folks need to chime in before such a determination can be made -- it's just the nature of github repos. I'm sorry if you feel that this was wasted effort on your part, but from our perspective this is clearly not the case.

I can imagine that the description of #63795 is clear in itself to you. But this is open-source, not an internal work tracking system. For outsiders like me, it's not at all obvious that fixing #63795 will address the incorrect JSON path. So yes, I'd like my case added to the user story. To ensure fixing this is considered part of its scope, but also to make it easier for future users hitting the same problem to be able to find it.

Your issue has already been linked to the user story. In my original response I provided you with a high-level explanation of the root cause and our future plans to mitigate (not fix) the issue you are experiencing. The user story itself ontains more details and we're always happy to provide more context should questions arise.

but also to make it easier for future users hitting the same problem to be able to find it.

Even though this issue has been closed users will still be able to find it when they search for this particular problem.