Fix to return the proper media type in GraphqlService (line#4451)

Motivation: According to the spec, we respect the accept header first. If there's no accept header, we should return using `application/json` before 2025 year and `application/graphal-response+json` after that. Modification: - 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 - Return using `application/json` if there's no accept header. Result: - `GraphqlService` now respects the accept header correctly.
marcingrzejszczak · Sep 28, 2022 · 9b12ce9 · 9b12ce9
1 parent f4e2482
commit 9b12ce9
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 34 deletions.
diff --git a/core/src/main/java/com/linecorp/armeria/common/MediaType.java b/core/src/main/java/com/linecorp/armeria/common/MediaType.java
@@ -65,6 +65,7 @@
 import com.google.common.collect.Multimaps;
 
 import com.linecorp.armeria.common.annotation.Nullable;
+import com.linecorp.armeria.common.annotation.UnstableApi;
 
 /**
  * Represents an <a href="https://en.wikipedia.org/wiki/Internet_media_type">Internet Media Type</a>
@@ -779,11 +780,22 @@ private static MediaType addKnownType(MediaType mediaType) {
     public static final MediaType GRAPHQL = createConstant(APPLICATION_TYPE, "graphql");
 
     /**
-     * <a href="https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#content-types">GraphQL over JSON</a>
-     * which is the official GraphQL content type.
+     * The GraphQL response content type is changed from {@link #GRAPHQL_JSON} to {@link #GRAPHQL_RESPONSE_JSON}
+     * in this PR. <a href="https://github.com/graphql/graphql-over-http/pull/215">Change media type</a>
+     *
+     * @deprecated Use {@link #GRAPHQL_RESPONSE_JSON} if the client can recognize the media type.
      */
+    @Deprecated
     public static final MediaType GRAPHQL_JSON = createConstant(APPLICATION_TYPE, "graphql+json");
 
+    /**
+     * <a href="https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#content-types">GraphQL over JSON</a>
+     * which is the official GraphQL response content type.
+     */
+    @UnstableApi
+    public static final MediaType GRAPHQL_RESPONSE_JSON =
+            createConstant(APPLICATION_TYPE, "graphql-response+json");
+
     private static final Charset NO_CHARSET = new Charset("NO_CHARSET", null) {
         @Override
         public boolean contains(Charset cs) {

diff --git a/core/src/main/java/com/linecorp/armeria/common/MediaTypeNames.java b/core/src/main/java/com/linecorp/armeria/common/MediaTypeNames.java
@@ -604,6 +604,10 @@ public final class MediaTypeNames {
      * {@value #GRAPHQL_JSON}.
      */
     public static final String GRAPHQL_JSON = "application/graphql+json";
+    /**
+     * {@value #GRAPHQL_JSON}.
+     */
+    public static final String GRAPHQL_RESPONSE_JSON = "application/graphql-response+json";
 
     private MediaTypeNames() {}
 }
diff --git a/...ocol/src/main/java/com/linecorp/armeria/internal/server/graphql/protocol/GraphqlUtil.java b/...ocol/src/main/java/com/linecorp/armeria/internal/server/graphql/protocol/GraphqlUtil.java
@@ -19,7 +19,6 @@
 import java.util.List;
 
 import com.linecorp.armeria.common.HttpHeaderNames;
-import com.linecorp.armeria.common.HttpMethod;
 import com.linecorp.armeria.common.MediaType;
 import com.linecorp.armeria.common.RequestHeaders;
 import com.linecorp.armeria.common.annotation.Nullable;
@@ -44,32 +43,33 @@ public final class GraphqlUtil {
      */
     @Nullable
     public static MediaType produceType(RequestHeaders headers) {
-        final MediaType contentType = headers.contentType();
-        if (HttpMethod.POST == headers.method() && contentType != null) {
-            if (contentType.is(MediaType.GRAPHQL) ||
-                contentType.is(MediaType.MULTIPART_FORM_DATA)) {
-                return MediaType.GRAPHQL_JSON;
-            }
-        }
-
         final List<MediaType> acceptTypes = headers.accept();
-        if (acceptTypes.isEmpty()) {
-            // If there is no Accept header in the request, the response MUST include
-            // a Content-Type: application/graphql+json header
-            return MediaType.GRAPHQL_JSON;
-        }
-
-        for (MediaType accept : acceptTypes) {
-            if (MediaType.ANY_TYPE.is(accept) || MediaType.ANY_APPLICATION_TYPE.is(accept)) {
-                return MediaType.GRAPHQL_JSON;
-            }
-            if (accept.is(MediaType.GRAPHQL_JSON) || accept.is(MediaType.JSON)) {
-                return accept;
+        // Check the accept header first.
+        if (!acceptTypes.isEmpty()) {
+            for (MediaType accept : acceptTypes) {
+                if (MediaType.ANY_TYPE.is(accept) || MediaType.ANY_APPLICATION_TYPE.is(accept)) {
+                    // This will be changed to return MediaType.GRAPHQL_RESPONSE_JSON after 2025.
+                    // https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#legacy-watershed-1
+                    return MediaType.JSON;
+                }
+                if (accept.is(MediaType.GRAPHQL_RESPONSE_JSON) ||
+                    accept.is(MediaType.GRAPHQL_JSON) ||
+                    accept.is(MediaType.JSON)) {
+                    return accept;
+                }
             }
+
+            // When the accept header is invalid, we have 2 options:
+            // - Disregard the Accept header and respond with the default media type.
+            // - Respond with a 406 Not Acceptable.
+            // https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#body
+            // We just return null to send 406 response, but we might revisit later to use different option.
+            return null;
         }
 
-        // Not acceptable
-        return null;
+        // This will be changed to return MediaType.GRAPHQL_RESPONSE_JSON after 2025.
+        // https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#legacy-watershed-1
+        return MediaType.JSON;
     }
 
     private GraphqlUtil() {}

diff --git a/.../src/test/java/com/linecorp/armeria/internal/server/graphql/protocol/GraphqlUtilTest.java b/.../src/test/java/com/linecorp/armeria/internal/server/graphql/protocol/GraphqlUtilTest.java
@@ -43,12 +43,12 @@ private static Stream<Arguments> provideMediaType() {
                                            .contentType(MediaType.ANY_TYPE)
                                            .accept(MediaType.ANY_TYPE)
                                            .build(),
-                             MediaType.GRAPHQL_JSON),
+                             MediaType.JSON),
                 Arguments.of(RequestHeaders.builder(HttpMethod.GET, "/graphql")
                                            .contentType(MediaType.ANY_TYPE)
                                            .accept(MediaType.ANY_APPLICATION_TYPE)
                                            .build(),
-                             MediaType.GRAPHQL_JSON),
+                             MediaType.JSON),
                 Arguments.of(RequestHeaders.builder(HttpMethod.GET, "/graphql")
                                            .contentType(MediaType.ANY_TYPE)
                                            .accept(MediaType.JSON)
@@ -63,10 +63,11 @@ private static Stream<Arguments> provideMediaType() {
                                            .contentType(MediaType.ANY_TYPE)
                                            // accept is null
                                            .build(),
-                             MediaType.GRAPHQL_JSON),
+                             MediaType.JSON),
                 Arguments.of(RequestHeaders.builder(HttpMethod.GET, "/graphql")
                                            .contentType(MediaType.ANY_TYPE)
-                                           .accept(MediaType.GRAPHQL)
+                                           // https://graphql.org/learn/serving-over-http/#post-request
+                                           .accept(MediaType.GRAPHQL) // Invalid accept type.
                                            .build(),
                              null),
                 Arguments.of(RequestHeaders.builder(HttpMethod.GET, "/graphql")
@@ -88,7 +89,12 @@ private static Stream<Arguments> provideMediaType() {
                                            .contentType(MediaType.JSON)
                                            .accept(MediaType.GRAPHQL_JSON)
                                            .build(),
-                             MediaType.GRAPHQL_JSON)
+                             MediaType.GRAPHQL_JSON),
+                Arguments.of(RequestHeaders.builder(HttpMethod.POST, "/graphql")
+                                           .contentType(MediaType.JSON)
+                                           .accept(MediaType.GRAPHQL_RESPONSE_JSON)
+                                           .build(),
+                             MediaType.GRAPHQL_RESPONSE_JSON)
         );
     }
 }
diff --git a/...rc/test/java/com/linecorp/armeria/server/graphql/protocol/AbstractGraphqlServiceTest.java b/...rc/test/java/com/linecorp/armeria/server/graphql/protocol/AbstractGraphqlServiceTest.java
@@ -88,7 +88,7 @@ void defaultContentType() throws Exception {
         final HttpRequest request = HttpRequest.of(HttpMethod.GET, "/graphql?" + query.toQueryString());
         final ServiceRequestContext ctx = ServiceRequestContext.of(request);
         testGraphqlService.serve(ctx, request);
-        assertThat(testGraphqlService.produceType).isEqualTo(MediaType.GRAPHQL_JSON);
+        assertThat(testGraphqlService.produceType).isEqualTo(MediaType.JSON);
     }
 
     @ArgumentsSource(MediaTypeProvider.class)

diff --git a/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlService.java b/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlService.java
@@ -65,8 +65,8 @@ protected HttpResponse executeGraphql(ServiceRequestContext ctx, GraphqlRequest
         final MediaType produceType = GraphqlUtil.produceType(ctx.request().headers());
         if (produceType == null) {
             return HttpResponse.of(HttpStatus.NOT_ACCEPTABLE, MediaType.PLAIN_TEXT,
-                                   "Only application/graphql+json and application/json compatible " +
-                                   "media types are acceptable");
+                                   "Only %s and %s compatible media types are acceptable",
+                                   MediaType.GRAPHQL_RESPONSE_JSON, MediaType.JSON);
         }
 
         final ExecutionInput.Builder builder = ExecutionInput.newExecutionInput(req.query());

diff --git a/...ngria_2.13/src/main/scala/com/linecorp/armeria/server/sangria/SangriaGraphqlService.scala b/...ngria_2.13/src/main/scala/com/linecorp/armeria/server/sangria/SangriaGraphqlService.scala
@@ -64,7 +64,10 @@ final class SangriaGraphqlService[Ctx, Val] private[sangria] (
       return HttpResponse.of(
         HttpStatus.NOT_ACCEPTABLE,
         MediaType.PLAIN_TEXT,
-        "Only application/graphql+json and application/json compatible media types are acceptable")
+        "Only %s and %s compatible media types are acceptable",
+        MediaType.GRAPHQL_RESPONSE_JSON,
+        MediaType.JSON
+      )
     }
 
     QueryParser.parse(req.query()) match {