Proof-of-concept serialization of advanced JSON types, records

[onionhammer]

Description

Today, complex objects being passed to/from dapr actors must be serializable with DataContractSerializer, which means shotgunning "DataMember" on any primary-constructors (records and C# 8 classes), and difficult workarounds types such as JsonElement.

This PR is a proof-of-concept, as there are areas where this implementation is compromised. A deeper rewrite of Dapr serialization (while still maintaining backwards compatibility) is still necessary.

Issue reference

• Actors uses different serializers for different purposes, not all of them are pluggable #476
• Can't use C# 9.0 Record for actor method return type #679
• Actor serialization doesn't work as dapr client #1050

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[halspang]

Overall this is really nice, thanks taking on this work!

Just a few nitpicks and you'll need to fix the DCO check by signing off on your commit (e.g. git commit -s).

[codecov]

Codecov Report

Merging #1073 (5c8e872) into master (17f849b) will decrease coverage by 0.58%.
The diff coverage is 9.34%.

@@            Coverage Diff             @@
##           master    #1073      +/-   ##
==========================================
- Coverage   67.02%   66.45%   -0.58%     
==========================================
  Files         170      171       +1     
  Lines        5693     5750      +57     
  Branches      607      624      +17     
==========================================
+ Hits         3816     3821       +5     
- Misses       1732     1782      +50     
- Partials      145      147       +2     

Flag	Coverage Δ
net6	66.45% <9.34%> (-0.58%)	⬇️
net7	66.45% <9.34%> (-0.58%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/Dapr.Actors/Client/ActorProxyOptions.cs	70.00% <0.00%> (-7.78%)	⬇️
...torMessageBodyDataContractSerializationProvider.cs	4.16% <0.00%> (ø)
...ors/Communication/ActorMessageBodyJsonConverter.cs	0.00% <0.00%> (ø)
...ation/ActorMessageBodyJsonSerializationProvider.cs	0.00% <0.00%> (ø)
src/Dapr.Actors/DaprHttpInteractor.cs	48.50% <25.00%> (ø)
src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs	74.46% <33.33%> (-2.81%)	⬇️
src/Dapr.Actors/Client/ActorProxyFactory.cs	77.77% <50.00%> (-5.56%)	⬇️
src/Dapr.Actors/Runtime/ActorManager.cs	52.59% <57.14%> (-0.07%)	⬇️
src/Dapr.Actors/Runtime/ActorRuntime.cs	84.21% <100.00%> (+0.21%)	⬆️

[onionhammer]

Fixed nits

[shivamkm07]

@onionhammer Can you please rebase this PR to master? It seems to be having cherry-picked master commits. Instead of that, rebasing could be more appropriate.

[onionhammer]

@shivamkm07 My git fu is weak, so please let me know if what I did there helps

[shivamkm07]

@shivamkm07 My git fu is weak, so please let me know if what I did there helps

@onionhammer All the extra commits are still there. Seems you have cherry-picked master commits to your branch to update. You must not do that. Instead just merge/rebase master to update the changes. To fix the PR, run the following commands:

   git checkout add-actor-serialization-options
   git branch temp add-actor-serialization-options // create temp branch to have backup
   git reset --hard upstream/master
   git cherry-pick c0edbd0624dd00d05c9e6eb9869993937c3ccfa7
   git cherry-pick 765204feaaad640c4aafa6ee9056ee122e3cd953
   git cherry-pick e2f94b953b63fc18b3c437508c822e83caf60750
   git push -f origin add-actor-serialization-options

Note that above I have used commit hash of the three original commits in the PR. Please ensure these are the ones with the changes you want . Also do re-check everything before doing git push -f as it will override all the changes.

[onionhammer]

Seems you have cherry-picked master commits to your branch to update

This is what Github does, built in, no?

[onionhammer]

@shivamkm07 ok, how about now?

[shivamkm07]

@shivamkm07 ok, how about now?

Good now. Thanks for updating the PR.
@halspang the workflows are awaiting for approval. Can you please approve them?

[shivamkm07]

@onionhammer can you please fix the broken test? One of the integration test is failing

[onionhammer]

@shivamkm07 To be honest, I'm looking at this now, and I'm simply unable to reproduce the error that the CI is getting, any help investigating this would be much appreciated.

I'm available on discord today if anyone wants to ping me about it.

"error invoke actor method: failed to cast response" is an error I get in production dapr actors occasionally, this branch aside.

Even on master branch, the E2E tests fail intermittently on my machine (except TestWorkflows which fails 100% of the time)

For example:

[onionhammer]

I would love to get some assistance on this one if anyone has the time; I'm available on discord - IM me

[onionhammer]

@shivamkm07 still unable to reproduce any issues with the tests locally. Is there a way to run the github actions locally (in a way that works?) Act doesnt seem to work for this project.

[shivamkm07]

@shivamkm07 still unable to reproduce any issues with the tests locally. Is there a way to run the github actions locally (in a way that works?) Act doesnt seem to work for this project.

Tests seem to pass after the changes. Thanks for the fix. Please also fix the DCO.

[darraghjones]

https://github.com/dapr/dotnet-sdk/blob/master/daprdocs/content/en/dotnet-sdk-docs/dotnet-actors/dotnet-actors-serialization.md should probably be updated

[WhitWaldo]

https://github.com/dapr/dotnet-sdk/blob/master/daprdocs/content/en/dotnet-sdk-docs/dotnet-actors/dotnet-actors-serialization.md should probably be updated

Following up here - just merged a PR that'll deploy with 1.15 in which I've addressed the JSON serialization alongside the existing Data Contract serialization.