-
Notifications
You must be signed in to change notification settings - Fork 345
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
Weakly-typed actors should support polymorphic response and null response #1214
Merged
philliphoff
merged 2 commits into
dapr:master
from
RemcoBlok:weaklyTypedActorPolymorphismAndNullResponses
Jan 31, 2024
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
test/Dapr.E2E.Test.Actors/WeaklyTypedTesting/DerivedResponse.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// ------------------------------------------------------------------------ | ||
// Copyright 2021 The Dapr Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ------------------------------------------------------------------------ | ||
|
||
namespace Dapr.E2E.Test.Actors.WeaklyTypedTesting | ||
{ | ||
public class DerivedResponse : ResponseBase | ||
{ | ||
public string DerivedProperty { get; set; } | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
test/Dapr.E2E.Test.Actors/WeaklyTypedTesting/IWeaklyTypedTestingActor.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// ------------------------------------------------------------------------ | ||
// Copyright 2021 The Dapr Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ------------------------------------------------------------------------ | ||
|
||
using System.Threading.Tasks; | ||
using Dapr.Actors; | ||
|
||
namespace Dapr.E2E.Test.Actors.WeaklyTypedTesting | ||
{ | ||
public interface IWeaklyTypedTestingActor : IPingActor, IActor | ||
{ | ||
Task<ResponseBase> GetPolymorphicResponse(); | ||
|
||
Task<ResponseBase> GetNullResponse(); | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
test/Dapr.E2E.Test.Actors/WeaklyTypedTesting/ResponseBase.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// ------------------------------------------------------------------------ | ||
// Copyright 2021 The Dapr Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ------------------------------------------------------------------------ | ||
|
||
using System.Text.Json.Serialization; | ||
|
||
namespace Dapr.E2E.Test.Actors.WeaklyTypedTesting | ||
{ | ||
#if NET7_0_OR_GREATER | ||
[JsonDerivedType(typeof(DerivedResponse), typeDiscriminator: nameof(DerivedResponse))] | ||
#endif | ||
public class ResponseBase | ||
{ | ||
public string BasePropeprty { get; set; } | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// ------------------------------------------------------------------------ | ||
// Copyright 2021 The Dapr Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ------------------------------------------------------------------------ | ||
|
||
using System.Threading.Tasks; | ||
using Dapr.Actors.Runtime; | ||
|
||
namespace Dapr.E2E.Test.Actors.WeaklyTypedTesting | ||
{ | ||
public class WeaklyTypedTestingActor : Actor, IWeaklyTypedTestingActor | ||
{ | ||
public WeaklyTypedTestingActor(ActorHost host) | ||
: base(host) | ||
{ | ||
} | ||
|
||
public Task<ResponseBase> GetNullResponse() | ||
{ | ||
return Task.FromResult<ResponseBase>(null); | ||
} | ||
|
||
public Task<ResponseBase> GetPolymorphicResponse() | ||
{ | ||
var response = new DerivedResponse | ||
{ | ||
BasePropeprty = "Base property value", | ||
DerivedProperty = "Derived property value" | ||
}; | ||
|
||
return Task.FromResult<ResponseBase>(response); | ||
} | ||
|
||
public Task Ping() | ||
{ | ||
return Task.CompletedTask; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
// ------------------------------------------------------------------------ | ||
// Copyright 2021 The Dapr Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ------------------------------------------------------------------------ | ||
namespace Dapr.E2E.Test | ||
{ | ||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Dapr.Actors; | ||
using Dapr.E2E.Test.Actors.WeaklyTypedTesting; | ||
using FluentAssertions; | ||
using Xunit; | ||
|
||
public partial class E2ETests : IAsyncLifetime | ||
{ | ||
#if NET8_0_OR_GREATER | ||
[Fact] | ||
public async Task WeaklyTypedActorCanReturnPolymorphicResponse() | ||
{ | ||
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); | ||
var pingProxy = this.ProxyFactory.CreateActorProxy<IWeaklyTypedTestingActor>(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
var proxy = this.ProxyFactory.Create(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
|
||
await WaitForActorRuntimeAsync(pingProxy, cts.Token); | ||
|
||
var result = await proxy.InvokeMethodAsync<ResponseBase>(nameof(IWeaklyTypedTestingActor.GetPolymorphicResponse)); | ||
|
||
result.Should().BeOfType<DerivedResponse>().Which.DerivedProperty.Should().NotBeNullOrWhiteSpace(); | ||
} | ||
#else | ||
[Fact] | ||
public async Task WeaklyTypedActorCanReturnDerivedResponse() | ||
{ | ||
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); | ||
var pingProxy = this.ProxyFactory.CreateActorProxy<IWeaklyTypedTestingActor>(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
var proxy = this.ProxyFactory.Create(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
|
||
await WaitForActorRuntimeAsync(pingProxy, cts.Token); | ||
|
||
var result = await proxy.InvokeMethodAsync<DerivedResponse>(nameof(IWeaklyTypedTestingActor.GetPolymorphicResponse)); | ||
|
||
result.Should().BeOfType<DerivedResponse>().Which.DerivedProperty.Should().NotBeNullOrWhiteSpace(); | ||
} | ||
#endif | ||
[Fact] | ||
public async Task WeaklyTypedActorCanReturnNullResponse() | ||
{ | ||
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); | ||
var pingProxy = this.ProxyFactory.CreateActorProxy<IWeaklyTypedTestingActor>(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
var proxy = this.ProxyFactory.Create(ActorId.CreateRandom(), "WeaklyTypedTestingActor"); | ||
|
||
await WaitForActorRuntimeAsync(pingProxy, cts.Token); | ||
|
||
var result = await proxy.InvokeMethodAsync<ResponseBase>(nameof(IWeaklyTypedTestingActor.GetNullResponse)); | ||
|
||
result.Should().BeNull(); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 responseand
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theJsonSerializerOptions.TypeInfoResolver
property adding aJsonDerivedType
instance to theJsonPolymorphismOptions.DerivedTypes
property on theJsonTypeInfo.PolymorphismOptions
property.Note that while .NET 7 introduced polymorphic (de)serialization using
JsonDerivedType
attribute orJsonPolymorphismOptions
on a customIJsonTypeInfoResolver
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 theJsonDerivedType
attribute.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philliphoff I noticed the regression tests for .NET 7 failed. Not sure why. Can you kick off the test again please?