Remove Microsoft.Extensions.Caching.Memory reference since it is platform provided

[nkolev92]

Follow up to https://devblogs.microsoft.com/dotnet/nuget-package-pruning-in-dotnet-10/?commentid=22975#comment-22975.

QUALITY CHECKLIST

• Changes implemented in code
• Complies with our contributing guidelines
• Adapted tests Existing tests should pass just fine. Not audit issues reported.
• Documentation updated

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.12%. Comparing base (034b101) to head (2f21ada).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2017   +/-   ##
=======================================
  Coverage   92.12%   92.12%           
=======================================
  Files         438      438           
  Lines       15044    15044           
  Branches     2488     2488           
=======================================
  Hits        13859    13859           
  Misses        731      731           
  Partials      454      454           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bkoelman]

That breaks the build.

[nkolev92]

I see the problem.

The ASP.NET framework ref is what actually provides Microsoft.Extensions.Caching.Memory and because it's transitive and to make restore repeatable, the framework refs the SDK uses are the ones specified in the project.

By adding the framework ref explicitly, you get a broader set of prunable packages.
This should now fix the build.

I'll file some issues to improve this behavior. We had talked about a warning to help identify this scenario at build time. I'll file some issues and improve the documentation.

[bkoelman]

Okay, I think that's a reasonable change for this project, because JsonApiDotNetCore.OpenApi.Swashbuckle depends on JsonApiDotNetCore, which already has the framework ref, so we can safely add it there, too. Though it should be redundant, because Swashbuckle.AspNetCore (which our JsonApiDotNetCore.OpenApi.Swashbuckle depends on) already pulls in the framework ref, and JsonApiDotNetCore pulls that in, too.

Though I wonder what the guidance would be for low-level libraries. For example, a hypothetical YAML parser that uses only Base64UrlTextEncoder, which is defined in the Microsoft.AspNetCore.WebUtilities NuGet package. Once that package becomes vulnerable, should it also add a framework ref to Microsoft.AspNetCore.App, just to silence the warning? That seems overkill. There are many such general-purpose libraries that should not require a dependency on ASP.NET (think of configuration providers, logging frameworks, database providers and test assertion libraries). Once the framework ref is added, all users are required to install the ASP.NET runtime in production, instead of just the .NET runtime, right? That's a breaking change. It sounds better to me for packages to express the minimal dependencies they require, keeping consumption flexible.

The NU1510 warning that is emitted when I remove the warning suppression:

<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.1"
    Condition="'$(TargetFramework)' == 'net8.0'">

Is:

PackageReference Microsoft.Extensions.Caching.Memory will not be pruned. Consider removing this package from your dependencies, as it is likely unnecessary.

This looks questionable, because:

1. It isn't actually correct, because the reference does get pruned (the framework ref overrides it).
2. The warning is emitted for both net8.0 and net10.0, while it should apply to net8.0 only:
   

The implication of how this warning works today is that package owners need to choose between:

1. Don't care about vulnerabilities in dependencies. It's the responsibility of consuming apps to decide whether to bump to a non-vulnerable version. Consumers have no way to know whether a higher non-vulnerable version of the dependency is compatible. (uncommon)
2. Remove the dependency and add a framework ref instead (a breaking change). The warning doesn't suggest doing so.
3. Bump to a non-vulnerable version, as I did. The warning must be suppressed.

I think many package authors choose 3 for good reasons. The warning should be a suggestion because its message reads "likely unnecessary," and low-level libraries are a legitimate use case to suppress it. In general, keeping the reference should be fine, since it will be pruned anyway and is harmless.

I'd love to hear your thoughts on this.

[bkoelman]

Perhaps I'm misunderstanding all of this. https://stackoverflow.com/questions/64478618/what-does-frameworkreference-include-microsoft-aspnetcore-app-actually-do, suggests that FrameworkReference only pulls in packages without a pinned version. Doing so doesn't require the ASP.NET runtime to be installed. And, apparently, it isn't transitive. So that would mean all low-level libraries could safely add a framework reference (or multiple). But the same link states it results in larger binaries when trimming is used. I'm a bit lost on the implications and how it all works.

I also noticed that nuget.org doesn't list framework dependencies, although they do appear in the .nuspec file.

[nkolev92]

FrameworkReferences are transitive already.

So we're not really adding anything new by explicitly specifying the FrameworkReference.
There's no net change for build/publish by doing that, it's just restore.

[bkoelman]

FrameworkReferences are transitive already.

Then why do I need to add it in JsonApiDotNetCore.OpenApi.Swashbuckle, which depends on two libraries that already pull in the Microsoft.AspNetCore.App framework reference?

[nkolev92]

Transitive framework reference application and pruning are both restore applied.

Currently, pruning affects framework reference application by virtue of picking packages.
The opposite can't happen cause it may lead to an indeterministic restore in some cases.

NuGet/Home#14920 option 1 touches on it.

[bkoelman]

Thanks for the clarification and for opening an issue.

Could you please address my concerns in #2017 (comment), especially about low-level libraries? At work, I'm part of a team that builds many of those, and discussions keep coming up about whether we should keep references to System/Microsoft libraries or add framework references instead. Some examples here and here. My point of view has always been to keep these explicit references, but now I'm not so sure anymore. Would it be recommended to replace them all with framework references? Is there any guidance/docs, or are there limitations I should be aware of before doing so?

And are the oddities regarding NU1510 tracked anywhere? Particularly, I find the message "will not be pruned" very confusing. As a developer, my understanding of the concept of pruning is: what will be removed from the set of libraries my project depends on, not what the restore command does internally to validate the graph.