Replace Newtonsoft.Json with System.Text.Json

[IanKemp]

(This was discussed in #2316 but deserves to be its own issue, as the work necessary to close this issue would be different to the work to close that one.)

I'm tired of typing JsonSerializer in my test projects and getting it autocompleted to Newtonsoft.Json.JsonSerializer because Microsoft.Net.Test.Sdk (via Microsoft.TestPlatform.TestHost) references (a very old version) of the now-essentially-deprecated Newtonsoft.Json, especially when my greenfields .NET Core app under test has zero references to Newtonsoft.Json.

This would also help people who do have explicit dependencies on Newtonsoft.Json and are being bitten by #2279.

[nohwnd]

We would love to do this, but we need to keep our dependencies compatible with net452 and System.Text.Json is not compatible with that.

[0xced]

It's actually much worse, the dependency on Newtonsoft.Json is not properly declared in the NuGet package.

I was trying to replace Newtonsoft.Json with System.Text.Json in Stryker.NET and the dependency on Newtonsoft.Json hit me hard. After some work to replace all JSON handling everywhere in Stryker.NET I was finally able to remove <PackageReference Include="Newtonsoft.Json" Version="13.0.1" /> from its csproj. But thanks to JetBrains Rider great feature where you can see transitive packages (Implicitly Installed Packages in Solution), I saw that Newtonsoft.Json was still referenced.

It turns out it was referenced through Buildalyzer/3.2.2 → Microsoft.Extensions.DependencyModel/2.1.0 → Newtonsoft.Json/9.0.1. So in order to completely get rid of Newtonsoft.Json I explicitly updated Microsoft.Extensions.DependencyModel to version 5.0.0 which has zero dependencies for the net5.0 target framework. I added <PackageReference Include="Microsoft.Extensions.DependencyModel" Version="5.0.0" /> to Stryker.Core.csproj and Newtonsoft.Json was definitely gone.

But then… this happened at runtime:

System.IO.FileNotFoundException: Could not load file or assembly 'Newtonsoft.Json, Version=9.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed'. The system cannot find the file specified.

File name: 'Newtonsoft.Json, Version=9.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed'
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.JsonDataSerializer..ctor()
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.JsonDataSerializer.get_Instance()
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.SocketCommunicationManager..ctor()
   at Microsoft.TestPlatform.VsTestConsole.TranslationLayer.VsTestConsoleRequestSender..ctor()
   at Microsoft.TestPlatform.VsTestConsole.TranslationLayer.VsTestConsoleWrapper..ctor(String vstestConsolePath, ConsoleParameters consoleParameters)
   at Stryker.Core.TestRunners.VsTest.VsTestRunner.PrepareVsTestConsole()

This is because Microsoft.TestPlatform.TranslationLayer is lying about its dependencies. Its JsonDataSerializer class depends on Newtonsoft.Json on .NET Standard 2.0 but Newtonsoft.Json is not declared as a dependency.

Q.E.D.

[SimonCropp]

almost 2 years later. perhaps it is time to drop support for net452? support is ending in 2 months anyway

[michael-hawker]

I was just doing some refactoring of the base file new UWP Unit Test project to coordinate the multiple copies we have across our project to a single set of code for our custom test harness.

Doing this suddenly broke the UWP app from connecting to the vstest.console or VS runner. Took me a day, but finally saw a FileNotFoundException about Newtonsoft.Json followed by a SocketException.

Added a reference to Newtonsoft.Json directly to the UWP Unit Test csproj file and everything went back to normal... 🙁

Not sure exactly why what I did broke the dependency look up, but it seems we're using the SdkReference for TestPlatform.Universal, so I'm not exactly sure how that works. (Compared to our WinAppSDK head which continued to work fine after our refactor, but it's referencing Microsoft.TestPlatform.TestHost directly and that at least declares the old Newtonsoft dependency.

[nohwnd]

#2316 (comment) we are still researching how to get rid of that dependency altogether, unfortunately System.Text.Json does not support the data annotations we use, and we don't want to add additional dependency to object model for .NET Framework.

#3622

[SimonCropp]

@nohwnd can u point to where/how u use data annotations?

[nohwnd]

@SimonCropp Sure, for example here in ObjectModel which has no dependency on the serializer we have the TestCase object, and it is annotated with DataMember attribute.

https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.ObjectModel/TestCase.cs#L69

Theoretically we could work around this on the serializer / deserializer side by explicitly listing the properties to use for every object.

Another issue with System.Text.Json is that it is not built-in in .NET Framework, and serializers are missing for common types that we use.

[michael-hawker]

Another issue with System.Text.Json is that it is not built-in in .NET Framework, and serializers are missing for common types that we use.

System.Text.Json is available as a NuGet package which multi-targets across .NET 6/7, .NET Standard, and .NET Framework 4.6.2, so that shouldn't be a problem?

[nohwnd]

@michael-hawker One of the reasons is that we get complaints about newtonsoft.json being required to test code that is not relying on it. We keep using very old version of the library to avoid upgrading the tested code that needs it. But if the tested code does not need it, we would ideally not bring it in. Or any other dependency that is not present in the targetted runtime.

[gregsdennis]

Any progress on this?

[hadrienbecle]

Wouldn't using PrivateAssets="all" on the dependency help at least a bit for the autocompletion problems? Some users would need to explicitly reference Newtonsoft when upgrading, but that doesn't seem much compared to the benefits.

[SimonCropp]

perhaps u could alias newtonsoft https://github.com/getsentry/dotnet-assembly-alias/ and use the internal option
to avoid conflicts for consuming libs

[rcdailey]

Wouldn't using PrivateAssets="all" on the dependency help at least a bit for the autocompletion problems? Some users would need to explicitly reference Newtonsoft when upgrading, but that doesn't seem much compared to the benefits.

PrivateAssets only impacts whether or not the DLL gets copied to the output directory. It won't impact code-completion or the ability to reference symbols from the Newtonsoft namespace. There's an open issue here that requests a way to disable/remove implicit dependencies at the package-level. I recommend folks go there to upvote that issue. Which ever issue gets addressed first (that one or this one) is the one I will use!

[silkfire]

Any status update?

[nohwnd]

There is no update, the reasons in #2488 (comment) are still true, and why we don't work on replacing newtonsoft.json.

[AndersMalmgren]

Data annotation is a bad pattern because the tight coupling. can be solved in other ways, like fluent mapping.

If you want to keep supporting newtonsoft for full framework you could use compile flags that let you compile newtonsoft specific code when building for .NET Fullframework 4.8

[nohwnd]

Data annotation is a bad pattern because the tight coupling.

Can you be more specific why this is a bad practice? From what I see in our code we use a in-built attribute that is general purpose for data annotation, and newtonsoft.json is picking that up. So to me that is the opposite of coupling.

While if we use fluent mapping we either need to let a "serializer" library know about all the shapes of all our messages. Or we need to add references to serializer to the dll that has our messages.

you could use compile flags that let you compile newtonsoft specific code

Yes that is a way forward, but at that point we can also just move to STJ fully and maintain it in a single way. But we don't because the newtonsoft signature is part of our public api and we don't have good opportunity to break when it works.

[AndersMalmgren]

Data annotation is a bad pattern because the tight coupling.

Can you be more specific why this is a bad practice? From what I see in our code we use a in-built attribute that is general purpose for data annotation, and newtonsoft.json is picking that up. So to me that is the opposite of coupling.

While if we use fluent mapping we either need to let a "serializer" library know about all the shapes of all our messages. Or we need to add references to serializer to the dll that has our messages.

you could use compile flags that let you compile newtonsoft specific code

Yes that is a way forward, but at that point we can also just move to STJ fully and maintain it in a single way. But we don't because the newtonsoft signature is part of our public api and we don't have good opportunity to break when it works.

Datannotation are bad practice because it favours high coupling.
Problem with newtonsoft is that if you have a unit test project in your SLN and a less careful developer in the team uses resharper to auto reference newtonsoft dependency from mstest instead of correctly using System.Text.Json. If newtonsoft is not present in SLN resharper will not suggest using it but instead reference System.Text.Json. Right now Im seriously thinking of moving to nunit just to get rid of newtonsoft.

[SimonCropp]

@AndersMalmgren

Right now Im seriously thinking of moving to nunit just to get rid of newtonsoft.

that wont help. nunit still requires Microsoft.NET.Test.Sdk to work. and that refs Microsoft.TestPlatform.TestHost which refs Newtonsoft.Json

[IanKemp]

Data annotation is a bad pattern because the tight coupling.

Can you be more specific why this is a bad practice? From what I see in our code we use a in-built attribute that is general purpose for data annotation, and newtonsoft.json is picking that up. So to me that is the opposite of coupling.
While if we use fluent mapping we either need to let a "serializer" library know about all the shapes of all our messages. Or we need to add references to serializer to the dll that has our messages.

you could use compile flags that let you compile newtonsoft specific code

Yes that is a way forward, but at that point we can also just move to STJ fully and maintain it in a single way. But we don't because the newtonsoft signature is part of our public api and we don't have good opportunity to break when it works.

Datannotation are bad practice because it favours high coupling. Problem with newtonsoft is that if you have a unit test project in your SLN and a less careful developer in the team uses resharper to auto reference newtonsoft dependency from mstest instead of correctly using System.Text.Json. If newtonsoft is not present in SLN resharper will not suggest using it but instead reference System.Text.Json.

This is what code reviews are for.

[rcdailey]

How is the debate about attributes & tight coupling helping to move us closer to a solution? If it's not, we should limit discussion to on-topic updates. I'd prefer to remain subscribed to replies so that I can keep up with meaningful conversation on the topic. Asking for status updates and debating attributes are not what I consider meaningful.

The issue is in open status and repository maintainers are aware of it. Let's patiently wait on a solution, or if you're impatient, I'm sure they would appreciate a pull request.

[ranma42]

Would an approach based on STJ contract customization be considered acceptable? (see dotnet/runtime#29975 (comment) )
(aka does it make sense for me to try and work on a PR in that direction?)

[nohwnd]

I know this will be unpopular but we would have to break the public API to do this, and we still would have to ship STJ for .NET Framework, so this would require significant changes to vstest. For this reasion it won't be implemented, we are focusing on adding new features to Testing.Platform instead, where we avoided this problem since the beginning. https://aka.ms/testingplatform