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 JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup #46225

Closed
wants to merge 6 commits into from

Conversation

brunolins16
Copy link
Member

@brunolins16 brunolins16 commented Jan 23, 2023

Fixes #46143

Waiting for #46303

@ghost ghost added the area-runtime label Jan 23, 2023
@@ -13,13 +12,8 @@ public void PostConfigure(string? name, JsonOptions options)
{
if (options.SerializerOptions.TypeInfoResolver is not null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we checking for null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think make sense we setup our internal context when the app is configured for reflection-based, and actually this condition might be something like options.SerializerOptions.TypeInfoResolver is not null && if (options.SerializerOptions.TypeInfoResolver is not DefaultTypeInfoResolver or options.SerializerOptions.TypeInfoResolver is JsonSerializerContext

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it is useless, I believe our context will never be used in this case since it will be added later or even worse, if it was null, now the app might break because we set a context. The only scenario where adding it helps it if, when null, another post-config sets a context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the app break if we set a context?

Copy link
Member

@davidfowl davidfowl Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary is, once you add a context, the reflection based one will not be used ever?

cc @eiriktsarpalis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks right:

var options = new JsonSerializerOptions();
options.AddContext<MyContext>();
JsonSerializer.Serialize(new Student("David", 1), options);

record Student(string Name, int Grade);
record Person(string Name);

[JsonSerializable(typeof(Person))]
partial class MyContext : JsonSerializerContext
{

}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary is, once you add a context, the reflection based one will not be used ever?

Correct. If the TypeInfoResolver property has been configured, then the serializer will not attempt to populate it with reflection resolution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary is, once you add a context, the reflection based one will not be used ever?

The only scenario it will not be true is if you set the DefaultTypeInfoResolver first, but in this case your source gen context will not be used. This will work:

var options = new JsonSerializerOptions() { TypeInfoResolver = new DefaultJsonTypeInfoResolver() };
options.AddContext<MyContext>();
JsonSerializer.Serialize(new Student("David", 1), options);

record Student(string Name, int Grade);
record Person(string Name);

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunolins16 brunolins16 marked this pull request as ready for review January 25, 2023 00:21
@brunolins16 brunolins16 requested a review from a team January 25, 2023 00:21
@@ -11,15 +11,10 @@ internal sealed class ProblemDetailsJsonOptionsSetup : IPostConfigureOptions<Jso
{
public void PostConfigure(string? name, JsonOptions options)
{
if (options.SerializerOptions.TypeInfoResolver is not null)
if (options.SerializerOptions.TypeInfoResolver is not null && options.SerializerOptions.TypeInfoResolver is not DefaultJsonTypeInfoResolver)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't making sense to me. Why don't we always want to use this context? Also changes like this that aren't obvious need to big comment 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a 😂 "huge" comment when we decided what is the right thing to do. As I mentioned && options.SerializerOptions.TypeInfoResolver is not DefaultJsonTypeInfoResolver can be easily removed, however, it will be useless, unless we change how the AddContext works (to prepend instead of append).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless we change how the AddContext works (to prepend instead of append)

After a conversation with @eiriktsarpalis, changing it doesn't make sense. As an example:

options.AddContext<Foo>();
options.AddContext<Bar>();
options.AddContext<Baz>();

Should be equivalent to writing:

options.TypeInfoResolver = JsonTypeInfoResolver.Combine(Foo.Default, Bar.Default, Baz.Default);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem seems like the JSON fallback needs to be applied BEFORE the first class to AddContext to make everything work in all cases. Is that right? We don't want to override the reflection resolver if it'll eventually be newed up lazily by the the JSON library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON fallback needs to be applied BEFORE the first class to AddContext to make

After. but everything else is correct. I believe we should keep the options.SerializerOptions.TypeInfoResolver is not null to avoid breaking an app due to our internal context but I am less concerned about options.SerializerOptions.TypeInfoResolver is not DefaultJsonTypeInfoResolver since it will not make any difference, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with David that the source generated context could go first even in cases where you have reflection enabled. It would save you a relatively small amount of initialization costs, particularly around JITing all the dynamic methods that ProblemDetails requires. But maybe not a huge win if everything else is dynamically generated.

You wouldn't need to change the semantics of AddContext to achieve that though, you could simply use the Combine method to prepend whatever resolver you like to user configuration.

@brunolins16 brunolins16 added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Jan 31, 2023
brunolins16 added a commit to brunolins16/aspnetcore that referenced this pull request Feb 3, 2023
@brunolins16
Copy link
Member Author

Closing this PR in favor of #46303.

@brunolins16 brunolins16 closed this Feb 3, 2023
@ghost
Copy link

ghost commented Feb 3, 2023

Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup
3 participants