Skip to content
This repository was archived by the owner on Oct 25, 2021. It is now read-only.

Making content type resolution more robust #13

Merged
merged 2 commits into from
Jul 12, 2020
Merged

Making content type resolution more robust #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d48c05
Making content type resolution more robust
marceloverdijk May 17, 2019
1511fb4
Merge branch 'master' into mediatype
andimarek Jul 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ subprojects {

ext {
graphqlJavaVersion = "15.0"
springVersion = "5.1.2.RELEASE"
springBootVersion = "2.1.0.RELEASE"
jacksonVersion = "2.9.6"
springVersion = "5.1.7.RELEASE"
springBootVersion = "2.1.5.RELEASE"
jacksonVersion = "2.9.8"
assertJVersion = "3.11.1"
}

Expand Down
17 changes: 15 additions & 2 deletions ...pring-webflux/src/main/java/graphql/spring/web/reactive/components/GraphQLController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.InvalidMediaTypeException;
import org.springframework.http.MediaType;
import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RequestMapping;
Expand All @@ -27,6 +29,9 @@
@Internal
public class GraphQLController {

static String APPLICATION_GRAPHQL_VALUE = "application/graphql";
static MediaType APPLICATION_GRAPHQL = MediaType.parseMediaType(APPLICATION_GRAPHQL_VALUE);

@Autowired
GraphQLInvocation graphQLInvocation;

Expand All @@ -47,6 +52,14 @@ public Object graphqlPOST(
@RequestBody(required = false) String body,
ServerWebExchange serverWebExchange) {

MediaType mediaType = null;
if (!StringUtils.isEmpty(contentType)) {
try {
mediaType = MediaType.parseMediaType(contentType);
} catch (InvalidMediaTypeException ignore) {
Copy link
Member

Choose a reason for hiding this comment

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

why are we ignoring wrong content types here? is that a real case we need to deal with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the parsing fails at that point we could throw also a throw new ResponseStatusException(HttpStatus.UNPROCESSABLE_ENTITY, "Could not process GraphQL request");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note we do the same at the end in that method. Do you prefer to add that?

You want a test case with an invalid content type? I can add that one yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andimarek I can do something like throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid Content-Type: " + contentType, e);

Testing will be difficult as MockMvc does not allow to send invalid media types:
https://github.com/spring-projects/spring-framework/blob/master/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java#L648

So effectively the test will never get in the controller itself (I just tried).

Let me know if you prefer the 400 I suggested above or keep as it and fail later (HTTP 422 at end of controller)?

}
}

if (body == null) {
body = "";
}
Expand All @@ -62,7 +75,7 @@ public Object graphqlPOST(
// "variables": { "myVariable": "someValue", ... }
// }

if (MediaType.APPLICATION_JSON_VALUE.equals(contentType)) {
if (MediaType.APPLICATION_JSON.equalsTypeAndSubtype(mediaType)) {
GraphQLRequestBody request = jsonSerializer.deserialize(body, GraphQLRequestBody.class);
if (request.getQuery() == null) {
request.setQuery("");
Expand All @@ -82,7 +95,7 @@ public Object graphqlPOST(
// * If the "application/graphql" Content-Type header is present,
// treat the HTTP POST body contents as the GraphQL query string.

if ("application/graphql".equals(contentType)) {
if (APPLICATION_GRAPHQL.equalsTypeAndSubtype(mediaType)) {
return executeRequest(body, null, null, serverWebExchange);
}

Expand Down
31 changes: 12 additions & 19 deletions ...g-webflux/src/test/java/graphql/spring/web/reactive/components/GraphQLControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import reactor.core.publisher.Mono;
import testconfig.TestAppConfig;

import java.net.URLEncoder;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
Expand Down Expand Up @@ -109,9 +108,7 @@ public void testSimplePostRequest() throws Exception {
@Test
public void testQueryParamPostRequest() throws Exception {
String variablesJson = "{\"variable\":\"variableValue\"}";
String variablesValue = URLEncoder.encode(variablesJson, "UTF-8");
String query = "query myQuery {foo}";
String queryString = URLEncoder.encode(query, "UTF-8");
String operationName = "myQuery";

ExecutionResultImpl executionResult = ExecutionResultImpl.newExecutionResult()
Expand All @@ -122,10 +119,10 @@ public void testQueryParamPostRequest() throws Exception {
Mockito.when(graphql.executeAsync(captor.capture())).thenReturn(cf);

client.post().uri(uriBuilder -> uriBuilder.path("/graphql")
.queryParam("variables", variablesValue)
.queryParam("query", queryString)
.queryParam("operationName", operationName)
.build(variablesJson, queryString))
.queryParam("variables", "{variables}")
.queryParam("query", "{query}")
.queryParam("operationName", "{operationName}")
.build(variablesJson, query, operationName))
.accept(MediaType.APPLICATION_JSON_UTF8)
.exchange()
.expectStatus().isOk()
Expand All @@ -144,7 +141,6 @@ public void testQueryParamPostRequest() throws Exception {
@Test
public void testSimpleQueryParamPostRequest() throws Exception {
String query = "{foo}";
String queryString = URLEncoder.encode(query, "UTF-8");

ExecutionResultImpl executionResult = ExecutionResultImpl.newExecutionResult()
.data("bar")
Expand All @@ -154,8 +150,8 @@ public void testSimpleQueryParamPostRequest() throws Exception {
Mockito.when(graphql.executeAsync(captor.capture())).thenReturn(cf);

client.post().uri(uriBuilder -> uriBuilder.path("/graphql")
.queryParam("query", queryString)
.build())
.queryParam("query", "{query}")
.build(query))
.accept(MediaType.APPLICATION_JSON_UTF8)
.exchange()
.expectStatus().isOk()
Expand Down Expand Up @@ -196,9 +192,7 @@ public void testApplicationGraphqlPostRequest() throws Exception {
@Test
public void testGetRequest() throws Exception {
String variablesJson = "{\"variable\":\"variableValue\"}";
String variablesValue = URLEncoder.encode(variablesJson, "UTF-8");
String query = "query myQuery {foo}";
String queryString = URLEncoder.encode(query, "UTF-8");
String operationName = "myQuery";

ExecutionResultImpl executionResult = ExecutionResultImpl.newExecutionResult()
Expand All @@ -209,10 +203,10 @@ public void testGetRequest() throws Exception {
Mockito.when(graphql.executeAsync(captor.capture())).thenReturn(cf);

client.get().uri(uriBuilder -> uriBuilder.path("/graphql")
.queryParam("variables", variablesValue)
.queryParam("query", queryString)
.queryParam("operationName", operationName)
.build(variablesJson, queryString))
.queryParam("variables", "{variables}")
.queryParam("query", "{query}")
.queryParam("operationName", "{operationName}")
.build(variablesJson, query, operationName))
.accept(MediaType.APPLICATION_JSON_UTF8)
.exchange()
.expectStatus().isOk()
Expand All @@ -231,7 +225,6 @@ public void testGetRequest() throws Exception {
@Test
public void testSimpleGetRequest() throws Exception {
String query = "{foo}";
String queryString = URLEncoder.encode(query, "UTF-8");

ExecutionResultImpl executionResult = ExecutionResultImpl.newExecutionResult()
.data("bar")
Expand All @@ -241,8 +234,8 @@ public void testSimpleGetRequest() throws Exception {
Mockito.when(graphql.executeAsync(captor.capture())).thenReturn(cf);

client.get().uri(uriBuilder -> uriBuilder.path("/graphql")
.queryParam("query", queryString)
.build())
.queryParam("query", "{query}")
.build(query))
.accept(MediaType.APPLICATION_JSON_UTF8)
.exchange()
.expectStatus().isOk()
Expand Down
17 changes: 15 additions & 2 deletions ...-spring-webmvc/src/main/java/graphql/spring/web/servlet/components/GraphQLController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.InvalidMediaTypeException;
import org.springframework.http.MediaType;
import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RequestMapping;
Expand All @@ -28,6 +30,9 @@
@Internal
public class GraphQLController {

static String APPLICATION_GRAPHQL_VALUE = "application/graphql";
static MediaType APPLICATION_GRAPHQL = MediaType.parseMediaType(APPLICATION_GRAPHQL_VALUE);

@Autowired
GraphQLInvocation graphQLInvocation;

Expand All @@ -48,6 +53,14 @@ public Object graphqlPOST(
@RequestBody(required = false) String body,
WebRequest webRequest) throws IOException {

MediaType mediaType = null;
if (!StringUtils.isEmpty(contentType)) {
try {
mediaType = MediaType.parseMediaType(contentType);
} catch (InvalidMediaTypeException ignore) {
}
}

if (body == null) {
body = "";
}
Expand All @@ -63,7 +76,7 @@ public Object graphqlPOST(
// "variables": { "myVariable": "someValue", ... }
// }

if (MediaType.APPLICATION_JSON_VALUE.equals(contentType)) {
if (MediaType.APPLICATION_JSON.equalsTypeAndSubtype(mediaType)) {
GraphQLRequestBody request = jsonSerializer.deserialize(body, GraphQLRequestBody.class);
if (request.getQuery() == null) {
request.setQuery("");
Expand All @@ -83,7 +96,7 @@ public Object graphqlPOST(
// * If the "application/graphql" Content-Type header is present,
// treat the HTTP POST body contents as the GraphQL query string.

if ("application/graphql".equals(contentType)) {
if (APPLICATION_GRAPHQL.equalsTypeAndSubtype(mediaType)) {
return executeRequest(body, null, null, webRequest);
}

Expand Down