-
Notifications
You must be signed in to change notification settings - Fork 384
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
should be fixed with the latest commit |
This comment was marked as resolved.
This comment was marked as resolved.
^ 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. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
// [JsonConstructor] | ||
private RollColumn() | ||
[JsonConstructor] | ||
private RollColumn(string name, string text, ColumnType type) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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),
There was a problem hiding this 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.
65cedcb
to
1d673ef
Compare
1d673ef
to
52d9cf2
Compare
52d9cf2
to
c2f7ff6
Compare
I guess tests are actually useful?
c2f7ff6
to
ad4ad58
Compare
This switches the entire codebase from using
Newtonsoft.Json
toSystem.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 theEmuHawk
->BizHawk.Client.EmuHawk
assembly name change.Polymorphism and deserializing based on a given
$type
in the json is not really supported inSystem.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
andSystem.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.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.IEnumerable<T>
will be serialized asIEnumerable<T>
, ignoring all class field and properties entirely. There is no straightforward way to change this, the suggested workaround is to implement a fullJsonConverter
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.System.Text.Json
, so I had to write aJsonConverter
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).System.Text.Json
uses a method that guarantees enough precision, but often overshoots. I've implemented a simpleFloatConverter
to handle this case nicely, but doubles may have the same issue.[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 / doublesdoneCheck if completed: