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.

docs/features/dependencyinjection.rst

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

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

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

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

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

Bump System.Text.Encodings.Web to 8.0.0

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❗

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

EngRajabi and others added 4 commits November 2, 2024 16:52

feat: rm newtonsoft

6f7cf07

feat: add system text json config

43410a6

fix: AcceptanceTest ConsulServiceDiscovery

705018b

fix: fix indent

9d5b664

EngRajabi and others added 18 commits November 2, 2024 16:54

feat: change un use

bd0a7c5

feat: add json benchmark

90928b9

feat: add json serialize result json

6efb018

fix

57ce8e7

update doc

50787cb

Revert file MessageInvokerPoolTests.cs

da04d60

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

Revert file ocelot.json

ffe0c51

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

Revert file Steps.cs

709917b

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

Apply changes in file Steps.cs

1a0025a

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

Revert file MessageInvokerPool.cs

a7d202a

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

Code review by @raman-m

c719a8b

fix: rename and format JsonSerializerOptions class

63fddf0

fix: remove unused JsonDocument object

a83a1bf

add

6d507e7

fix: rm JsonPath.Net

b31a548

fix: add more test

e6ca2b3

Fix errors and warnings

aae932c

Bump System.Text.Json to 8.0.5

9ae0cc4

Bump System.Text.Encodings.Web to 8.0.0

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

Fix Ocelot.Benchmarks build

e7d7b2d

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

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

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 •

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

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

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

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❗