Skip to content

Add STJ path for PluginCacheEntry#7421

Open
Nigusu-Allehu wants to merge 3 commits into
devfrom
dev-nyenework-pluginCacheentry-stj
Open

Add STJ path for PluginCacheEntry#7421
Nigusu-Allehu wants to merge 3 commits into
devfrom
dev-nyenework-pluginCacheentry-stj

Conversation

@Nigusu-Allehu
Copy link
Copy Markdown
Member

@Nigusu-Allehu Nigusu-Allehu commented May 27, 2026

Bug

Fixes:

Description

Add STJ deserialization path for PluginCacheEntry

Adds a feature-flagged System.Text.Json deserialization path for plugin operation claims cache file read/write in PluginCacheEntry. The existing NSJ path is unchanged.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a System.Text.Json (STJ) serialization/deserialization path for plugin operation-claims cache entries, gated by the existing NuGet feature switch/environment variable, to align PluginCacheEntry with the broader NuGet.Protocol STJ migration.

Changes:

  • Added STJ-based read/write logic to PluginCacheEntry, controlled by NuGetFeatureFlags switch/env var.
  • Introduced a source-generated JsonSerializerContext for IReadOnlyList<OperationClaim>.
  • Expanded unit tests to run both Newtonsoft/STJ paths and validate cross-compatibility between the two formats.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/PluginCacheEntryTests.cs Parameterizes tests to exercise both serializer paths and adds cross-deserialization compatibility coverage.
src/NuGet.Core/NuGet.Protocol/Plugins/PluginCacheJsonContext.cs Adds STJ source-generation context for plugin cache payload (IReadOnlyList<OperationClaim>).
src/NuGet.Core/NuGet.Protocol/Plugins/PluginCacheEntry.cs Adds STJ-based cache file serialization/deserialization behind existing feature gating and injects env-var reader for tests.

Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginCacheEntry.cs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review May 28, 2026 22:40
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner May 28, 2026 22:40
@Nigusu-Allehu Nigusu-Allehu requested review from jebriede and jeffkl May 28, 2026 22:40
var serializer = new JsonSerializer();
using (var sr = new StreamReader(content))
using (var jsonTextReader = new JsonTextReader(sr))
if (NuGetFeatureFlags.UseSystemTextJsonDeserializationFeatureSwitch
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that this is just internal, NuGet generated, nuget read, I think we may be able to remove the non STJ path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

doesn't LoadFromFile() expose this method because it is a public API and uses this method as a utility method.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am assuming your point is rather about cache file. But we still have a public API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants