-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add AttributeTargets.Interface to JsonConverterAttribute #33112
Comments
I found a nice workaround. But still, I think it should be supported out of the box with JsonConverterAttribute. [AttributeUsage(AttributeTargets.Interface, AllowMultiple = false)]
public class JsonInterfaceConverterAttribute : JsonConverterAttribute
{
public JsonInterfaceConverterAttribute(Type converterType)
: base(converterType)
{
}
} [JsonInterfaceConverter(typeof(MyConverter))]
public interface IMyInterface
{
// ...
} |
Are you assuming only supporting the first interface that has a Currently the converter model only allows for a single converter for a given Type (each property can have its own converter however). |
I'm not sure I understand the question. The first interface of what? For example, I can write: JsonSerializer.Deserialize<IMyInterface>(text);
JsonSerializer.Serialize<IMyInterface>(instance); or class MyClass
{
public IMyInterface Instance { get; set; }
}
JsonSerializer.Deserialize<MyClass>(text);
JsonSerializer.Serialize<MyClass>(instance); And if IMyInterface has a JsonConverterAttribute attribute, it doesn't matter whether IMyInterface is a class or an abstract class or an interface. So, there is no polymorphism involved on a serializer level, it may be involved only on a converter level, but it is up to user. |
We could support interfaces today given the current semantics:
This would be the behavior: [JsonConverter(typeof(IFoo1Impl))] interface IFoo1{ ... }
[JsonConverter(typeof(IFoo2Impl))] interface IFoo2{ ... }
class MyClass : IFoo1, IFoo2 { ... }
// No custom converter would get called
object obj = new MyClass();
JsonSerializer.Serialize<object>(obj); interface IFoo1 : IFoo2, IFoo3 { ... }
[JsonConverter(typeof(IFoo2Impl))] interface IFoo2 { ... }
[JsonConverter(typeof(IFoo3Impl))] interface IFoo3 { ... }
class MyClass : IFoo1 { ... }
// No custom converter would get called
IFoo1 obj = new MyClass();
JsonSerializer.Serialize<IFoo1>(obj); Future polymorphism thoughts: Potential future feature (c) would cause the examples above to be ambiguous with interfaces since there can be more than one "correct" answer. They are not ambiguous, however, when using inheritance since the most derived type \ converter would be selected.
|
I don't understand what is the purpose of "known types" in your last example. Why does [KnownType(typeof(DerivedClass1))]
[KnownType(typeof(DerivedClass2))]
class BaseClass {}
class DerivedClass1 : BaseClass { }
class DerivedClass2 : BaseClass { }
[KnownType(typeof(DerivedClass1))]
[KnownType(typeof(DerivedClass2))]
class AnyClass
{
public object Value { get; set; }
}
// can be DerivedClass1 or DerivedClass2 depending on type information in json
var obj = JsonSerializer.Deserialize<BaseClass>(json);
// can contain DerivedClass1 or DerivedClass2 depending on type information in json
var obj = JsonSerializer.Deserialize<AnyClass>(json); In this case I don't understand how known types can be practically combined with custom converters at all.
This behavior is kind of wierd for me. Also it's either a breaking change or a disabled-by-default option. But even if it will be an option (like And this feature is ok for serialization, but how will it behave for deserialization? So, my opinion is if the type of a property or a root serialize-deserialize type has a converter attribute, it should be used, in other case runtime-provided or default converter should be used for that exact (declared) type (just like now, simple and working strategy). Known types is just a settings for the default converter which will support polymorphism through this known types. Anyway, now converters for interfaces are not forbidden at all, they can be attached in options or with the workaround attribute (see above). |
Yep thanks. My last example on known types was not correct. I just removed it since I don't want to presume the actual design for known types in this PR. "Known types" is likely to be used for serialization to write a "safe" type name as metadata (one that doesn't include the full namespace and class type) which is used later during deserialization to map to the appropriate concrete Type to instantiate. A potential issue I mentioned with interfaces is feature "c" where we inspect base classes and interfaces for a converter (if no converter found on the more derived type). If we decide to do that, however, it only makes sense to support base classes and not interfaces to avoid any ambiguity. By not implementing this feature for base classes means if we have a "abstract base class declared on a property" which has a converter but the "actual concrete derived class" (obtained via obj.GetType()) does not have a custom converter, then the custom converter on the base class will not be used and instead the default object converter will be used. So in summary I don't see a reason to not allow converters for interfaces at this time. |
From @jamalabo1 in #38939:
|
Since custom converters for interfaces are already supported via property-level declarations or via adding them to namespace System.Text.Json.Serialization
- [AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=false)]
+ [AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Interface | AttributeTargets.Enum | AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=false)]
public class JsonConverterAttribute : JsonAttribute
{
} Note that this should not have any bearing on how types implementing annotated interfaces are serialized, since as already alluded to in this conversation it would result in diamond ambiguity when resolving the serialization contract for a given value. This problem falls in the domain of polymorphic serialization and will be addressed independently via #29937 and #30083. |
namespace System.Text.Json.Serialization
{
// Adding AttributeTargets.Interface
[AttributeUsage(AttributeTargets.Class |
AttributeTargets.Struct |
AttributeTargets.Interface |
AttributeTargets.Enum |
AttributeTargets.Property |
AttributeTargets.Field, AllowMultiple = false)]
public class JsonConverterAttribute : JsonAttribute
{
}
} |
* Allow JsonConverterAttribute usage on interfaces. Fix #33112 * update ApiCompat baseline
JsonConverterAttribute does not contain target AttributeTargets.Interface. I did not find any design notes about this behavior.
It's strange that JsonConverterAttribute can be applied to abstract classes, but not to interfaces.
In fact, JsonConverterAttribute can be used on property of an interface type, and it will work as expected. Also, specific JsonConverter that works with an interface type can be added to JsonSerializerOptions, and will work as expected.
Similar issue about enums: #30361
EDIT: See #33112 (comment) for the API proposal.
The text was updated successfully, but these errors were encountered: