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

Serialization of properties from inherited interfaces vs abstract base classes #41749

Closed
Tracked by #73255 ...
qsdfplkj opened this issue Sep 2, 2020 · 20 comments · Fixed by #78788
Closed
Tracked by #73255 ...

Serialization of properties from inherited interfaces vs abstract base classes #41749

qsdfplkj opened this issue Sep 2, 2020 · 20 comments · Fixed by #78788
Assignees
Labels
area-System.Text.Json partner-impact This issue impacts a partner who needs to be kept updated Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@qsdfplkj
Copy link
Contributor

qsdfplkj commented Sep 2, 2020

I would expect that inherited properties of interfaces would be serialized similar to (abstract) base classes.

Given:

public interface IBase
{
    string BaseProperty { get; set; }
}

public interface IDerived : IBase
{
    string DerivedProperty { get; set; }
}

public class Derived : IDerived
{
    public string BaseProperty { get; set; }
    public string DerivedProperty { get; set; }
}

public class Data 
{
    public IDerived Derived { get; set; }
}

Data data = new Data 
{ 
    Derived = new Derived
    { 
        BaseProperty = "B", 
        DerivedProperty = "D" 
    } 
};
string json = JsonSerializer.Serialize(data);

Where the json string is missing the BaseProperty:

{"Derived":{"DerivedProperty":"D"}}

replacing the interface with an abstract class results in:

{"Derived":{"DerivedProperty":"D","BaseProperty":"B"}}

public abstract class  IBase
{
    public string BaseProperty { get; set; }
}

public abstract class IDerived : IBase
{
    public string DerivedProperty { get; set; }
}

public class Derived : IDerived
{
}

public class Data 
{
    public IDerived Derived { get; set; }
}
@Dotnet-GitSync-Bot
Copy link
Collaborator

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-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 2, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Sep 4, 2020
@layomia layomia added this to the 5.0.0 milestone Sep 4, 2020
@layomia
Copy link
Contributor

layomia commented Sep 4, 2020

System.Text.Json uses the declared type (typeof(IDerived)) to determine what properties to serialize, and does not provide polymorphic serialization by inspecting the runtime type. See #29937 which tracks an opt-in feature to enable this behavior. The reason the abstract class example works is that we can discover the base properties from the derived type.

Also see a corresponding issue for polymorphic deserialization - #30083.

@layomia layomia closed this as completed Sep 4, 2020
@qsdfplkj
Copy link
Contributor Author

qsdfplkj commented Sep 4, 2020

It is not about the runtime type. Why does it not discover the base properties from the base interface.

@layomia
Copy link
Contributor

layomia commented Sep 8, 2020

Discovering the base properties from base interfaces can be tracked individually from (but designed/implemented alongside) the polymorphic (de)serialization features. We should probably do it by default (potentially breaking as more properties may be (de)serialized), but provide an option to turn off this behavior.

This is not a regression from 3.1, so I'll set the milestone as 6.0.

@layomia
Copy link
Contributor

layomia commented Feb 2, 2021

From @ramondeklein in #47753:

Description

It seems that System.Text.Json.JsonSerializer only serializes the properties of the top-level interface, instead of all the underlying interface. Suppose the following code:

public interface IBase
{
    int Count { get; set; }
}

public interface IDerived : IBase
{
    string Name { get; set; }
}

public class Implementation : IDerived
{
    public int Count { get; set; }
    public string Name { get; set; }
}

internal static class Program
{
    private static void Main(string[] args)
    {
        var obj = new Implementation {Count = 1, Name = "Test"};

        // {"Count":1,"Name":"Test"}
        System.Console.WriteLine(System.Text.Json.JsonSerializer.Serialize(obj));

        // {"Name":"Test"}   <-- Count is not serialized, because it's defined in a base interface
        System.Console.WriteLine(System.Text.Json.JsonSerializer.Serialize((IDerived)obj));
    }
}

When serializing the object as IDerived then only the properties that are defined in this interface are exported. I could imagine this behaviour when IDerived wasn't derived from IBase and when Implementation would derive from both base interfaces. But the properties of IDerived should include also the base properties to make sure you can serialize the object appropriately.

I am familiar that the behaviour has changed (compared to the Newtonsoft.Json serializer), but this behaviour is very unexpected and doesn't seem right. The properties of the base interface are part of IDerived and should be serialized. I couldn't think of a reason not to (de)serialize the base properties, but if it's breaking existing code, then please add a property to opt-in for base-property serialization.

