Conversation
Forward the runtime FeatureSwitchDefinitionAttribute while retaining the compatibility polyfill for older target frameworks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Preserve one-based line and UTF-8 byte-column positions across stream buffer boundaries and use them in package-spec parse diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Add source-generated metadata and explicit converters, preserve the public Newtonsoft surface and fallback, and select System.Text.Json through the standard feature switch or environment opt-in. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Parse SDK resolver data incrementally with the shared buffered reader while preserving caching, diagnostics, Newtonsoft fallback, and the existing lazy-load behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
cf29f6b to
36956ea
Compare
Add opt-in V1, V2, and V3 packages.lock.json parsing with stream-based reading, preserve malformed-file diagnostics and stream ownership, and retain Newtonsoft as the default reader and writer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
36956ea to
1ef2127
Compare
baronfel
left a comment
There was a problem hiding this comment.
Some notes for the reader about why certain decisions were made
| #if NET9_0_OR_GREATER | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| // This is a supporting forwarder for an internal polyfill API. | ||
| [assembly: TypeForwardedTo(typeof(FeatureSwitchDefinitionAttribute))] | ||
|
|
||
| #else |
There was a problem hiding this comment.
This is porting of a pattern that @DustinCampbell corrected on MSBuild's polyfills.
There was a problem hiding this comment.
Do you have a link to the reasoning? If the class is internal (and assuming no IVT), it shouldn't be necessary from my understanding.
There was a problem hiding this comment.
I'm not 100% certain myself, but here's where the change I'm referencing was introduced: dotnet/msbuild#12977
|
|
||
| internal JsonTokenType TokenType => _reader.TokenType; | ||
|
|
||
| internal int LineNumber |
There was a problem hiding this comment.
Adding this tracking to the STJ parser means that error messages that used Newtonsoft's Line Info interfaces can keep line info on STJ parsing paths - we use this here.
| if (useSystemTextJson) | ||
| { | ||
| jsonStream = File.OpenRead(globalJsonPath); | ||
| } | ||
| else | ||
| { | ||
| json = File.ReadAllText(globalJsonPath); | ||
| } |
There was a problem hiding this comment.
This parser, like the other parsers used in this repo, chooses which implementation (NJ or STJ) to use. Here I'm also trying to be a little more performant by streaming the data for STJ instead of reading the whole global.json into a string and checking for the msbuild-sdks node via contains. We can reduce this diff a bit by loading the string in memory like we do for NJ, but that feels memory-inefficient to me.
There was a problem hiding this comment.
Why continue the split? Are we concerned about perf? Compatibility?
There was a problem hiding this comment.
Compatibility is my main concern here. The NuGet team historically has cared a lot about having fall backs in case things didn't work, so the hopefully temporary parallel path with fallback was something I did to hopefully reduce the concern around taking this implementation.
| if (NuGetFeatureFlags.UseSystemTextJsonDeserializationFeatureSwitch) | ||
| { | ||
| return ReadRuntimeGraphWithSystemTextJson(stream); | ||
| } | ||
|
|
||
| if (NuGetFeatureFlags.IsSystemTextJsonDeserializationEnabledByEnvironment(environmentVariableReader)) | ||
| { | ||
| return ReadRuntimeGraph(streamReader); | ||
| return ReadRuntimeGraphWithSystemTextJson(stream); | ||
| } |
There was a problem hiding this comment.
STJ doesn't work great with a TextReader-based usage pattern. So for this parser, if we detect that STJ should be used, we pivot to using Stream directly so that we aren't forced to buffer the TextReader. This leads to a tiny bit of code duplication (if you use the TextReader-based call paths we do buffer when using STJ, which is not the best), but I think this is relatively understandable.
| { | ||
| RuntimeGraphJsonModel json = STJJsonSerializer.Deserialize( | ||
| stream, | ||
| JsonRuntimeFormatContext.Default.RuntimeGraphJsonModel) |
There was a problem hiding this comment.
We're using source-generated STJ serializers here instead of hand-rolling the parsing. Raise a flag if this is not desired, but I think we have all of the extensibility we need even with the auto-generated parsers.
There was a problem hiding this comment.
Do we even need to use STJJsonSerializer.Deserialize here? The RuntimeGraphJsonModelConverter has all the logic to turn a stream into a RuntimeGraphJsonModel, why not just use that directly?
There was a problem hiding this comment.
A few colliding thoughts:
- the ideal interface would be
STJJsonSerializer.Deserialize<T>(....)in the general case - I'd like as much as possible all serialization/deserialization to have that consistent interface for auditing/consistency reasons - if we tried to use the converter directly, we'd have to handle managing the Utf8JsonReader, etc ourselves. the nice calling interface here saves us that bookkeeping
| internal sealed class RuntimeGraphJsonModel | ||
| { | ||
| [JsonPropertyName("runtimes")] | ||
| [JsonConverter(typeof(RuntimeDescriptionCollectionJsonConverter))] |
There was a problem hiding this comment.
Having these custom converters lets us do the mapping of these complex types in a streaming fashion, without having to parse into intermediate Dictionary<string, object> mappings and then re-parse into the final structures. Should save some memory/intermediate allocations.
There was a problem hiding this comment.
This would make a good comment in the code.
There was a problem hiding this comment.
Do these attributes actually help? Since the whole type has a JsonConverter, and that converter reads these 2 properties and calls the converters itself, what good are these attributes?
There was a problem hiding this comment.
Good call out - will end up removing the attributes. The type level converter is necessary to keep some NJ compatibility. NJ takes the 'last' property when property names match, while STJ keeps the first one by default in generated type converters, so some compat tests in this repo failed when removing the type-level converter in favor of just the property-level ones.
There was a problem hiding this comment.
As I wrote in #7601 (comment), System.Text.Json's JsonConverters causes it to load the full object into a single buffer, so if the object is huge (many properties, nested objects, etc), it can easily need a LoH sized buffer. This won't come up in BenchmarkDotNet allocation numbers because STJ uses pooled buffers and will re-use the same buffer in the single threaded benchmark. But when Restore runs on multiple projects concurrently, it might need multiple buffers at the same time and therefore create multiple LoH sized buffers.
So, an intermediate Dictionary<string, object> might be less bad. Or using the non-async serializer that we had to write for assets file reading, since that can actually do streaming deserialization with custom converters, with only small buffers.
Unfortunately, NuGet's design of using data structures which are not a 1:1 mapping to the json schema, plus very large files, makes System.Text.Json a poor fit. But if we're going to get rid of newtonsoft.json, someone needs to implement this in a way that doesn't cause perf problems.
There was a problem hiding this comment.
Great feedback overall @zivkan - I just pushed up a line of commits that 'commits' to STJ reading of all of these formats, always streaming and never buffering large file contents.
| PackagesLockFile lockFile; | ||
| if (NuGetFeatureFlags.UseSystemTextJsonDeserializationFeatureSwitch | ||
| || NuGetFeatureFlags.IsSystemTextJsonDeserializationEnabledByEnvironment()) | ||
| { | ||
| lockFile = ReadLockFileWithSystemTextJson(stream); | ||
| } | ||
| else | ||
| { | ||
| using var textReader = new StreamReader(stream); | ||
| lockFile = ReadLockFile(textReader); | ||
| } |
There was a problem hiding this comment.
Similar to the runtime graph format, STJ works better with streams instead of TextReaders. So if we detect that STJ is used, we do a slightly-parallel code path that works on Streams to lean into what STJ expects. Slight duplication with the "parse, the set Path, and return good errors" pattern, but worth it I think for the memory usage implications.
Preserve encoded stream and duplicate property behavior while sharing the common JSON helpers. Simplify related documentation and comments, and add focused regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
| bytes = bytes.Slice(newlineIndex + 1); | ||
| } | ||
|
|
||
| bytePositionInLine += bytes.Length; |
There was a problem hiding this comment.
Does this logic still work correctly when this method is called from GetMoreBytesFromStream?
| var json = WriteLockFile(lockFile); | ||
| #pragma warning disable IL2026, IL3050 // WriteTo without converters is safe. See https://github.com/JamesNK/Newtonsoft.Json/blob/13.0.4/Src/Newtonsoft.Json/Linq/JToken.cs | ||
| json.WriteTo(jsonWriter, Array.Empty<JsonConverter>()); | ||
| #pragma warning restore IL2026, IL3050 |
There was a problem hiding this comment.
@baronfel - while you are in here, can you fix this place too?
There was a problem hiding this comment.
yep, just added parallel handling in here as well!
Remove property attributes that the root converter bypasses and document why root-level parsing is required for duplicate properties. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Document why non-seekable and non-UTF-8 streams use a TextReader before System.Text.Json parsing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Use the framework attribute directly on .NET 9 and later while retaining the internal compatibility definition for older targets. This avoids exposing the framework type as NuGet public API. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
| if (NuGetFeatureFlags.UseSystemTextJsonDeserializationFeatureSwitch) | ||
| { | ||
| return ReadRuntimeGraphWithSystemTextJson(textReader); | ||
| } | ||
|
|
||
| if (NuGetFeatureFlags.IsSystemTextJsonDeserializationEnabledByEnvironment(environmentVariableReader)) | ||
| { | ||
| return ReadRuntimeGraphWithSystemTextJson(textReader); | ||
| } | ||
|
|
||
| return ReadRuntimeGraphWithNewtonsoftJson(textReader); |
There was a problem hiding this comment.
The logic for determining whether to use System.Text.Json or Newtonsoft.Json is implemented multiple times in this PR. I think it would be better to centralize it to improve maintainability
There was a problem hiding this comment.
Per Andy's comments, it needs to be 'exploded' out a bit like this or else the trimmer can't reliably trim the unused portions away.
| PackagesLockFile lockFile, | ||
| IEnvironmentVariableReader environmentVariableReader) | ||
| { | ||
| if (NuGetFeatureFlags.UseSystemTextJsonDeserializationFeatureSwitch |
There was a problem hiding this comment.
This comment is out of scope for this PR but a switch named for deserialization also controls serialization .
There was a problem hiding this comment.
My agent thinks that this dual-meaning already existed before my PR in at least one case, but I hear what you're saying. I probably would have called the flag 'serialization' anticipating its use in both scenarios eventually.
| } | ||
| } | ||
|
|
||
| private static void WriteWithSystemTextJson(TextWriter textWriter, PackagesLockFile lockFile) |
There was a problem hiding this comment.
The PR description says, Newtonsoft remains the default compatibility fallback, and writing remains Newtonsoft-based and intentionally out of scope. However, it looks like we also use STJ for writing. Updating the PR description to reflect this would help.
There was a problem hiding this comment.
I've done another pass now - please let me know if there's anything more I can clean up!
| internal static RuntimeGraph ReadRuntimeGraphWithSystemTextJson(TextReader textReader) | ||
| { | ||
| RuntimeGraphJsonModel json = STJJsonSerializer.Deserialize( | ||
| textReader.ReadToEnd(), | ||
| JsonRuntimeFormatContext.Default.RuntimeGraphJsonModel) | ||
| ?? throw new STJJsonException(); | ||
|
|
||
| return ReadRuntimeGraph(json); |
There was a problem hiding this comment.
The Newtonsoft path closes the supplied TextReader because JsonTextReader.CloseInput defaults to true, while the STJ path calls ReadToEnd() and leaves the reader open. Could we preserve the existing behavior by closing the reader in the STJ path as well?
There was a problem hiding this comment.
Per Andy's suggestion I ended up purging the TextReader-based APIs in any case. Methods that take streams now more broadly communicate their expectations of the stream - content, ownership, etc.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
| WriteLockFile(jsonWriter, lockFile); | ||
| } | ||
|
|
||
| string json = Encoding.UTF8.GetString(stream.ToArray()).Replace("\r\n", "\n"); |
There was a problem hiding this comment.
I strongly believe we should not be creating APIs that have such performance problems. It's better to mark APIs with TextReader as obsolete and not AoT compatible, and force all usage onto Stream APIs that work with System.Text.Json without such bad perf issues.
There was a problem hiding this comment.
I see this is being called from public string Render(PackagesLockFile). Is there another overload that writes directly to a stream, perhaps something like public void Save(PackagesLockFile, Stream)? This just isn't a good API, the lock file can be huge, so allocating the whole thing in memory (multiple times!) is something we should fix.
I also really don't understand the point of Replace("\r\n", "\n") and the 3 lines down Replace("\n", textWriter.NewLine);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
| } | ||
| } | ||
|
|
||
| internal static class JsonElementExtensions |
There was a problem hiding this comment.
I no longer see GetRequiredString or GetUniqueProperties being called, so I think these extensions can be deleted.
| /// <summary>Feature switch for System.Text.Json deserialization. Defaults to <see langword="false"/> (Newtonsoft is the default).</summary> | ||
| /// <summary> | ||
| /// Selects System.Text.Json deserialization and <c>packages.lock.json</c> serialization. | ||
| /// Defaults to <see langword="false"/> (Newtonsoft.Json is the default). |
There was a problem hiding this comment.
If we have another commit, let's undo this comment.
| } | ||
| } | ||
|
|
||
| public static RuntimeGraph ReadRuntimeGraph(TextReader textReader) |
There was a problem hiding this comment.
Did you intend to delete this overload? If so, does it make the Newtonsoft.Json parsing code unreachable, so it all should be deleted?
I think it's worthwhile to keep it, but give it an [Obsolete] attribute, so anyone using it has a few versions to switch before we delete it. But honestly, I don't expect many people actually use this this API, so it's probably low impact to delete all the newtonsoft.json code
There was a problem hiding this comment.
I think this PR already does a lot.
It creates STJ readers/writers, it enables nullability, and it's deleting public APIs.
It makes hard to review and even harder to find bugs if introduces, so I'd really prefer to split things out.
| if (reader.TokenType != JsonTokenType.StartObject) | ||
| { | ||
| reader.Skip(); | ||
| return null; |
There was a problem hiding this comment.
Is this a change in behavior? I would have expected some kind of json exception when the property has the wrong type, but NuGet has a lot of questionable Newtonsoft.Json parsing code, so either way I won't be surprised. But I also know that LLMs love to write code that silently ignores errors, making it so much harder to investigate bugs.
This pattern is repeated several times in this file, so if we change it here, we should change it in all the places.
| context.MockSdkLogger.LoggedMessages.Count.Should().Be(1); | ||
| context.MockSdkLogger.LoggedMessages.First().Message.Should().Be( | ||
| $"Failed to parse \"{expectedGlobalJsonPath}\". Invalid character after parsing property name. Expected ':' but got: J. Path 'msbuild-sdks.Sdk2', line 5, position 10."); | ||
| $"Failed to parse \"{expectedGlobalJsonPath}\". 'i' is an invalid start of a property name. Expected a '\"'. LineNumber: 4 | BytePositionInLine: 2."); |
There was a problem hiding this comment.
Why did the old code think the error was on line 5 col 10, but the new code line 4, col 2? It could be a sign that the line and column counting has a bug.
| [Fact] | ||
| public void PackagesLockFileFormat_ReadWithCommentsAndVersionAfterDependencies() | ||
| { | ||
| var lockFileContent = @"{ |
There was a problem hiding this comment.
nitpick: The last two tests added in this file use """ string literals, but this one, and the next use @"
| { | ||
| const string content = """ | ||
| { | ||
| "runtimes": "invalid", |
There was a problem hiding this comment.
Were these tests validated on the Newtonsoft.Json parsers, to make sure they're asserting compatible behavior? I know Copilot likes to add exhaustive tests for code it writes, even if it would fail to pass on the test it was originally replacing.
| [Fact] | ||
| public void Read_WhenReadingMultilineJson_TracksTokenPosition() | ||
| { | ||
| var json = Encoding.UTF8.GetBytes("{\n \"a\": \"value\"\n}"); |
There was a problem hiding this comment.
nitpick: It would be easier for me as a human to verify if this sting used """, so I can see the line and columns myself
| [Fact] | ||
| public void Read_WhenTokenIsAfterBufferBoundary_TracksTokenPosition() | ||
| { | ||
| string json = "{\n \"padding\": \"" + new string('a', 1000) + "\",\n \"target\": \"value\"\n}"; |
There was a problem hiding this comment.
nitpick: same as the previous comment, if it used a """ string, then, I'd be able to more easily count the lines and columns to check the asserts have the right numbers.
| WriteLockFile(jsonWriter, lockFile); | ||
| } | ||
|
|
||
| string json = Encoding.UTF8.GetString(stream.ToArray()).Replace("\r\n", "\n"); |
There was a problem hiding this comment.
I see this is being called from public string Render(PackagesLockFile). Is there another overload that writes directly to a stream, perhaps something like public void Save(PackagesLockFile, Stream)? This just isn't a good API, the lock file can be huge, so allocating the whole thing in memory (multiple times!) is something we should fix.
I also really don't understand the point of Replace("\r\n", "\n") and the 3 lines down Replace("\n", textWriter.NewLine);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0782b2a6-d495-4dd2-ade4-1039d29ae149
|
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 30 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
|
@baronfel - were you splitting this up? or is your intention to get this PR merged? |
I've got it split up into separate PRs for each reader/writer to make it easier to review (and rebased on your work), but was waiting for some signal from the NuGet team before slamming them with 4 PRs :) |
|
@baronfel We'll be happy to take these as smaller PRs. It'll make things much easier to review. What I have in mind is this thread: #7601 (comment). |
Wonderful! I'll send those back probably Wednesday, when I'm back at the computer that has the work prestaged |
Replace the checked-in dist/ nupkgs with the official 7.12.0-rc.25 packages from the dotnet11 feed, and delete the dist package source. The NuGet.* version for the CLI now lives in eng/Versions.props as NuGetPackageVersionForCli, matching the existing StreamJsonRpcPackageVersionForCli pattern, so Aspire.Cli and Aspire.Cli.Tests stay in sync. The dotnet11 feed carries only prerelease NuGet.* versions and dotnet-public carries only the stable ones, so both sources map NuGet.* at the same specificity. Giving exact patterns to just one source makes it win over the other's wildcard and breaks the other consumer with NU1103. Since rc.25 still uses Newtonsoft.Json in places, disable Newtonsoft's serialization, component model, and dynamic feature switches, and collapse the residual dynamic warnings from Microsoft.CSharp and System.Linq.Expressions to a single warning per assembly. These can be removed once NuGet drops Newtonsoft.Json entirely (NuGet/NuGet.Client#7601). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: baa86aab-4a9f-44c7-a92e-34f6498c9e4e
Bug
Progress: dotnet/sdk#55497
Description
Migrate the NuGet JSON paths used by the .NET SDK and .NET CLI from Newtonsoft.Json to System.Text.Json so NativeAOT and trimming can remove the Newtonsoft deserialization closure. The final design makes the NativeAOT-critical APIs use System.Text.Json directly rather than retaining feature-switched compatibility branches.
What changed
TextReaderoverload was removed; the existingJTokenoverload and runtime graph writers remain Newtonsoft-based for compatibility.global.jsonwith the shared streaming System.Text.Json reader. File encoding normalization preserves support for BOM-detected non-UTF-8 files.packages.lock.jsonfile, stream, and string reads, plus file, stream, and string writes, now use System.Text.Json directly for formats V1, V2, and V3. The obsoleteTextReaderoverload was removed; the obsoleteTextWriteroverload remains Newtonsoft-based.NuGet.UseSystemTextJsonDeserializationAppContext switch andNUGET_USE_SYSTEM_TEXT_JSON_DESERIALIZATIONenvironment opt-in. Newtonsoft remains their default compatibility implementation.This PR does not remove every Newtonsoft reference from NuGet. Remaining uses include runtime graph and other legacy writers, the runtime graph
JTokencompatibility overload, the obsolete package-lockTextWriteroverload, and plugin DTO metadata.NativeAOT .NET CLI impact
A controlled win-x64 Release experiment integrated the reader-only checkpoint (
ace289d42) into the NativeAOT .NET CLI at SDK commit7e04840bcf. The baseline used NuGet7.10.0-rc.36417; the variant used packages built from this branch as7.10.0-rc.60002. The experiment predates the final direct package-lock writer and API cleanup, so these numbers describe the reader migration checkpoint rather than the current commit exactly.dotnet-aot.dllThe smaller graph also reduced NativeAOT build times:
IlcCompileValidation
net472andnet10.0.net472andnet10.0.net472andnet10.0.Newtonsoft.Jsonmarker.msbuild -getProperty, package-delivered SDK resolution, and locked package-SDK evaluation.PR Checklist