Companion: bring back default upload protocol (#3967)

* add (failing) e2e test for remote xhr (multipart) regression caused by #3834 * Revert "do not use a default upload protocol" This reverts commit c7e61dd. * add note on todo * dry code a bit * explicitly add protocol: 'multipart' in the client * fix review comment
transloadit · Aug 11, 2022 · 31a9e0c · 31a9e0c
1 parent 4044971
commit 31a9e0c
Show file tree

Hide file tree

Showing 11 changed files with 97 additions and 38 deletions.
diff --git a/e2e/clients/dashboard-xhr/app.js b/e2e/clients/dashboard-xhr/app.js
@@ -0,0 +1,18 @@
+import { Uppy } from '@uppy/core'
+import Dashboard from '@uppy/dashboard'
+import XHRUpload from '@uppy/xhr-upload'
+import Unsplash from '@uppy/unsplash'
+import Url from '@uppy/url'
+
+import '@uppy/core/dist/style.css'
+import '@uppy/dashboard/dist/style.css'
+
+const companionUrl = 'http://localhost:3020'
+const uppy = new Uppy()
+  .use(Dashboard, { target: '#app', inline: true })
+  .use(XHRUpload, { endpoint: 'https://xhr-server.herokuapp.com/upload', limit: 6 })
+  .use(Url, { target: Dashboard, companionUrl })
+  .use(Unsplash, { target: Dashboard, companionUrl })
+
+// Keep this here to access uppy in tests
+window.uppy = uppy
diff --git a/e2e/clients/dashboard-xhr/index.html b/e2e/clients/dashboard-xhr/index.html
@@ -0,0 +1,11 @@
+<!doctype html>
+<html lang="en">
+  <head>
+    <meta charset="utf-8"/>
+    <title>dashboard-xhr</title>
+    <script defer type="module" src="app.js"></script>
+  </head>
+  <body>
+    <div id="app"></div>
+  </body>
+</html>
diff --git a/e2e/clients/index.html b/e2e/clients/index.html
@@ -12,6 +12,7 @@ <h1>Test apps</h1>
 				<li><a href="react/index.html">react</a></li>
         <li><a href="dashboard-transloadit/index.html">dashboard-transloadit</a></li>
         <li><a href="dashboard-tus/index.html">dashboard-tus</a></li>
+        <li><a href="dashboard-xhr/index.html">dashboard-xhr</a></li>
 				<li><a href="dashboard-ui/index.html">dashboard-ui</a></li>
 				<li><a href="dashboard-vue/index.html">dashboard-vue</a></li>
 			</ul>

diff --git a/e2e/cypress/integration/dashboard-tus.spec.ts b/e2e/cypress/integration/dashboard-tus.spec.ts
@@ -1,5 +1,7 @@
 import type BaseTus from '@uppy/tus'
 
+import { interceptCompanionUrlRequest, interceptCompanionUnsplashRequest, runRemoteUrlImageUploadTest, runRemoteUnsplashUploadTest } from './reusable-tests'
+
 type Tus = BaseTus & {
   requests: { isPaused: boolean }
 }
@@ -12,8 +14,8 @@ describe('Dashboard with Tus', () => {
     cy.visit('/dashboard-tus')
     cy.get('.uppy-Dashboard-input:first').as('file-input')
     cy.intercept('/files/*').as('tus')
-    cy.intercept('http://localhost:3020/url/*').as('url')
-    cy.intercept('http://localhost:3020/search/unsplash/*').as('unsplash')
+    interceptCompanionUrlRequest()
+    interceptCompanionUnsplashRequest()
   })
 
   it('should upload cat image successfully', () => {
@@ -53,33 +55,10 @@ describe('Dashboard with Tus', () => {
   )
 
   it('should upload remote image with URL plugin', () => {
-    cy.get('[data-cy="Url"]').click()
-    cy.get('.uppy-Url-input').type('https://raw.githubusercontent.com/transloadit/uppy/main/e2e/cypress/fixtures/images/cat.jpg')
-    cy.get('.uppy-Url-importButton').click()
-    cy.get('.uppy-StatusBar-actionBtn--upload').click()
-    cy.wait('@url')
-    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+    runRemoteUrlImageUploadTest()
   })
 
   it('should upload remote image with Unsplash plugin', () => {
-    cy.get('[data-cy="Unsplash"]').click()
-    cy.get('.uppy-SearchProvider-input').type('book')
-    cy.get('.uppy-SearchProvider-searchButton').click()
-    cy.wait('@unsplash')
-    // Test that the author link is visible
-    cy.get('.uppy-ProviderBrowserItem')
-      .first()
-      .within(() => {
-        cy.root().click()
-        // We have hover states that show the author
-        // but we don't have hover in e2e, so we focus after the click
-        // to get the same effect. Also tests keyboard users this way.
-        cy.get('input[type="checkbox"]').focus()
-        cy.get('a').should('have.css', 'display', 'block')
-      })
-    cy.get('.uppy-c-btn-primary').click()
-    cy.get('.uppy-StatusBar-actionBtn--upload').click()
-    cy.wait('@unsplash')
-    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+    runRemoteUnsplashUploadTest()
   })
 })
diff --git a/e2e/cypress/integration/dashboard-xhr.spec.ts b/e2e/cypress/integration/dashboard-xhr.spec.ts
@@ -0,0 +1,17 @@
+import { interceptCompanionUrlRequest, interceptCompanionUnsplashRequest, runRemoteUrlImageUploadTest, runRemoteUnsplashUploadTest } from './reusable-tests'
+
+describe('Dashboard with XHR', () => {
+  beforeEach(() => {
+    cy.visit('/dashboard-xhr')
+    interceptCompanionUrlRequest()
+    interceptCompanionUnsplashRequest()
+  })
+
+  it('should upload remote image with URL plugin', () => {
+    runRemoteUrlImageUploadTest()
+  })
+
+  it('should upload remote image with Unsplash plugin', () => {
+    runRemoteUnsplashUploadTest()
+  })
+})
diff --git a/e2e/cypress/integration/reusable-tests.ts b/e2e/cypress/integration/reusable-tests.ts
@@ -0,0 +1,35 @@
+/* global cy */
+
+export const interceptCompanionUrlRequest = () => cy.intercept('http://localhost:3020/url/*').as('url')
+export const interceptCompanionUnsplashRequest = () => cy.intercept('http://localhost:3020/search/unsplash/*').as('unsplash')
+
+export function runRemoteUrlImageUploadTest () {
+  cy.get('[data-cy="Url"]').click()
+  cy.get('.uppy-Url-input').type('https://raw.githubusercontent.com/transloadit/uppy/main/e2e/cypress/fixtures/images/cat.jpg')
+  cy.get('.uppy-Url-importButton').click()
+  cy.get('.uppy-StatusBar-actionBtn--upload').click()
+  cy.wait('@url')
+  cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+}
+
+export function runRemoteUnsplashUploadTest () {
+  cy.get('[data-cy="Unsplash"]').click()
+  cy.get('.uppy-SearchProvider-input').type('book')
+  cy.get('.uppy-SearchProvider-searchButton').click()
+  cy.wait('@unsplash')
+  // Test that the author link is visible
+  cy.get('.uppy-ProviderBrowserItem')
+    .first()
+    .within(() => {
+      cy.root().click()
+      // We have hover states that show the author
+      // but we don't have hover in e2e, so we focus after the click
+      // to get the same effect. Also tests keyboard users this way.
+      cy.get('input[type="checkbox"]').focus()
+      cy.get('a').should('have.css', 'display', 'block')
+    })
+  cy.get('.uppy-c-btn-primary').click()
+  cy.get('.uppy-StatusBar-actionBtn--upload').click()
+  cy.wait('@unsplash')
+  cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+}
diff --git a/packages/@uppy/aws-s3/src/MiniXHRUpload.js b/packages/@uppy/aws-s3/src/MiniXHRUpload.js
@@ -263,6 +263,7 @@ export default class MiniXHRUpload {
 
     const res = await client.post(file.remote.url, {
       ...file.remote.body,
+      protocol: 'multipart',
       endpoint: opts.endpoint,
       size: file.data.size,
       fieldname: opts.fieldName,

diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js
@@ -92,8 +92,9 @@ function validateOptions (options) {
   }
 
   // validate protocol
-  if (options.protocol == null || !Object.keys(PROTOCOLS).some((key) => PROTOCOLS[key] === options.protocol)) {
-    throw new ValidationError('please specify a valid protocol')
+  // @todo this validation should not be conditional once the protocol field is mandatory
+  if (options.protocol && !Object.keys(PROTOCOLS).some((key) => PROTOCOLS[key] === options.protocol)) {
+    throw new ValidationError('unsupported protocol specified')
   }
 
   // s3 uploads don't require upload destination
@@ -206,7 +207,9 @@ class Uploader {
   }
 
   async _uploadByProtocol () {
-    const { protocol } = this.options
+    // todo a default protocol should not be set. We should ensure that the user specifies their protocol.
+    // after we drop old versions of uppy client we can remove this
+    const protocol = this.options.protocol || PROTOCOLS.multipart
 
     switch (protocol) {
       case PROTOCOLS.multipart:

diff --git a/packages/@uppy/companion/test/__tests__/companion.js b/packages/@uppy/companion/test/__tests__/companion.js
@@ -36,7 +36,7 @@ describe('validate upload data', () => {
         protocol: 'tusInvalid',
       })
       .expect(400)
-      .then((res) => expect(res.body.message).toBe('please specify a valid protocol'))
+      .then((res) => expect(res.body.message).toBe('unsupported protocol specified'))
   })
 
   test('invalid upload fieldname gets rejected', () => {

diff --git a/packages/@uppy/companion/test/__tests__/uploader.js b/packages/@uppy/companion/test/__tests__/uploader.js
@@ -20,13 +20,10 @@ process.env.COMPANION_DATADIR = './test/output'
 process.env.COMPANION_DOMAIN = 'localhost:3020'
 const { companionOptions } = standalone()
 
-const protocol = 'tus'
-
 describe('uploader with tus protocol', () => {
   test('uploader respects uploadUrls', async () => {
     const opts = {
       endpoint: 'http://localhost/files',
-      protocol,
       companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/url.myendpoint.com\//] },
     }
 
@@ -36,7 +33,6 @@ describe('uploader with tus protocol', () => {
   test('uploader respects uploadUrls, valid', async () => {
     const opts = {
       endpoint: 'http://url.myendpoint.com/files',
-      protocol,
       companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/url.myendpoint.com\//] },
     }
 
@@ -47,7 +43,6 @@ describe('uploader with tus protocol', () => {
   test('uploader respects uploadUrls, localhost', async () => {
     const opts = {
       endpoint: 'http://localhost:1337/',
-      protocol,
       companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/localhost:1337\//] },
     }
 
@@ -231,7 +226,6 @@ describe('uploader with tus protocol', () => {
     const opts = {
       companionOptions,
       endpoint: 'http://localhost',
-      protocol,
     }
 
     // eslint-disable-next-line no-new
@@ -253,7 +247,6 @@ describe('uploader with tus protocol', () => {
   test('uploader respects maxFileSize correctly', async () => {
     const opts = {
       endpoint: 'http://url.myendpoint.com/files',
-      protocol,
       companionOptions: { ...companionOptions, maxFileSize: 100 },
       size: 99,
     }

diff --git a/packages/@uppy/xhr-upload/src/index.js b/packages/@uppy/xhr-upload/src/index.js
@@ -366,6 +366,7 @@ export default class XHRUpload extends BasePlugin {
       const client = new Client(this.uppy, file.remote.providerOptions)
       client.post(file.remote.url, {
         ...file.remote.body,
+        protocol: 'multipart',
         endpoint: opts.endpoint,
         size: file.data.size,
         fieldname: opts.fieldName,