dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.12k stars 340 forks source link

Weakly-typed actors should support polymorphic response and null response #1214

Closed RemcoBlok closed 10 months ago

RemcoBlok commented 11 months ago

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 commented 10 months ago

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

RemcoBlok commented 10 months ago

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

resolved the DCO issue.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1214/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | Coverage Δ | | |---|---|---| | [net6](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <100.00%> (ø)` | | | [net7](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <100.00%> (ø)` | | | [net8](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

RemcoBlok commented 10 months ago

@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 commented 10 months ago

@philliphoff Thanks for approving the PR!

philliphoff commented 10 months ago

@RemcoBlok Thanks for the contribution!