JsonConverter precedence differs from Newtonsoft and may be unintuitive

[ylibrach]

Newtonsoft's precedence for converters is as follows (as defined by their docs for converters):

• JsonConverter defined by attribute on a member
• JsonConverter defined by an attribute on a class
• any converters passed to the JsonSerializer

This seems to align pretty well with what I imagine is expected by most developers. However, the precedence used by System.Text.Json is different (as defined by the official docs for converters):

• [JsonConverter] applied to a property.
• A converter added to the Converters collection.
• [JsonConverter] applied to a custom value type or POCO.

This was very unexpected and we found it to be unintuitive. I'm curious as to why an attribute defined on a specific class type would have less priority than a converter added to the global Converters collection? It seems to go against the general concept of specificity that developers are familiar with.

In addition, this introduces a strange interaction between a JsonConverter/JsonConverterFactory and a JsonConverterAttribute, because it forces the converters to check for a JsonConverterAttribute on the type they are de/serializing, which shouldn't be their responsibility:

public class SomeConverter<T> : JsonConverter<T> {
    public SomeConverter(bool someOption) { ... }
}

public class SomeConverter : JsonConverterFactory {
    public override JsonConverter CreateConverter(Type type, JsonSerializerOptions options) {
        /* In order to be able to de/serialize `type` using `SomeConverterAttribute`, 
           we're forced to have this factory check if the attribute is set on `type`, 
           before creating `SomeConverter<T>`. */
    }
}

public class SomeConverterAttribute : JsonConverterAttribute {
    public bool SomeOption { get; set; }

    public override JsonConverter CreateConverter() {
        /* Create a `SomeConverter<T>` type, and call it with `SomeOption` */
    }
}

[SomeConverter(someOption = true)]
public class SomeClass { ... }

...

serializerOptions.Converters.Add(new SomeConverter());

Even more, it forces a JsonConverterAttribute and a JsonConverterFactory to have two different CreateConverter methods, instead of allowing the attribute to call a factory's CreateConverter. This is because the factory would need to call the attribute's CreateConverter if it exists on the type, thereby causing a circular call chain if the attribute's CreateConverter were to call back to the factory.

[Clockwork-Muse]

I'm guessing this was to enable configuring output-specific converter overrides, and its placement was to strike a balance between usability for fields and types.

Although it does look a little strange to me. Perhaps what we should have instead is a way to override the converter for a type/field based on path?

[ylibrach]

I'm guessing this was to enable configuring output-specific converter overrides, and its placement was to strike a balance between usability for fields and types.

Can you elaborate a bit on this? I'm wondering if there is a legitimate use-case for that, and if there is, if it isn't better served through a different mechanism?

As for overriding a converter based on path, can you show an example? I'm not sure if I'm picturing what you mean correctly.

[Clockwork-Muse]

No, you might be right, because most of the instances I'm thinking of would either have converters on the fields (where it couldn't override it), or just have it configured on the output itself.

For overriding based on path, you'd have something like serializerOptions.Converters.Add(new SomeConverter(), "$.rootElement.someContainer.someProperty"); (where the property path is in the JSON document, not necessarily the actual object property).

[ahsonkhan]

However, the precedence used by System.Text.Json is different

I believe the intention behind this order of precedence is to make run-time changes override design-time choices.

@steveharter - can you expand on the rational for the precedence order and is this something we should consider changing?

[ylibrach]

For overriding based on path, you'd have something like serializerOptions.Converters.Add(new SomeConverter(), "$.rootElement.someContainer.someProperty"); (where the property path is in the JSON document, not necessarily the actual object property).

Hmmm, wouldn't this defeat the purpose of what a JsonConverter attribute is setting out to do?

I believe the intention behind this order of precedence is to make run-time changes override design-time choices.

Sorry, do you mean that this is Newtonsoft's intention, or System.Text.Json's intention? From what I can see, Newtonsoft's precedence follows this intention, but System.Text.Json's doesn't.

[Clockwork-Muse]

Hmmm, wouldn't this defeat the purpose of what a JsonConverter attribute is setting out to do?

JsonConverter attributes are design time settings, whereas putting an override on serializer options is a runtime setting. You may not be in control of the design-time setting (if it's part of some third-party library, usually), so defeating the attribute would actually be the point.

Sorry, do you mean that this is Newtonsoft's intention, or System.Text.Json's intention? From what I can see, Newtonsoft's precedence follows this intention, but System.Text.Json's doesn't.

I think you're looking at the listing backwards - it's in order of precedence. That is, Newtonsoft's serializer-registered converters override nothing, but instead are providing "default" or fallback converters.

[ylibrach]

@Clockwork-Muse ok I think I am finally understanding what you mean by "runtime" overrides (I put it in quotes because in actuality the scenario you're describing - overriding a third-party library - is still going to be specified at design time, unless you think that the string path is going to be set by some runtime configuration provider).

That said:

• First, is this even a valid scenario that should be supported? I've never encountered a need to do anything like this. Was this a known limitation of Newtonsoft's approach?
• Even if it's a necessary scenario to support, it seems that the current implementation only supports at design-time.
• Again, if it's a necessary scenario, I would argue that it's much more of an edge-case scenario than attribute-based converters. I would say that attributes should be given precedence (ala Newtonsoft), but that some additional override capability is added (perhaps through the path-based approached that you mentioned, @Clockwork-Muse)

[Clockwork-Muse]

Perhaps I more should have said "consume-time", instead of "run-time", because it's more about letting consumers override some of the behavior.
Note that System.Text.Json still gives precedence to field attributes, it's just class attributes that are overridden. Presumably you'd provide a custom converter to whatever class has fields that you want to override to change their behavior, but that feels heavier than I'd want to deal with (hence the "please override this field" syntax).

[steveharter]

Runtime has precedence over design-time since design-time normally comes first, and there is an intent to change that behavior with run-time settings.

Here's some design notes from https://github.com/dotnet/corefx/issues/36639:

Priority of converter registration

Converters can be registered at design-time vs run-time and at a property-level vs. class-level. The basic rules for priority over which is selected:

• Run-time has precedence over design-time
• Property-level has precedence over class-level.
• User-specified has precedence over built-in.

Thus the priority from highest to lowest:

• runtime+property: Future feature: options.Converters.Add(converter, property)
• designtime+property: [JsonConverter] applied to a property.
• runtime+class: options.Converters.Add(converter)
• designtime+class: [JsonConverter] applied to a custom data type or POCO.
• Built-in converters (primitive types, JsonElement, arrays, etc).

[steveharter]

If you have a custom converter factory that covers multiple types (and not just closing an open generic, which is really the primary intent) then yes you may want to check for the presence of the custom converter attribute. Normally I would not recommend that type of converter except for a low-level, work-around-missing-feature case.

[ylibrach]

Thanks @steveharter. Maybe I should give our exact scenario as an example to showcase why I think this priority order may not be working as intended.

We are using F#, and along with that we use Discriminated Unions. To support that, we have a custom JsonUnionConverter<T> and JsonUnionConverter (factory), which is registered through options.Converters.Add(new JsonUnionConverter()).

So if we have these types:

type SomeUnion = 
| CaseA of string
| CaseB of string

type SomeType = {
   Union: SomeUnion
}

Then JsonUnionConverter will serialize SomeType like so:

{
   "union": {
      "kind": "CaseA",
      "value": "Some value"
   }
}

However, we need to provide the option for some union types to use different naming for the "kind" and "value" fields when de/serializing. To support this, we have a JsonUnionConverterAttribute (inherits JsonConverterAttribute):

[<JsonUnionConverter(KindPropertyName = "Mode", ValuePropertyName = "Items")>]
type ModedFilter<'T> =
| ModedInclusion of 'T
| ModedExclusion of 'T

However, this attribute will never be called by System.Text.Json because options.Converters already has a global converter registered which will return true for the CanConvert() call. Other than manually checking for the existence of this attribute within the converter's logic, how else can we get this to work?

This is why I believe that despite the explanation in the design meeting, this approach may not be working as cleanly as intended.

[steveharter]

Linking https://github.com/dotnet/corefx/issues/38348 for F#; we should ensure the scenario above can be addressed with custom converters.

There is also some community support for F# - see https://github.com/Tarmil/FSharp.SystemTextJson for an example.

[ylibrach]

While my specific example dealt with an F#-specific scenario, this issue is not F#-specific. Also, adding built-in support for F# types into System.Text.Json will not alleviate the root cause of this problem, either. This F# scenario simply highlights the underlying problem, but it isn't the cause of it.

[steveharter]

However, this attribute will never be called by System.Text.Json because options.Converters already has a global converter registered which will return true for the CanConvert() call.

@ylibrach so the run-time converter JsonUnionConverter created through JsonUnionConverter is the "default" implementation but you need to provide an "override" (and extra metadata) for certain discriminated unions and would like to use an attribute to do that?

One option is to always use the attribute, and not register the factory. It may make it more difficult to use since I believe you'd have to use the attribute on every discriminated union.

You may already be doing this, but to pass extra metadata to a converter through your own JsonConverter-attribute, override the attribute's CreateConverter() method and pass any state declared on the attribute to the converter's constructor.

Currently I assume you are having the factory check for the presence of your attribute on the type, and if so, return "false" for CanConvert() so that the attribute works and creates the correct converter. I also assume this is working fine.

Note that all the converters are cached for a particular Type and all of its properties, so the extra work to look for the attribute can be amortized over several (de)serializations of a given Type.

[steveharter]

FWIW another way to think about the original design:

• Every type's converter should be allowed to be overridden (even built-in types like System.String).
• It is not possible to place attributes on types you don't own (like System.String). Think of System.String of having a built-in [JsonConverter] - that's not how it is implemented, but that's the runtime semantics of built-in converters.

Thus design-time is the "fallback" and run-time is the "override".

[layomia]

Linking #29812

Linking https://github.com/dotnet/corefx/issues/38348 for F#; we should ensure the scenario above can be addressed with custom converters.

There is also some community support for F# - see https://github.com/Tarmil/FSharp.SystemTextJson for an example.

[eiriktsarpalis]

Closing as by-design behavior.