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.

Sign up for GitHub

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

Jump to bottom

Allow the configuration of the response content type via options #913

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Allow the configuration of the response content type via options #913

merged 2 commits into from
Sep 2, 2022

Conversation

jeffw-wherethebitsroam
Copy link
Contributor

This allows users to override the default response content type without
having to implement their own middleware.

@jeffw-wherethebitsroam
Allow the configuration of the response content type via options
bed65b8 
This allows users to override the default response content type without
having to implement their own middleware.
@Shane32
Copy link
Member

Shane32 commented Aug 26, 2022

@jeffw-wherethebitsroam If you run the tests locally, it will automatically update the apiapprovals file; commit the change and tests should pass again.

@Shane32
Copy link
Member

Shane32 commented Aug 26, 2022 •
edited
Loading

Out of curiosity, how has this change to the default response content type been affecting your application? Any comments as to your experience migrating to GraphQL.NET Server v7 would be of interest.

@jeffw-wherethebitsroam
Copy link
Contributor Author

@Shane32 we have a javascript app which uses graphql-request with code generated with graphql-codegen. Responses from v7 look identical except for the new content-type. For some reason the response was interpreted as an error with the 'application/graphql+json' content type. There could be a fix for the package, but I just want to get the update out without waiting for frontend changes.

Other than that, the upgrade was fairly smooth. It was good to have a migration doc. A lot of deprecation warnings for the corresponding GraphQL v7 upgrade.

@Shane32
Copy link
Member

Shane32 commented Aug 26, 2022

Ref:

Shane32
Shane32 approved these changes Aug 26, 2022
View reviewed changes
Copy link
Member

@Shane32 Shane32 left a comment

Choose a reason for hiding this comment

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

@sungam3r Please review. I'll update the readme in a separate PR.

@Shane32 Shane32 requested a review from sungam3r August 26, 2022 15:35
@Shane32
Copy link
Member

Shane32 commented Aug 26, 2022

@jeffw-wherethebitsroam @sungam3r Before merging this, I would like to consider modifying the middleware to inspect the Accept header and providing an appropriate response. This should provide wider compatibility. This option then could become a default response content-type.

Thoughts?

@Shane32
Copy link
Member

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 due to proper support missing from client-side tooling. As such, I'm considering this a critical enhancement and will be releasing 7.1 with this enhancement shortly if you have no comments. See also #917 and #919

@jeffw-wherethebitsroam
Copy link
Contributor Author

@Shane32 I'm happy for you to add support for the Accept header and that seems like a more 'correct' way to handle it. But I would also like this PR merged first to give an easier way of overriding the default. Also, graphql-request seems to send Accept: */*, so we would still need to override the default.

@jeffw-wherethebitsroam
Copy link
Contributor Author

Also, please see my comment on the Accept PR, https://github.com/graphql-dotnet/server/pull/919/files#r956978257. Only supporting the Accept header with a hard coded default creates more work for handling misbehaving clients, since you would now need to subclass GraphQLHttpMiddleware and copy the Accept logic.

@Shane32
Copy link
Member

Shane32 commented Aug 29, 2022

I agree we should still add the configuration option.

@Shane32 Shane32 merged commit d80414d into graphql-dotnet:master Sep 2, 2022
sungam3r
sungam3r reviewed Sep 5, 2022
View reviewed changes
src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs
/// <summary>
/// The Content-Type to use for GraphQL responses
/// </summary>
public string ResponseContentType { get; set; } = GraphQLHttpMiddleware.CONTENTTYPE_GRAPHQLJSON;
Copy link
Member

Choose a reason for hiding this comment

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

Now it's better to place CONTENTTYPE_GRAPHQLJSON internal constant in options class, I think.

@sungam3r sungam3r added the enhancement New feature or request label Sep 5, 2022
@sungam3r
Copy link
Member

sungam3r commented Sep 5, 2022

reviewed

Kent-Chen-Conning added a commit to Conning/graphql-dotnet-server that referenced this pull request Oct 28, 2022
@Kent-Chen-Conning
Merge commit 'af5ac03f9f5505d476850cee23c5aaa145110c94' into TRAC-669…
977b74c 
…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
Reviewers

@sungam3r sungam3r sungam3r left review comments

@Shane32 Shane32 Shane32 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeffw-wherethebitsroam @Shane32 @sungam3r