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

Correctly infer and set content-type #37

Closed
wants to merge 1 commit into from
Closed

Correctly infer and set content-type #37

wants to merge 1 commit into from

Conversation

abeland
Copy link

@abeland abeland commented Nov 8, 2017

Firstly, thanks for your work in getting file uploading working with Apollo.

I was testing v6.0.0-beta1 of apollo-upload-client and ran into a mysterious bug.

Context:

We have a component which will first fetch a list of items (via GraphQL), then render them in the UI. The user can then click on an item and attach a document.

When I tried to upload documents, the request would barf on the server (I use a custom gem to make this all work in a Rails server context). It was because the content-type header in the request was application/json.

After some (a lot because I'm new to npm debugging) debugging, I confirmed that this line(51):

fetchOptions.headers['content-type'] = 'application/json'

was altering the original headers object which I gave when I called createUploadLink(...). This is obvious in hindsight since objects are passed by reference in Javscript.

I see two possible solutions:

  1. Deep clone the original given fetchOptions every request
  2. Delete the content-type header when there are files attached and let fetch infer the content-type which it does so well today

I like (2) because it's easier.

I'm putting up this PR in case you want to go this route. I realize you may go another route to address this problem, but in the meantime I will just be using this for my project.

Testing

The npm build script fails in my environment, so I just tested by copying the index.mjs file directly over and verifying that uploading a file worked in the aforementioned scenario/context. Let me know if you'd like me to test in a different way.

I was testing v6.0.0-beta1 of apollo-upload-client and ran into a mysterious bug.

Context:

We have a component which will first fetch a list of items (via GraphQL), then render them in the UI. The user can then click on an item and attach a document.

When I tried to upload documents, the request would barf on the server (I use a [custom gem](https://github.com/abeland/apollo_fetch_upload_rails_middleware) to make this all work in a Rails server context). It was because the `content-type` header in the request was `application/json`.

After some (a lot because I'm new to npm debugging) debugging, I confirmed that this line(52):

```js
fetchOptions.body = JSON.stringify(requestOperation)
```
was altering the original headers object which I gave when I called `createUploadLink(...)`. This is obvious in hindsight since objects are passed by reference in Javscript.

I see two possible solutions:

1. Deep clone the original given `fetchOptions` every request
2. Delete the content-type header when there are files attached and let fetch infer the content-type which it does so well today

I like (2) because it's easier.
@jaydenseric
Copy link
Owner

Thanks for testing out the beta 🙂

Trying to get my head around what was going wrong. So for multipart upload requests, the content-type header received by the server was application/json?

@abeland
Copy link
Author

abeland commented Nov 9, 2017

First request was just a normal GraphQL query (fetching a list of items). The content-type was correctly set to application/json.

Then, a document upload is attempted (via GQL mutation). I stepped through in index.mjs and saw that the content-type header was set to application/json, even though the body of the request is FormData, and then fetcher(...) is called.

So my theory was that the headers object for both requests was literally the same object, and thus after the first request happened, the content-type: application/json header had been added, and so the second request (the document upload one with FormData body) had that content-type header, causing the request to barf on the server.

To confirm this, I added this code where I construct my ApolloClient instance:

const headers = { 'X-CSRF-Token': $('meta[name=csrf-token]').attr('content') };
const apolloClient = new ApolloClient({
  cache: ...,
  link: createUploadLink({
    fetchOptions: {
      credentials: 'same-origin',
      headers,
    },
    uri: '/graphql',
  }),
});

window.setInterval(() => console.log('HEADERS: ', headers), 1000);

export default apolloClient;

Initially, headers had only my X-CSRF-Token key/value pair. After the first GraphQL request (the one fetching the list of items), it then also had content-type set to application/json. And of course when I uploaded my file, the header object still had that wrong header and so the request failed as expected.

@jaydenseric
Copy link
Owner

Hmmm. I think I see now what is happening.

@jaydenseric
Copy link
Owner

Please try out apollo-upload-client@6.0.0-beta.2 and let me know if all is well 🙏

@jaydenseric jaydenseric closed this Nov 9, 2017
@abeland
Copy link
Author

abeland commented Nov 9, 2017

beta2 = 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants