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

Remove implicit fallback to reflection-based serialization in System.Text.Json sourcegen #71714

Closed
eiriktsarpalis opened this issue Jul 6, 2022 · 1 comment · Fixed by #71746 or #72228
Assignees
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 6, 2022

Consider the following source gen example in .NET 6:

JsonSerializer.Serialize(new Poco2(), typeof(Poco2), MyContext.Default);

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

public class Poco1 { }
public class Poco2 { }

Since MyContext does not include Poco2 in its serializable types, the above will fail with the following exception:

System.InvalidOperationException:

'Metadata for type 'Poco2' was not provided to the serializer. The serializer method used does not 
support reflection-based creation of serialization-related type metadata. If using source generation, 
ensure that all root types passed to the serializer have been indicated with 'JsonSerializableAttribute', 
along with any types that might be serialized polymorphically.

Note however that if we try to serialize the same type using the JsonSerializerOptions instance constructed by the source generator:

JsonSerializer.Serialize(new Poco2(), MyContext.Default.Options);

The options instance will silently incorporate the default reflection-based contract resolver as a fallback mechanism, and as such the above will serialize successfully -- using reflection.

We believe that this behavior violates the principle of least surprise and ultimately defeats the purpose of source generation. With the release of #63686 users will have the ability to fine tune the sources of their contract metadata -- as such silently introducing alternative sources becomes even less desirable.

This issue proposes that we remove the fallback-to-reflection behavior. Namely the call

JsonSerializer.Serialize(new Poco2(), MyContext.Default.Options);

Should fail with the same exception as using the JsonSerializerContext overload.

The same fallback logic applies to JsonSerializerOptions.GetConverter for options instances attached to a JsonSerializerContext. The following statement

JsonConverter converter = MyContext.Default.Options.GetConverter(typeof(Poco2));

will return a converter using the built-in reflection converter. In .NET 7 this will start failing with NotSupportedException.

We acknowledge that certain users might depend on the current behavior, either intentionally or unintentionally. As such, we propose the following workaround using the APIs released in #63686:

var options = new JsonSerializerOptions
{
     TypeInfoResolver = JsonTypeInfoResolver.Combine(MyContext.Default, new DefaultJsonTypeInfoResolver());
}

JsonSerializer.Serialize(new Poco2(), options); // contract resolution falls back to the default reflection-based resolver.
options.GetConverter(typeof(Poco2)); // returns the reflection-based converter.

cc @krwq @layomia @jeffhandley @ericstj

@eiriktsarpalis eiriktsarpalis added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 6, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Jul 6, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@ghost
Copy link

ghost commented Jul 6, 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

Consider the following source gen example in .NET 6:

JsonSerializer.Serialize(new Poco2(), typeof(Poco2), MyContext.Default);

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

public class Poco1 { }
public class Poco2 { }

Since MyContext does not include Poco2 in its serializable types, the above will fail with the following exception:

System.InvalidOperationException:
'Metadata for type 'Poco2' was not provided to the serializer. The serializer method used does not 
support reflection-based creation of serialization-related type metadata. If using source generation, 
ensure that all root types passed to the serializer have been indicated with 'JsonSerializableAttribute', 
along with any types that might be serialized polymorphically.

Note however that if we try to serialize the same type using the JsonSerializerOptions instance constructed by the source generator:

JsonSerializer.Serialize(new Poco2(), MyContext.Default.Options);

The options instance will silently incorporate the default reflection-based contract resovler as a fallback mechanism, and as such the above will serialize successfully -- using reflection.

We believe that this behavior violates the principle of least surprise and ultimately defeats the purpose of source generation. With the release of #63686 users will have the ability to fine tune the sources of their contract metadata -- as such silently introducing alternative sources becomes even less desirable.

This issue proposes that we remove the fallback-to-reflection behavior. Namely the call

JsonSerializer.Serialize(new Poco2(), MyContext.Default.Options);

Should fail with the same exception as using the JsonSerializerContext overload.

We acknowledge that certain users might depend on the current behavior, either intentionally or unintentionally. As such, we propose the following workaround using the APIs released in #63686:

var options = new JsonSerializerOptions
{
     TypeInfoResolver = JsonTypeInfoResolver.Combine(MyContext.Default, new DefaultJsonTypeInfoResolver());
}

JsonSerializer.Serialize(new Poco2(), options); // contract resolution falls back to the default reflection-based resolver.

cc @krwq @layomia @jeffhandley @ericstj

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, breaking-change

Milestone: -

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jul 6, 2022
@eiriktsarpalis eiriktsarpalis added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 6, 2022
eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this issue Jul 7, 2022
…t#71714

Include JsonSerializerContext in JsonSerializerOptions copy constructor. Fix dotnet#71716

Move reflection-based converter resolution out of JsonSerializerOptions. Fix dotnet#68878
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2022
eiriktsarpalis added a commit that referenced this issue Jul 7, 2022
* Remove implicit fallback to reflection-based serialization. Fix #71714

Include JsonSerializerContext in JsonSerializerOptions copy constructor. Fix #71716

Move reflection-based converter resolution out of JsonSerializerOptions. Fix #68878

* Address feedback & add one more test

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

* fix build

* Bring back throwing behavior in JsonSerializerContext and add tests

* Only create caching contexts if a resolver is populated

* Add null test for JsonSerializerContext interface implementation.

* skip RemoteExecutor test in netfx targets

* Add DefaultJsonTypeInfoResolver test for types with JsonConverterAttribute

* remove nullability annotation

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>

Co-authored-by: Stephen Toub <stoub@microsoft.com>
Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2022
@eiriktsarpalis eiriktsarpalis linked a pull request Jul 14, 2022 that will close this issue
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
1 participant