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

ReferenceHandler.IgnoreCycles doesn't work with Custom Converters #51715

Closed
YeskaNova opened this issue Apr 22, 2021 · 10 comments
Closed

ReferenceHandler.IgnoreCycles doesn't work with Custom Converters #51715

YeskaNova opened this issue Apr 22, 2021 · 10 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@YeskaNova
Copy link

I need to add $type property for an array that will contain objects of different types ex : ( [A,B,C] of IEnumerable< A> => [A,{$type:"B",$value:B},{$type:"C",$value:C}] )
My custom JsonConverter will have a JsonSerializer.Serialize in the Write method but that resets the state of the serialization so It can no more check the cycles :

        private abstract class AbstractEnumerableJsonConverter<TObj, TCollection> :
            JsonConverter<TCollection> where TCollection: IEnumerable<TObj>
        {


            public override TCollection? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                var list = new List<TObj>();
                while (reader.Read())
                {
                    if (reader.TokenType == JsonTokenType.EndArray)
                    {
                        break;
                    }
                    if (reader.TokenType == JsonTokenType.StartObject)
                    {
                        var deserializedObj = default(TObj);
                        var document = JsonDocument.ParseValue(ref reader);
                        if (document.RootElement.TryGetProperty("$type", out var typeName))
                        {
                            var type = Type.GetType(typeName.GetString());
                            if (type.IsAssignableTo(typeof(IDbo)))
                                deserializedObj = (TObj)JsonSerializer.Deserialize(document.RootElement.GetProperty("$value").GetRawText(), type, options);
                        }
                        deserializedObj ??= (TObj)JsonSerializer.Deserialize(document.RootElement.ToString(), typeof(TObj), options);
                        list.Add(deserializedObj);
                    }
                }
                var ret = (IList)Activator.CreateInstance(typeToConvert, list.Count);
                if (ret.Count == 0)
                    for (var i = 0; i < list.Count; ++i)
                        ret.Add(list[i]);
                else
                    for (var i = 0; i < ret.Count; ++i)
                        ret[i] = list[i];

                return (TCollection)ret;
            }
            
            public override void Write(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options)
            {
                writer.WriteStartArray();
                foreach (var obj in value)
                {
                    if( obj.GetType() != typeof(TObj))
                    {
                        writer.WriteStartObject();
                        writer.WriteString("$type", obj.GetType().FullName);
                        writer.WritePropertyName("$value");
                        JsonSerializer.Serialize(writer, obj, options);   //<---- The state is lost so we can no more check cycles**
                        writer.WriteEndObject();
                    }
                    else
                    JsonSerializer.Serialize(writer, obj, options);       //<---- The state is lost so we can no more check cycles**
                }
                writer.WriteEndArray();
            }
        }

I have tried to do something like dotnet/docs#21777 (comment) , but it seems that IgnoreCycles relies on a lot of things that are internal and resides in the Core code ( like PushReferenceForCycleDetection that maybe would help if it's protected and not internal

internal override void PushReferenceForCycleDetection(object value)
).

Is there any way I can get around these cycles?

(I have also tried to serialize the properties of the object one by one to avoid calling JsonSerializer.Serialize, but the code to enumerate one object's properties is also internal )

Originally posted by @YeskaNova in dotnet/docs#21777 (comment)

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 22, 2021
@ghost
Copy link

ghost commented Apr 23, 2021

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

Issue Details

I need to add $type property for an array that will contain objects of different types ex : ( [A,B,C] of IEnumerable< A> => [A,{$type:"B",$value:B},{$type:"C",$value:C}] )
My custom JsonConverter will have a JsonSerializer.Serialize in the Write method but that resets the state of the serialization so It can no more check the cycles :

        private abstract class AbstractEnumerableJsonConverter<TObj, TCollection> :
            JsonConverter<TCollection> where TCollection: IEnumerable<TObj>
        {


            public override TCollection? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                var list = new List<TObj>();
                while (reader.Read())
                {
                    if (reader.TokenType == JsonTokenType.EndArray)
                    {
                        break;
                    }
                    if (reader.TokenType == JsonTokenType.StartObject)
                    {
                        var deserializedObj = default(TObj);
                        var document = JsonDocument.ParseValue(ref reader);
                        if (document.RootElement.TryGetProperty("$type", out var typeName))
                        {
                            var type = Type.GetType(typeName.GetString());
                            if (type.IsAssignableTo(typeof(IDbo)))
                                deserializedObj = (TObj)JsonSerializer.Deserialize(document.RootElement.GetProperty("$value").GetRawText(), type, options);
                        }
                        deserializedObj ??= (TObj)JsonSerializer.Deserialize(document.RootElement.ToString(), typeof(TObj), options);
                        list.Add(deserializedObj);
                    }
                }
                var ret = (IList)Activator.CreateInstance(typeToConvert, list.Count);
                if (ret.Count == 0)
                    for (var i = 0; i < list.Count; ++i)
                        ret.Add(list[i]);
                else
                    for (var i = 0; i < ret.Count; ++i)
                        ret[i] = list[i];

                return (TCollection)ret;
            }
            
            public override void Write(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options)
            {
                writer.WriteStartArray();
                foreach (var obj in value)
                {
                    if( obj.GetType() != typeof(TObj))
                    {
                        writer.WriteStartObject();
                        writer.WriteString("$type", obj.GetType().FullName);
                        writer.WritePropertyName("$value");
                        JsonSerializer.Serialize(writer, obj, options);   //<---- The state is lost so we can no more check cycles**
                        writer.WriteEndObject();
                    }
                    else
                    JsonSerializer.Serialize(writer, obj, options);       //<---- The state is lost so we can no more check cycles**
                }
                writer.WriteEndArray();
            }
        }

I have tried to do something like dotnet/docs#21777 (comment) , but it seems that IgnoreCycles relies on a lot of things that are internal and resides in the Core code ( like PushReferenceForCycleDetection that maybe would help if it's protected and not internal

internal override void PushReferenceForCycleDetection(object value)
).

Is there any way I can get around these cycles?

(I have also tried to serialize the properties of the object one by one to avoid calling JsonSerializer.Serialize, but the code to enumerate one object's properties is also internal )

Originally posted by @YeskaNova in dotnet/docs#21777 (comment)

Author: YeskaNova
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Hi @YeskaNova, this is by design. The reference resolver is stored in the WriteStack instance which is initialized in every call of JsonSerializer.Serialize. However, it seems that even if I replace your code with a call to a nested converter:

public override void Write(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options)
{
    JsonConverter<TObj> converter = (JsonConverter<TObj>)options.GetConverter(typeof(TObj));

    writer.WriteStartArray();
    foreach (var obj in value)
    {
        converter.Write(writer, obj, options);
    }
    writer.WriteEndArray();
}

The new WriteStack will also be initialized with a fresh ReferenceResolver instance. @jozkee @layomia is this by design?

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Apr 23, 2021

Also @YeskaNova do note that using Type.GetType to load a type name from the wire is strongly discouraged, since under certain circumstances it could allow attackers to do remote code execution on your machine.

Related to #45189

@jozkee
Copy link
Member

jozkee commented Apr 23, 2021

The new WriteStack will also be initialized with a fresh ReferenceResolver instance.

Yes, it was by desing but can be addressed either by one of these options:

@YeskaNova did you try implement your own ReferenceResolver that uses Preserve semantics and root it as described in dotnet/docs#21777 (comment)? Preserve is also useful for break cycles.

@YeskaNova
Copy link
Author

YeskaNova commented Apr 23, 2021

@jozkee I see what you mean, I'll try to do that and update.

@YeskaNova
Copy link
Author

@jozkee It can work but that will add "$id" everywhere and also forces to reset the reference handler after each use, it seems "hacky", it's better if we can find a solution that fits in the logic without bloating the code.

@YeskaNova
Copy link
Author

@jozkee Can't we have a method like Utf8JsonWriter.WriteObject(object) ? It will give a lot of flexibility for customconverters, because now if we want to custom-convert even one small property, we will have to write an exhaustive code that should work for all kinds of properties and object types.

@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 Jul 21, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jul 21, 2021
@eiriktsarpalis
Copy link
Member

This should be addressed by #36785.

@eiriktsarpalis
Copy link
Member

Closing in favor of #63795.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2022
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
Projects
None yet
Development

No branches or pull requests

4 participants