-
Notifications
You must be signed in to change notification settings - Fork 340
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
Actors uses different serializers for different purposes, not all of them are pluggable #476
Comments
We're not going to make any changes here. This has some rough edges (using the same types with state and actor proxy invocation) but users that are using Actors today are overall happy with the experience. We'll revisit this in the future based on feedback. |
We are actually having issues with this. Would be good to standardize on one serializer. |
This is a also an issue for us. @rynowak I started conversation on discord in the dotnet-sdk channel. I want to make a proposal for the change. Can you please comment on that and let me know if you think I should create new proposal. |
Having a ton of issues with this using records and STJ's polymorphism. Can anyone explain what the OP means about 'non-remoting' actor invocations? |
Is there an update to this issue? Now we encounter that if some newer types (such as dateonly) are returned in the interface method of the actor, DCS(DataContract serializer)related errors will appear Under Net7, STJ can support dateonly these types |
Is there a documentation on unsupported actor method parameters serialization features? How can we make unit tests for our actors that validates the method parameter serialization? |
Using Data Contract serialization is utterly excruciating in 2023. I would love to be able to transmit JsonElements without hacky workarounds. What advantage does DCS have?
|
We're also having this issue. We expect a base class as an actor method parameter and want to store that in the actor state. Currently on the base type we have to set "KnownTypes" for the client -> actor communication to get it serialized correctly and then we need to also need to add "JsonDerivedType" to get actor -> state communication working. Is that really needed? |
Any updates on this issue? It is causing my team quite a bit of grief currently, making it very hard to use Dapr Actors for complex data types (Polymorphism). |
Does the |
@onionhammer Thanks for the tip, unfortunately this doesn't seem to work correctly, do I need to provide some special All methods with parameters fail with this exception:
@darraghjones Edit: I was mistaken, it does not, by coincidence the model I was using could be serialized fine, but deserialization does not work. |
It seems like the current experimental implementation is broken, because a recursion is happening that shouldn't happen:
In the Stacktrace above you can see the {
"value": {
"loadData": {
"someStringProperty1": " a nice string",
"someStringProperty2": "a nice string",
"someStringProperty3": "a nice string",
"someArrayProperty": []
}
}
} The
So I guess currently this experiment can't be used. @onionhammer are you aware of this issue or do you have any tips? P.S.: The problematic line is: if (reader.TokenType != JsonTokenType.PropertyName || reader.GetString() != "value") throw new JsonException(); |
@onionhammer, I debug this behavior with a colleague a bit more. And actually, the issue is more the constructor than the if statement. As you can see there are 3 types defined. if (this.wrappedRequestMessageTypes != null && this.wrappedRequestMessageTypes.Count == 1)
{
this.wrapperMessageType = this.wrappedRequestMessageTypes[0];
} This line checks if the count is exactly 1. So the method if (this.wrapperMessageType != null)
{
/// deserialization logic
} |
Hi @paule96, if you're able to create a PR with a failing test (such as in E2ETests.CustomSerializerTests.cs) I will take a look |
@onionhammer would a sample project also help? I tried to adjust the tests, but the test seems to not work in a DevContainer. You can find the adjusted test setup here. But as mentioned I don't know if the existing test will fail because I can't run them. |
Is there any update to this issue? We were trying to use the JSON serialization in our .NET solution but we had no success. In our scenario we have dapr actors implemented in various technologies but always have a .NET based actor proxy. Therefore only the JSON serialization seems to the one which is working across different technology SDKs. So far, this is a blocking issue for us. |
Expected Behavior
Actors uses a single serializer by default, and allows flexiblity for different choices for compatibility or user freedom.
The problem with not providing a consistent default is that different serializers have non-overlapping features sets, and different configuration settings. The choice of serializer usually ends up being tightly coupled with the users' data model through attributes and other extensibility.
For instance DataContract serializer requires types and properties to "opt-in" to serialization - System.Text.Json assumes that types and public properties should be serialized - and you should "opt-out" for anything you want to exclude. It takes non-obvious work to migrate between these models, and many users are not familiar with the nuances.
Actual Behavior
Actors uses DataContract serializer (XML) for "remoting" method invocation (the choice of DCS is not configurable).
Actors uses System.Text.Json (JSON) for "non-remoting" method invocation (neither the choice of S.T.J nor the options passed to it are configurable).
Actors uses System.Text.Json (JSON) for state storage (the serializer and options have a replaceable abstraction).
Sidenote: Actors currently uses BinaryFormatter for exceptions in "remoting" method invocation, but that is planned for removal.
Steps to Reproduce the Problem
Build an Actor system that uses "remoting". Try to share data models between the state store and the actor method invocation. You'll find that you need to annotate data models for both DataContract serializer and System.Text.Json.
Release Note
We need to surface a design proposal and assess the impact of a change here. This is not a good users experience, but no simple change will fix it.
The text was updated successfully, but these errors were encountered: