@uppy/transloadit: cancel assemblies when all its files have been removed (#3893)

When starting an upload and removing it right away, it creates a race
condition when the assembly is created with no files and is never
closed. This commit ensures that the assembly is closed if all files
have been removed, or updated with the new expected number of
files otherwise.

Co-authored-by: Aaron Heimlich <aaron.heimlich@gmail.com>

e2e/cypress/integration/dashboard-transloadit.spec.ts

@@ -44,6 +44,58 @@ describe('Dashboard with Transloadit', () => {
     })
   })
 
+  it('should not create assembly when all individual files have been cancelled', () => {
+    cy.get('@file-input').attachFile(['images/cat.jpg', 'images/traffic.jpg'])
+    cy.get('.uppy-StatusBar-actionBtn--upload').click()
+
+    cy.window().then(({ uppy }) => {
+      expect(Object.values(uppy.getPlugin('Transloadit').activeAssemblies).length).to.equal(0)
+
+      const { files } = uppy.getState()
+      uppy.removeFiles(Object.keys(files))
+
+      cy.wait('@createAssemblies').then(() => {
+        expect(Object.values(uppy.getPlugin('Transloadit').activeAssemblies).some((a: any) => a.pollInterval)).to.equal(false)
+      })
+    })
+  })
+
+  // Not working, the upstream changes have not landed yet.
+  it.skip('should create assembly if there is still one file to upload', () => {
+    cy.get('@file-input').attachFile(['images/cat.jpg', 'images/traffic.jpg'])
+    cy.get('.uppy-StatusBar-actionBtn--upload').click()
+
+    cy.window().then(({ uppy }) => {
+      expect(Object.values(uppy.getPlugin('Transloadit').activeAssemblies).length).to.equal(0)
+
+      const { files } = uppy.getState()
+      const [fileID] = Object.keys(files)
+      uppy.removeFile(fileID)
+
+      cy.wait('@createAssemblies').then(() => {
+        cy.wait('@resumable')
+        cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+      })
+    })
+  })
+
+  // Not working, the upstream changes have not landed yet.
+  it.skip('should complete upload if one gets cancelled mid-flight', () => {
+    cy.get('@file-input').attachFile(['images/cat.jpg', 'images/traffic.jpg'])
+    cy.get('.uppy-StatusBar-actionBtn--upload').click()
+
+    cy.wait('@createAssemblies')
+    cy.wait('@resumable')
+
+    cy.window().then(({ uppy }) => {
+      const { files } = uppy.getState()
+      const [fileID] = Object.keys(files)
+      uppy.removeFile(fileID)
+
+      cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+    })
+  })
+
   it('should not emit error if upload is cancelled right away', () => {
     cy.get('@file-input').selectFile('cypress/fixtures/images/cat.jpg', { force:true })
     cy.get('.uppy-StatusBar-actionBtn--upload').click()

packages/@uppy/core/src/Uppy.js

@@ -677,6 +677,12 @@ class Uppy {
         return
       }
 
+      const { capabilities } = this.getState()
+      if (newFileIDs.length !== currentUploads[uploadID].fileIDs.length
+          && !capabilities.individualCancellation) {
+        throw new Error('individualCancellation is disabled')
+      }
+
       updatedUploads[uploadID] = {
         ...currentUploads[uploadID],
         fileIDs: newFileIDs,

packages/@uppy/core/src/Uppy.test.js

@@ -1,6 +1,7 @@
 /* eslint no-console: "off", no-restricted-syntax: "off" */
 import { afterEach, beforeEach, describe, expect, it, jest, xit } from '@jest/globals'
 
+import assert from 'node:assert'
 import fs from 'node:fs'
 import prettierBytes from '@transloadit/prettier-bytes'
 import Core from '../lib/index.js'
@@ -263,6 +264,117 @@ describe('src/Core', () => {
     expect(Object.keys(core.getState().files).length).toEqual(0)
   })
 
+  it('should allow remove all uploads when individualCancellation is disabled', () => {
+    const core = new Core()
+
+    const { capabilities } = core.getState()
+    core.setState({
+      capabilities: {
+        ...capabilities,
+        individualCancellation: false,
+      },
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo1.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo2.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    const fileIDs = Object.keys(core.getState().files)
+    const id = core[Symbol.for('uppy test: createUpload')](fileIDs)
+
+    expect(core.getState().currentUploads[id]).toBeDefined()
+    expect(Object.keys(core.getState().files).length).toEqual(2)
+
+    core.removeFiles(fileIDs)
+
+    expect(core.getState().currentUploads[id]).toBeUndefined()
+    expect(Object.keys(core.getState().files).length).toEqual(0)
+  })
+
+  it('should disallow remove one upload when individualCancellation is disabled', () => {
+    const core = new Core()
+
+    const { capabilities } = core.getState()
+    core.setState({
+      capabilities: {
+        ...capabilities,
+        individualCancellation: false,
+      },
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo1.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo2.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    const fileIDs = Object.keys(core.getState().files)
+    const id = core[Symbol.for('uppy test: createUpload')](fileIDs)
+
+    expect(core.getState().currentUploads[id]).toBeDefined()
+    expect(Object.keys(core.getState().files).length).toEqual(2)
+
+    assert.throws(() => core.removeFile(fileIDs[0]), /individualCancellation is disabled/)
+
+    expect(core.getState().currentUploads[id]).toBeDefined()
+    expect(Object.keys(core.getState().files).length).toEqual(2)
+  })
+
+  it('should allow remove one upload when individualCancellation is enabled', () => {
+    const core = new Core()
+
+    const { capabilities } = core.getState()
+    core.setState({
+      capabilities: {
+        ...capabilities,
+        individualCancellation: true,
+      },
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo1.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    core.addFile({
+      source: 'jest',
+      name: 'foo2.jpg',
+      type: 'image/jpeg',
+      data: new File([sampleImage], { type: 'image/jpeg' }),
+    })
+
+    const fileIDs = Object.keys(core.getState().files)
+    const id = core[Symbol.for('uppy test: createUpload')](fileIDs)
+
+    expect(core.getState().currentUploads[id]).toBeDefined()
+    expect(Object.keys(core.getState().files).length).toEqual(2)
+
+    core.removeFile(fileIDs[0])
+
+    expect(core.getState().currentUploads[id]).toBeDefined()
+    expect(Object.keys(core.getState().files).length).toEqual(1)
+  })
+
   it('should close, reset and uninstall when the close method is called', () => {
     // use DeepFrozenStore in some tests to make sure we are not mutating things
     const core = new Core({

packages/@uppy/transloadit/src/Client.js

@@ -20,6 +20,11 @@ export default class Client {
     this.#fetchWithNetworkError = this.opts.rateLimitedQueue.wrapPromiseFunction(fetchWithNetworkError)
   }
 
+  /**
+   * @param  {RequestInfo | URL} input
+   * @param  {RequestInit} init
+   * @returns {Promise<any>}
+   */
   #fetchJSON (...args) {
     return this.#fetchWithNetworkError(...args).then(response => {
       if (response.status === 429) {
@@ -126,6 +131,25 @@ export default class Client {
       .catch((err) => this.#reportError(err, { assembly, file, url, type: 'API_ERROR' }))
   }
 
+  /**
+   * Update the number of expected files in an already created assembly.
+   *
+   * @param {object} assembly
+   * @param {number} num_expected_upload_files
+   */
+  updateNumberOfFilesInAssembly (assembly, num_expected_upload_files) {
+    const url = new URL(assembly.assembly_ssl_url)
+    url.pathname = '/update_assemblies'
+    const body = JSON.stringify({
+      assembly_updates: [{
+        assembly_id: assembly.assembly_id,
+        num_expected_upload_files,
+      }],
+    })
+    return this.#fetchJSON(url, { method: 'post', headers: this.#headers, body })
+      .catch((err) => this.#reportError(err, { url, type: 'API_ERROR' }))
+  }
+
   /**
    * Cancel a running Assembly.
    *

packages/@uppy/transloadit/src/index.js

@@ -190,7 +190,18 @@ export default class Transloadit extends BasePlugin {
       fields: options.fields,
       expectedFiles: fileIDs.length,
       signature: options.signature,
-    }).then((newAssembly) => {
+    }).then(async (newAssembly) => {
+      const files = this.uppy.getFiles().filter(({ id }) => fileIDs.includes(id))
+      if (files.length !== fileIDs.length) {
+        if (files.length === 0) {
+          // All files have been removed, cancelling.
+          await this.client.cancelAssembly(newAssembly)
+          return null
+        }
+        // At least one file has been removed.
+        await this.client.updateNumberOfFilesInAssembly(newAssembly, files.length)
+      }
+
       const assembly = new Assembly(newAssembly, this.#rateLimitedQueue)
       const { status } = assembly
       const assemblyID = status.assembly_id
@@ -212,7 +223,6 @@ export default class Transloadit extends BasePlugin {
         },
       })
 
-      const files = this.uppy.getFiles() // []
       const updatedFiles = {}
       files.forEach((file) => {
         updatedFiles[file.id] = this.#attachAssemblyMetadata(file, status)
@@ -228,12 +238,18 @@ export default class Transloadit extends BasePlugin {
       const fileRemovedHandler = (fileRemoved, reason) => {
         if (reason === 'cancel-all') {
           assembly.close()
+          this.client.cancelAssembly(newAssembly).catch(() => { /* ignore potential errors */ })
           this.uppy.off(fileRemovedHandler)
         } else if (fileRemoved.id in updatedFiles) {
           delete updatedFiles[fileRemoved.id]
-          if (Object.keys(updatedFiles).length === 0) {
+          const nbOfRemainingFiles = Object.keys(updatedFiles).length
+          if (nbOfRemainingFiles === 0) {
             assembly.close()
+            this.client.cancelAssembly(newAssembly).catch(() => { /* ignore potential errors */ })
             this.uppy.off(fileRemovedHandler)
+          } else {
+            this.client.updateNumberOfFilesInAssembly(newAssembly, nbOfRemainingFiles)
+              .catch(() => { /* ignore potential errors */ })
           }
         }
       }
@@ -633,7 +649,8 @@ export default class Transloadit extends BasePlugin {
 
     return assemblyOptions.build()
       .then((assemblies) => Promise.all(assemblies.map(createAssembly)))
-      .then((createdAssemblies) => {
+      .then((maybeCreatedAssemblies) => {
+        const createdAssemblies = maybeCreatedAssemblies.filter(Boolean)
         const assemblyIDs = createdAssemblies.map(assembly => assembly.status.assembly_id)
         this.#createAssemblyWatcher(assemblyIDs, uploadID)
         return Promise.all(createdAssemblies.map(assembly => this.#connectAssembly(assembly)))