Reworked Status Code Behavior (#7117)

Author: michaelstaib

src/HotChocolate/AspNetCore/src/AspNetCore/Serialization/DefaultHttpResponseFormatter.cs

@@ -184,13 +184,13 @@ public async ValueTask FormatAsync(
             case OperationResultBatch resultBatch:
             {
                 var statusCode = (int)OnDetermineStatusCode(resultBatch, format, proposedStatusCode);
-                
+
                 response.ContentType = format.ContentType;
                 response.StatusCode = statusCode;
                 response.Headers.CacheControl = HttpHeaderValues.NoCache;
                 OnWriteResponseHeaders(resultBatch, format, response.Headers);
                 await response.Body.FlushAsync(cancellationToken);
-                
+
                 await format.Formatter.FormatAsync(result, response.Body, cancellationToken);
                 break;
             }
@@ -284,7 +284,7 @@ protected virtual HttpStatusCode OnDetermineStatusCode(
                 }
 
                 // next we check if the validation of the request failed.
-                // if that is the case we will we will return a BadRequest status code (400).
+                // if that is the case we will return a BadRequest status code (400).
                 if (contextData.ContainsKey(ValidationErrors))
                 {
                     return HttpStatusCode.BadRequest;
@@ -296,22 +296,21 @@ protected virtual HttpStatusCode OnDetermineStatusCode(
                 }
             }
 
-            // if data is not null then we have a valid result. The result of executing
-            // a GraphQL operation may contain partial data as well as encountered errors.
-            // Errors that happen during execution of the GraphQL operation typically
-            // become part of the result, as long as the server is still able to produce
-            // a well-formed response.
-            if (result.Data is not null)
+            // if data is set then the execution as begun and has produced a result.
+            // The result of executing GraphQL operation may contain partial data as
+            // well as encountered errors. Errors that happen during execution of the
+            // GraphQL operation typically become part of the result, as long as the
+            // server is still able to produce a well-formed response.
+            // Even null represents a valid response, in this case of a non-null propagation
+            // that erased the result.
+            if (result.IsDataSet)
             {
                 return HttpStatusCode.OK;
             }
 
-            // if data is null we consider the result not valid and return a 500 if the user did
-            // not override the status code with a different status code.
-            // this is however at the moment a point of discussion as there are opposing views
-            // towards what constitutes a valid response.
-            // we will update this status code as the spec moves towards release.
-            return HttpStatusCode.InternalServerError;
+            // if data was never set the result not valid and execution has never started, and we return a 400
+            // if the user did not override the status code with a different status code.
+            return HttpStatusCode.BadRequest;
         }
 
         // we allow for users to implement alternative protocols or response content-type.
@@ -389,7 +388,7 @@ protected virtual void OnWriteResponseHeaders(
         IResponseStream responseStream,
         FormatInfo format,
         IHeaderDictionary headers) { }
-    
+
     /// <summary>
     /// Determines which status code shall be returned for a result batch.
     /// </summary>
@@ -626,9 +625,6 @@ private enum ResultKind
         Subscription,
     }
 
-    private sealed class SealedDefaultHttpResponseFormatter : DefaultHttpResponseFormatter
-    {
-        public SealedDefaultHttpResponseFormatter(HttpResponseFormatterOptions options)
-            : base(options) { }
-    }
+    private sealed class SealedDefaultHttpResponseFormatter(HttpResponseFormatterOptions options)
+        : DefaultHttpResponseFormatter(options);
 }

src/HotChocolate/AspNetCore/test/AspNetCore.Tests.Utilities/ServerTestBase.cs

@@ -76,6 +76,16 @@ protected virtual TestServer CreateStarWarsServer(
                         .AddDiagnosticEventListener(_ => new SubscriptionTestDiagnostics(output));
                 }
 
+                services
+                    .AddGraphQLServer("notnull")
+                    .AddQueryType(c =>
+                    {
+                        c.Name("Query");
+                        c.Field("error")
+                            .Type<NonNullType<StringType>>()
+                            .Resolve(_ => Task.FromResult<object?>(null!));
+                    });
+
                 configureServices?.Invoke(services);
             },
             app => app
