Annotating a type with a JSON converter blocks the JSON source generator crawling

[Sergio0694]

Description

While continuing the migration of the Microsoft Store to System.Text.Json (we're getting there! 😄), I'm now migrating some unstructured JSON data we have (which currently deserializes to a dictionary and then uses a whole bunch of manual lookups, which is pretty bad), to instead using proper strongly typed models with System.Text.Json. Due to how some of these models are crafted, we end up needing a few custom converters to make the whole thing work correctly. This is fine, but I've noticed some weird interplay between custom converters and the JSON source generators that I'm not sure is by design, so I figured I'd ask 🙂

Reproduction Steps

Consider this:

[JsonSerializable(typeof(A))]
public partial class MyContext : JsonSerializerContext
{
}

[JsonConverter(typeof(AConverter))]
public class A
{
    public B B { get; set; }
}

public class B
{
}

public class AConverter : JsonConverter<A>
{
    public override A? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }

    public override void Write(Utf8JsonWriter writer, A value, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }
}

In this case, AConverter will deserialize A instances, with some custom logic. As part of this, it will also create a B instance by deserializing the corresponding property in the JSON. That is, after moving the reader to the start of the object token, it'll just call JsonSerializer.DeserializeObject<B>(ref reader, options). We use this pattern extensively in our JSON contracts.

Expected behavior

I wouldn't expend the annotation to cause the source generator to stop traversing the type hierarchy.

Actual behavior

If you annotate A with [JsonSerializable(typeof(A))], the JSON source generator will stop crawling properties, and as a result the metadata for B and derived types will not be generated. Thus, trying to deserialize A will throw at runtime.

Regression?

Don't think so.

Known Workarounds

A workaround is to manually also specify [JsonSerializable(typeof(B))] on the JSON context. This works, but is pretty error prone, as it requires consumers to not just annotate the types they want to deserialize on their end, but also all types that are skipped by the generator due to the internal implementation detail of how the JSON models are defined.

I can understand if this is by design to potentially save unnecessary metadata (say, if the custom converter never created that instance through a JSON serialization API, but rather manually through other means), but the resulting behavior is not ideal for consumers. Would it be possible for the contract author to somehow inform the generator that if someone marks the root type as being serialized, then the metadata for also X property (or properties) should be generated?

This would allow both to keep the current behavior and save metadata, while at the same time allowing authors of these contracts to make the behavior transparent and avoid nasty surprises on the consumer side.

For instance: it would be nice if you could add [JsonSerializable(typeof(B))] over B in this case so that the source generator could see that "ok this type has a custom serializer so I'll stop traversing the hierarchy but I'll keep gathering metadata for these additional types that you have specified, sure". Would that make sense? 🙂

cc. @eiriktsarpalis

[ghost]

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

Issue Details

---

Description

While continuing the migration of the Microsoft Store to System.Text.Json (we're getting there! 😄), I'm now migrating some unstructured JSON data we have (which currently deserializes to a dictionary and then uses a whole bunch of manual lookups, which is pretty bad), to instead using proper strongly typed models with System.Text.Json. Due to how some of these models are crafted, we end up needing a few custom converters to make the whole thing work correctly. This is fine, but I've noticed some weird interplay between custom converters and the JSON source generators that I'm not sure is by design, so I figured I'd ask 🙂

Reproduction Steps

Consider this:

[JsonSerializable(typeof(A))]
public partial class MyContext : JsonSerializerContext
{
}

[JsonConverter(typeof(AConverter))]
public class A
{
    public B B { get; set; }
}

public class B
{
}

public class AConverter : JsonConverter<A>
{
    public override A? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }

    public override void Write(Utf8JsonWriter writer, A value, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }
}

In this case, AConverter will deserialize A instances, with some custom logic. As part of this, it will also create a B instance by deserializing the corresponding property in the JSON. That is, after moving the reader to the start of the object token, it'll just call JsonSerializer.DeserializeObject<B>(ref reader, options). We use this pattern extensively in our JSON contracts.

Expected behavior

I wouldn't expend the annotation to cause the source generator to stop traversing the type hierarchy.

Actual behavior

If you annotate A with [JsonSerializable(typeof(A))], the JSON source generator will stop crawling properties, and as a result the metadata for B and derived types will not be generated. Thus, trying to deserialize A will throw at runtime.

Regression?

Don't think so.

Known Workarounds

A workaround is to manually also specify [JsonSerializable(typeof(B))] on the JSON context. This works, but is pretty error prone, as it requires consumers to not just annotate the types they want to deserialize on their end, but also all types that are skipped by the generator due to the internal implementation detail of how the JSON models are defined.

I can understand if this is by design to potentially save unnecessary metadata (say, if the custom converter never created that instance through a JSON serialization API, but rather manually through other means), but the resulting behavior is not ideal for consumers. Would it be possible for the contract author to somehow inform the generator that if someone marks the root type as being serialized, then the metadata for also X property (or properties) should be generated?

This would allow both to keep the current behavior and save metadata, while at the same time allowing authors of these contracts to make the behavior transparent and avoid nasty surprises on the consumer side.

For instance: it would be nice if you could add [JsonSerializable(typeof(B))] over B in this case so that the source generator could see that "ok this type has a custom serializer so I'll stop traversing the hierarchy but I'll keep gathering metadata for these additional types that you have specified, sure". Would that make sense? 🙂

cc. @eiriktsarpalis

Configuration

No response

Other information

No response

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

[eiriktsarpalis]

You're right to point out that this is absolutely by-design and intended as a way to avoid unnecessary metadata generation. The source generator isn't able to tell what dependencies a custom converter may have so you're pretty much on your own when introducing them. Out of curiosity, what type of customization does your converter introduce? If it's just serializing properties as usual, could the same result be achieved using contract customization?

As a side note, we might want to relax that behaviour once #63791 is implemented, as types specifying, say, [JsonConverter(typeof(JsonObjectConverter))] would need to have property metadata generated at compile time.

[Sergio0694]

Ah, yeah that makes sense, I think the current behavior is the best default 🙂

"Out of curiosity, what type of customization does your converter introduce?"

We currently have several custom converters, handling scenarios such as:

• "Payload converter": this is used over an object property and internally has a mapping of type discriminators to strongly typed data models, and it simply parses the type discriminator and forwards to the correct serialization method. We can't just use the built-in type discriminator support here as there isn't a single type hierarchy here, but we have like 100+ different types to support that can be unrelated from each other.
• "Newtonsoft compatible bool converter": deserializes a bool like with Newtonsoft (reads from both boolean tokens, string-s tokens with case invariant comparisons, or null (and returns false).
• "Resilient enum converter": deserializes an enum as numeric value, string value with case invariant comparison, and returns default if the value isn't recognized (instead of throwing). This enables adding new enum values without breaking older clients (the built-in enum converter will instead throw if it finds a value that's not recognized).
• "Newtonsoft dictionary converter": deserializes a dictionary and skips the type token (also see [API Proposal]: ignore type discriminator ("$type" properties) in JSON model properties #82012).
• "Newtonsoft dictionary converter": deserializes a list and skips the type token.
• We also have a bunch of custom converters that deserialize list/dictionaries/objects from string values with custom formatting, often in ways that might not strictly speaking be standard JSON. For instance:
  • Deserializing a string with a comma separated list of values, where every value might have an additional part after a given separator character, into a Dictionary<string, object?>.
  • Deserializing a CSV string into a List<string>
  • Deserializing a string containing another JSON serialized object, directly into that JSON model object.

"could the same result be achieved using contract customization?"

Not sure, I haven't really ever used that feature in System.Text.Json so far. Might need to look into it 😄

"we might want to relax that behaviour"

Would that mean letting the generator crawl everything, or would that have some opt-in mechanism like suggested here?

[eiriktsarpalis]

"we might want to relax that behaviour"

Would that mean letting the generator crawl everything, or would that have some opt-in mechanism like suggested here?

In that case, the specific JsonConverter annotation would trigger opt in to a specific mode of metadata generation

[eiriktsarpalis]

One possible workaround might be to specify the custom converter at run time rather than design time via the JsonSerializerOptions.Converters property.