Using System.Text.Json for assets file parsing is failing .NET SDK tests

[nkolev92]

See: dotnet/sdk#38774.

[zivkan]

I only checked one test, Microsoft.NET.Build.Tests.GivenThatWeWantDiagnosticsWhenAssetsFileCannotBeRead.It_reports_corrupt_file, but its failure is:

 System.NullReferenceException: Object reference not set to an instance of an object.
    at NuGet.ProjectModel.LockFile.GetTarget(String frameworkAlias, String runtimeIdentifier)
    at Microsoft.NET.Build.Tasks.LockFileExtensions.GetTargetAndReturnNullIfNotFound(LockFile lockFile, String frameworkAlias, String runtimeIdentifier) in D:\src\sdk\src\Tasks\Microsoft.NET.Build.Tasks\LockFileExtensions.cs:line 14

My understanding is that this means LockFileFormat didn't not error out while loading the assets file, and now the SDK the is trying to pull information out of the LockFile data structure (via the LockFile.GetTarget method we provide), and it's throwing a null reference exception.

The test name makes it clear it's testing error scenarios, so it's not a "serious" failure (maybe the new code correctly handles all happy cases?), but the error handling appears to have changed with our STJ implementation.

[akoeplinger]

NuGet/NuGet.Client#5637 fixed the It_reports_corrupt_file test, now we just have the other one remaining:

Microsoft.NET.Build.Tasks.UnitTests.GivenAResolvePackageDependenciesTask.ItAssignsDiagnosticLevel [FAIL]
Expected value to be 3, but found 0 (difference of -3).
  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
     at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
     at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
     at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
     at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
     at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
     at FluentAssertions.Numeric.NumericAssertions`2.Be(T expected, String because, Object[] becauseArgs)
  /_/src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAResolvePackageDependenciesTask.cs(174,0): at Microsoft.NET.Build.Tasks.UnitTests.GivenAResolvePackageDependenciesTask.ItAssignsDiagnosticLevel()
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
     at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[akoeplinger]

@zivkan @jgonz120 I extracted the test into a standalone repro:

var lockFile = new NuGet.ProjectModel.LockFileFormat().Read("failing.lock.json");

with this file: failing.lock.json

It looks like it's failing to parse the file, e.g. Version is -2147483648 instead of the expected 3

Before:

After:

[jgonz120]

@zivkan @jgonz120 I extracted the test into a standalone repro:

var lockFile = new NuGet.ProjectModel.LockFileFormat().Read("failing.lock.json");

with this file: failing.lock.json

It looks like it's failing to parse the file, e.g. Version is -2147483648 instead of the expected 3

Before:

After:

The lock file contains a string for the warning level when it's supposed to be a number. The test needs to be updated to create a properly formatted file.

[zivkan]

@jgonz120 shouldn't LockFileFormat be throwing an format exception, rather than successfully returning an object with an invalid value? I don't think it's just a test setup problem.

[akoeplinger]

I can confirm it parses when I change the warningLevel to be a number instead of a string so this should unblock the sdk code flow.

I agree though that throwing some exception during parsing would be better.

[jgonz120]

Agreed, but the previous code flow behaves this way so I feel like updating it would be out of scope for this change.

https://github.com/NuGet/NuGet.Client/blob/d49b8abb6b64d2a49a439380af8c62d099ba31fb/src/NuGet.Core/NuGet.ProjectModel/LockFile/LockFileFormat.cs#L118

[akoeplinger]

Interesting. That also means code like this doesn't make sense since we'll never get an exception: https://github.com/dotnet/sdk/blob/302ca4c93537954c056289cde3a2c4faac0d9ce2/src/Cli/Microsoft.DotNet.Cli.Utils/Extensions/LockFileFormatExtensions.cs#L25

[jgonz120]

Yea seems like it. In the LockFileCache class there's a logger that throws an error when LogError is called, something like that could be used to make that code make sense. https://github.com/dotnet/sdk/blob/302ca4c93537954c056289cde3a2c4faac0d9ce2/src/Tasks/Microsoft.NET.Build.Tasks/LockFileCache.cs#L79

[akoeplinger]

Actually I take that back, this calls public LockFile Read(string filePath) which uses File.OpenRead(filePath) before passing the stream to the other Read method so we'd catch the exception from there.

I think we can close this issue.