Fix to return the proper media type in GraphqlService
#4451
Conversation
Motivation: The default produce type for a GraphQL response has been changed to `application/graphql-response json` from `application/graphal json`. We can't just change the default produce type at the moment because it will break the backward compatibility. Instead, we provide a way to set default produce type so that the service owner can choose the default produce type. Modification: - Add `GraphqlServiceBuilder.defaultProduceType(type)` - Respect accept header first in `GraphqlService` as described in the spec. - See https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#body Result: - You can now set the default produce type for a GraphQL response.
Codecov ReportBase: 74.06% // Head: 74.08% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4451 /- ##
============================================
Coverage 74.06% 74.08% 0.02%
- Complexity 18107 18112 5
============================================
Files 1527 1527
Lines 67103 67107 4
Branches 8484 8483 -1
============================================
Hits 49698 49717 19
Misses 13351 13337 -14
Partials 4054 4053 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlService.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| public static MediaType produceType(RequestHeaders headers, MediaType defaultProduceType) { |
There was a problem hiding this comment.
Should we take a function to select a response type dynamically which would be useful to handle both legacy and new clients in a server?
public static MediaType produceType(ServiceRequestContext ctx, Function<ServiceRequestContext, MediaType> defaultProduceTypeFunction) {
...
}
...
GrpcService.builder()
// statically set
.defaultProduceType(MediaType.GRAPHQL_JSON)
// or dynamically choose
.defaultProduceType(ctx -> {
if (legacyclient) {
return MediaType.GRAPHQL_JSON;
} else {
return MediaType.GRAPHQL_RESPONSE_JSON;
}
});There was a problem hiding this comment.
Should we take a function to select a response type dynamically
Is there any usage to select the type dynamically except using accept header which we are already handling?
There was a problem hiding this comment.
defaultProduceType is used when Accept header is empty.
As per Legacy watershed, the response content-type could be either application/json or application/graphql-response json.
Before 1st January 2025 (2025-01-01T00:00:00Z), if the client does not supply an Accept header, the server SHOULD treat the request as if it had Accept: application/json. From 1st January 2025 (2025-01-01T00:00:00Z), if the client does not supply an Accept header, the server SHOULD treat the request as if it had Accept: application/graphql-response json.
If an Accept header is absent, I imagine that a server could choose application/json for a legacy client and application/graphql-response json for a new client in corner cases.
But I'm not sure there is such a desire for that.
There was a problem hiding this comment.
I imagine that a server could choose application/json for a legacy client and application/graphql-response json for a new client in corner cases.
If it's distinguishable, I think we can add the logic to our code. However, I thought it was hard to determine if the client is the legacy one or not.
There was a problem hiding this comment.
User-Agent of clients may be used. In addition, GraphQL servers could switch to the response type after the deadline.
If you think it looks overkill, we add it later without breaking changes when there is a demand for it.
There was a problem hiding this comment.
Had a chat with the team we found out that the current implementation does not respect spec.
If accept is empty, we should return application/json before 2025 year according to the spec.
So I will fix the current logic and remove the setter at the moment.
minwoox
changed the title
GraphqlServiceGraphqlService
|
Thanks, reviewers. 😉 |
Motivation:
According to the spec, we respect the accept header first.
If there's no accept header, we should return using
application/jsonbefore 2025 year andapplication/graphal-response jsonafter that.Modification:
GraphqlServiceas described in the spec.application/jsonif there's no accept header.Result:
GraphqlServicenow respects the accept header correctly.