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

Switch from Newtonsoft.Json to System.Text.Json #32

Merged

Conversation

RenanCarlosPereira
Copy link
Collaborator

Why the Change?

  1. Performance: System.Text.Json is faster and uses less memory.
  2. Built-in: It's part of .NET Core and .NET 5+, so fewer external dependencies.
  3. Security: Better default security settings.
  4. Future-proof: Regular updates from the .NET team.

What’s Different?

  • Serialization:

    • Before: JsonConvert.SerializeObject(myObject)
    • After: JsonSerializer.Serialize(myObject)
  • Deserialization:

    • Before: JsonConvert.DeserializeObject<MyClass>(jsonString)
    • After: JsonSerializer.Deserialize<MyClass>(jsonString)
  • Enum Conversion:

    • Before: [JsonConverter(typeof(StringEnumConverter))]
    • After: [JsonConverter(typeof(JsonStringEnumConverter))]

Other Details

  • Code Coverage, CodeQL, and Unit Tests: All passing.

Things to Watch Out For

  • Feature Differences: Some advanced Newtonsoft.Json features might not have direct equivalents.
  • Configuration: Default behaviors might differ (e.g., property naming). Adjust JsonSerializerOptions as needed.

@RenanCarlosPereira
Copy link
Collaborator Author

@abbasc52, could please also take a look at this PR if not asking too much?

I don't think changing from Newtonsoft to System.Text.Json will break anything, any thoughts?

@asulwer
Copy link
Owner

asulwer commented Jul 1, 2024

i referenced the reason why this shouldn't occur with this issue

Table of differences

@abbasc52
Copy link

abbasc52 commented Jul 1, 2024

Json parsing is only relevent in 2 places, when user passes workflow as string and secondly in Action context parsing.(I do remember making Action context System.Text.Json compatible)

One major difference is System.Text.Json does not guess type while parsing without a type(by design) .This can lead to some corner cases which might need validation.
I will go through the code and report if I see something obvious but old repo had a few asks to migrate and I probably provided reasons.

One way to go forward would be to uncouple json parser as separate extension packages.

Or newtonsoft and other changes can be part of a compatibility nuget, so that main project remains clean and people can migrate as per their convenience.

@RenanCarlosPereira
Copy link
Collaborator Author

RenanCarlosPereira commented Jul 1, 2024

Nice input @abbasc52

I can cover in unit tests the type s converts

Any ideas on how I can test it?

I don't see any issues moving to System.Text.Json
in trying to think in a situation where I can cover in ActionContext

value = JsonConvert.SerializeObject(kv.Value);

Copy link
Owner

@asulwer asulwer left a comment

Choose a reason for hiding this comment

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

thank you to @abbasc52 for you input with this change.

@asulwer asulwer merged commit f9aa7c6 into asulwer:main Jul 1, 2024
3 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.

3 participants