Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Status codes per GraphQL over HTTP spec #1142

Merged
merged 5 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -720,9 +720,11 @@ methods allowing for different options for each configured endpoint.
| `ReadFormOnPost` | Enables parsing of form data for POST requests (may have security implications). | False |
| `ReadQueryStringOnPost` | Enables parsing the query string on POST requests. | True |
| `ReadVariablesFromQueryString` | Enables reading variables from the query string. | True |
| `ValidationErrorsReturnBadRequest` | When enabled, GraphQL requests with validation errors have the HTTP status code set to 400 Bad Request. | True |
| `ValidationErrorsReturnBadRequest` | When enabled, GraphQL requests with validation errors have the HTTP status code set to 400 Bad Request. | Automatic[^1] |
| `WebSockets` | Returns a set of configuration properties for WebSocket connections. | |

[^1]: Automatic mode will return a 200 OK status code when the returned content type is `application/json`; otherwise 400 or as defined by the error.

#### GraphQLWebSocketOptions

| Property | Description | Default value |
Expand Down
9 changes: 9 additions & 0 deletions docs/migration/migration8.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,17 @@
types for the file by using the new `[MediaType]` attribute on the argument or input object field.
- Cross-site request forgery (CSRF) protection has been added for both GET and POST requests,
enabled by default.
- Status codes for validation errors are now, by default, determined by the response content type,
and for authentication errors may return a 401 or 403 status code. These changes are purusant
to the [GraphQL over HTTP specification](https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md).
See the breaking changes section below for more information.

## Breaking changes

- `GraphQLHttpMiddlewareOptions.ValidationErrorsReturnBadRequest` is now a nullable boolean where
`null` means "use the default behavior". The default behavior is to return a 200 status code
when the response content type is `application/json` and a 400 status code otherwise. The
default value for this in v7 was `true`; set this option to retain the v7 behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new default is the only correct behavior. Let's deprecate this configuration, maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, users may likely have clients that count on GraphQL returning 400 even if they use application/json. The GraphQL over http spec only recommends 200 be returned, and only because (in theory) proxy services may return 400 with application/json. Users will know if their configuration meets that criteria or not, and if such a concern is warranted. And other users’ client code may always expect 200 even in the case of an error.

I think this is rather important to keep flexible, just as it was configurable in v7.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client can choose the status codes by using either application/json or application/graphql-response+json. The additional option: using application/json and expecting 4xx codes is not forbidden but discouraged. What can force users to prefer this behavior when they can just change the Accept header?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're assuming that they can control the clients behavior easily. In our company, we have a large infrastructure of GraphQL services that talk to each other. One client may be .NET code, one may be javascript, we even have python code. Changing the format of responses would require us to review each of these different pieces of code scattered throughout our various codebases. Any tests we've written that validate the returned status code may need to change. Since we are not exposing GraphQL to the public, compatibility with the GraphQL over HTTP protocol is not a concern. And if we had a public API, it would be important to maintain behavior of the endpoint throughout the supported version of that API. I've always had our servers return 400 for validation errors, and I expect that to continue.

It is also important to note that Apollo GraphQL server behaves as if this option were false by default, always returning 200 for validation errors (which are not transport errors). For compatibility with other servers alone, we may want to keep this option.

I really don't see any reason this isn't a valid and normal configuration option for the server project, just like changing the default content type returned, enabling/disabling CSRF protection, or enabling/disabling form parsing. Keep in mind that it's just an option, and it's configured to the recommended default per the draft GraphQL over HTTP spec. (The spec isn't even finalized yet!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood

- The validation rules' signatures have changed slightly due to the underlying changes to the
GraphQL.NET library. Please see the GraphQL.NET v8 migration document for more information.
- The obsolete (v6 and prior) authorization validation rule has been removed. See the v7 migration
Expand All @@ -25,6 +33,7 @@
a 400 status code (e.g. the execution of the document has not yet begun), and (2) all errors
in the response prefer the same status code. For practical purposes, this means that the included
errors triggered by the authorization validation rule will now return 401 or 403 when appropriate.
- The `SelectResponseContentType` method now returns a `MediaTypeHeaderValue` instead of a string.

## Other changes

Expand Down
143 changes: 94 additions & 49 deletions src/Transports.AspNetCore/GraphQLHttpMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -583,29 +583,18 @@
// Normal execution with single graphql request
var userContext = await BuildUserContextAsync(context, null);
var result = await ExecuteRequestAsync(context, gqlRequest, context.RequestServices, userContext);
HttpStatusCode statusCode = HttpStatusCode.OK;
// when the request fails validation (this logic does not apply to execution errors)
if (!result.Executed)
{
// always return 405 Method Not Allowed when applicable, as this is a transport problem, not really a validation error,
// even though it occurs during validation (because the query text must be parsed to know if the request is a query or a mutation)
if (result.Errors?.Any(e => e is HttpMethodValidationError) == true)
{
statusCode = HttpStatusCode.MethodNotAllowed;
}
// otherwise use 4xx error codes when configured to do so
else if (_options.ValidationErrorsReturnBadRequest)
{
statusCode = HttpStatusCode.BadRequest;
// if all errors being returned prefer the same status code, use that
if (result.Errors?.Count > 0 && result.Errors[0] is IHasPreferredStatusCode initialError)
{
if (result.Errors.All(e => e is IHasPreferredStatusCode e2 && e2.PreferredStatusCode == initialError.PreferredStatusCode))
statusCode = initialError.PreferredStatusCode;
}
await WriteJsonResponseAsync(context, HttpStatusCode.MethodNotAllowed, result);
return;
}
}
await WriteJsonResponseAsync(context, statusCode, result);
await WriteJsonResponseAsync(context, result);
}

/// <summary>
Expand Down Expand Up @@ -750,10 +739,11 @@
ValueTask<IDictionary<string, object?>?> IUserContextBuilder.BuildUserContextAsync(HttpContext context, object? payload)
=> BuildUserContextAsync(context, payload);

private static readonly MediaTypeHeaderValueMs _applicationJsonMediaType = MediaTypeHeaderValueMs.Parse(CONTENTTYPE_JSON);
private static readonly MediaTypeHeaderValueMs[] _validMediaTypes = new[]
{
MediaTypeHeaderValueMs.Parse(CONTENTTYPE_GRAPHQLRESPONSEJSON),
MediaTypeHeaderValueMs.Parse(CONTENTTYPE_JSON),
_applicationJsonMediaType,
MediaTypeHeaderValueMs.Parse(CONTENTTYPE_GRAPHQLJSON), // deprecated
};

Expand All @@ -771,62 +761,87 @@
/// For more complex behavior patterns, override
/// <see cref="WriteJsonResponseAsync{TResult}(HttpContext, HttpStatusCode, TResult)"/>.
/// </summary>
protected virtual string SelectResponseContentType(HttpContext context)
protected virtual MediaTypeHeaderValueMs SelectResponseContentType(HttpContext context)
{
// pull the Accept header, which may contain multiple content types
var acceptHeaders = context.Request.Headers.ContainsKey(Microsoft.Net.Http.Headers.HeaderNames.Accept)
? context.Request.GetTypedHeaders().Accept
: Array.Empty<MediaTypeHeaderValueMs>();

if (acceptHeaders.Count > 0)
if (acceptHeaders.Count == 1)
{
var response = IsSupportedMediaType(acceptHeaders[0]);
if (response != null)
return response;
}
else if (acceptHeaders.Count > 0)
{
// enumerate through each content type and see if it matches a supported content type
// give priority to specific types, then to types with wildcards
foreach (var acceptHeader in acceptHeaders.OrderBy(x => x.MatchesAllTypes ? 4 : x.MatchesAllSubTypes ? 3 : x.MatchesAllSubTypesWithoutSuffix ? 2 : 1))
{
var response = CheckForMatch(acceptHeader);
var response = IsSupportedMediaType(acceptHeader);
if (response != null)
return response;
}
}

// return the default content type if no match is found, or if there is no 'Accept' header
return _options.DefaultResponseContentTypeString;
return _options.DefaultResponseContentType;
}

string? CheckForMatch(MediaTypeHeaderValueMs acceptHeader)
{
// strip quotes from charset
if (acceptHeader.Charset.Length > 0 && acceptHeader.Charset[0] == '\"' && acceptHeader.Charset[acceptHeader.Charset.Length - 1] == '\"')
{
acceptHeader.Charset = acceptHeader.Charset.Substring(1, acceptHeader.Charset.Length - 2);
}
/// <summary>
/// Checks to see if the specified <see cref="MediaTypeHeaderValueMs"/> matches any of the supported content types
/// by this middleware. If a match is found, the matching content type is returned; otherwise, <see langword="null"/>.
/// Prioritizes <see cref="GraphQLHttpMiddlewareOptions.DefaultResponseContentType"/>, then
/// <c>application/graphql-response+json</c>, then <c>application/json</c>.
/// </summary>
private MediaTypeHeaderValueMs? IsSupportedMediaType(MediaTypeHeaderValueMs acceptHeader)
=> IsSupportedMediaType(acceptHeader, _options.DefaultResponseContentType, _validMediaTypes);

// check if this matches the default content type header
if (IsSubsetOf(_options.DefaultResponseContentType, acceptHeader))
return _options.DefaultResponseContentTypeString;
/// <summary>
/// Checks to see if the specified <see cref="MediaTypeHeaderValueMs"/> matches any of the supported content types
/// by this middleware. If a match is found, the matching content type is returned; otherwise, <see langword="null"/>.
/// Prioritizes <see cref="GraphQLHttpMiddlewareOptions.DefaultResponseContentType"/>, then
/// <c>application/graphql-response+json</c>, then <c>application/json</c>.
/// </summary>
private static MediaTypeHeaderValueMs? IsSupportedMediaType(MediaTypeHeaderValueMs acceptHeader, MediaTypeHeaderValueMs preferredContentType, MediaTypeHeaderValueMs[] allowedContentTypes)
{
// speeds check in WriteJsonResponseAsync
if (acceptHeader == preferredContentType)
return preferredContentType;

// if the default content type header does not contain a charset, test with utf-8 as the charset
if (_options.DefaultResponseContentType.Charset.Length == 0)
{
var contentType2 = _options.DefaultResponseContentType.Copy();
contentType2.Charset = "utf-8";
if (IsSubsetOf(contentType2, acceptHeader))
return contentType2.ToString();
}
// strip quotes from charset
if (acceptHeader.Charset.Length > 0 && acceptHeader.Charset[0] == '\"' && acceptHeader.Charset[acceptHeader.Charset.Length - 1] == '\"')
{
acceptHeader.Charset = acceptHeader.Charset.Substring(1, acceptHeader.Charset.Length - 2);
}

// loop through the other supported media types, attempting to find a match
for (int j = 0; j < _validMediaTypes.Length; j++)
{
var mediaType = _validMediaTypes[j];
if (IsSubsetOf(mediaType, acceptHeader))
// when a match is found, return the match
return mediaType.ToString();
}
// check if this matches the default content type header
if (IsSubsetOf(preferredContentType, acceptHeader))
return preferredContentType;

// if the default content type header does not contain a charset, test with utf-8 as the charset
if (preferredContentType.Charset.Length == 0)
{
var contentType2 = preferredContentType.Copy();
contentType2.Charset = "utf-8";
if (IsSubsetOf(contentType2, acceptHeader))
return contentType2;
}

// no match
return null;
// loop through the other supported media types, attempting to find a match
for (int j = 0; j < allowedContentTypes.Length; j++)
{
var mediaType = allowedContentTypes[j];
if (IsSubsetOf(mediaType, acceptHeader))
// when a match is found, return the match
return mediaType;
}

// no match
return null;

// --- note: the below functions were copied from ASP.NET Core 2.1 source ---
// see https://github.com/dotnet/aspnetcore/blob/v2.1.33/src/Http/Headers/src/MediaTypeHeaderValue.cs

Expand Down Expand Up @@ -940,11 +955,41 @@
}

/// <summary>
/// Writes the specified object (usually a GraphQL response represented as an instance of <see cref="ExecutionResult"/>) as JSON to the HTTP response stream.
/// Writes the specified <see cref="ExecutionResult"/> as JSON to the HTTP response stream,
/// selecting the proper content type and status code based on the request Accept header and response.
/// </summary>
protected virtual Task WriteJsonResponseAsync(HttpContext context, ExecutionResult result)
{
var contentType = SelectResponseContentType(context);
context.Response.ContentType = contentType == _options.DefaultResponseContentType ? _options.DefaultResponseContentTypeString : contentType.ToString();
context.Response.StatusCode = (int)HttpStatusCode.OK;
if (result.Executed == false)

Check notice

Code scanning / CodeQL

Unnecessarily complex Boolean expression Note

The expression 'A == false' can be simplified to '!A'.
{
var useBadRequest = _options.ValidationErrorsReturnBadRequest ?? IsSupportedMediaType(contentType, _applicationJsonMediaType, Array.Empty<MediaTypeHeaderValueMs>()) == null;
if (useBadRequest)
{
context.Response.StatusCode = (int)HttpStatusCode.BadRequest;

// if all errors being returned prefer the same status code, use that
if (result.Errors?.Count > 0 && result.Errors[0] is IHasPreferredStatusCode initialError)
{
if (result.Errors.All(e => e is IHasPreferredStatusCode e2 && e2.PreferredStatusCode == initialError.PreferredStatusCode))
context.Response.StatusCode = (int)initialError.PreferredStatusCode;
}
}
}

return _serializer.WriteAsync(context.Response.Body, result, context.RequestAborted);
}

/// <summary>
/// Writes the specified object (usually a GraphQL response represented as an instance of <see cref="ExecutionResult"/>)
/// as JSON to the HTTP response stream, using the specified status code.
/// </summary>
protected virtual Task WriteJsonResponseAsync<TResult>(HttpContext context, HttpStatusCode httpStatusCode, TResult result)
{
context.Response.ContentType = SelectResponseContentType(context);
var contentType = SelectResponseContentType(context);
context.Response.ContentType = contentType == _options.DefaultResponseContentType ? _options.DefaultResponseContentTypeString : contentType.ToString();
context.Response.StatusCode = (int)httpStatusCode;

return _serializer.WriteAsync(context.Response.Body, result, context.RequestAborted);
Expand Down
16 changes: 13 additions & 3 deletions src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,22 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions

/// <summary>
/// When enabled, GraphQL requests with validation errors have the HTTP status code
/// set to 400 Bad Request or the error status code dictated by the error.
/// GraphQL requests with execution errors are unaffected.
/// set to 400 Bad Request or the error status code dictated by the error, while
/// setting this to <c>false</c> will use a 200 status code for all responses.
/// <br/><br/>
/// GraphQL requests with execution errors are unaffected and return a 200 status code.
/// <br/><br/>
/// Transport errors, such as a transport-level authentication failure, are not affected
/// and return a error-specific status code, such as 405 Method Not Allowed if a mutation
/// is attempted over a HTTP GET connection.
/// <br/><br/>
/// Does not apply to batched or WebSocket requests.
/// <br/><br/>
/// Settings this to <see langword="null"/> will use a 200 status code for
/// <c>application/json</c> responses and use a 4xx status code for
/// <c>application/graphql-response+json</c> and other responses.
/// </summary>
public bool ValidationErrorsReturnBadRequest { get; set; } = true;
public bool? ValidationErrorsReturnBadRequest { get; set; }

/// <summary>
/// Enables parsing the query string on POST requests.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,11 @@ namespace GraphQL.Server.Transports.AspNetCore
"SingleRequest",
"BatchRequest"})]
protected virtual System.Threading.Tasks.Task<System.ValueTuple<GraphQL.Transport.GraphQLRequest?, System.Collections.Generic.IList<GraphQL.Transport.GraphQLRequest?>?>?> ReadPostContentAsync(Microsoft.AspNetCore.Http.HttpContext context, Microsoft.AspNetCore.Http.RequestDelegate next, string? mediaType, System.Text.Encoding? sourceEncoding) { }
protected virtual string SelectResponseContentType(Microsoft.AspNetCore.Http.HttpContext context) { }
protected virtual Microsoft.Net.Http.Headers.MediaTypeHeaderValue SelectResponseContentType(Microsoft.AspNetCore.Http.HttpContext context) { }
protected virtual System.Threading.Tasks.Task WriteErrorResponseAsync(Microsoft.AspNetCore.Http.HttpContext context, GraphQL.ExecutionError executionError) { }
protected virtual System.Threading.Tasks.Task WriteErrorResponseAsync(Microsoft.AspNetCore.Http.HttpContext context, System.Net.HttpStatusCode httpStatusCode, GraphQL.ExecutionError executionError) { }
protected virtual System.Threading.Tasks.Task WriteErrorResponseAsync(Microsoft.AspNetCore.Http.HttpContext context, System.Net.HttpStatusCode httpStatusCode, string errorMessage) { }
protected virtual System.Threading.Tasks.Task WriteJsonResponseAsync(Microsoft.AspNetCore.Http.HttpContext context, GraphQL.ExecutionResult result) { }
protected virtual System.Threading.Tasks.Task WriteJsonResponseAsync<TResult>(Microsoft.AspNetCore.Http.HttpContext context, System.Net.HttpStatusCode httpStatusCode, TResult result) { }
}
public class GraphQLHttpMiddlewareOptions : GraphQL.Server.Transports.AspNetCore.IAuthorizationOptions
Expand All @@ -143,7 +144,7 @@ namespace GraphQL.Server.Transports.AspNetCore
public bool ReadFormOnPost { get; set; }
public bool ReadQueryStringOnPost { get; set; }
public bool ReadVariablesFromQueryString { get; set; }
public bool ValidationErrorsReturnBadRequest { get; set; }
public bool? ValidationErrorsReturnBadRequest { get; set; }
public GraphQL.Server.Transports.AspNetCore.WebSockets.GraphQLWebSocketOptions WebSockets { get; set; }
}
public class GraphQLHttpMiddleware<TSchema> : GraphQL.Server.Transports.AspNetCore.GraphQLHttpMiddleware
Expand Down
Loading
Loading