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

JsonConverter constructor causes MissingMetadataException upon instantiation #78029

Open
Tracked by #79122
Sergio0694 opened this issue Nov 8, 2022 · 19 comments
Open
Tracked by #79122
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 8, 2022

Description

We're currently working on migrating all our JSON serialization from Newtonsoft.Json to System.Text.Json in the Microsoft Store (see also #77897), and we're hitting some issues with trimming (we're on .NET Native). In particular, this line:

IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;

This is crashing when trimming is enabled, because the linker will remove support for getting the assembly info from types. We can fix this by adding some .rd.xml directives, but it's error prone and not really a great solution. Eg. we can use:

<Type Name="System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.DictionaryOfTKeyTValueConverter`3" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.ListOfTConverter`2" Activate="Required Public" />

Etc. for all converters we need. It'd be much better if this was just fixed in System.Text.Json directly. I'm aware that reflection-free mode isn't supported (see #68093), but fixing this would also benefit other scenarios (such as our case) by still allowing the linker to just trim out more metadata and reduce the binary size further.

Note: to clarify, the ask is not to support the reflection-free mode, just to make this path friendlier to trimming.

cc. @eiriktsarpalis @MichalStrehovsky

Reproduction Steps

The repro is pretty much the same as in the linked issue:

string json = """
    {
        "SomeMapping": { "A": "B" },
        "SomeList": ["A", "B"]
    }
    """;

_ = System.Text.Json.JsonSerializer.Deserialize(rawJson, MicrosoftStoreJsonSerializerContext.Default.SomeModel);

public sealed class SomeModel
{
    public Dictionary<string, string> SomeMapping { get; set; }
    public List<string> SomeList { get; set; }
}

[JsonSerializable(typeof(SomeModel))]
[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
internal sealed partial class MicrosoftStoreJsonSerializerContext : JsonSerializerContext
{
}

Expected behavior

This should just work fine.

Actual behavior

We're getting a MissingMetadataException:

at System.Reflection.Runtime.TypeInfos.RuntimeNoMetadataNamedTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeNoMetadataNamedTypeInfo.cs:line 38 
at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeConstructedGenericTypeInfo.cs:line 136
at System.Text.Json.Serialization.JsonConverter`1..ctor(Boolean initialize) 
at System.Text.Json.Serialization.JsonConverter`1..ctor()
at System.Text.Json.Serialization.JsonResumableConverter`1..ctor() 
at System.Text.Json.Serialization.JsonObjectConverter`1..ctor()
at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1..ctor()
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1.<>c.<GetConverter>b__3_1()
at System.Func`1.Invoke()
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_Converter() 
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_ElementType() 
at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonConverter converter, JsonSerializerOptions options)
at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1..ctor(JsonConverter converter, JsonSerializerOptions options) 
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1..ctor(JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo)
at System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateObjectInfo[T](JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo) 
at SomeProject.MicrosoftStoreJsonSerializerContext.Create_SomeModel(JsonSerializerOptions options)
at SomeProject.MicrosoftStoreJsonSerializerContext.get_SomeModel()  
at SomeProject.<<TryDeserializeSomeModel>g__Foo|77_0>d.MoveNext() 

Regression?

I have a possible idea on how to fix this, by making that path entirely reflection-free. Consider this:

public abstract class JsonConverter<T>
{
    protected JsonConverter()
    {
        IsInternalType = CheckIsInternalType();
    }

    private protected virtual bool CheckIsInternalType() => false;
}

Now, all converter types in System.Text.Json would just override the method accordingly:

public class SomeSealedJsonConverter<T> : JsonConverter<T>
{
    private protected override bool CheckIsInternalType() => true;
}

public class SomeUnsealedJsonConverter<T> : JsonConverter<T>r
{
    private protected override bool CheckIsInternalType() => GetType() == typeof(SomeUnsealedJsonConverter<T>);
}

This makes sure that:

  • External converters directly inheriting from JsonConverter<T> will be marked as external.
  • External converters that inherit from unsealed STJ converters will also be marked as external.

Essentially this should provide a reflection-free way of checking whether a concrete converter type is from the STJ assembly.

Configuration

  • System.Text.Json 7.0
  • .NET Native 6.2.14
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 8, 2022
@ghost
Copy link

ghost commented Nov 8, 2022

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

We're currently working on migrating all our JSON serialization from Newtonsoft.Json to System.Text.Json in the Microsoft Store (see also #77897), and we're hitting some issues with trimming (we're on .NET Native). In particular, this line:

IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;

This is crashing when trimming is enabled, because the linker will remove support for getting the assembly info from types. We can fix this by adding some .rd.xml directives, but it's error prone and not really a great solution. Eg. we can use:

<Type Name="System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.DictionaryOfTKeyTValueConverter`3" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.ListOfTConverter`2" Activate="Required Public" />

Etc. for all converters we need. It'd be much better if this was just fixed in System.Text.Json directly. I'm aware that reflection-free mode isn't supported (see #68093), but fixing this would also benefit other scenarios (such as our case) by still allowing the linker to just trim out more metadata and reduce the binary size further.

cc. @eiriktsarpalis @MichalStrehovsky

Reproduction Steps

The repro is pretty much the same as in the linked issue:

string json = """
    {
        "SomeMapping": { "A": "B" },
        "SomeList": ["A", "B"]
    }
    """;

_ = System.Text.Json.JsonSerializer.Deserialize(rawJson, MicrosoftStoreJsonSerializerContext.CamelCase.DAOfferDataContract);

public sealed class SomeModel
{
    public Dictionary<string, string> SomeMapping { get; set; }
    public List<string> SomeList { get; set; }
}

[JsonSerializable(typeof(SomeModel))]
[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
internal sealed partial class MicrosoftStoreJsonSerializerContext : JsonSerializerContext
{
}

Expected behavior

This should just work fine.

Actual behavior

We're getting a MissingMetadataException:

at System.Reflection.Runtime.TypeInfos.RuntimeNoMetadataNamedTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeNoMetadataNamedTypeInfo.cs:line 38 
at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeConstructedGenericTypeInfo.cs:line 136
at System.Text.Json.Serialization.JsonConverter`1..ctor(Boolean initialize) 
at System.Text.Json.Serialization.JsonConverter`1..ctor()
at System.Text.Json.Serialization.JsonResumableConverter`1..ctor() 
at System.Text.Json.Serialization.JsonObjectConverter`1..ctor()
at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1..ctor()
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1.<>c.<GetConverter>b__3_1()
at System.Func`1.Invoke()
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_Converter() 
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_ElementType() 
at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonConverter converter, JsonSerializerOptions options)
at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1..ctor(JsonConverter converter, JsonSerializerOptions options) 
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1..ctor(JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo)
at System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateObjectInfo[T](JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo) 
at SomeProject.MicrosoftStoreJsonSerializerContext.Create_SomeModel(JsonSerializerOptions options)
at SomeProject.MicrosoftStoreJsonSerializerContext.get_SomeModel()  
at SomeProject.<<TryDeserializeSomeModel>g__Foo|77_0>d.MoveNext() 

Regression?

I have a possible idea on how to fix this, by making that path entirely reflection-free. Consider this:

// New internal interface in System.Text.Json
internal interface IInternalConverter
{
    Type TypeRoot { get; }
}

// Updated constructor for JsonConverter<T>
public abstract class JsonConverter<T>
{
    protected JsonConverter()
    {
        IsInternalType = GetType() == (this as IInternalConverter)?.TypeRoot;
    }
}

Now, all converter types in System.Text.Json would implement the interface explicitly. like so:

// Converter in System.Text.Json
public class SomeJsonConverter<T> : JsonConverter<T>, IInternalConverter
{
    Type IInternalConverter.TypeRoot => typeof(SomeJsonConverter<T>);
}

This makes sure that:

  • External converters directly inheriting from JsonConverter<T> won't have the interface
  • External converters that inherit from unsealed STJ converters won't have a matching type at runtime

Essentially this should provide a reflection-free way of checking whether a concrete converter type is from the STJ assembly.

Known Workarounds

No response

Configuration

  • System.Text.Json 7.0
  • .NET Native 6.2.14

Other information

No response

Author: Sergio0694
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@MichalStrehovsky
Copy link
Member

For further context, in .NET Native it's possible to have a type with all of its metadata removed. So the GetType().Assembly call is likely getting a Type object for a type whose metadata was removed. Such Type objects are going to throw (because they don't know their assembly... they don't even know their names).

