Add a JsonConverter.Type property.

[bobbyangers]

EDIT see #63898 (comment) for an API proposal

Original Proposal

Description

internal sealed override Type TypeToConvert => typeof(T);

Error CS0122 'JsonConverter.TypeToConvert' is inaccessible due to its protection level CustomJsonStringEnumConverter.cs

Reproduction Steps

public class MyConverter : JsonConverter<T>

     private void SomeLogic()
     {
            this.TypeToCovert ///Do Something
     }

Expected behavior

This property should be accessible to derived classes

protected internal sealed override Type TypeToConvert => typeof(T);

Actual behavior

Error CS0122 'JsonConverter.TypeToConvert' is inaccessible due to its protection level CustomJsonStringEnumConverter.cs

Regression?

No response

Known Workarounds

Define my own copy of the method in the derived class.

Configuration

No response

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

internal sealed override Type TypeToConvert => typeof(T);

Error CS0122 'JsonConverter.TypeToConvert' is inaccessible due to its protection level CustomJsonStringEnumConverter.cs

Reproduction Steps

public class MyConverter : JsonConverter<T>

     private void SomeLogic()
     {
            this.TypeToCovert ///Do Something
     }

Expected behavior

This property should be accessible to derived classes

protected internal sealed override Type TypeToConvert => typeof(T);

Actual behavior

Error CS0122 'JsonConverter.TypeToConvert' is inaccessible due to its protection level CustomJsonStringEnumConverter.cs

Regression?

No response

Known Workarounds

Define my own copy of the method in the derived class.

Configuration

No response

Other information

No response

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

[huoyaoyuan]

Why do you need to access the method?

[bobbyangers]

when derivind a class, some logic is based on that information.

Just need to read the prop.

[huoyaoyuan]

In derived class, you should have enough access to the type argument provided. Accessing typeof(T) shouldn't be wrong.

[bobbyangers]

Yes you are right, but duplicated logic can and should be avoided.
If the property is made portected internal (on the get), then all is good.

[huoyaoyuan]

I don't think the property is really "logic". It should be an architectural helper since the non-generic base class has no access with type argument.

[bobbyangers]

Hi @huoyaoyuan.
I hear and get what you say. If you allow me to rephrase, duplicate code and/or logic should be avoided.

Currently if a user wants to derive a class from JsonConvert<T> and want's to access the typeof(T) he can do it by simply copying that property into his class. But that is considered copied/duplicate code.

My suggestion what to simply expose that property so that derived classes would have access to it. and avoid the developers from having to having to copy the code (which is what I had to do). If it's useful in the base class.... why could it not be useful in the derived class?

Of course there is a workaround, but I just thought I would point it out.

This library is slowly making it's way into mainstream, and maybe JsonCovert<T> is not being used a lot at the moment.

I use Netwonsoft.Json and migrating code to Json.Text.Json; and this was one of the thing I encountered trying to migrate code;

But again there is an simple workaround, in principle, duplicating code and/or logic should be avoided, that is all.

[huoyaoyuan]

why could it not be useful in the derived class?

Because it is sealed override, means it always equals to typeof(T). You should not copy this property since you can always get the actual body. Just use typeof(T).

I use Netwonsoft.Json and migrating code to Json.Text.Json; and this was one of the thing I encountered trying to migrate code;

You'd better provide a sample to demonstrate how you would use the property if accessible.

[eiriktsarpalis]

I think it's a useful addition, for example somebody might want to index untyped JsonConverter instances by type. @bobbyangers would it be possible to modify your original post to follow the API proposal template?

[ghost]

This issue has been marked needs-author-action since it may be missing important information. Please refer to our contribution guidelines for tips on how to report issues effectively.

[bobbyangers]

Hi @huoyaoyuan,

Because it is sealed override, means it always equals to typeof(T). You should not copy this property since you can always get the actual body. Just use typeof(T).

That is exactly my point....

right now it's an

internal sealed override Type TypeToConvert => typeof(T);

all I am asking is that it becomes protected internal so that it becomes readable from derived classes:

protected internal sealed override Type TypeToConvert => typeof(T);

cc: @eiriktsarpalis

[huoyaoyuan]

for example somebody might want to index untyped JsonConverter instances by type.

The it's required to be public. Being readable from inherited type provide totally no sense since it can always get it in a more convenient way.

[bobbyangers]

Hi @huoyaoyuan,

The property is set as internal, why ? it's because it's being used by derived classes in that module. Or else it's would be private

When deriving custom classes from JsonConvert<T> , that are outisde the scope of internal; same use case.

You seem to think otherwise, let's leave it at that.

[eiriktsarpalis]

for example somebody might want to index untyped JsonConverter instances by type.

The it's required to be public. Being readable from inherited type provide totally no sense since it can always get it in a more convenient way.

Yes I think it should be public. I don't think there is much benefit from marking it protected since you almost always inherit from the typed JsonConverter<T>. One possible issue however is how such a property could interfere with JsonConverterFactory types.

[eiriktsarpalis]

Upon further consideration, I don't think we should expose JsonConverter.TypeToConverter, precisely because it would have undefined behavior in JsonConverterFactory instances. On the other hand, it is not needed for JsonConverter<T> since it's just typeof(T).

[rangers-globecar]

That is a strange conclusion; since as you say.. it’s just a typeof, furthermore, the developer eho whishes to create a derivative should be responsible enough,as with any other case, test and prevent the “possible” undefined behaviors.

But I guess that is your call!

[eiriktsarpalis]

@robert-stratecglobal what should be the value of TypeToConvert for something like JsonStringEnumConverter?

[eiriktsarpalis]

I think we could contemplate a design where the property is nullable. This should cover the case of JsonConverterFactory:

public abstract class JsonConverter
{
     internal JsonConverter() { }
+    public abstract Type? Type { get; }
}

public abstract class JsonConverter<T> : JsonConverter
{
+    public sealed override Type Type => typeof(T);
}

public abstract class JsonConverterFactory : JsonConverter
{
+    public sealed override Type? Type => null;
}

[bartonjs]

Video

Looks good as proposed

public abstract class JsonConverter
{
     internal JsonConverter() { }
+    public abstract Type? Type { get; }
}

public abstract class JsonConverter<T> : JsonConverter
{
+    public sealed override Type Type => typeof(T);
}

public abstract class JsonConverterFactory : JsonConverter
{
+    public sealed override Type? Type => null;
}

[eerhardt]

I missed the API review, but I had this question:

public abstract class JsonConverterFactory : JsonConverter
{
+    public sealed override Type? Type => null;
}

Because this is sealed override, derived classes can't override it. If I have a class derived from JsonConverterFactory, is there no scenario where I'd want to return a Type from it? For example, the new JsonStringEnumConverter<TEnum>, wouldn't I want to return typeof(TEnum) in it?

[eiriktsarpalis]

The property is meant as a proxy for the T in JsonConverter<T>, which is the only converter type that can be used when serializing values. Even though JsonStringEnumConverter<TEnum> technically creates converters for a single type, Type => typeof(TEnum) should only be returned by the converter that it produces, not itself. JsonConverterFactories can indicate what types they support using the CanConvert method.