@@ -87,7 +97,7 @@ protected virtual TestServer CreateStarWarsServer(
 #if NET8_0_OR_GREATER
                         endpoints.MapGraphQLPersistedOperations();
 #endif
-                        
+
                         var builder = endpoints.MapGraphQL(pattern)
                             .WithOptions(new GraphQLServerOptions
                             {
@@ -96,6 +106,7 @@ protected virtual TestServer CreateStarWarsServer(
                             });
 
                         configureConventions?.Invoke(builder);
+                        endpoints.MapGraphQL("/notnull", "notnull");
                         endpoints.MapGraphQL("/evict", "evict");
                         endpoints.MapGraphQL("/arguments", "arguments");
                         endpoints.MapGraphQL("/upload", "upload");

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/GraphQLOverHttpSpecTests.cs

@@ -425,7 +425,7 @@ public async Task DeferredQuery_EventStream(string acceptHeader)
     }
 
     [Fact]
-    public async Task DefferedQuery_NoStreamableAcceptHeader()
+    public async Task DeferredQuery_NoStreamableAcceptHeader()
     {
         // arrange
         var server = CreateStarWarsServer();
@@ -591,6 +591,24 @@ public async Task VariableBatch()
         await snapshot.MatchMarkdownAsync();
     }
 
+    [Fact]
+    public async Task After_Execution_StatusCode_Is_200()
+    {
+        // arrange
+        var server = CreateStarWarsServer();
+        var client = new DefaultGraphQLHttpClient(server.CreateClient());
+
+        // act
+        var request = new GraphQLHttpRequest(
+            new OperationRequest("{ error }"),
+            new Uri("http://localhost:5000/notnull"));
+
+        using var response = await client.SendAsync(request);
+
+        // assert
+        Assert.Equal(response.StatusCode, OK);
+    }
+
     private HttpClient GetClient(HttpTransportVersion serverTransportVersion)
     {
         var server = CreateStarWarsServer(

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/__snapshots__/HttpGetMiddlewareTests.SingleRequest_CreateReviewForEpisode_Omit_NonNull_Variable.snap

@@ -1,6 +1,6 @@
 {
   "ContentType": "application/graphql-response+json; charset=utf-8",
-  "StatusCode": "InternalServerError",
+  "StatusCode": "BadRequest",
   "Data": null,
   "Errors": [
     {

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/__snapshots__/HttpGetMiddlewareTests.Throw_Custom_GraphQL_Error.snap

@@ -1,6 +1,6 @@
 {
   "ContentType": "application/graphql-response+json; charset=utf-8",
-  "StatusCode": "InternalServerError",
+  "StatusCode": "BadRequest",
   "Data": null,
   "Errors": [
     {

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/__snapshots__/HttpPostMiddlewareTests.SingleRequest_CreateReviewForEpisode_Omit_NonNull_Variable.snap

@@ -1,6 +1,6 @@
 {
   "ContentType": "application/graphql-response+json; charset=utf-8",
-  "StatusCode": "InternalServerError",
+  "StatusCode": "BadRequest",
   "Data": null,
   "Errors": [
     {

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/__snapshots__/HttpPostMiddlewareTests.Throw_Custom_GraphQL_Error.snap

@@ -1,6 +1,6 @@
 {
   "ContentType": "application/graphql-response+json; charset=utf-8",
-  "StatusCode": "InternalServerError",
+  "StatusCode": "BadRequest",
   "Data": null,
   "Errors": [
     {

src/HotChocolate/Diagnostics/src/Diagnostics/Scopes/ExecuteOperationScope.cs

@@ -19,15 +19,15 @@ protected override void EnrichActivity()
 
     protected override void SetStatus()
     {
-        if (Context.Result is not null and not IOperationResult { Data: null, })
+        if (Context.Result is null or IOperationResult { Errors: [_, ..] })
         {
-            Activity.SetStatus(Status.Ok);
-            Activity.SetStatus(ActivityStatusCode.Ok);
+            Activity.SetStatus(Status.Error);
+            Activity.SetStatus(ActivityStatusCode.Error);
         }
         else
         {
-            Activity.SetStatus(Status.Error);
-            Activity.SetStatus(ActivityStatusCode.Error);
+            Activity.SetStatus(Status.Ok);
+            Activity.SetStatus(ActivityStatusCode.Ok);
         }
     }
 }

src/HotChocolate/Diagnostics/src/Diagnostics/Scopes/ExecuteRequestScope.cs

@@ -19,7 +19,7 @@ protected override void EnrichActivity()
 
     protected override void SetStatus()
     {
-        if (Context.Result is IOperationResult { Data: null, })
+        if (Context.Result is null or IOperationResult { Errors: [_, ..] })
         {
             Activity.SetStatus(Status.Error);
             Activity.SetStatus(ActivityStatusCode.Error);