System.Text.Json object cycle error message could be better

[meziantou]

Description

When migrating to S.T.Json from Newtonsoft.Json I got an error when serializing a CultureInfo.

JsonSerializer.Serialize(CultureInfo.GetCultureInfo("en-US"));

This is ok that the json serializer doesn't handle every possible type. In this case I wrote a custom JsonConverter and it works well.

However, it tooks me a few minutes to find the problematic property in my model as the message/call stack doesn't include any useful data.

Unhandled exception. System.Text.Json.JsonException: A possible object cycle was detected. This can either be due to a cycle or if the object depth is larger than the maximum allowed depth of 64. Consider using ReferenceHandler.Preserve on JsonSerializerOptions to support cycles.
   at System.Text.Json.ThrowHelper.ThrowJsonException_SerializerCycleDetected(Int32 maxDepth)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonPropertyInfo`1.GetMemberAndWriteJson(Object obj, WriteStack& state, Utf8JsonWriter writer)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
...
   at System.Text.Json.JsonPropertyInfo`1.GetMemberAndWriteJson(Object obj, WriteStack& state, Utf8JsonWriter writer)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonSerializer.WriteCore[TValue](JsonConverter jsonConverter, Utf8JsonWriter writer, TValue value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonSerializer.WriteCore[TValue](Utf8JsonWriter writer, TValue value, Type inputType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, Type inputType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
   at ConsoleApp6.Program.Main(String[] args) in C:\Users\user\source\repos\ConsoleApp6\ConsoleApp6\Program.cs:line 12

I think this could be useful to add the name of the property and its type when this exception is thrown. Maybe it's even possible to get the full property path. In my case, knowing that the problematic property is "Parent" of type "CultureInfo" would have helped.

BTW, S.T.Json could also support serializing CultureInfo out of the box but this is another issue.

Configuration

<PackageReference Include="System.Text.Json" Version="5.0.0-preview.6.20305.6" />

[layomia]

cc @jozkee

[jozkee]

Maybe it's even possible to get the full property path.

@meziantou can you try looking at JsonException.Path property, is it of any help?
How did you end up solving this issue? Did you try the suggested workaround of setting ReferenceHandler.Preserve?

[meziantou]

can you try looking at JsonException.Path property, is it of any help?

Indeed, JsonException.Path contains the full path which is very nice:

$.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent

Maybe it should be part of the exception message, so it would be displayed in JsonException.ToString().

How did you end up solving this issue?

I use a custom JsonConverter for CultureInfo. It uses CultureInfo.Name when serializing and use CultureInfo.Get() when deserializing.

Did you try the suggested workaround of setting ReferenceHandler.Preserve?

I don't think this is a good workaround here. Even if that work for serialization, I don't think it will correctly instantiate the CultureInfo when deserializing.

[jozkee]

I think including the full JSON path here could be too cumbersome, I was thinking that we could append the last property name before throwing but that would not work well for reference loops in arrays (e.g: List<object> which has an element that points to itself).

I use a custom JsonConverter for CultureInfo.

@meziantou that's a nice workaround.

[meziantou]

Adding the full path could be useful. In this case the Parent property is not part of my code (it is a property of System.Globalization.CultureInfo), so just knowing this name may not be that useful for debugging.
The full path would be useful to know the actual property to fix but I understand this could be cumbersome. Maybe the path could be trimmed to only keep the path before the cycle (e.g. $.A.B.C.Parent).

Another idea is to change the message to indicate that the Path property exists and it could be useful for debugging.

[jozkee]

Maybe the path could be trimmed to only keep the path before the cycle (e.g. $.A.B.C.Parent).

That implies saving a reference of each object in the current object branch and then removing them when we are done with it, that's probably very expensive. The fact about that error message is that we are not detecting a cycle, we are just validating that the MaxDepth is not exceeded.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Lines 241 to 244 in 682141e

	if (writer.CurrentDepth >= options.EffectiveMaxDepth)
	{
	ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth);
	}

However I think maybe we can do something to make the message clearer as you suggests.

[raffaeler]

Other execeptions in System.Text.Json already provide the Path in the message.
I expected to see that piece of information here as well.
Can't the path eventually be rebuilt in the case of the exception without affecting the general perf?

[layomia]

May I ask why CultureInfo is being serialized in the first place? Seems like one of the types we should explicitly not support.

I think the object-cycle exception message is fine for general use-cases, just that a NotSupportedException would be more appropriate when serializing CultureInfo.

[meziantou]

May I ask why CultureInfo is being serialized in the first place? Seems like one of the types we should explicitly not support.

I converted a code from Newtonsoft.Json to S.T.Json. Using Newtonsoft.Json, Console.WriteLine(JsonConvert.SerializeObject(CultureInfo.GetCultureInfo("en-US"))); works and serialize the culture name: "en-US"

[ultimaweapon]

@layomia may I ask what is the reason why CultureInfo should not serialized?

[MaxAtoms]

The path has been added to the exception text in the meantime.
It was added with #56903, if I am not mistaken: ThrowJsonException_SerializerCycleDetected. From my point of view, this issue can be closed.