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

Json serialization problem on System.Text.Json #3190

Closed
VAllens opened this issue Feb 21, 2022 · 8 comments
Closed

Json serialization problem on System.Text.Json #3190

VAllens opened this issue Feb 21, 2022 · 8 comments

Comments

@VAllens
Copy link

VAllens commented Feb 21, 2022

System.Text.Json does not use the System.Runtime.Serialization.IgnoreDataMember attribute, it uses the System.Text.Json.Serialization.JsonIgnore attribute defined by itself.
Therefore, Changing and Changed and ThrownExceptions are serialized.

I have two suggestions:

  1. mark these three properties as virtual to allow subclasses to override them.
  2. in the ReactiveObject and ReactiveRecord classes, add the JsonIgnore attribute to these three properties, but it needs to reference the System.Text.Json package.

FYI. :)

@VAllens
Copy link
Author

VAllens commented Feb 21, 2022

I personally recommend the first option, which gives more flexibility while reducing the possibility of other dependencies.

@HugoRoss
Copy link

A quick check shows that there are other usages of the IgnoreDataMember in the repo:

  • ReactiveUI\Platforms\android\ReactiveViewHost.cs
  • ReactiveUI\ReactiveObject\ReactiveObject.cs
  • ReactiveUI\ReactiveObject\ReactiveRecord.cs
  • ReactiveUI\Routing\RoutingState.cs
  • ReactiveUI.AndroidSupport\ReactiveRecyclerViewViewHolder.cs
  • ReactiveUI.AndroidX\ReactiveRecyclerViewViewHolder.cs
  • ReactiveUI.Tests\ReactiveObject\Mocks\OaphNameOfTestFixture.cs
  • ReactiveUI.Tests\ReactiveObject\Mocks\OaphTestFixture.cs
  • ReactiveUI.Tests\ReactiveObject\Mocks\TestFixture.cs
  • ReactiveUI.Tests\ReactiveObject\Mocks\WhenAnyTestFixture.cs

Whether these are all candidates for Json serialization with System.Text.Json I can't say.

@VAllens
Copy link
Author

VAllens commented Mar 24, 2022

Does anyone have any suggestions?

@xecrets
Copy link

xecrets commented Jun 5, 2023

Seeing as System.Text.Json appears to be the .NET way forward, which also supports source generation for reflection-free serialization, and thus assembly trimming etc, adding the reference to System.Text.Json seems fairly non-controversial.

As a work around until this is fixed in a better way, one way or another, the following hack can work for the special case of serializing a ReactiveObject (beware, it's a hack, there might be some edge cases not handled, but it does handle dangling commas). Also, in real code, use the [GeneratedRegex] attribute for performance.

    private static string CleanUpSerializedReactiveObjectHack(string json)
    {
        json = Regex.Replace(json, @"\s*""(Changing|Changed|ThrownExceptions)"":\s*\{},?\n?", string.Empty);
        json = Regex.Replace(json, @",(\s*})", "$1");
        return json;
    }

@glennawatson
Copy link
Contributor

Yeah probably time we bite the bullet and swap to using full on system.text.json.

glennawatson pushed a commit that referenced this issue Nov 18, 2023
<!-- Please be sure to read the
[Contribute](https://github.com/reactiveui/reactiveui#contribute)
section of the README -->

**What kind of change does this PR introduce?**
<!-- Bug fix, feature, docs update, ... -->

Feature #3190 

**What is the current behaviour?**
<!-- You can also link to an open issue here. -->

No support for System.Text.Json

**What is the new behaviour?**
<!-- If this is a feature change -->

System.Text.Json support for Serialization added

https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-7-0

**What might this PR break?**

If other Serializers are used verification of correct serialisation
should be confirmed

**Please check if the PR fulfills these requirements**
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)

**Other information**:
@ChrisPulman
Copy link
Member

We have added support for System.Text.Json, please raise a new issue with any new findings, thank you

@VAllens
Copy link
Author

VAllens commented Jan 6, 2024

goooooooooooooooooooooooooood !!!

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants