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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Morilli
Copy link
Collaborator

@Morilli Morilli commented May 31, 2024

This switches the entire codebase from using Newtonsoft.Json to System.Text.Json. This is probably more future-proof, supported by .NET directly and uhh see #2261 I guess. It would be nice to be able to load older configs with less ugly exception boxes that delete your entire config if you don't read it carefully enough, and currently all 2.9.1 configs and .tasprojs will fail to load due to the EmuHawk->BizHawk.Client.EmuHawk assembly name change.
Polymorphism and deserializing based on a given $type in the json is not really supported in System.Text.Json, but this is by design and for security reasons. I've changed the code to work without this parameter and because this $type caused the assembly name mismatches, this should also fix all config errors.

There's a couple caveats and potential failure cases here:

  • Newtonsoft.Json and System.Text.Json are naturally not 100% interchangable; a couple important differences are listed here: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft.
  • System.Text.Json does not include non-public fields and properties of objects in serialization, and that behavior can only be changed by applying the [JsonInclude] attribute to each non-public field or attribute, there is no serializer option for it.
  • As in Newtonsoft.Json, System.Text.json serializes byte arrays as base64-encoded strings, which means we still need to code our own converter to get them serialized as number arrays.
  • There are hardcoded conventions for how certain interface types are handled, for example a class implementing IEnumerable<T> will be serialized as IEnumerable<T>, ignoring all class field and properties entirely. There is no straightforward way to change this, the suggested workaround is to implement a full JsonConverter for each affected class and decorate the class with it. This is... unfortunate to say the least, see Developers should have access to System.Text.Json's default internal converters dotnet/runtime#63791 and related issues for discussion around it. Given the issues age, this is most likely not going to be fixed soon.
  • Multi-dimensional arrays seem to not be serializable by default in System.Text.Json, so I had to write a JsonConverter for them. Additional headache when you have a multi-dimensional byte array: You cannot decorate the field with both the multidimensional array converter and the byte array converter, so either the multidimensional array converter needs to hardcode its serialization behavior and choose the number array or base64 string representation, or the serializer for the parent class needs to be passed the byte array converter if desired (this is currently the case).
  • Float serialization is a bit garbage on .net framework; passing certain float values can result in too much precision in the string representation. Apparently .net framework's default float/double ToString behavior is inaccurate and does not guarantee round-trip safety, so to counter that System.Text.Json uses a method that guarantees enough precision, but often overshoots. I've implemented a simple FloatConverter to handle this case nicely, but doubles may have the same issue.
  • Serialization order of elements could be different, like with private fields manually [JsonInclude]d.

Now, having considered and handled most of the above points, some of those issues are hard to spot and/or invisible until an existing type is serialized and exhibits unexpected behavior or serialization failure.

TODO:

  • create more test cases to test the added converters, like multi-dimensional arrays, byte array serialization, round-trip serialization for floats / doubles
  • check all existing serialized classes to ensure they are all serialized the same as before and there are no mismatches in representation, missing fields, extra fields, wrong field names, etc.

Note that there are most likely some fatal bugs still in here, this is mainly a proof-of-concept at the moment.

Check if completed:

@kalimag

This comment was marked as resolved.

@Morilli
Copy link
Collaborator Author

Morilli commented Jun 1, 2024

Tool settings can't be deserialized yet

System.ArgumentException
  HResult=0x80070057
  Message=Object of type 'System.Text.Json.JsonElement' cannot be converted to type 'BizHawk.Client.EmuHawk.LuaConsole+LuaConsoleSettings'.
  Source=mscorlib
  StackTrace:
   at System.Reflection.RuntimePropertyInfo.SetValue(Object obj, Object value, Object[] index) in f:\dd\ndp\clr\src\BCL\system\reflection\propertyinfo.cs:line 643
   at BizHawk.Client.EmuHawk.ToolManager.InstallCustomConfig(IToolForm tool, Dictionary`2 data) in H:\Projekte\Visual Studio\BizHawk-upstream\src\BizHawk.Client.EmuHawk\tools\ToolManager.cs:line 422
   at BizHawk.Client.EmuHawk.ToolManager.Load[T](Boolean focus, String toolPath) in H:\Projekte\Visual Studio\BizHawk-upstream\src\BizHawk.Client.EmuHawk\tools\ToolManager.cs:line 145
   at BizHawk.Client.EmuHawk.MainForm.OpenLuaConsole() in H:\Projekte\Visual Studio\BizHawk-upstream\src\BizHawk.Client.EmuHawk\MainForm.cs:line 1543
   at BizHawk.Client.EmuHawk.MainForm.LuaConsoleMenuItem_Click(Object sender, EventArgs e) in H:\Projekte\Visual Studio\BizHawk-upstream\src\BizHawk.Client.EmuHawk\MainForm.Events.cs:line 1192

should be fixed with the latest commit

@kalimag

This comment was marked as resolved.

@Morilli
Copy link
Collaborator Author

Morilli commented Jun 1, 2024

^ That first error should be fixed with 16a8e92, and the second is due to an incorrect config generated from a commit without the TypeConverter handling, so you'll need to clear that part of the config and let it regenerate.

@kalimag

This comment was marked as resolved.

@Morilli

This comment was marked as resolved.

@kalimag

This comment was marked as resolved.

// [JsonConstructor]
private RollColumn()
[JsonConstructor]
private RollColumn(string name, string text, ColumnType type)
Copy link
Member

Choose a reason for hiding this comment

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

Uhh? Currently every property is de/serialised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The setters are private, so it's either this or adding [JsonInclude] to every property that has a private setter (while still keeping a dummy private constructor),

Copy link
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

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

I probably shouldn't be spamming individual comments.

But I'll leave this with you for now. I've seen a few changes which could easily be split off and pushed to master, so consider doing that and rebasing.

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.

None yet

3 participants