Weakly-typed actors should support polymorphic response and null response

[RemcoBlok]

In .NET 7.0 or later an operation on an actor invoked using a weakly-typed actor client should support a polymorphic response. The response json should include a type discriminator property to allow for polymorphic deserialization by the actor client. The weakly-typed actor client must polymorphically deserialize the response when invoking InvokeMethodAsync<ResponseBase> on the weakly-typed actor client. This should yield a DerivedResponse instance. Note that invoking InvokeMethodAsync<DerivedResponse> is not polymorphic deserialization.

While System.Text.Json support polymorphic deserialization from .NET 7, the Dapr .NET Actors SDK only builds .NET 6 and .NET 8, but not .NET 7. The E2E tests on the other hand do still build .NET 6, .NET 7 and .NET 8. As a consequence the .NET 7 build of the E2E tests uses the .NET 6 build of the .NET Actors SDK and does not run the polymorphic response test. Instead it runs a non-polymorphic test that directly deserializes to the derived class.

An operation on an actor invoked using a weakly-typed actor client should support a null response.

The problem was in the ActorManager.DispatchWithoutRemotingAsync method that serializes the response using result.GetType(). This throws a null reference exception when the result is null. To support polymorphic deserialization in .NET 7 or later the serializer should use the declared return type on the interface instead of the runtime type.

Closes #1213

[philliphoff]

My first inclination is to not change the existing behavior for .NET 6 (and, by extension, .NET 7 given we don't ship that specific target), as that would be an invisible change that has potential privacy/security concerns if unexpected properties were serialized. If users desire the polymorphic serialization behavior, they can move to .NET 8 and use the new serialization attributes.

(If there was significant push by users to support the serialization behavior on .NET 6, then I suppose some sort of flag could exist in the options object, but I feel that it would need to be opt-in. Still, the attributes feel like a better approach.)

That said, null responses should be supported, so I appreciate the fix for that.

[RemcoBlok]

Thanks for reviewing the PR. As I understood things, apart from supporting null responses my change did not change the behavior in .NET 6. I understood that in .NET 6 for a non-null response

await JsonSerializer.SerializeAsync(responseBodyStream, result, result.GetType(), jsonSerializerOptions);

and

await JsonSerializer.SerializeAsync<object>(responseBodyStream, result, jsonSerializerOptions);

produce the same json. And as the current version of the code uses the runtime type the json includes properties of derived classes. The E2E tests I added assert that for .NET 6 my change does the same. As such my change should not introduce new privacy or security concerns. But please correct me if I'm wrong.

I'm not sure I understand your comment regarding adding some sort of flag on the options object to support polymorphic (de)serialization in .NET 6. As System.Text.Json in .NET 6 does not support polymorphic (de)serialization out-of-the-box I don't think Dapr needs to support it and a flag is not needed for .NET 6.

System.Text.Json in .NET 7 does support polymorphic (de)serialization out-of-the-box, provided we serialize using the declared type on the interface instead of the runtime type as follows:

var returnType = methodInfo.ReturnType.GenericTypeArguments[0];
await JsonSerializer.SerializeAsync(responseBodyStream, result, returnType, jsonSerializerOptions);

I think using the declared type instead of the runtime type should be the default in .NET 7 or later. Again, no flag needed. If programmers require polymorphic (de)serialization then they can use the JsonDerivedTypeAttribute or, as I prefer, write a JsonTypeInfoResolver.

[philliphoff]

Looking at the docs again, you're right, there's no change to the existing .NET 6 usage, so I retract my earlier comments.

This would still represent a breaking change for users of the SDK that rely on the .NET 6 polymorphic behavior when they move to .NET 8 though, right, in that they'd need to decorate their model types with the appropriate attributes? (I agree that that seems like the best approach.) Given that, we'd need to make sure there was adequate documentation of that change.

[RemcoBlok]

This would still represent a breaking change for users of the SDK that rely on the .NET 6 polymorphic behavior when they move to .NET 8 though, right

Correct, that would be a breaking change. Although one could argue in .NET 6 one has polymorphic serialization, not polymorphic deserialization, but it is exactly the serialization of derived properties that would break when moving to .NET 8.

we'd need to make sure there was adequate documentation of that change

As in a release note?
Or as in Dapr docs somewhere in https://docs.dapr.io/developing-applications/sdks/dotnet/dotnet-actors/ ?

The Dapr docs have no current mention on polymorphism. It would be good to write about polymorphism in the Dapr docs for both weakly-typed and strongly-typed actors using both DataContractSerializer and json serializer. And also for service invocation.

The E2E tests I added for this change are also the first tests on polymorphism in the .NET SDK if I'm not mistaken. I only added E2E to test polymorphism in a response object for weakly-typed actors, because that was not currently working. It would be good to also add E2E tests for polymorphism in a request object in weakly typed actors. And for both request and ressponse objects in strongly-typed actors using both the DataContractSerializer and the newly added support for json serialization in strongly-typed actors. But I think it is best to add those additional tests in a separate PR so that this PR remains focussed on the fix.

[philliphoff]

As in a release note?
Or as in Dapr docs somewhere in https://docs.dapr.io/developing-applications/sdks/dotnet/dotnet-actors/ ?

At a minimum, there should be a note in the release docs related to the implicit change in behavior. Having general docs on Dapr serialization and polymorphism would be great but no necessarily required (given there is ample MS docs on the subject). Still, there have been other places where having a basic summary of .NET behavior can be helpful (see #1199) to a Dapr developer.

[WhitWaldo]

#1199 is now #1222 since I had a typo in the DCO. @RemcoBlok I'd encourage you to type up the documentation in the actors for your changes so it's available immediately in the next release instead of being lost in this PR thread indefinitely. There's a flag you can request they add linking the docs to your PR here so one isn't accepted without the other.

[RemcoBlok]

Thanks all for your replies. I've been travelling and not had a chance to respond earlier. I have some text for a release note. Is it best to discuss that in the issue I created related to the PR instead of here? The issue template seemed to suggest that the release notes are picked up from the issue, not the PR.

[RemcoBlok]

On second thoughts, let's discuss the release note here so that we don't have to jump between threads.

Release Note

Weakly-typed actor should support polymorphic response.

Problem

The .NET 6 and .NET 8 targets of the Dapr SDK have different serialization behavior of properties of a derived response object on an operation of a weakly-typed actor client.

Impact

Impacts users upgrading a consumer application to .NET 8 running Dapr <version> or later.

Root Cause

In the .NET 6 target of the SDK the actor runtime serializes the response object on an operation of a weakly-typed actor client using the runtime type of the response object. In the .NET 8 target of the SDK the actor runtime serializes the response object using the base response type as declared on the operation on the actor interface.

Solution

The programmer must decorate the base response class that is declared on the operation on the actor interface with a JsonDerivedType attribute specifying the derived response class.
Alternatively, a custom IJsonTypeInfoResolver implementation can be set on the JsonSerializerOptions.TypeInfoResolver property adding a JsonDerivedType instance to the JsonPolymorphismOptions.DerivedTypes property on the JsonTypeInfo.PolymorphismOptions property.

Note that while .NET 7 introduced polymorphic (de)serialization using JsonDerivedType attribute or JsonPolymorphismOptions on a custom IJsonTypeInfoResolver implementation, Dapr does not ship a .NET 7 target of the SDK. Hence, a .NET 7 consumer application uses the .NET 6 target of the Dapr SDK. Therefore, this breaking change only applies when upgrading a consumer application to .NET 8.

Note that the new serialization behavior in the .NET 8 version of the SDK allows for polymorphic deserialization of the response by a weakly-typed actor client. This requires that the programmer specifies a typeDiscriminator on the JsonDerivedType attribute.

[RemcoBlok]

@philliphoff I noticed the regression tests for .NET 7 failed. Not sure why. Can you kick off the test again please?

[philliphoff]

@RemcoBlok It looks like this is good, pending abiding by the DCO (sign-off) policy.

[RemcoBlok]

@RemcoBlok It looks like this is good, pending abiding by the DCO (sign-off) policy.

resolved the DCO issue.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d023a43) 68.47% compared to head (27b8ad4) 68.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
+ Coverage   68.47%   68.48%   +0.01%     
==========================================
  Files         172      172              
  Lines        5846     5848       +2     
  Branches      648      648              
==========================================
+ Hits         4003     4005       +2     
  Misses       1681     1681              
  Partials      162      162              

Flag	Coverage Δ
net6	68.46% <100.00%> (ø)
net7	68.46% <100.00%> (ø)
net8	68.46% <100.00%> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RemcoBlok]

@philliphoff It would appear the integration tests for .NET 7.0 fail occasionally. Last time they succeeded when trying again. I'm sure this occasional failure is not related to this PR. Could you try again please?

[RemcoBlok]

@philliphoff Thanks for approving the PR!

[philliphoff]

@RemcoBlok Thanks for the contribution!