Fixes Strawberry shake exception handling (#4596) (#6039)

Co-authored-by: Michael Staib <michael@chillicream.com>

src/HotChocolate/Core/src/Types/Types/Descriptors/ObjectFieldDescriptor.cs

@@ -25,7 +25,7 @@ public class ObjectFieldDescriptor
     private ParameterInfo[] _parameterInfos = Array.Empty<ParameterInfo>();
 
     /// <summary>
-    ///  Creates a new instance of <see cref="ObjectFieldDescriptor"/>
+    /// Creates a new instance of <see cref="ObjectFieldDescriptor"/>
     /// </summary>
     protected ObjectFieldDescriptor(
         IDescriptorContext context,
@@ -38,7 +38,7 @@ protected ObjectFieldDescriptor(
     }
 
     /// <summary>
-    ///  Creates a new instance of <see cref="ObjectFieldDescriptor"/>
+    /// Creates a new instance of <see cref="ObjectFieldDescriptor"/>
     /// </summary>
     protected ObjectFieldDescriptor(
         IDescriptorContext context,
@@ -76,7 +76,7 @@ protected ObjectFieldDescriptor(
     }
 
     /// <summary>
-    ///  Creates a new instance of <see cref="ObjectFieldDescriptor"/>
+    /// Creates a new instance of <see cref="ObjectFieldDescriptor"/>
     /// </summary>
     protected ObjectFieldDescriptor(
         IDescriptorContext context,

src/StrawberryShake/Client/src/Core/OperationResultBuilder.cs

@@ -66,6 +66,23 @@ public IOperationResult<TResultData> Build(
             errors = list;
         }
 
+        // If we have a transport error but the response does not contain any client errors
+        // we will create a client error from the provided transport error.
+        if (response.Exception is not null && errors is not { Count: > 0 })
+        {
+            errors = new IClientError[]
+            {
+                new ClientError(
+                    response.Exception.Message,
+                    ErrorCodes.InvalidResultDataStructure,
+                    exception: response.Exception,
+                    extensions: new Dictionary<string, object?>
+                    {
+                        { nameof(response.Exception.StackTrace), response.Exception.StackTrace }
+                    })
+            };
+        }
+
         return new OperationResult<TResultData>(
             data,
             dataInfo,

src/StrawberryShake/Client/src/Core/Response.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using StrawberryShake.Properties;
 using static StrawberryShake.Properties.Resources;
 
 namespace StrawberryShake;
@@ -38,7 +39,7 @@ public sealed class Response<TBody> : IDisposable where TBody : class
     /// Additional custom data provided by client extensions.
     /// </param>
     public Response(
-        TBody body,
+        TBody? body,
         Exception? exception,
         bool isPatch = false,
         bool hasNext = false,
@@ -61,7 +62,7 @@ public Response(
     /// <summary>
     /// The serialized response body.
     /// </summary>
-    public TBody Body { get; }
+    public TBody? Body { get; }
 
     /// <summary>
     /// The transport exception.

src/StrawberryShake/Client/src/Resources/Properties/Resources.resx

@@ -132,4 +132,7 @@
   <data name="JsonResultPatcher_PathSegmentMustBeStringOrInt" xml:space="preserve">
     <value>A path segment must be a string or an integer.</value>
   </data>
+  <data name="ResponseEnumerator_HttpNoSuccessStatusCode" xml:space="preserve">
+    <value>Response status code does not indicate success: {0} ({1}).</value>
+  </data>
 </root>

src/StrawberryShake/Client/src/Transport.Http/ResponseEnumerator.cs

@@ -9,6 +9,7 @@
 using Microsoft.AspNetCore.WebUtilities;
 using static System.Net.Http.HttpCompletionOption;
 using static System.StringComparison;
+using static StrawberryShake.Properties.Resources;
 using static StrawberryShake.Transport.Http.ResponseHelper;
 
 namespace StrawberryShake.Transport.Http;
@@ -67,8 +68,37 @@ public async ValueTask<bool> MoveNextAsync()
             {
                 try
                 {
-                    response.EnsureSuccessStatusCode();
-                    Current = await stream.TryParseResponse(_abort).ConfigureAwait(false);
+                    Exception? transportError = null;
+
+                    // If we detect that the response has a non-success status code we will
+                    // create a transport error that will be added to the response.
+                    if (!response.IsSuccessStatusCode)
+                    {
+#if NET5_0_OR_GREATER
+                        transportError =
+                            new HttpRequestException(
+                                string.Format(
+                                    ResponseEnumerator_HttpNoSuccessStatusCode,
+                                    (int)response.StatusCode,
+                                    response.ReasonPhrase),
+                                null,
+                                response.StatusCode);
+#else
+                        transportError =
+                            new HttpRequestException(
+                                string.Format(
+                                    ResponseEnumerator_HttpNoSuccessStatusCode,
+                                    (int)response.StatusCode,
+                                    response.ReasonPhrase),
+                                null);
+#endif
+                    }
+
+                    // We now try to parse the possible GraphQL response, this step could fail
+                    // as the response might not be a GraphQL response. It could in some cases
+                    // be a HTML error page.
+                    Current = await stream.TryParseResponse(transportError, _abort)
+                        .ConfigureAwait(false);
                 }
                 catch (Exception ex)
                 {
@@ -93,7 +123,7 @@ public async ValueTask<bool> MoveNextAsync()
         using var body = multipartSection.Body;
 #endif
 
-        Current = await body.TryParseResponse(_abort).ConfigureAwait(false);
+        Current = await body.TryParseResponse(null, _abort).ConfigureAwait(false);
 
         if (Current.Exception is not null)
         {

src/StrawberryShake/Client/src/Transport.Http/ResponseHelper.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Buffers;
 using System.IO;
+using System.Net.Http;
 using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
@@ -13,6 +14,7 @@ internal static class ResponseHelper
 {
     public static async Task<Response<JsonDocument>> TryParseResponse(
         this Stream stream,
+        Exception? transportError,
         CancellationToken cancellationToken)
     {
         try
@@ -35,14 +37,15 @@ await JsonDocument.ParseAsync(
                     hasNext = true;
                 }
 
-                return new Response<JsonDocument>(document, null, isPatch, hasNext);
+                return new Response<JsonDocument>(document, transportError, isPatch, hasNext);
             }
 
-            return new Response<JsonDocument>(document, null);
+            return new Response<JsonDocument>(document, transportError);
         }
         catch (Exception ex)
         {
-            return new Response<JsonDocument>(CreateBodyFromException(ex), ex);
+            var error = transportError ?? ex;
+            return new Response<JsonDocument>(CreateBodyFromException(error), error);
         }
     }
 

src/StrawberryShake/Client/test/Transport.WebSocket.Tests/TestHelper/TestServerHelper.cs

@@ -1,11 +1,14 @@
 using System;
+using HotChocolate;
+using HotChocolate.Execution;
 using HotChocolate.Execution.Configuration;
 using HotChocolate.StarWars;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
+using static HotChocolate.WellKnownContextData;
 
 namespace StrawberryShake.Transport.WebSockets;
 
@@ -24,55 +27,93 @@ public static IWebHost CreateServer(Action<IRequestExecutorBuilder> configure, o
                 var host = new WebHostBuilder()
                     .UseConfiguration(config)
                     .UseKestrel()
-                    .ConfigureServices(services =>
-                    {
-                        var builder = services.AddRouting().AddGraphQLServer();
+                    .ConfigureServices(
+                        services =>
+                        {
+                            var builder = services.AddRouting().AddGraphQLServer();
 
-                        configure(builder);
+                            configure(builder);
 
-                        builder
-                            .AddStarWarsTypes()
-                            .AddExportDirectiveType()
-                            .AddStarWarsRepositories()
-                            .AddInMemorySubscriptions()
-                            .ModifyOptions(
-                                o =>
-                                {
-                                    o.EnableDefer = true;
-                                    o.EnableStream = true;
-                                });
-                    })
-                    .Configure(app =>
-                        app.Use(async (ct, next) =>
-                            {
-                                try
-                                {
-                                    // Kestrel does not return proper error responses:
-                                    // https://github.com/aspnet/KestrelHttpServer/issues/43
-                                    await next();
-                                }
-                                catch (Exception ex)
-                                {
-                                    if (ct.Response.HasStarted)
+                            builder
+                                .AddStarWarsTypes()
+                                .AddExportDirectiveType()
+                                .AddStarWarsRepositories()
+                                .AddInMemorySubscriptions()
+                                .ModifyOptions(
+                                    o =>
                                     {
-                                        throw;
-                                    }
+                                        o.EnableDefer = true;
+                                        o.EnableStream = true;
+                                    })
+                                .UseDefaultPipeline()
+                                .UseRequest(
+                                    next => async context =>
+                                    {
+                                        if (context.ContextData.TryGetValue(
+                                                nameof(HttpContext),
+                                                out var value) &&
+                                            value is HttpContext httpContext &&
+                                            context.Result is IQueryResult result)
+                                        {
+                                            var headers = httpContext.Request.Headers;
+                                            if (headers.ContainsKey("sendErrorStatusCode"))
+                                            {
+                                                context.Result = result =
+                                                    QueryResultBuilder
+                                                        .FromResult(result)
+                                                        .SetContextData(HttpStatusCode, 403)
+                                                        .Create();
+                                            }
+
+                                            if (headers.ContainsKey("sendError"))
+                                            {
+                                                context.Result =
+                                                    QueryResultBuilder
+                                                        .FromResult(result)
+                                                        .AddError(new Error("Some error!"))
+                                                        .Create();
+                                            }
+                                        }
+
+                                        await next(context);
+                                    });
+                        })
+                    .Configure(
+                        app =>
+                            app.Use(
+                                    async (ct, next) =>
+                                    {
+                                        try
+                                        {
+                                            // Kestrel does not return proper error responses:
+                                            // https://github.com/aspnet/KestrelHttpServer/issues/43
+                                            await next();
+                                        }
+                                        catch (Exception ex)
+                                        {
+                                            if (ct.Response.HasStarted)
+                                            {
+                                                throw;
+                                            }
 
-                                    ct.Response.StatusCode = 500;
-                                    ct.Response.Headers.Clear();
-                                    await ct.Response.WriteAsync(ex.ToString());
-                                }
-                            })
-                            .UseWebSockets()
-                            .UseRouting()
-                            .UseEndpoints(e => e.MapGraphQL()))
+                                            ct.Response.StatusCode = 500;
+                                            ct.Response.Headers.Clear();
+                                            await ct.Response.WriteAsync(ex.ToString());
+                                        }
+                                    })
+                                .UseWebSockets()
+                                .UseRouting()
+                                .UseEndpoints(e => e.MapGraphQL()))
                     .Build();
 
                 host.Start();
 
                 return host;
             }
-            catch { }
+            catch
+            {
+                // we ignore any errors here and try the next port
+            }
         }
 
         throw new InvalidOperationException("Not port found");