@shaggygi
Copy link
Contributor

Anybody know which preview this feature will start to light up in? Just curious.

@qsdfplkj
Copy link
Contributor Author

qsdfplkj commented May 30, 2021

When I think of it. This is just how it should be. Any type that deserializes to iderived should be sufficient. (Inherited interfaces are just runtime implementations) If you have polymorphic serialization it would be the runtime type. For which I created a polymorphic converter. Which is pretty straight forward. Please check the converter here #29937 (comment)

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 5, 2021

Inherited interfaces are in some ways similar to implementation inheritance: multiple inheritance is not permitted and members of the base interface are considered part of the signature for the derived interface. The hierarchy

public interface IBase
{
    string BaseProperty { get; set; }
}

public interface IDerived : IBase
{
    string DerivedProperty { get; set; }
}

should result in the same JSON contract as:

public class Base
{
    public string BaseProperty { get; set; }
}

public class Derived : Base
{
    public string DerivedProperty { get; set; }
}

BaseProperty should only be skipped in the case of explicit interface implementations (this is something STJ is already honoring):

public interface IBase
{
    string BaseProperty { get; set; }
}

public class Derived : IBase
{
    public string DerivedProperty { get; set; }
    string IBase.BaseProperty { get; set; } // not serialized
}

@qsdfplkj
Copy link
Contributor Author

qsdfplkj commented Jun 5, 2021

Maybe explicit interface can just be named explicitly “Ibase.baseproperty”

@eiriktsarpalis
Copy link
Member

We should check what the behaviour of Newtonsoft.Json is in this case before making the change.

@layomia
Copy link
Contributor

layomia commented Jul 23, 2021

We didn't get to this in .NET 6 and any change here is risky given where we are in the release cycle. We can consider for .NET 7.

@dancojocaru2000
Copy link

We didn't get to this in .NET 6

Well that's disappointing.

Abstract classes are the only alternative for now?

@eiriktsarpalis
Copy link
Member

Sharing #73121 (comment) since it turns out that addressing this issue is less trivial that what we might have originally thought.

@krwq
Copy link
Member

krwq commented Oct 21, 2022

I'm bumping priority here - we should at least consider 8.0 fix here

@ramondeklein
Copy link

It's pretty unfortunate that after several years of System.Text.Json this kind of basic functionality is still not present. I have created a workaround.

I created a JsonConverter that writes out the actual underlying type:

public class JsonSerializeImplementation<TInterface> : JsonConverter<TInterface>
{
    public override TInterface Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => throw new NotSupportedException();

    public override void Write(Utf8JsonWriter writer, TInterface value, JsonSerializerOptions options)
    {
        if (value == null) throw new ArgumentNullException(nameof(value));
        JsonSerializer.Serialize(writer, value, value.GetType(), options);
    }
}

This can be added to an interface by annotating the type with JsonConverter(typeof(JsonSerializeImplementation<...>)). But to make life a bit easier, I also added an attribute that can be applied:

[AttributeUsage(AttributeTargets.Interface)]
public class JsonSerializeImplementationAttribute : Attribute
{
}

Of course, just adding an attribute to a type doesn't make a real difference, so I created a JsonConverterFactory that creates the proper JsonConverter if the JsonSerializeImplementationAttribute attribute is applied to an interface.

public class JsonSerializeImplementationConverter : JsonConverterFactory
{
    public override bool CanConvert(Type typeToConvert) =>
        typeToConvert.GetCustomAttribute<JsonSerializeImplementationAttribute>() != null;

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
        => (JsonConverter?)Activator.CreateInstance(
            typeof(JsonSerializeImplementation<>).MakeGenericType(typeToConvert),
            BindingFlags.Instance | BindingFlags.Public,
            null,
            Array.Empty<object>(),
            null);
}

All you need to do is to register this factory using services.AddTransient<JsonConverter,JsonSerializeImplementationConverter>().

There are some severe drawbacks to this solution:

  • It only supports writing the type.
  • It writes the actual implementation that may contain much more properties, so you may serialize properties that are not in the interface.

@krwq
Copy link
Member

krwq commented Nov 23, 2022

There exist simple way to workaround this scenario currently (more accurate one is further below):

using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;

Data data = new Data
{
    Derived = new Derived
    {
        BaseProperty = "B",
        DerivedProperty = "D"
    }
};

Console.WriteLine(JsonSerializer.Serialize(data));
// {"Derived":{"$type":"Derived","BaseProperty":"B","DerivedProperty":"D"}}

public interface IBase
{
    string BaseProperty { get; set; }
}

[JsonPolymorphic]
[JsonDerivedType(typeof(Derived), "Derived")]
public interface IDerived : IBase
{
    string DerivedProperty { get; set; }
}

public class Derived : IDerived
{
    public string BaseProperty { get; set; }
    public string DerivedProperty { get; set; }
}

public class Data
{
    public IDerived Derived { get; set; }
}

and slightly more elaborate one:

  • adding properties we want at the contract level - this code has been only tested on the original code - adjust if needed:
using System.Collections.Generic;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;

Data data = new Data
{
    Derived = new Derived
    {
        BaseProperty = "B",
        DerivedProperty = "D"
    }
};

JsonSerializerOptions options = new()
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver()
    {
        Modifiers =
        {
            AddAllInterfacesModifier
        }
    }
};

Console.WriteLine(JsonSerializer.Serialize(data, options));
// {"Derived":{"DerivedProperty":"D","BaseProperty":"B"}}

static void AddAllInterfacesModifier(JsonTypeInfo typeInfo)
{
    if (typeInfo.Kind != JsonTypeInfoKind.Object)
        return;

    if (typeInfo.Type.IsInterface)
    {
        Type[] interfaces = typeInfo.Type.GetInterfaces();
        if (interfaces.Length == 0)
            return;

        Dictionary<string, JsonPropertyInfo> properties = new();
        foreach (var prop in typeInfo.Properties)
        {
            properties.Add(prop.Name, prop);
        }

        foreach (var inter in typeInfo.Type.GetInterfaces())
        {
            var interTypeInfo = typeInfo.Options.GetTypeInfo(inter);

            foreach (var prop in interTypeInfo.Properties)
            {
                JsonPropertyInfo propUnderNewTypeInfo = CopyProperty(prop, typeInfo);
                if (properties.TryAdd(prop.Name, propUnderNewTypeInfo))
                {
                    // we don't want to add duplicate names here
                    typeInfo.Properties.Add(propUnderNewTypeInfo);
                }
            }
        }
    }
}

static JsonPropertyInfo CopyProperty(JsonPropertyInfo property, JsonTypeInfo newTypeInfo)
{
    JsonPropertyInfo copy = newTypeInfo.CreateJsonPropertyInfo(property.PropertyType, property.Name);
    copy.AttributeProvider = property.AttributeProvider;
    copy.Order = property.Order;
    copy.Set = property.Set;
    copy.Get = property.Get;
    copy.CustomConverter = property.CustomConverter;
    copy.IsExtensionData = property.IsExtensionData;
    copy.IsRequired = property.IsRequired;
    copy.NumberHandling = property.NumberHandling;
    copy.ShouldSerialize = property.ShouldSerialize;
    return copy;
}

public interface IBase
{
    string BaseProperty { get; set; }
}

public interface IDerived : IBase
{

    string DerivedProperty { get; set; }
}

public class Derived : IDerived
{
    public string BaseProperty { get; set; }
    public string DerivedProperty { get; set; }
}

public class Data
{
    public IDerived Derived { get; set; }
}

@krwq krwq modified the milestones: 8.0.0, Future Nov 23, 2022
@eiriktsarpalis eiriktsarpalis added the partner-impact This issue impacts a partner who needs to be kept updated label Nov 23, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Nov 23, 2022
@eiriktsarpalis
Copy link
Member

Let's keep this in 8.0.0 since it's being requested by a partner team.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 23, 2022
@angelaki
Copy link

I guess .Net offers pretty much solutions to serialize interfaces the way you expect them to. I don't think the framework needs to provide default converters for it. How about explicit property implementations? Keep them? Thats something the class decides, not the interface. So you can't tell for sure. How about overlapping properties? Just to name some possible issues. JTS7 offers quite perfect solutions to customize (interface) serialization your way. By the way: a big thank you to the team for this nice lib.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 25, 2022
@eiriktsarpalis
Copy link
Member

How about explicit property implementations? Keep them? Thats something the class decides, not the interface.

Note that this issue concerns contracts of interface types, not classes. Using the example given in the OP, the change would impact the serialization contract of values serialized as IDerived, but would have no effect if the same value gets serialized as Derived. From the perspective of an interface hierarchy, there is no such thing as an explicit implementation.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json partner-impact This issue impacts a partner who needs to be kept updated Priority:2 Work that is important, but not critical for the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants