Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Incorrect Content-Type for apollo-server-adonis #910

Conversation

CaptainJojo
Copy link
Contributor

#Issue #867

Intended outcome

Responses from apollo-server-adonis shall have a Content-Type: application/json header set.

Actual outcome

Responses have Content-Type: text/plain; charset=utf-8 header set.

Analysis

The module apollo-server-adonis calls response.json(gqlResponse) after it gets the output from runHttpQuery.

However, gqlResponse is already stringified (see link 1). The only reason .json method works, is because it is an "alias" to .send and uses heuristics to determine the content type in the end, regardless of the response method used. Adonis has it's own response object, with a similar interface to Express but a different implementation.

Proposed Solutions

Continue returning gqlResponse as-is, but use send method and manually set the content type header to application/json. The easiest to implement.

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Brief question, but this looks good to me.

@@ -35,7 +35,8 @@ export function graphqlAdonis(
query,
}).then(
gqlResponse => {
response.json(gqlResponse);
response.header('Content-Type', 'application/json');
Copy link
Member

Choose a reason for hiding this comment

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

Not familiar with Adonis patterns but, aside from being more concise, is there a benefit to using the response.type('application/json') technique (outlined in the docs here) as opposed to this more explicit response.header?

(Keep in mind that we've never supported Adonis 3.0, so the fact that it isn't present in v3 docs shouldn't matter.)

@CaptainJojo CaptainJojo force-pushed the fix/incorrect-response-apollo-server-adonis branch 2 times, most recently from 2d3e65b to d2e5947 Compare April 5, 2018 06:03
@CaptainJojo CaptainJojo force-pushed the fix/incorrect-response-apollo-server-adonis branch from 110c3a3 to 8a5f3ad Compare April 5, 2018 06:06
@CaptainJojo
Copy link
Contributor Author

Thank a lot @abernix, I change for response.type you need more ?

@CaptainJojo CaptainJojo force-pushed the fix/incorrect-response-apollo-server-adonis branch from 8a5f3ad to efc3377 Compare April 5, 2018 06:08
@@ -35,7 +35,8 @@ export function graphqlAdonis(
query,
}).then(
gqlResponse => {
response.json(gqlResponse);
response.type('application/json');
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for tweaking that, @CaptainJojo.

One last question: Do you know if the Content-length is set automatically, or should we set it like we do for express? If it's not set, it would be beneficial for it to be there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is set automatically, they are not option in adonis :)

@abernix abernix merged commit afe5df4 into apollographql:master Apr 11, 2018
@abernix
Copy link
Member

abernix commented Apr 18, 2018

This should be published in apollo-server-adonis@1.3.5. Please report back how it works!

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.

None yet

2 participants