-
Notifications
You must be signed in to change notification settings - Fork 337
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
Can't use C# 9.0 Record for actor method return type #679
Comments
I agree that this would be nice to enable, but it's not something we can easily do within the .NET SDK unless we make more options available for choosing the serializer. .NET Actors use DataContractSerializer when used with strongly-typed contracts, and so we have all of its limitations and properties. |
Okay, that makes sense. Would it be worth me looking into using something like System.Text.Json in the Actors? It looks like it's being used for the state persistence. |
I think it be worth starting a design discussion of what it would mean to make the serializer pluggable for actors. #476 As with all things we need to provide a graceful migration path and avoid changing things in a big-bang. For instance. If you could attribute an interface in a way that configured the serializer. This allows client and server to change together, and would allow migrating apps per-interfacer/per-actor. Also complicating this is that DataContractSerializer is pretty feature-rich, and not everything it does will be supported in other serializers. Eg. cycle handling, knowntypes. Realistically we will need to keep DCS support forever. Most users don't intimately understand what the serializer is doing, they just have working code, and they would be unhappy if we turn it into non-working code in a version upgrade. |
Bumping.. cant seem to use them for arguments either. Easily system.text.json serializable stuff just doesnt work with whatever voodoo Why provide |
@halspang @johnewart, thoughts? I hit this too immediately building Actor quickstart. |
@halspang is there a tracking issue for custom serialization support for Dapr actor remoting functions? I'm seeing a lot of beating around the bush, but the clear culprit for a lot of issues is the use of the DataContractSerializer, and that it is not able to be configured or replaced Maybe #476 but it's only tangentially related and old |
/assign |
@onionhammer PR #1073 seems to add Json Serialization capability to the actor response serialization. So would that resolve this issue of using Records as return type? |
The reason serialization breaks is because the data contract serialization expects a parameterless constructor and records don't generally have one. If they're provided with one (this rather breaks the simplicity of setting up records) or decorated with the appropriate Using the quickstart example, one can teak This one works without attribute decoration because there's an implicit parameterless constructor since the primary constructor isn't used: public record SmartDeviceData
{
public string Status { get; set; } = default!;
public string Location { get; set; } = default!;
public override string ToString() => $"Location: {this.Location}, Status: {this.Status}";
} And if we'd rather use primary constructor approach, we need to use the [DataContract]
public record SmartDeviceData (
[property: DataMember] string Status,
[property: DataMember] string Location)
{
public override string ToString() => $"Location: {this.Location}, Status: {this.Status}";
} There's no bug here - just a misunderstanding about what's changed about records versus classes. In the short term, perhaps this is just a matter of putting together better documentation to explain the problem and the path forward for new Dapr users. Indicate that this is the recommended approach for today and point to the WCF data serialization documentation for more in-depth reading. In the longer term, perhaps the serialization itself can be made more pluggable to support JSON or other approaches. |
/assign |
Found the problem. If the .NET team aren't updating data contract serialization to support language features, its effectively deprecated. |
Personally, I'd just like to move away from the remoting-based proxy (see #1158 which would allow doing so on the client side). |
I wouldn't say they're not updating to support language features so much as it has always required either a parameterless constructor or the Again, this is working precisely as intended and is not a bug. I wouldn't argue it's effectively deprecated because nothing about the contract as it's already stood has changed with the introduction of records. Is it as straightforward as the JSON serializers from Newtonsoft or System.Text.Json? No, but nothing about this changed with the introduction of records - more to the point, people were likely just caught off-guard by a rule that they probably hadn't thought much about beforehand. |
I think there are a lot of benefits to the approach you took and I'm all for giving the developer the option of which they'd prefer using. My only goal in responding here was to correct the record that this is not a bug, but rather an inconvenient side-effect from what the OP was expecting, and isn't worth developer resources to fix because the fix (to use remoted clients at least) is already known and needs to be implemented by the developer - add a parameterless constructor or mark with the necessary attributes. |
Sorry, but DX and ergonomics should take precedence over "um technically not a bug". Undocumented foot-guns that people only find out the resolution to by googling runtime errors and hopefully finding an explanation buried in this thread is NOT ideal. |
I completely agree - that's why I propose the documentation and examples be updated to very clearly spell this out. I'm typing up a tentative draft right now. This could just as easily be a gotcha to anyone trying to do the same thing with a class that lacks a parameterless constructor too - yes, the error message pretty clearly spells out a viable remedy, but I'm in the camp of educating the user so they don't experience the error in the first place. Limiting exposure just to this thread would introduce unnecessary and continued frustration at something that's working precisely as it should. |
Expected Behavior
I would like to use a record as a return type for an actor method.
Actual Behavior
Here is the record definition:
It fails on this line in the dotnet-sdk:
https://github.com/dapr/dotnet-sdk/blob/master/src/Dapr.Actors/Communication/ActorMessageBodyDataContractSerializationProvider.cs#L206
Which throws this exception:
Steps to Reproduce the Problem
Return a Record type from an actor and call it using the dotnet-sdk
Release Note
RELEASE NOTE:
The text was updated successfully, but these errors were encountered: