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.

Sign up for GitHub

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

Jump to bottom

Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch #45886

Merged
merged 11 commits into from
Jan 20, 2023
Merged

Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch #45886

merged 11 commits into from
Jan 20, 2023

Conversation

brunolins16
Copy link
Member

@brunolins16 brunolins16 commented Jan 5, 2023 •
edited
Loading

@build-analysis build-analysis bot mentioned this pull request Jan 5, 2023
Open
5 tasks
@brunolins16 brunolins16 marked this pull request as ready for review January 9, 2023 21:14
@eerhardt
Copy link
Member

eerhardt commented Jan 9, 2023

This looks good to start, but we should have code that switches based on this setting.

@brunolins16
Copy link
Member Author

brunolins16 commented Jan 9, 2023 •
edited
Loading

This looks good to start, but we should have code that switches based on this setting.

💯 I was holding this PR until #45405 is merged to have JsonOptions behave based on this switch, but it is taking longer than I expected.

@brunolins16 brunolins16 marked this pull request as draft January 10, 2023 21:33
@brunolins16 brunolins16 marked this pull request as ready for review January 12, 2023 00:41
@brunolins16 brunolins16 changed the title Adding EnsureJsonTrimmability switch Making JsonnOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch Jan 12, 2023
@brunolins16 brunolins16 changed the title Making JsonnOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch Jan 12, 2023
@JamesNK
Copy link
Member

JamesNK commented Jan 13, 2023

Please rebase 🙏

There have been some suppressions added to JsonOptions since the changes were made. Should verify that they are no longer needed with this improvement.

@JamesNK
Copy link
Member

JamesNK commented Jan 13, 2023

With this change, should the warnings be removed from ProblemDetails.Extensions?

@JamesNK JamesNK mentioned this pull request Jan 13, 2023
Merged
@brunolins16
Copy link
Member Author

With this change, should the warnings be removed from ProblemDetails.Extensions?

Excellent question. IDK, users can now specify the types,needed for Extensions, in their own context and will combine with the internal context #45906 (review)

@davidfowl
Copy link
Member

Yes, the warnings should be removed.

eerhardt
eerhardt reviewed Jan 18, 2023
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

It would be good to get some tests written when Microsoft.AspNetCore.EnsureJsonTrimmability == false. In dotnet/runtime, we use the RemoteExecutor class to run test code with different runtime config options. See https://github.com/dotnet/runtime/blob/df9de14e807b5327977bcd288fac5dfb4a6c6a6b/src/libraries/System.Reflection.Emit.Lightweight/tests/DynamicMethodCtor.cs#L178-L201 for an example.

src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetails.cs Show resolved Hide resolved
src/Http/Http.Extensions/src/HttpRequestJsonExtensions.cs
@@ -32,13 +32,14 @@ public static class HttpRequestJsonExtensions
/// <param name="request">The request to read from.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
[RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)]
Copy link
Member

Choose a reason for hiding this comment

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

Why havne't we done this for all methods in this class? For example,

    public static async ValueTask<TValue?> ReadFromJsonAsync<TValue>(
        this HttpRequest request,
        JsonSerializerOptions? options,
        CancellationToken cancellationToken = default)

Copy link
Member

Choose a reason for hiding this comment

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

Similar question for HttpResponseJsonExtensions.

Copy link
Member Author

@brunolins16 brunolins16 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When the user provides a JsonSerializerOptions the TypeInfoResolver could be null, default value, and we cannot call GetTypeInfo dotnet/runtime#80529

src/Http/Http.Extensions/src/JsonOptions.cs
@@ -26,7 +27,7 @@ public class JsonOptions
// The JsonSerializerOptions.GetTypeInfo method is called directly and needs a defined resolver
// setting the default resolver (reflection-based) but the user can overwrite it directly or calling
// .AddContext<TContext>()
TypeInfoResolver = CreateDefaultTypeResolver()
TypeInfoResolver = TrimmingAppContextSwitches.EnsureJsonTrimmability ? null : CreateDefaultTypeResolver()
};

// Use a copy so the defaults are not modified.
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan for the suppressions on line 39-44?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, they will suppress in the Microsoft.AspNetCore.Http.Extensions.WarningSuppressions.xml and I will file an issue to start a discussion if we need something similar to runtime (LibraryBuild.xml).

src/Http/Http.Extensions/src/ProblemDetailsJsonContext.cs Show resolved Hide resolved
@brunolins16 brunolins16 requested review from a team, dougbu and wtgodbe as code owners January 18, 2023 22:43
@brunolins16
Copy link
Member Author

brunolins16 commented Jan 18, 2023 •
edited
Loading

@dougbu @wtgodbe I don't know much about the testing infrastructure, but I tried to follow the pattern we already have. Feel free to point out what is the right approach to introduce the Microsoft.Dotnet.RemoteExecutor package.

5b432db

@wtgodbe
Copy link
Member

wtgodbe commented Jan 18, 2023

@dougbu @wtgodbe I don't know much about the testing infrastructure, but I tried to follow the pattern we already have. Feel free to point out what is the right approach to introduce the Microsoft.Dotnet.RemoteExecutor package.

Looks good to me

@brunolins16
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@wtgodbe wtgodbe merged commit 40dd230 into dotnet:main Jan 20, 2023
@ghost ghost added this to the 8.0-preview1 milestone Jan 20, 2023
@brunolins16 brunolins16 deleted the brunolins16/json/aot/feature-switch branch January 20, 2023 19:01
@brunolins16 brunolins16 mentioned this pull request Jan 26, 2023
Closed
@eiriktsarpalis eiriktsarpalis mentioned this pull request Mar 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layomia layomia layomia left review comments

@eerhardt eerhardt eerhardt left review comments

@JamesNK JamesNK JamesNK left review comments

@davidfowl davidfowl davidfowl approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@dougbu dougbu Awaiting requested review from dougbu

@wtgodbe wtgodbe Awaiting requested review from wtgodbe wtgodbe is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.0-preview1
Development

Successfully merging this pull request may close these issues.
6 participants
@brunolins16 @eerhardt @JamesNK @davidfowl @wtgodbe @layomia