Skip to content

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

Merged
wtgodbe merged 11 commits intodotnet:mainfrom
brunolins16:brunolins16/json/aot/feature-switch
Jan 20, 2023
Merged

Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch#45886
wtgodbe merged 11 commits intodotnet:mainfrom
brunolins16:brunolins16/json/aot/feature-switch

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
@brunolins16 brunolins16 requested a review from eerhardt 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
@brunolins16 brunolins16 requested a review from eerhardt January 12, 2023 21:39
@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

brunolins16 commented Jan 13, 2023

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

davidfowl commented Jan 13, 2023

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.Extensions/src/HttpRequestJsonExtensions.cs
/// <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

@eerhardt eerhardt Jan 18, 2023

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

@eerhardt eerhardt Jan 18, 2023

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
TypeInfoResolver = TrimmingAppContextSwitches.EnsureJsonTrimmability ? null : CreateDefaultTypeResolver()
};

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

@eerhardt eerhardt Jan 18, 2023

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

@brunolins16 brunolins16 Jan 18, 2023

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).

JamesNK
JamesNK reviewed Jan 18, 2023
View reviewed changes
@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

davidfowl
davidfowl reviewed Jan 18, 2023
View reviewed changes
@brunolins16
Copy link
Member Author

brunolins16 commented Jan 19, 2023

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 19, 2023

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

@JamesNK JamesNK JamesNK left review comments

@eerhardt eerhardt eerhardt 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

+1 more reviewer

@layomia layomia layomia left review comments

Reviewers whose approvals may not affect merge requirements

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