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

companion: add required 'content-length' header if not present #2387

Closed

Conversation

johnnyperkins
Copy link
Contributor

This fixes an issue of uploads to s3 from companion failing due to no 'content-length' header being sent.

@johnnyperkins
Copy link
Contributor Author

@goto-bus-stop @arturi any input on this? Would be great to get this usable.

@ifedapoolarewaju ifedapoolarewaju self-requested a review July 27, 2020 12:02
@johnnyperkins
Copy link
Contributor Author

This may not be the best solution but it got things working for me.
Happy to help investing this issue further.

@ifedapoolarewaju
Copy link
Contributor

This indeed is an issue, thank you for the PR!

However, in the case of formData, the content-length isn't simply the file size as it may contain other fields and content that consist the form data. An ideal fix for this may be something around this.

Is this an update you can make? This is only needed for the formData case.

@mejiaej
Copy link
Contributor

mejiaej commented Aug 13, 2020

In my organization we are currently experiencing this same issue, I can create a PR to cover the formData case. Do you guys think using stream-length similar to this PR is worth it? my plan is to create a promise using stream-length then await for the length and place it in the content-length header. I'm gonna be working in this solution, I'd like to read your thoughts on this approach.

@johnnyperkins
Copy link
Contributor Author

I don't currently have a setup to easily test the formData use case.
@mejiaej Would be awesome if you could take on the formData case.
@ifedapoolarewaju Does this solution look right for the non formData case? If so can we get this in and then have the formData handled in another pass?

@mejiaej
Copy link
Contributor

mejiaej commented Aug 14, 2020

@johnnyperkins I don't have the right setup to test the formdata case but found a hacky way to test it. Probably I'm gonna be working on that today if my team finds it necessary. Additionally, I tried your solution and it works when sending the file in the body. Hopefully @ifedapoolarewaju can give us some head ups on what would be the ideal path to finally fix the issue.

@ifedapoolarewaju
Copy link
Contributor

@johnnyperkins is it confirmed that this issue also exists for the case where formData isn't used? AFAICT from this issue, the problem only exists when we are using form data.

How are using instantiating the S3 uploader on your end? By default, the S3 plugin uses formData for POST requests. So maybe you were using asformData as well? cc @goto-bus-stop

@johnnyperkins
Copy link
Contributor Author

@johnnyperkins is it confirmed that this issue also exists for the case where formData isn't used? AFAICT from this issue, the problem only exists when we are using form data.

How are using instantiating the S3 uploader on your end? By default, the S3 plugin uses formData for POST requests. So maybe you were using asformData as well? cc @goto-bus-stop

@ifedapoolarewaju
In my case yes this is an issue when not using formData.
I'm using a signed upload URL to upload to S3 which requires using PUT not POST

uppy.use(AwsS3, {
      limit: 5,
      timeout: 30 * 1000, // 30s
      companionUrl: 'http://localhost:3020/',
      getUploadParameters: Uploader.getUploadParameters
    })
static async getUploadParameters (file) {
  const md5 = file.isRemote
    ? undefined // TODO have companion attach an md5 to it's response so we can do file.md5
    : await fileMD5(file.data, { base64: true })

  const signedurl = await fetchSignedUrl(file, md5)

  return Promise.resolve({
    method: 'PUT',
    url: signedurl,
    headers: {
      'Content-Type': file.type,
      'Content-MD5': md5
    }
  })
}

@ifedapoolarewaju
Copy link
Contributor

@johnnyperkins @mejiaej Closing this in favour of #2466. Thank you for the work both 🙏

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