From 846432c9ac4599cbd24ceac6e756627d3f1859c7 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 6 Mar 2018 22:11:46 +1300 Subject: [PATCH] Improve unexpected server error message to client (#1532) * Improve unexpected server error message to client * Separated expected vs unexpected errors in error message. Fixed broken tests * Fix ts functional tests --- .../FunctionalTests/ts/HubConnectionTests.ts | 10 ++--- .../Internal/DefaultHubDispatcher.cs | 38 +++++++++++++++---- .../HubConnectionTests.cs | 10 ++--- .../HubEndpointTests.cs | 2 +- 4 files changed, 41 insertions(+), 19 deletions(-) diff --git a/client-ts/FunctionalTests/ts/HubConnectionTests.ts b/client-ts/FunctionalTests/ts/HubConnectionTests.ts index 039d3b3cee..1e77f66ffb 100644 --- a/client-ts/FunctionalTests/ts/HubConnectionTests.ts +++ b/client-ts/FunctionalTests/ts/HubConnectionTests.ts @@ -122,7 +122,7 @@ describe("hubConnection", () => { }); it("rethrows an exception from the server when invoking", (done) => { - const errorMessage = "An error occurred."; + const errorMessage = "An unexpected error occurred invoking 'ThrowException' on the server. InvalidOperationException: An error occurred."; const hubConnection = new HubConnection(TESTHUBENDPOINT_URL, { logger: LogLevel.Trace, protocol, @@ -130,7 +130,7 @@ describe("hubConnection", () => { }); hubConnection.start().then(() => { - hubConnection.invoke("ThrowException", errorMessage).then(() => { + hubConnection.invoke("ThrowException", "An error occurred.").then(() => { // exception expected but none thrown fail(); }).catch((e) => { @@ -195,7 +195,7 @@ describe("hubConnection", () => { }); it("rethrows an exception from the server when streaming", (done) => { - const errorMessage = "An error occurred."; + const errorMessage = "An unexpected error occurred invoking 'StreamThrowException' on the server. InvalidOperationException: An error occurred."; const hubConnection = new HubConnection(TESTHUBENDPOINT_URL, { logger: LogLevel.Trace, protocol, @@ -203,13 +203,13 @@ describe("hubConnection", () => { }); hubConnection.start().then(() => { - hubConnection.stream("StreamThrowException", errorMessage).subscribe({ + hubConnection.stream("StreamThrowException", "An error occurred.").subscribe({ complete: function complete() { hubConnection.stop(); fail(); }, error: function error(err) { - expect(err.message).toEqual("An error occurred."); + expect(err.message).toEqual(errorMessage); hubConnection.stop(); done(); }, diff --git a/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs b/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs index 4bf490d887..2c82b3a75e 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs @@ -170,6 +170,13 @@ namespace Microsoft.AspNetCore.SignalR.Internal return; } + if (hubMethodInvocationMessage.ArgumentBindingException != null) + { + Log.FailedInvokingHubMethod(_logger, hubMethodInvocationMessage.Target, hubMethodInvocationMessage.ArgumentBindingException); + await SendInvocationError(hubMethodInvocationMessage, connection, $"Failed to invoke '{hubMethodInvocationMessage.Target}'. {hubMethodInvocationMessage.ArgumentBindingException.Message}"); + return; + } + var hubActivator = scope.ServiceProvider.GetRequiredService>(); var hub = hubActivator.Create(); @@ -181,7 +188,15 @@ namespace Microsoft.AspNetCore.SignalR.Internal if (isStreamedInvocation) { - var enumerator = GetStreamingEnumerator(connection, hubMethodInvocationMessage.InvocationId, methodExecutor, result, methodExecutor.MethodReturnType); + if (!TryGetStreamingEnumerator(connection, hubMethodInvocationMessage.InvocationId, methodExecutor, result, methodExecutor.MethodReturnType, out var enumerator)) + { + Log.InvalidReturnValueFromStreamingMethod(_logger, methodExecutor.MethodInfo.Name); + + await SendInvocationError(hubMethodInvocationMessage, connection, + $"The value returned by the streaming method '{methodExecutor.MethodInfo.Name}' is null, does not implement the IObservable<> interface or is not a ReadableChannel<>."); + return; + } + Log.StreamingResult(_logger, hubMethodInvocationMessage.InvocationId, methodExecutor); await StreamResultsAsync(hubMethodInvocationMessage.InvocationId, connection, enumerator); } @@ -195,12 +210,12 @@ namespace Microsoft.AspNetCore.SignalR.Internal catch (TargetInvocationException ex) { Log.FailedInvokingHubMethod(_logger, hubMethodInvocationMessage.Target, ex); - await SendInvocationError(hubMethodInvocationMessage, connection, ex.InnerException.Message); + await SendInvocationError(hubMethodInvocationMessage, connection, BuildUnexpectedErrorMessage(hubMethodInvocationMessage.Target, ex.InnerException)); } catch (Exception ex) { Log.FailedInvokingHubMethod(_logger, hubMethodInvocationMessage.Target, ex); - await SendInvocationError(hubMethodInvocationMessage, connection, ex.Message); + await SendInvocationError(hubMethodInvocationMessage, connection, BuildUnexpectedErrorMessage(hubMethodInvocationMessage.Target, ex)); } finally { @@ -209,6 +224,11 @@ namespace Microsoft.AspNetCore.SignalR.Internal } } + private string BuildUnexpectedErrorMessage(string methodName, Exception exception) + { + return $"An unexpected error occurred invoking '{methodName}' on the server. {exception.GetType().Name}: {exception.Message}"; + } + private async Task StreamResultsAsync(string invocationId, HubConnectionContext connection, IAsyncEnumerator enumerator) { string error = null; @@ -369,7 +389,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal return false; } - private IAsyncEnumerator GetStreamingEnumerator(HubConnectionContext connection, string invocationId, ObjectMethodExecutor methodExecutor, object result, Type resultType) + private bool TryGetStreamingEnumerator(HubConnectionContext connection, string invocationId, ObjectMethodExecutor methodExecutor, object result, Type resultType, out IAsyncEnumerator enumerator) { if (result != null) { @@ -378,17 +398,19 @@ namespace Microsoft.AspNetCore.SignalR.Internal resultType.GetInterfaces().FirstOrDefault(IsIObservable); if (observableInterface != null) { - return AsyncEnumeratorAdapters.FromObservable(result, observableInterface, CreateCancellation()); + enumerator = AsyncEnumeratorAdapters.FromObservable(result, observableInterface, CreateCancellation()); + return true; } if (IsChannel(resultType, out var payloadType)) { - return AsyncEnumeratorAdapters.FromChannel(result, payloadType, CreateCancellation()); + enumerator = AsyncEnumeratorAdapters.FromChannel(result, payloadType, CreateCancellation()); + return true; } } - Log.InvalidReturnValueFromStreamingMethod(_logger, methodExecutor.MethodInfo.Name); - throw new InvalidOperationException($"The value returned by the streaming method '{methodExecutor.MethodInfo.Name}' is null, does not implement the IObservable<> interface or is not a ReadableChannel<>."); + enumerator = null; + return false; CancellationToken CreateCancellation() { diff --git a/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/HubConnectionTests.cs b/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/HubConnectionTests.cs index aaf4c72f20..24d82ad1d9 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/HubConnectionTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/HubConnectionTests.cs @@ -397,7 +397,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests var channel = await connection.StreamAsync("StreamException").OrTimeout(); var ex = await Assert.ThrowsAsync(() => channel.ReadAllAsync().OrTimeout()); - Assert.Equal("Error occurred while streaming.", ex.Message); + Assert.Equal("An unexpected error occurred invoking 'StreamException' on the server. InvalidOperationException: Error occurred while streaming.", ex.Message); } catch (Exception ex) { @@ -451,7 +451,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests await connection.StartAsync().OrTimeout(); var ex = await Assert.ThrowsAsync(() => connection.InvokeAsync("Echo", "p1", 42)).OrTimeout(); - Assert.Equal("Invocation provides 2 argument(s) but target expects 1.", ex.Message); + Assert.Equal("Failed to invoke 'Echo'. Invocation provides 2 argument(s) but target expects 1.", ex.Message); } catch (Exception ex) { @@ -478,7 +478,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests await connection.StartAsync().OrTimeout(); var ex = await Assert.ThrowsAsync(() => connection.InvokeAsync("Echo", new int[] { 42 })).OrTimeout(); - Assert.StartsWith("Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex.Message); + Assert.StartsWith("Failed to invoke 'Echo'. Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex.Message); } catch (Exception ex) { @@ -535,7 +535,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests var channel = await connection.StreamAsync("Stream", 42, 42); var ex = await Assert.ThrowsAsync(() => channel.ReadAllAsync().OrTimeout()); - Assert.Equal("Invocation provides 2 argument(s) but target expects 1.", ex.Message); + Assert.Equal("Failed to invoke 'Stream'. Invocation provides 2 argument(s) but target expects 1.", ex.Message); } catch (Exception ex) { @@ -563,7 +563,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests var channel = await connection.StreamAsync("Stream", "xyz"); var ex = await Assert.ThrowsAsync(() => channel.ReadAllAsync().OrTimeout()); - Assert.StartsWith("Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex.Message); + Assert.StartsWith("Failed to invoke 'Stream'. Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex.Message); } catch (Exception ex) { diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs index d50c33a0ff..e3192dd582 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs @@ -449,7 +449,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests var result = (await client.InvokeAsync(methodName).OrTimeout()); - Assert.Equal("BOOM!", result.Error); + Assert.Equal($"An unexpected error occurred invoking '{methodName}' on the server. InvalidOperationException: BOOM!", result.Error); // kill the connection client.Dispose();