From ed68f285bf693c10b5ec4d94c9f01a077d8df48a Mon Sep 17 00:00:00 2001 From: paule96 Date: Tue, 3 Dec 2024 21:26:00 +0100 Subject: [PATCH] =?UTF-8?q?Bug/476=20multiple=20methods=20per=20interface?= =?UTF-8?q?=20with=20JSON=20serialization=20doesn=C2=B4t=20work=20(#1343)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * update devcontainer Signed-off-by: paule96 * update test setup Signed-off-by: paule96 * Now the json serialization should work with multiple methods in an interface Signed-off-by: paule96 * fixed devcontainer to run actors Now the devcontainer uses docker in docker, so you can reach the dapr setup after you did run dapr init. This will then only affect the dev container, without compromising the host of the devcontainer Signed-off-by: paule96 * fix bugs with the current implementation Signed-off-by: paule96 * add a test that checks excatly the behavior Signed-off-by: paule96 * fix devcontainer post creatd command Signed-off-by: paule96 * change the default to dotnet 8.0 Signed-off-by: paule96 * I don't know what is different but we commit. Maybe it resolves the need of chmod for it 🤷‍♀️ Signed-off-by: paule96 * make it easier to see why the application of an E2E test couldn't start Signed-off-by: paule96 * make the exception in E2E more percise Signed-off-by: paule96 * fix exception message Signed-off-by: paule96 --------- Signed-off-by: paule96 Co-authored-by: Yaron Schneider Co-authored-by: Whit Waldo Signed-off-by: Siri Varma Vegiraju --- .devcontainer/devcontainer.json | 20 +++-- .devcontainer/localinit.sh | 0 ...torMessageBodyJsonSerializationProvider.cs | 5 +- .../ActorMessageSerializersManager.cs | 44 +++++++--- src/Dapr.Actors/DaprHttpInteractor.cs | 4 +- src/Dapr.Actors/Runtime/ActorManager.cs | 10 +-- .../ISerializationActor.cs | 2 + .../Actors/SerializationActor.cs | 5 ++ .../Actors/E2ETests.CustomSerializerTests.cs | 27 ++++++ test/Dapr.E2E.Test/DaprCommand.cs | 85 +++++++++++++++++-- 10 files changed, 170 insertions(+), 32 deletions(-) mode change 100644 => 100755 .devcontainer/localinit.sh diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index e179c66b2..3658b56ba 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -10,11 +10,15 @@ "ghcr.io/devcontainers/features/azure-cli:1": { "version": "2.38" }, - "ghcr.io/devcontainers/features/docker-from-docker:1": { - "version": "20.10" + "ghcr.io/devcontainers/features/docker-in-docker": { + "version": "latest" }, - "ghcr.io/devcontainers/features/dotnet:1": { - "version": "6.0" + "ghcr.io/devcontainers/features/dotnet": { + "version": "8.0", + "additionalVersions": [ + "6.0", + "7.0" + ] }, "ghcr.io/devcontainers/features/github-cli:1": { "version": "2" @@ -32,7 +36,8 @@ "ms-dotnettools.csharp", "ms-dotnettools.vscode-dotnet-runtime", "ms-azuretools.vscode-dapr", - "GitHub.copilot" + "GitHub.copilot", + "ms-dotnettools.csdevkit" ], "forwardPorts": [ 3000, @@ -42,10 +47,9 @@ 5000, 5007 ], - "postCreateCommand": ".devcontainer/localinit.sh", + "postCreateCommand": "chmod +x .devcontainer/localinit.sh && .devcontainer/localinit.sh", "remoteUser": "vscode", "hostRequirements": { "memory": "8gb" } - } - \ No newline at end of file +} \ No newline at end of file diff --git a/.devcontainer/localinit.sh b/.devcontainer/localinit.sh old mode 100644 new mode 100755 diff --git a/src/Dapr.Actors/Communication/ActorMessageBodyJsonSerializationProvider.cs b/src/Dapr.Actors/Communication/ActorMessageBodyJsonSerializationProvider.cs index 062d3c742..88bc11cef 100644 --- a/src/Dapr.Actors/Communication/ActorMessageBodyJsonSerializationProvider.cs +++ b/src/Dapr.Actors/Communication/ActorMessageBodyJsonSerializationProvider.cs @@ -103,7 +103,10 @@ public MemoryStreamMessageBodySerializer( { var _methodRequestParameterTypes = new List(methodRequestParameterTypes); var _wrappedRequestMessageTypes = new List(wrappedRequestMessageTypes); - + if(_wrappedRequestMessageTypes.Count > 1){ + throw new NotSupportedException("JSON serialisation should always provide the actor method (or nothing), that was called" + + " to support (de)serialisation. This is a Dapr SDK error, open an issue on GitHub."); + } this.serializerOptions = new(serializerOptions) { // Workaround since WrappedMessageBody creates an object diff --git a/src/Dapr.Actors/Communication/ActorMessageSerializersManager.cs b/src/Dapr.Actors/Communication/ActorMessageSerializersManager.cs index fb68e4cf2..3355aff1a 100644 --- a/src/Dapr.Actors/Communication/ActorMessageSerializersManager.cs +++ b/src/Dapr.Actors/Communication/ActorMessageSerializersManager.cs @@ -15,11 +15,14 @@ namespace Dapr.Actors.Communication { using System; using System.Collections.Concurrent; + using System.Collections.Generic; + using System.Diagnostics.CodeAnalysis; + using System.Linq; using Dapr.Actors.Builder; internal class ActorMessageSerializersManager { - private readonly ConcurrentDictionary cachedBodySerializers; + private readonly ConcurrentDictionary<(int, string), CacheEntry> cachedBodySerializers; private readonly IActorMessageHeaderSerializer headerSerializer; private readonly IActorMessageBodySerializationProvider serializationProvider; @@ -38,7 +41,7 @@ public ActorMessageSerializersManager( } this.serializationProvider = serializationProvider; - this.cachedBodySerializers = new ConcurrentDictionary(); + this.cachedBodySerializers = new ConcurrentDictionary<(int, string), CacheEntry>(); this.headerSerializer = headerSerializer; } @@ -52,19 +55,19 @@ public IActorMessageHeaderSerializer GetHeaderSerializer() return this.headerSerializer; } - public IActorRequestMessageBodySerializer GetRequestMessageBodySerializer(int interfaceId) + public IActorRequestMessageBodySerializer GetRequestMessageBodySerializer(int interfaceId, [AllowNull] string methodName = null) { - return this.cachedBodySerializers.GetOrAdd(interfaceId, this.CreateSerializers).RequestMessageBodySerializer; + return this.cachedBodySerializers.GetOrAdd((interfaceId, methodName), this.CreateSerializers).RequestMessageBodySerializer; } - public IActorResponseMessageBodySerializer GetResponseMessageBodySerializer(int interfaceId) + public IActorResponseMessageBodySerializer GetResponseMessageBodySerializer(int interfaceId, [AllowNull] string methodName = null) { - return this.cachedBodySerializers.GetOrAdd(interfaceId, this.CreateSerializers).ResponseMessageBodySerializer; + return this.cachedBodySerializers.GetOrAdd((interfaceId, methodName), this.CreateSerializers).ResponseMessageBodySerializer; } - internal CacheEntry CreateSerializers(int interfaceId) + internal CacheEntry CreateSerializers((int interfaceId, string methodName) data) { - var interfaceDetails = this.GetInterfaceDetails(interfaceId); + var interfaceDetails = this.GetInterfaceDetails(data.interfaceId); // get the service interface type from the code gen layer var serviceInterfaceType = interfaceDetails.ServiceInterfaceType; @@ -74,10 +77,29 @@ internal CacheEntry CreateSerializers(int interfaceId) // get the known types from the codegen layer var responseBodyTypes = interfaceDetails.ResponseKnownTypes; + if (data.methodName is null) + { + // Path is mainly used for XML serialization + return new CacheEntry( + this.serializationProvider.CreateRequestMessageBodySerializer(serviceInterfaceType, requestBodyTypes, interfaceDetails.RequestWrappedKnownTypes), + this.serializationProvider.CreateResponseMessageBodySerializer(serviceInterfaceType, responseBodyTypes, interfaceDetails.ResponseWrappedKnownTypes)); + } + else + { + // This path should be used for JSON serialization + var requestWrapperTypeAsList = interfaceDetails.RequestWrappedKnownTypes.Where(r => r.Name == $"{data.methodName}ReqBody").ToList(); + if(requestWrapperTypeAsList.Count > 1){ + throw new NotSupportedException($"More then one wrappertype was found for {data.methodName}"); + } + var responseWrapperTypeAsList = interfaceDetails.ResponseWrappedKnownTypes.Where(r => r.Name == $"{data.methodName}RespBody").ToList(); + if(responseWrapperTypeAsList.Count > 1){ + throw new NotSupportedException($"More then one wrappertype was found for {data.methodName}"); + } + return new CacheEntry( + this.serializationProvider.CreateRequestMessageBodySerializer(serviceInterfaceType, requestBodyTypes, requestWrapperTypeAsList), + this.serializationProvider.CreateResponseMessageBodySerializer(serviceInterfaceType, responseBodyTypes, responseWrapperTypeAsList)); + } - return new CacheEntry( - this.serializationProvider.CreateRequestMessageBodySerializer(serviceInterfaceType, requestBodyTypes, interfaceDetails.RequestWrappedKnownTypes), - this.serializationProvider.CreateResponseMessageBodySerializer(serviceInterfaceType, responseBodyTypes, interfaceDetails.ResponseWrappedKnownTypes)); } internal InterfaceDetails GetInterfaceDetails(int interfaceId) diff --git a/src/Dapr.Actors/DaprHttpInteractor.cs b/src/Dapr.Actors/DaprHttpInteractor.cs index 2565bab62..4643ad2f4 100644 --- a/src/Dapr.Actors/DaprHttpInteractor.cs +++ b/src/Dapr.Actors/DaprHttpInteractor.cs @@ -116,7 +116,7 @@ public async Task InvokeActorMethodWithRemotingAsync(Acto var serializedHeader = serializersManager.GetHeaderSerializer() .SerializeRequestHeader(remotingRequestRequestMessage.GetHeader()); - var msgBodySeriaizer = serializersManager.GetRequestMessageBodySerializer(interfaceId); + var msgBodySeriaizer = serializersManager.GetRequestMessageBodySerializer(interfaceId, methodName); var serializedMsgBody = msgBodySeriaizer.Serialize(remotingRequestRequestMessage.GetBody()); // Send Request @@ -170,7 +170,7 @@ HttpRequestMessage RequestFunc() // Deserialize Actor Response Message Body // Deserialize to ActorInvokeException when there is response header otherwise normal path - var responseBodySerializer = serializersManager.GetResponseMessageBodySerializer(interfaceId); + var responseBodySerializer = serializersManager.GetResponseMessageBodySerializer(interfaceId, methodName); // actorResponseMessageHeader is not null, it means there is remote exception if (actorResponseMessageHeader != null) diff --git a/src/Dapr.Actors/Runtime/ActorManager.cs b/src/Dapr.Actors/Runtime/ActorManager.cs index a641440cf..c78126ccd 100644 --- a/src/Dapr.Actors/Runtime/ActorManager.cs +++ b/src/Dapr.Actors/Runtime/ActorManager.cs @@ -106,8 +106,8 @@ internal async Task> DispatchWithRemotingAsync(ActorId act var interfaceId = actorMessageHeader.InterfaceId; // Get the deserialized Body. - var msgBodySerializer = this.serializersManager.GetRequestMessageBodySerializer(actorMessageHeader.InterfaceId); - + var msgBodySerializer = this.serializersManager.GetRequestMessageBodySerializer(actorMessageHeader.InterfaceId, actorMethodContext.MethodName); + IActorRequestMessageBody actorMessageBody; using (var stream = new MemoryStream()) { @@ -130,7 +130,7 @@ async Task> RequestFunc(Actor actor, CancellationToken ct) this.messageBodyFactory, ct); - return this.CreateResponseMessage(responseMsgBody, interfaceId); + return this.CreateResponseMessage(responseMsgBody, interfaceId, actorMethodContext.MethodName); } return await this.DispatchInternalAsync(actorId, actorMethodContext, RequestFunc, cancellationToken); @@ -386,12 +386,12 @@ private async Task DispatchInternalAsync(ActorId actorId, ActorMethodConte return retval; } - private Tuple CreateResponseMessage(IActorResponseMessageBody msgBody, int interfaceId) + private Tuple CreateResponseMessage(IActorResponseMessageBody msgBody, int interfaceId, string methodName) { var responseMsgBodyBytes = Array.Empty(); if (msgBody != null) { - var responseSerializer = this.serializersManager.GetResponseMessageBodySerializer(interfaceId); + var responseSerializer = this.serializersManager.GetResponseMessageBodySerializer(interfaceId, methodName); responseMsgBodyBytes = responseSerializer.Serialize(msgBody); } diff --git a/test/Dapr.E2E.Test.Actors/ISerializationActor.cs b/test/Dapr.E2E.Test.Actors/ISerializationActor.cs index 28190a0d7..46455c28d 100644 --- a/test/Dapr.E2E.Test.Actors/ISerializationActor.cs +++ b/test/Dapr.E2E.Test.Actors/ISerializationActor.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Text.Json; using System.Text.Json.Serialization; @@ -10,6 +11,7 @@ namespace Dapr.E2E.Test.Actors public interface ISerializationActor : IActor, IPingActor { Task SendAsync(string name, SerializationPayload payload, CancellationToken cancellationToken = default); + Task AnotherMethod(DateTime payload); } public record SerializationPayload(string Message) diff --git a/test/Dapr.E2E.Test.App/Actors/SerializationActor.cs b/test/Dapr.E2E.Test.App/Actors/SerializationActor.cs index e8da59826..09e650fae 100644 --- a/test/Dapr.E2E.Test.App/Actors/SerializationActor.cs +++ b/test/Dapr.E2E.Test.App/Actors/SerializationActor.cs @@ -1,4 +1,5 @@  +using System; using System.Threading; using System.Threading.Tasks; using Dapr.Actors.Runtime; @@ -22,5 +23,9 @@ public Task SendAsync(string name, { return Task.FromResult(payload); } + + public Task AnotherMethod(DateTime payload){ + return Task.FromResult(payload); + } } } diff --git a/test/Dapr.E2E.Test/Actors/E2ETests.CustomSerializerTests.cs b/test/Dapr.E2E.Test/Actors/E2ETests.CustomSerializerTests.cs index c393f2ef1..5a20fff3d 100644 --- a/test/Dapr.E2E.Test/Actors/E2ETests.CustomSerializerTests.cs +++ b/test/Dapr.E2E.Test/Actors/E2ETests.CustomSerializerTests.cs @@ -84,5 +84,32 @@ public async Task ActorCanSupportCustomSerializer() Assert.Equal(JsonSerializer.Serialize(kvp.Value), JsonSerializer.Serialize(value)); } } + + /// + /// This was actually a problem that is why the test exists. + /// It just checks, if the interface of the actor has more than one method defined, + /// that if can call it and serialize the payload correctly. + /// + /// + /// More than one methods means here, that in the exact interface must be two methods defined. + /// That excludes hirachies. + /// So wouldn't count here, because it's not directly defined in + /// . (it's defined in the base of it.) + /// That why was created, + /// so there are now more then one method. + /// + [Fact] + public async Task ActorCanSupportCustomSerializerAndCallMoreThenOneDefinedMethod() + { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); + var proxy = this.ProxyFactory.CreateActorProxy(ActorId.CreateRandom(), "SerializationActor"); + + await ActorRuntimeChecker.WaitForActorRuntimeAsync(this.AppId, this.Output, proxy, cts.Token); + + var payload = DateTime.MinValue; + var result = await proxy.AnotherMethod(payload); + + Assert.Equal(payload, result); + } } } diff --git a/test/Dapr.E2E.Test/DaprCommand.cs b/test/Dapr.E2E.Test/DaprCommand.cs index a692ec638..21e31365d 100644 --- a/test/Dapr.E2E.Test/DaprCommand.cs +++ b/test/Dapr.E2E.Test/DaprCommand.cs @@ -16,6 +16,7 @@ namespace Dapr.E2E.Test using System; using System.Collections.Generic; using System.Diagnostics; + using System.Drawing; using System.Linq; using System.Threading; using Xunit.Abstractions; @@ -23,6 +24,7 @@ namespace Dapr.E2E.Test public class DaprCommand { private readonly ITestOutputHelper output; + private readonly CircularBuffer logBuffer = new CircularBuffer(1000); public DaprCommand(ITestOutputHelper output) { @@ -66,7 +68,12 @@ public void Run() var done = outputReceived.WaitOne(this.Timeout); if (!done) { - throw new Exception($"Command: \"{this.Command}\" timed out while waiting for output: \"{this.OutputToMatch}\""); + var ex = new Exception($"Command: \"{this.Command}\" timed out while waiting for output: \"{this.OutputToMatch}\"{System.Environment.NewLine}" + + "This could also mean the E2E app had a startup error. For more details see the Data property of this exception."); + // we add here the log buffer of the last 1000 lines, of the application log + // to make it easier to debug failing tests + ex.Data.Add("log", this.logBuffer.ToArray()); + throw ex; } } @@ -79,8 +86,7 @@ private void CheckOutput(object sendingProcess, DataReceivedEventArgs e) try { - // see: https://github.com/xunit/xunit/issues/2146 - this.output.WriteLine(e.Data.TrimEnd(Environment.NewLine.ToCharArray())); + WriteLine(e.Data); } catch (InvalidOperationException) { @@ -101,12 +107,81 @@ private void OnErrorOutput(object sender, DataReceivedEventArgs e) try { - // see: https://github.com/xunit/xunit/issues/2146 - this.output.WriteLine(e.Data.TrimEnd(Environment.NewLine.ToCharArray())); + WriteLine(e.Data); } catch (InvalidOperationException) { } } + + private void WriteLine(string message) + { + // see: https://github.com/xunit/xunit/issues/2146 + var formattedMessage = message.TrimEnd(Environment.NewLine.ToCharArray()); + this.output.WriteLine(formattedMessage); + this.logBuffer.Add(formattedMessage); + } + } + + /// + /// A circular buffer that can be used to store a fixed number of items. + /// When the buffer is full, the oldest item is overwritten. + /// The buffer can be read in the same order as the items were added. + /// More information can be found here. + /// + /// + /// The buffer gets initialized by the call to the constructor and will allocate, + /// the memory for the buffer. The buffer is not resizable. + /// That means be carefull with , because it can cause an . + /// + /// The type of what the cicular buffer is off. + internal class CircularBuffer{ + private readonly int size; + private readonly T[] buffer; + private int readPosition = 0; + private int writePosition = 0; + /// + /// Initialize the buffer with the buffer size of . + /// + /// + /// The size the buffer will have + /// + public CircularBuffer(int size) + { + this.size = size; + buffer = new T[size]; + } + /// + /// Adds an item and move the write position to the next value + /// + /// The item that should be written. + public void Add(T item) + { + buffer[writePosition] = item; + writePosition = (writePosition + 1) % size; + } + /// + /// Reads on value and move the position to the next value + /// + /// + public T Read(){ + var value = buffer[readPosition]; + readPosition = (readPosition + 1) % size; + return value; + } + /// + /// Read the full buffer. + /// While the buffer is read, the read position is moved to the next value + /// + /// + public T[] ToArray() + { + var result = new T[size]; + for (int i = 0; i < size; i++) + { + result[i] = Read(); + } + return result; + } } }