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 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
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
28 changes: 9 additions & 19 deletions docs/migration/migration7.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

- Configuration simplified to a single line of code
- Single middleware to support GET, POST and WebSocket connections (configurable)
- Media type of 'application/graphql-response+json' is accepted and returned as recommended by the draft spec (configurable via virtual method)
- Media type of 'application/graphql-response+json' is returned as recommended by the draft spec (configurable)
- Batched requests will execute in parallel within separate service scopes (configurable)
- Authorization rules can be set on endpoints, regardless of schema configuration
- Mutation requests are disallowed over GET connections, as required by the spec
Expand Down Expand Up @@ -188,27 +188,17 @@ app.UseGraphQL<MySchema>("/graphqlsubscription", o => {
<details><summary>To retain prior media type of `application/json`</summary><p>

```csharp
class MyMiddleware<TSchema> : GraphQLHttpMiddleware<TSchema>
where TSchema : ISchema
{
public MyMiddleware(
RequestDelegate next,
IGraphQLTextSerializer serializer,
IDocumentExecuter<TSchema> documentExecuter,
IServiceScopeFactory serviceScopeFactory,
GraphQLHttpMiddlewareOptions options,
IHostApplicationLifetime hostApplicationLifetime)
: base(next, serializer, documentExecuter, serviceScopeFactory, options, hostApplicationLifetime)
{
}
// with no charset specified
app.UseGraphQL("/graphql", o => o.DefaultResponseContentType = new("application/json"));

protected override string SelectResponseContentType(HttpContext context)
=> "application/json";
}

app.UseGraphQL<MyMiddleware<ISchema>>("/graphql", new GraphQLHttpMiddlewareOptions());
// with utf-8 charset specified
app.UseGraphQL("/graphql", o => o.DefaultResponseContentType = new("application/json") { Charset = "utf-8" });
```

Note that if a request is received with a specific supported media type such as `application/graphql-response+json`,
sungam3r marked this conversation as resolved.
Show resolved Hide resolved
then the supported media type will be returned rather than the default. Override the `SelectResponseContentType`
method within the middleware for more precise control of the Content-Type header in the response.

</p></details>

<details><summary>If you had code within the `RequestExecutedAsync` protected method</summary><p>
Expand Down
194 changes: 187 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,192 @@ 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
// 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);
if (response != null)
return response;
}
Comment on lines +482 to +487

Check notice

Code scanning / CodeQL

Missed opportunity to use Select

This foreach loop immediately maps its iteration variable to another variable [here](1) - consider mapping the sequence explicitly using '.Select(...)'.
}

// return the default content type if no match is found, or if there is no 'Accept' header
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what if client specified "application/pdf" or "application/some_unknown_mime" in accept header?

From https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2

A request without any Accept header field implies that the user agent
will accept any media type in response. If the header field is
present in a request and none of the available representations for
the response have a media type that is listed as acceptable, the
origin server can either honor the header field by sending a 406 (Not
Acceptable) response or disregard the header field by treating the
response as if it is not subject to content negotiation.

Do you want to go with second option?

Copy link
Member Author

Choose a reason for hiding this comment

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

That’s the case now (merged in this PR). Please read my response to your comments on this earlier today. TLDR: maybe update in v8 to reject with 406

return _options.DefaultResponseContentType.ToString();
Copy link
Member

Choose a reason for hiding this comment

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

public override string ToString()
        {
            StringBuilder stringBuilder = new StringBuilder();
            stringBuilder.Append(_mediaType);
            NameValueHeaderValue.ToString(_parameters, ';', leadingSeparator: true, stringBuilder);
            return stringBuilder.ToString();
        }

😕 called each time


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

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

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

// 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;
sungam3r marked this conversation as resolved.
Show resolved Hide resolved

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);
sungam3r marked this conversation as resolved.
Show resolved Hide resolved
}
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; }
sungam3r marked this conversation as resolved.
Show resolved Hide resolved
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