Switch from Newtonsoft.Json to System.Text.Json#3932
Switch from Newtonsoft.Json to System.Text.Json#3932Morilli wants to merge 8 commits intoTASEmulators:masterfrom
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.
Uhh? Currently every property is de/serialised.
There was a problem hiding this comment.
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),
YoshiRulz
left a comment
There was a problem hiding this comment.
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
c2f7ff6 to
ad4ad58
Compare
ad4ad58 to
5d3dd04
Compare
I guess tests are actually useful?
5d3dd04 to
4378ebb
Compare
This switches the entire codebase from using
Newtonsoft.JsontoSystem.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.EmuHawkassembly name change.Polymorphism and deserializing based on a given
$typein 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$typecaused the assembly name mismatches, this should also fix all config errors.There's a couple caveats and potential failure cases here:
Newtonsoft.JsonandSystem.Text.Jsonare 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.Jsondoes 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.jsonserializes 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 fullJsonConverterfor 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 aJsonConverterfor 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.Jsonuses a method that guarantees enough precision, but often overshoots. I've implemented a simpleFloatConverterto 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: