core: Don’t add/overwrite existing files, allow duplicates from different folders (#1767)

* Add file.data.relativePath to the file.id

* Don’t add/overwrite a file if a file with the same id already exsists, issue a warning

* override @uppy/tus fingerprint to uppy’s file.id

* combine cordove/react native handling, don’t log twice

* always use onError for errors in addFile

* use file.meta.relativePath instead of file.data.relativePath

what do you think, @lakesare?

* update tests to include allowing/diallowing duplicates and file.id generation with relativePath

* update tus-js-client

* refactor error handling for addFile and upload into a mutual  _showOrLogErrorAndThrow method

* explain duplicate files and relativePath

* throw TypeError vs Error when allowedFileTypes is not an array

* fix tests

* tweak docs

* Emit restriction-failed for all restriction errors, move it to _showOrLogErrorAndThrow

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

@@ -116,7 +116,7 @@ class Uppy {
     if (this.opts.restrictions.allowedFileTypes &&
         this.opts.restrictions.allowedFileTypes !== null &&
         !Array.isArray(this.opts.restrictions.allowedFileTypes)) {
-      throw new Error(`'restrictions.allowedFileTypes' must be an array`)
+      throw new TypeError(`'restrictions.allowedFileTypes' must be an array`)
     }
 
     // i18n
@@ -423,6 +423,28 @@ class Uppy {
     }
   }
 
+  _showOrLogErrorAndThrow (err, { showInformer = true, file = null } = {}) {
+    const message = typeof err === 'object' ? err.message : err
+    const details = (typeof err === 'object' && err.details) ? err.details : ''
+
+    // Restriction errors should be logged, but not as errors,
+    // as they are expected and shown in the UI.
+    if (err.isRestriction) {
+      this.log(`${message} ${details}`)
+      this.emit('restriction-failed', file, err)
+    } else {
+      this.log(`${message} ${details}`, 'error')
+    }
+
+    // Sometimes informer has to be shown manually by the developer,
+    // for example, in `onBeforeFileAdded`.
+    if (showInformer) {
+      this.info({ message: message, details: details }, 'error', 5000)
+    }
+
+    throw (typeof err === 'object' ? err : new Error(err))
+  }
+
   /**
    * Add a new file to `state.files`. This will run `onBeforeFileAdded`,
    * try to guess file type in a clever way, check file against restrictions,
@@ -434,15 +456,8 @@ class Uppy {
   addFile (file) {
     const { files, allowNewUpload } = this.getState()
 
-    const onError = (msg) => {
-      const err = typeof msg === 'object' ? msg : new Error(msg)
-      this.log(err.message)
-      this.info(err.message, 'error', 5000)
-      throw err
-    }
-
     if (allowNewUpload === false) {
-      onError(new Error('Cannot add new files: already uploading.'))
+      this._showOrLogErrorAndThrow(new RestrictionError('Cannot add new files: already uploading.'), { file })
     }
 
     const fileType = getFileType(file)
@@ -451,8 +466,8 @@ class Uppy {
     const onBeforeFileAddedResult = this.opts.onBeforeFileAdded(file, files)
 
     if (onBeforeFileAddedResult === false) {
-      this.log('Not adding file because onBeforeFileAdded returned false')
-      return
+      // Don’t show UI info for this error, as it should be done by the developer
+      this._showOrLogErrorAndThrow(new RestrictionError('Cannot add the file because onBeforeFileAdded returned false.'), { showInformer: false, file })
     }
 
     if (typeof onBeforeFileAddedResult === 'object' && onBeforeFileAddedResult) {
@@ -472,6 +487,10 @@ class Uppy {
 
     const fileID = generateFileID(file)
 
+    if (files[fileID]) {
+      this._showOrLogErrorAndThrow(new RestrictionError(`Cannot add the duplicate file '${fileName}', it already exists.`), { file })
+    }
+
     const meta = file.meta || {}
     meta.name = fileName
     meta.type = fileType
@@ -502,8 +521,7 @@ class Uppy {
     try {
       this._checkRestrictions(newFile)
     } catch (err) {
-      this.emit('restriction-failed', newFile, err)
-      onError(err)
+      this._showOrLogErrorAndThrow(err, { file: newFile })
     }
 
     this.setState({
@@ -1313,19 +1331,7 @@ class Uppy {
         return this._runUpload(uploadID)
       })
       .catch((err) => {
-        const message = typeof err === 'object' ? err.message : err
-        const details = (typeof err === 'object' && err.details) ? err.details : ''
-
-        if (err.isRestriction) {
-          this.emit('restriction-failed', null, err)
-          this.log(`${message} ${details}`, 'info')
-          this.info({ message: message, details: details }, 'info', 5000)
-        } else {
-          this.log(`${message} ${details}`, 'error')
-          this.info({ message: message, details: details }, 'error', 5000)
-        }
-
-        throw (typeof err === 'object' ? err : new Error(err))
+        this._showOrLogErrorAndThrow(err)
       })
   }
 }

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

@@ -647,6 +647,50 @@ describe('src/Core', () => {
       }
     })
 
+    it('should not allow a dupicate file, a file with the same id', () => {
+      const core = new Core()
+      core.addFile({
+        source: 'jest',
+        name: 'foo.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      expect(() => {
+        core.addFile({
+          source: 'jest',
+          name: 'foo.jpg',
+          type: 'image/jpeg',
+          data: new File([sampleImage], { type: 'image/jpeg' }),
+          meta: {
+            notARelativePath: 'folder/a'
+          }
+        })
+      }).toThrow(
+        'Cannot add the duplicate file \'foo.jpg\', it already exists'
+      )
+      expect(core.getFiles().length).toEqual(1)
+    })
+
+    it('should allow a duplicate file if its relativePath is different, thus the id is different', () => {
+      const core = new Core()
+      core.addFile({
+        source: 'jest',
+        name: 'foo.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      core.addFile({
+        source: 'jest',
+        name: 'foo.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' }),
+        meta: {
+          relativePath: 'folder/a'
+        }
+      })
+      expect(core.getFiles().length).toEqual(2)
+    })
+
     it('should not allow a file if onBeforeFileAdded returned false', () => {
       const core = new Core({
         onBeforeFileAdded: (file, files) => {
@@ -655,12 +699,16 @@ describe('src/Core', () => {
           }
         }
       })
-      core.addFile({
-        source: 'jest',
-        name: 'foo.jpg',
-        type: 'image/jpeg',
-        data: new File([sampleImage], { type: 'image/jpeg' })
-      })
+      expect(() => {
+        core.addFile({
+          source: 'jest',
+          name: 'foo.jpg',
+          type: 'image/jpeg',
+          data: new File([sampleImage], { type: 'image/jpeg' })
+        })
+      }).toThrow(
+        'Cannot add the file because onBeforeFileAdded returned false.'
+      )
       expect(core.getFiles().length).toEqual(0)
     })
 

packages/@uppy/tus/package.json

@@ -24,7 +24,7 @@
   "dependencies": {
     "@uppy/companion-client": "file:../companion-client",
     "@uppy/utils": "file:../utils",
-    "tus-js-client": "^1.8.0-0"
+    "tus-js-client": "^1.8.0-2"
   },
   "peerDependencies": {
     "@uppy/core": "^1.0.0"

packages/@uppy/tus/src/getFingerprint.js

@@ -0,0 +1,39 @@
+const tus = require('tus-js-client')
+
+function isCordova () {
+  return typeof window !== 'undefined' && (
+    typeof window.PhoneGap !== 'undefined' ||
+    typeof window.Cordova !== 'undefined' ||
+    typeof window.cordova !== 'undefined'
+  )
+}
+
+function isReactNative () {
+  return typeof navigator !== 'undefined' &&
+    typeof navigator.product === 'string' &&
+    navigator.product.toLowerCase() === 'reactnative'
+}
+
+// We override tus fingerprint to uppy’s `file.id`, since the `file.id`
+// now also includes `relativePath` for files added from folders.
+// This means you can add 2 identical files, if one is in folder a,
+// the other in folder b — `a/file.jpg` and `b/file.jpg`, when added
+// together with a folder, will be treated as 2 separate files.
+//
+// For React Native and Cordova, we let tus-js-client’s default
+// fingerprint handling take charge.
+module.exports = function getFingerprint (uppyFileObj) {
+  return function (file, options, callback) {
+    if (isCordova() || isReactNative()) {
+      return tus.Upload.defaultOptions.fingerprint(file, options, callback)
+    }
+
+    const uppyFingerprint = [
+      'tus',
+      uppyFileObj.id,
+      options.endpoint
+    ].join('-')
+
+    return callback(null, uppyFingerprint)
+  }
+}

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

@@ -5,6 +5,7 @@ const emitSocketProgress = require('@uppy/utils/lib/emitSocketProgress')
 const getSocketHost = require('@uppy/utils/lib/getSocketHost')
 const settle = require('@uppy/utils/lib/settle')
 const limitPromises = require('@uppy/utils/lib/limitPromises')
+const getFingerprint = require('./getFingerprint')
 
 // Extracted from https://github.com/tus/tus-js-client/blob/master/lib/upload.js#L13
 // excepted we removed 'fingerprint' key to avoid adding more dependencies
@@ -137,6 +138,12 @@ module.exports = class Tus extends Plugin {
         file.tus || {}
       )
 
+      // We override tus fingerprint to uppy’s `file.id`, since the `file.id`
+      // now also includes `relativePath` for files added from folders.
+      // This means you can add 2 identical files, if one is in folder a,
+      // the other in folder b.
+      optsTus.fingerprint = getFingerprint(file)
+
       optsTus.onError = (err) => {
         this.uppy.log(err)
         this.uppy.emit('upload-error', file, err)

packages/@uppy/utils/src/generateFileID.js

@@ -12,6 +12,7 @@ module.exports = function generateFileID (file) {
     'uppy',
     file.name ? encodeFilename(file.name.toLowerCase()) : '',
     file.type,
+    file.meta && file.meta.relativePath ? encodeFilename(file.meta.relativePath.toLowerCase()) : '',
     file.data.size,
     file.data.lastModified
   ].filter(val => val).join('-')

packages/@uppy/utils/src/generateFileID.test.js

@@ -1,7 +1,7 @@
 const generateFileID = require('./generateFileID')
 
 describe('generateFileID', () => {
-  it('should take the filename object and produce a lowercase file id made up of uppy- prefix, file name (cleaned up to be lowercase, letters and numbers only), type, size and lastModified date', () => {
+  it('should take the filename object and produce a lowercase file id made up of uppy- prefix, file name (cleaned up to be lowercase, letters and numbers only), type, relative path (folder) from file.meta.relativePath, size and lastModified date', () => {
     const fileObj = {
       name: 'fOo0Fi@£$.jpg',
       type: 'image/jpeg',
@@ -25,5 +25,19 @@ describe('generateFileID', () => {
     })).toEqual(
       'uppy-/////////p/////////jpg-11k-11m-123-11s-11r-11g-1d-11k-11m-123-11s-11r-11g-122-11l-121-122-1e-image/jpeg-2271173-1498510508000'
     )
+
+    expect(generateFileID({
+      name: 'hello.jpg',
+      type: 'image/jpeg',
+      data: {
+        lastModified: 1498510508000,
+        size: 2271173
+      },
+      meta: {
+        relativePath: 'folder/a'
+      }
+    })).toEqual(
+      'uppy-hello/jpg-1e-image/jpeg-folder/a-1f-2271173-1498510508000'
+    )
   })
 })

website/src/docs/uppy.md

@@ -343,9 +343,11 @@ uppy.addFile({
 
 `addFile` gives an error if the file cannot be added, either because `onBeforeFileAdded(file)` gave an error, or because `uppy.opts.restrictions` checks failed.
 
+If you try to add a file that already exists, `addFile` will throw an error. Unless that duplicate file was dropped with a folder — duplicate files from different folders are allowed, when selected with that folder. This is because we add `file.meta.relativePath` to the `file.id`.
+
 If `uppy.opts.autoProceed === true`, Uppy will begin uploading automatically when files are added.
 
-This function will return the generated id for the file that was added.
+`addFile` will return the generated id for the file that was added.
 
 > Sometimes you might need to add a remote file to Uppy. This can be achieved by [fetching the file, then creating a Blob object, or using the Url plugin with Companion](https://github.com/transloadit/uppy/issues/1006#issuecomment-413495493).
 >