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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uppy incompatibility due to OPTIONS request #436

Closed
cilim opened this issue Dec 6, 2019 · 7 comments
Closed

Uppy incompatibility due to OPTIONS request #436

cilim opened this issue Dec 6, 2019 · 7 comments
Labels

Comments

@cilim
Copy link

@cilim cilim commented Dec 6, 2019

Hi @janko 馃憢

Before writing the issue details, I wanna say thank you for this amazing gem! Helps out a bunch every day!

And now:

Brief Description

I believe that uppy isn't compatible with Shrine with versions 1.3.0 and newer.

Expected behavior

Direct upload to S3 with uppy & Shrine.

Actual behavior

Uppy sends a OPTIONS preflight request to /s3/params and the response is Method Not Allowed (405)

Simplest self-contained example code to demonstrate issue

const fileInput = this.fileTarget # file input from HTML
fileInput.style.display = 'none'

const uppy = Uppy({
  autoProceed: false,
  restrictions: {
    maxFileSize: 10737418240,
    maxNumberOfFiles: 1,
    allowedFileTypes: ['video/*']
  }
})

uppy.use(Dashboard, {
  inline: true,
  target: '.drag-drop-area',
  showProgressDetails: true,
  browserBackButtonClose: true
})

uppy.use(AwsS3, {
  companionUrl: '/'
})

uppy.on('upload-success', function(file, response) {
  var uploadedFileData = JSON.stringify({
    id: file.meta['key'].match(/^cache\/(.+)/)[1],
    storage: 'cache',
    metadata: {
      size:      file.size,
      filename:  file.name,
      mime_type: file.type
    }
  })

  var hiddenInput = fileInput.parentNode.querySelector('.upload-hidden')
  hiddenInput.value = uploadedFileData
})

The exact same code passes when using uppy < 1.3.0

System configuration

Ruby version: 2.6.5

Shrine version: 3.0.1

@janko

This comment has been minimized.

Copy link
Member

@janko janko commented Dec 6, 2019

Before writing the issue details, I wanna say thank you for this amazing gem! Helps out a bunch every day!

That's great to hear! 鉂わ笍

Uppy sends a OPTIONS preflight request to /s3/params and the response is Method Not Allowed (405)

Yes, this is a known issue, but I haven't opened a ticket for it yet, so thanks for doing that 馃槂

Even though the OPTIONS request is failing, the direct upload itself should still succeed, at least that's how it is for me locally on Chrome/Firefox. So, it's more of an annoying failed request in the network inspector tab than an incompatibility.

That being said, we definitely plan to fix this. We need the presign_endpoint to implement an OPTIONS route with some basic CORS response (we can see some examples in Uppy Companion and tus-ruby-server). It seems from RequestClient.js that Uppy only looks at the Access-Control-Allow-Headers response header.

@janko

This comment has been minimized.

Copy link
Member

@janko janko commented Dec 6, 2019

@cilim Let me know if you would perhaps like to give it a try 馃槈 (I think it shouldn't be difficult, though having some Rack knowledge would definitely help).

Otherwise we'll probably find time for it sometime next week.

@jrochkind

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Dec 6, 2019

Any chance that such a fix could be backported to shrine 2.x too?

@mike1o1

This comment has been minimized.

Copy link

@mike1o1 mike1o1 commented Dec 6, 2019

I ended up fixing this by overwriting the getUploadParameters method on Uppy to make a get request directly. Here is the code snippet if that helps anybody else:

uppy
  .use(AwsS3, {
    limit: 5,
    getUploadParameters(file) {
      let url = `/s3/params?filename=${file.name}&type=${file.type}`;
      return fetch(url, {
        method: 'get',
        headers: {
          accept: 'application/json',
          'content-type': 'application/json'
        }
      })
        .then((response) => {
          return response.json();
        })
        .then((data) => {
          return {
            method: data.method,
            url: data.url,
            fields: data.fields
          };
        });
    }
  })
@janko

This comment has been minimized.

Copy link
Member

@janko janko commented Dec 7, 2019

Any chance that such a fix could be backported to shrine 2.x too?

There should be no need, as the upload itself should still be working. Uppy is the one making the OPTIONS request to fetch information from the Uppy Companion (which in our case is presign_enpdoint predending to be Uppy Companion), so it's not the browser that's making the request (browsers only make preflight requests for non-GET verbs), and Uppy still seems to proceed with the upload when the OPTIONS verb is not implemented.

@janko

This comment has been minimized.

Copy link
Member

@janko janko commented Dec 8, 2019

Since this doesn't seem to be causing any problems at the moment, I will leave this as an opportunity for others to send a PR for the OPTIONS route.

@janko janko added the pr-welcome label Dec 8, 2019
@cilim

This comment has been minimized.

Copy link
Author

@cilim cilim commented Dec 9, 2019

@janko I'm up for helping out and implementing an OPTIONS route on the presign_endpoint, but I won't make it this week, since the holidays are around the block and all clients have deadlines 馃檲

@janko janko closed this in 0a6adbb Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can鈥檛 perform that action at this time.