It's not possible to get such Type object with .NET Core trimming - either PublishAot or PublishTrimmed will always return a Type that always knows its assembly. So this is just a .NET Native issue.

But .NET Native is NetStandard 2.0 and supported so...

It can probably be worked around with:

<Assembly Name="System.Text.Json" Dynamic="All" />

Which tells the .NET Native compiler to keep the metadata for everything that was kept. It will keep more than necessary, but it should be safe for this line of code.

@Sergio0694
Copy link
Contributor Author

That works, but causes a lot of metadata to be preserved. I narrowed that down a bit and this also seems to work:

<Type Name="System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1">
    <Subtypes Activate="Required Public" />
</Type>

But even just this causes 1MB of increase on the package size, which doesn't seem worth it given the issue is just that line. Adding the individual directives like I mentioned in the OP only adds 40KB of binary size, which is great, though of course it's a bit more brittle and requires additional testing to ensure no converter type is missed.

I guess what I'm trying to say is - it feels like this issue could be fixed relatively easy by using a private protected virtual method like in my proposal, and that would benefit a lot of UWP developers (this is not the first time I see this specific issue causing problems in UWP apps, and not everyone is familiar with .rd.xml directives enough to fix this on their own).

To be clear - I'd be happy to contribute the fix myself 🙂

"It's not possible to get such Type object with .NET Core trimming"

Out of curiosity - I know the reflection-free mode isn't supported in NativeAOT, but would the same still apply to the "enhanced reflection free" mode that was in the works? Ie. the one that would remove all MethodBase support or something (can't remember all the details exactly). As in, would Type.Assembly still be available even in that case anyway?

@eiriktsarpalis
Copy link
Member

it feels like this issue could be fixed relatively easy by using a private protected virtual method like in my proposal

I don't believe that would work in the general case. Apart from the fact that each internal derived type would need to be explicitly marked as internal (allowing for the possibility of new internal types potentially missing that declaration), it doesn't account for custom converters deriving from built-in converters that are both unsealed and public. Currently there's only one such class, JsonStringEnumConverter (which is actually a factory, so the value of IsInternalConverter will be imprecise but not necessarily carry any performance impact) but we are planning on adding more public converters like that, see #73124 (comment) and #63791.

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Nov 9, 2022

"it doesn't account for custom converters deriving from built-in converters that are both unsealed and public"

I'm not sure I understand why this wouldn't work, could you elaborate? Consider this case:

// This is public and unsealed
public class InternalConverter<T> : JsonConverter<T>
{
    private protected override bool CheckIsInternalType() => GetType() == typeof(InternalConverter<T>);
}

// This is an external converter that inherits from it
public class ExternalConverter<T> : InternalConverter<T>
{
}

