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

merged 2 commits into from
Jul 12, 2020

Conversation

marceloverdijk
Copy link
Contributor

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 WebTestClient because of a bug in 5.1.2 which resulted in different behaviour.

Copy link
Member

@andimarek andimarek left a comment

Choose a reason for hiding this comment

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

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) {
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)?

@symm
Copy link

symm commented Sep 24, 2019

I've run into this same issue whilst trying to write some integration tests using MockMvc e.g.

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: application/json;charset=UTF-8 and doesn't match the strict check of application/json.

Is there a workaround or anything that can be done to get this fix merged in?

@andimarek andimarek merged commit d9ac5c2 into graphql-java:master Jul 12, 2020
@andimarek andimarek added this to the 2.0 milestone Jul 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants