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

Select return content type based on Accept header #919

Merged
merged 10 commits into from
Sep 3, 2022
Merged
Show file tree
Hide file tree
Changes from 8 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
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ methods allowing for different options for each configured endpoint.
| `AuthorizationRequired` | Requires `HttpContext.User` to represent an authenticated user. | False |
| `AuthorizedPolicy` | If set, requires `HttpContext.User` to pass authorization of the specified policy. | |
| `AuthorizedRoles` | If set, requires `HttpContext.User` to be a member of any one of a list of roles. | |
| `DefaultResponseContentType` | Sets the default response content type used within responses. | `application/graphql-response+json; charset=utf-8` |
| `EnableBatchedRequests` | Enables handling of batched GraphQL requests for POST requests when formatted as JSON. | True |
| `ExecuteBatchedRequestsInParallel` | Enables parallel execution of batched GraphQL requests. | True |
| `HandleGet` | Enables handling of GET requests. | True |
Expand Down Expand Up @@ -672,7 +673,7 @@ A list of methods are as follows:
| `HandleWebSocketSubProtocolNotSupportedAsync` | Writes a '400 Invalid WebSocket sub-protocol.' message to the output. |

Below is a sample of custom middleware to change the response content type to `application/json`,
rather than the default of `application/graphql-response+json`:
regardless of the value of the HTTP 'Accept' header or default value set in the options:

```csharp
class MyMiddleware<TSchema> : GraphQLHttpMiddleware<TSchema>
Expand Down
186 changes: 179 additions & 7 deletions src/Transports.AspNetCore/GraphQLHttpMiddleware.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#pragma warning disable CA1716 // Identifiers should not match keywords

using Microsoft.Extensions.Primitives;
using MediaTypeHeaderValueMs = Microsoft.Net.Http.Headers.MediaTypeHeaderValue;
Copy link
Member

Choose a reason for hiding this comment

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

Why alias?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two different MediaTypeHeaderValue classes both within scope (if I recall correctly).


namespace GraphQL.Server.Transports.AspNetCore;

/// <inheritdoc/>
Expand Down Expand Up @@ -51,10 +54,12 @@ public class GraphQLHttpMiddleware : IUserContextBuilder
private const string VARIABLES_KEY = "variables";
private const string EXTENSIONS_KEY = "extensions";
private const string OPERATION_NAME_KEY = "operationName";
private const string MEDIATYPE_GRAPHQLJSON = "application/graphql+json";
private const string MEDIATYPE_GRAPHQLJSON = "application/graphql+json"; // deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Techically, it is not deprecated. It just does not exist anymore since they removed it from draft spec.

private const string MEDIATYPE_JSON = "application/json";
private const string MEDIATYPE_GRAPHQL = "application/graphql";
internal const string CONTENTTYPE_GRAPHQLJSON = "application/graphql-response+json; charset=utf-8";
internal const string CONTENTTYPE_JSON = "application/json; charset=utf-8";
internal const string CONTENTTYPE_GRAPHQLJSON = "application/graphql+json; charset=utf-8"; // deprecated
internal const string CONTENTTYPE_GRAPHQLRESPONSEJSON = "application/graphql-response+json; charset=utf-8";

/// <summary>
/// Initializes a new instance.
Expand Down Expand Up @@ -195,7 +200,7 @@ public virtual async Task InvokeAsync(HttpContext context)

switch (mediaType?.ToLowerInvariant())
{
case MEDIATYPE_GRAPHQLJSON:
case MEDIATYPE_GRAPHQLJSON: // deprecated
case MEDIATYPE_JSON:
IList<GraphQLRequest?>? deserializationResult;
try
Expand Down Expand Up @@ -444,17 +449,184 @@ protected virtual async Task<ExecutionResult> ExecuteRequestAsync(HttpContext co
ValueTask<IDictionary<string, object?>?> IUserContextBuilder.BuildUserContextAsync(HttpContext context, object? payload)
=> BuildUserContextAsync(context, payload);

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

/// <summary>
/// Selects a response content type string based on the <see cref="HttpContext"/>.
/// Defaults to <see cref="CONTENTTYPE_GRAPHQLJSON"/>. Override this value for compatibility
/// with non-conforming GraphQL clients.
/// The default implementation attempts to match the content-type requested by the
/// client through the 'Accept' HTTP header to the default content type specified
/// within <see cref="GraphQLHttpMiddlewareOptions.DefaultResponseContentType"/>.
/// If matched, the specified content-type is returned; if not, supported
/// content-types are tested ("application/json", "application/graphql+json", and
/// "application/graphql-response+json") to see if they match the 'Accept' header.
/// <br/><br/>
/// Note that by default, the response will be written as UTF-8 encoded JSON, regardless
/// of the content-type value here. For more complex behavior patterns, override
/// of the content-type value here, and this method's default implementation assumes as much.
/// For more complex behavior patterns, override
/// <see cref="WriteJsonResponseAsync{TResult}(HttpContext, HttpStatusCode, TResult)"/>.
/// </summary>
protected virtual string SelectResponseContentType(HttpContext context)
=> _options.ResponseContentType;
{
// pull the Accept header, which may contain multiple content types
var acceptHeaders = context.Request.GetTypedHeaders().Accept;

if (acceptHeaders != null)
{
// enumerate through each content type and see if it matches a supported content type
for (int i = 0; i < acceptHeaders.Count; i++)
{
// pull the Accept header to be checked
var acceptHeader = acceptHeaders[i];

// 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);
}

// check if this matches the default content type header
if (IsSubsetOf(_options.DefaultResponseContentType, acceptHeader))
return _options.DefaultResponseContentType.ToString();

// 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();
}

// 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();
}
}
}

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

// --- 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

// The ASP.NET Core 6.0 source contains logic that is not suitable -- it will consider
// "application/graphql-response+json" to match an 'Accept' header of "application/json",
// which can break client applications.

/*
* Copyright (c) .NET Foundation. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use
* these files except in compliance with the License. You may obtain a copy of the
* License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed
* under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
*/

static bool IsSubsetOf(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs otherMediaType)
{
// "text/plain" is a subset of "text/plain", "text/*" and "*/*". "*/*" is a subset only of "*/*".
return MatchesType(mediaType, otherMediaType) &&
MatchesSubtype(mediaType, otherMediaType) &&
MatchesParameters(mediaType, otherMediaType);
}

static bool MatchesType(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs set)
{
return set.MatchesAllTypes ||
set.Type.Equals(mediaType.Type, StringComparison.OrdinalIgnoreCase);
}

static bool MatchesSubtype(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs set)
{
if (set.MatchesAllSubTypes)
{
return true;
}
if (set.Suffix.HasValue)
{
if (mediaType.Suffix.HasValue)
{
return MatchesSubtypeWithoutSuffix(mediaType, set) && MatchesSubtypeSuffix(mediaType, set);
}
else
{
return false;
}
Comment on lines +572 to +579

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement return - consider using '?' to express intent better.
}
else
{
return set.SubType.Equals(mediaType.SubType, StringComparison.OrdinalIgnoreCase);
}
}

static bool MatchesSubtypeWithoutSuffix(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs set)
{
return set.MatchesAllSubTypesWithoutSuffix ||
set.SubTypeWithoutSuffix.Equals(mediaType.SubTypeWithoutSuffix, StringComparison.OrdinalIgnoreCase);
}

static bool MatchesParameters(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs set)
{
if (set.Parameters.Count != 0)
{
// Make sure all parameters in the potential superset are included locally. Fine to have additional
// parameters locally; they make this one more specific.
foreach (var parameter in set.Parameters)
{
if (parameter.Name.Equals("*", StringComparison.OrdinalIgnoreCase))
{
// A parameter named "*" has no effect on media type matching, as it is only used as an indication
// that the entire media type string should be treated as a wildcard.
continue;
}

if (parameter.Name.Equals("q", StringComparison.OrdinalIgnoreCase))
{
// "q" and later parameters are not involved in media type matching. Quoting the RFC: The first
// "q" parameter (if any) separates the media-range parameter(s) from the accept-params.
break;
}

var localParameter = Microsoft.Net.Http.Headers.NameValueHeaderValue.Find(mediaType.Parameters, parameter.Name);
if (localParameter == null)
{
// Not found.
return false;
}

if (!StringSegment.Equals(parameter.Value, localParameter.Value, StringComparison.OrdinalIgnoreCase))
{
return false;
}
}
}
return true;
}

static bool MatchesSubtypeSuffix(MediaTypeHeaderValueMs mediaType, MediaTypeHeaderValueMs set)
// We don't have support for wildcards on suffixes alone (e.g., "application/entity+*")
// because there's no clear use case for it.
=> set.Suffix.Equals(mediaType.Suffix, StringComparison.OrdinalIgnoreCase);

// --- end of ASP.NET Core 2.1 copied functions ---
}

/// <summary>
/// Writes the specified object (usually a GraphQL response represented as an instance of <see cref="ExecutionResult"/>) as JSON to the HTTP response stream.
Expand Down
7 changes: 5 additions & 2 deletions src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using MediaTypeHeaderValueMs = Microsoft.Net.Http.Headers.MediaTypeHeaderValue;

namespace GraphQL.Server.Transports.AspNetCore;

/// <summary>
Expand Down Expand Up @@ -95,7 +97,8 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions
public GraphQLWebSocketOptions WebSockets { get; set; } = new();

/// <summary>
/// The Content-Type to use for GraphQL responses
/// The Content-Type to use for GraphQL responses, if it matches the 'Accept'
Copy link
Member

Choose a reason for hiding this comment

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

I read it several times and this comment still seems strange to me. I don't understand the meaning of matches.

Copy link
Member

Choose a reason for hiding this comment

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

I mean in generally I understand what you want to say but from the point of view of some "external" developer it may look strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do the best I can. Sometimes I'm tongue tied. Here it seemed to make sense to me, but I get it. Feel free to suggest an improvement.

/// HTTP request header. Defaults to "application/graphql-response+json; charset=utf-8".
/// </summary>
public string ResponseContentType { get; set; } = GraphQLHttpMiddleware.CONTENTTYPE_GRAPHQLJSON;
public MediaTypeHeaderValueMs DefaultResponseContentType { get; set; } = MediaTypeHeaderValueMs.Parse(GraphQLHttpMiddleware.CONTENTTYPE_GRAPHQLRESPONSEJSON);
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ namespace GraphQL.Server.Transports.AspNetCore
public bool AuthorizationRequired { get; set; }
public string? AuthorizedPolicy { get; set; }
public System.Collections.Generic.List<string> AuthorizedRoles { get; set; }
public Microsoft.Net.Http.Headers.MediaTypeHeaderValue DefaultResponseContentType { get; set; }
public bool EnableBatchedRequests { get; set; }
public bool ExecuteBatchedRequestsInParallel { get; set; }
public bool HandleGet { get; set; }
Expand All @@ -109,7 +110,6 @@ namespace GraphQL.Server.Transports.AspNetCore
public bool ReadExtensionsFromQueryString { get; set; }
public bool ReadQueryStringOnPost { get; set; }
public bool ReadVariablesFromQueryString { get; set; }
public string ResponseContentType { get; set; }
public bool ValidationErrorsReturnBadRequest { get; set; }
public GraphQL.Server.Transports.AspNetCore.WebSockets.GraphQLWebSocketOptions WebSockets { get; set; }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ namespace GraphQL.Server.Transports.AspNetCore
public bool AuthorizationRequired { get; set; }
public string? AuthorizedPolicy { get; set; }
public System.Collections.Generic.List<string> AuthorizedRoles { get; set; }
public Microsoft.Net.Http.Headers.MediaTypeHeaderValue DefaultResponseContentType { get; set; }
public bool EnableBatchedRequests { get; set; }
public bool ExecuteBatchedRequestsInParallel { get; set; }
public bool HandleGet { get; set; }
Expand All @@ -116,7 +117,6 @@ namespace GraphQL.Server.Transports.AspNetCore
public bool ReadExtensionsFromQueryString { get; set; }
public bool ReadQueryStringOnPost { get; set; }
public bool ReadVariablesFromQueryString { get; set; }
public string ResponseContentType { get; set; }
public bool ValidationErrorsReturnBadRequest { get; set; }
public GraphQL.Server.Transports.AspNetCore.WebSockets.GraphQLWebSocketOptions WebSockets { get; set; }
}
Expand Down
Loading