When either is instantiated, you can have two cases:

  • If InternalConverter<T> is instantiated (say, it's InternalConverter<string>), that CheckIsInternalType will return true, because it will result in InternalConverter<string> == typeof(InternalConverter<string>).
  • If ExternalConverter<T> is instantiated (say, it's ExternalConverter<string>), that CheckIsInternalType will return false, because the check will now result in ExternalConverter<string> == InternalConverter<string>.

Essentially, this solution accounts both cases just fine, as far as I can tell. 🙂

Note: this is also a very similar approach to what MemoryStream does to check against derived types.

"allowing for the possibility of new internal types potentially missing that declaration"

I can see that, but I think we can address that with tests, especially because we know the full set of converters. We might even be able to just come up with a way to fetch and validate all tests via reflection (eg. get all non abstract internal converters, instantiate them with random type arguments, invoke CheckIsInternalType and verify it returns true).

To clarify again, I'm happy to contribute this improvement myself, since it'd benefit us directly (and NativeAOT users) 😄

@eiriktsarpalis
Copy link
Member

Consider this case:

I assumed the virtual would simply set a flag, but this is running an equality comparison against the declared type. Would this work if reflection metadata has been stripped?

@Sergio0694
Copy link
Contributor Author

Yup this will work just fine, even in reflection-free mode you can directly compare Type instances for checking equality (which is exactly what we need), that is explicitly allowed. As in, that would work both on .NET Native (where the entire metadata is stripped) and in NativeAOT reflection-free mode (where the entire reflection stack is gone) 🙂

If performance is a concern, such a type comparison is actually just a direct comparison, so it's also faster than the current code (as another side bonus on top of not using reflection). See sharplab example, where the whole check just compiles to:

JsonSerializer`1[[System.Int32, System.Private.CoreLib]].M()
    L0000: mov rax, 0x7ffb7e5ad460
    L000a: cmp [rcx], rax
    L000d: sete al
    L0010: movzx eax, al
    L0013: ret

@eiriktsarpalis
Copy link
Member

Performance shouldn't be a concern, the virtual would be invoked only once by the constructor anyways. What does concern me is maintainability and risk of regression -- clearly our testing pipelines don't cover that particular scenario.

@MichalStrehovsky
Copy link
Member

But even just this causes 1MB of increase on the package size, which doesn't seem worth it given the issue is just that line

Try dropping the Required part and leave it at <Subtypes Activate="Public" />. Required roots things that would otherwise be unused.

@Sergio0694
Copy link
Contributor Author

That doesn't seem to be enough, I tried but unfortunately I still got MissingMetadataException-s with that 🥲

@MichalStrehovsky
Copy link
Member

Out of curiosity - I know the reflection-free mode isn't supported in NativeAOT, but would the same still apply to the "enhanced reflection free" mode that was in the works?

The compiler doesn't generate such data structures and I deleted the representation for this from the reflection stack in #73612.

@Sergio0694
Copy link
Contributor Author

"What does concern me is maintainability and risk of regression -- clearly our testing pipelines don't cover that particular scenario."

@eiriktsarpalis if we want to make sure to not miss any types, could we add a test that does the following:

  • Go through all public types in System.Text.Json's assembly
  • Filter those that are deriving from JsonConverter<T>
  • If they're generic, call MakeGenericType on them with some random type (say, object)
  • Get a default instance via RuntimeHelpers.GetUninitializedObject
  • Invoke CheckIsInternalType on them via reflection
  • Verify that it returns true

And we can then also define a couple of converter types in the same test project and check those return false.

Seems like that would give us a reliable way to automatically test all existing converters to ensure this works?
What do you think? 🙂

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 10, 2022

I mean we'll do that if we absolutely have to, but hoping for a cleaner solution or a potential workaround in other platforms that do need this.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Nov 10, 2022
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Nov 10, 2022
@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework labels Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

We're currently working on migrating all our JSON serialization from Newtonsoft.Json to System.Text.Json in the Microsoft Store (see also #77897), and we're hitting some issues with trimming (we're on .NET Native). In particular, this line:

IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;

This is crashing when trimming is enabled, because the linker will remove support for getting the assembly info from types. We can fix this by adding some .rd.xml directives, but it's error prone and not really a great solution. Eg. we can use:

<Type Name="System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.DictionaryOfTKeyTValueConverter`3" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.ListOfTConverter`2" Activate="Required Public" />

Etc. for all converters we need. It'd be much better if this was just fixed in System.Text.Json directly. I'm aware that reflection-free mode isn't supported (see #68093), but fixing this would also benefit other scenarios (such as our case) by still allowing the linker to just trim out more metadata and reduce the binary size further.

Note: to clarify, the ask is not to support the reflection-free mode, just to make this path friendlier to trimming.

cc. @eiriktsarpalis @MichalStrehovsky

Reproduction Steps

The repro is pretty much the same as in the linked issue:

string json = """
    {
        "SomeMapping": { "A": "B" },
        "SomeList": ["A", "B"]
    }
    """;

_ = System.Text.Json.JsonSerializer.Deserialize(rawJson, MicrosoftStoreJsonSerializerContext.CamelCase.DAOfferDataContract);

public sealed class SomeModel
{
    public Dictionary<string, string> SomeMapping { get; set; }
    public List<string> SomeList { get; set; }
}

[JsonSerializable(typeof(SomeModel))]
[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
internal sealed partial class MicrosoftStoreJsonSerializerContext : JsonSerializerContext
{
}

Expected behavior

This should just work fine.

Actual behavior

We're getting a MissingMetadataException:

at System.Reflection.Runtime.TypeInfos.RuntimeNoMetadataNamedTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeNoMetadataNamedTypeInfo.cs:line 38 
at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.get_Assembly() in f:\\dd\\ndp\\fxcore\\CoreRT\\src\\System.Private.Reflection.Core\\src\\System\\Reflection\\Runtime\\TypeInfos\\RuntimeConstructedGenericTypeInfo.cs:line 136
at System.Text.Json.Serialization.JsonConverter`1..ctor(Boolean initialize) 
at System.Text.Json.Serialization.JsonConverter`1..ctor()
at System.Text.Json.Serialization.JsonResumableConverter`1..ctor() 
at System.Text.Json.Serialization.JsonObjectConverter`1..ctor()
at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1..ctor()
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1.<>c.<GetConverter>b__3_1()
at System.Func`1.Invoke()
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_Converter() 
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.get_ElementType() 
at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonConverter converter, JsonSerializerOptions options)
at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1..ctor(JsonConverter converter, JsonSerializerOptions options) 
at System.Text.Json.Serialization.Metadata.SourceGenJsonTypeInfo`1..ctor(JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo)
at System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateObjectInfo[T](JsonSerializerOptions options, JsonObjectInfoValues`1 objectInfo) 
at SomeProject.MicrosoftStoreJsonSerializerContext.Create_SomeModel(JsonSerializerOptions options)
at SomeProject.MicrosoftStoreJsonSerializerContext.get_SomeModel()  
at SomeProject.<<TryDeserializeSomeModel>g__Foo|77_0>d.MoveNext() 

Regression?

I have a possible idea on how to fix this, by making that path entirely reflection-free. Consider this:

public abstract class JsonConverter<T>
{
    protected JsonConverter()
    {
        IsInternalType = CheckIsInternalType();
    }

    private protected virtual bool CheckIsInternalType() => false;
}

Now, all converter types in System.Text.Json would just override the method accordingly:

public class SomeSealedJsonConverter<T> : JsonConverter<T>
{
    private protected override bool CheckIsInternalType() => true;
}

public class SomeUnsealedJsonConverter<T> : JsonConverter<T>r
{
    private protected override bool CheckIsInternalType() => GetType() == typeof(SomeUnsealedJsonConverter<T>);
}

This makes sure that:

  • External converters directly inheriting from JsonConverter<T> will be marked as external.
  • External converters that inherit from unsealed STJ converters will also be marked as external.

Essentially this should provide a reflection-free way of checking whether a concrete converter type is from the STJ assembly.

Configuration

  • System.Text.Json 7.0
  • .NET Native 6.2.14
Author: Sergio0694
Assignees: -
Labels:

enhancement, area-System.Text.Json, linkable-framework

Milestone: 8.0.0

@Sergio0694
Copy link
Contributor Author

The workarounds we have are either to preserve all converter types, which adds too much binary size as it preserves any converter type anywhere even if unused, or manually annotating the ones you need, which only adds a bit more metadata than needed but is acceptable on this front. The downside to this though is that it's a rather brittle solution, as it's fairly easy to miss one especially if a given converter is only used on some specific JSON models. I'd feel much more comfortable if this was just fixed at the source, and it'd allow us to proceed with more confidence to switch over the whole Store to System.Text.Json 😅

Also if it's any help (I know the team is busy), I really wouldn't mind contributing this myself 🙂

@eiriktsarpalis
Copy link
Member

Another possible solution is to investigate current usage of the IsInternalConverter flag and possibly replace it with something that doesn't require reflection.

@Sergio0694
Copy link
Contributor Author

That sounds like an even better idea 😄
Just glancing at the code it's not immediately obvious to me what this property is even for (there's also no comments).

@Sergio0694
Copy link
Contributor Author

Great to see this being added to the new AOT user story for .NET 8! 🎉

@eiriktsarpalis should we add the partner-impact tag here too, since we're hitting this in the Store?
This specific reflection dependency being addressed in the next release would be a pretty nice win for us there 😄

@eiriktsarpalis eiriktsarpalis added the partner-impact This issue impacts a partner who needs to be kept updated label Dec 5, 2022
@dongle-the-gadget
Copy link

One thing I also want to mention, System.Text.Json apparently treats List<A<B>> as requiring converter JsonConverter<List<A<B>>>, which has its own reflection code:


That means Sergio's previously mentioned workaround to add possibly every converter System.Text.Json implements needs to be changed to

<Type Name="System.Text.Json.Serialization.JsonConverter`1">
    <Subtypes Activate="Required Public" />
</Type>

for this scenario to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

No branches or pull requests

4 participants