Merge pull request #884 from Cysharp/feature/HandleUnimplementedHubMe…

…thod Handling when calling a non-implemented hub method
Cysharp · Dec 26, 2024 · 1707590 · 1707590
2 parents b2e17dc + beaf470
commit 1707590
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 29 deletions.
diff --git a/src/MagicOnion.Server/Diagnostics/MagicOnionServerLog.cs b/src/MagicOnion.Server/Diagnostics/MagicOnionServerLog.cs
@@ -88,6 +88,9 @@ static string MethodTypeToString(MethodType type) =>
     [LoggerMessage(EventId = 16, Level = LogLevel.Trace, EventName = nameof(ServiceMethodNotDiscovered), Message = "Could not found gRPC and StreamingHub methods for '{serviceName}'")]
     public static partial void ServiceMethodNotDiscovered(ILogger logger, string serviceName);
 
+    [LoggerMessage(EventId = 17, Level = LogLevel.Information, EventName = nameof(HubMethodNotFound), Message = "StreamingHub method '{methodId}' was not found in '{hubName}'.")]
+    public static partial void HubMethodNotFound(ILogger logger, string hubName, int methodId);
+
     [LoggerMessage(EventId = 90, Level = LogLevel.Error, EventName = nameof(ErrorOnServiceMethod), Message = "A service handler throws an exception occurred in {method}")]
     public static partial void ErrorOnServiceMethod(ILogger logger, Exception ex, string method);
 

diff --git a/src/MagicOnion.Server/Hubs/Internal/StreamingHubPayloadBuilder.cs b/src/MagicOnion.Server/Hubs/Internal/StreamingHubPayloadBuilder.cs
@@ -0,0 +1,29 @@
+using MagicOnion.Internal;
+using MagicOnion.Internal.Buffers;
+using MagicOnion.Serialization;
+
+namespace MagicOnion.Server.Hubs.Internal;
+
+internal static class StreamingHubPayloadBuilder
+{
+    public static StreamingHubPayload Build(int methodId, int messageId)
+    {
+        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
+        StreamingHubMessageWriter.WriteResponseMessage(buffer, methodId, messageId);
+        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
+    }
+
+    public static StreamingHubPayload Build<T>(int methodId, int messageId, T v, IMagicOnionSerializer messageSerializer)
+    {
+        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
+        StreamingHubMessageWriter.WriteResponseMessage(buffer, methodId, messageId, v, messageSerializer);
+        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
+    }
+
+    public static StreamingHubPayload BuildError(int messageId, int statusCode, string detail, Exception? ex, bool isReturnExceptionStackTraceInErrorDetail)
+    {
+        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
+        StreamingHubMessageWriter.WriteResponseMessageForError(buffer, messageId, statusCode, detail, ex, isReturnExceptionStackTraceInErrorDetail);
+        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
+    }
+}
diff --git a/src/MagicOnion.Server/Hubs/StreamingHub.cs b/src/MagicOnion.Server/Hubs/StreamingHub.cs
@@ -312,7 +312,9 @@ async ValueTask ProcessRequestAsync(UniqueHashDictionary<StreamingHubHandler> ha
         }
         else
         {
-            throw new InvalidOperationException("Handler not found in received methodId, methodId:" + methodId);
+            MagicOnionServerLog.HubMethodNotFound(Context.Logger, Context.ServiceName, methodId);
+            var payload = StreamingHubPayloadBuilder.BuildError(messageId, (int)StatusCode.Unimplemented, $"StreamingHub method '{methodId}' is not found in StreamingHub.", null, isReturnExceptionStackTraceInErrorDetail);
+            StreamingServiceContext.QueueResponseStreamWrite(payload);
         }
     }
 

diff --git a/src/MagicOnion.Server/Hubs/StreamingHubContext.cs b/src/MagicOnion.Server/Hubs/StreamingHubContext.cs
@@ -1,4 +1,3 @@
-using MagicOnion.Internal.Buffers;
 using MessagePack;
 using System.Collections.Concurrent;
 using MagicOnion.Internal;
@@ -102,15 +101,15 @@ internal ValueTask WriteResponseMessageNil(ValueTask value)
         ResponseType = typeof(Nil);
         if (value.IsCompletedSuccessfully)
         {
-            WriteMessageCore(BuildMessage());
+            WriteMessageCore(StreamingHubPayloadBuilder.Build(MethodId, MessageId));
             return default;
         }
         return Await(this, value);
 
         static async ValueTask Await(StreamingHubContext ctx, ValueTask value)
         {
             await value.ConfigureAwait(false);
-            ctx.WriteMessageCore(ctx.BuildMessage());
+            ctx.WriteMessageCore(StreamingHubPayloadBuilder.Build(ctx.MethodId, ctx.MessageId));
         }
     }
 
@@ -124,21 +123,21 @@ internal ValueTask WriteResponseMessage<T>(ValueTask<T> value)
         ResponseType = typeof(T);
         if (value.IsCompletedSuccessfully)
         {
-            WriteMessageCore(BuildMessage(value.Result));
+            WriteMessageCore(StreamingHubPayloadBuilder.Build(MethodId, MessageId, value.Result, ServiceContext.MessageSerializer));
             return default;
         }
         return Await(this, value);
 
         static async ValueTask Await(StreamingHubContext ctx, ValueTask<T> value)
         {
             var vv = await value.ConfigureAwait(false);
-            ctx.WriteMessageCore(ctx.BuildMessage(vv));
+            ctx.WriteMessageCore(StreamingHubPayloadBuilder.Build(ctx.MethodId, ctx.MessageId, vv, ctx.ServiceContext.MessageSerializer));
         }
     }
 
     internal ValueTask WriteErrorMessage(int statusCode, string detail, Exception? ex, bool isReturnExceptionStackTraceInErrorDetail)
     {
-        WriteMessageCore(BuildMessageForError(statusCode, detail, ex, isReturnExceptionStackTraceInErrorDetail));
+        WriteMessageCore(StreamingHubPayloadBuilder.BuildError(MessageId, statusCode, detail, ex, isReturnExceptionStackTraceInErrorDetail));
         return default;
     }
 
@@ -147,25 +146,4 @@ void WriteMessageCore(StreamingHubPayload payload)
         ResponseSize = payload.Length; // NOTE: We cannot use the payload after QueueResponseStreamWrite.
         streamingServiceContext.QueueResponseStreamWrite(payload);
     }
-
-    StreamingHubPayload BuildMessage()
-    {
-        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
-        StreamingHubMessageWriter.WriteResponseMessage(buffer, MethodId, MessageId);
-        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
-    }
-
-    StreamingHubPayload BuildMessage<T>(T v)
-    {
-        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
-        StreamingHubMessageWriter.WriteResponseMessage(buffer, MethodId, MessageId, v, streamingServiceContext.MessageSerializer);
-        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
-    }
-
-    StreamingHubPayload BuildMessageForError(int statusCode, string detail, Exception? ex, bool isReturnExceptionStackTraceInErrorDetail)
-    {
-        using var buffer = ArrayPoolBufferWriter.RentThreadStaticWriter();
-        StreamingHubMessageWriter.WriteResponseMessageForError(buffer, MessageId, statusCode, detail, ex, isReturnExceptionStackTraceInErrorDetail);
-        return StreamingHubPayloadPool.Shared.RentOrCreate(buffer.WrittenSpan);
-    }
 }
diff --git a/tests/MagicOnion.Integration.Tests/StreamingHubTest.UnknownMethodId.cs b/tests/MagicOnion.Integration.Tests/StreamingHubTest.UnknownMethodId.cs
@@ -0,0 +1,39 @@
+using Grpc.Net.Client;
+using MagicOnion.Server.Hubs;
+
+namespace MagicOnion.Integration.Tests
+{
+    public partial class StreamingHubTest
+    {
+        [Theory]
+        [MemberData(nameof(EnumerateStreamingHubClientFactory))]
+        public async Task UnknownMethodId(TestStreamingHubClientFactory clientFactory)
+        {
+            // Arrange
+            var httpClient = factory.CreateDefaultClient();
+            var channel = GrpcChannel.ForAddress("http://localhost", new GrpcChannelOptions() { HttpClient = httpClient });
+
+            var receiver = Substitute.For<IStreamingHubTestHubReceiver>();
+            var client = await clientFactory.CreateAndConnectAsync<MagicOnion.Integration.Tests.UnknownMethodIdTest.IStreamingHubTestHub, IStreamingHubTestHubReceiver>(channel, receiver);
+
+            // Act
+            var ex = await Record.ExceptionAsync(async () => await client.CustomMethodId().WaitAsync(TimeSpan.FromSeconds(1)));
+
+            // Assert
+            ex.Should().NotBeNull();
+            var rpcException = ex.Should().BeOfType<RpcException>().Subject;
+            rpcException.Status.StatusCode.Should().Be(StatusCode.Unimplemented);
+            factory.Logs.Should().Contain(x => x.Contains("HubMethodNotFound\tStreamingHub method '-1'"));
+        }
+    }
+
+}
+
+namespace MagicOnion.Integration.Tests.UnknownMethodIdTest
+{
+    public interface IStreamingHubTestHub : IStreamingHub<IStreamingHubTestHub, IStreamingHubTestHubReceiver>
+    {
+        [MethodId(-1)]
+        Task CustomMethodId();
+    }
+}
diff --git a/tests/MagicOnion.Integration.Tests/StreamingHubTest.cs b/tests/MagicOnion.Integration.Tests/StreamingHubTest.cs
@@ -6,7 +6,7 @@
 
 namespace MagicOnion.Integration.Tests;
 
-public class StreamingHubTest : IClassFixture<MagicOnionApplicationFactory<StreamingHubTestHub>>
+public partial class StreamingHubTest : IClassFixture<MagicOnionApplicationFactory<StreamingHubTestHub>>
 {
     readonly MagicOnionApplicationFactory<StreamingHubTestHub> factory;