Migrate CacheFileFormat to System.Text.Json

[Nigusu-Allehu]

Bug

Fixes: NuGet/Home#13059

Description

This PR makes sure we use System.Text.Json to read the cache file in CacheFileFormat.

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.

[nkolev92]

There's a few comments, but these are really conversations, stylistic ideas.

Once we're closed on this, I think this is ready to

[zivkan]

The only place these properties are used are in CacheFile, in JsonPropertyName attributes. I think we're better off just putting the strings directly in the attribute (for example [JsonPropertyName("version")]), rather than having this class.

[zivkan]

Regarding https://github.com/NuGet/NuGet.Client/pull/6081/files#r1825078556, we lose about 30% perf needing to go though an additional converter:

code

using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

[MemoryDiagnoser]
public class Program
{
    private static void Main(string[] args)
    {
        BenchmarkRunner.Run<Program>();
    }

    private static JsonSerializerOptions options1;
    private static JsonSerializerOptions options2;

    [GlobalSetup]
    public void Setup()
    {
        options1 = new JsonSerializerOptions();
        options1.Converters.Add(new ThingConverter1());

        options2 = new JsonSerializerOptions();
        options2.Converters.Add(new ThingConverter2());
    }

    private byte[] json = Encoding.UTF8.GetBytes("{\"Value\":\"something\"}");

    [Benchmark]
    public IThing? JsonSerializerDeserialize()
    {
        return JsonSerializer.Deserialize<IThing>(json, options1);
    }

    [Benchmark]
    public IThing? JsonConverterRead()
    {
        return JsonSerializer.Deserialize<IThing>(json, options2);
    }

    [Benchmark(Baseline = true)]
    public Thing? Baseline()
    {
        return JsonSerializer.Deserialize<Thing>(json);
    }

    public interface IThing
    {
        public string Value { get; }
    }

    public class Thing : IThing
    {
        public required string Value { get; init; }
    }

    private class ThingConverter1 : JsonConverter<IThing>
    {
        public override IThing? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            return JsonSerializer.Deserialize<Thing>(ref reader, options);
        }

        public override void Write(Utf8JsonWriter writer, IThing value, JsonSerializerOptions options)
        {
            throw new NotImplementedException();
        }
    }

    private class ThingConverter2 : JsonConverter<IThing>
    {
        public override IThing? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            JsonConverter<Thing> converter = (JsonConverter<Thing>)options.GetConverter(typeof(Thing));
            return converter.Read(ref reader, typeof(Thing), options);
        }

        public override void Write(Utf8JsonWriter writer, IThing value, JsonSerializerOptions options)
        {
            throw new NotImplementedException();
        }
    }
}

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
JsonSerializerDeserialize	189.38 ns	1.185 ns	1.050 ns	2.00	0.02	0.0076	128 B	1.00
JsonConverterRead	123.56 ns	0.691 ns	0.647 ns	1.31	0.01	0.0076	128 B	1.00
Baseline	94.51 ns	0.602 ns	0.563 ns	1.00	0.01	0.0076	128 B	1.00

We lose even more perf calling JsonSerializer.Deserialize from the converter, rather than my suggestion above, getting the converter directly from options, and calling the converter.

It depends on how much we want to microoptimize restore. 30 nanoseconds per project, on my machine, but nanoseconds are super small. Is it worth the public API breaking changes for it? Probably not. But I still think that IAssetsLogMessage doesn't have a reason to exist.

But I think the additional 60 nanoseconds lost by using JsonSerializer.Deserialize is not worth the 1 line of code it saves over getting the converter directly.

[zivkan]

I'm pretty sure that System.Text.Json can still call the constructor if it's internal. If this class had #nullable enable, then you'd see the problem with adding this constructor: it doesn't ensure that types the rest of our code expects to always be non-null, is in fact non-null. Therefore, we'd have to make Message a string?, and now everything that processes AssetsLogMessages has to handle when Message is null.

Technically, there's still the problem even if this constructor is internal or private, but at least we can suppress the nullable warning explaining why it happens. Still, I'd prefer if it could be avoided. Can we somehow use [JsonConstructor] on the LogLevel, NuGetLogCode, string, string overload? I don't have experience with trying to tell the JsonSerializer to set some values via the constructor, and different values via properties.

For much the same reasons, a good API is one you can't use wrong, so we don't want the rest of NuGet.Client being able to construct instances where message is null. If we need this constructor for JSON deserialization, then we should limit it's potential misuse.

[zivkan]

this logLevel == LogLevel.Warnings condition in order to set WarningLevel's default looks like a behaviour change from the old code, and it's not intuitive to me why the condition is needed.

[zivkan]

I believe you can use what's already built in: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/customize-properties?pivots=dotnet-8-0#enums-as-strings

[microsoft-github-policy-service]

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

[microsoft-github-policy-service]

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