Skip to content

Enable trimming for middleware and friends #41373

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

Merged
merged 1 commit into from
Apr 30, 2022

Conversation

pranavkm
Copy link
Contributor

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 26, 2022
@@ -132,8 +134,6 @@ private async Task InvokeCore(HttpContext context)
return null;
}

var contextType = Type.GetType(contextTypeName)!;
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not? The current code expects contextTypeName to match an exact FQN but doesn't use the resulting match: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/Diagnostics.EntityFrameworkCore/src/MigrationsEndPointMiddleware.cs#L121-L135.

@@ -9,6 +10,8 @@ namespace Microsoft.AspNetCore.Session;

internal static class CookieProtection
{
[RequiresUnreferencedCode("This API uses Data Protection which may attempt to load types in a way that cannot be statically analyzed. " +
Copy link
Member

Choose a reason for hiding this comment

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

@blowdart This is problematic and we don't have a trimmable approach for doing something like data protection as yet. Something to think about in the future.

@@ -76,6 +77,8 @@ public class SessionMiddleware
/// </summary>
/// <param name="context">The <see cref="HttpContext"/>.</param>
/// <returns>A <see cref="Task"/> that completes when the middleware has completed processing.</returns>
[RequiresUnreferencedCode("This API uses Data Protection which may attempt to load types in a way that cannot be statically analyzed. " +
Copy link
Member

Choose a reason for hiding this comment

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

😢

@@ -119,3 +119,117 @@ public UnconditionalSuppressMessageAttribute(string category, string checkId)
/// </summary>
public string? Justification { get; set; }
}

[AttributeUsage(
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 this for? NS2.0 projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Healthchecks cross-compiles and needs it. The current approach is to include an internal attribute rather than ifdef the attribute usage.

@pranavkm pranavkm force-pushed the middleware-trimming branch 3 times, most recently from 0b57518 to a620b06 Compare April 27, 2022 04:35
@pranavkm pranavkm marked this pull request as ready for review April 27, 2022 13:07
@JamesNK
Copy link
Member

JamesNK commented Apr 28, 2022

Let's play around with DataProtection trimming annotations before propagating its warning out to APIs that depend on it - #41411

@pranavkm pranavkm force-pushed the middleware-trimming branch from a620b06 to d6e57b0 Compare April 28, 2022 13:35
@pranavkm
Copy link
Contributor Author

Took out Antiforgery and Session from the list as they were the only two affected by DataProtection. I'll have a another look at DataProtection, see what might work there.

@@ -13,6 +13,7 @@

<ItemGroup>
<!-- Packages required to produce a complete dependency graph for the trimmer -->
<Reference Include="System.Configuration.ConfigurationManager" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does unchanged code need a new dependency e.g., is this related to the src/Shared/TrimmingAttributes.cs changes❔

Copy link
Member

Choose a reason for hiding this comment

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

This project references everything in TrimmableProjects.props

A newly added trimmable project requires it.

@JamesNK JamesNK merged commit 81cb87e into dotnet:main Apr 30, 2022
@ghost ghost added this to the 7.0-preview5 milestone Apr 30, 2022
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants