Remove the `Newtonsoft.Json` package and migrate to `System.Text.Json` #2125

EngRajabi · 2024-07-19T14:48:39Z

In this request, we deleted the newtonsoft package and migrated to text json system.
The reason for the changes

Newtonsoft package is no longer developed
System text json package was developed by Microsoft itself and has many changes and improvements day by day
System text json has a much better performance
It consumes much less RAM
The changes given include the removal of Newtonsoft. A benchmark has also been added for this

@MohammadAminPourmoradian helped me with this

BenchmarkDotNet v0.13.11, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3)
Intel Core i7-10870H CPU 2.20GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.303
[Host] : .NET 6.0.32 (6.0.3224.31407), X64 RyuJIT AVX2 [AttachedDebugger]
.NET 8.0 : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2

Job=.NET 8.0 Runtime=.NET 8.0

Method	Count	Mean	Error	StdDev	Median	Op/s	Gen0	Gen1	Gen2	Allocated
MicrosoftDeserializeBigData	1000	856.3 us	53.98 us	157.47 us	797.1 us	1,167.8	39.0625	13.6719	-	328.78 KB
NewtonsoftDeserializeBigData	1000	1,137.2 us	18.74 us	17.53 us	1,132.8 us	879.4	54.6875	17.5781	-	457.94 KB
MicrosoftSerializeBigData	1000	646.4 us	12.72 us	20.90 us	645.7 us	1,546.9	110.3516	110.3516	110.3516	350.02 KB
NewtonsoftSerializeBigData	1000	1,033.4 us	19.37 us	42.53 us	1,022.8 us	967.7	109.3750	109.3750	109.3750	837.82 KB

raman-m · 2024-07-22T11:16:09Z

build - Skipped
This is very strange... We need to fix the PR build

raman-m · 2024-07-22T12:41:03Z

Mohsen, if you don't mind, I will rebase the branch once again... I hope to fix the build.

raman-m

For the moment, I would appreciate it if you could eliminate all non-substantive changes such as:

End of line characters
Fixes to whitespace issues
Formatting corrections

If this cannot be done, please inform me, and I will assist you. Let's concentrate on the actual changes.

test/Ocelot.AcceptanceTests/Steps.cs

test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

raman-m · 2024-07-22T13:15:28Z

build - Success

Very well❕

test/Ocelot.IntegrationTests/ocelot.json

raman-m

Thank you for the excellent benchmark testing with the new JsonSerializerBenchmark class!

I'm concerned that a direct upgrade to the Microsoft native library might work, but it could cause issues or even breaking changes for our development consumers. Therefore, we need to seek a more advanced and gentle solution through redesign.

My suggestions are:

Maintain the old Newtonsoft.Json reference indefinitely for backward compatibility.
Implement the new System.Text.Json functionality and set it as the default, as your PR suggests.
Create a new Ocelot.Infrastructure.JsonUtil helper to establish a concrete dependency on the consumed library (requires a new Core infrastructure interface).
Introduce a new global JSON setting in the GlobalConfiguration of ocelot.json, such as JsonOptions and a Type property, to choose between using Newtonsoft.Json or System.Text.Json libraries.
Instantiate and delegate serialization operations to the actual parser instance.
Inject the new interface object into all consuming classes through the DI mechanism.

This method is more adaptable: we maintain backward compatibility while gradually transitioning to Microsoft's implementations to enhance performance.

raman-m · 2024-07-22T14:27:22Z

docs/features/dependencyinjection.rst

@@ -132,7 +132,14 @@ Current `implementation <https://github.com/search?q=repo%3AThreeMammals%2FOcelo
                .AddApplicationPart(assembly)
                .AddControllersAsServices()
                .AddAuthorization()
-                .AddNewtonsoftJson();
+                .AddJsonOptions(op =>


I'm hesitant to rewrite the documentation until all the code has been verified and approved.

I do not believe we have the authority to change the default settings of the library. Providing recommendations in the documentation to the community should have reasonable goals!
I do not think all Ocelot solutions on the Internet should adhere to these JSON options.

This change does not change the library's behavior, so changing this method does not cause any problems.

raman-m · 2024-07-22T14:33:02Z

docs/features/dependencyinjection.rst

@@ -154,74 +161,6 @@ The next section shows you an example of designing custom Ocelot pipeline by cus

 .. _di-custom-builder:

-Custom Builder


I understand that this sample might contain outdated code, yet it's essential to convert it into something usable. The sample illustrates how to override the default services in the DI feature. We can demonstrate how to revert to the old NewtonSoft services using new package versions. However, the rewriting should be deferred until all C# code is approved. Ultimately, updating the documentation should be the final step in the PR process.

raman-m · 2024-07-22T14:36:03Z

src/Ocelot.Provider.Consul/ConsulFileConfigurationRepository.cs

@@ -53,14 +54,14 @@ public async Task<Response<FileConfiguration>> Get()

        var bytes = queryResult.Response.Value;
        var json = Encoding.UTF8.GetString(bytes);
-        var consulConfig = JsonConvert.DeserializeObject<FileConfiguration>(json);
+        var consulConfig = JsonSerializer.Deserialize<FileConfiguration>(json, JsonSerializerOptionsExtensions.Web);


Well... The new JsonSerializer is from the System.Text.Json namespace.
But the package depends on Ocelot one and it consumes Ocelot interfaces.
Such hard changes constitute a major upgrade of the Ocelot Core and may introduce a breaking change, which will be detailed in the Release Notes.
I believe we need a more advanced solution.
Finally, the package should depend on Ocelot interfaces only!

src/Ocelot/DependencyInjection/OcelotBuilder.cs

src/Ocelot/Infrastructure/JsonSerializerOptionsExtensions.cs

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

src/Ocelot/Ocelot.csproj

raman-m · 2024-07-22T15:59:43Z

@EngRajabi @MohammadAminPourmoradian Mohsen and Mohammad, congratulations! The PR has entered the official code review stage. The build is green and stable now. I've made some on-the-fly code review adjustments, including superficial changes. The pull request is now well-prepared for an easy review.
Could you consider working on the suggestions from my code review plz?

@ggnaegi @RaynaldM Hello, I anticipate your thorough code review due to the significant and important upgrade in the Core involving the replacement of the JSON parsing library. I would value your consideration of my proposal to redesign the current draft solution, as I foresee potential issues.

EngRajabi · 2024-07-22T18:59:48Z

@raman-m requested changes on Jul 22, 2024

You gave a good suggestion, but is it really that much work?
Microsoft package system text json is executed on net 6 7 8. Old versions are not affected because json behavior has not changed.
It can be done by separating a serializer layer, but is it really necessary? How often does it change?

ggnaegi · 2024-07-22T22:48:08Z

@raman-m let's check this...

raman-m

`JsonPath.Net` reference must be removed❗

By including this library reference, you are breaching our Development Process for the Ocelot core package! Refer to point 5:

The main Ocelot package must not have taken on any non MS dependencies.

The significant issue with this PR is the included reference. It is essential to eliminate this reference and begin utilizing the full capabilities of native Microsoft functionality: native .NET classes or native target lib.

src/Ocelot/Infrastructure/JsonSerializerOptionsFactory.cs

raman-m · 2024-09-24T11:15:04Z

@MohammadAminPourmoradian
After the v23.3 Hotfixes release the feature branch must be rebased onto develop and all merge-conflicts must be resolved.

EngRajabi · 2024-10-03T21:06:30Z

JsonPath.Net reference must be removed❗

By including this library reference, you are breaching our Development Process for the Ocelot core package! Refer to point 5:

The main Ocelot package must not have taken on any non MS dependencies.

The significant issue with this PR is the included reference. It is essential to eliminate this reference and begin utilizing the full capabilities of native Microsoft functionality: native .NET classes or native target lib.

I replaced

raman-m · 2024-11-11T16:47:13Z

Successor to #1183

#651 Merge custom JSON properties across multiple ocelot.X.json configuration files using Newtonsoft's JToken merge functionality #1183

Congratulations, guys! 🎉 We are dependent on PR #1183, which is slated for merging first❗

../test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

../test/Ocelot.IntegrationTests/ocelot.json

../test/Ocelot.AcceptanceTests/Steps.cs

../src/Ocelot/Requester/MessageInvokerPool.cs

raman-m · 2024-12-20T11:31:33Z

Now the build is green 🟢 Very well!

@EngRajabi Please be aware that this is the final assistance I will provide. If you violate the Dev Process again, you will be banned due to multiple notifications last year.

raman-m · 2024-12-20T11:33:35Z

Ready for Review❕

@ggnaegi @RaynaldM FYI
Please note that the author did not address my suggestion to maintain backward compatibility by introducing a new interface in the Ocelot infrastructure. He expressed some concerns regarding this approach.
Please share your opinions as well!

raman-m

@ggnaegi @RaynaldM Please note that Microsoft continues to support the Microsoft.AspNetCore.Mvc.NewtonsoftJson package with the latest 9.0.0 version.

Ocelot/src/Ocelot/Ocelot.csproj

Line 50 in 9304dd2

    
           <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="9.0.0" />

However the Newtonsoft.Json package was not released .NET 7, 8, or 9, indicating that maintenance may have been discontinued. The last .NET 6 release was on March 8, 2023.
Given that Microsoft supports the Microsoft.AspNetCore.Mvc.NewtonsoftJson package, which we utilize in Ocelot, I believe we should retain the reference for backward compatibility.

raman-m · 2024-12-20T11:37:41Z

docs/features/dependencyinjection.rst

@@ -132,7 +132,14 @@ Current `implementation <https://github.com/search?q=repo%3AThreeMammals%2FOcelo
                .AddApplicationPart(assembly)
                .AddControllersAsServices()
                .AddAuthorization()
-                .AddNewtonsoftJson();
+                .AddJsonOptions(op =>


I do not believe we have the authority to change the default settings of the library. Providing recommendations in the documentation to the community should have reasonable goals!
I do not think all Ocelot solutions on the Internet should adhere to these JSON options.

raman-m · 2024-12-20T11:41:14Z

src/Ocelot/DependencyInjection/OcelotBuilder.cs

+                op.JsonSerializerOptions.NumberHandling = JsonNumberHandling.AllowReadingFromString;
+                op.JsonSerializerOptions.WriteIndented = false;
+                op.JsonSerializerOptions.PropertyNameCaseInsensitive = true;
+                op.JsonSerializerOptions.Encoder = JavaScriptEncoder.Create(UnicodeRanges.All);


Could you explain the final goal of these changes to me please?
What problem do these settings solve?

This setting prevents characters from being escaped when creating JSON. It causes problems in Arabic, Persian, etc. languages.

src/Ocelot/Infrastructure/JsonSerializerOptionsFactory.cs

raman-m · 2024-12-20T11:56:33Z

test/Ocelot.AcceptanceTests/BearerToken.cs

+    [JsonPropertyName("access_token")]
    public string AccessToken { get; set; }

-    [JsonProperty("expires_in")]
+    [JsonPropertyName("expires_in")]
    public int ExpiresIn { get; set; }

-    [JsonProperty("token_type")]
+    [JsonPropertyName("token_type")]


What is the ultimate goal of transitioning from the Newtonsoft.Json library to System.Text.Json in all tests?
I am struggling to understand it.
The old Newtonsoft helpers are already performing the testing, so why should we change the lib in testing projects?

Because I moved the entire library to System.Text.Json . It's better to do this complete migration.

raman-m · 2024-12-20T11:59:00Z

test/Ocelot.Benchmarks/Ocelot.Benchmarks.csproj

@@ -25,8 +25,11 @@
    <PackageReference Update="Microsoft.SourceLink.GitHub" Version="8.0.0" />
    <PackageReference Include="BenchmarkDotNet" Version="0.14.0" />
    <PackageReference Include="Serilog.AspNetCore" Version="9.0.0-dev-02301" />
+    <PackageReference Include="Bogus" Version="35.5.1" />


What is the Bogus library? Why did you include the reference?

This library is used to generate data for tests. Data is required for json testing.

raman-m · 2024-12-20T12:31:40Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

@@ -49,14 +50,14 @@ public Task<Response<FileConfiguration>> Get()
            jsonConfiguration = FileSys.ReadAllText(_environmentFile.FullName);
        }

-        var fileConfiguration = JsonConvert.DeserializeObject<FileConfiguration>(jsonConfiguration);
+        var fileConfiguration = JsonSerializer.Deserialize<FileConfiguration>(jsonConfiguration, JsonSerializerOptionsFactory.Web);


I am not in favor of maintaining this direct dependency. It is time to decouple this logic. As I previously suggested, we should introduce a new utility service in the infrastructure, and its interface must be injected into all constructors in Ocelot. My concern is about utilizing only one utility, whereas the community desires more libs.

Separation is good but it has its problems.
For example, you separate the Json lib but use JsonPropertyName inside your classes. This makes separation meaningless.

EngRajabi mentioned this pull request Jul 19, 2024

Remove the Newtonsoft.Json package and migrate to System.Text.Json #2124

Closed

raman-m added the proposal Proposal for a new functionality in Ocelot label Jul 22, 2024

raman-m reviewed Jul 22, 2024

View reviewed changes

test/Ocelot.AcceptanceTests/Steps.cs Outdated Show resolved Hide resolved

test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs Outdated Show resolved Hide resolved

raman-m added Core Ocelot Core related or system upgrade (not a public feature) Configuration Ocelot feature: Configuration Dependency Injection Ocelot feature: Dependency Injection labels Jul 22, 2024

raman-m reviewed Jul 22, 2024

View reviewed changes

test/Ocelot.IntegrationTests/ocelot.json Outdated Show resolved Hide resolved

raman-m assigned EngRajabi and raman-m Jul 22, 2024

raman-m requested changes Jul 22, 2024

View reviewed changes

raman-m requested review from RaynaldM and ggnaegi July 22, 2024 15:29

MohammadAminPourmoradian force-pushed the feat_rm_newtonsoft branch from 61baceb to 93bea24 Compare July 28, 2024 09:59

RaynaldM approved these changes Sep 23, 2024

View reviewed changes

raman-m requested changes Sep 24, 2024

View reviewed changes

src/Ocelot/Infrastructure/JsonSerializerOptionsFactory.cs Outdated Show resolved Hide resolved

EngRajabi requested review from RaynaldM and raman-m October 3, 2024 21:12

raman-m added the conflicts Feature branch has merge conflicts label Nov 2, 2024

raman-m force-pushed the feat_rm_newtonsoft branch from fdca813 to 9ae0cc4 Compare November 2, 2024 14:40

raman-m removed the conflicts Feature branch has merge conflicts label Nov 2, 2024

raman-m mentioned this pull request Nov 11, 2024

#651 Merge custom JSON properties across multiple ocelot.X.json configuration files using Newtonsoft's JToken merge functionality #1183

Open

EngRajabi and others added 23 commits December 20, 2024 13:38

feat: add system text json config

635439b

fix: AcceptanceTest ConsulServiceDiscovery

4d14ebb

fix: fix indent

646b81c

feat: change un use

fe5b2fd

feat: add json benchmark

d5c23c0

feat: add json serialize result json

c7a641c

fix

61247cc

update doc

0048df8

Revert file MessageInvokerPoolTests.cs

c30672f

../test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

Revert file ocelot.json

6215382

../test/Ocelot.IntegrationTests/ocelot.json

Revert file Steps.cs

dfbc87f

../test/Ocelot.AcceptanceTests/Steps.cs

Apply changes in file Steps.cs

12f1b21

../test/Ocelot.AcceptanceTests/Steps.cs

Revert file MessageInvokerPool.cs

8667047

../src/Ocelot/Requester/MessageInvokerPool.cs

Code review by @raman-m

ac74d64

fix: rename and format JsonSerializerOptions class

c6897c9

fix: remove unused JsonDocument object

147271c

add

b65a6d6

fix: rm JsonPath.Net

0fda6af

fix: add more test

4863b2e

Fix errors and warnings

dea5e37

Fix Ocelot.Benchmarks build

885ee5b

fix: fix something

bd2da05

Fix build errors, warnings, messages after rebasing

6b67250

raman-m force-pushed the feat_rm_newtonsoft branch from 20e5411 to 6b67250 Compare December 20, 2024 11:20

raman-m removed the conflicts Feature branch has merge conflicts label Dec 20, 2024

raman-m requested changes Dec 20, 2024

View reviewed changes

EngRajabi added 2 commits December 21, 2024 08:21

fix: rm newstonsoft

f59ca5e

fix: rename JsonSerializerOptionsFactory to OcelotSerializerOptions

2a213d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the `Newtonsoft.Json` package and migrate to `System.Text.Json` #2125

Remove the `Newtonsoft.Json` package and migrate to `System.Text.Json` #2125

EngRajabi commented Jul 19, 2024

raman-m commented Jul 22, 2024 •

edited

Loading

raman-m commented Jul 22, 2024 •

edited

Loading

raman-m left a comment

raman-m commented Jul 22, 2024

raman-m left a comment

raman-m Jul 22, 2024

raman-m Dec 20, 2024

EngRajabi Dec 21, 2024

raman-m Jul 22, 2024

raman-m Jul 22, 2024 •

edited

Loading

raman-m commented Jul 22, 2024

EngRajabi commented Jul 22, 2024 •

edited by raman-m

Loading

ggnaegi commented Jul 22, 2024

raman-m left a comment

raman-m commented Sep 24, 2024

EngRajabi commented Oct 3, 2024

`JsonPath.Net` reference must be removed❗

raman-m commented Nov 11, 2024

raman-m commented Dec 20, 2024

raman-m commented Dec 20, 2024 •

edited

Loading

raman-m left a comment

raman-m Dec 20, 2024

raman-m Dec 20, 2024

EngRajabi Dec 21, 2024

raman-m Dec 20, 2024

EngRajabi Dec 21, 2024

raman-m Dec 20, 2024

EngRajabi Dec 21, 2024

raman-m Dec 20, 2024

EngRajabi Dec 21, 2024

		@@ -154,74 +161,6 @@ The next section shows you an example of designing custom Ocelot pipeline by cus

		.. _di-custom-builder:

		Custom Builder

Remove the Newtonsoft.Json package and migrate to System.Text.Json #2125

Are you sure you want to change the base?

Remove the Newtonsoft.Json package and migrate to System.Text.Json #2125

Conversation

EngRajabi commented Jul 19, 2024

raman-m commented Jul 22, 2024 • edited Loading

raman-m commented Jul 22, 2024 • edited Loading

raman-m left a comment

Choose a reason for hiding this comment

raman-m commented Jul 22, 2024

raman-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m commented Jul 22, 2024

EngRajabi commented Jul 22, 2024 • edited by raman-m Loading

ggnaegi commented Jul 22, 2024

raman-m left a comment

Choose a reason for hiding this comment

JsonPath.Net reference must be removed❗

raman-m commented Sep 24, 2024

EngRajabi commented Oct 3, 2024

JsonPath.Net reference must be removed❗

raman-m commented Nov 11, 2024

Successor to #1183

raman-m commented Dec 20, 2024

raman-m commented Dec 20, 2024 • edited Loading

Ready for Review❕

raman-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove the `Newtonsoft.Json` package and migrate to `System.Text.Json` #2125

Remove the `Newtonsoft.Json` package and migrate to `System.Text.Json` #2125

raman-m commented Jul 22, 2024 •

edited

Loading

raman-m commented Jul 22, 2024 •

edited

Loading

raman-m Jul 22, 2024 •

edited

Loading

EngRajabi commented Jul 22, 2024 •

edited by raman-m

Loading

`JsonPath.Net` reference must be removed❗

`JsonPath.Net` reference must be removed❗

raman-m commented Dec 20, 2024 •

edited

Loading