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

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Aug 26, 2022

No description provided.

@Shane32 Shane32 requested a review from sungam3r August 26, 2022 23:58
@Shane32 Shane32 self-assigned this Aug 26, 2022
@github-actions github-actions bot added the test label Aug 26, 2022
@Shane32 Shane32 linked an issue Aug 28, 2022 that may be closed by this pull request
@Shane32
Copy link
Member Author

Shane32 commented Aug 28, 2022

@sungam3r It seems that there may be many applications which require application/json as the content-type rather than application/graphql-response+json. As such, I'm considering this a critical bug and will be releasing 7.1 with this fix shortly if you have no comments. I would really appreciate comments on the implementation - see issue #917 .

}
}

return CONTENTTYPE_GRAPHQLRESPONSEJSON;
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem here is that if you only have a single misbehaving client, then you would need to subclass the GraphQLHttpMiddleware and recreate the logic above with a different fall-back. It would still be really good to be able to just override the default response content type.

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 agree.

Base automatically changed from change_default_response_content_type to master September 2, 2022 13:01
Comment on lines +564 to +571
if (mediaType.Suffix.HasValue)
{
return MatchesSubtypeWithoutSuffix(mediaType, set) && MatchesSubtypeSuffix(mediaType, set);
}
else
{
return false;
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement return - consider using '?' to express intent better.
Comment on lines +482 to +487
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;
}

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(...)'.
@codecov-commenter
Copy link

Codecov Report

Merging #919 (e01c2fa) into master (d80414d) will decrease coverage by 0.08%.
The diff coverage is 93.47%.

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   95.18%   95.09%   -0.09%     
==========================================
  Files          42       42              
  Lines        1992     2078      +86     
  Branches      336      358      +22     
==========================================
+ Hits         1896     1976      +80     
- Misses         55       59       +4     
- Partials       41       43       +2     
Impacted Files Coverage Δ
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs 95.13% <93.40%> (-0.50%) ⬇️
...nsports.AspNetCore/GraphQLHttpMiddlewareOptions.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Shane32
Copy link
Member Author

Shane32 commented Sep 2, 2022

@jeffw-wherethebitsroam Want to review these changes for me? I think I have the logic figured out, but just wondering if perhaps the MediaTypeHeaderValue type would be better than string for the option, considering the comparison logic now included here.

@sungam3r I would appreciate your comments, but have not heard from you in the last week. I will wait another 24 hours for a review before merging these changes and releasing 7.1. Thanks!

General logic this PR now has:

  1. Loops through each media type listed in the Accept header
  2. Prioritizes so that specific media types are given priority, whereas wildcard media types are given the least priority
  3. For each type listed:
    a. Checks if it matches the default content type. Includes matching on charset. So a request of application/json will match on a default content type of application/json; charset=utf-8.
    b. Checks if it matches application/graphql+response+json; charset=utf-8.
    c. Checks if it matches application/json; charset=utf-8.
    d. Checks if it matches the deprecated application/graphql+json; charset=utf-8.
  4. The first match found is returned; if no matches are found, the default content type is returned.

This means that by default, these responses are given:

Accept request header Content-Type response header
(none) application/graphql-response+json; charset=utf-8
*/* application/graphql-response+json; charset=utf-8
application/* application/graphql-response+json; charset=utf-8
application/*+json application/graphql-response+json; charset=utf-8
application/graphql-response+json application/graphql-response+json; charset=utf-8
application/graphql+json application/graphql+json; charset=utf-8
application/json application/json; charset=utf-8
*/*, application/json application/json; charset=utf-8

By changing the default to application/json, the prior default, then you get these results:

Accept request header Content-Type response header
(none) application/json
*/* application/json
application/* application/json
application/*+json application/graphql-response+json; charset=utf-8
application/graphql-response+json application/graphql-response+json; charset=utf-8
application/graphql+json application/graphql+json; charset=utf-8
application/json application/json
*/*, application/json application/json

Since 7.0 is only a couple weeks old, the migration document was modified to reflect changes from 6.x to 7.1.

@Shane32 Shane32 added enhancement New feature or request and removed test labels Sep 2, 2022
@Shane32 Shane32 merged commit 9fe3325 into master Sep 3, 2022
@Shane32 Shane32 deleted the smart_content_type branch September 3, 2022 16:05
@Shane32 Shane32 added this to the 7.0.1 milestone Sep 3, 2022
@jeffw-wherethebitsroam
Copy link
Contributor

Hi @Shane32. Busy weekend, so didn't get a chance to look before now, but it looks good to me.

@bithavoc
Copy link

Thanks so much, this fixed an issue while trying to consume from graphql-request where it always expects application/json

Comment on lines +68 to +150
[Theory]
[InlineData(null, "application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData(null, "application/json", "application/json; charset=utf-8")]
[InlineData(null, "application/json; charset=utf-8", "application/json; charset=utf-8")]
[InlineData(null, "application/json; charset=UTF-8", "application/json; charset=utf-8")]
[InlineData(null, "APPLICATION/JSON", "application/json; charset=utf-8")]
[InlineData(null, "APPLICATION/JSON; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData(null, "*/*; CHARSET=\"UTF-8\" ", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*+json; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/json; charset=utf-7", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/json", "application/json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/json; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/json; charset=UTF-8", "application/json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "APPLICATION/JSON", "application/json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "APPLICATION/JSON; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "*/*; CHARSET=\"UTF-8\" ", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/*; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/*+json; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql-response+json; charset=utf-8", "application/json; charset=utf-7", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/json", "application/json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/json; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/json; charset=UTF-8", "application/json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "APPLICATION/JSON", "application/json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "APPLICATION/JSON; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "*/*; CHARSET=\"UTF-8\" ", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/*; charset=utf-8", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/*+json; charset=utf-8", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/pdf", "application/graphql+json; charset=utf-8")]
[InlineData("application/graphql+json; charset=utf-8", "application/json; charset=utf-7", "application/graphql+json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/json", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/json; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/json; charset=UTF-8", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "APPLICATION/JSON", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "APPLICATION/JSON; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "*/*; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/*; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/*+json; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/pdf", "application/json; charset=utf-8")]
[InlineData("application/json; charset=utf-8", "application/json; charset=utf-7", "application/json; charset=utf-8")]
[InlineData("application/json", "application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json", "application/json", "application/json")]
[InlineData("application/json", "application/json; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/json", "application/json; charset=UTF-8", "application/json; charset=utf-8")]
[InlineData("application/json", "APPLICATION/JSON", "application/json")]
[InlineData("application/json", "APPLICATION/JSON; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/json", "*/*; CHARSET=\"UTF-8\" ", "application/json; charset=utf-8")]
[InlineData("application/json", "application/*; charset=utf-8", "application/json; charset=utf-8")]
[InlineData("application/json", "application/*+json; charset=utf-8", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json", "application/pdf", "application/json")]
[InlineData("application/json", "application/json; charset=utf-7", "application/json")]
[InlineData(null, "*/*, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "*/*, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData(null, "*/*, application/json", "application/json; charset=utf-8")]
[InlineData(null, "*/*, application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData(null, "application/*, application/json", "application/json; charset=utf-8")]
[InlineData(null, "application/*, application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*+json, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*+json, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData(null, "application/*+json, application/json", "application/json; charset=utf-8")]
[InlineData(null, "application/*+json, application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/graphql+json, application/json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json", "*/*, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json", "*/*, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json", "*/*, application/json", "application/json")]
[InlineData("application/json", "*/*, application/pdf", "application/json")]
[InlineData("application/json", "application/*, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json", "application/*, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json", "application/*, application/json", "application/json")]
[InlineData("application/json", "application/*, application/pdf", "application/json")]
[InlineData("application/json", "application/*+json, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json", "application/*+json, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData("application/json", "application/*+json, application/json", "application/json")]
[InlineData("application/json", "application/*+json, application/pdf", "application/graphql-response+json; charset=utf-8")]
[InlineData("application/json", "application/graphql+json, application/json", "application/graphql+json; charset=utf-8")]
public async Task AcceptHeaderHonored(string? defaultMediaType, string mediaType, string expected)
Copy link
Member

Choose a reason for hiding this comment

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

wooooooow

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 know I like my [Theory]s...

[InlineData(null, "application/*+json, application/graphql-response+json", "application/graphql-response+json; charset=utf-8")]
[InlineData(null, "application/*+json, application/graphql+json", "application/graphql+json; charset=utf-8")]
[InlineData(null, "application/*+json, application/json", "application/json; charset=utf-8")]
[InlineData(null, "application/*+json, application/pdf", "application/graphql-response+json; charset=utf-8")]
Copy link
Member

Choose a reason for hiding this comment

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

I think thre should be test for "application/pdf" accept header when server returns error. From https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2:
изображение

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the draft spec (as of today; it's a moving target!):

In alignment with the HTTP 1.1 Accept specification, when a client does not include at least one supported media type in the Accept HTTP header, the server MUST either:

  • Disregard the Accept header and respond with the default media type of application/json, specifying this in the Content-Type header; OR
  • Respond with a 406 Not Acceptable status code and stop processing the request.

I elected the first option. I would agree that the latter would be better, but I was thinking this change would come in 8.0, as it would involve processing the header earlier in the flow and so would change the design of the middleware.

We could also support additional character encodings; we do so for requests -- why not responses also? I have no urgency to do so; you're the only one I know of that mentioned using charsets besides utf-8 -- just an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: as it relates to RFC 7231, we are in compliance now -- see where it says "or disregard the header field...". Again, I like better the use of status code 406, but probably implement it in version 8, reconfiguring the middleware to perform content type negotiation prior to execution of the GraphQL request.

Copy link
Member

Choose a reason for hiding this comment

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

agree

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Regarding encodings - spec become rather stric about encoding:
изображение

MUST is strong requirement )

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -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.

@@ -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.

@@ -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).

}
}

// 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 the default content type if no match is found, or if there is no 'Accept' header
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

@sungam3r
Copy link
Member

Reviewed. LGTM. I left some comments and posted #931

Kent-Chen-Conning added a commit to Conning/graphql-dotnet-server that referenced this pull request Oct 28, 2022
…4-upgrade-to-7.1

* commit 'af5ac03f9f5505d476850cee23c5aaa145110c94': (352 commits)
  Update response content type for `ExecutionResultActionResult` (graphql-dotnet#923)
  Bump Serilog.Sinks.Console from 4.0.1 to 4.1.0 (graphql-dotnet#924)
  Select return content type based on Accept header (graphql-dotnet#919)
  Allow the configuration of the response content type via options (graphql-dotnet#913)
  Change default response content type to match draft spec (graphql-dotnet#918)
  Add note regarding ASP.NET Core 2.1 support policy (graphql-dotnet#914)
  Bump Microsoft.NET.Test.Sdk from 17.3.0 to 17.3.1 (graphql-dotnet#920)
  Bump Shouldly from 4.0.3 to 4.1.0 (graphql-dotnet#912)
  Bump GraphQL to 7.0.2 (graphql-dotnet#911)
  Add file upload/download documentation to readme (graphql-dotnet#909)
  Fix packing readme and analysis recommendations (graphql-dotnet#897)
  Fix call to UseApolloTracing in Complex sample (graphql-dotnet#903)
  Add docs link
  Update migration document link (graphql-dotnet#895)
  Add federation tracing extension method (graphql-dotnet#894)
  Bump to GraphQL.NET 7.0.0 (graphql-dotnet#893)
  Feature/assembly-strongname (graphql-dotnet#892)
  Add JWT bearer authentication sample (graphql-dotnet#885)
  Bump Microsoft.AspNetCore.TestHost from 6.0.0 to 6.0.8 (graphql-dotnet#889)
  Bump Microsoft.NET.Test.Sdk from 17.2.0 to 17.3.0 (graphql-dotnet#890)
  ...

# Conflicts:
#	src/Transports.AspNetCore/GraphQLExtensions.cs
#	src/Transports.AspNetCore/GraphQLHttpMiddleware.cs
#	src/Transports.AspNetCore/Transports.AspNetCore.csproj
#	src/Transports.Subscriptions.Abstractions/ProtocolMessageListener.cs
#	src/Transports.Subscriptions.Abstractions/Subscription.cs
#	src/Transports.Subscriptions.Abstractions/SubscriptionServer.cs
#	src/Transports.Subscriptions.Abstractions/Transports.Subscriptions.Abstractions.csproj
#	src/Transports.Subscriptions.WebSockets/GraphQlWebSocketsMiddleware.cs
#	src/Transports.Subscriptions.WebSockets/Transports.Subscriptions.WebSockets.csproj
#	src/Transports.Subscriptions.WebSockets/WebSocketConnection.cs
#	src/Transports.Subscriptions.WebSockets/WebSocketReaderPipeline.cs
#	src/Ui.GraphiQL/Ui.GraphiQL.csproj
#	src/Ui.Playground/Ui.Playground.csproj
#	src/Ui.Voyager/Ui.Voyager.csproj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add proper support of Accept HTTP header
5 participants