Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Newtonsoft.Json with System.Text.Json #21

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

webwarrior-ws
Copy link
Contributor

Replace Newtonsoft.Json with System.Text.Json so that we don't
have different libraries for JSON, because System.Text.Json
will be used by GRPC services.

Also use FSharp.SystemTextJson library so that all F# types,
including discriminated unions, can be serialized using
System.Text.Json.

@knocte
Copy link
Member

knocte commented Feb 12, 2024

@webwarrior-ws I didn't tell you to add FSharp.SystemTextJson, I told you to play with it and, if it works, add a TODO. Because I didn't want this dependency in this PR! Please put the converters back. And save these commits for a future PR.

@webwarrior-ws
Copy link
Contributor Author

@webwarrior-ws I didn't tell you to add FSharp.SystemTextJson, I told you to play with it and, if it works, add a TODO. Because I didn't want this dependency in this PR! Please put the converters back. And save these commits for a future PR.

But converters didn't work. Or they were not enough. With FSharp.SystemTextJson, it works.

@knocte
Copy link
Member

knocte commented Feb 12, 2024

Isn't there a way of doing what you did in 5845515 but with System.Text.Json? I'm sure there is.

@webwarrior-ws
Copy link
Contributor Author

Isn't there a way of doing what you did in 5845515 but with System.Text.Json? I'm sure there is.

But it was another bug. Failed condition was

Assert.That(value.HasValue, Is.EqualTo(true),
, but with decimal it is
Assert.That(order.ToString(),

@knocte
Copy link
Member

knocte commented Feb 12, 2024

Then in the same way you have splitted the decimal thing in 2, you have to separate the 1st commit in 2 where you add FSharpSystemJson in the 2nd commit and explain the fix.

Replace Newtonsoft.Json with System.Text.Json so that we don't
have different libraries for JSON, because System.Text.Json
will be used by GRPC services.
@knocte
Copy link
Member

knocte commented Feb 12, 2024

But it was another bug. Failed condition was

Assert.That(value.HasValue, Is.EqualTo(true),
, but with decimal it is
Assert.That(order.ToString(),

You didn't explain the above in the 2nd commit msg.

@webwarrior-ws
Copy link
Contributor Author

@knocte expanded message of second commit with explanation of what didn't work with just System.Text.Json

@knocte
Copy link
Member

knocte commented Feb 14, 2024

indicating that something is wrong with serizlization.

Something is wrong with serialization? I thought you had looked into what is wrong? Then explain it. Also typo 'serizlization' -> 'serialization'.

// which may cause problems when comparing serialized values.
// To avoid this, add 0.0m to value before writing, so all decimals will be serilized with fractional
// part, even if it's 0.
// For more info, see https://colinmackay.scot/2021/07/03/why-is-there-a-difference-between-decimal-0-and-0-0/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webwarrior-ws great comment and great investigation, however, let's improve some things here:

  1. Instead of writing a converter, it's better to just make the tests be more liberal, and accept that x.0 is the same as x; let's write our own Assert.DecimalEquals function for example. We switched to FSharp.SystemTextJson to avoid converters!
  2. This comment should go in the commit message as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If test was comparing decimals, there would be no problem. But test compares strings (serialized json).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests are just making sure that Redis works; you could deserialize the JSON and make sure the ending object is the same (since they are records, an Equals invokation should work)

@webwarrior-ws
Copy link
Contributor Author

indicating that something is wrong with serizlization.

Something is wrong with serialization? I thought you had looked into what is wrong? Then explain it. Also typo 'serizlization' -> 'serialization'.

I haven't found what exactly is wrong.

@webwarrior-ws
Copy link
Contributor Author

Fixed typo in second commit's message.

@knocte
Copy link
Member

knocte commented Feb 14, 2024

I haven't found what exactly is wrong.

Let's investigate then, I want clear reasons for migrating to FSharp.SystemTextJson.

Use System.Text.Json instead of Newtonsoft.Json in integration
tests.
@webwarrior-ws
Copy link
Contributor Author

I haven't found what exactly is wrong.

Let's investigate then, I want clear reasons for migrating to FSharp.SystemTextJson.

It turns out that tests were using Newtonsoft.Json. I don't know how that project compiled without references to Newtonsoft.Json in any project files or Directory.Packages.props.

@webwarrior-ws
Copy link
Contributor Author

With that fixed, we don't need custom converter for decimal.

Implement JSON converters for F# Discriminated Union types
because System.Text.Json can't serialize them out of the box.
Without converters, got the following error:
```
System.NotSupportedException : F# discriminated union serialization is not supported. Consider authoring a custom converter for the type.
The unsupported member type is located on type 'FsharpExchangeDotNetStandard.Currency'. Path: $.Market.BuyCurrency.
```
@knocte knocte merged commit df90233 into nblockchain:master Feb 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants