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

Support client requests with content-type "application/graphql" #948

Closed
rstoyanchev opened this issue Apr 11, 2024 · 2 comments
Closed

Support client requests with content-type "application/graphql" #948

rstoyanchev opened this issue Apr 11, 2024 · 2 comments
Assignees
@rstoyanchev
Labels
type: enhancement A general enhancement
Milestone
1.2.7

Comments

@rstoyanchev
Copy link
Contributor

It's not a media type from GraphQL over HTTP spec, but it's used widely enough by clients and some tooling, e.g. IntelliJ uses it in the HTTP client. However, message converters and codecs are only configured for "application/json". We need to be more lenient, and allow such requests to be handled.
@rstoyanchev rstoyanchev added the type: enhancement A general enhancement label Apr 11, 2024
@rstoyanchev rstoyanchev added this to the 1.2.7 milestone Apr 11, 2024
rstoyanchev added a commit that referenced this issue Apr 22, 2024
@rstoyanchev
Minor refactoring in HTTP handler tests
bf9c584 
Change WebFlux handler to actually decode from JSON rather
than take an already decoded SerializableGraphQlRequest.

See gh-948
@github-actions GitHub Actions
Copy link
Contributor

Fixed via 7dd9cf5

@rstoyanchev rstoyanchev self-assigned this Apr 22, 2024
@rstoyanchev
Copy link
Contributor Author

GraphQlHttpHandler implementations now check for "application/graphql" defensively, if we fail to decode due to an unsupported media type, and then try again with "application/json". This fallback approach is justified given the general direction of the spec which expects "application/json" and decreasing use of "application/graphql" judging from graphql/graphql-over-http#249 (comment) for example.

That makes the handlers capable, but the router mappings also need an update. For 1.3 there is a new GraphQlRequestPredicates based on #906 that needs to also be updated to allow "application/graphql", so I'm re-opening this issue to do that. For 1.2 the mapping is in the Boot starter, so we can't change that here.

@rstoyanchev rstoyanchev reopened this Apr 22, 2024
@gjinajgledis gjinajgledis mentioned this issue Jul 25, 2024
Closed
bclozel added a commit that referenced this issue Jul 26, 2024
@bclozel
Support application/graphql with charset information
66cece6 
Prior to this commit, gh-948 added a fallback support for the
"application/graphql" content-type sent by clients. This media type is
not widely used and advised against by the spec group.

This fallback checked for an exact match of the content type, not taking
into account potential media type parameters such as charset.

This commit ensure that a `MediaType#include` comparison is used to
trigger the fallback.

Fixes gh-1036
bclozel added a commit that referenced this issue Jul 26, 2024
@bclozel
Support application/graphql with charset information
5ffe512 
Prior to this commit, gh-948 added a fallback support for the
"application/graphql" content-type sent by clients. This media type is
not widely used and advised against by the spec group.

This fallback checked for an exact match of the content type, not taking
into account potential media type parameters such as charset.

This commit ensure that a `MediaType#include` comparison is used to
trigger the fallback.

Fixes gh-1038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rstoyanchev rstoyanchev

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
1.2.7
Development

No branches or pull requests
1 participant
@rstoyanchev