Making content type resolution more robust#13
Making content type resolution more robust#13andimarek merged 2 commits intographql-java:masterfrom marceloverdijk:mediatype
Conversation
andimarek
left a comment
andimarek
left a comment
There was a problem hiding this comment.
see my comment about wrong content types.
Also: At least one more test would be good.
| if (!StringUtils.isEmpty(contentType)) { | ||
| try { | ||
| mediaType = MediaType.parseMediaType(contentType); | ||
| } catch (InvalidMediaTypeException ignore) { |
There was a problem hiding this comment.
why are we ignoring wrong content types here? is that a real case we need to deal with?
There was a problem hiding this comment.
If the parsing fails at that point we could throw also a throw new ResponseStatusException(HttpStatus.UNPROCESSABLE_ENTITY, "Could not process GraphQL request");
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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)?
|
I've run into this same issue whilst trying to write some integration tests using def request = post("/graphql")
.content(content)
.contentType(MediaType.APPLICATION_JSON_VALUE)
mockMvc.perform(request).andExpect(status().isOk())The request fails because content type gets sent over as: Is there a workaround or anything that can be done to get this fix merged in? |
3 participants
Similar as micronaut-projects/micronaut-graphql#7 this makes the content type resolution more robust. E.g. case insensitive and ignoring content type parameters.
To do so I upgraded the Spring Boot + Spring minor versions to use equalsTypeAndSubtype introduced in Spring 5.1.4.
This also caused I had to change some integration tests using
WebTestClientbecause of a bug in 5.1.2 which resulted in different behaviour.