Merge pull request #19400 from overleaf/dp-duplicate-file-folder-name

Improvements to handling of file/folder upload conflicts

GitOrigin-RevId: 526edf30dfbaec7ee1e03ffd156365f09be25e86

services/web/app/src/Features/Errors/ErrorController.js

@@ -65,6 +65,12 @@ async function handleError(error, req, res, next) {
       res.status(400)
       plainTextResponse(res, error.message)
     }
+  } else if (error instanceof Errors.DuplicateNameError) {
+    req.logger.setLevel('warn')
+    if (shouldSendErrorResponse) {
+      res.status(400)
+      plainTextResponse(res, error.message)
+    }
   } else if (error instanceof Errors.InvalidNameError) {
     req.logger.setLevel('warn')
     if (shouldSendErrorResponse) {

services/web/app/src/Features/Errors/Errors.js

@@ -38,6 +38,8 @@ class ServiceNotConfiguredError extends BackwardCompatibleError {}
 
 class TooManyRequestsError extends BackwardCompatibleError {}
 
+class DuplicateNameError extends OError {}
+
 class InvalidNameError extends BackwardCompatibleError {}
 
 class UnsupportedFileTypeError extends BackwardCompatibleError {}
@@ -270,6 +272,7 @@ module.exports = {
   ForbiddenError,
   ServiceNotConfiguredError,
   TooManyRequestsError,
+  DuplicateNameError,
   InvalidNameError,
   UnsupportedFileTypeError,
   FileTooLargeError,

services/web/app/src/Features/History/RestoreManager.js

@@ -222,8 +222,8 @@ const RestoreManager = {
     try {
       return await addEntityWithName(basename)
     } catch (error) {
-      if (error instanceof Errors.InvalidNameError) {
+      if (error instanceof Errors.DuplicateNameError) {
-        // likely a duplicate name, so try with a prefix
+        // Duplicate name, so try with a prefix
         const date = moment(new Date()).format('Do MMM YY H:mm:ss')
         // Move extension to the end so the file type is preserved
         const extension = Path.extname(basename)

services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js

@@ -618,7 +618,7 @@ function _checkValidElementName(folder, name) {
     .concat(folder.folders || [])
   for (const element of elements) {
     if (element.name === name) {
-      throw new Errors.InvalidNameError('file already exists')
+      throw new Errors.DuplicateNameError('file already exists')
     }
   }
 }

services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js

@@ -146,7 +146,10 @@ async function updateProjectContents(req, res, next) {
   try {
     await UpdateMerger.promises.mergeUpdate(null, projectId, path, req, source)
   } catch (error) {
-    if (error.constructor === Errors.InvalidNameError) {
+    if (
+      error instanceof Errors.InvalidNameError ||
+      error instanceof Errors.DuplicateNameError
+    ) {
       return res.sendStatus(422)
     } else {
       throw error

services/web/app/src/Features/Uploads/ProjectUploadController.js

@@ -12,6 +12,7 @@ const { InvalidZipFileError } = require('./ArchiveErrors')
 const multer = require('multer')
 const { defaultsDeep } = require('lodash')
 const { expressify } = require('@overleaf/promise-utils')
+const { DuplicateNameError } = require('../Errors/Errors')
 
 const upload = multer(
   defaultsDeep(
@@ -107,6 +108,11 @@ async function uploadFile(req, res, next) {
             success: false,
             error: 'invalid_filename',
           })
+        } else if (error instanceof DuplicateNameError) {
+          return res.status(422).json({
+            success: false,
+            error: 'duplicate_file_name',
+          })
         } else if (error.message === 'project_has_too_many_files') {
           return res.status(422).json({
             success: false,

services/web/frontend/extracted-translations.json

@@ -428,6 +428,7 @@
   "file_name_figure_modal": "",
   "file_name_in_this_project": "",
   "file_name_in_this_project_figure_modal": "",
+  "file_or_folder_name_already_exists": "",
   "file_outline": "",
   "file_size": "",
   "files_cannot_include_invalid_characters": "",
@@ -1385,6 +1386,7 @@
   "thanks_for_subscribing_you_help_sl": "",
   "thanks_settings_updated": "",
   "the_following_files_already_exist_in_this_project": "",
+  "the_following_files_and_folders_already_exist_in_this_project": "",
   "the_following_folder_already_exists_in_this_project": "",
   "the_following_folder_already_exists_in_this_project_plural": "",
   "the_original_text_has_changed": "",

services/web/frontend/js/features/file-tree/components/file-tree-create/error-message.jsx

@@ -42,6 +42,13 @@ export default function ErrorMessage({ error }) {
           </DangerMessage>
         )
 
+      case 'duplicate_file_name':
+        return (
+          <DangerMessage>
+            {t('file_or_folder_name_already_exists')}
+          </DangerMessage>
+        )
+
       case 'rate-limit-hit':
         return (
           <DangerMessage>

services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-upload-conflicts.tsx

@@ -4,90 +4,110 @@ import { useProjectContext } from '@/shared/context/project-context'
 import { useCallback } from 'react'
 import { syncDelete } from '@/features/file-tree/util/sync-mutation'
 import { Button } from 'react-bootstrap'
+import { TFunction } from 'i18next'
 
-export function UploadConflicts({
+export type Conflict = {
+  entity: FileTreeEntity
+  type: 'file' | 'folder'
+}
+
+const getConflictText = (conflicts: Conflict[], t: TFunction) => {
+  const hasFolderConflict = conflicts.some(
+    conflict => conflict.type === 'folder'
+  )
+
+  const hasFileConflict = conflicts.some(conflict => conflict.type === 'file')
+
+  if (hasFolderConflict && hasFileConflict) {
+    return t('the_following_files_and_folders_already_exist_in_this_project')
+  }
+
+  if (hasFolderConflict) {
+    return t('the_following_folder_already_exists_in_this_project', {
+      count: conflicts.length,
+    })
+  }
+
+  return t('the_following_files_already_exist_in_this_project')
+}
+
+export function FileUploadConflicts({
   cancel,
   conflicts,
-  folderConflicts,
   handleOverwrite,
-  setError,
 }: {
   cancel: () => void
-  conflicts: FileTreeEntity[]
+  conflicts: Conflict[]
-  folderConflicts: FileTreeEntity[]
   handleOverwrite: () => void
-  setError: (error: string) => void
 }) {
   const { t } = useTranslation()
 
-  // ensure that no uploads happen while there are folder conflicts
+  // Don't allow overwriting folders with files
-  if (folderConflicts.length > 0) {
+  const hasFolderConflict = conflicts.some(
-    return (
+    conflict => conflict.type === 'folder'
-      <FolderUploadConflicts
-        cancel={cancel}
-        folderConflicts={folderConflicts}
-        handleOverwrite={handleOverwrite}
-        setError={setError}
-      />
   )
-  }
 
   return (
     <div className="small modal-new-file--body-conflict">
       {conflicts.length > 0 && (
         <>
-          <p className="text-center mb-0">
+          <p className="text-center mb-0">{getConflictText(conflicts, t)}</p>
-            {t('the_following_files_already_exist_in_this_project')}
-          </p>
 
           <ul className="text-center list-unstyled row-spaced-small mt-1">
             {conflicts.map((conflict, index) => (
               <li key={index}>
-                <strong>{conflict.name}</strong>
+                <strong>{conflict.entity.name}</strong>
               </li>
             ))}
           </ul>
         </>
       )}
 
+      {!hasFolderConflict && (
         <p className="text-center row-spaced-small">
           {t('do_you_want_to_overwrite_them')}
         </p>
+      )}
 
       <p className="text-center">
         <Button bsStyle={null} className="btn-secondary" onClick={cancel}>
           {t('cancel')}
         </Button>
         &nbsp;
+        {!hasFolderConflict && (
           <Button bsStyle="danger" onClick={handleOverwrite}>
             {t('overwrite')}
           </Button>
+        )}
       </p>
     </div>
   )
 }
 
-function FolderUploadConflicts({
+export function FolderUploadConflicts({
   cancel,
   handleOverwrite,
-  folderConflicts,
+  conflicts,
   setError,
 }: {
   cancel: () => void
   handleOverwrite: () => void
-  folderConflicts: FileTreeEntity[]
+  conflicts: Conflict[]
   setError: (error: string) => void
 }) {
   const { t } = useTranslation()
   const { _id: projectId } = useProjectContext()
 
+  // Don't allow overwriting files with a folder
+  const hasFileConflict = conflicts.some(conflict => conflict.type === 'file')
+
   const deleteAndRetry = useCallback(async () => {
     // TODO: confirm deletion?
 
     try {
       await Promise.all(
-        folderConflicts.map(
+        conflicts.map(conflict =>
-          entity => syncDelete(projectId, 'folder', entity._id) // TODO: might be a file!
+          syncDelete(projectId, 'folder', conflict.entity._id)
         )
       )
 
@@ -95,40 +115,40 @@ function FolderUploadConflicts({
     } catch (error: any) {
       setError(error.message)
     }
-  }, [setError, folderConflicts, handleOverwrite, projectId])
+  }, [setError, conflicts, handleOverwrite, projectId])
 
   return (
     <div className="small modal-new-file--body-conflict">
-      <p className="text-center mb-0">
+      <p className="text-center mb-0">{getConflictText(conflicts, t)}</p>
-        {t('the_following_folder_already_exists_in_this_project', {
-          count: folderConflicts.length,
-        })}
-      </p>
 
       <ul className="text-center list-unstyled row-spaced-small mt-1">
-        {folderConflicts.map((entity, index) => (
+        {conflicts.map((conflict, index) => (
           <li key={index}>
-            <strong>{entity.name}</strong>
+            <strong>{conflict.entity.name}</strong>
           </li>
         ))}
       </ul>
 
+      {!hasFileConflict && (
         <p className="text-center row-spaced-small">
           {t('overwriting_the_original_folder')}
           <br />
           {t('do_you_want_to_overwrite_it', {
-          count: folderConflicts.length,
+            count: conflicts.length,
           })}
         </p>
+      )}
 
       <p className="text-center">
         <Button bsStyle={null} className="btn-secondary" onClick={cancel}>
           {t('cancel')}
         </Button>
         &nbsp;
+        {!hasFileConflict && (
           <Button bsStyle="danger" onClick={deleteAndRetry}>
             {t('overwrite')}
           </Button>
+        )}
       </p>
     </div>
   )

services/web/frontend/js/features/file-tree/components/file-tree-create/modes/file-tree-upload-doc.tsx

@@ -12,10 +12,16 @@ import { refreshProjectMetadata } from '../../../util/api'
 import ErrorMessage from '../error-message'
 import { debugConsole } from '@/utils/debugging'
 import { isAcceptableFile } from '@/features/file-tree/util/is-acceptable-file'
-import { findByNameInFolder } from '@/features/file-tree/util/is-name-unique-in-folder'
+import {
+  findFileByNameInFolder,
+  findFolderByNameInFolder,
+} from '@/features/file-tree/util/is-name-unique-in-folder'
 import { useFileTreeData } from '@/shared/context/file-tree-data-context'
-import { FileTreeEntity } from '../../../../../../../types/file-tree-entity'
+import {
-import { UploadConflicts } from '@/features/file-tree/components/file-tree-create/file-tree-upload-conflicts'
+  Conflict,
+  FileUploadConflicts,
+  FolderUploadConflicts,
+} from '@/features/file-tree/components/file-tree-create/file-tree-upload-conflicts'
 import getMeta from '@/utils/meta'
 
 export default function FileTreeUploadDoc() {
@@ -26,8 +32,8 @@ export default function FileTreeUploadDoc() {
 
   const [error, setError] = useState<string>()
 
-  const [conflicts, setConflicts] = useState<FileTreeEntity[]>([])
+  const [conflicts, setConflicts] = useState<Conflict[]>([])
-  const [folderConflicts, setFolderConflicts] = useState<FileTreeEntity[]>([])
+  const [folderConflicts, setFolderConflicts] = useState<Conflict[]>([])
   const [overwrite, setOverwrite] = useState(false)
 
   const maxNumberOfFiles = 180
@@ -35,46 +41,79 @@ export default function FileTreeUploadDoc() {
 
   // calculate conflicts
   const buildConflicts = (files: Record<string, any>) => {
-    const conflicts = new Set<FileTreeEntity>()
+    const conflicts: Conflict[] = []
 
     for (const file of Object.values(files)) {
       const { name, relativePath } = file.meta
 
       if (!relativePath) {
         const targetFolderId = file.meta.targetFolderId ?? parentFolderId
-        const duplicate = findByNameInFolder(fileTreeData, targetFolderId, name)
+        const duplicateFile = findFileByNameInFolder(
-        if (duplicate) {
+          fileTreeData,
-          conflicts.add(duplicate)
+          targetFolderId,
+          name
+        )
+        if (duplicateFile) {
+          conflicts.push({
+            entity: duplicateFile,
+            type: 'file',
+          })
+        }
+
+        const duplicateFolder = findFolderByNameInFolder(
+          fileTreeData,
+          targetFolderId,
+          name
+        )
+        if (duplicateFolder) {
+          conflicts.push({
+            entity: duplicateFolder,
+            type: 'folder',
+          })
         }
       }
     }
 
-    return [...conflicts]
+    return conflicts
   }
 
   const buildFolderConflicts = (files: Record<string, any>) => {
-    const conflicts = new Set<FileTreeEntity>()
+    const conflicts: Conflict[] = []
 
     for (const file of Object.values(files)) {
       const { relativePath } = file.meta
 
       if (relativePath) {
         const [rootName] = relativePath.replace(/^\//, '').split('/')
-        if (!conflicts.has(rootName)) {
+
         const targetFolderId = file.meta.targetFolderId ?? parentFolderId
-          const duplicateEntity = findByNameInFolder(
+        const duplicateFile = findFileByNameInFolder(
           fileTreeData,
           targetFolderId,
           rootName
         )
-          if (duplicateEntity) {
+        if (duplicateFile) {
-            conflicts.add(duplicateEntity)
+          conflicts.push({
+            entity: duplicateFile,
+            type: 'file',
+          })
         }
+
+        const duplicateFolder = findFolderByNameInFolder(
+          fileTreeData,
+          targetFolderId,
+          rootName
+        )
+        if (duplicateFolder) {
+          conflicts.push({
+            entity: duplicateFolder,
+            type: 'folder',
+          })
         }
       }
     }
 
-    return [...conflicts]
+    return conflicts
   }
 
   const buildEndpoint = (projectId: string, targetFolderId: string) => {
@@ -214,25 +253,32 @@ export default function FileTreeUploadDoc() {
     uppy.upload()
   }, [uppy])
 
-  // whether to show a message about conflicting files
+  const showFolderUploadConflicts = !overwrite && folderConflicts.length > 0
-  const showConflicts =
+  const showFileUploadConfilcts =
-    !overwrite && (conflicts.length > 0 || folderConflicts.length > 0)
+    !overwrite && !showFolderUploadConflicts && conflicts.length > 0
+  const showDashboard = !showFileUploadConfilcts && !showFolderUploadConflicts
 
   return (
     <>
       {error && (
         <UploadErrorMessage error={error} maxNumberOfFiles={maxNumberOfFiles} />
       )}
-
+      {showFolderUploadConflicts && (
-      {showConflicts ? (
+        <FolderUploadConflicts
-        <UploadConflicts
           cancel={cancel}
-          conflicts={conflicts}
+          conflicts={folderConflicts}
-          folderConflicts={folderConflicts}
           handleOverwrite={handleOverwrite}
           setError={setError}
         />
-      ) : (
+      )}
+      {showFileUploadConfilcts && (
+        <FileUploadConflicts
+          cancel={cancel}
+          conflicts={conflicts}
+          handleOverwrite={handleOverwrite}
+        />
+      )}
+      {showDashboard && (
         <Dashboard
           uppy={uppy}
           showProgressDetails

services/web/frontend/js/features/file-tree/util/is-name-unique-in-folder.ts

@@ -8,21 +8,35 @@ export function isNameUniqueInFolder(
   parentFolderId: string,
   name: string
 ): boolean {
-  return !findByNameInFolder(tree, parentFolderId, name)
+  return !(
+    findFileByNameInFolder(tree, parentFolderId, name) ||
+    findFolderByNameInFolder(tree, parentFolderId, name)
+  )
 }
 
-export function findByNameInFolder(
+export function findFileByNameInFolder(
   tree: Folder,
   parentFolderId: string,
   name: string
-): Doc | FileRef | Folder | undefined {
+): Doc | FileRef | undefined {
   if (tree._id !== parentFolderId) {
     tree = findInTree(tree, parentFolderId).entity
   }
 
   return (
     tree.docs.find(entity => entity.name === name) ||
-    tree.fileRefs.find(entity => entity.name === name) ||
+    tree.fileRefs.find(entity => entity.name === name)
-    tree.folders.find(entity => entity.name === name)
   )
 }
+
+export function findFolderByNameInFolder(
+  tree: Folder,
+  parentFolderId: string,
+  name: string
+): Folder | undefined {
+  if (tree._id !== parentFolderId) {
+    tree = findInTree(tree, parentFolderId).entity
+  }
+
+  return tree.folders.find(entity => entity.name === name)
+}

services/web/locales/en.json

@@ -634,6 +634,7 @@
   "file_name_figure_modal": "File name",
   "file_name_in_this_project": "File Name In This Project",
   "file_name_in_this_project_figure_modal": "File name in this project",
+  "file_or_folder_name_already_exists": "A file or folder with this name already exists",
   "file_outline": "File outline",
   "file_size": "File size",
   "file_too_large": "File too large",
@@ -1990,6 +1991,7 @@
   "thanks_settings_updated": "Thanks, your settings have been updated.",
   "the_file_supplied_is_of_an_unsupported_type ": "The link to open this content on Overleaf pointed to the wrong kind of file. Valid file types are .tex documents and .zip files. If this keeps happening for links on a particular site, please report this to them.",
   "the_following_files_already_exist_in_this_project": "The following files already exist in this project:",
+  "the_following_files_and_folders_already_exist_in_this_project": "The following files and folders already exist in this project:",
   "the_following_folder_already_exists_in_this_project": "The following folder already exists in this project:",
   "the_following_folder_already_exists_in_this_project_plural": "The following folders already exist in this project:",
   "the_original_text_has_changed": "The original text has changed, so this suggestion can’t be applied",

services/web/test/unit/src/History/RestoreManagerTests.js

@@ -180,9 +180,9 @@ describe('RestoreManager', function () {
       })
     })
 
-    describe('with an invalid name', function () {
+    describe('with a duplicate name', function () {
       beforeEach(async function () {
-        this.addEntityWithName.rejects(new Errors.InvalidNameError())
+        this.addEntityWithName.rejects(new Errors.DuplicateNameError())
         this.addEntityWithName
           .onSecondCall()
           .resolves((this.entity = 'mock-entity'))

services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js

@@ -856,7 +856,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
             'doc',
             this.folder.name
           )
-        ).to.be.rejectedWith(Errors.InvalidNameError)
+        ).to.be.rejectedWith(Errors.DuplicateNameError)
       })
     })
   })
@@ -990,7 +990,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
                 file,
                 'file'
               )
-            ).to.be.rejectedWith(Errors.InvalidNameError)
+            ).to.be.rejectedWith(Errors.DuplicateNameError)
           })
         })
       })